From 6c0f8f447c6fba272b3d68f173338ab145622ebc Mon Sep 17 00:00:00 2001 From: Luke-Bilhorn Date: Wed, 22 Apr 2026 16:39:47 -0500 Subject: [PATCH 01/19] Add subdivision-aware milestone pagination resolver Introduce a dedicated subdivision model that lets milestones describe custom rendering breaks, while preserving today's arithmetic 50-cell chunking as the default fallback. Pagination, subsection lookup, and subsection progress now all go through a single resolver, so future writes to `metadata.data.subdivisions` immediately change what the editor renders without touching any other code path. - Types: add `MilestoneSubdivisionPlacement`, `SubdivisionInfo`, and `metadata.data.subdivisions` / `metadata.data.subdivisionNames` on milestone cells; extend `MilestoneInfo` with resolved `subdivisions` and add `updateMilestoneSubdivisions` / `updateMilestoneSubdivisionName` editor messages for upcoming writers. - Resolver (`utils/subdivisionUtils.ts`): pure function that returns ordered, non-overlapping subdivisions for a milestone. Handles arithmetic fallback, stable ordering, anchor pruning on deleted cells, duplicate collapse, and target-side name overrides via `FIRST_SUBDIVISION_KEY`. - CodexCellDocument: `buildMilestoneIndex`, `getCellsForMilestone`, `getSubsectionCountForMilestone`, `findMilestoneAndSubsectionForCell`, and `calculateSubsectionProgress` now share the resolver. Behavior is byte-identical for documents without custom subdivisions. - Tests: add unit + integration coverage for the resolver and the document APIs (custom breaks, slicing, stale anchor pruning, legacy 50-cell behavior). Made-with: Cursor --- .../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 44bb039dbc99704d4e2688b608894150a20cc728 Mon Sep 17 00:00:00 2001 From: Luke-Bilhorn Date: Wed, 22 Apr 2026 16:47:09 -0500 Subject: [PATCH 02/19] =?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 Wire up `updateMilestoneSubdivisions` and `updateMilestoneSubdivisionName` editor messages so the UI can now persist custom subdivision breaks and names. Placements are source-authoritative — edits from a `.codex` document are rejected and mirrored onto the paired source's target only after a root-cell-ID sanity check confirms the two milestones line up. Names live on a separate `subdivisionNames` map per document so source and target can be renamed independently. - Message handlers: validate anchors against current root content cells, strip names on mirror (target uses its own override map), recover gracefully when the paired document diverges, and refresh the webview via the existing milestone-refresh path. - Provider helpers: `getPairedNotebookUri` and `getOrOpenDocumentForUri` keep the paired-document plumbing localized to the provider and reuse any already-open `CodexCellDocument`. - Document: expose `getRootContentCellIdsForMilestone` for anchor validation / pairing checks, and invalidate the milestone-index cache whenever `subdivisions` or `subdivisionNames` change so renames and break edits appear on the next render. - Tests: cover anchor-set extraction (including paratext / child / deleted exclusions), cache invalidation on both subdivisions and name overrides, and the full round-trip from placements → arithmetic fallback when placements are cleared. Made-with: Cursor --- .../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 f1185b78e506f08a8c0455fa285ea761f0e8ebb0 Mon Sep 17 00:00:00 2001 From: Luke-Bilhorn Date: Wed, 22 Apr 2026 16:51:16 -0500 Subject: [PATCH 03/19] Thread resolver subdivisions through the editor webview The editor now prefers provider-computed subdivisions when building the UI's `Subsection` list, so custom milestone breaks and their names flow through to navigation the moment the resolver emits them. The prior arithmetic calculation remains as a pure fallback for the brief window before the first `milestoneIndex` lands (or for any stale payloads missing `subdivisions`), which preserves today's behaviour exactly for notebooks without custom breaks. - Extract the subsection-building logic into `CodexCellEditor/utils/subdivisionUtils.ts` so it can be unit-tested independently and reused by future tail-append / renaming paths. - Extend the `Subsection` type with `name`, `key`, `startCellId`, and `source` so downstream UI (accordion, header) can display names, tell custom breaks from auto ones, and echo the stable key back to the provider on renames. - Add unit tests covering empty milestones, subdivision preference, arithmetic fallback parity, ID stability, and edge-case page sizes. Made-with: Cursor --- .../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 2ea9e94131a9e51b6a3bdecbd118d90a2fb430bf Mon Sep 17 00:00:00 2001 From: Luke-Bilhorn Date: Wed, 22 Apr 2026 22:28:13 -0500 Subject: [PATCH 04/19] Allow renaming milestone subdivisions from the accordion Each keyed subsection in the milestone accordion now carries a hover "rename" affordance. Editing is inline, works on both source and target documents (names are stored per-document in `subdivisionNames`), and an optimistic local cache keeps the new name on screen while the provider refreshes. Numeric ranges remain visible alongside the name so users never lose track of where a subdivision starts and ends. - Skip the affordance entirely for legacy subsections without a `key` (no resolver payload, nothing to persist) so the UI degrades cleanly. - Clearing the input posts an empty `newName`, which the provider handler interprets as "remove override" and falls back to the numeric range. - Pressing Enter saves, Escape cancels; cancel never posts a message. - Add focused tests covering affordance gating, display behaviour, the save/clear/no-op paths, and cancel. Made-with: Cursor --- .../components/MilestoneAccordion.test.tsx | 185 ++++++++++++++ .../components/MilestoneAccordion.tsx | 234 +++++++++++++++--- 2 files changed, 388 insertions(+), 31 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..a3169d603 100644 --- a/webviews/codex-webviews/src/CodexCellEditor/components/MilestoneAccordion.test.tsx +++ b/webviews/codex-webviews/src/CodexCellEditor/components/MilestoneAccordion.test.tsx @@ -758,6 +758,191 @@ 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("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..d1c2b216b 100644 --- a/webviews/codex-webviews/src/CodexCellEditor/components/MilestoneAccordion.tsx +++ b/webviews/codex-webviews/src/CodexCellEditor/components/MilestoneAccordion.tsx @@ -92,6 +92,25 @@ 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}`]; + }; + // Calculate position and dimensions const calculatePositionAndDimensions = () => { if (isOpen && anchorRef.current) { @@ -577,6 +596,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 +907,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" + > + + + ) + )} + {!isEditingThisRow && ( + + )} +
); })} From cd4b3605e1cbf916cddac4a2a65f3218401ff954 Mon Sep 17 00:00:00 2001 From: Luke-Bilhorn Date: Thu, 23 Apr 2026 14:30:29 -0500 Subject: [PATCH 05/19] Add per-subsection delete and per-milestone reset controls (source only) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Source-side authors can now remove an individual custom subdivision break and, if they want to start over, reset all custom breaks in a milestone back to the default arithmetic chunking. Both actions post `updateMilestoneSubdivisions` with a recomputed placement list derived from the resolver's subdivisions (custom-index- >0 with a stable startCellId). The provider already mirrors the new list to the paired target document, so source and target stay aligned. - The × button only appears for subdivisions whose `source === "custom"` and that carry a `startCellId`, so legacy payloads and the implicit first subdivision are untouched. - "Reset to default breaks" uses an in-place two-click confirmation (arm-then-commit, auto-disarming after 3s) to avoid accidental loss without pulling in a modal. The accessible label flips between "Reset Subdivisions" and "Confirm Reset Subdivisions" so assistive tech announces the armed state. - Neither control is rendered on target documents; placements are source-authoritative and the provider rejects writes from target URIs anyway — the UI just omits the affordance up front. - Extend the lucide-react test mock with the new X/Undo2 icons and add focused coverage for the control visibility and both post shapes. Made-with: Cursor --- .../components/MilestoneAccordion.test.tsx | 181 +++++++++++++++++ .../components/MilestoneAccordion.tsx | 183 ++++++++++++++++-- 2 files changed, 345 insertions(+), 19 deletions(-) diff --git a/webviews/codex-webviews/src/CodexCellEditor/components/MilestoneAccordion.test.tsx b/webviews/codex-webviews/src/CodexCellEditor/components/MilestoneAccordion.test.tsx index a3169d603..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", () => ({ @@ -943,6 +945,185 @@ describe("MilestoneAccordion - Milestone Editing", () => { }); }); + 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 d1c2b216b..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"; @@ -111,6 +111,89 @@ export function MilestoneAccordion({ 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) { @@ -1005,24 +1088,46 @@ export function MilestoneAccordion({ ) : ( - canRename && ( - - handleSubsectionEditClick( - e, - milestoneIdx, - subsectionIdx, - subsection - ) - } - className="opacity-0 group-hover:opacity-100 focus:opacity-100 transition-opacity" - > - - - ) + <> + {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 a91827774b36e354de26c45505260205357fe930 Mon Sep 17 00:00:00 2001 From: Luke-Bilhorn Date: Thu, 23 Apr 2026 15:03:04 -0500 Subject: [PATCH 06/19] Add addMilestoneSubdivisionAnchor handler + shared commit pipeline MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Extract the save/mirror/refresh pipeline from updateMilestoneSubdivisions into commitMilestoneSubdivisionPlacements so a second entry point — a source-only add-break command that takes a 1-based cellNumber — can reuse the same source-authoritative guarantees (root-cell validation, target mirroring with names stripped, divergence-skip on root mismatch). Tests exercise the new handler via a test-only export of the handler map, covering the happy path, the preserve-names-on-append path, out-of-range rejection, idempotence on re-add, and the non-source rejection guard. Made-with: Cursor --- .../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 4dbf6df4f66044b0074e681a2dee27c44e68bb64 Mon Sep 17 00:00:00 2001 From: Luke-Bilhorn Date: Thu, 23 Apr 2026 15:08:27 -0500 Subject: [PATCH 07/19] =?UTF-8?q?Add=20inline=20"Add=20break=20at=20cell?= =?UTF-8?q?=E2=80=A6"=20form=20to=20milestone=20accordion=20(source=20only?= =?UTF-8?q?)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Source-side users can now split a milestone by typing the target cell number directly into the accordion footer instead of having to touch the underlying data. The form validates the number against the milestone's root-cell range before posting addMilestoneSubdivisionAnchor, surfaces an inline error on out-of-range input, and respects Escape/Cancel to close without committing. Hidden entirely on target documents and on milestones with fewer than 2 cells. Made-with: Cursor --- .../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 14d057ab42762b0259941e3b46b3d5fdb6261e61 Mon Sep 17 00:00:00 2001 From: Luke-Bilhorn Date: Thu, 23 Apr 2026 16:05:06 -0500 Subject: [PATCH 08/19] Add useSubdivisionNumberLabels setting to force numeric subsection labels Introduces codex-editor-extension.useSubdivisionNumberLabels (default false). When enabled, the milestone accordion always shows the numeric cell range instead of a user-assigned subdivision name. The provider broadcasts the current value on initial pagination load and whenever the setting changes. Made-with: Cursor --- package.json | 6 +++ .../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 ++++++- 8 files changed, 142 insertions(+), 2 deletions(-) diff --git a/package.json b/package.json index 9b411107c..b51c0fbd4 100644 --- a/package.json +++ b/package.json @@ -947,6 +947,12 @@ "maximum": 200, "description": "Number of cells to display per page when a chapter has many cells. Helps with performance for large chapters." }, + "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", "type": "boolean", 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 ( From e6703ba56e74c1457e4e7717860be64e3054fad0 Mon Sep 17 00:00:00 2001 From: Luke-Bilhorn Date: Thu, 23 Apr 2026 16:09:30 -0500 Subject: [PATCH 09/19] Surface cellsPerPage and useSubdivisionNumberLabels in Interface Settings Adds a Pagination & Subdivisions section to the Interface Settings webview so users can adjust the default milestone page size and toggle whether subdivisions render as numeric ranges instead of their names. The panel listens for configuration changes and rebroadcasts initial data when either setting is updated from elsewhere. Made-with: Cursor --- package.json | 3 +- src/interfaceSettings/interfaceSettings.ts | 49 +++++++++ .../src/InterfaceSettings/index.tsx | 100 ++++++++++++++++++ 3 files changed, 151 insertions(+), 1 deletion(-) diff --git a/package.json b/package.json index b51c0fbd4..5e4fb045d 100644 --- a/package.json +++ b/package.json @@ -941,11 +941,12 @@ "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", 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/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 9720c19bbc75187be4a3d4100ff34f391d06c3e7 Mon Sep 17 00:00:00 2001 From: Luke-Bilhorn Date: Thu, 23 Apr 2026 20:38:36 -0500 Subject: [PATCH 10/19] Refresh paired target webview after source-side subdivision placement edits MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When a user adds, removes, or resets subdivision breaks on a source document, the placement mirror was already being persisted to the paired target file but the open target webview was never told to re-render — users had to close and reopen the target tab to see the change. This threads the existing milestone-refresh path through to the target panel so the change appears immediately. The cost per source-side break edit is one extra postMessage plus a milestone-index rebuild on the target (cached and invalidated by updateCellData), which is negligible even on older hardware. When no target webview is open the refresh is skipped silently and the target picks up the change on next load via the persisted file. Subdivision name edits remain per-side by design and don't trigger a target refresh. Made-with: Cursor --- .../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: Thu, 23 Apr 2026 20:38:47 -0500 Subject: [PATCH 11/19] Move subdivision edit affordances behind a gear/settings toggle MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The milestone accordion now opens in a read-only baseline: the only edit-related control in the header is a gear icon. Clicking the gear reveals the title pencil, surfaces the per-subsection rename pencils (always visible — no more hover-only reveal), and unhides the source- only "Add break…" / "Reset" footers. Re-clicking the gear collapses the affordances back, and reopening the dropdown always starts from the read-only baseline so the gear-on state is never sticky across sessions. The "Add break at cell" placeholder also drops its compact "2–N" range hint in favor of a single illustrative number, matching the underlying input semantics (one cell number, not a range). Tests opt back into settings mode by default via a new `initialSettingsMode` prop so the existing edit/rename/break test suites keep exercising those flows without first having to click the gear; three new tests cover the gear toggle itself. Made-with: Cursor --- .../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"} + + )} + + ); + })()} From dae9ae79100aa854f1bf46e510a2dfaecddedb0e Mon Sep 17 00:00:00 2001 From: Luke-Bilhorn Date: Fri, 24 Apr 2026 01:30:14 -0500 Subject: [PATCH 17/19] MilestoneAccordion: fix rename click behavior Ensure the clicked milestone expands before entering edit mode, and move the rename affordance into the milestone row. --- .../components/MilestoneAccordion.tsx | 72 +++++++++++-------- 1 file changed, 44 insertions(+), 28 deletions(-) diff --git a/webviews/codex-webviews/src/CodexCellEditor/components/MilestoneAccordion.tsx b/webviews/codex-webviews/src/CodexCellEditor/components/MilestoneAccordion.tsx index 3ad22a6fa..ad567b4fb 100644 --- a/webviews/codex-webviews/src/CodexCellEditor/components/MilestoneAccordion.tsx +++ b/webviews/codex-webviews/src/CodexCellEditor/components/MilestoneAccordion.tsx @@ -713,19 +713,23 @@ export function MilestoneAccordion({ return milestone?.value || ""; }; - const handleEditMilestoneClick = (e: React.MouseEvent) => { + const beginEditMilestone = (e: React.MouseEvent, milestoneIdx: number): void => { e.stopPropagation(); - const displayedMilestone = getDisplayedMilestone(); - if (displayedMilestone) { - setOriginalMilestoneValue(displayedMilestone.value); - setEditedMilestoneValue(displayedMilestone.value); - setIsEditingMilestone(true); - // Focus the input after state update - setTimeout(() => { - inputRef.current?.focus(); - inputRef.current?.select(); - }, 0); - } + const milestone = milestoneIndex?.milestones[milestoneIdx]; + if (!milestone) return; + + // Ensure the edited milestone is what the header will render. + setExpandedMilestone(milestoneIdx.toString()); + + const displayValue = localMilestoneValues[milestoneIdx] || milestone.value; + setOriginalMilestoneValue(displayValue); + setEditedMilestoneValue(displayValue); + setIsEditingMilestone(true); + + setTimeout(() => { + inputRef.current?.focus(); + inputRef.current?.select(); + }, 0); }; const handleSaveMilestone = (e: React.MouseEvent) => { @@ -945,20 +949,6 @@ 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 && ( - - - - )}
+ {isSettingsMode && ( + <> + + beginEditMilestone( + e, + milestoneIdx + ) + } + > + + + + + )}
@@ -1451,7 +1467,7 @@ export function MilestoneAccordion({ milestoneIdx ) } - className="flex items-center gap-1 text-xs px-2 py-1 rounded text-[var(--vscode-descriptionForeground)] hover:text-[var(--vscode-foreground)] hover:bg-secondary transition-colors" + className="flex items-center gap-1 text-xs pl-0 pr-2 py-1 rounded text-[var(--vscode-descriptionForeground)] hover:text-[var(--vscode-foreground)] hover:bg-secondary transition-colors" > Add break… From d6d98ff302af236ba7dc518d0108951c1b765440 Mon Sep 17 00:00:00 2001 From: Luke-Bilhorn Date: Wed, 29 Apr 2026 22:57:39 -0500 Subject: [PATCH 18/19] Fix tests on milestone subdivisions --- package-lock.json | 54 ++++++++++++-------- src/test/suite/milestoneSubdivisions.test.ts | 18 +++---- 2 files changed, 41 insertions(+), 31 deletions(-) diff --git a/package-lock.json b/package-lock.json index e6af9de62..bb81ea4b6 100644 --- a/package-lock.json +++ b/package-lock.json @@ -2305,6 +2305,7 @@ "resolved": "https://registry.npmjs.org/@aws-sdk/client-sso-oidc/-/client-sso-oidc-3.609.0.tgz", "integrity": "sha512-0bNPAyPdkWkS9EGB2A9BZDkBNrnVCBzk5lYRezoT4K3/gi9w1DTYH5tuRdwaTZdxW19U1mq7CV0YJJARKO1L9Q==", "license": "Apache-2.0", + "peer": true, "dependencies": { "@aws-crypto/sha256-browser": "5.2.0", "@aws-crypto/sha256-js": "5.2.0", @@ -2358,6 +2359,7 @@ "resolved": "https://registry.npmjs.org/@aws-sdk/client-sts/-/client-sts-3.609.0.tgz", "integrity": "sha512-A0B3sDKFoFlGo8RYRjDBWHXpbgirer2bZBkCIzhSPHc1vOFHt/m2NcUoE2xnBKXJFrptL1xDkvo1P+XYp/BfcQ==", "license": "Apache-2.0", + "peer": true, "dependencies": { "@aws-crypto/sha256-browser": "5.2.0", "@aws-crypto/sha256-js": "5.2.0", @@ -4982,7 +4984,6 @@ "integrity": "sha512-T1NCJqT/j9+cn8fvkt7jtwbLBfLC/1y1c7NtCeXFRgzGTsafi68MRv8yzkYSapBnFA6L3U2VSc02ciDzoAJhJg==", "dev": true, "license": "MIT", - "peer": true, "engines": { "node": ">=6.9.0" } @@ -5025,7 +5026,6 @@ "integrity": "sha512-BR7VvDCVHO+q2xBEWskxS6DJE1qRnb7DxzUrogb71CWoSficBxYsiAGd+Kl0mmq/MprG9yArRkyrQxTO6XjMzA==", "dev": true, "license": "ISC", - "peer": true, "bin": { "semver": "bin/semver.js" } @@ -5066,7 +5066,6 @@ "integrity": "sha512-JYtls3hqi15fcx5GaSNL7SCTJ2MNmjrkHXg4FSpOA/grxK8KwyZ5bubHsCq8FXCkua6xhuaaBit+3b7+VZRfcA==", "dev": true, "license": "MIT", - "peer": true, "dependencies": { "@babel/compat-data": "^7.28.6", "@babel/helper-validator-option": "^7.27.1", @@ -5084,7 +5083,6 @@ "integrity": "sha512-BR7VvDCVHO+q2xBEWskxS6DJE1qRnb7DxzUrogb71CWoSficBxYsiAGd+Kl0mmq/MprG9yArRkyrQxTO6XjMzA==", "dev": true, "license": "ISC", - "peer": true, "bin": { "semver": "bin/semver.js" } @@ -5151,7 +5149,6 @@ "integrity": "sha512-l5XkZK7r7wa9LucGw9LwZyyCUscb4x37JWTPz7swwFE/0FMQAGpiWUZn8u9DzkSBWEcK25jmvubfpw2dnAMdbw==", "dev": true, "license": "MIT", - "peer": true, "dependencies": { "@babel/traverse": "^7.28.6", "@babel/types": "^7.28.6" @@ -5166,7 +5163,6 @@ "integrity": "sha512-67oXFAYr2cDLDVGLXTEABjdBJZ6drElUSI7WKp70NrpyISso3plG9SAGEF6y7zbha/wOzUByWWTJvEDVNIUGcA==", "dev": true, "license": "MIT", - "peer": true, "dependencies": { "@babel/helper-module-imports": "^7.28.6", "@babel/helper-validator-identifier": "^7.28.5", @@ -5260,7 +5256,6 @@ "integrity": "sha512-YvjJow9FxbhFFKDSuFnVCe2WxXk1zWc22fFePVNEaWJEu8IrZVlda6N0uHwzZrUM1il7NC9Mlp4MaJYbYd9JSg==", "dev": true, "license": "MIT", - "peer": true, "engines": { "node": ">=6.9.0" } @@ -5271,7 +5266,6 @@ "integrity": "sha512-xOBvwq86HHdB7WUDTfKfT/Vuxh7gElQ+Sfti2Cy6yIWNW05P8iUslOVcZ4/sKbE+/jQaukQAdz/gf3724kYdqw==", "dev": true, "license": "MIT", - "peer": true, "dependencies": { "@babel/template": "^7.28.6", "@babel/types": "^7.28.6" @@ -6017,6 +6011,7 @@ "resolved": "https://registry.npmjs.org/@jimp/custom/-/custom-0.22.12.tgz", "integrity": "sha512-xcmww1O/JFP2MrlGUMd3Q78S3Qu6W3mYTXYuIqFq33EorgYHV/HqymHfXy9GjiCJ7OI+7lWx6nYFOzU7M4rd1Q==", "license": "MIT", + "peer": true, "dependencies": { "@jimp/core": "^0.22.12" } @@ -6053,6 +6048,7 @@ "resolved": "https://registry.npmjs.org/@jimp/plugin-blit/-/plugin-blit-0.22.12.tgz", "integrity": "sha512-xslz2ZoFZOPLY8EZ4dC29m168BtDx95D6K80TzgUi8gqT7LY6CsajWO0FAxDwHz6h0eomHMfyGX0stspBrTKnQ==", "license": "MIT", + "peer": true, "dependencies": { "@jimp/utils": "^0.22.12" }, @@ -6065,6 +6061,7 @@ "resolved": "https://registry.npmjs.org/@jimp/plugin-blur/-/plugin-blur-0.22.12.tgz", "integrity": "sha512-S0vJADTuh1Q9F+cXAwFPlrKWzDj2F9t/9JAbUvaaDuivpyWuImEKXVz5PUZw2NbpuSHjwssbTpOZ8F13iJX4uw==", "license": "MIT", + "peer": true, "dependencies": { "@jimp/utils": "^0.22.12" }, @@ -6089,6 +6086,7 @@ "resolved": "https://registry.npmjs.org/@jimp/plugin-color/-/plugin-color-0.22.12.tgz", "integrity": "sha512-xImhTE5BpS8xa+mAN6j4sMRWaUgUDLoaGHhJhpC+r7SKKErYDR0WQV4yCE4gP+N0gozD0F3Ka1LUSaMXrn7ZIA==", "license": "MIT", + "peer": true, "dependencies": { "@jimp/utils": "^0.22.12", "tinycolor2": "^1.6.0" @@ -6132,6 +6130,7 @@ "resolved": "https://registry.npmjs.org/@jimp/plugin-crop/-/plugin-crop-0.22.12.tgz", "integrity": "sha512-FNuUN0OVzRCozx8XSgP9MyLGMxNHHJMFt+LJuFjn1mu3k0VQxrzqbN06yIl46TVejhyAhcq5gLzqmSCHvlcBVw==", "license": "MIT", + "peer": true, "dependencies": { "@jimp/utils": "^0.22.12" }, @@ -6255,6 +6254,7 @@ "resolved": "https://registry.npmjs.org/@jimp/plugin-resize/-/plugin-resize-0.22.12.tgz", "integrity": "sha512-3NyTPlPbTnGKDIbaBgQ3HbE6wXbAlFfxHVERmrbqAi8R3r6fQPxpCauA8UVDnieg5eo04D0T8nnnNIX//i/sXg==", "license": "MIT", + "peer": true, "dependencies": { "@jimp/utils": "^0.22.12" }, @@ -6267,6 +6267,7 @@ "resolved": "https://registry.npmjs.org/@jimp/plugin-rotate/-/plugin-rotate-0.22.12.tgz", "integrity": "sha512-9YNEt7BPAFfTls2FGfKBVgwwLUuKqy+E8bDGGEsOqHtbuhbshVGxN2WMZaD4gh5IDWvR+emmmPPWGgaYNYt1gA==", "license": "MIT", + "peer": true, "dependencies": { "@jimp/utils": "^0.22.12" }, @@ -6282,6 +6283,7 @@ "resolved": "https://registry.npmjs.org/@jimp/plugin-scale/-/plugin-scale-0.22.12.tgz", "integrity": "sha512-dghs92qM6MhHj0HrV2qAwKPMklQtjNpoYgAB94ysYpsXslhRTiPisueSIELRwZGEr0J0VUxpUY7HgJwlSIgGZw==", "license": "MIT", + "peer": true, "dependencies": { "@jimp/utils": "^0.22.12" }, @@ -6418,7 +6420,6 @@ "integrity": "sha512-LI9u/+laYG4Ds1TDKSJW2YPrIlcVYOwi2fUC6xB43lueCjgxV4lffOCZCtYFiH6TNOX+tQKXx97T4IKHbhyHEQ==", "dev": true, "license": "MIT", - "peer": true, "dependencies": { "@jridgewell/gen-mapping": "^0.3.5", "@jridgewell/trace-mapping": "^0.3.24" @@ -8275,6 +8276,7 @@ "integrity": "sha512-z9VXpC7MWrhfWipitjNdgCauoMLRdIILQsAEV+ZesIzBq/oUlxk0m3ApZuMFCXdnS4U7KrI+l3WRUEGQ8K1QKw==", "dev": true, "license": "MIT", + "peer": true, "dependencies": { "@types/prop-types": "*", "csstype": "^3.2.2" @@ -8472,6 +8474,7 @@ "integrity": "sha512-tbsV1jPne5CkFQCgPBcDOt30ItF7aJoZL997JSF7MhGQqOeT3svWRYxiqlfA5RUdlHN6Fi+EI9bxqbdyAUZjYQ==", "dev": true, "license": "BSD-2-Clause", + "peer": true, "dependencies": { "@typescript-eslint/scope-manager": "6.21.0", "@typescript-eslint/types": "6.21.0", @@ -9976,6 +9979,7 @@ "integrity": "sha512-nQyp0o1/mNdbTO1PO6kHkwSrmgZ0MT/jCCpNiwbUjGoRN4dlBhqJtoQuCnEOKzgTVwg0ZWiCoQy6SxMebQVh8A==", "dev": true, "license": "MIT", + "peer": true, "bin": { "acorn": "bin/acorn" }, @@ -10502,6 +10506,7 @@ "resolved": "https://registry.npmjs.org/bare-events/-/bare-events-2.8.2.tgz", "integrity": "sha512-riJjyv1/mHLIPX4RwiK+oW9/4c3TEUeORHKefKAKnZ5kyslbN+HXowtbaVEqt4IMUB7OXlfixcs6gsFeo/jhiQ==", "license": "Apache-2.0", + "peer": true, "peerDependencies": { "bare-abort-controller": "*" }, @@ -10934,6 +10939,7 @@ } ], "license": "MIT", + "peer": true, "dependencies": { "baseline-browser-mapping": "^2.9.0", "caniuse-lite": "^1.0.30001759", @@ -11821,8 +11827,7 @@ "resolved": "https://registry.npmjs.org/convert-source-map/-/convert-source-map-2.0.0.tgz", "integrity": "sha512-Kvp459HrV2FEJ1CAsi1Ku+MY3kasH19TFykTz2xWmMeq6bk2NU3XXvfJ+Q61m0xktWwt+1HSYf3JZsTms3aRJg==", "dev": true, - "license": "MIT", - "peer": true + "license": "MIT" }, "node_modules/cookie": { "version": "0.7.2", @@ -12508,7 +12513,8 @@ "resolved": "https://registry.npmjs.org/devtools-protocol/-/devtools-protocol-0.0.1400418.tgz", "integrity": "sha512-U8j75zDOXF8IP3o0Cgb7K4tFA9uUHEOru2Wx64+EUqL4LNOh9dRe1i8WKR1k3mSpjcCe3aIkTDvEwq0YkI4hfw==", "dev": true, - "license": "BSD-3-Clause" + "license": "BSD-3-Clause", + "peer": true }, "node_modules/diff": { "version": "7.0.0", @@ -12820,6 +12826,7 @@ "integrity": "sha512-ETBauow1T35Y/WZMkio9jiM0Z5xjHHmJ4XmjZOq1l/dXz3lr2sRn87nJy20RupqSh1F2m3HHPSp8ShIPQJrJ3A==", "devOptional": true, "license": "MIT", + "peer": true, "dependencies": { "iconv-lite": "^0.6.2" } @@ -13050,6 +13057,7 @@ "deprecated": "This version is no longer supported. Please see https://eslint.org/version-support for other options.", "dev": true, "license": "MIT", + "peer": true, "dependencies": { "@babel/code-frame": "7.12.11", "@eslint/eslintrc": "^0.4.3", @@ -14421,7 +14429,6 @@ "integrity": "sha512-3hN7NaskYvMDLQY55gnW3NQ+mesEAepTqlg+VEbj7zzqEMBVNhzcGYYeqFo/TlYz6eQiFcp1HcsCZO+nGgS8zg==", "dev": true, "license": "MIT", - "peer": true, "engines": { "node": ">=6.9.0" } @@ -16386,7 +16393,6 @@ "integrity": "sha512-XmOWe7eyHYH14cLdVPoyg+GOH3rYX++KpzrylJwSW98t3Nk+U8XOl8FWKOgwtzdb8lXGf6zYwDUzeHMWfxasyg==", "dev": true, "license": "MIT", - "peer": true, "bin": { "json5": "lib/cli.js" }, @@ -16829,7 +16835,6 @@ "resolved": "https://registry.npmjs.org/loose-envify/-/loose-envify-1.4.0.tgz", "integrity": "sha512-lyuxPGr/Wfhrlem2CL/UcnUc1zcqKAImBDzukY7Y5F/yQiNdko6+fRLevlw1HgMySw7f611UIY408EtxRSoK3Q==", "license": "MIT", - "peer": true, "dependencies": { "js-tokens": "^3.0.0 || ^4.0.0" }, @@ -16866,7 +16871,6 @@ "integrity": "sha512-KpNARQA3Iwv+jTA0utUVVbrh+Jlrr1Fv0e56GGzAFOXN7dk/FviaDW8LHmK52DlcH4WP2n6gI8vN1aesBFgo9w==", "dev": true, "license": "ISC", - "peer": true, "dependencies": { "yallist": "^3.0.2" } @@ -19645,6 +19649,7 @@ "dev": true, "inBundle": true, "license": "MIT", + "peer": true, "engines": { "node": ">=12" }, @@ -21575,7 +21580,6 @@ "resolved": "https://registry.npmjs.org/prop-types/-/prop-types-15.8.1.tgz", "integrity": "sha512-oj87CgZICdulUohogVAR7AjlC0327U4el4L6eAvOqCeudMDVU0NThNaV+b9Df4dXgSP1gXMTnPdhfe/2qDH5cg==", "license": "MIT", - "peer": true, "dependencies": { "loose-envify": "^1.4.0", "object-assign": "^4.1.1", @@ -21586,8 +21590,7 @@ "version": "16.13.1", "resolved": "https://registry.npmjs.org/react-is/-/react-is-16.13.1.tgz", "integrity": "sha512-24e6ynE2H+OKt4kqsOvNd8kBpV65zoxbA4BVsEOB3ARVWQki/DHzaUoC5KuON/BiccDaCCTZBuOcfZs70kR8bQ==", - "license": "MIT", - "peer": true + "license": "MIT" }, "node_modules/proxy-addr": { "version": "2.0.7", @@ -22691,6 +22694,7 @@ "resolved": "https://registry.npmjs.org/ajv/-/ajv-8.18.0.tgz", "integrity": "sha512-PlXPeEWMXMZ7sPYOHqmDyCJzcfNrUr3fGNKtezX14ykXOEIvyK81d+qydx89KY5O71FKMPaQ2vBfBFI5NHR63A==", "license": "MIT", + "peer": true, "dependencies": { "fast-deep-equal": "^3.1.3", "fast-uri": "^3.0.1", @@ -24213,7 +24217,8 @@ "version": "2.8.1", "resolved": "https://registry.npmjs.org/tslib/-/tslib-2.8.1.tgz", "integrity": "sha512-oJFu94HQb+KVduSUQL7wnpmqnfmLsOA/nAh6b6EH0wCEoK0/mPeXU6c3wKDV83MkOuHPRHtSXKKU99IBazS/2w==", - "license": "0BSD" + "license": "0BSD", + "peer": true }, "node_modules/turndown": { "version": "7.2.4", @@ -24284,6 +24289,7 @@ "integrity": "sha512-jl1vZzPDinLr9eUt3J/t7V6FgNEw9QjvBPdysz9KfQDD41fQrC2Y4vKQdiaUpFT4bXlb1RHhLpp8wtm6M5TgSw==", "dev": true, "license": "Apache-2.0", + "peer": true, "bin": { "tsc": "bin/tsc", "tsserver": "bin/tsserver" @@ -27232,6 +27238,7 @@ "integrity": "sha512-SyrSVpygEdPzvgpapVZRQCy8XIOecadp56bPQewpfSfo9ypB6wdOUkx13NBu2ANDlUAtJX7KaLJpTtywVHNlVw==", "dev": true, "license": "MIT", + "peer": true, "dependencies": { "@types/node": "^22.2.0", "@wdio/config": "8.46.0", @@ -27312,6 +27319,7 @@ "resolved": "https://registry.npmjs.org/webpack/-/webpack-5.105.4.tgz", "integrity": "sha512-jTywjboN9aHxFlToqb0K0Zs9SbBoW4zRUlGzI2tYNxVYcEi/IPpn+Xi4ye5jTLvX2YeLuic/IvxNot+Q1jMoOw==", "license": "MIT", + "peer": true, "dependencies": { "@types/eslint-scope": "^3.7.7", "@types/estree": "^1.0.8", @@ -27361,6 +27369,7 @@ "integrity": "sha512-MfwFQ6SfwinsUVi0rNJm7rHZ31GyTcpVE5pgVA3hwFRb7COD4TzjUUwhGWKfO50+xdc2MQPuEBBJoqIMGt3JDw==", "dev": true, "license": "MIT", + "peer": true, "dependencies": { "@discoveryjs/json-ext": "^0.6.1", "@webpack-cli/configtest": "^3.0.1", @@ -27437,6 +27446,7 @@ "resolved": "https://registry.npmjs.org/acorn/-/acorn-8.16.0.tgz", "integrity": "sha512-UVJyE9MttOsBQIDKw1skb9nAwQuR5wuGD3+82K6JgJlm/Y+KI92oNsMNGZCYdDsVtRHSak0pcV5Dno5+4jh9sw==", "license": "MIT", + "peer": true, "bin": { "acorn": "bin/acorn" }, @@ -27681,6 +27691,7 @@ "integrity": "sha512-blAT2mjOEIi0ZzruJfIhb3nps74PRWTCz1IjglWEEpQl5XS/UNama6u2/rjFkDDouqr4L67ry+1aGIALViWjDg==", "devOptional": true, "license": "MIT", + "peer": true, "engines": { "node": ">=10.0.0" }, @@ -27791,8 +27802,7 @@ "resolved": "https://registry.npmjs.org/yallist/-/yallist-3.1.1.tgz", "integrity": "sha512-a4UGQaWPH59mOXUYnAG2ewncQS4i4F43Tv3JoAM+s2VDAmS9NsK8GpDMLrCHPksFT7h3K6TOoUNn2pb7RoXx4g==", "dev": true, - "license": "ISC", - "peer": true + "license": "ISC" }, "node_modules/yargs": { "version": "17.7.2", diff --git a/src/test/suite/milestoneSubdivisions.test.ts b/src/test/suite/milestoneSubdivisions.test.ts index f27c8c974..40eec1a9d 100644 --- a/src/test/suite/milestoneSubdivisions.test.ts +++ b/src/test/suite/milestoneSubdivisions.test.ts @@ -64,7 +64,7 @@ suite("Milestone Subdivisions Test Suite", () => { // --------------------------------------------------------------------------- suite("resolveSubdivisions()", () => { - const ids = (count: number) => Array.from({ length: count }, (_, i) => `c${i + 1}`); + const ids = (count: number) => Array.from({ length: count }, (_, i) => `c${i}`); test("returns empty array when no root cells", () => { const result = resolveSubdivisions({ @@ -86,7 +86,7 @@ suite("Milestone Subdivisions Test Suite", () => { ); assert.strictEqual(result[0].source, "auto"); assert.strictEqual(result[0].key, FIRST_SUBDIVISION_KEY); - assert.strictEqual(result[1].startCellId, "c51"); + assert.strictEqual(result[1].startCellId, "c50"); }); test("single page when count <= pageSize", () => { @@ -106,11 +106,11 @@ suite("Milestone Subdivisions Test Suite", () => { assert.strictEqual(result.length, 2); assert.deepStrictEqual( [result[0].startRootIndex, result[0].endRootIndex], - [0, 5], + [0, 6], ); assert.deepStrictEqual( [result[1].startRootIndex, result[1].endRootIndex], - [5, 10], + [6, 10], ); assert.strictEqual(result[1].name, "Second Half"); assert.strictEqual(result[1].source, "custom"); @@ -131,7 +131,7 @@ suite("Milestone Subdivisions Test Suite", () => { assert.strictEqual(result.length, 4); assert.deepStrictEqual( result.map((s) => s.startRootIndex), - [0, 2, 5, 7], + [0, 3, 6, 8], ); }); @@ -149,7 +149,7 @@ suite("Milestone Subdivisions Test Suite", () => { 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], + [0, 3, 3, 5], ); }); @@ -166,11 +166,11 @@ suite("Milestone Subdivisions Test Suite", () => { assert.strictEqual(result.length, 2); }); - test("placement at c1 names the implicit first subdivision rather than creating a new one", () => { + test("placement at c0 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" }], + placements: [{ startCellId: "c0", name: "Intro" }], cellsPerPage: 50, }); assert.strictEqual(result.length, 1, "No actual break, just a name on the first subdivision"); @@ -203,7 +203,7 @@ suite("Milestone Subdivisions Test Suite", () => { const rootIds = ids(10); const subs = resolveSubdivisions({ rootContentCellIds: rootIds, - placements: [{ startCellId: "c4" }, { startCellId: "c8" }], + placements: [{ startCellId: "c3" }, { startCellId: "c7" }], cellsPerPage: 50, }); assert.strictEqual(findSubdivisionIndexForRoot(subs, 0), 0); From 9e452c272df692cb64edecc6576d1f574fcf5e47 Mon Sep 17 00:00:00 2001 From: Luke-Bilhorn Date: Tue, 5 May 2026 11:02:37 -0500 Subject: [PATCH 19/19] Fix MilestoneAccordion tests for new per-row rename UI The /build CI was failing because the webview test suite couldn't load MilestoneAccordion: the lucide-react mock explicitly listed icons and silently broke the moment the component imported a new one (Trash2). Even after fixing the mock, ~30 assertions still pointed at the old single-pencil "Edit Milestone" label that no longer exists, and the new per-row "Rename Milestone" pencils caused getByLabelText to throw on multi-element matches. Two coupled fixes, one outcome: 1. Standardize rename-flow aria-labels around the user-facing nouns: - Milestone rename: "Rename Milestone" / "Save Milestone Rename" / "Cancel Milestone Rename" (was "Edit/Save Milestone" + "Revert Changes"). - Milestone subdivision rename: "Rename/Save/Cancel Milestone Subdivision Rename" (was "Rename/Save/Cancel Subsection [Name]"). Non-rename actions (gear, add/remove break, reset) keep their existing names. 2. Make the test queries match the new UI: - vi.mock("lucide-react") now uses importOriginal so newly imported icons don't silently break the suite again. - Per-milestone rename pencils are looked up via a small helper that scopes within the row's data-testid="accordion-item-{idx}", so tests read as "Chapter 1's pencil" rather than "the first getByLabelText match". - Describe blocks renamed Edit Mode -> Milestone Rename and Subsection Rename -> Milestone Subdivision Rename to match the user-facing language. --- .../components/MilestoneAccordion.test.tsx | 298 ++++++++++-------- .../components/MilestoneAccordion.tsx | 20 +- 2 files changed, 176 insertions(+), 142 deletions(-) diff --git a/webviews/codex-webviews/src/CodexCellEditor/components/MilestoneAccordion.test.tsx b/webviews/codex-webviews/src/CodexCellEditor/components/MilestoneAccordion.test.tsx index 97b9f3000..401eb3a24 100644 --- a/webviews/codex-webviews/src/CodexCellEditor/components/MilestoneAccordion.test.tsx +++ b/webviews/codex-webviews/src/CodexCellEditor/components/MilestoneAccordion.test.tsx @@ -1,5 +1,5 @@ import React from "react"; -import { render, screen, fireEvent, act } from "@testing-library/react"; +import { render, screen, fireEvent, act, within } from "@testing-library/react"; import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; import "@testing-library/jest-dom/vitest"; import { MilestoneAccordion } from "./MilestoneAccordion"; @@ -59,15 +59,14 @@ vi.mock("./ProgressDots", () => ({ ProgressDots: () =>
ProgressDots
, })); -// Mock icons -vi.mock("lucide-react", () => ({ - Languages: () =>
Languages
, - Check: () =>
Check
, - RotateCcw: () =>
RotateCcw
, - X: () =>
X
, - Undo2: () =>
Undo2
, - Plus: () =>
Plus
, -})); +// Mock icons. Use importOriginal so any new icon imported by the component +// (e.g. Trash2) is automatically available in tests without having to be +// re-listed here. The previous explicit-list approach silently broke tests +// every time a new icon was introduced. +vi.mock("lucide-react", async (importOriginal) => { + const actual = await importOriginal(); + return { ...actual }; +}); vi.mock("../../components/ui/icons/MicrophoneIcon", () => ({ default: () =>
Microphone
, @@ -157,6 +156,28 @@ describe("MilestoneAccordion - Milestone Editing", () => { vi.restoreAllMocks(); }); + /** + * Scope-helpers for per-milestone queries. + * + * The accordion renders one row per milestone, each wrapped in + * `data-testid="accordion-item-${idx}"` (see the AccordionItem mock above). + * The "Rename Milestone" pencil now lives on every row, so an unscoped + * `getByLabelText("Rename Milestone")` would match N elements and throw. + * Tests almost always exercise the *current* milestone (idx 0 by default), + * so we expose a small helper that scopes the lookup to that row. + * + * Use `getRenameMilestoneButton(idx)` for "must exist" assertions and + * `queryRenameMilestoneButton(idx)` for "must not exist" assertions on a + * specific row. For "no rename pencils anywhere" (e.g. settings mode off) + * use `screen.queryAllByLabelText("Rename Milestone")`. + */ + const getMilestoneRow = (milestoneIdx: number = 0): HTMLElement => + screen.getByTestId(`accordion-item-${milestoneIdx}`); + const getRenameMilestoneButton = (milestoneIdx: number = 0): HTMLElement => + within(getMilestoneRow(milestoneIdx)).getByLabelText("Rename Milestone"); + const queryRenameMilestoneButton = (milestoneIdx: number = 0): HTMLElement | null => + within(getMilestoneRow(milestoneIdx)).queryByLabelText("Rename Milestone"); + function renderMilestoneAccordion( props: Partial> = {} ) { @@ -179,47 +200,49 @@ 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`. + // Most rename-flow tests assert directly against the milestone + // pencil, per-milestone-subdivision 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(); } - describe("Edit Mode - Starting Edit", () => { - it("should enter edit mode when edit button is clicked", async () => { + describe("Milestone Rename - Starting", () => { + it("swaps the dropdown header from gear → save/cancel and reveals the rename input", async () => { renderMilestoneAccordion(); - const editButton = screen.getByLabelText("Edit Milestone"); - expect(editButton).toBeInTheDocument(); + const renameButton = getRenameMilestoneButton(); + expect(renameButton).toBeInTheDocument(); await act(async () => { - fireEvent.click(editButton); + fireEvent.click(renameButton); }); - // Should show input field + // Header now hosts the rename input, prefilled with the milestone's + // current display value. const input = screen.getByDisplayValue("Chapter 1"); expect(input).toBeInTheDocument(); expect(input.tagName).toBe("INPUT"); - // Should show save and revert buttons - expect(screen.getByLabelText("Save Milestone")).toBeInTheDocument(); - expect(screen.getByLabelText("Revert Changes")).toBeInTheDocument(); - - // Edit button should not be visible - expect(screen.queryByLabelText("Edit Milestone")).not.toBeInTheDocument(); + // Save + Cancel replace the gear in the header during a rename. + expect(screen.getByLabelText("Save Milestone Rename")).toBeInTheDocument(); + expect(screen.getByLabelText("Cancel Milestone Rename")).toBeInTheDocument(); + expect( + screen.queryByLabelText("Toggle Milestone Settings") + ).not.toBeInTheDocument(); }); it("should initialize input with current milestone value", async () => { renderMilestoneAccordion(); - const editButton = screen.getByLabelText("Edit Milestone"); + const renameButton = getRenameMilestoneButton(); await act(async () => { - fireEvent.click(editButton); + fireEvent.click(renameButton); }); const input = screen.getByDisplayValue("Chapter 1") as HTMLInputElement; @@ -229,9 +252,9 @@ describe("MilestoneAccordion - Milestone Editing", () => { it("should show input field when entering edit mode", async () => { renderMilestoneAccordion(); - const editButton = screen.getByLabelText("Edit Milestone"); + const renameButton = getRenameMilestoneButton(); await act(async () => { - fireEvent.click(editButton); + fireEvent.click(renameButton); }); // Input should be visible and editable @@ -241,14 +264,14 @@ describe("MilestoneAccordion - Milestone Editing", () => { }); }); - describe("Edit Mode - Saving Changes", () => { + describe("Milestone Rename - Saving Changes", () => { it("should save milestone when save button is clicked with valid value", async () => { renderMilestoneAccordion(); // Enter edit mode - const editButton = screen.getByLabelText("Edit Milestone"); + const renameButton = getRenameMilestoneButton(); await act(async () => { - fireEvent.click(editButton); + fireEvent.click(renameButton); }); // Change the value @@ -258,7 +281,7 @@ describe("MilestoneAccordion - Milestone Editing", () => { }); // Click save - const saveButton = screen.getByLabelText("Save Milestone"); + const saveButton = screen.getByLabelText("Save Milestone Rename"); await act(async () => { fireEvent.click(saveButton); }); @@ -282,9 +305,9 @@ describe("MilestoneAccordion - Milestone Editing", () => { it("should trim whitespace when saving", async () => { renderMilestoneAccordion(); - const editButton = screen.getByLabelText("Edit Milestone"); + const renameButton = getRenameMilestoneButton(); await act(async () => { - fireEvent.click(editButton); + fireEvent.click(renameButton); }); const input = screen.getByDisplayValue("Chapter 1") as HTMLInputElement; @@ -292,7 +315,7 @@ describe("MilestoneAccordion - Milestone Editing", () => { fireEvent.change(input, { target: { value: " Trimmed Chapter 1 " } }); }); - const saveButton = screen.getByLabelText("Save Milestone"); + const saveButton = screen.getByLabelText("Save Milestone Rename"); await act(async () => { fireEvent.click(saveButton); }); @@ -309,9 +332,9 @@ describe("MilestoneAccordion - Milestone Editing", () => { it("should not save if value is empty after trimming", async () => { renderMilestoneAccordion(); - const editButton = screen.getByLabelText("Edit Milestone"); + const renameButton = getRenameMilestoneButton(); await act(async () => { - fireEvent.click(editButton); + fireEvent.click(renameButton); }); const input = screen.getByDisplayValue("Chapter 1") as HTMLInputElement; @@ -319,7 +342,7 @@ describe("MilestoneAccordion - Milestone Editing", () => { fireEvent.change(input, { target: { value: " " } }); }); - const saveButton = screen.getByLabelText("Save Milestone"); + const saveButton = screen.getByLabelText("Save Milestone Rename"); expect(saveButton).toBeDisabled(); await act(async () => { @@ -333,12 +356,12 @@ describe("MilestoneAccordion - Milestone Editing", () => { it("should not save if value hasn't changed", async () => { renderMilestoneAccordion(); - const editButton = screen.getByLabelText("Edit Milestone"); + const renameButton = getRenameMilestoneButton(); await act(async () => { - fireEvent.click(editButton); + fireEvent.click(renameButton); }); - const saveButton = screen.getByLabelText("Save Milestone"); + const saveButton = screen.getByLabelText("Save Milestone Rename"); expect(saveButton).toBeDisabled(); await act(async () => { @@ -352,9 +375,9 @@ describe("MilestoneAccordion - Milestone Editing", () => { it("should update local cache immediately after saving", async () => { renderMilestoneAccordion(); - const editButton = screen.getByLabelText("Edit Milestone"); + const renameButton = getRenameMilestoneButton(); await act(async () => { - fireEvent.click(editButton); + fireEvent.click(renameButton); }); const input = screen.getByDisplayValue("Chapter 1") as HTMLInputElement; @@ -362,7 +385,7 @@ describe("MilestoneAccordion - Milestone Editing", () => { fireEvent.change(input, { target: { value: "Cached Chapter 1" } }); }); - const saveButton = screen.getByLabelText("Save Milestone"); + const saveButton = screen.getByLabelText("Save Milestone Rename"); await act(async () => { fireEvent.click(saveButton); }); @@ -376,9 +399,9 @@ describe("MilestoneAccordion - Milestone Editing", () => { it("should handle save when milestone index is valid", async () => { renderMilestoneAccordion(); - const editButton = screen.getByLabelText("Edit Milestone"); + const renameButton = getRenameMilestoneButton(); await act(async () => { - fireEvent.click(editButton); + fireEvent.click(renameButton); }); const input = screen.getByDisplayValue("Chapter 1") as HTMLInputElement; @@ -386,7 +409,7 @@ describe("MilestoneAccordion - Milestone Editing", () => { fireEvent.change(input, { target: { value: "Valid Save" } }); }); - const saveButton = screen.getByLabelText("Save Milestone"); + const saveButton = screen.getByLabelText("Save Milestone Rename"); await act(async () => { fireEvent.click(saveButton); }); @@ -402,13 +425,13 @@ describe("MilestoneAccordion - Milestone Editing", () => { }); }); - describe("Edit Mode - Reverting Changes", () => { + describe("Milestone Rename - Reverting Changes", () => { it("should revert to original value when revert button is clicked", async () => { renderMilestoneAccordion(); - const editButton = screen.getByLabelText("Edit Milestone"); + const renameButton = getRenameMilestoneButton(); await act(async () => { - fireEvent.click(editButton); + fireEvent.click(renameButton); }); const input = screen.getByDisplayValue("Chapter 1") as HTMLInputElement; @@ -416,7 +439,7 @@ describe("MilestoneAccordion - Milestone Editing", () => { fireEvent.change(input, { target: { value: "Changed Value" } }); }); - const revertButton = screen.getByLabelText("Revert Changes"); + const revertButton = screen.getByLabelText("Cancel Milestone Rename"); await act(async () => { fireEvent.click(revertButton); }); @@ -431,9 +454,9 @@ describe("MilestoneAccordion - Milestone Editing", () => { it("should not send postMessage when reverting", async () => { renderMilestoneAccordion(); - const editButton = screen.getByLabelText("Edit Milestone"); + const renameButton = getRenameMilestoneButton(); await act(async () => { - fireEvent.click(editButton); + fireEvent.click(renameButton); }); const input = screen.getByDisplayValue("Chapter 1") as HTMLInputElement; @@ -441,7 +464,7 @@ describe("MilestoneAccordion - Milestone Editing", () => { fireEvent.change(input, { target: { value: "Changed Value" } }); }); - const revertButton = screen.getByLabelText("Revert Changes"); + const revertButton = screen.getByLabelText("Cancel Milestone Rename"); await act(async () => { fireEvent.click(revertButton); }); @@ -451,13 +474,13 @@ describe("MilestoneAccordion - Milestone Editing", () => { }); }); - describe("Edit Mode - Keyboard Shortcuts", () => { + describe("Milestone Rename - Keyboard Shortcuts", () => { it("should save when Enter key is pressed", async () => { renderMilestoneAccordion(); - const editButton = screen.getByLabelText("Edit Milestone"); + const renameButton = getRenameMilestoneButton(); await act(async () => { - fireEvent.click(editButton); + fireEvent.click(renameButton); }); const input = screen.getByDisplayValue("Chapter 1") as HTMLInputElement; @@ -481,9 +504,9 @@ describe("MilestoneAccordion - Milestone Editing", () => { it("should revert when Escape key is pressed", async () => { renderMilestoneAccordion(); - const editButton = screen.getByLabelText("Edit Milestone"); + const renameButton = getRenameMilestoneButton(); await act(async () => { - fireEvent.click(editButton); + fireEvent.click(renameButton); }); const input = screen.getByDisplayValue("Chapter 1") as HTMLInputElement; @@ -508,9 +531,9 @@ describe("MilestoneAccordion - Milestone Editing", () => { it("should prevent default behavior for Enter key", async () => { renderMilestoneAccordion(); - const editButton = screen.getByLabelText("Edit Milestone"); + const renameButton = getRenameMilestoneButton(); await act(async () => { - fireEvent.click(editButton); + fireEvent.click(renameButton); }); const input = screen.getByDisplayValue("Chapter 1") as HTMLInputElement; @@ -539,9 +562,9 @@ describe("MilestoneAccordion - Milestone Editing", () => { it("should prevent default behavior for Escape key", async () => { renderMilestoneAccordion(); - const editButton = screen.getByLabelText("Edit Milestone"); + const renameButton = getRenameMilestoneButton(); await act(async () => { - fireEvent.click(editButton); + fireEvent.click(renameButton); }); const input = screen.getByDisplayValue("Chapter 1") as HTMLInputElement; @@ -565,14 +588,14 @@ describe("MilestoneAccordion - Milestone Editing", () => { }); }); - describe("Edit Mode - Local Cache", () => { + describe("Milestone Rename - Local Cache", () => { it("should use cached value when displaying previously edited milestone", async () => { renderMilestoneAccordion(); // Edit and save milestone 0 - const editButton = screen.getByLabelText("Edit Milestone"); + const renameButton = getRenameMilestoneButton(); await act(async () => { - fireEvent.click(editButton); + fireEvent.click(renameButton); }); const input = screen.getByDisplayValue("Chapter 1") as HTMLInputElement; @@ -580,7 +603,7 @@ describe("MilestoneAccordion - Milestone Editing", () => { fireEvent.change(input, { target: { value: "Saved Chapter 1" } }); }); - const saveButton = screen.getByLabelText("Save Milestone"); + const saveButton = screen.getByLabelText("Save Milestone Rename"); await act(async () => { fireEvent.click(saveButton); }); @@ -597,9 +620,9 @@ describe("MilestoneAccordion - Milestone Editing", () => { renderMilestoneAccordion(); // Edit and save milestone 0 - const editButton = screen.getByLabelText("Edit Milestone"); + const renameButton = getRenameMilestoneButton(); await act(async () => { - fireEvent.click(editButton); + fireEvent.click(renameButton); }); const input = screen.getByDisplayValue("Chapter 1") as HTMLInputElement; @@ -607,7 +630,7 @@ describe("MilestoneAccordion - Milestone Editing", () => { fireEvent.change(input, { target: { value: "Cached Chapter 1" } }); }); - const saveButton = screen.getByLabelText("Save Milestone"); + const saveButton = screen.getByLabelText("Save Milestone Rename"); await act(async () => { fireEvent.click(saveButton); }); @@ -622,13 +645,13 @@ describe("MilestoneAccordion - Milestone Editing", () => { }); }); - describe("Edit Mode - Button States", () => { + describe("Milestone Rename - Button States", () => { it("should disable save button when value is empty", async () => { renderMilestoneAccordion(); - const editButton = screen.getByLabelText("Edit Milestone"); + const renameButton = getRenameMilestoneButton(); await act(async () => { - fireEvent.click(editButton); + fireEvent.click(renameButton); }); const input = screen.getByDisplayValue("Chapter 1") as HTMLInputElement; @@ -636,16 +659,16 @@ describe("MilestoneAccordion - Milestone Editing", () => { fireEvent.change(input, { target: { value: "" } }); }); - const saveButton = screen.getByLabelText("Save Milestone"); + const saveButton = screen.getByLabelText("Save Milestone Rename"); expect(saveButton).toBeDisabled(); }); it("should disable save button when value is only whitespace", async () => { renderMilestoneAccordion(); - const editButton = screen.getByLabelText("Edit Milestone"); + const renameButton = getRenameMilestoneButton(); await act(async () => { - fireEvent.click(editButton); + fireEvent.click(renameButton); }); const input = screen.getByDisplayValue("Chapter 1") as HTMLInputElement; @@ -653,28 +676,28 @@ describe("MilestoneAccordion - Milestone Editing", () => { fireEvent.change(input, { target: { value: " " } }); }); - const saveButton = screen.getByLabelText("Save Milestone"); + const saveButton = screen.getByLabelText("Save Milestone Rename"); expect(saveButton).toBeDisabled(); }); it("should disable save button when value hasn't changed", async () => { renderMilestoneAccordion(); - const editButton = screen.getByLabelText("Edit Milestone"); + const renameButton = getRenameMilestoneButton(); await act(async () => { - fireEvent.click(editButton); + fireEvent.click(renameButton); }); - const saveButton = screen.getByLabelText("Save Milestone"); + const saveButton = screen.getByLabelText("Save Milestone Rename"); expect(saveButton).toBeDisabled(); }); it("should enable save button when value has changed", async () => { renderMilestoneAccordion(); - const editButton = screen.getByLabelText("Edit Milestone"); + const renameButton = getRenameMilestoneButton(); await act(async () => { - fireEvent.click(editButton); + fireEvent.click(renameButton); }); const input = screen.getByDisplayValue("Chapter 1") as HTMLInputElement; @@ -682,42 +705,42 @@ describe("MilestoneAccordion - Milestone Editing", () => { fireEvent.change(input, { target: { value: "New Value" } }); }); - const saveButton = screen.getByLabelText("Save Milestone"); + const saveButton = screen.getByLabelText("Save Milestone Rename"); expect(saveButton).not.toBeDisabled(); }); it("should always enable revert button", async () => { renderMilestoneAccordion(); - const editButton = screen.getByLabelText("Edit Milestone"); + const renameButton = getRenameMilestoneButton(); await act(async () => { - fireEvent.click(editButton); + fireEvent.click(renameButton); }); - const revertButton = screen.getByLabelText("Revert Changes"); + const revertButton = screen.getByLabelText("Cancel Milestone Rename"); expect(revertButton).not.toBeDisabled(); }); }); - describe("Edit Mode - Source Text Mode", () => { - it("should show edit button when isSourceText is true", () => { + describe("Milestone Rename - Source Text Mode", () => { + it("renders the Rename Milestone pencil on source documents", () => { renderMilestoneAccordion({ isSourceText: true }); - expect(screen.getByLabelText("Edit Milestone")).toBeInTheDocument(); + expect(getRenameMilestoneButton()).toBeInTheDocument(); }); - it("should show edit button when isSourceText is false", () => { + it("renders the Rename Milestone pencil on target documents", () => { renderMilestoneAccordion({ isSourceText: false }); - expect(screen.getByLabelText("Edit Milestone")).toBeInTheDocument(); + expect(getRenameMilestoneButton()).toBeInTheDocument(); }); it("should allow editing milestones in source files", async () => { renderMilestoneAccordion({ isSourceText: true }); - const editButton = screen.getByLabelText("Edit Milestone"); + const renameButton = getRenameMilestoneButton(); await act(async () => { - fireEvent.click(editButton); + fireEvent.click(renameButton); }); const input = screen.getByDisplayValue("Chapter 1") as HTMLInputElement; @@ -725,7 +748,7 @@ describe("MilestoneAccordion - Milestone Editing", () => { fireEvent.change(input, { target: { value: "Source Chapter 1" } }); }); - const saveButton = screen.getByLabelText("Save Milestone"); + const saveButton = screen.getByLabelText("Save Milestone Rename"); await act(async () => { fireEvent.click(saveButton); }); @@ -743,9 +766,9 @@ describe("MilestoneAccordion - Milestone Editing", () => { it("should allow editing milestones in target files", async () => { renderMilestoneAccordion({ isSourceText: false }); - const editButton = screen.getByLabelText("Edit Milestone"); + const renameButton = getRenameMilestoneButton(); await act(async () => { - fireEvent.click(editButton); + fireEvent.click(renameButton); }); const input = screen.getByDisplayValue("Chapter 1") as HTMLInputElement; @@ -753,7 +776,7 @@ describe("MilestoneAccordion - Milestone Editing", () => { fireEvent.change(input, { target: { value: "Target Chapter 1" } }); }); - const saveButton = screen.getByLabelText("Save Milestone"); + const saveButton = screen.getByLabelText("Save Milestone Rename"); await act(async () => { fireEvent.click(saveButton); }); @@ -769,7 +792,7 @@ describe("MilestoneAccordion - Milestone Editing", () => { }); }); - describe("Subsection Rename", () => { + describe("Milestone Subdivision Rename", () => { const createSubsectionWithKey = ( id: string, label: string, @@ -786,7 +809,7 @@ describe("MilestoneAccordion - Milestone Editing", () => { source: "custom", }); - it("renders rename button only for subsections that carry a key", async () => { + it("renders rename button only for milestone subdivisions that carry a key", async () => { mockGetSubsectionsForMilestone = vi.fn((milestoneIdx: number) => [ createSubsectionWithKey( `s-${milestoneIdx}-0`, @@ -808,7 +831,7 @@ describe("MilestoneAccordion - Milestone Editing", () => { getSubsectionsForMilestone: mockGetSubsectionsForMilestone, }); - const renameButtons = await screen.findAllByLabelText("Rename Subsection"); + const renameButtons = await screen.findAllByLabelText("Rename Milestone Subdivision"); // Two keyed subsections → two rename affordances; the legacy one is omitted. expect(renameButtons).toHaveLength(2); }); @@ -827,7 +850,7 @@ describe("MilestoneAccordion - Milestone Editing", () => { expect(screen.getByText("1-5")).toBeInTheDocument(); }); - it("posts updateMilestoneSubdivisionName when the subsection rename is saved", async () => { + it("posts updateMilestoneSubdivisionName when the milestone subdivision rename is saved", async () => { mockGetSubsectionsForMilestone = vi.fn((milestoneIdx: number) => [ createSubsectionWithKey(`s-${milestoneIdx}-0`, "1-5", "v1"), ]); @@ -836,7 +859,7 @@ describe("MilestoneAccordion - Milestone Editing", () => { getSubsectionsForMilestone: mockGetSubsectionsForMilestone, }); - const renameBtn = await screen.findByLabelText("Rename Subsection"); + const renameBtn = await screen.findByLabelText("Rename Milestone Subdivision"); await act(async () => { fireEvent.click(renameBtn); }); @@ -847,7 +870,7 @@ describe("MilestoneAccordion - Milestone Editing", () => { fireEvent.change(input, { target: { value: "Opening" } }); }); - const saveBtn = screen.getByLabelText("Save Subsection Name"); + const saveBtn = screen.getByLabelText("Save Milestone Subdivision Rename"); await act(async () => { fireEvent.click(saveBtn); }); @@ -871,7 +894,7 @@ describe("MilestoneAccordion - Milestone Editing", () => { getSubsectionsForMilestone: mockGetSubsectionsForMilestone, }); - const renameBtn = await screen.findByLabelText("Rename Subsection"); + const renameBtn = await screen.findByLabelText("Rename Milestone Subdivision"); await act(async () => { fireEvent.click(renameBtn); }); @@ -881,7 +904,7 @@ describe("MilestoneAccordion - Milestone Editing", () => { fireEvent.change(input, { target: { value: "" } }); }); - const saveBtn = screen.getByLabelText("Save Subsection Name"); + const saveBtn = screen.getByLabelText("Save Milestone Subdivision Rename"); await act(async () => { fireEvent.click(saveBtn); }); @@ -905,12 +928,12 @@ describe("MilestoneAccordion - Milestone Editing", () => { getSubsectionsForMilestone: mockGetSubsectionsForMilestone, }); - const renameBtn = await screen.findByLabelText("Rename Subsection"); + const renameBtn = await screen.findByLabelText("Rename Milestone Subdivision"); await act(async () => { fireEvent.click(renameBtn); }); - const saveBtn = screen.getByLabelText("Save Subsection Name"); + const saveBtn = screen.getByLabelText("Save Milestone Subdivision Rename"); await act(async () => { fireEvent.click(saveBtn); }); @@ -930,7 +953,7 @@ describe("MilestoneAccordion - Milestone Editing", () => { getSubsectionsForMilestone: mockGetSubsectionsForMilestone, }); - const renameBtn = await screen.findByLabelText("Rename Subsection"); + const renameBtn = await screen.findByLabelText("Rename Milestone Subdivision"); await act(async () => { fireEvent.click(renameBtn); }); @@ -940,7 +963,7 @@ describe("MilestoneAccordion - Milestone Editing", () => { fireEvent.change(input, { target: { value: "Something Else" } }); }); - const cancelBtn = screen.getByLabelText("Cancel Rename"); + const cancelBtn = screen.getByLabelText("Cancel Milestone Subdivision Rename"); await act(async () => { fireEvent.click(cancelBtn); }); @@ -954,7 +977,7 @@ describe("MilestoneAccordion - Milestone Editing", () => { }); }); - describe("Subsection Delete and Reset (source only)", () => { + describe("Milestone Subdivision Delete and Reset (source only)", () => { const makeSubsection = ( id: string, label: string, @@ -1020,7 +1043,7 @@ describe("MilestoneAccordion - Milestone Editing", () => { makeSubsection("s-2", "16-30", "v16", "custom", "v16", "Final"), ]; - it("shows remove button only for custom subsections in source", async () => { + it("shows remove button only for custom milestone subdivisions in source", async () => { renderMilestoneAccordion({ isSourceText: true, milestoneIndex: createIndexWithSubdivisions(), @@ -1374,11 +1397,16 @@ describe("MilestoneAccordion - Milestone Editing", () => { 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(); + // in the header, every milestone's rename pencil is gone, and the + // per-subdivision pencils + add-break / reset footers stay hidden. + // Use queryAllByLabelText so the assertion is precise about the + // *count* of pencils (zero) without throwing when there happen + // to be multiple rows. + expect(screen.queryAllByLabelText("Rename Milestone")).toHaveLength(0); + expect( + screen.queryAllByLabelText("Rename Milestone Subdivision") + ).toHaveLength(0); + expect(screen.queryAllByLabelText("Add Subdivision Break")).toHaveLength(0); const gearButton = screen.getByLabelText("Toggle Milestone Settings"); expect(gearButton).toHaveAttribute("aria-pressed", "false"); @@ -1386,17 +1414,20 @@ describe("MilestoneAccordion - Milestone Editing", () => { fireEvent.click(gearButton); }); - expect(screen.getByLabelText("Edit Milestone")).toBeInTheDocument(); + // Settings on → every milestone row exposes its rename pencil. + // We assert the current row's pencil specifically (the others + // mirror it) so the test reads as "pencil for Chapter 1 is back". + expect(getRenameMilestoneButton()).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. + it("reveals per-milestone-subdivision rename pencils when settings mode is open", async () => { + // Milestone subdivisions 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(() => [ @@ -1410,13 +1441,13 @@ describe("MilestoneAccordion - Milestone Editing", () => { ]), }); - expect(screen.queryByLabelText("Rename Subsection")).not.toBeInTheDocument(); + expect(screen.queryByLabelText("Rename Milestone Subdivision")).not.toBeInTheDocument(); await act(async () => { fireEvent.click(screen.getByLabelText("Toggle Milestone Settings")); }); - const renamePencils = screen.getAllByLabelText("Rename Subsection"); + const renamePencils = screen.getAllByLabelText("Rename Milestone Subdivision"); expect(renamePencils.length).toBeGreaterThan(0); }); @@ -1445,7 +1476,7 @@ describe("MilestoneAccordion - Milestone Editing", () => { await act(async () => { fireEvent.click(screen.getByLabelText("Toggle Milestone Settings")); }); - expect(screen.getByLabelText("Edit Milestone")).toBeInTheDocument(); + expect(getRenameMilestoneButton()).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. @@ -1496,7 +1527,10 @@ describe("MilestoneAccordion - Milestone Editing", () => { ); }); - expect(screen.queryByLabelText("Edit Milestone")).not.toBeInTheDocument(); + // After remount in read-only mode, NO milestone exposes a rename + // pencil — the gear must collapse settings rather than persist + // through an accordion close/reopen cycle. + expect(screen.queryAllByLabelText("Rename Milestone")).toHaveLength(0); expect(screen.getByLabelText("Toggle Milestone Settings")).toHaveAttribute( "aria-pressed", "false" @@ -1504,13 +1538,13 @@ describe("MilestoneAccordion - Milestone Editing", () => { }); }); - describe("Edit Mode - Accordion Close (no refresh on close)", () => { + describe("Milestone Rename - 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(); - const editButton = screen.getByLabelText("Edit Milestone"); + const renameButton = getRenameMilestoneButton(); await act(async () => { - fireEvent.click(editButton); + fireEvent.click(renameButton); }); const input = screen.getByDisplayValue("Chapter 1") as HTMLInputElement; @@ -1518,7 +1552,7 @@ describe("MilestoneAccordion - Milestone Editing", () => { fireEvent.change(input, { target: { value: "New Value" } }); }); - const saveButton = screen.getByLabelText("Save Milestone"); + const saveButton = screen.getByLabelText("Save Milestone Rename"); await act(async () => { fireEvent.click(saveButton); }); diff --git a/webviews/codex-webviews/src/CodexCellEditor/components/MilestoneAccordion.tsx b/webviews/codex-webviews/src/CodexCellEditor/components/MilestoneAccordion.tsx index ad567b4fb..f1305cb60 100644 --- a/webviews/codex-webviews/src/CodexCellEditor/components/MilestoneAccordion.tsx +++ b/webviews/codex-webviews/src/CodexCellEditor/components/MilestoneAccordion.tsx @@ -927,9 +927,9 @@ export function MilestoneAccordion({ {isEditingMilestone ? ( <> @@ -1237,9 +1237,9 @@ export function MilestoneAccordion({ {isEditingThisRow ? ( <> handleSubsectionEditClick( e,