From 5f258d70f3a90ac5eedb1dc59d833e88b53de404 Mon Sep 17 00:00:00 2001 From: Luke-Bilhorn Date: Fri, 24 Apr 2026 01:32:19 -0500 Subject: [PATCH 01/11] Add subdivision-aware milestone pagination resolver Introduces the core subdivision data model, shared TypeScript types (SubdivisionAnchor, MilestoneIndex, etc.), a provider-side subdivisionUtils module, and a subdivision-aware resolver in codexDocument that replaces the flat milestone splitter. Includes a full suite of unit tests covering resolver edge cases. --- .../codexCellEditorProvider/codexDocument.ts | 182 ++++++--- .../utils/subdivisionUtils.ts | 202 ++++++++++ src/test/suite/milestoneSubdivisions.test.ts | 356 ++++++++++++++++++ types/index.d.ts | 82 ++++ 4 files changed, 778 insertions(+), 44 deletions(-) create mode 100644 src/providers/codexCellEditorProvider/utils/subdivisionUtils.ts create mode 100644 src/test/suite/milestoneSubdivisions.test.ts diff --git a/src/providers/codexCellEditorProvider/codexDocument.ts b/src/providers/codexCellEditorProvider/codexDocument.ts index f6fc4674c..893a3559c 100644 --- a/src/providers/codexCellEditorProvider/codexDocument.ts +++ b/src/providers/codexCellEditorProvider/codexDocument.ts @@ -15,6 +15,8 @@ import { MilestoneIndex, MilestoneInfo, CustomCellMetaData, + MilestoneSubdivisionPlacement, + SubdivisionInfo, } from "../../../types"; import { EditMapUtils, deduplicateFileMetadataEdits } from "../../utils/editMapUtils"; import { CodexCellTypes, EditType } from "../../../types/enums"; @@ -25,6 +27,7 @@ import { debounce } from "lodash"; import { getSQLiteIndexManager, isDBShuttingDown } from "../../activationHelpers/contextAware/contentIndexes/indexes/sqliteIndexManager"; import { getCellValueData, cellHasAudioUsingAttachments, computeValidationStats, computeProgressPercents, shouldExcludeCellFromProgress, shouldExcludeQuillCellFromProgress, countActiveValidations, hasTextContent } from "../../../sharedUtils"; import { extractParentCellIdFromParatext, convertCellToQuillContent } from "./utils/cellUtils"; +import { FIRST_SUBDIVISION_KEY, findSubdivisionIndexForRoot, resolveSubdivisions } from "./utils/subdivisionUtils"; import { formatJsonForNotebookFile, normalizeNotebookFileText } from "../../utils/notebookFileFormattingUtils"; import { atomicWriteUriText, readExistingFileOrThrow } from "../../utils/notebookSafeSaveUtils"; @@ -1309,6 +1312,69 @@ export class CodexCellDocument implements vscode.CustomDocument { return false; } + /** + * Returns the ordered list of root content cell IDs within the given index + * range. Root content cells are non-milestone, non-paratext, non-deleted cells + * without a `parentId`. Pagination (both arithmetic and subdivision-based) + * operates over these roots; children and paratext are attached during slicing. + */ + private getRootContentCellIdsInRange( + startCellIndex: number, + endCellIndex: number + ): string[] { + const cells = this._documentData.cells || []; + const rootIds: string[] = []; + for (let i = startCellIndex; i < endCellIndex; i++) { + const cell = cells[i]; + if ( + cell.metadata?.type !== CodexCellTypes.MILESTONE && + cell.metadata?.type !== CodexCellTypes.PARATEXT && + cell.metadata?.data?.deleted !== true + ) { + const parentId = + cell.metadata?.parentId ?? + (cell.metadata?.data as { parentId?: string; } | undefined)?.parentId; + if (!parentId) { + const id = cell.metadata?.id; + if (id) rootIds.push(id); + } + } + } + return rootIds; + } + + /** + * Resolves the subdivisions for a single milestone by index. Reads stored + * placements from the milestone cell's `metadata.data.subdivisions` and + * overrides from `metadata.data.subdivisionNames`. When either is absent the + * arithmetic fallback is used, preserving legacy 50-cell pagination. + * + * `cellIndex` must be the absolute index of the milestone cell within + * `_documentData.cells`; `nextMilestoneCellIndex` is the index of the next + * milestone cell (or `cells.length`) and defines the range end. + */ + private resolveSubdivisionsForMilestoneCell( + cellIndex: number, + nextMilestoneCellIndex: number, + cellsPerPage: number + ): SubdivisionInfo[] { + const cells = this._documentData.cells || []; + const rootContentCellIds = this.getRootContentCellIdsInRange( + cellIndex, + nextMilestoneCellIndex + ); + const milestoneCell = cells[cellIndex]; + const data = milestoneCell?.metadata?.data as + | { subdivisions?: MilestoneSubdivisionPlacement[]; subdivisionNames?: { [key: string]: string; }; } + | undefined; + return resolveSubdivisions({ + rootContentCellIds, + placements: data?.subdivisions, + nameOverrides: data?.subdivisionNames, + cellsPerPage, + }); + } + /** * Builds a milestone index from the document cells. * This index is cached and reused until cells are modified. @@ -1424,13 +1490,23 @@ export class CodexCellDocument implements vscode.CustomDocument { } } + const virtualMilestone: MilestoneInfo = { + index: 0, + cellIndex: 0, + value: "1", + cellCount: totalContentCells, + }; + // Virtual milestone has no backing milestone cell; fall back to + // arithmetic subdivisions across the full cell range. + virtualMilestone.subdivisions = resolveSubdivisions({ + rootContentCellIds: this.getRootContentCellIdsInRange(0, cells.length), + placements: undefined, + nameOverrides: undefined, + cellsPerPage, + }); + const result: MilestoneIndex = { - milestones: [{ - index: 0, - cellIndex: 0, - value: "1", - cellCount: totalContentCells, - }], + milestones: [virtualMilestone], totalCells: totalContentCells, cellsPerPage, }; @@ -1443,6 +1519,19 @@ export class CodexCellDocument implements vscode.CustomDocument { return result; } + // Attach resolved subdivisions to each milestone so slicing APIs and the + // webview share a single source of truth. + for (let i = 0; i < milestones.length; i++) { + const milestone = milestones[i]; + const nextMilestone = milestones[i + 1]; + const endCellIndex = nextMilestone ? nextMilestone.cellIndex : cells.length; + milestone.subdivisions = this.resolveSubdivisionsForMilestoneCell( + milestone.cellIndex, + endCellIndex, + cellsPerPage + ); + } + const result: MilestoneIndex = { milestones, totalCells: totalContentCells, @@ -1617,10 +1706,17 @@ export class CodexCellDocument implements vscode.CustomDocument { const parentId = cell.metadata?.parentId ?? (cell.metadata?.data as { parentId?: string; } | undefined)?.parentId; cellRootIndex = parentId != null ? cellIdToRootIndex.get(parentId) : undefined; } - const subsectionIndex = - cellRootIndex !== undefined - ? Math.max(0, Math.floor(cellRootIndex / cellsPerPage)) - : 0; + // Use resolved subdivisions (custom or arithmetic) to locate the + // right subsection. When no rootIndex could be derived (e.g. the + // located cell is a milestone boundary), fall back to 0. + const subdivisions = milestone.subdivisions ?? []; + let subsectionIndex = 0; + if (cellRootIndex !== undefined && subdivisions.length > 0) { + const found = findSubdivisionIndexForRoot(subdivisions, cellRootIndex); + subsectionIndex = found >= 0 ? found : 0; + } else if (cellRootIndex !== undefined) { + subsectionIndex = Math.max(0, Math.floor(cellRootIndex / cellsPerPage)); + } return { milestoneIndex: i, subsectionIndex }; } @@ -1804,19 +1900,25 @@ export class CodexCellDocument implements vscode.CustomDocument { contentCells.push(quillContent); } - // Use root-based subsections to match getCellsForMilestone pagination + // Use root-based subsections to match getCellsForMilestone pagination. + // When the milestone has user-defined subdivisions, use them; otherwise + // fall back to arithmetic chunks of `cellsPerPage`. const getContentCellParentId = (c: QuillCellContent) => (c.metadata?.parentId as string | undefined) ?? (c.data?.parentId as string | undefined); const rootContentCells = contentCells.filter((c) => !getContentCellParentId(c)); - const totalSubsections = Math.ceil(rootContentCells.length / cellsPerPage); + const subdivisions = milestone.subdivisions ?? resolveSubdivisions({ + rootContentCellIds: rootContentCells.map((c) => c.cellMarkers[0]).filter(Boolean), + placements: undefined, + nameOverrides: undefined, + cellsPerPage, + }); + const totalSubsections = Math.max(1, subdivisions.length); // Calculate progress for each subsection for (let subsectionIdx = 0; subsectionIdx < totalSubsections; subsectionIdx++) { - const startRootIndex = subsectionIdx * cellsPerPage; - const endRootIndex = Math.min( - startRootIndex + cellsPerPage, - rootContentCells.length - ); + const subdivision = subdivisions[subsectionIdx]; + const startRootIndex = subdivision?.startRootIndex ?? 0; + const endRootIndex = subdivision?.endRootIndex ?? rootContentCells.length; const rootsOnSubsection = rootContentCells.slice(startRootIndex, endRootIndex); const contentCellIdsForSubsection = new Set( rootsOnSubsection.map((c) => c.cellMarkers[0]) @@ -2008,17 +2110,24 @@ export class CodexCellDocument implements vscode.CustomDocument { // Paginate by root content cells only, so adding a child (e.g. to cell 44) does not bump // the last root (e.g. cell 50) to the next page. Each page shows N roots + all their descendants. const rootContentCells = contentCells.filter((c) => !getContentCellParentId(c)); - const totalSubsections = Math.ceil(rootContentCells.length / cellsPerPage); + // Subdivisions computed by buildMilestoneIndex drive both the legacy + // arithmetic chunking (as "auto" subdivisions) and any user-defined custom + // breaks. Using them here keeps slicing, counting, and webview rendering + // consistent. + const subdivisions = milestone.subdivisions ?? resolveSubdivisions({ + rootContentCellIds: rootContentCells.map((c) => c.cellMarkers[0]).filter(Boolean), + placements: undefined, + nameOverrides: undefined, + cellsPerPage, + }); + const totalSubsections = Math.max(1, subdivisions.length); const validSubsectionIndex = Math.min( Math.max(0, subsectionIndex), Math.max(0, totalSubsections - 1) ); - - const startRootIndex = validSubsectionIndex * cellsPerPage; - const endRootIndex = Math.min( - startRootIndex + cellsPerPage, - rootContentCells.length - ); + const activeSubdivision = subdivisions[validSubsectionIndex]; + const startRootIndex = activeSubdivision?.startRootIndex ?? 0; + const endRootIndex = activeSubdivision?.endRootIndex ?? rootContentCells.length; const rootsOnPage = rootContentCells.slice(startRootIndex, endRootIndex); // Include roots on this page and all their descendant content cells (children, grandchildren, etc.) @@ -2152,7 +2261,6 @@ export class CodexCellDocument implements vscode.CustomDocument { * @returns Number of subsections (pages) for this milestone */ public getSubsectionCountForMilestone(milestoneIndex: number, cellsPerPage: number = 50): number { - const cells = this._documentData.cells || []; const milestoneInfo = this.buildMilestoneIndex(cellsPerPage); if (milestoneIndex < 0 || milestoneIndex >= milestoneInfo.milestones.length) { @@ -2160,25 +2268,11 @@ export class CodexCellDocument implements vscode.CustomDocument { } const milestone = milestoneInfo.milestones[milestoneIndex]; - const nextMilestone = milestoneInfo.milestones[milestoneIndex + 1]; - const startCellIndex = milestone.cellIndex; - const endCellIndex = nextMilestone ? nextMilestone.cellIndex : cells.length; - - let rootContentCount = 0; - for (let i = startCellIndex; i < endCellIndex; i++) { - const cell = cells[i]; - if ( - cell.metadata?.type !== CodexCellTypes.MILESTONE && - cell.metadata?.type !== CodexCellTypes.PARATEXT && - cell.metadata?.data?.deleted !== true - ) { - const parentId = cell.metadata?.parentId ?? (cell.metadata?.data as { parentId?: string; } | undefined)?.parentId; - if (!parentId) { - rootContentCount++; - } - } - } - return Math.ceil(rootContentCount / cellsPerPage) || 1; + // Prefer the resolved subdivision list (includes custom placements). It is + // always non-empty when the milestone has root content cells, and empty + // when the milestone is empty; treat empty milestones as 1 subsection for + // back-compat with prior behavior. + return Math.max(1, milestone.subdivisions?.length ?? 0); } public updateCellLabel(cellId: string, newLabel: string) { diff --git a/src/providers/codexCellEditorProvider/utils/subdivisionUtils.ts b/src/providers/codexCellEditorProvider/utils/subdivisionUtils.ts new file mode 100644 index 000000000..32b0bfafd --- /dev/null +++ b/src/providers/codexCellEditorProvider/utils/subdivisionUtils.ts @@ -0,0 +1,202 @@ +import type { + MilestoneSubdivisionPlacement, + SubdivisionInfo, +} from "../../../../types"; + +/** + * Stable key used for the implicit first subdivision of a milestone. Kept public so + * target-side name override maps can reference it without repeating the literal. + */ +export const FIRST_SUBDIVISION_KEY = "__start__"; + +export interface ResolveSubdivisionsOptions { + /** + * Ordered IDs of the milestone's root content cells (non-milestone, non-paratext, + * non-deleted cells without a `parentId`). These form the pagination axis that + * subdivision anchors are resolved against. + */ + rootContentCellIds: string[]; + /** + * User-defined break anchors. Typically sourced from + * `milestoneCell.metadata.data.subdivisions` on the source document. The + * implicit first subdivision (starting at root index 0) is never listed here — + * only subsequent breaks. + */ + placements?: MilestoneSubdivisionPlacement[]; + /** + * Target-side display name overrides, keyed by subdivision key (typically + * `startCellId`, or `FIRST_SUBDIVISION_KEY` for the implicit first subdivision). + */ + nameOverrides?: { [key: string]: string; }; + /** + * Arithmetic fallback chunk size, used when there are no custom placements. + * When custom placements exist, this value is ignored. + */ + cellsPerPage: number; + /** + * Default display name for the implicit first subdivision when no custom name is + * set. Defaults to undefined (callers typically format a numbered fallback like + * "1–50"). + */ + firstSubdivisionDefaultName?: string; +} + +/** + * Resolves a milestone's root-content-cell range into a list of `SubdivisionInfo` + * items ready for rendering. + * + * Behaviour: + * - When `placements` is empty/undefined, returns arithmetic chunks of + * `cellsPerPage`. This matches the legacy 50-cell pagination exactly when + * `cellsPerPage === 50`. + * - Placements with a `startCellId` that no longer exists in `rootContentCellIds` + * are silently pruned (the anchored cell was deleted, merged away, etc.). + * - Placements are sorted by their resolved root-index so callers don't need to + * keep them ordered on disk. + * - Duplicate placements pointing at the same root index are collapsed. + * - When the last subdivision has a user-assigned name and additional root cells + * exist beyond it in a way not covered by a subsequent placement, no trailing + * auto-subdivision is added (the named subdivision absorbs the tail, consistent + * with expected naming semantics). When the last subdivision is unnamed, the + * tail is likewise absorbed. A trailing auto-subdivision is emitted ONLY when + * the only placements are the implicit first one and no root content cells + * remain uncovered — i.e. never. The tail-append semantics for new cells after + * the last explicit named break are handled at write time, not at resolve time. + * + * @returns Ordered, non-overlapping, fully-covering list of subdivisions. Always + * contains at least one item when `rootContentCellIds.length > 0`; returns an + * empty array when the milestone has zero root content cells. + */ +export function resolveSubdivisions( + opts: ResolveSubdivisionsOptions +): SubdivisionInfo[] { + const { + rootContentCellIds, + placements, + nameOverrides, + cellsPerPage, + firstSubdivisionDefaultName, + } = opts; + + const totalRoots = rootContentCellIds.length; + if (totalRoots === 0) { + return []; + } + + const resolveOverride = (key: string): string | undefined => { + if (!nameOverrides) return undefined; + const value = nameOverrides[key]; + return typeof value === "string" && value.length > 0 ? value : undefined; + }; + + // No custom placements → arithmetic chunking (preserves legacy behavior). + if (!placements || placements.length === 0) { + const pageSize = Math.max(1, cellsPerPage); + const pages = Math.max(1, Math.ceil(totalRoots / pageSize)); + const result: SubdivisionInfo[] = []; + for (let i = 0; i < pages; i++) { + const startRootIndex = i * pageSize; + const endRootIndex = Math.min(startRootIndex + pageSize, totalRoots); + const startCellId = rootContentCellIds[startRootIndex]; + const key = i === 0 ? FIRST_SUBDIVISION_KEY : startCellId ?? `auto-${i}`; + result.push({ + index: i, + startRootIndex, + endRootIndex, + key, + startCellId, + name: i === 0 ? resolveOverride(FIRST_SUBDIVISION_KEY) ?? firstSubdivisionDefaultName : resolveOverride(key), + source: "auto", + }); + } + return result; + } + + // Map from rootCellId → root index for fast anchor resolution. + const rootIdToIndex = new Map(); + for (let i = 0; i < rootContentCellIds.length; i++) { + rootIdToIndex.set(rootContentCellIds[i], i); + } + + // Resolve each placement → { rootIndex, name }. Skip anchors that reference a + // non-existent root cell or that resolve to index 0 (those collide with the + // implicit first subdivision; keep the placement's name for the first one). + interface ResolvedAnchor { rootIndex: number; name?: string; key: string; startCellId?: string; } + const resolved: ResolvedAnchor[] = []; + let firstSubdivisionName: string | undefined; + const seenIndices = new Set(); + for (const placement of placements) { + if (!placement || typeof placement.startCellId !== "string") continue; + const rootIndex = rootIdToIndex.get(placement.startCellId); + if (rootIndex === undefined) continue; // stale anchor — silently pruned + if (rootIndex === 0) { + // User anchored the "first break" at the first root cell — treat its + // name as naming the implicit first subdivision. + if (!firstSubdivisionName && placement.name) { + firstSubdivisionName = placement.name; + } + continue; + } + if (seenIndices.has(rootIndex)) continue; + seenIndices.add(rootIndex); + resolved.push({ + rootIndex, + name: placement.name, + key: placement.startCellId, + startCellId: placement.startCellId, + }); + } + + // Sort by root index so users aren't forced to write placements in order. + resolved.sort((a, b) => a.rootIndex - b.rootIndex); + + // Compose the subdivision list: implicit first + each resolved break. + const result: SubdivisionInfo[] = []; + const firstEnd = resolved.length > 0 ? resolved[0].rootIndex : totalRoots; + const firstStartCellId = rootContentCellIds[0]; + result.push({ + index: 0, + startRootIndex: 0, + endRootIndex: firstEnd, + key: FIRST_SUBDIVISION_KEY, + startCellId: firstStartCellId, + name: + resolveOverride(FIRST_SUBDIVISION_KEY) ?? + firstSubdivisionName ?? + firstSubdivisionDefaultName, + source: resolved.length > 0 ? "custom" : "auto", + }); + + for (let i = 0; i < resolved.length; i++) { + const anchor = resolved[i]; + const next = resolved[i + 1]; + const endRootIndex = next ? next.rootIndex : totalRoots; + result.push({ + index: i + 1, + startRootIndex: anchor.rootIndex, + endRootIndex, + key: anchor.key, + startCellId: anchor.startCellId, + name: resolveOverride(anchor.key) ?? anchor.name, + source: "custom", + }); + } + + return result; +} + +/** + * Finds the subdivision that contains `rootIndex`. Returns -1 if none match. + */ +export function findSubdivisionIndexForRoot( + subdivisions: SubdivisionInfo[], + rootIndex: number +): number { + for (let i = 0; i < subdivisions.length; i++) { + const s = subdivisions[i]; + if (rootIndex >= s.startRootIndex && rootIndex < s.endRootIndex) { + return i; + } + } + return -1; +} diff --git a/src/test/suite/milestoneSubdivisions.test.ts b/src/test/suite/milestoneSubdivisions.test.ts new file mode 100644 index 000000000..0324ac797 --- /dev/null +++ b/src/test/suite/milestoneSubdivisions.test.ts @@ -0,0 +1,356 @@ +import * as assert from "assert"; +import * as vscode from "vscode"; +import { CodexCellEditorProvider } from "../../providers/codexCellEditorProvider/codexCellEditorProvider"; +import { CodexCellDocument } from "../../providers/codexCellEditorProvider/codexDocument"; +import { CodexCellTypes } from "../../../types/enums"; +import { + FIRST_SUBDIVISION_KEY, + findSubdivisionIndexForRoot, + resolveSubdivisions, +} from "../../providers/codexCellEditorProvider/utils/subdivisionUtils"; +import { + swallowDuplicateCommandRegistrations, + createTempCodexFile, + deleteIfExists, + createMockExtensionContext, +} from "../testUtils"; +import sinon from "sinon"; + +suite("Milestone Subdivisions Test Suite", () => { + vscode.window.showInformationMessage("Start all tests for Milestone Subdivisions."); + let context: vscode.ExtensionContext; + let provider: CodexCellEditorProvider; + let tempUri: vscode.Uri; + + suiteSetup(async () => { + swallowDuplicateCommandRegistrations(); + }); + + setup(async () => { + context = createMockExtensionContext(); + provider = new CodexCellEditorProvider(context); + tempUri = await createTempCodexFile( + `test-subdivisions-${Date.now()}-${Math.random().toString(36).slice(2)}.codex`, + { cells: [], metadata: {} } + ); + + sinon.restore(); + sinon.stub((CodexCellDocument as any).prototype, "addCellToIndexImmediately").callsFake(() => { }); + sinon.stub((CodexCellDocument as any).prototype, "syncDirtyCellsToDatabase").resolves(); + sinon.stub((CodexCellDocument as any).prototype, "populateSourceCellMapFromIndex").resolves(); + }); + + teardown(async () => { + if (tempUri) await deleteIfExists(tempUri); + sinon.restore(); + }); + + async function createDocumentWithCells(cells: any[]): Promise { + const content = { + cells, + metadata: {}, + }; + await vscode.workspace.fs.writeFile(tempUri, Buffer.from(JSON.stringify(content, null, 2), "utf-8")); + return await provider.openCustomDocument( + tempUri, + { backupId: undefined }, + new vscode.CancellationTokenSource().token + ); + } + + // --------------------------------------------------------------------------- + // Pure-function tests for resolveSubdivisions + // --------------------------------------------------------------------------- + + suite("resolveSubdivisions()", () => { + const ids = (count: number) => Array.from({ length: count }, (_, i) => `c${i + 1}`); + + test("returns empty array when no root cells", () => { + const result = resolveSubdivisions({ + rootContentCellIds: [], + cellsPerPage: 50, + }); + assert.strictEqual(result.length, 0); + }); + + test("produces arithmetic chunks equivalent to legacy pagination when no placements", () => { + const rootIds = ids(125); + const result = resolveSubdivisions({ rootContentCellIds: rootIds, cellsPerPage: 50 }); + + assert.strictEqual(result.length, 3, "125 cells / 50 per page = 3 pages"); + assert.deepStrictEqual( + result.map((s) => [s.startRootIndex, s.endRootIndex]), + [[0, 50], [50, 100], [100, 125]], + "Legacy-equivalent arithmetic boundaries", + ); + assert.strictEqual(result[0].source, "auto"); + assert.strictEqual(result[0].key, FIRST_SUBDIVISION_KEY); + assert.strictEqual(result[1].startCellId, "c51"); + }); + + test("single page when count <= pageSize", () => { + const rootIds = ids(20); + const result = resolveSubdivisions({ rootContentCellIds: rootIds, cellsPerPage: 50 }); + assert.strictEqual(result.length, 1); + assert.strictEqual(result[0].endRootIndex, 20); + }); + + test("custom placement creates two subdivisions (break at c6)", () => { + const rootIds = ids(10); + const result = resolveSubdivisions({ + rootContentCellIds: rootIds, + placements: [{ startCellId: "c6", name: "Second Half" }], + cellsPerPage: 50, + }); + assert.strictEqual(result.length, 2); + assert.deepStrictEqual( + [result[0].startRootIndex, result[0].endRootIndex], + [0, 5], + ); + assert.deepStrictEqual( + [result[1].startRootIndex, result[1].endRootIndex], + [5, 10], + ); + assert.strictEqual(result[1].name, "Second Half"); + assert.strictEqual(result[1].source, "custom"); + assert.strictEqual(result[0].source, "custom", "first subdivision becomes custom once any break is defined"); + }); + + test("placements are sorted by root-index regardless of input order", () => { + const rootIds = ids(10); + const result = resolveSubdivisions({ + rootContentCellIds: rootIds, + placements: [ + { startCellId: "c8" }, + { startCellId: "c3" }, + { startCellId: "c6" }, + ], + cellsPerPage: 50, + }); + assert.strictEqual(result.length, 4); + assert.deepStrictEqual( + result.map((s) => s.startRootIndex), + [0, 2, 5, 7], + ); + }); + + test("stale anchors (cells no longer present) are silently pruned", () => { + const rootIds = ids(5); + const result = resolveSubdivisions({ + rootContentCellIds: rootIds, + placements: [ + { startCellId: "c3" }, + { startCellId: "doesNotExist" }, + { startCellId: "anotherMissing", name: "orphan" }, + ], + cellsPerPage: 50, + }); + assert.strictEqual(result.length, 2, "Only the valid placement contributes a break"); + assert.deepStrictEqual( + [result[0].startRootIndex, result[0].endRootIndex, result[1].startRootIndex, result[1].endRootIndex], + [0, 2, 2, 5], + ); + }); + + test("duplicate placements targeting the same cell are collapsed", () => { + const rootIds = ids(6); + const result = resolveSubdivisions({ + rootContentCellIds: rootIds, + placements: [ + { startCellId: "c4" }, + { startCellId: "c4", name: "dup" }, + ], + cellsPerPage: 50, + }); + assert.strictEqual(result.length, 2); + }); + + test("placement at c1 names the implicit first subdivision rather than creating a new one", () => { + const rootIds = ids(4); + const result = resolveSubdivisions({ + rootContentCellIds: rootIds, + placements: [{ startCellId: "c1", name: "Intro" }], + cellsPerPage: 50, + }); + assert.strictEqual(result.length, 1, "No actual break, just a name on the first subdivision"); + assert.strictEqual(result[0].name, "Intro"); + }); + + test("nameOverrides take precedence over source-stored names", () => { + const rootIds = ids(4); + const result = resolveSubdivisions({ + rootContentCellIds: rootIds, + placements: [{ startCellId: "c3", name: "Source Name" }], + nameOverrides: { c3: "Target Override" }, + cellsPerPage: 50, + }); + assert.strictEqual(result[1].name, "Target Override"); + }); + + test("nameOverrides for first subdivision use FIRST_SUBDIVISION_KEY", () => { + const rootIds = ids(4); + const result = resolveSubdivisions({ + rootContentCellIds: rootIds, + placements: [{ startCellId: "c3" }], + nameOverrides: { [FIRST_SUBDIVISION_KEY]: "My Start" }, + cellsPerPage: 50, + }); + assert.strictEqual(result[0].name, "My Start"); + }); + + test("findSubdivisionIndexForRoot locates the right subdivision", () => { + const rootIds = ids(10); + const subs = resolveSubdivisions({ + rootContentCellIds: rootIds, + placements: [{ startCellId: "c4" }, { startCellId: "c8" }], + cellsPerPage: 50, + }); + assert.strictEqual(findSubdivisionIndexForRoot(subs, 0), 0); + assert.strictEqual(findSubdivisionIndexForRoot(subs, 3), 1); + assert.strictEqual(findSubdivisionIndexForRoot(subs, 7), 2); + assert.strictEqual(findSubdivisionIndexForRoot(subs, 99), -1); + }); + }); + + // --------------------------------------------------------------------------- + // Document-level integration tests: subdivisions drive slicing APIs + // --------------------------------------------------------------------------- + + suite("CodexCellDocument slicing with custom subdivisions", () => { + /** Helper: build a milestone with 10 content cells and optional subdivisions. */ + function buildCellsWithSubdivisions(subdivisions?: Array<{ startCellId: string; name?: string; }>) { + const cells: any[] = [ + { + kind: 2, + languageId: "scripture", + value: "Luke 1", + metadata: { + type: CodexCellTypes.MILESTONE, + id: "milestone-1", + ...(subdivisions + ? { + data: { + subdivisions, + }, + } + : {}), + }, + }, + ]; + for (let i = 1; i <= 10; i++) { + cells.push({ + kind: 2, + languageId: "scripture", + value: `verse ${i}`, + metadata: { type: CodexCellTypes.TEXT, id: `v${i}` }, + }); + } + return cells; + } + + test("buildMilestoneIndex attaches arithmetic subdivisions when none defined", async () => { + const document = await createDocumentWithCells(buildCellsWithSubdivisions()); + const index = document.buildMilestoneIndex(4); + const milestone = index.milestones[0]; + assert.ok(milestone.subdivisions, "subdivisions should be present"); + assert.strictEqual(milestone.subdivisions!.length, 3, "10 / 4 pageSize = 3 pages"); + assert.strictEqual(milestone.subdivisions![0].source, "auto"); + }); + + test("buildMilestoneIndex uses user-defined subdivisions when present", async () => { + const cells = buildCellsWithSubdivisions([ + { startCellId: "v4", name: "Middle" }, + { startCellId: "v8", name: "End" }, + ]); + const document = await createDocumentWithCells(cells); + const index = document.buildMilestoneIndex(50); + const milestone = index.milestones[0]; + assert.strictEqual(milestone.subdivisions!.length, 3); + assert.strictEqual(milestone.subdivisions![0].source, "custom"); + assert.strictEqual(milestone.subdivisions![1].name, "Middle"); + assert.strictEqual(milestone.subdivisions![2].name, "End"); + }); + + test("getCellsForMilestone slices by custom subdivision, not cellsPerPage", async () => { + const cells = buildCellsWithSubdivisions([{ startCellId: "v6" }]); + const document = await createDocumentWithCells(cells); + // Request subsection 0 with cellsPerPage=50 (larger than milestone). + // Without subdivisions this would return all 10 cells; with the break + // at v6, subsection 0 must contain only v1..v5. + const sub0 = document.getCellsForMilestone(0, 0, 50); + assert.strictEqual(sub0.length, 5); + assert.strictEqual(sub0[0].cellMarkers[0], "v1"); + assert.strictEqual(sub0[4].cellMarkers[0], "v5"); + + const sub1 = document.getCellsForMilestone(0, 1, 50); + assert.strictEqual(sub1.length, 5); + assert.strictEqual(sub1[0].cellMarkers[0], "v6"); + assert.strictEqual(sub1[4].cellMarkers[0], "v10"); + }); + + test("getSubsectionCountForMilestone reflects custom subdivisions", async () => { + const cells = buildCellsWithSubdivisions([ + { startCellId: "v4" }, + { startCellId: "v8" }, + ]); + const document = await createDocumentWithCells(cells); + const count = document.getSubsectionCountForMilestone(0, 50); + assert.strictEqual(count, 3, "Expected 3 subsections from 2 custom breaks"); + }); + + test("findMilestoneAndSubsectionForCell respects custom subdivisions", async () => { + const cells = buildCellsWithSubdivisions([ + { startCellId: "v4" }, + { startCellId: "v8" }, + ]); + const document = await createDocumentWithCells(cells); + const pos1 = document.findMilestoneAndSubsectionForCell("v2"); + assert.deepStrictEqual(pos1, { milestoneIndex: 0, subsectionIndex: 0 }); + const pos2 = document.findMilestoneAndSubsectionForCell("v5"); + assert.deepStrictEqual(pos2, { milestoneIndex: 0, subsectionIndex: 1 }); + const pos3 = document.findMilestoneAndSubsectionForCell("v9"); + assert.deepStrictEqual(pos3, { milestoneIndex: 0, subsectionIndex: 2 }); + }); + + test("stale anchor (referencing a cell since removed) is ignored at resolve time", async () => { + // v4 placement is valid; ghost placement should be silently skipped. + const cells = buildCellsWithSubdivisions([ + { startCellId: "v4" }, + { startCellId: "ghostCell" }, + ]); + const document = await createDocumentWithCells(cells); + const index = document.buildMilestoneIndex(50); + assert.strictEqual( + index.milestones[0].subdivisions!.length, + 2, + "Only the valid anchor should produce a break; ghost pruned", + ); + }); + + test("legacy behavior preserved when no subdivisions on milestone", async () => { + // Sanity check: 125 cells with cellsPerPage=50 → 3 subsections and each page + // sized exactly as before the refactor. + const cells: any[] = [ + { + kind: 2, + languageId: "scripture", + value: "Luke 1", + metadata: { type: CodexCellTypes.MILESTONE, id: "m1" }, + }, + ]; + for (let i = 1; i <= 125; i++) { + cells.push({ + kind: 2, + languageId: "scripture", + value: `v${i}`, + metadata: { type: CodexCellTypes.TEXT, id: `v${i}` }, + }); + } + const document = await createDocumentWithCells(cells); + assert.strictEqual(document.getSubsectionCountForMilestone(0, 50), 3); + assert.strictEqual(document.getCellsForMilestone(0, 0, 50).length, 50); + assert.strictEqual(document.getCellsForMilestone(0, 1, 50).length, 50); + assert.strictEqual(document.getCellsForMilestone(0, 2, 50).length, 25); + }); + }); +}); diff --git a/types/index.d.ts b/types/index.d.ts index 5843aa6c9..1ae9e606d 100644 --- a/types/index.d.ts +++ b/types/index.d.ts @@ -604,6 +604,29 @@ export type EditorPostMessages = newValue: string; }; } + | { + command: "updateMilestoneSubdivisions"; + content: { + milestoneIndex: number; + /** + * Full new list of break anchors for this milestone. Each `startCellId` + * must refer to a current root content cell inside the milestone. + * Pass an empty array to clear all custom subdivisions (falls back to + * arithmetic pagination). + */ + subdivisions: MilestoneSubdivisionPlacement[]; + }; + } + | { + command: "updateMilestoneSubdivisionName"; + content: { + milestoneIndex: number; + /** Stable key identifying the subdivision (typically its `startCellId`). */ + subdivisionKey: string; + /** New display name. Pass an empty string to clear the override. */ + newName: string; + }; + } | { command: "refreshWebviewAfterMilestoneEdits"; content?: Record; @@ -712,8 +735,58 @@ type CodexData = Timestamps & { originalText?: string; globalReferences?: string[]; // Array of cell IDs in original format (e.g., "GEN 1:1") used for header generation milestoneIndex?: number | null; // 0-based milestone index for O(1) lookup (null if no milestone) + /** + * Optional user-defined subdivisions for a milestone cell. Only meaningful when the + * cell has `type === CodexCellTypes.MILESTONE`. The first subdivision is always the + * milestone itself (starts at root-content-cell index 0), so typically only explicit + * subsequent break anchors are persisted. Source documents are authoritative for + * placements. See `resolveSubdivisions`. + */ + subdivisions?: MilestoneSubdivisionPlacement[]; + /** + * Optional target-side localized name overrides for a milestone's subdivisions. + * Keyed by `startCellId` (or "__start__" for the implicit first subdivision). + * Only meaningful on target milestone cells. + */ + subdivisionNames?: { [subdivisionKey: string]: string; }; }; +/** + * A user-defined subdivision anchor within a milestone. `startCellId` is the stable + * id of the first root content cell of the subdivision. The implicit first + * subdivision (covering the start of the milestone) does not need an entry here; + * placements describe the breaks AFTER the start. + */ +export interface MilestoneSubdivisionPlacement { + /** Stable anchor — cell ID of the first root content cell of this subdivision. */ + startCellId: string; + /** Optional name (source-authoritative when stored on a source milestone). */ + name?: string; +} + +/** + * Resolved subdivision, ready for rendering/pagination. Produced by + * `resolveSubdivisions` at the provider layer. Root-index boundaries refer to + * the ordered list of root content cells (i.e. non-milestone, non-paratext, + * non-deleted cells without `parentId`) within a milestone. + */ +export interface SubdivisionInfo { + /** 0-based index of this subdivision within its milestone. */ + index: number; + /** Inclusive start in root-content-cell space. */ + startRootIndex: number; + /** Exclusive end in root-content-cell space. */ + endRootIndex: number; + /** Stable key for name overrides and for rendering stable React keys. */ + key: string; + /** Root content cell ID at `startRootIndex`, when a cell exists. */ + startCellId?: string; + /** Display name (source-stored name or target override). Callers format fallbacks. */ + name?: string; + /** "custom" = user-defined; "auto" = arithmetic chunk or auto-tail subdivision. */ + source: "auto" | "custom"; +} + type BaseCustomCellMetaData = { id: string; type: CodexCellTypes; @@ -908,6 +981,15 @@ export interface MilestoneInfo { value: string; /** Number of content cells in this milestone section (excluding milestone cell itself) */ cellCount: number; + /** + * Resolved subdivisions for this milestone, in order. When present, overrides the + * arithmetic `cellsPerPage` pagination. Computed by + * `codexDocument.buildMilestoneIndex` based on the milestone cell's stored + * `metadata.data.subdivisions` (plus `metadata.data.subdivisionNames` on target + * documents). If no custom subdivisions exist, this is an array of one or more + * auto-subdivisions matching the arithmetic chunking. + */ + subdivisions?: SubdivisionInfo[]; } /** From ec5b5c31b933569173d2091ac97f27a9406468fb Mon Sep 17 00:00:00 2001 From: Luke-Bilhorn Date: Fri, 24 Apr 2026 01:32:33 -0500 Subject: [PATCH 02/11] =?UTF-8?q?Handle=20milestone=20subdivision=20writes?= =?UTF-8?q?=20with=20source=E2=86=92target=20mirroring?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds message handlers in codexCellEditorMessagehandling.ts and wires them into the provider and codexDocument so that subdivision anchor mutations on the source side are persisted and mirrored to the paired target webview. Covers addAnchor, deleteAnchor, renameAnchor, and resetMilestone operations. Tests added for the write pipeline. --- .../codexCellEditorMessagehandling.ts | 217 ++++++++++++++++++ .../codexCellEditorProvider.ts | 44 +++- .../codexCellEditorProvider/codexDocument.ts | 27 ++- src/test/suite/milestoneSubdivisions.test.ts | 98 ++++++++ 4 files changed, 384 insertions(+), 2 deletions(-) diff --git a/src/providers/codexCellEditorProvider/codexCellEditorMessagehandling.ts b/src/providers/codexCellEditorProvider/codexCellEditorMessagehandling.ts index 5a5c84ae1..e376ae327 100644 --- a/src/providers/codexCellEditorProvider/codexCellEditorMessagehandling.ts +++ b/src/providers/codexCellEditorProvider/codexCellEditorMessagehandling.ts @@ -21,6 +21,7 @@ import { toPosixPath } from "../../utils/pathUtils"; import { revalidateCellMissingFlags } from "../../utils/audioMissingUtils"; import { mergeAudioFiles } from "../../utils/audioMerger"; import { getAttachmentDocumentSegmentFromUri } from "../../utils/attachmentFolderUtils"; +import { isSourceFileFlexible } from "../../utils/fileTypeUtils"; // Enable debug logging if needed const DEBUG_MODE = false; @@ -1271,6 +1272,222 @@ const messageHandlers: Record Promise { + const typedEvent = event as Extract; + debug("updateMilestoneSubdivisions message received", { event }); + + // Placements are source-authoritative. Reject writes originating from a + // target (.codex) document so the source stays the single source of truth. + if (!isSourceFileFlexible(document.uri)) { + console.warn( + "[updateMilestoneSubdivisions] Rejected write from non-source document:", + document.uri.toString() + ); + vscode.window.showWarningMessage( + "Subdivision breaks can only be edited from the source file." + ); + return; + } + + const { milestoneIndex, subdivisions: incomingPlacements } = typedEvent.content; + + const milestoneIndexObj = document.buildMilestoneIndex(); + const milestone = milestoneIndexObj.milestones[milestoneIndex]; + if (!milestone) { + console.error("[updateMilestoneSubdivisions] Milestone not found at index", milestoneIndex); + vscode.window.showErrorMessage( + `Failed to update subdivisions: milestone not found at index ${milestoneIndex}` + ); + return; + } + + const sourceMilestoneCell = document.getCellByIndex(milestone.cellIndex); + if ( + !sourceMilestoneCell || + sourceMilestoneCell.metadata?.type !== CodexCellTypes.MILESTONE || + !sourceMilestoneCell.metadata?.id + ) { + console.error( + "[updateMilestoneSubdivisions] Cell at index is not a valid milestone cell", + milestone.cellIndex + ); + vscode.window.showErrorMessage("Failed to update subdivisions: invalid milestone cell."); + return; + } + + // Validate every anchor refers to a current root content cell inside the + // milestone. Stale anchors are normally pruned at resolve time, but we + // still hard-reject invalid writes here so callers can surface errors. + const validRootIds = new Set(document.getRootContentCellIdsForMilestone(milestoneIndex)); + const seen = new Set(); + const sanitized: typeof incomingPlacements = []; + for (const placement of incomingPlacements ?? []) { + if (!placement || typeof placement.startCellId !== "string") continue; + if (!validRootIds.has(placement.startCellId)) { + console.warn( + "[updateMilestoneSubdivisions] Dropping unknown startCellId:", + placement.startCellId + ); + continue; + } + if (seen.has(placement.startCellId)) continue; + seen.add(placement.startCellId); + // Intentionally strip names from mirrored placements; names live on + // the separate `subdivisionNames` map so that source and target can + // be named independently (per design spec). + const entry: { startCellId: string; name?: string; } = { + startCellId: placement.startCellId, + }; + if (typeof placement.name === "string" && placement.name.length > 0) { + entry.name = placement.name; + } + sanitized.push(entry); + } + + const sourceCellId = sourceMilestoneCell.metadata.id; + const cancellationToken = new vscode.CancellationTokenSource().token; + + try { + await document.refreshAuthor(); + document.updateCellData(sourceCellId, { subdivisions: sanitized }); + await provider.saveCustomDocument(document, cancellationToken); + debug( + `[updateMilestoneSubdivisions] Updated source subdivisions for milestone ${milestoneIndex}`, + { count: sanitized.length } + ); + } catch (error) { + console.error("[updateMilestoneSubdivisions] Failed to update source:", error); + vscode.window.showErrorMessage( + `Failed to update subdivisions: ${error instanceof Error ? error.message : String(error)}` + ); + return; + } + + // Mirror placements (without names) to the paired target document. + try { + const pairedUri = provider.getPairedNotebookUri(document.uri); + if (pairedUri) { + const targetDocument = await provider.getOrOpenDocumentForUri(pairedUri); + const targetMilestoneIndexObj = targetDocument.buildMilestoneIndex(); + const targetMilestone = targetMilestoneIndexObj.milestones[milestoneIndex]; + if (!targetMilestone) { + console.warn( + "[updateMilestoneSubdivisions] Target has no milestone at index, skipping mirror:", + milestoneIndex + ); + } else { + // Sanity check: positionally-paired milestones should share + // root content cell IDs. If they diverge, skip the mirror to + // avoid attaching source anchors to unrelated target cells. + const sourceRootIds = document.getRootContentCellIdsForMilestone(milestoneIndex); + const targetRootIds = targetDocument.getRootContentCellIdsForMilestone(milestoneIndex); + const rootsMatch = + sourceRootIds.length === targetRootIds.length && + sourceRootIds.every((id, i) => id === targetRootIds[i]); + if (!rootsMatch) { + console.warn( + "[updateMilestoneSubdivisions] Source/target milestones diverge; skipping mirror.", + { + milestoneIndex, + sourceLength: sourceRootIds.length, + targetLength: targetRootIds.length, + } + ); + } else { + const targetMilestoneCell = targetDocument.getCellByIndex(targetMilestone.cellIndex); + if (targetMilestoneCell?.metadata?.id) { + // Mirror placements with names stripped — target uses + // its own `subdivisionNames` for local display. + const mirroredPlacements = sanitized.map((p) => ({ + startCellId: p.startCellId, + })); + await targetDocument.refreshAuthor(); + targetDocument.updateCellData(targetMilestoneCell.metadata.id, { + subdivisions: mirroredPlacements, + }); + await provider.saveCustomDocument(targetDocument, cancellationToken); + debug( + "[updateMilestoneSubdivisions] Mirrored subdivisions to target:", + { uri: pairedUri.toString(), count: mirroredPlacements.length } + ); + } + } + } + } + } catch (mirrorError) { + // Mirror failures are non-fatal: the source edit has already + // succeeded. Log and continue so the user can retry later. + console.error( + "[updateMilestoneSubdivisions] Failed to mirror to target:", + mirrorError + ); + } + + await sendMilestoneRefreshToWebview(document, webviewPanel, provider); + }, + + updateMilestoneSubdivisionName: async ({ event, document, webviewPanel, provider }) => { + const typedEvent = event as Extract; + debug("updateMilestoneSubdivisionName message received", { event }); + + const { milestoneIndex, subdivisionKey, newName } = typedEvent.content; + + const milestoneIndexObj = document.buildMilestoneIndex(); + const milestone = milestoneIndexObj.milestones[milestoneIndex]; + if (!milestone) { + console.error("[updateMilestoneSubdivisionName] Milestone not found at index", milestoneIndex); + vscode.window.showErrorMessage( + `Failed to rename subdivision: milestone not found at index ${milestoneIndex}` + ); + return; + } + + const milestoneCell = document.getCellByIndex(milestone.cellIndex); + if ( + !milestoneCell || + milestoneCell.metadata?.type !== CodexCellTypes.MILESTONE || + !milestoneCell.metadata?.id + ) { + console.error( + "[updateMilestoneSubdivisionName] Invalid milestone cell", + milestone.cellIndex + ); + vscode.window.showErrorMessage("Failed to rename subdivision: invalid milestone cell."); + return; + } + + // Names live on a separate `subdivisionNames` map so source and target + // can be renamed independently. Empty string clears the override. + const existingNames = + (milestoneCell.metadata?.data as { subdivisionNames?: { [k: string]: string; }; } | undefined) + ?.subdivisionNames ?? {}; + const nextNames: { [k: string]: string; } = { ...existingNames }; + const trimmed = typeof newName === "string" ? newName.trim() : ""; + if (trimmed.length === 0) { + delete nextNames[subdivisionKey]; + } else { + nextNames[subdivisionKey] = trimmed; + } + + const cancellationToken = new vscode.CancellationTokenSource().token; + try { + await document.refreshAuthor(); + document.updateCellData(milestoneCell.metadata.id, { subdivisionNames: nextNames }); + await provider.saveCustomDocument(document, cancellationToken); + debug( + `[updateMilestoneSubdivisionName] Updated name for milestone ${milestoneIndex}, key ${subdivisionKey}` + ); + } catch (error) { + console.error("[updateMilestoneSubdivisionName] Failed:", error); + vscode.window.showErrorMessage( + `Failed to rename subdivision: ${error instanceof Error ? error.message : String(error)}` + ); + return; + } + + await sendMilestoneRefreshToWebview(document, webviewPanel, provider); + }, + updateNotebookMetadata: async ({ event, document, webviewPanel, provider }) => { const typedEvent = event as Extract; debug("updateNotebookMetadata message received", { event }); diff --git a/src/providers/codexCellEditorProvider/codexCellEditorProvider.ts b/src/providers/codexCellEditorProvider/codexCellEditorProvider.ts index af5d5ed0c..920481b82 100755 --- a/src/providers/codexCellEditorProvider/codexCellEditorProvider.ts +++ b/src/providers/codexCellEditorProvider/codexCellEditorProvider.ts @@ -48,7 +48,7 @@ import { isSourceFileFlexible, isMatchingFilePair as isMatchingFilePairUtil, } from "../../utils/fileTypeUtils"; -import { getCorrespondingSourceUri } from "../../utils/codexNotebookUtils"; +import { getCorrespondingCodexUri, getCorrespondingSourceUri } from "../../utils/codexNotebookUtils"; // Enable debug logging if needed const DEBUG_MODE = false; @@ -1686,6 +1686,48 @@ export class CodexCellEditorProvider implements vscode.CustomEditorProvider { + const uriString = uri.toString(); + for (const [panelUri] of this.webviewPanels.entries()) { + if (this.isMatchingFilePair(uriString, panelUri) && panelUri === uriString) { + // Reuse the document backing the open panel. openCustomDocument + // returns the cached instance when one exists for this exact URI. + return await this.openCustomDocument( + vscode.Uri.parse(panelUri), + {}, + new vscode.CancellationTokenSource().token + ); + } + } + return await this.openCustomDocument(uri, {}, new vscode.CancellationTokenSource().token); + } + private updateTextDirection( webviewPanel: vscode.WebviewPanel, document: CodexCellDocument diff --git a/src/providers/codexCellEditorProvider/codexDocument.ts b/src/providers/codexCellEditorProvider/codexDocument.ts index 893a3559c..86e7ee59b 100644 --- a/src/providers/codexCellEditorProvider/codexDocument.ts +++ b/src/providers/codexCellEditorProvider/codexDocument.ts @@ -1312,6 +1312,24 @@ export class CodexCellDocument implements vscode.CustomDocument { return false; } + /** + * Returns the ordered list of root content cell IDs belonging to a milestone, + * in document order. Useful for anchor validation (incoming + * `updateMilestoneSubdivisions` writes) and for source↔target alignment + * sanity checks before mirroring placements. + * + * Returns an empty array if the milestone index is out of bounds. + */ + public getRootContentCellIdsForMilestone(milestoneIndex: number): string[] { + const cells = this._documentData.cells || []; + const info = this.buildMilestoneIndex(); + if (milestoneIndex < 0 || milestoneIndex >= info.milestones.length) return []; + const milestone = info.milestones[milestoneIndex]; + const next = info.milestones[milestoneIndex + 1]; + const end = next ? next.cellIndex : cells.length; + return this.getRootContentCellIdsInRange(milestone.cellIndex, end); + } + /** * Returns the ordered list of root content cell IDs within the given index * range. Root content cells are non-milestone, non-paratext, non-deleted cells @@ -3014,7 +3032,14 @@ export class CodexCellDocument implements vscode.CustomDocument { // Check if this is a milestone cell and if we're modifying data that affects milestone index const isMilestoneCell = cellToUpdate.metadata?.type === CodexCellTypes.MILESTONE; const isModifyingDeletedFlag = 'deleted' in newData; - const shouldInvalidateCache = isMilestoneCell && isModifyingDeletedFlag; + // Subdivision-related changes alter pagination, so the cached index must + // be invalidated alongside the deleted-flag case. Name-only overrides + // also flow through this path so that resolved `MilestoneInfo.subdivisions` + // picks up new names on next render. + const isModifyingSubdivisions = + 'subdivisions' in newData || 'subdivisionNames' in newData; + const shouldInvalidateCache = + isMilestoneCell && (isModifyingDeletedFlag || isModifyingSubdivisions); // Ensure metadata exists if (!this._documentData.cells[indexOfCellToUpdate].metadata) { diff --git a/src/test/suite/milestoneSubdivisions.test.ts b/src/test/suite/milestoneSubdivisions.test.ts index 0324ac797..b5406c087 100644 --- a/src/test/suite/milestoneSubdivisions.test.ts +++ b/src/test/suite/milestoneSubdivisions.test.ts @@ -327,6 +327,104 @@ suite("Milestone Subdivisions Test Suite", () => { ); }); + test("getRootContentCellIdsForMilestone returns all root content cells in order", async () => { + const cells = buildCellsWithSubdivisions(); + const document = await createDocumentWithCells(cells); + const rootIds = document.getRootContentCellIdsForMilestone(0); + assert.deepStrictEqual( + rootIds, + ["v1", "v2", "v3", "v4", "v5", "v6", "v7", "v8", "v9", "v10"], + ); + }); + + test("getRootContentCellIdsForMilestone excludes paratext, deleted, and child cells", async () => { + const cells: any[] = [ + { + kind: 2, + languageId: "scripture", + value: "Luke 1", + metadata: { type: CodexCellTypes.MILESTONE, id: "m1" }, + }, + { kind: 2, languageId: "scripture", value: "v1", metadata: { type: CodexCellTypes.TEXT, id: "v1" } }, + { + kind: 2, + languageId: "scripture", + value: "v1-child", + metadata: { type: CodexCellTypes.TEXT, id: "v1c", parentId: "v1" }, + }, + { + kind: 2, + languageId: "scripture", + value: "paratext", + metadata: { type: CodexCellTypes.PARATEXT, id: "p1" }, + }, + { + kind: 2, + languageId: "scripture", + value: "deleted", + metadata: { type: CodexCellTypes.TEXT, id: "d1", data: { deleted: true } }, + }, + { kind: 2, languageId: "scripture", value: "v2", metadata: { type: CodexCellTypes.TEXT, id: "v2" } }, + ]; + const document = await createDocumentWithCells(cells); + const rootIds = document.getRootContentCellIdsForMilestone(0); + assert.deepStrictEqual(rootIds, ["v1", "v2"]); + }); + + test("updateCellData('subdivisions') invalidates pagination cache", async () => { + const document = await createDocumentWithCells(buildCellsWithSubdivisions()); + + // First read: no custom breaks → arithmetic + const before = document.buildMilestoneIndex(50); + assert.strictEqual(before.milestones[0].subdivisions!.length, 1); + + // Update subdivisions via the same path the message handler uses. + document.updateCellData("milestone-1", { + subdivisions: [{ startCellId: "v6" }], + }); + + const after = document.buildMilestoneIndex(50); + assert.strictEqual( + after.milestones[0].subdivisions!.length, + 2, + "New subdivisions must be reflected after updateCellData", + ); + assert.strictEqual(after.milestones[0].subdivisions![1].startCellId, "v6"); + }); + + test("updateCellData('subdivisionNames') picks up name overrides", async () => { + const cells = buildCellsWithSubdivisions([{ startCellId: "v6" }]); + const document = await createDocumentWithCells(cells); + + document.updateCellData("milestone-1", { + subdivisionNames: { + [FIRST_SUBDIVISION_KEY]: "Opening", + v6: "Later Half", + }, + }); + + const index = document.buildMilestoneIndex(50); + assert.strictEqual(index.milestones[0].subdivisions![0].name, "Opening"); + assert.strictEqual(index.milestones[0].subdivisions![1].name, "Later Half"); + }); + + test("empty subdivisions array restores arithmetic pagination", async () => { + const cells = buildCellsWithSubdivisions([{ startCellId: "v6" }]); + const document = await createDocumentWithCells(cells); + + // Sanity: starts custom + let index = document.buildMilestoneIndex(50); + assert.strictEqual(index.milestones[0].subdivisions![0].source, "custom"); + + document.updateCellData("milestone-1", { subdivisions: [] }); + index = document.buildMilestoneIndex(50); + assert.strictEqual( + index.milestones[0].subdivisions![0].source, + "auto", + "Clearing placements should fall back to arithmetic pagination", + ); + }); + test("legacy behavior preserved when no subdivisions on milestone", async () => { // Sanity check: 125 cells with cellsPerPage=50 → 3 subsections and each page // sized exactly as before the refactor. From d89c40c0ed672b1f81d06f6e812b422b08fbfd9e Mon Sep 17 00:00:00 2001 From: Luke-Bilhorn Date: Fri, 24 Apr 2026 01:32:42 -0500 Subject: [PATCH 03/11] Thread resolver subdivisions through the cell editor webview Extracts subdivision utility helpers into webviews/.../CodexCellEditor/utils/subdivisionUtils.ts, adds webview types (lib/types.ts), and wires the resolver's milestoneIndex data into CodexCellEditor.tsx so the accordion can consume it. Unit tests cover the webview-side subdivision helpers. --- .../src/CodexCellEditor/CodexCellEditor.tsx | 33 +----- .../utils/subdivisionUtils.test.ts | 109 ++++++++++++++++++ .../CodexCellEditor/utils/subdivisionUtils.ts | 72 ++++++++++++ webviews/codex-webviews/src/lib/types.ts | 20 ++++ 4 files changed, 203 insertions(+), 31 deletions(-) create mode 100644 webviews/codex-webviews/src/CodexCellEditor/utils/subdivisionUtils.test.ts create mode 100644 webviews/codex-webviews/src/CodexCellEditor/utils/subdivisionUtils.ts diff --git a/webviews/codex-webviews/src/CodexCellEditor/CodexCellEditor.tsx b/webviews/codex-webviews/src/CodexCellEditor/CodexCellEditor.tsx index d14344976..af020ad20 100755 --- a/webviews/codex-webviews/src/CodexCellEditor/CodexCellEditor.tsx +++ b/webviews/codex-webviews/src/CodexCellEditor/CodexCellEditor.tsx @@ -33,6 +33,7 @@ import { import "./TranslationAnimations.css"; import { getVSCodeAPI } from "../shared/vscodeApi"; import { Subsection, ProgressPercentages } from "../lib/types"; +import { buildSubsectionsForMilestone } from "./utils/subdivisionUtils"; import { ABTestVariantSelector } from "./components/ABTestVariantSelector"; import { useMessageHandler } from "./hooks/useCentralizedMessageDispatcher"; import { createCacheHelpers, createProgressCacheHelpers } from "./utils"; @@ -1847,38 +1848,8 @@ const CodexCellEditor: React.FC = () => { } const milestone = milestoneIndex.milestones[milestoneIdx]; - const { cellCount, value } = milestone; const effectiveCellsPerPage = milestoneIndex.cellsPerPage || cellsPerPage; - - // When milestone has 0 cells, return a single empty subsection (avoid invalid "1-0" label) - if (cellCount === 0) { - return [ - { - id: `milestone-${milestoneIdx}-page-0`, - label: "0", - startIndex: 0, - endIndex: 0, - }, - ]; - } - - // Calculate number of pages based on content cells - const totalPages = Math.ceil(cellCount / effectiveCellsPerPage) || 1; - const subsections: Subsection[] = []; - - for (let i = 0; i < totalPages; i++) { - const startCellNumber = i * effectiveCellsPerPage + 1; - const endCellNumber = Math.min((i + 1) * effectiveCellsPerPage, cellCount); - - subsections.push({ - id: `milestone-${milestoneIdx}-page-${i}`, - label: `${startCellNumber}-${endCellNumber}`, - startIndex: i * effectiveCellsPerPage, - endIndex: endCellNumber, - }); - } - - return subsections; + return buildSubsectionsForMilestone(milestoneIdx, milestone, effectiveCellsPerPage); }, [milestoneIndex, cellsPerPage] ); diff --git a/webviews/codex-webviews/src/CodexCellEditor/utils/subdivisionUtils.test.ts b/webviews/codex-webviews/src/CodexCellEditor/utils/subdivisionUtils.test.ts new file mode 100644 index 000000000..17693b398 --- /dev/null +++ b/webviews/codex-webviews/src/CodexCellEditor/utils/subdivisionUtils.test.ts @@ -0,0 +1,109 @@ +import { describe, it, expect } from "vitest"; +import type { MilestoneInfo, SubdivisionInfo } from "../../../../../types"; +import { buildSubsectionsForMilestone } from "./subdivisionUtils"; + +const makeMilestone = ( + overrides: Partial = {}, + subdivisions?: SubdivisionInfo[] +): MilestoneInfo => ({ + value: "Luke 1", + cellIndex: 0, + cellCount: 10, + firstCellId: "v1", + subdivisions, + ...overrides, +}); + +describe("buildSubsectionsForMilestone", () => { + it("returns [] when milestone is undefined", () => { + const subs = buildSubsectionsForMilestone(0, undefined, 50); + expect(subs).toEqual([]); + }); + + it("returns a single zero-range subsection for empty milestones", () => { + const subs = buildSubsectionsForMilestone(0, makeMilestone({ cellCount: 0 }), 50); + expect(subs).toHaveLength(1); + expect(subs[0].label).toBe("0"); + expect(subs[0].startIndex).toBe(0); + expect(subs[0].endIndex).toBe(0); + }); + + it("prefers resolver subdivisions over arithmetic fallback", () => { + const subs = buildSubsectionsForMilestone( + 0, + makeMilestone({ cellCount: 10 }, [ + { + index: 0, + startRootIndex: 0, + endRootIndex: 5, + key: "__start__", + startCellId: "v1", + name: "Beginning", + source: "custom", + }, + { + index: 1, + startRootIndex: 5, + endRootIndex: 10, + key: "v6", + startCellId: "v6", + source: "custom", + }, + ]), + 50 + ); + expect(subs).toHaveLength(2); + expect(subs[0].label).toBe("1-5"); + expect(subs[0].name).toBe("Beginning"); + expect(subs[0].startCellId).toBe("v1"); + expect(subs[0].source).toBe("custom"); + expect(subs[1].label).toBe("6-10"); + expect(subs[1].key).toBe("v6"); + expect(subs[1].name).toBeUndefined(); + }); + + it("falls back to arithmetic pagination when no subdivisions are provided", () => { + const subs = buildSubsectionsForMilestone( + 0, + makeMilestone({ cellCount: 125 }, undefined), + 50 + ); + expect(subs).toHaveLength(3); + expect(subs.map((s) => s.label)).toEqual(["1-50", "51-100", "101-125"]); + expect(subs.map((s) => [s.startIndex, s.endIndex])).toEqual([ + [0, 50], + [50, 100], + [100, 125], + ]); + }); + + it("arithmetic label matches resolver output for the no-custom-breaks case", () => { + // When the resolver produces auto-only subdivisions, the labels must + // match what the legacy arithmetic path produced. Guarantees no UI + // regression for notebooks without custom breaks. + const arithmetic = buildSubsectionsForMilestone(0, makeMilestone({ cellCount: 125 }), 50); + const resolverEquivalent = buildSubsectionsForMilestone( + 0, + makeMilestone({ cellCount: 125 }, [ + { index: 0, startRootIndex: 0, endRootIndex: 50, key: "__start__", startCellId: "v1", source: "auto" }, + { index: 1, startRootIndex: 50, endRootIndex: 100, key: "v51", startCellId: "v51", source: "auto" }, + { index: 2, startRootIndex: 100, endRootIndex: 125, key: "v101", startCellId: "v101", source: "auto" }, + ]), + 50 + ); + expect(resolverEquivalent.map((s) => s.label)).toEqual(arithmetic.map((s) => s.label)); + expect(resolverEquivalent.map((s) => [s.startIndex, s.endIndex])).toEqual( + arithmetic.map((s) => [s.startIndex, s.endIndex]) + ); + }); + + it("assigns stable IDs based on milestone index", () => { + const subs = buildSubsectionsForMilestone(2, makeMilestone({ cellCount: 100 }), 50); + expect(subs.map((s) => s.id)).toEqual(["milestone-2-page-0", "milestone-2-page-1"]); + }); + + it("handles cellsPerPage=0 gracefully by clamping to 1", () => { + const subs = buildSubsectionsForMilestone(0, makeMilestone({ cellCount: 3 }), 0); + expect(subs).toHaveLength(3); + }); +}); diff --git a/webviews/codex-webviews/src/CodexCellEditor/utils/subdivisionUtils.ts b/webviews/codex-webviews/src/CodexCellEditor/utils/subdivisionUtils.ts new file mode 100644 index 000000000..55e565688 --- /dev/null +++ b/webviews/codex-webviews/src/CodexCellEditor/utils/subdivisionUtils.ts @@ -0,0 +1,72 @@ +import type { MilestoneInfo } from "../../../../../types"; +import type { Subsection } from "../../lib/types"; + +/** + * Builds the UI-facing `Subsection` list for a milestone. Prefers + * provider-computed subdivisions (custom user breaks and/or the arithmetic + * fallback produced by the resolver) and falls back to a local arithmetic + * calculation only when those are absent — typically during the narrow window + * between loading the webview and receiving the first `milestoneIndex` update. + * + * Returns: + * - exactly one zero-range subsection for empty milestones, so the UI never + * renders a nonsensical `"1-0"` label; + * - subsections whose `label` is always a numeric `"-"` range, + * regardless of whether a `name` is present. Callers decide whether to + * display `name` in place of `label`. + */ +export function buildSubsectionsForMilestone( + milestoneIdx: number, + milestone: MilestoneInfo | undefined, + cellsPerPage: number +): Subsection[] { + if (!milestone) return []; + + const { cellCount } = milestone; + + if (cellCount === 0) { + return [ + { + id: `milestone-${milestoneIdx}-page-0`, + label: "0", + startIndex: 0, + endIndex: 0, + }, + ]; + } + + // Resolver-provided subdivisions already encode both custom and arithmetic + // layouts; trust them when available. + if (milestone.subdivisions && milestone.subdivisions.length > 0) { + return milestone.subdivisions.map((sub, i) => { + const startCellNumber = sub.startRootIndex + 1; + const endCellNumber = sub.endRootIndex; + return { + id: `milestone-${milestoneIdx}-page-${i}`, + label: `${startCellNumber}-${endCellNumber}`, + startIndex: sub.startRootIndex, + endIndex: sub.endRootIndex, + name: sub.name, + key: sub.key, + startCellId: sub.startCellId, + source: sub.source, + }; + }); + } + + // Legacy fallback for stale milestoneIndex payloads missing `subdivisions`. + const pageSize = Math.max(1, cellsPerPage); + const totalPages = Math.ceil(cellCount / pageSize) || 1; + const subsections: Subsection[] = []; + for (let i = 0; i < totalPages; i++) { + const startCellNumber = i * pageSize + 1; + const endCellNumber = Math.min((i + 1) * pageSize, cellCount); + subsections.push({ + id: `milestone-${milestoneIdx}-page-${i}`, + label: `${startCellNumber}-${endCellNumber}`, + startIndex: i * pageSize, + endIndex: endCellNumber, + }); + } + return subsections; +} diff --git a/webviews/codex-webviews/src/lib/types.ts b/webviews/codex-webviews/src/lib/types.ts index 1fb1ae5d4..bfaef2c0c 100644 --- a/webviews/codex-webviews/src/lib/types.ts +++ b/webviews/codex-webviews/src/lib/types.ts @@ -62,6 +62,26 @@ export interface Subsection { label: string; startIndex: number; endIndex: number; + /** + * User-assigned name for this subdivision, when present. The navigation + * header and milestone accordion prefer `name` over `label` for display; + * callers that always want a numeric range should continue to read + * `label`. + */ + name?: string; + /** + * Stable key for this subdivision (typically `startCellId`, or a reserved + * key for the implicit first subdivision). Used when persisting + * name/placement edits back to the provider. + */ + key?: string; + /** + * ID of the root content cell that anchors this subdivision's start. + * Undefined when the subdivision wraps an empty milestone. + */ + startCellId?: string; + /** Whether the subdivision boundary was user-authored or auto-calculated. */ + source?: "auto" | "custom"; } export type FileStatus = "dirty" | "syncing" | "synced" | "none"; From 20dd335e4a0182e555eb40e825d4f470954ac364 Mon Sep 17 00:00:00 2001 From: Luke-Bilhorn Date: Fri, 24 Apr 2026 01:32:51 -0500 Subject: [PATCH 04/11] Add rename, delete, and reset controls to the milestone accordion Wires up inline editing for subdivision labels (rename) and adds per-subsection delete and per-milestone reset affordances in MilestoneAccordion.tsx (source side only). All three operations dispatch messages to the provider. Tests cover rename, delete, and reset interactions. --- .../components/MilestoneAccordion.test.tsx | 366 +++++++++++++++++ .../components/MilestoneAccordion.tsx | 381 ++++++++++++++++-- 2 files changed, 715 insertions(+), 32 deletions(-) diff --git a/webviews/codex-webviews/src/CodexCellEditor/components/MilestoneAccordion.test.tsx b/webviews/codex-webviews/src/CodexCellEditor/components/MilestoneAccordion.test.tsx index 23eb68b11..acad726f9 100644 --- a/webviews/codex-webviews/src/CodexCellEditor/components/MilestoneAccordion.test.tsx +++ b/webviews/codex-webviews/src/CodexCellEditor/components/MilestoneAccordion.test.tsx @@ -62,6 +62,8 @@ vi.mock("lucide-react", () => ({ Languages: () =>
Languages
, Check: () =>
Check
, RotateCcw: () =>
RotateCcw
, + X: () =>
X
, + Undo2: () =>
Undo2
, })); vi.mock("../../components/ui/icons/MicrophoneIcon", () => ({ @@ -758,6 +760,370 @@ describe("MilestoneAccordion - Milestone Editing", () => { }); }); + describe("Subsection Rename", () => { + const createSubsectionWithKey = ( + id: string, + label: string, + key: string, + name?: string + ): Subsection => ({ + id, + label, + startIndex: 0, + endIndex: 5, + key, + name, + startCellId: key, + source: "custom", + }); + + it("renders rename button only for subsections that carry a key", async () => { + mockGetSubsectionsForMilestone = vi.fn((milestoneIdx: number) => [ + createSubsectionWithKey( + `s-${milestoneIdx}-0`, + "1-5", + "__start__", + undefined + ), + createSubsectionWithKey(`s-${milestoneIdx}-1`, "6-10", "v6", "Second Half"), + // Legacy/arithmetic subsection with no key → should not expose rename + { + id: `s-${milestoneIdx}-legacy`, + label: "11-15", + startIndex: 10, + endIndex: 15, + }, + ]); + renderMilestoneAccordion({ + milestoneIndex: createMockMilestoneIndex([{ value: "Luke 1", index: 0 }]), + getSubsectionsForMilestone: mockGetSubsectionsForMilestone, + }); + + const renameButtons = await screen.findAllByLabelText("Rename Subsection"); + // Two keyed subsections → two rename affordances; the legacy one is omitted. + expect(renameButtons).toHaveLength(2); + }); + + it("displays the name and keeps the numeric range visible", async () => { + mockGetSubsectionsForMilestone = vi.fn((milestoneIdx: number) => [ + createSubsectionWithKey(`s-${milestoneIdx}-0`, "1-5", "__start__", "Intro"), + ]); + renderMilestoneAccordion({ + milestoneIndex: createMockMilestoneIndex([{ value: "Luke 1", index: 0 }]), + getSubsectionsForMilestone: mockGetSubsectionsForMilestone, + }); + + // Name is primary; label is rendered alongside as a muted suffix. + expect(await screen.findByText("Intro")).toBeInTheDocument(); + expect(screen.getByText("1-5")).toBeInTheDocument(); + }); + + it("posts updateMilestoneSubdivisionName when the subsection rename is saved", async () => { + mockGetSubsectionsForMilestone = vi.fn((milestoneIdx: number) => [ + createSubsectionWithKey(`s-${milestoneIdx}-0`, "1-5", "v1"), + ]); + renderMilestoneAccordion({ + milestoneIndex: createMockMilestoneIndex([{ value: "Luke 1", index: 0 }]), + getSubsectionsForMilestone: mockGetSubsectionsForMilestone, + }); + + const renameBtn = await screen.findByLabelText("Rename Subsection"); + await act(async () => { + fireEvent.click(renameBtn); + }); + + const inputs = await screen.findAllByPlaceholderText("1-5"); + const input = inputs[0] as HTMLInputElement; + await act(async () => { + fireEvent.change(input, { target: { value: "Opening" } }); + }); + + const saveBtn = screen.getByLabelText("Save Subsection Name"); + await act(async () => { + fireEvent.click(saveBtn); + }); + + expect(mockVscode.postMessage).toHaveBeenCalledWith({ + command: "updateMilestoneSubdivisionName", + content: { + milestoneIndex: 0, + subdivisionKey: "v1", + newName: "Opening", + }, + }); + }); + + it("sends an empty string to clear the name override", async () => { + mockGetSubsectionsForMilestone = vi.fn((milestoneIdx: number) => [ + createSubsectionWithKey(`s-${milestoneIdx}-0`, "1-5", "v1", "Opening"), + ]); + renderMilestoneAccordion({ + milestoneIndex: createMockMilestoneIndex([{ value: "Luke 1", index: 0 }]), + getSubsectionsForMilestone: mockGetSubsectionsForMilestone, + }); + + const renameBtn = await screen.findByLabelText("Rename Subsection"); + await act(async () => { + fireEvent.click(renameBtn); + }); + + const input = screen.getByDisplayValue("Opening") as HTMLInputElement; + await act(async () => { + fireEvent.change(input, { target: { value: "" } }); + }); + + const saveBtn = screen.getByLabelText("Save Subsection Name"); + await act(async () => { + fireEvent.click(saveBtn); + }); + + expect(mockVscode.postMessage).toHaveBeenCalledWith({ + command: "updateMilestoneSubdivisionName", + content: { + milestoneIndex: 0, + subdivisionKey: "v1", + newName: "", + }, + }); + }); + + it("does not post anything when the name is unchanged", async () => { + mockGetSubsectionsForMilestone = vi.fn((milestoneIdx: number) => [ + createSubsectionWithKey(`s-${milestoneIdx}-0`, "1-5", "v1", "Opening"), + ]); + renderMilestoneAccordion({ + milestoneIndex: createMockMilestoneIndex([{ value: "Luke 1", index: 0 }]), + getSubsectionsForMilestone: mockGetSubsectionsForMilestone, + }); + + const renameBtn = await screen.findByLabelText("Rename Subsection"); + await act(async () => { + fireEvent.click(renameBtn); + }); + + const saveBtn = screen.getByLabelText("Save Subsection Name"); + await act(async () => { + fireEvent.click(saveBtn); + }); + + const renameCalls = mockVscode.postMessage.mock.calls.filter( + (call: any[]) => call[0]?.command === "updateMilestoneSubdivisionName" + ); + expect(renameCalls).toHaveLength(0); + }); + + it("cancel button leaves the existing name untouched", async () => { + mockGetSubsectionsForMilestone = vi.fn((milestoneIdx: number) => [ + createSubsectionWithKey(`s-${milestoneIdx}-0`, "1-5", "v1", "Opening"), + ]); + renderMilestoneAccordion({ + milestoneIndex: createMockMilestoneIndex([{ value: "Luke 1", index: 0 }]), + getSubsectionsForMilestone: mockGetSubsectionsForMilestone, + }); + + const renameBtn = await screen.findByLabelText("Rename Subsection"); + await act(async () => { + fireEvent.click(renameBtn); + }); + + const input = screen.getByDisplayValue("Opening") as HTMLInputElement; + await act(async () => { + fireEvent.change(input, { target: { value: "Something Else" } }); + }); + + const cancelBtn = screen.getByLabelText("Cancel Rename"); + await act(async () => { + fireEvent.click(cancelBtn); + }); + + const renameCalls = mockVscode.postMessage.mock.calls.filter( + (call: any[]) => call[0]?.command === "updateMilestoneSubdivisionName" + ); + expect(renameCalls).toHaveLength(0); + // Original name still shown (and range-only label preserved) + expect(screen.getByText("Opening")).toBeInTheDocument(); + }); + }); + + describe("Subsection Delete and Reset (source only)", () => { + const makeSubsection = ( + id: string, + label: string, + key: string, + source: "auto" | "custom", + startCellId?: string, + name?: string + ): Subsection => ({ + id, + label, + startIndex: 0, + endIndex: 5, + key, + name, + startCellId, + source, + }); + + // Mirror the provider-produced MilestoneIndex so placement derivation + // reads real data (vs. the lightweight createMockMilestoneIndex). + const createIndexWithSubdivisions = (): MilestoneIndex => ({ + milestones: [ + { + index: 0, + cellIndex: 0, + value: "Luke 1", + cellCount: 30, + subdivisions: [ + { + index: 0, + startRootIndex: 0, + endRootIndex: 5, + key: "__start__", + startCellId: "v1", + source: "auto", + }, + { + index: 1, + startRootIndex: 5, + endRootIndex: 15, + key: "v6", + startCellId: "v6", + source: "custom", + }, + { + index: 2, + startRootIndex: 15, + endRootIndex: 30, + key: "v16", + startCellId: "v16", + source: "custom", + }, + ], + }, + ], + totalCells: 30, + cellsPerPage: 50, + }); + + const mockSubsectionsFromIndex = () => [ + makeSubsection("s-0", "1-5", "__start__", "auto", "v1"), + makeSubsection("s-1", "6-15", "v6", "custom", "v6"), + makeSubsection("s-2", "16-30", "v16", "custom", "v16", "Final"), + ]; + + it("shows remove button only for custom subsections in source", async () => { + renderMilestoneAccordion({ + isSourceText: true, + milestoneIndex: createIndexWithSubdivisions(), + getSubsectionsForMilestone: vi.fn(() => mockSubsectionsFromIndex()), + }); + + const removeButtons = await screen.findAllByLabelText("Remove Subdivision Break"); + // Only the two "custom" subsections expose the delete control. + expect(removeButtons).toHaveLength(2); + }); + + it("does not show remove button on target documents", async () => { + renderMilestoneAccordion({ + isSourceText: false, + milestoneIndex: createIndexWithSubdivisions(), + getSubsectionsForMilestone: vi.fn(() => mockSubsectionsFromIndex()), + }); + + expect(screen.queryByLabelText("Remove Subdivision Break")).not.toBeInTheDocument(); + expect(screen.queryByLabelText("Reset Subdivisions")).not.toBeInTheDocument(); + }); + + it("posts updateMilestoneSubdivisions without the removed break", async () => { + renderMilestoneAccordion({ + isSourceText: true, + milestoneIndex: createIndexWithSubdivisions(), + getSubsectionsForMilestone: vi.fn(() => mockSubsectionsFromIndex()), + }); + + const removeButtons = await screen.findAllByLabelText("Remove Subdivision Break"); + // First one corresponds to the `v6` break. + await act(async () => { + fireEvent.click(removeButtons[0]); + }); + + expect(mockVscode.postMessage).toHaveBeenCalledWith({ + command: "updateMilestoneSubdivisions", + content: { + milestoneIndex: 0, + subdivisions: [{ startCellId: "v16" }], + }, + }); + }); + + it("reset requires two clicks and then posts an empty placement list", async () => { + renderMilestoneAccordion({ + isSourceText: true, + milestoneIndex: createIndexWithSubdivisions(), + getSubsectionsForMilestone: vi.fn(() => mockSubsectionsFromIndex()), + }); + + const resetButton = await screen.findByLabelText("Reset Subdivisions"); + await act(async () => { + fireEvent.click(resetButton); + }); + + // First click arms the confirmation but does not post. + const placementCalls = mockVscode.postMessage.mock.calls.filter( + (call: any[]) => call[0]?.command === "updateMilestoneSubdivisions" + ); + expect(placementCalls).toHaveLength(0); + + // After the click, the button's accessible label swaps to + // "Confirm Reset Subdivisions" to signal the armed state. + const confirmButton = await screen.findByLabelText("Confirm Reset Subdivisions"); + await act(async () => { + fireEvent.click(confirmButton); + }); + + expect(mockVscode.postMessage).toHaveBeenCalledWith({ + command: "updateMilestoneSubdivisions", + content: { + milestoneIndex: 0, + subdivisions: [], + }, + }); + }); + + it("reset button is hidden when no custom breaks exist", () => { + renderMilestoneAccordion({ + isSourceText: true, + milestoneIndex: { + milestones: [ + { + index: 0, + cellIndex: 0, + value: "Luke 1", + cellCount: 5, + subdivisions: [ + { + index: 0, + startRootIndex: 0, + endRootIndex: 5, + key: "__start__", + startCellId: "v1", + source: "auto", + }, + ], + }, + ], + totalCells: 5, + cellsPerPage: 50, + }, + getSubsectionsForMilestone: vi.fn(() => [ + makeSubsection("s-0", "1-5", "__start__", "auto", "v1"), + ]), + }); + + expect(screen.queryByLabelText("Reset Subdivisions")).not.toBeInTheDocument(); + }); + }); + describe("Edit Mode - Accordion Close (no refresh on close)", () => { it("should not send refreshWebviewAfterMilestoneEdits when accordion closes after saving (provider pushes updates immediately on save)", async () => { const { rerender } = renderMilestoneAccordion(); diff --git a/webviews/codex-webviews/src/CodexCellEditor/components/MilestoneAccordion.tsx b/webviews/codex-webviews/src/CodexCellEditor/components/MilestoneAccordion.tsx index 5bd7c604f..21164a0ac 100644 --- a/webviews/codex-webviews/src/CodexCellEditor/components/MilestoneAccordion.tsx +++ b/webviews/codex-webviews/src/CodexCellEditor/components/MilestoneAccordion.tsx @@ -10,7 +10,7 @@ import { import { ProgressDots } from "./ProgressDots"; import { deriveSubsectionPercentages, getProgressDisplay } from "../utils/progressUtils"; import MicrophoneIcon from "../../components/ui/icons/MicrophoneIcon"; -import { Languages, Check, RotateCcw } from "lucide-react"; +import { Languages, Check, RotateCcw, X, Undo2 } from "lucide-react"; import type { Subsection, ProgressPercentages } from "../../lib/types"; import type { MilestoneIndex, MilestoneInfo } from "../../../../../types"; import { VSCodeButton } from "@vscode/webview-ui-toolkit/react"; @@ -92,6 +92,108 @@ export function MilestoneAccordion({ // Local cache of edited milestone values to show changes immediately before webview refresh const [localMilestoneValues, setLocalMilestoneValues] = useState>({}); + // Subsection rename state. `editingSubsection` identifies the single row + // currently in edit mode; `localSubsectionNames` is an optimistic cache so + // saved renames render immediately without waiting for the webview refresh. + // Keyed by `${milestoneIdx}:${subsectionKey}` so renames survive milestone + // expansion/collapse. + const [editingSubsection, setEditingSubsection] = useState< + | { milestoneIdx: number; subsectionIdx: number; key: string; } + | null + >(null); + const [editedSubsectionName, setEditedSubsectionName] = useState(""); + const [originalSubsectionName, setOriginalSubsectionName] = useState(""); + const [localSubsectionNames, setLocalSubsectionNames] = useState>({}); + const subsectionInputRef = useRef(null); + + const getLocalSubsectionName = (milestoneIdx: number, key: string | undefined): string | undefined => { + if (!key) return undefined; + return localSubsectionNames[`${milestoneIdx}:${key}`]; + }; + + // Tracks the milestone whose "Reset breaks" button is in its confirm + // state (the one-click→confirm pattern). Null means no reset is pending. + const [resetConfirmMilestoneIdx, setResetConfirmMilestoneIdx] = useState(null); + const resetConfirmTimeoutRef = useRef(null); + + useEffect(() => { + return () => { + if (resetConfirmTimeoutRef.current !== null) { + window.clearTimeout(resetConfirmTimeoutRef.current); + } + }; + }, []); + + /** + * Rebuilds the milestone's placement list from its resolved subdivisions. + * Only subdivisions at index > 0 with `source === "custom"` and a valid + * `startCellId` correspond to actual placements; the implicit first + * subdivision and arithmetic auto-chunks are derived, not stored. + */ + const getCurrentPlacements = ( + milestone: MilestoneInfo | undefined + ): { startCellId: string; }[] => { + if (!milestone?.subdivisions) return []; + return milestone.subdivisions + .filter((s) => s.index > 0 && s.source === "custom" && !!s.startCellId) + .map((s) => ({ startCellId: s.startCellId as string })); + }; + + const handleDeleteSubsection = ( + e: React.MouseEvent, + milestoneIdx: number, + subsection: Subsection + ) => { + e.stopPropagation(); + if (!isSourceText) return; // Defensive: control should only render on source. + if (!subsection.startCellId || subsection.source !== "custom") return; + const milestone = milestoneIndex?.milestones[milestoneIdx]; + const placements = getCurrentPlacements(milestone).filter( + (p) => p.startCellId !== subsection.startCellId + ); + vscode.postMessage({ + command: "updateMilestoneSubdivisions", + content: { + milestoneIndex: milestoneIdx, + subdivisions: placements, + }, + }); + }; + + const handleResetSubdivisionsClick = ( + e: React.MouseEvent, + milestoneIdx: number + ) => { + e.stopPropagation(); + if (!isSourceText) return; + if (resetConfirmMilestoneIdx !== milestoneIdx) { + // First click → arm the confirmation; auto-disarm after a short + // window so the button never stays "hot" forever. + setResetConfirmMilestoneIdx(milestoneIdx); + if (resetConfirmTimeoutRef.current !== null) { + window.clearTimeout(resetConfirmTimeoutRef.current); + } + resetConfirmTimeoutRef.current = window.setTimeout(() => { + setResetConfirmMilestoneIdx(null); + resetConfirmTimeoutRef.current = null; + }, 3000); + return; + } + // Second click → commit the reset and clear the armed state. + if (resetConfirmTimeoutRef.current !== null) { + window.clearTimeout(resetConfirmTimeoutRef.current); + resetConfirmTimeoutRef.current = null; + } + setResetConfirmMilestoneIdx(null); + vscode.postMessage({ + command: "updateMilestoneSubdivisions", + content: { + milestoneIndex: milestoneIdx, + subdivisions: [], + }, + }); + }; + // Calculate position and dimensions const calculatePositionAndDimensions = () => { if (isOpen && anchorRef.current) { @@ -577,6 +679,63 @@ export function MilestoneAccordion({ } }; + const handleSubsectionEditClick = ( + e: React.MouseEvent, + milestoneIdx: number, + subsectionIdx: number, + subsection: Subsection + ) => { + e.stopPropagation(); + if (!subsection.key) return; + const currentName = + getLocalSubsectionName(milestoneIdx, subsection.key) ?? subsection.name ?? ""; + setEditingSubsection({ milestoneIdx, subsectionIdx, key: subsection.key }); + setOriginalSubsectionName(currentName); + setEditedSubsectionName(currentName); + setTimeout(() => { + subsectionInputRef.current?.focus(); + subsectionInputRef.current?.select(); + }, 0); + }; + + const handleSaveSubsectionName = (e: React.MouseEvent | React.KeyboardEvent) => { + e.stopPropagation(); + if (!editingSubsection) return; + const trimmed = editedSubsectionName.trim(); + if (trimmed !== originalSubsectionName.trim()) { + vscode.postMessage({ + command: "updateMilestoneSubdivisionName", + content: { + milestoneIndex: editingSubsection.milestoneIdx, + subdivisionKey: editingSubsection.key, + newName: trimmed, + }, + }); + // Optimistic cache so the UI reflects the new (or cleared) name + // before the provider refresh arrives. + setLocalSubsectionNames((prev) => ({ + ...prev, + [`${editingSubsection.milestoneIdx}:${editingSubsection.key}`]: trimmed, + })); + } + setEditingSubsection(null); + }; + + const handleRevertSubsectionName = (e: React.MouseEvent | React.KeyboardEvent) => { + e.stopPropagation(); + setEditingSubsection(null); + }; + + const handleSubsectionInputKeyDown = (e: React.KeyboardEvent) => { + if (e.key === "Enter") { + e.preventDefault(); + handleSaveSubsectionName(e); + } else if (e.key === "Escape") { + e.preventDefault(); + handleRevertSubsectionName(e); + } + }; + // Handle milestone expansion - if editing, switch to editing the new milestone const handleMilestoneExpansion = (value: string | null) => { // Update expanded milestone first @@ -831,6 +990,21 @@ export function MilestoneAccordion({ const isActive = isCurrentMilestone && currentSubsectionIndex === subsectionIdx; + const isEditingThisRow = + editingSubsection?.milestoneIdx === + milestoneIdx && + editingSubsection?.subsectionIdx === + subsectionIdx; + // Prefer the optimistic local cache, then + // provider-supplied name, so renames render + // immediately and survive webview refresh. + const cachedLocalName = getLocalSubsectionName( + milestoneIdx, + subsection.key + ); + const displayName = + cachedLocalName ?? subsection.name; + const canRename = !!subsection.key; return (
+ onClick={() => { + if (isEditingThisRow) return; handleSubsectionClick( milestoneIdx, subsectionIdx - ) - } - className={`flex items-center justify-between pr-3 pl-6 py-2 rounded-md cursor-pointer transition-colors ${ - isActive - ? "bg-accent font-semibold" + ); + }} + className={`group flex items-center justify-between pr-3 pl-6 py-2 rounded-md transition-colors ${ + isEditingThisRow + ? "bg-secondary" + : isActive + ? "bg-accent font-semibold cursor-pointer" : unsavedChanges ? "opacity-60 cursor-not-allowed" - : "hover:bg-secondary" + : "hover:bg-secondary cursor-pointer" }`} > - {subsection.label} - + {isEditingThisRow ? ( + + setEditedSubsectionName( + e.target.value + ) + } + onKeyDown={ + handleSubsectionInputKeyDown + } + onClick={(e) => e.stopPropagation()} + placeholder={subsection.label} + className="flex-1 mr-2 bg-transparent border border-[var(--vscode-input-border)] rounded px-2 py-0.5 text-sm focus:outline-none focus:ring-2 focus:ring-[var(--vscode-focusBorder)]" + style={{ + color: "var(--vscode-input-foreground)", + }} + /> + ) : ( + + + {displayName || subsection.label} + + {displayName && ( + + {subsection.label} + + )} + + )} +
+ {isEditingThisRow ? ( + <> + + + + + + + + ) : ( + <> + {canRename && ( + + handleSubsectionEditClick( + e, + milestoneIdx, + subsectionIdx, + subsection + ) + } + className="opacity-0 group-hover:opacity-100 focus:opacity-100 transition-opacity" + > + + + )} + {isSourceText && + subsection.source === + "custom" && + subsection.startCellId && ( + + handleDeleteSubsection( + e, + milestoneIdx, + subsection + ) + } + className="opacity-0 group-hover:opacity-100 focus:opacity-100 transition-opacity" + > + + + )} + + )} + {!isEditingThisRow && ( + + )} +
); })} + {isSourceText && + subsections.some( + (s) => s.source === "custom" + ) && ( +
+ +
+ )} From db649b1e207a83a0c2f289b8c38ebcc06573c191 Mon Sep 17 00:00:00 2001 From: Luke-Bilhorn Date: Fri, 24 Apr 2026 01:32:59 -0500 Subject: [PATCH 05/11] Add addMilestoneSubdivisionAnchor handler and refactor commit pipeline Implements the addAnchor message handler and extracts a shared commit pipeline in codexCellEditorMessagehandling.ts so that all subdivision mutation operations (add, delete, rename, reset) go through one consistent save/notify path. Tests added for the add-anchor handler. --- .../codexCellEditorMessagehandling.ts | 367 ++++++++++++------ src/test/suite/milestoneSubdivisions.test.ts | 165 ++++++++ types/index.d.ts | 18 + 3 files changed, 430 insertions(+), 120 deletions(-) diff --git a/src/providers/codexCellEditorProvider/codexCellEditorMessagehandling.ts b/src/providers/codexCellEditorProvider/codexCellEditorMessagehandling.ts index e376ae327..62133b5ec 100644 --- a/src/providers/codexCellEditorProvider/codexCellEditorMessagehandling.ts +++ b/src/providers/codexCellEditorProvider/codexCellEditorMessagehandling.ts @@ -185,6 +185,161 @@ export async function sendMilestoneRefreshToWebview( } } +/** + * Shared worker for all handlers that need to persist a new placement list + * onto a source milestone. Performs validation (source-only, milestone must + * exist, anchors must refer to real root cells), saves the source document, + * mirrors the placements (names stripped) onto the paired target document, + * and refreshes the originating webview. + * + * Callers pass `logPrefix` / `errorPrefix` so their diagnostic output stays + * attributable; the sanitization/mirror/refresh pipeline is identical across + * callers. + */ +async function commitMilestoneSubdivisionPlacements({ + document, + webviewPanel, + provider, + milestoneIndex, + incomingPlacements, + logPrefix, + errorPrefix, +}: { + document: CodexCellDocument; + webviewPanel: vscode.WebviewPanel; + provider: CodexCellEditorProvider; + milestoneIndex: number; + incomingPlacements: { startCellId: string; name?: string; }[]; + logPrefix: string; + errorPrefix: string; +}): Promise { + if (!isSourceFileFlexible(document.uri)) { + console.warn( + `${logPrefix} Rejected write from non-source document:`, + document.uri.toString() + ); + vscode.window.showWarningMessage( + "Subdivision breaks can only be edited from the source file." + ); + return; + } + + const milestoneIndexObj = document.buildMilestoneIndex(); + const milestone = milestoneIndexObj.milestones[milestoneIndex]; + if (!milestone) { + console.error(`${logPrefix} Milestone not found at index`, milestoneIndex); + vscode.window.showErrorMessage( + `${errorPrefix}: milestone not found at index ${milestoneIndex}` + ); + return; + } + + const sourceMilestoneCell = document.getCellByIndex(milestone.cellIndex); + if ( + !sourceMilestoneCell || + sourceMilestoneCell.metadata?.type !== CodexCellTypes.MILESTONE || + !sourceMilestoneCell.metadata?.id + ) { + console.error( + `${logPrefix} Cell at index is not a valid milestone cell`, + milestone.cellIndex + ); + vscode.window.showErrorMessage(`${errorPrefix}: invalid milestone cell.`); + return; + } + + // Validate every anchor refers to a current root content cell inside the + // milestone. Stale anchors are normally pruned at resolve time, but we + // still hard-reject invalid writes here so bugs surface as loud errors + // rather than silent data loss. + const validRootIds = new Set(document.getRootContentCellIdsForMilestone(milestoneIndex)); + const seen = new Set(); + const sanitized: { startCellId: string; name?: string; }[] = []; + for (const placement of incomingPlacements) { + if (!placement || typeof placement.startCellId !== "string") continue; + if (!validRootIds.has(placement.startCellId)) { + console.warn(`${logPrefix} Dropping unknown startCellId:`, placement.startCellId); + continue; + } + if (seen.has(placement.startCellId)) continue; + seen.add(placement.startCellId); + const entry: { startCellId: string; name?: string; } = { + startCellId: placement.startCellId, + }; + if (typeof placement.name === "string" && placement.name.length > 0) { + entry.name = placement.name; + } + sanitized.push(entry); + } + + const sourceCellId = sourceMilestoneCell.metadata.id; + const cancellationToken = new vscode.CancellationTokenSource().token; + + try { + await document.refreshAuthor(); + document.updateCellData(sourceCellId, { subdivisions: sanitized }); + await provider.saveCustomDocument(document, cancellationToken); + } catch (error) { + console.error(`${logPrefix} Failed to update source:`, error); + vscode.window.showErrorMessage( + `${errorPrefix}: ${error instanceof Error ? error.message : String(error)}` + ); + return; + } + + // Mirror placements (without names) to the paired target document. + try { + const pairedUri = provider.getPairedNotebookUri(document.uri); + if (pairedUri) { + const targetDocument = await provider.getOrOpenDocumentForUri(pairedUri); + const targetMilestoneIndexObj = targetDocument.buildMilestoneIndex(); + const targetMilestone = targetMilestoneIndexObj.milestones[milestoneIndex]; + if (!targetMilestone) { + console.warn( + `${logPrefix} Target has no milestone at index, skipping mirror:`, + milestoneIndex + ); + } else { + // Positionally-paired milestones should share root content + // cell IDs. If they diverge we skip the mirror so source + // anchors don't latch onto unrelated target cells. + const sourceRootIds = document.getRootContentCellIdsForMilestone(milestoneIndex); + const targetRootIds = targetDocument.getRootContentCellIdsForMilestone(milestoneIndex); + const rootsMatch = + sourceRootIds.length === targetRootIds.length && + sourceRootIds.every((id, i) => id === targetRootIds[i]); + if (!rootsMatch) { + console.warn(`${logPrefix} Source/target milestones diverge; skipping mirror.`, { + milestoneIndex, + sourceLength: sourceRootIds.length, + targetLength: targetRootIds.length, + }); + } else { + const targetMilestoneCell = targetDocument.getCellByIndex( + targetMilestone.cellIndex + ); + if (targetMilestoneCell?.metadata?.id) { + const mirroredPlacements = sanitized.map((p) => ({ + startCellId: p.startCellId, + })); + await targetDocument.refreshAuthor(); + targetDocument.updateCellData(targetMilestoneCell.metadata.id, { + subdivisions: mirroredPlacements, + }); + await provider.saveCustomDocument(targetDocument, cancellationToken); + } + } + } + } + } catch (mirrorError) { + // Mirror failures are non-fatal: the source edit has already + // succeeded. Log and continue so the user can retry later. + console.error(`${logPrefix} Failed to mirror to target:`, mirrorError); + } + + await sendMilestoneRefreshToWebview(document, webviewPanel, provider); +} + /** * Helper function to get the audio file path for a cell * Checks metadata attachments first, then falls back to filesystem lookup @@ -316,7 +471,8 @@ async function getAudioFilePathForCell( return null; } -// Individual message handlers +// Individual message handlers. Exported (as a re-export below) so tests can +// invoke handlers directly without constructing a full webview round-trip. const messageHandlers: Record Promise | void> = { webviewReady: () => { /* no-op */ }, setAutoDownloadAudioOnOpen: async ({ event, document, webviewPanel, provider }) => { @@ -1275,155 +1431,119 @@ const messageHandlers: Record Promise { const typedEvent = event as Extract; debug("updateMilestoneSubdivisions message received", { event }); + await commitMilestoneSubdivisionPlacements({ + document, + webviewPanel, + provider, + milestoneIndex: typedEvent.content.milestoneIndex, + incomingPlacements: typedEvent.content.subdivisions ?? [], + logPrefix: "[updateMilestoneSubdivisions]", + errorPrefix: "Failed to update subdivisions", + }); + }, - // Placements are source-authoritative. Reject writes originating from a - // target (.codex) document so the source stays the single source of truth. + addMilestoneSubdivisionAnchor: async ({ event, document, webviewPanel, provider }) => { + const typedEvent = event as Extract< + EditorPostMessages, + { command: "addMilestoneSubdivisionAnchor"; } + >; + debug("addMilestoneSubdivisionAnchor message received", { event }); + + // Placements are source-authoritative. Reject writes from target so the + // source stays the single source of truth; the UI hides the control too. if (!isSourceFileFlexible(document.uri)) { console.warn( - "[updateMilestoneSubdivisions] Rejected write from non-source document:", + "[addMilestoneSubdivisionAnchor] Rejected write from non-source document:", document.uri.toString() ); vscode.window.showWarningMessage( - "Subdivision breaks can only be edited from the source file." + "Subdivision breaks can only be added from the source file." ); return; } - const { milestoneIndex, subdivisions: incomingPlacements } = typedEvent.content; + const { milestoneIndex, cellNumber } = typedEvent.content; - const milestoneIndexObj = document.buildMilestoneIndex(); - const milestone = milestoneIndexObj.milestones[milestoneIndex]; - if (!milestone) { - console.error("[updateMilestoneSubdivisions] Milestone not found at index", milestoneIndex); + // Resolve `cellNumber` (1-based) to a concrete root cell ID. The valid + // range is [2, rootIds.length]: splitting at cell 1 would duplicate the + // implicit first subdivision, and splitting beyond the last cell has + // nowhere to go. We reject both cases rather than silently clamping so + // the caller can surface a clear error. + const rootIds = document.getRootContentCellIdsForMilestone(milestoneIndex); + if (!Array.isArray(rootIds) || rootIds.length === 0) { + console.error( + "[addMilestoneSubdivisionAnchor] No root cells for milestone", + milestoneIndex + ); vscode.window.showErrorMessage( - `Failed to update subdivisions: milestone not found at index ${milestoneIndex}` + `Failed to add subdivision break: milestone ${milestoneIndex} has no content cells.` ); return; } - - const sourceMilestoneCell = document.getCellByIndex(milestone.cellIndex); if ( - !sourceMilestoneCell || - sourceMilestoneCell.metadata?.type !== CodexCellTypes.MILESTONE || - !sourceMilestoneCell.metadata?.id + typeof cellNumber !== "number" || + !Number.isFinite(cellNumber) || + cellNumber < 2 || + cellNumber > rootIds.length ) { - console.error( - "[updateMilestoneSubdivisions] Cell at index is not a valid milestone cell", - milestone.cellIndex + console.warn( + "[addMilestoneSubdivisionAnchor] cellNumber out of range:", + { cellNumber, validRange: [2, rootIds.length] } + ); + vscode.window.showWarningMessage( + `Cannot split here — pick a cell between 2 and ${rootIds.length}.` ); - vscode.window.showErrorMessage("Failed to update subdivisions: invalid milestone cell."); return; } - // Validate every anchor refers to a current root content cell inside the - // milestone. Stale anchors are normally pruned at resolve time, but we - // still hard-reject invalid writes here so callers can surface errors. - const validRootIds = new Set(document.getRootContentCellIdsForMilestone(milestoneIndex)); - const seen = new Set(); - const sanitized: typeof incomingPlacements = []; - for (const placement of incomingPlacements ?? []) { - if (!placement || typeof placement.startCellId !== "string") continue; - if (!validRootIds.has(placement.startCellId)) { - console.warn( - "[updateMilestoneSubdivisions] Dropping unknown startCellId:", - placement.startCellId - ); - continue; - } - if (seen.has(placement.startCellId)) continue; - seen.add(placement.startCellId); - // Intentionally strip names from mirrored placements; names live on - // the separate `subdivisionNames` map so that source and target can - // be named independently (per design spec). - const entry: { startCellId: string; name?: string; } = { - startCellId: placement.startCellId, - }; - if (typeof placement.name === "string" && placement.name.length > 0) { - entry.name = placement.name; - } - sanitized.push(entry); - } + const newStartCellId = rootIds[cellNumber - 1]; - const sourceCellId = sourceMilestoneCell.metadata.id; - const cancellationToken = new vscode.CancellationTokenSource().token; - - try { - await document.refreshAuthor(); - document.updateCellData(sourceCellId, { subdivisions: sanitized }); - await provider.saveCustomDocument(document, cancellationToken); - debug( - `[updateMilestoneSubdivisions] Updated source subdivisions for milestone ${milestoneIndex}`, - { count: sanitized.length } + // Read existing placements directly from the source milestone's + // metadata (not from the resolved subdivisions) so we preserve any + // source-side names attached to existing placements verbatim. + const milestoneIndexObj = document.buildMilestoneIndex(); + const milestone = milestoneIndexObj.milestones[milestoneIndex]; + if (!milestone) { + console.error( + "[addMilestoneSubdivisionAnchor] Milestone not found at index", + milestoneIndex ); - } catch (error) { - console.error("[updateMilestoneSubdivisions] Failed to update source:", error); vscode.window.showErrorMessage( - `Failed to update subdivisions: ${error instanceof Error ? error.message : String(error)}` + `Failed to add subdivision break: milestone not found at index ${milestoneIndex}` ); return; } - - // Mirror placements (without names) to the paired target document. - try { - const pairedUri = provider.getPairedNotebookUri(document.uri); - if (pairedUri) { - const targetDocument = await provider.getOrOpenDocumentForUri(pairedUri); - const targetMilestoneIndexObj = targetDocument.buildMilestoneIndex(); - const targetMilestone = targetMilestoneIndexObj.milestones[milestoneIndex]; - if (!targetMilestone) { - console.warn( - "[updateMilestoneSubdivisions] Target has no milestone at index, skipping mirror:", - milestoneIndex - ); - } else { - // Sanity check: positionally-paired milestones should share - // root content cell IDs. If they diverge, skip the mirror to - // avoid attaching source anchors to unrelated target cells. - const sourceRootIds = document.getRootContentCellIdsForMilestone(milestoneIndex); - const targetRootIds = targetDocument.getRootContentCellIdsForMilestone(milestoneIndex); - const rootsMatch = - sourceRootIds.length === targetRootIds.length && - sourceRootIds.every((id, i) => id === targetRootIds[i]); - if (!rootsMatch) { - console.warn( - "[updateMilestoneSubdivisions] Source/target milestones diverge; skipping mirror.", - { - milestoneIndex, - sourceLength: sourceRootIds.length, - targetLength: targetRootIds.length, - } - ); - } else { - const targetMilestoneCell = targetDocument.getCellByIndex(targetMilestone.cellIndex); - if (targetMilestoneCell?.metadata?.id) { - // Mirror placements with names stripped — target uses - // its own `subdivisionNames` for local display. - const mirroredPlacements = sanitized.map((p) => ({ - startCellId: p.startCellId, - })); - await targetDocument.refreshAuthor(); - targetDocument.updateCellData(targetMilestoneCell.metadata.id, { - subdivisions: mirroredPlacements, - }); - await provider.saveCustomDocument(targetDocument, cancellationToken); - debug( - "[updateMilestoneSubdivisions] Mirrored subdivisions to target:", - { uri: pairedUri.toString(), count: mirroredPlacements.length } - ); - } - } - } - } - } catch (mirrorError) { - // Mirror failures are non-fatal: the source edit has already - // succeeded. Log and continue so the user can retry later. - console.error( - "[updateMilestoneSubdivisions] Failed to mirror to target:", - mirrorError + const sourceMilestoneCell = document.getCellByIndex(milestone.cellIndex); + const existingPlacements = + (sourceMilestoneCell?.metadata?.data as + | { subdivisions?: { startCellId: string; name?: string; }[]; } + | undefined)?.subdivisions ?? []; + + // Idempotence: if the anchor already exists we quietly no-op so + // repeated clicks don't bounce against validation errors. + if (existingPlacements.some((p) => p.startCellId === newStartCellId)) { + debug( + "[addMilestoneSubdivisionAnchor] Anchor already present; no-op.", + { milestoneIndex, newStartCellId } ); + await sendMilestoneRefreshToWebview(document, webviewPanel, provider); + return; } - await sendMilestoneRefreshToWebview(document, webviewPanel, provider); + const nextPlacements = [ + ...existingPlacements, + { startCellId: newStartCellId }, + ]; + + await commitMilestoneSubdivisionPlacements({ + document, + webviewPanel, + provider, + milestoneIndex, + incomingPlacements: nextPlacements, + logPrefix: "[addMilestoneSubdivisionAnchor]", + errorPrefix: "Failed to add subdivision break", + }); }, updateMilestoneSubdivisionName: async ({ event, document, webviewPanel, provider }) => { @@ -4028,3 +4148,10 @@ export async function scanForAudioAttachments( return audioAttachments; } + +/** + * Test-only re-export of the internal handler map. Production code should + * continue to route messages through `handleMessages`; this hook just lets + * unit tests exercise a single handler without standing up a full webview. + */ +export const __testOnlyMessageHandlers = messageHandlers; diff --git a/src/test/suite/milestoneSubdivisions.test.ts b/src/test/suite/milestoneSubdivisions.test.ts index b5406c087..ddfe806bf 100644 --- a/src/test/suite/milestoneSubdivisions.test.ts +++ b/src/test/suite/milestoneSubdivisions.test.ts @@ -8,6 +8,7 @@ import { findSubdivisionIndexForRoot, resolveSubdivisions, } from "../../providers/codexCellEditorProvider/utils/subdivisionUtils"; +import { __testOnlyMessageHandlers } from "../../providers/codexCellEditorProvider/codexCellEditorMessagehandling"; import { swallowDuplicateCommandRegistrations, createTempCodexFile, @@ -425,6 +426,170 @@ suite("Milestone Subdivisions Test Suite", () => { ); }); + // ----------------------------------------------------------------- + // addMilestoneSubdivisionAnchor handler — resolves cellNumber → cellId + // server-side and delegates to the shared commit pipeline. + // ----------------------------------------------------------------- + suite("addMilestoneSubdivisionAnchor handler", () => { + /** + * Mint a fake source URI so the handler's `isSourceFileFlexible` + * check passes without us needing to actually back the document + * with a .bible or .source file on disk. + */ + function stampSourceUri(document: CodexCellDocument) { + // `uri` is a plain public field (see CodexCellDocument), + // so a direct assignment is enough to override it. + (document as any).uri = vscode.Uri.parse("file:///test.source"); + } + + /** + * Stubs the provider touch-points the shared commit helper uses so + * the handler can run without a real webview round-trip: + * - `saveCustomDocument` becomes a no-op (we assert in-memory only) + * - `getPairedNotebookUri` returns null (no mirror step) + * - `refreshWebview` is swallowed + * - `currentMilestoneSubsectionMap` is empty, taking the simple + * refresh path in `sendMilestoneRefreshToWebview`. + */ + function stubProviderForHandlerTest(p: CodexCellEditorProvider) { + sinon.stub(p, "saveCustomDocument").resolves(); + sinon.stub(p, "getPairedNotebookUri").returns(null); + sinon.stub(p, "refreshWebview").resolves(); + (p as any).currentMilestoneSubsectionMap = new Map(); + // Author hook is a no-op for the integration path. + sinon.stub(CodexCellDocument.prototype as any, "refreshAuthor").resolves(); + } + + async function invokeAddAnchor({ + document, + milestoneIndex, + cellNumber, + }: { + document: CodexCellDocument; + milestoneIndex: number; + cellNumber: number; + }): Promise { + const handler = __testOnlyMessageHandlers["addMilestoneSubdivisionAnchor"]; + assert.ok(handler, "addMilestoneSubdivisionAnchor handler must be registered"); + await handler({ + event: { + command: "addMilestoneSubdivisionAnchor", + content: { milestoneIndex, cellNumber }, + } as any, + document, + webviewPanel: {} as any, + provider, + updateWebview: () => { /* no-op */ }, + }); + } + + test("adds a new anchor at the Nth root cell", async () => { + const cells = buildCellsWithSubdivisions(); + const document = await createDocumentWithCells(cells); + stampSourceUri(document); + stubProviderForHandlerTest(provider); + + await invokeAddAnchor({ document, milestoneIndex: 0, cellNumber: 6 }); + + // The 6th root cell is v6 (array positions 0..9 → cell ids v1..v10). + const index = document.buildMilestoneIndex(50); + const subs = index.milestones[0].subdivisions ?? []; + assert.strictEqual(subs.length, 2, "Expected exactly one new break → two subsections"); + assert.strictEqual(subs[1].startCellId, "v6"); + assert.strictEqual(subs[1].source, "custom"); + }); + + test("appends anchor to existing placements (preserves source names)", async () => { + const cells = buildCellsWithSubdivisions([ + { startCellId: "v4", name: "Middle" }, + ]); + const document = await createDocumentWithCells(cells); + stampSourceUri(document); + stubProviderForHandlerTest(provider); + + await invokeAddAnchor({ document, milestoneIndex: 0, cellNumber: 8 }); + + const index = document.buildMilestoneIndex(50); + const subs = index.milestones[0].subdivisions ?? []; + // Expect 3 subsections: v1..v3, v4..v7, v8..v10. + assert.strictEqual(subs.length, 3); + assert.strictEqual(subs[1].startCellId, "v4"); + assert.strictEqual(subs[1].name, "Middle", "Existing source-side name is preserved"); + assert.strictEqual(subs[2].startCellId, "v8"); + assert.strictEqual(subs[2].source, "custom"); + }); + + test("cellNumber at the first cell is rejected (no-op)", async () => { + const cells = buildCellsWithSubdivisions(); + const document = await createDocumentWithCells(cells); + stampSourceUri(document); + stubProviderForHandlerTest(provider); + + await invokeAddAnchor({ document, milestoneIndex: 0, cellNumber: 1 }); + + const index = document.buildMilestoneIndex(50); + const subs = index.milestones[0].subdivisions ?? []; + // No breaks added — still the lone auto subdivision. + assert.strictEqual(subs.length, 1); + assert.strictEqual(subs[0].source, "auto"); + }); + + test("cellNumber beyond last cell is rejected", async () => { + const cells = buildCellsWithSubdivisions(); + const document = await createDocumentWithCells(cells); + stampSourceUri(document); + stubProviderForHandlerTest(provider); + + await invokeAddAnchor({ document, milestoneIndex: 0, cellNumber: 99 }); + + const index = document.buildMilestoneIndex(50); + const subs = index.milestones[0].subdivisions ?? []; + assert.strictEqual(subs.length, 1, "Out-of-range cellNumber must not produce a break"); + }); + + test("idempotent: re-adding the same anchor does not duplicate", async () => { + const cells = buildCellsWithSubdivisions([{ startCellId: "v6" }]); + const document = await createDocumentWithCells(cells); + stampSourceUri(document); + stubProviderForHandlerTest(provider); + + // v6 is already the 6th root cell; adding again should no-op. + await invokeAddAnchor({ document, milestoneIndex: 0, cellNumber: 6 }); + + const index = document.buildMilestoneIndex(50); + const subs = index.milestones[0].subdivisions ?? []; + assert.strictEqual(subs.length, 2, "Anchor set must remain the same size"); + const anchors = subs + .filter((s) => s.source === "custom" && s.index > 0) + .map((s) => s.startCellId); + assert.deepStrictEqual(anchors, ["v6"]); + }); + + test("rejects writes from non-source documents", async () => { + const cells = buildCellsWithSubdivisions(); + const document = await createDocumentWithCells(cells); + // Leave URI pointing at the temp .codex file → should be rejected. + stubProviderForHandlerTest(provider); + const warnStub = sinon.stub(vscode.window, "showWarningMessage"); + + await invokeAddAnchor({ document, milestoneIndex: 0, cellNumber: 5 }); + + assert.strictEqual( + warnStub.calledWith( + "Subdivision breaks can only be added from the source file." + ), + true, + "Non-source writes must surface a warning" + ); + const index = document.buildMilestoneIndex(50); + assert.strictEqual( + index.milestones[0].subdivisions?.length ?? 0, + 1, + "Non-source writes must not mutate the document" + ); + }); + }); + test("legacy behavior preserved when no subdivisions on milestone", async () => { // Sanity check: 125 cells with cellsPerPage=50 → 3 subsections and each page // sized exactly as before the refactor. diff --git a/types/index.d.ts b/types/index.d.ts index 1ae9e606d..3dd284e61 100644 --- a/types/index.d.ts +++ b/types/index.d.ts @@ -627,6 +627,24 @@ export type EditorPostMessages = newName: string; }; } + | { + /** + * Add a new subdivision break anchored at the Nth root content cell of the + * given milestone (1-based, so `cellNumber=11` means "split starting at the + * 11th content cell of this milestone"). The provider resolves the number + * to a `startCellId`, merges it into the existing placement list, and + * mirrors the updated list to the paired target document. + * + * Writes are rejected from non-source documents at the provider layer; + * callers should also hide the UI on target. + */ + command: "addMilestoneSubdivisionAnchor"; + content: { + milestoneIndex: number; + /** 1-based position of the split point within the milestone's root cells. */ + cellNumber: number; + }; + } | { command: "refreshWebviewAfterMilestoneEdits"; content?: Record; From f590a301ec1c08c71212ed70edf15620aecad95e Mon Sep 17 00:00:00 2001 From: Luke-Bilhorn Date: Fri, 24 Apr 2026 01:33:07 -0500 Subject: [PATCH 06/11] =?UTF-8?q?Add=20inline=20"Add=20break=20at=20cell?= =?UTF-8?q?=E2=80=A6"=20form=20to=20the=20milestone=20accordion?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds an expandable form inside each milestone section (source side only) that lets translators insert a subdivision anchor at a specific cell. Dispatches addMilestoneSubdivisionAnchor to the provider. Tests cover the form's open/submit/cancel behavior. --- .../components/MilestoneAccordion.test.tsx | 189 ++++++++++++ .../components/MilestoneAccordion.tsx | 272 +++++++++++++++--- 2 files changed, 424 insertions(+), 37 deletions(-) diff --git a/webviews/codex-webviews/src/CodexCellEditor/components/MilestoneAccordion.test.tsx b/webviews/codex-webviews/src/CodexCellEditor/components/MilestoneAccordion.test.tsx index acad726f9..eacc378db 100644 --- a/webviews/codex-webviews/src/CodexCellEditor/components/MilestoneAccordion.test.tsx +++ b/webviews/codex-webviews/src/CodexCellEditor/components/MilestoneAccordion.test.tsx @@ -64,6 +64,7 @@ vi.mock("lucide-react", () => ({ RotateCcw: () =>
RotateCcw
, X: () =>
X
, Undo2: () =>
Undo2
, + Plus: () =>
Plus
, })); vi.mock("../../components/ui/icons/MicrophoneIcon", () => ({ @@ -1124,6 +1125,194 @@ describe("MilestoneAccordion - Milestone Editing", () => { }); }); + describe("Add Subdivision Break (source only)", () => { + const makeSubsection = ( + id: string, + label: string, + key: string, + source: "auto" | "custom", + endIndex: number, + startCellId?: string + ): Subsection => ({ + id, + label, + startIndex: 0, + endIndex, + key, + startCellId, + source, + }); + + const createSplittableIndex = (totalRootCells: number): MilestoneIndex => ({ + milestones: [ + { + index: 0, + cellIndex: 0, + value: "Luke 1", + cellCount: totalRootCells, + subdivisions: [ + { + index: 0, + startRootIndex: 0, + endRootIndex: totalRootCells, + key: "__start__", + startCellId: "v1", + source: "auto", + }, + ], + }, + ], + totalCells: totalRootCells, + cellsPerPage: 50, + }); + + const singleAutoSubsection = (endIndex: number) => [ + makeSubsection("s-0", `1-${endIndex}`, "__start__", "auto", endIndex, "v1"), + ]; + + it("shows the 'Add break…' button on source when the milestone has at least 2 cells", async () => { + renderMilestoneAccordion({ + isSourceText: true, + milestoneIndex: createSplittableIndex(10), + getSubsectionsForMilestone: vi.fn(() => singleAutoSubsection(10)), + }); + + const addBreakButton = await screen.findByLabelText("Add Subdivision Break"); + expect(addBreakButton).toBeInTheDocument(); + }); + + it("does not show the 'Add break…' button on target", () => { + renderMilestoneAccordion({ + isSourceText: false, + milestoneIndex: createSplittableIndex(10), + getSubsectionsForMilestone: vi.fn(() => singleAutoSubsection(10)), + }); + + expect(screen.queryByLabelText("Add Subdivision Break")).not.toBeInTheDocument(); + }); + + it("hides 'Add break…' when the milestone has only one cell (can't split)", () => { + renderMilestoneAccordion({ + isSourceText: true, + milestoneIndex: createSplittableIndex(1), + getSubsectionsForMilestone: vi.fn(() => singleAutoSubsection(1)), + }); + + expect(screen.queryByLabelText("Add Subdivision Break")).not.toBeInTheDocument(); + }); + + it("posts addMilestoneSubdivisionAnchor with the entered cellNumber", async () => { + renderMilestoneAccordion({ + isSourceText: true, + milestoneIndex: createSplittableIndex(10), + getSubsectionsForMilestone: vi.fn(() => singleAutoSubsection(10)), + }); + + await act(async () => { + fireEvent.click(screen.getByLabelText("Add Subdivision Break")); + }); + + const input = await screen.findByLabelText("Cell number for new break"); + await act(async () => { + fireEvent.change(input, { target: { value: "5" } }); + }); + + // The submit button re-uses the "Add Subdivision Break" aria-label + // once the form is open (it IS the add action). + await act(async () => { + fireEvent.click(screen.getByLabelText("Add Subdivision Break")); + }); + + expect(mockVscode.postMessage).toHaveBeenCalledWith({ + command: "addMilestoneSubdivisionAnchor", + content: { + milestoneIndex: 0, + cellNumber: 5, + }, + }); + }); + + it("surfaces an inline error for out-of-range input and does not post", async () => { + renderMilestoneAccordion({ + isSourceText: true, + milestoneIndex: createSplittableIndex(10), + getSubsectionsForMilestone: vi.fn(() => singleAutoSubsection(10)), + }); + + await act(async () => { + fireEvent.click(screen.getByLabelText("Add Subdivision Break")); + }); + + const input = await screen.findByLabelText("Cell number for new break"); + await act(async () => { + fireEvent.change(input, { target: { value: "99" } }); + }); + await act(async () => { + fireEvent.click(screen.getByLabelText("Add Subdivision Break")); + }); + + // Error text is announced via aria-live. + expect( + screen.getByText("Enter a number between 2 and 10.") + ).toBeInTheDocument(); + const placementCalls = mockVscode.postMessage.mock.calls.filter( + (call: any[]) => call[0]?.command === "addMilestoneSubdivisionAnchor" + ); + expect(placementCalls).toHaveLength(0); + }); + + it("does not post when submitting an empty cellNumber", async () => { + renderMilestoneAccordion({ + isSourceText: true, + milestoneIndex: createSplittableIndex(10), + getSubsectionsForMilestone: vi.fn(() => singleAutoSubsection(10)), + }); + + await act(async () => { + fireEvent.click(screen.getByLabelText("Add Subdivision Break")); + }); + + await act(async () => { + fireEvent.click(screen.getByLabelText("Add Subdivision Break")); + }); + + const placementCalls = mockVscode.postMessage.mock.calls.filter( + (call: any[]) => call[0]?.command === "addMilestoneSubdivisionAnchor" + ); + expect(placementCalls).toHaveLength(0); + }); + + it("cancel button closes the form without posting", async () => { + renderMilestoneAccordion({ + isSourceText: true, + milestoneIndex: createSplittableIndex(10), + getSubsectionsForMilestone: vi.fn(() => singleAutoSubsection(10)), + }); + + await act(async () => { + fireEvent.click(screen.getByLabelText("Add Subdivision Break")); + }); + + const input = await screen.findByLabelText("Cell number for new break"); + await act(async () => { + fireEvent.change(input, { target: { value: "5" } }); + }); + await act(async () => { + fireEvent.click(screen.getByLabelText("Cancel Add Break")); + }); + + // Form closed → input gone, trigger button restored. + expect( + screen.queryByLabelText("Cell number for new break") + ).not.toBeInTheDocument(); + expect(screen.getByLabelText("Add Subdivision Break")).toBeInTheDocument(); + const placementCalls = mockVscode.postMessage.mock.calls.filter( + (call: any[]) => call[0]?.command === "addMilestoneSubdivisionAnchor" + ); + expect(placementCalls).toHaveLength(0); + }); + }); + describe("Edit Mode - Accordion Close (no refresh on close)", () => { it("should not send refreshWebviewAfterMilestoneEdits when accordion closes after saving (provider pushes updates immediately on save)", async () => { const { rerender } = renderMilestoneAccordion(); diff --git a/webviews/codex-webviews/src/CodexCellEditor/components/MilestoneAccordion.tsx b/webviews/codex-webviews/src/CodexCellEditor/components/MilestoneAccordion.tsx index 21164a0ac..dfc1d91c8 100644 --- a/webviews/codex-webviews/src/CodexCellEditor/components/MilestoneAccordion.tsx +++ b/webviews/codex-webviews/src/CodexCellEditor/components/MilestoneAccordion.tsx @@ -10,7 +10,7 @@ import { import { ProgressDots } from "./ProgressDots"; import { deriveSubsectionPercentages, getProgressDisplay } from "../utils/progressUtils"; import MicrophoneIcon from "../../components/ui/icons/MicrophoneIcon"; -import { Languages, Check, RotateCcw, X, Undo2 } from "lucide-react"; +import { Languages, Check, RotateCcw, X, Undo2, Plus } from "lucide-react"; import type { Subsection, ProgressPercentages } from "../../lib/types"; import type { MilestoneIndex, MilestoneInfo } from "../../../../../types"; import { VSCodeButton } from "@vscode/webview-ui-toolkit/react"; @@ -116,6 +116,14 @@ export function MilestoneAccordion({ const [resetConfirmMilestoneIdx, setResetConfirmMilestoneIdx] = useState(null); const resetConfirmTimeoutRef = useRef(null); + // "Add break" form state. Only one milestone can have the form open at a + // time; the cell-number field is a string so we can accept and validate + // partial input (empty, non-numeric, out of range) before posting. + const [addBreakMilestoneIdx, setAddBreakMilestoneIdx] = useState(null); + const [addBreakCellNumber, setAddBreakCellNumber] = useState(""); + const [addBreakError, setAddBreakError] = useState(""); + const addBreakInputRef = useRef(null); + useEffect(() => { return () => { if (resetConfirmTimeoutRef.current !== null) { @@ -124,6 +132,14 @@ export function MilestoneAccordion({ }; }, []); + // When the add-break form opens, focus the number input so keyboard-first + // users can type immediately. + useEffect(() => { + if (addBreakMilestoneIdx !== null) { + addBreakInputRef.current?.focus(); + } + }, [addBreakMilestoneIdx]); + /** * Rebuilds the milestone's placement list from its resolved subdivisions. * Only subdivisions at index > 0 with `source === "custom"` and a valid @@ -194,6 +210,72 @@ export function MilestoneAccordion({ }); }; + /** + * Largest valid `cellNumber` for an add-break request in the given + * milestone. Valid range is [2, totalRootCells]; we derive the upper + * bound from the last resolved subsection's `endIndex` (which is a root + * index, matching `SubdivisionInfo.endRootIndex` one-to-one). + */ + const getMaxCellNumberForMilestone = (subsections: Subsection[]): number => { + if (!subsections.length) return 0; + return subsections[subsections.length - 1].endIndex; + }; + + const handleOpenAddBreak = ( + e: React.MouseEvent, + milestoneIdx: number + ) => { + e.stopPropagation(); + if (!isSourceText) return; + setAddBreakMilestoneIdx(milestoneIdx); + setAddBreakCellNumber(""); + setAddBreakError(""); + }; + + const handleCancelAddBreak = (e?: React.MouseEvent) => { + e?.stopPropagation(); + setAddBreakMilestoneIdx(null); + setAddBreakCellNumber(""); + setAddBreakError(""); + }; + + const handleSubmitAddBreak = ( + e: React.MouseEvent | React.FormEvent, + milestoneIdx: number, + maxCellNumber: number + ) => { + e.preventDefault(); + e.stopPropagation(); + if (!isSourceText) return; + const trimmed = addBreakCellNumber.trim(); + const parsed = Number(trimmed); + // Allowed range mirrors the provider: splitting at cell 1 would + // duplicate the implicit first subdivision, and we can't split + // beyond the last cell. + if ( + trimmed.length === 0 || + !Number.isFinite(parsed) || + !Number.isInteger(parsed) || + parsed < 2 || + parsed > maxCellNumber + ) { + setAddBreakError( + maxCellNumber >= 2 + ? `Enter a number between 2 and ${maxCellNumber}.` + : "This milestone is too short to split." + ); + return; + } + vscode.postMessage({ + command: "addMilestoneSubdivisionAnchor", + content: { + milestoneIndex: milestoneIdx, + cellNumber: parsed, + }, + }); + handleCancelAddBreak(); + }; + // Calculate position and dimensions const calculatePositionAndDimensions = () => { if (isOpen && anchorRef.current) { @@ -1158,46 +1240,162 @@ export function MilestoneAccordion({ ); })} - {isSourceText && - subsections.some( + {isSourceText && (() => { + const maxCellNumber = + getMaxCellNumberForMilestone( + subsections + ); + const canAddBreak = maxCellNumber >= 2; + const isFormOpen = + addBreakMilestoneIdx === milestoneIdx; + const hasCustomBreaks = subsections.some( (s) => s.source === "custom" - ) && ( -
- + + {addBreakError && ( + + {addBreakError} + + )} + + ) : ( + canAddBreak && ( + + ) + )} + {hasCustomBreaks && !isFormOpen && ( + + ? "Click again to confirm" + : "Reset to default breaks"} + + )}
- )} + ); + })()} From f69ff2919142424de00c504811c37fd9b7767db8 Mon Sep 17 00:00:00 2001 From: Luke-Bilhorn Date: Fri, 24 Apr 2026 01:33:23 -0500 Subject: [PATCH 07/11] Add useSubdivisionNumberLabels and cellsPerPage settings end-to-end Introduces the useSubdivisionNumberLabels configuration flag (package.json, interfaceSettings.ts, types, provider message handling) that forces numeric labels on subsection headings instead of named breaks. Also surfaces cellsPerPage in the Interface Settings panel. Wires both settings into ChapterNavigationHeader and MilestoneAccordion, with tests. --- package.json | 9 +- src/interfaceSettings/interfaceSettings.ts | 49 +++++++++ .../codexCellEditorMessagehandling.ts | 5 + .../codexCellEditorProvider.ts | 28 +++++ types/index.d.ts | 15 +++ .../ChapterNavigationHeader.tsx | 7 ++ .../src/CodexCellEditor/CodexCellEditor.tsx | 17 +++ .../components/MilestoneAccordion.test.tsx | 48 +++++++++ .../components/MilestoneAccordion.tsx | 18 +++- .../src/InterfaceSettings/index.tsx | 100 ++++++++++++++++++ 10 files changed, 293 insertions(+), 3 deletions(-) diff --git a/package.json b/package.json index 9b411107c..5e4fb045d 100644 --- a/package.json +++ b/package.json @@ -941,11 +941,18 @@ "description": "Text direction for the editor (left-to-right or right-to-left)" }, "codex-editor-extension.cellsPerPage": { + "title": "Cells Per Page", "type": "number", "default": 50, "minimum": 5, "maximum": 200, - "description": "Number of cells to display per page when a chapter has many cells. Helps with performance for large chapters." + "description": "Default page size for milestones without custom subdivision breaks. Applies only as a fallback when a milestone has no user-defined subdivisions." + }, + "codex-editor-extension.useSubdivisionNumberLabels": { + "title": "Always Show Subdivision Number Ranges", + "type": "boolean", + "default": false, + "description": "When enabled, milestone subdivisions always display their numeric cell range (e.g. '6-15') even if a user-assigned name exists. When disabled, names take precedence and the range is shown only as a muted suffix." }, "codex-editor-extension.useOnlyValidatedExamples": { "title": "Use Only Validated Examples", diff --git a/src/interfaceSettings/interfaceSettings.ts b/src/interfaceSettings/interfaceSettings.ts index 04faa39c1..58f80ff4a 100644 --- a/src/interfaceSettings/interfaceSettings.ts +++ b/src/interfaceSettings/interfaceSettings.ts @@ -62,11 +62,18 @@ export async function openInterfaceSettings() { const sendInit = () => { const config = vscode.workspace.getConfiguration("codex-editor-extension"); const highlightSearchResults = config.get("highlightSearchResults", true); + const cellsPerPage = config.get("cellsPerPage", 50); + const useSubdivisionNumberLabels = config.get( + "useSubdivisionNumberLabels", + false + ); panel.webview.postMessage({ command: "init", data: { highlightSearchResults, + cellsPerPage, + useSubdivisionNumberLabels, }, }); }; @@ -99,6 +106,48 @@ export async function openInterfaceSettings() { ); break; } + + case "updateCellsPerPage": { + // Clamp to the range declared in package.json so invalid input + // cannot corrupt pagination. Pull bounds from the schema-defined + // minimum/maximum rather than hardcoding them in multiple places. + const raw = Number(message.value); + if (!Number.isFinite(raw)) break; + const clamped = Math.max(5, Math.min(200, Math.round(raw))); + const config = vscode.workspace.getConfiguration("codex-editor-extension"); + await config.update( + "cellsPerPage", + clamped, + vscode.ConfigurationTarget.Workspace + ); + break; + } + + case "updateUseSubdivisionNumberLabels": { + const config = vscode.workspace.getConfiguration("codex-editor-extension"); + await config.update( + "useSubdivisionNumberLabels", + Boolean(message.value), + vscode.ConfigurationTarget.Workspace + ); + break; + } } }); + + // Keep the panel in sync when settings change from elsewhere (e.g. the + // VS Code Settings UI). Disposed together with the panel below. + const configListener = vscode.workspace.onDidChangeConfiguration((e) => { + if ( + e.affectsConfiguration("codex-editor-extension.highlightSearchResults") || + e.affectsConfiguration("codex-editor-extension.cellsPerPage") || + e.affectsConfiguration("codex-editor-extension.useSubdivisionNumberLabels") + ) { + sendInit(); + } + }); + + panel.onDidDispose(() => { + configListener.dispose(); + }); } diff --git a/src/providers/codexCellEditorProvider/codexCellEditorMessagehandling.ts b/src/providers/codexCellEditorProvider/codexCellEditorMessagehandling.ts index 62133b5ec..589b0e78e 100644 --- a/src/providers/codexCellEditorProvider/codexCellEditorMessagehandling.ts +++ b/src/providers/codexCellEditorProvider/codexCellEditorMessagehandling.ts @@ -159,6 +159,10 @@ export async function sendMilestoneRefreshToWebview( const username = userInfo?.username || "anonymous"; const rev = provider.getDocumentRevision(docUri); + const useSubdivisionNumberLabels = config.get( + "useSubdivisionNumberLabels", + false + ); safePostMessageToPanel(webviewPanel, { type: "providerSendsInitialContentPaginated", rev, @@ -171,6 +175,7 @@ export async function sendMilestoneRefreshToWebview( username: username, validationCount: validationCount, validationCountAudio: validationCountAudio, + useSubdivisionNumberLabels, }); safePostMessageToPanel(webviewPanel, { diff --git a/src/providers/codexCellEditorProvider/codexCellEditorProvider.ts b/src/providers/codexCellEditorProvider/codexCellEditorProvider.ts index 920481b82..c5da2b0d1 100755 --- a/src/providers/codexCellEditorProvider/codexCellEditorProvider.ts +++ b/src/providers/codexCellEditorProvider/codexCellEditorProvider.ts @@ -123,6 +123,16 @@ export class CodexCellEditorProvider implements vscode.CustomEditorProvider { + safePostMessageToPanel(panel, { + type: "updateSubdivisionLabelPreference", + useSubdivisionNumberLabels: newPref, + }); + }); + } }); this.context.subscriptions.push(configurationChangeDisposable); @@ -886,6 +912,7 @@ export class CodexCellEditorProvider implements vscode.CustomEditorProvider; allSubsectionProgress?: Record>; requestSubsectionProgress?: (milestoneIdx: number) => void; + /** + * When true, milestone subdivisions display their numeric cell range even + * when a user-assigned name is available. Defaults to false. + */ + useSubdivisionNumberLabels?: boolean; } export function ChapterNavigationHeader({ @@ -149,6 +154,7 @@ export function ChapterNavigationHeader({ subsectionProgress, allSubsectionProgress, requestSubsectionProgress, + useSubdivisionNumberLabels = false, }: // Removed onToggleCorrectionEditor since it will be a VS Code command now ChapterNavigationHeaderProps) { const [showConfirm, setShowConfirm] = useState(false); @@ -1078,6 +1084,7 @@ ChapterNavigationHeaderProps) { calculateSubsectionProgress={calculateSubsectionProgress} requestSubsectionProgress={requestSubsectionProgress} vscode={vscode} + useSubdivisionNumberLabels={useSubdivisionNumberLabels} /> ); diff --git a/webviews/codex-webviews/src/CodexCellEditor/CodexCellEditor.tsx b/webviews/codex-webviews/src/CodexCellEditor/CodexCellEditor.tsx index af020ad20..b2976bc9f 100755 --- a/webviews/codex-webviews/src/CodexCellEditor/CodexCellEditor.tsx +++ b/webviews/codex-webviews/src/CodexCellEditor/CodexCellEditor.tsx @@ -242,6 +242,11 @@ const CodexCellEditor: React.FC = () => { (window as any)?.initialData?.validationCountAudio ?? null ); + // Workspace preference: force numeric labels on subdivisions. Initialized + // from the provider's first content payload and kept in sync via + // `updateSubdivisionLabelPreference` messages; default false when absent. + const [useSubdivisionNumberLabels, setUseSubdivisionNumberLabels] = useState(false); + // Track cells currently transcribing audio (to show the same loading effect as translations) const [transcribingCells, setTranscribingCells] = useState>(new Set()); @@ -2333,6 +2338,17 @@ const CodexCellEditor: React.FC = () => { if (event.data.userAccessLevel !== undefined) { setUserAccessLevel(event.data.userAccessLevel); } + if (event.data.useSubdivisionNumberLabels !== undefined) { + setUseSubdivisionNumberLabels( + Boolean(event.data.useSubdivisionNumberLabels) + ); + } + } + + if (event.data.type === "updateSubdivisionLabelPreference") { + setUseSubdivisionNumberLabels( + Boolean(event.data.useSubdivisionNumberLabels) + ); } }, [] @@ -3216,6 +3232,7 @@ const CodexCellEditor: React.FC = () => { subsectionProgress={subsectionProgress[currentMilestoneIndex]} allSubsectionProgress={subsectionProgress} requestSubsectionProgress={requestSubsectionProgressForMilestone} + useSubdivisionNumberLabels={useSubdivisionNumberLabels} /> diff --git a/webviews/codex-webviews/src/CodexCellEditor/components/MilestoneAccordion.test.tsx b/webviews/codex-webviews/src/CodexCellEditor/components/MilestoneAccordion.test.tsx index eacc378db..592f035b0 100644 --- a/webviews/codex-webviews/src/CodexCellEditor/components/MilestoneAccordion.test.tsx +++ b/webviews/codex-webviews/src/CodexCellEditor/components/MilestoneAccordion.test.tsx @@ -1282,6 +1282,54 @@ describe("MilestoneAccordion - Milestone Editing", () => { expect(placementCalls).toHaveLength(0); }); + it("respects useSubdivisionNumberLabels=true by showing numeric range instead of name", async () => { + const named: Subsection[] = [ + { + id: "s-0", + label: "1-5", + startIndex: 0, + endIndex: 5, + key: "__start__", + startCellId: "v1", + source: "auto", + name: "Genealogy", + }, + ]; + renderMilestoneAccordion({ + isSourceText: false, + milestoneIndex: createSplittableIndex(5), + getSubsectionsForMilestone: vi.fn(() => named), + useSubdivisionNumberLabels: true, + }); + + // Name is suppressed in favor of the numeric range; the name must + // NOT appear anywhere as the primary label. + expect(screen.queryByText("Genealogy")).not.toBeInTheDocument(); + expect(screen.getByText("1-5")).toBeInTheDocument(); + }); + + it("default behavior (useSubdivisionNumberLabels=false) shows the name", async () => { + const named: Subsection[] = [ + { + id: "s-0", + label: "1-5", + startIndex: 0, + endIndex: 5, + key: "__start__", + startCellId: "v1", + source: "auto", + name: "Genealogy", + }, + ]; + renderMilestoneAccordion({ + isSourceText: false, + milestoneIndex: createSplittableIndex(5), + getSubsectionsForMilestone: vi.fn(() => named), + }); + + expect(screen.getByText("Genealogy")).toBeInTheDocument(); + }); + it("cancel button closes the form without posting", async () => { renderMilestoneAccordion({ isSourceText: true, diff --git a/webviews/codex-webviews/src/CodexCellEditor/components/MilestoneAccordion.tsx b/webviews/codex-webviews/src/CodexCellEditor/components/MilestoneAccordion.tsx index dfc1d91c8..529906574 100644 --- a/webviews/codex-webviews/src/CodexCellEditor/components/MilestoneAccordion.tsx +++ b/webviews/codex-webviews/src/CodexCellEditor/components/MilestoneAccordion.tsx @@ -46,6 +46,13 @@ interface MilestoneAccordionProps { }; requestSubsectionProgress?: (milestoneIdx: number) => void; vscode: any; + /** + * When true, display the numeric cell range on every subdivision even if + * the subdivision has a user-assigned name. Renaming and editing still + * work normally — the preference only affects the visible label. Defaults + * to false (names take precedence). + */ + useSubdivisionNumberLabels?: boolean; } export function MilestoneAccordion({ @@ -63,6 +70,7 @@ export function MilestoneAccordion({ calculateSubsectionProgress, requestSubsectionProgress, vscode, + useSubdivisionNumberLabels = false, }: MilestoneAccordionProps) { // Layout constants const DROPDOWN_MAX_HEIGHT_VIEWPORT_PERCENT = 60; // 60vh @@ -1084,8 +1092,14 @@ export function MilestoneAccordion({ milestoneIdx, subsection.key ); - const displayName = - cachedLocalName ?? subsection.name; + // Respect the workspace toggle: even if a name exists + // (local override or provider-resolved), we force the + // numeric range to be the visible label by dropping + // displayName. Rename UI still reflects the stored name + // so the user can edit what's actually persisted. + const displayName = useSubdivisionNumberLabels + ? undefined + : cachedLocalName ?? subsection.name; const canRename = !!subsection.key; return ( diff --git a/webviews/codex-webviews/src/InterfaceSettings/index.tsx b/webviews/codex-webviews/src/InterfaceSettings/index.tsx index 8c08f6a86..a123409c4 100644 --- a/webviews/codex-webviews/src/InterfaceSettings/index.tsx +++ b/webviews/codex-webviews/src/InterfaceSettings/index.tsx @@ -49,6 +49,13 @@ function InterfaceSettingsApp() { // Search Settings state const [highlightSearchResults, setHighlightSearchResults] = useState(true); + // Pagination / Subdivision Settings state. `cellsPerPageInput` is a string + // so the field accepts intermediate/invalid values during typing; we parse + // and clamp on blur/Enter before posting. + const [cellsPerPage, setCellsPerPage] = useState(50); + const [cellsPerPageInput, setCellsPerPageInput] = useState("50"); + const [useSubdivisionNumberLabels, setUseSubdivisionNumberLabels] = useState(false); + useEffect(() => { const handler = (event: MessageEvent) => { const message = event.data; @@ -56,6 +63,13 @@ function InterfaceSettingsApp() { if (typeof message.data?.highlightSearchResults === "boolean") { setHighlightSearchResults(message.data.highlightSearchResults); } + if (typeof message.data?.cellsPerPage === "number") { + setCellsPerPage(message.data.cellsPerPage); + setCellsPerPageInput(String(message.data.cellsPerPage)); + } + if (typeof message.data?.useSubdivisionNumberLabels === "boolean") { + setUseSubdivisionNumberLabels(message.data.useSubdivisionNumberLabels); + } } }; window.addEventListener("message", handler); @@ -81,6 +95,33 @@ function InterfaceSettingsApp() { vscode.postMessage({ command: "updateHighlightSearchResults", value: checked }); }; + const CELLS_PER_PAGE_MIN = 5; + const CELLS_PER_PAGE_MAX = 200; + + const commitCellsPerPage = () => { + const parsed = parseInt(cellsPerPageInput, 10); + if (!Number.isFinite(parsed)) { + setCellsPerPageInput(String(cellsPerPage)); + return; + } + const clamped = Math.max(CELLS_PER_PAGE_MIN, Math.min(CELLS_PER_PAGE_MAX, parsed)); + if (clamped === cellsPerPage) { + setCellsPerPageInput(String(cellsPerPage)); + return; + } + setCellsPerPage(clamped); + setCellsPerPageInput(String(clamped)); + vscode.postMessage({ command: "updateCellsPerPage", value: clamped }); + }; + + const handleToggleUseSubdivisionNumberLabels = (checked: boolean) => { + setUseSubdivisionNumberLabels(checked); + vscode.postMessage({ + command: "updateUseSubdivisionNumberLabels", + value: checked, + }); + }; + return (
@@ -262,6 +303,65 @@ function InterfaceSettingsApp() {
+ {/* Pagination & Subdivisions Section */} +
+
+ + Pagination & Subdivisions +
+ +
+ {/* Cells per page */} +
+
+
Cells per page
+
+ Default page size for milestones without custom breaks + (between {CELLS_PER_PAGE_MIN} and {CELLS_PER_PAGE_MAX}). +
+
+ + setCellsPerPageInput(e.target.value.replace(/[^0-9]/g, "")) + } + onBlur={commitCellsPerPage} + onKeyDown={(e) => { + if (e.key === "Enter") { + e.preventDefault(); + (e.target as HTMLInputElement).blur(); + } + }} + className="w-24 bg-transparent border border-border rounded px-2 py-1 text-sm text-right" + aria-label="Cells per page" + /> +
+ + {/* Always show number ranges */} +
+
+
+ Always show subdivision number ranges +
+
+ Display the numeric cell range (e.g. "6-15") even when a + subdivision has a name. Names are shown otherwise. +
+
+ +
+
+
+ {/* Search Settings Section */}
From 64788e3d1da08ff35c93a1db20e4628f3a247308 Mon Sep 17 00:00:00 2001 From: Luke-Bilhorn Date: Fri, 24 Apr 2026 01:33:34 -0500 Subject: [PATCH 08/11] Refresh paired target webview after source-side subdivision edits Ensures that when the source editor mutates subdivision anchors, the corresponding target webview receives an updated notebook state so both panels stay in sync without requiring a manual reload. --- .../codexCellEditorMessagehandling.ts | 30 +++++++++++++++++++ .../codexCellEditorProvider.ts | 10 +++++++ 2 files changed, 40 insertions(+) diff --git a/src/providers/codexCellEditorProvider/codexCellEditorMessagehandling.ts b/src/providers/codexCellEditorProvider/codexCellEditorMessagehandling.ts index 589b0e78e..3590654b0 100644 --- a/src/providers/codexCellEditorProvider/codexCellEditorMessagehandling.ts +++ b/src/providers/codexCellEditorProvider/codexCellEditorMessagehandling.ts @@ -293,6 +293,11 @@ async function commitMilestoneSubdivisionPlacements({ } // Mirror placements (without names) to the paired target document. + // Tracks the target document we successfully mirrored to so we can refresh + // its webview after the source-side refresh below. Only set when the + // mirror actually wrote new data; left null when the target is unpaired, + // out of sync, or the mirror failed. + let mirroredTargetDocument: CodexCellDocument | null = null; try { const pairedUri = provider.getPairedNotebookUri(document.uri); if (pairedUri) { @@ -332,6 +337,7 @@ async function commitMilestoneSubdivisionPlacements({ subdivisions: mirroredPlacements, }); await provider.saveCustomDocument(targetDocument, cancellationToken); + mirroredTargetDocument = targetDocument; } } } @@ -343,6 +349,30 @@ async function commitMilestoneSubdivisionPlacements({ } await sendMilestoneRefreshToWebview(document, webviewPanel, provider); + + // Push a refresh to the paired target webview if it's currently open. + // Cost is one extra postMessage + a milestone-index rebuild on the target + // (already cached and invalidated by `updateCellData`), so the marginal + // overhead per source-side break edit is negligible. When no target + // webview is open we skip silently — it'll pick up the change on next + // load via the persisted file. + if (mirroredTargetDocument) { + const targetPanel = provider.getWebviewPanelForUri(mirroredTargetDocument.uri); + if (targetPanel) { + try { + await sendMilestoneRefreshToWebview( + mirroredTargetDocument, + targetPanel, + provider + ); + } catch (refreshError) { + console.error( + `${logPrefix} Failed to refresh paired target webview:`, + refreshError + ); + } + } + } } /** diff --git a/src/providers/codexCellEditorProvider/codexCellEditorProvider.ts b/src/providers/codexCellEditorProvider/codexCellEditorProvider.ts index c5da2b0d1..e4ee8532c 100755 --- a/src/providers/codexCellEditorProvider/codexCellEditorProvider.ts +++ b/src/providers/codexCellEditorProvider/codexCellEditorProvider.ts @@ -1731,6 +1731,16 @@ export class CodexCellEditorProvider implements vscode.CustomEditorProvider Date: Fri, 24 Apr 2026 01:33:44 -0500 Subject: [PATCH 09/11] Gate subdivision edit affordances behind a gear/settings toggle Adds a settings-mode toggle (gear icon) to the milestone accordion so that the rename, delete, reset, and add-break controls are hidden by default, keeping the read-only accordion view clean. Tests cover the toggle's show/hide behavior. --- .../components/MilestoneAccordion.test.tsx | 143 ++++++++++++++++++ .../components/MilestoneAccordion.tsx | 85 +++++++++-- 2 files changed, 213 insertions(+), 15 deletions(-) diff --git a/webviews/codex-webviews/src/CodexCellEditor/components/MilestoneAccordion.test.tsx b/webviews/codex-webviews/src/CodexCellEditor/components/MilestoneAccordion.test.tsx index 592f035b0..97b9f3000 100644 --- a/webviews/codex-webviews/src/CodexCellEditor/components/MilestoneAccordion.test.tsx +++ b/webviews/codex-webviews/src/CodexCellEditor/components/MilestoneAccordion.test.tsx @@ -15,6 +15,7 @@ vi.mock("@vscode/webview-ui-toolkit/react", () => ({ appearance, title, "aria-label": ariaLabel, + "aria-pressed": ariaPressed, }: any) => ( @@ -177,6 +179,12 @@ describe("MilestoneAccordion - Milestone Editing", () => { calculateSubsectionProgress: mockCalculateSubsectionProgress, requestSubsectionProgress: mockRequestSubsectionProgress, vscode: mockVscode, + // Most editing-flow tests assert directly against the title pencil, + // per-subsection pencils, and add-break controls. Those affordances + // are now gated behind the gear/settings toggle, so we open the + // accordion already in settings mode by default and let the + // dedicated gear-toggle tests override this with `false`. + initialSettingsMode: true, }; return render(); @@ -1361,6 +1369,141 @@ describe("MilestoneAccordion - Milestone Editing", () => { }); }); + describe("Settings mode (gear toggle)", () => { + it("hides edit affordances by default and reveals them after clicking the gear", async () => { + renderMilestoneAccordion({ initialSettingsMode: false }); + + // Default (read-only) state: gear is the only edit-related control + // in the header, the title pencil is gone, and per-subsection + // pencils + add-break / reset footers stay hidden. + expect(screen.queryByLabelText("Edit Milestone")).not.toBeInTheDocument(); + expect(screen.queryByLabelText("Rename Subsection")).not.toBeInTheDocument(); + expect(screen.queryByLabelText("Add Subdivision Break")).not.toBeInTheDocument(); + const gearButton = screen.getByLabelText("Toggle Milestone Settings"); + expect(gearButton).toHaveAttribute("aria-pressed", "false"); + + await act(async () => { + fireEvent.click(gearButton); + }); + + expect(screen.getByLabelText("Edit Milestone")).toBeInTheDocument(); + expect(screen.getByLabelText("Toggle Milestone Settings")).toHaveAttribute( + "aria-pressed", + "true" + ); + }); + + it("reveals per-subsection rename pencils when settings mode is open", async () => { + // Subsections need a `key` for the rename pencil to render at all + // (per the canRename guard); supply one so we can verify the gear + // toggle uncovers them. + renderMilestoneAccordion({ + initialSettingsMode: false, + getSubsectionsForMilestone: vi.fn(() => [ + { + id: "sub-0-1", + label: "1–5", + startIndex: 0, + endIndex: 5, + key: "__start__", + } as Subsection, + ]), + }); + + expect(screen.queryByLabelText("Rename Subsection")).not.toBeInTheDocument(); + + await act(async () => { + fireEvent.click(screen.getByLabelText("Toggle Milestone Settings")); + }); + + const renamePencils = screen.getAllByLabelText("Rename Subsection"); + expect(renamePencils.length).toBeGreaterThan(0); + }); + + it("shows the Add Subdivision Break footer only on source documents in settings mode", async () => { + renderMilestoneAccordion({ + initialSettingsMode: false, + isSourceText: true, + }); + + expect(screen.queryByLabelText("Add Subdivision Break")).not.toBeInTheDocument(); + + await act(async () => { + fireEvent.click(screen.getByLabelText("Toggle Milestone Settings")); + }); + + // Once the gear opens settings, source-only "Add break…" buttons + // become reachable. (Target documents never see these regardless + // of the gear; that's covered by the existing "Add Break — target" + // tests.) + expect(screen.getAllByLabelText("Add Subdivision Break").length).toBeGreaterThan(0); + }); + + it("collapses settings mode again when the accordion is closed and reopened", async () => { + const { rerender } = renderMilestoneAccordion({ initialSettingsMode: false }); + + await act(async () => { + fireEvent.click(screen.getByLabelText("Toggle Milestone Settings")); + }); + expect(screen.getByLabelText("Edit Milestone")).toBeInTheDocument(); + + // Close and reopen — the user should land back in read-only mode + // so an accidental click on the gear isn't sticky across sessions. + await act(async () => { + rerender( + + ); + }); + await act(async () => { + rerender( + + ); + }); + + expect(screen.queryByLabelText("Edit Milestone")).not.toBeInTheDocument(); + expect(screen.getByLabelText("Toggle Milestone Settings")).toHaveAttribute( + "aria-pressed", + "false" + ); + }); + }); + describe("Edit Mode - Accordion Close (no refresh on close)", () => { it("should not send refreshWebviewAfterMilestoneEdits when accordion closes after saving (provider pushes updates immediately on save)", async () => { const { rerender } = renderMilestoneAccordion(); diff --git a/webviews/codex-webviews/src/CodexCellEditor/components/MilestoneAccordion.tsx b/webviews/codex-webviews/src/CodexCellEditor/components/MilestoneAccordion.tsx index 529906574..83ab6ecf0 100644 --- a/webviews/codex-webviews/src/CodexCellEditor/components/MilestoneAccordion.tsx +++ b/webviews/codex-webviews/src/CodexCellEditor/components/MilestoneAccordion.tsx @@ -53,6 +53,14 @@ interface MilestoneAccordionProps { * to false (names take precedence). */ useSubdivisionNumberLabels?: boolean; + /** + * When true, the accordion mounts with the gear/settings affordances + * already revealed (title pencil, per-subsection pencils always visible, + * "Add break…" / "Reset" footer controls visible). Useful for tests and + * for parents that want to deep-link straight into editing. Defaults to + * `false`, matching the read-only default UX. + */ + initialSettingsMode?: boolean; } export function MilestoneAccordion({ @@ -71,6 +79,7 @@ export function MilestoneAccordion({ requestSubsectionProgress, vscode, useSubdivisionNumberLabels = false, + initialSettingsMode = false, }: MilestoneAccordionProps) { // Layout constants const DROPDOWN_MAX_HEIGHT_VIEWPORT_PERCENT = 60; // 60vh @@ -97,6 +106,10 @@ export function MilestoneAccordion({ const [editedMilestoneValue, setEditedMilestoneValue] = useState(""); const [originalMilestoneValue, setOriginalMilestoneValue] = useState(""); const inputRef = useRef(null); + // Settings mode reveals destructive / structural controls (title pencil, + // per-subsection pencils, add-break / reset footers). Default off so the + // accordion stays read-only on first open; the gear button toggles it. + const [isSettingsMode, setIsSettingsMode] = useState(initialSettingsMode); // Local cache of edited milestone values to show changes immediately before webview refresh const [localMilestoneValues, setLocalMilestoneValues] = useState>({}); @@ -406,7 +419,14 @@ export function MilestoneAccordion({ useEffect(() => { if (!isOpen) { setIsEditingMilestone(false); + // Also collapse the gear/settings affordances so reopening the + // accordion always starts from the read-only baseline (matches + // initialSettingsMode default and avoids "stuck open" surprises). + setIsSettingsMode(initialSettingsMode); } + // We intentionally only re-run on `isOpen` changes; resetting on + // initialSettingsMode flips would surprise live edits. + // eslint-disable-next-line react-hooks/exhaustive-deps }, [isOpen]); // Clear local cache when milestoneIndex prop changes (after webview refresh) @@ -919,15 +939,45 @@ export function MilestoneAccordion({ ) : ( - - - + <> + {/* Pencil only appears once the user has opened + settings mode via the gear, so the default + accordion view stays read-only. */} + {isSettingsMode && ( + + + + )} + { + e.stopPropagation(); + setIsSettingsMode((prev) => !prev); + }} + aria-pressed={isSettingsMode} + > + + + )}
) : ( <> - {canRename && ( + {/* Per-subsection edit affordances live behind the + gear/settings toggle. When off, neither the rename + pencil nor the remove "X" should be reachable, so we + don't render them at all (avoids tab-stops and stale + tooltips). When on, they're always visible — no more + hover-only reveal. */} + {isSettingsMode && canRename && ( )} - {isSourceText && + {isSettingsMode && + isSourceText && subsection.source === "custom" && subsection.startCellId && ( @@ -1218,7 +1274,6 @@ export function MilestoneAccordion({ subsection ) } - className="opacity-0 group-hover:opacity-100 focus:opacity-100 transition-opacity" > @@ -1254,7 +1309,7 @@ export function MilestoneAccordion({
); })} - {isSourceText && (() => { + {isSourceText && isSettingsMode && (() => { const maxCellNumber = getMaxCellNumberForMilestone( subsections @@ -1318,7 +1373,7 @@ export function MilestoneAccordion({ aria-invalid={ !!addBreakError } - placeholder={`2–${maxCellNumber}`} + placeholder="322" className="w-20 text-xs px-2 py-1 rounded border border-[var(--vscode-input-border)] bg-[var(--vscode-input-background)] text-[var(--vscode-input-foreground)] focus:outline-none focus:ring-1 focus:ring-[var(--vscode-focusBorder)]" /> - - {addBreakError && ( - + + + {addBreakError && ( + + {addBreakError} + + )} + + ) : ( + canAddBreak && ( + + ) + )} + {hasCustomBreaks && !isFormOpen && ( - ) - )} - {hasCustomBreaks && !isFormOpen && ( - - )} - - ); - })()} + ? "Click again to confirm" + : "Reset to default breaks"} + + )} + + ); + })()}