Milestone Editing#974
Open
Luke-Bilhorn wants to merge 35 commits into
Open
Conversation
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
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
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
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
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
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
…nly) 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
…bels 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
…ings 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
… edits 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
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
…istence - resolveSubdivisions now sub-chunks long stretches between user-defined breaks by cellsPerPage, so adding a break at cell 433 in a 700-cell milestone still produces [1-50]…[401-432],[433-482]…[683-700] rather than two huge pages. - New workspace setting codex-editor-extension.maxSubdivisionLength (0 = off): when positive, stretches between user breaks shorter than this value are left intact (e.g. a 103-cell logical page survives with threshold=120), while longer stretches continue to be sub-chunked. maxSubdivisionLength is threaded through buildMilestoneIndex, getCells- ForMilestone, findMilestoneAndSubsectionForCell, and the config-change listener so all callers stay in sync. - Source subdivision names are now the default for the paired target document. Names ride on the placement object and are mirrored into subdivisionNamesFromSource; resolveSubdivisions picks them up as a fallback when the target has no local override. A target translator can still set their own independent name. - Naming an auto-generated chunk (on the source side) promotes it to a persistent placement so that changes to cellsPerPage or maxSubdivision- Length cannot displace or orphan the name. Clearing a name leaves the placement intact; the implicit first subdivision is never promoted. - Cache-invalidation fix: updateCellData now also busts the milestone index when subdivisionNamesFromSource changes. - 24 new unit / integration tests covering all of the above. Made-with: Cursor
- New "Maximum subdivision length" toggle + numeric input in the Pagination & Subdivisions section. When enabled, defaults to cellsPerPage × 2. Wired to the codex-editor-extension.maxSubdivision- Length workspace setting with validation (clamped 1-5000). - Temporarily hides the "Always show subdivision number ranges" toggle (the underlying state and message wiring are preserved); names are always shown since numeric ranges are already visible in lighter text. - Removes the "(between 5 and 200)" parenthetical from the Cells per page description. - Updates the Maximum subdivision length description to: "Pagination allows ranges between user added subdivisions up to this length". Made-with: Cursor
- Replace the per-subdivision X remove button and the milestone-level reset button icons with Trash2 (lucide), tinted vscode-errorForeground. - Rows that cannot be deleted (auto-chunks, target side, first subdivision) now render an invisible aria-hidden phantom span the same size as the trash-can button (28×28 px, visibility:hidden) so the rename pencil icons stay vertically aligned across all rows. Made-with: Cursor
This keeps row spacing consistent with the active delete button in milestone settings mode.
Adds a touch more separation between row controls and progress dots.
Ensure the clicked milestone expands before entering edit mode, and move the rename affordance into the milestone row.
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.
Pulls `buildMilestoneCellPayload` and the chapter/book-name helpers out of `migrationUtils.ts` so importers, migrations, and in-editor structural edits can share a single source of truth for milestone-cell shape (label format, edit-history, kind/languageId envelope, optional initial `metadata.data`). Pure refactor — no behavior change.
Three additive primitives the new milestone-placement edits need: - `splitPlacementsAtAnchor` partitions a milestone's `MilestoneSubdivisionPlacement[]` at a boundary cell, lifting any pre-existing subdivision *at* the boundary onto the new milestone as its implicit first-subdivision name (so "Section B" becomes the promoted milestone's label and stops doubling as a subdivision). - `mergePlacementsForRemovedMilestone` merges two milestones' placement lists when one is removed or demoted; the demote path preserves the boundary as a new placement carrying the demoted milestone's label. - `CodexDocument.insertMilestoneCell` adds a top-level milestone cell at a chosen anchor with no `parentId`, using `buildMilestoneCellPayload` for shape parity with importers/migrations. Backed by a `force` flag on `updateCellMilestoneIndices` so structural edits that don't change the cell count still reflush the SQLite milestone-index FTS. No callers yet — wired up in the next commit.
Lets translators add, remove, promote (subdivision → milestone), and demote (milestone → subdivision break) milestones on the source side. Mirrors immediately to the paired target file when the milestone roots agree, preserving UUIDs and anchor cell IDs. Gated end-to-end by a new workspace setting `enableMilestonePlacementEditing` (default off) and surfaced in Interface Settings. Wiring: - `package.json`: declare the new boolean setting. - `interfaceSettings.ts` + `InterfaceSettings/index.tsx`: surface the toggle in the Pagination & Subdivisions section and propagate live changes back to the editor webview. - `codexCellEditorProvider.ts`: read the setting, plumb it into both initial-content payloads, and broadcast preference updates. - `codexCellEditorMessagehandling.ts`: four new handlers (`addMilestoneAtCell`, `removeMilestone`, `promoteSubdivisionToMilestone`, `demoteMilestoneToSubdivision`) built on a pair of helpers — `commitSplitMilestoneAtAnchor` and `commitMergeMilestoneIntoPrevious` — that handle source validation, subdivision redistribution, source/target mirror with root-divergence guards, milestone-index reflush (`force: true`), and webview refresh. - `MilestoneAccordion.tsx`: source-only, settings-mode-only controls for add/remove/promote/demote, with a two-click confirmation pattern on the destructive demote/remove paths. - `types/index.d.ts`: new `EditorPostMessages` commands, the `enableMilestonePlacementEditing` field on `providerSendsInitialContentPaginated`, and a new `updateMilestonePlacementEditingPreference` push.
Mocha (extension host): - `splitPlacementsAtAnchor` and `mergePlacementsForRemovedMilestone` cover boundary lifting (named subdivision → first-subdivision override on the new milestone), the `preserveBoundary` toggle that separates `removeMilestone` from `demoteMilestoneToSubdivision`, and the cell-ID partition by root order. - The four structural-edit handlers exercise the happy path (partition + mirror), the source-only guard (warns + no-op on target), the first-milestone refusal for remove/demote, and the promoted-from-named-subdivision label takeover. Vitest (webview): - `MilestoneAccordion` placement-editing controls are scoped to the feature setting + `isSourceText` + settings mode. Verifies the `addMilestoneAtCell` form payload, the promote button's `subdivisionKey` payload, the two-click arming for remove/demote, and that the first milestone never exposes destructive actions.
Brings milestone-rename UX to parity with the subdivision rename: the pencil now swaps the milestone label for an input on its own row (instead of swapping the dropdown header), and the matching save/cancel cluster takes over the row's pencil/destructive area. State changes: - `isEditingMilestone: boolean` becomes `editingMilestoneIdx: number | null` so we know WHICH row hosts the input (multiple milestones share the same accordion). - `handleMilestoneExpansion` no longer chases the user's expansion to re-target the editor — the input is anchored to the milestone it was opened on, just like subdivisions. - The header is now a static `<h2>` + persistent gear button, so the settings toggle stays reachable while a rename is in flight (e.g. the user can still close the accordion mid-edit). The `Save Milestone Rename` / `Cancel Milestone Rename` aria-labels move with the buttons unchanged, so per-row scoped tests keep working. The starting-rename test is updated to reflect the inline anchor and the gear staying put.
Demote is reversible — promote turns the resulting subdivision break right back into a milestone — so the two-click arm/confirm step was unnecessary friction. Drops the `demoteConfirm*` state and timer, the arming aria-label/title swap, and the matching armed-styling on the button. Pure remove keeps its two-click confirmation since it drops the seam entirely. Test now asserts a single click commits and that the "Confirm Demote Milestone" arming label never appears.
When neither the caller nor the boundary subdivision provides a label (typical for `addMilestoneAtCell`, or for promoting an unnamed subdivision break), `commitSplitMilestoneAtAnchor` was falling through to `buildMilestoneCellPayload`'s chapter-style auto-label — which clones the parent milestone's name (e.g. two "Luke 1"s side by side). Switch the cascade to: explicit caller override → boundary placement name → subdivisionNames map entry → "New milestone" placeholder. Centralises the placeholder as a `NEW_MILESTONE_DEFAULT_LABEL` constant. Adds two Mocha cases: addMilestoneAtCell stamping the placeholder, and promoteSubdivisionToMilestone using it when the underlying subdivision break is unnamed.
The promote/demote/add/remove handlers were awaiting two slow steps (saveCustomDocument + updateCellMilestoneIndices, on both source and target) BEFORE the webview refresh, so the user saw the new milestone structure only after a full disk write + SQLite FTS reflush per document. With both sides involved this could be 500ms–1s of dead time per click — visible enough that the user reported it as "delay". Reorder both `commitSplitMilestoneAtAnchor` and `commitMergeMilestoneIntoPrevious` to: 1. Apply the in-memory mutations on source and target. 2. Adjust cached cursor positions on both URIs. 3. Push `sendMilestoneRefreshToWebview` to source AND target panels (the milestone index they read is rebuilt from in-memory state, so it's already correct at this point). 4. Run `saveCustomDocument` + `updateCellMilestoneIndices` in parallel for source AND target via `Promise.all` AFTER the UI is updated. If a save fails the user gets a notification — the in-memory state already reflects the change (so subsequent edits compose correctly), matching the prior behavior on partial-save errors. Existing handler tests stub both persistence calls and assert on document state, so they keep passing under the new ordering.
The structural mirror already stamps the source's label on the target when a milestone is added/promoted, but subsequent renames stayed local to the source. Now `updateMilestoneValue` propagates the new label to the paired target's `cell.value` whenever the rename originates on a source file, gated by matching milestone cell IDs and root cell IDs so diverged structures are left alone. Target-side renames remain local; placements stay source-authoritative. Also reorders the handler's persistence step to mirror the snappy pattern used by the structural helpers: in-memory mutate both docs, refresh both webviews, then save in parallel — so the rename appears without waiting on disk I/O.
Apply the same in-memory-mutate / refresh-then-persist ordering already used by the structural milestone helpers to the two remaining mirror paths: `commitMilestoneSubdivisionPlacements` (subdivision break add/remove + the promotion path of subdivision rename) and the non- promotion path of `updateMilestoneSubdivisionName`. Both webview panels now see the new placements / `subdivisionNamesFromSource` immediately, and the source + target saves run in parallel after the UI has already updated — eliminating the disk-I/O wait the user could perceive on slower filesystems.
The CodexCellEditor stale guard rejected any providerSendsInitialContentPaginated whose currentMilestoneIndex / currentSubsectionIndex did not match the webview's refs. That guard is meant to keep an in-flight initial-content message from clobbering user navigation, but it also rejected every refresh the provider pushed after a structural milestone edit shifted the cursor (e.g. demote moves the viewer from milestone N to N-1). The accordion stayed frozen on the pre-edit structure and refreshCurrentPage re-requested cells from the now-stale ref position. Add a force flag to providerSendsInitialContentPaginated and refreshCurrentPage; sendMilestoneRefreshToWebview sets it on every push since by construction it only fires after the provider has mutated the document. The webview bypasses the stale guard, drops any in-flight navigation request, and realigns its refs to the message's position so the follow-up cell request hits the new range.
commitMergeMilestoneIntoPrevious mutated the source first and then called sourceAndTargetMilestoneRootsMatch on the post-mutation source to gate the structural mirror. After the merge, the source's previous milestone has absorbed the removed milestone's roots, so its root id list never matched the still-unmutated target. rootsMatchPrev always returned false and every remove/demote silently skipped the target — the target stayed on the pre-edit structure even when both sides shared identical milestone ids and roots before the click. Snapshot both the previous and removed milestones' root id lists from the source before any mutation, then compare those snapshots against the target's current roots. The divergence guard still fires when the target genuinely drifted, but a freshly-paired source / target pair now mirrors cleanly through demote and remove. Tests cover the mirrored delete, the mirrored demote (including the boundary placement and subdivisionNamesFromSource carry-over), and the no-op when target milestone ids have diverged.
Merge demote/remove on the paired file now folds the translator's subdivisionNames from both milestones onto the surviving cell, instead of leaving overrides stranded on the soft-deleted milestone. When promoting a break on the source, a target-only name at the seam wins for the new milestone label so it is not replaced by a source-only placeholder. Tests cover demote (names move to survivor) and promote (translator seam label on the new milestone).
….name Target mirror now preserves the seam label on the subdivision row rather than only subdivisionNamesFromSource; update assertions accordingly.
The accordion's focus-trap useEffect bundled `accordionRef.current.focus()` with the ESC + click-outside listener registration, and depended on the parent-supplied `onClose` callback. ChapterNavigationHeader passed `onClose` as an inline arrow, so every parent re-render produced a fresh reference, churning the deps and re-stealing focus from any open inline rename input. Subdivision renames (input lives in a plain div inside AccordionContent) were the visible casualty; milestone renames survived because their input lives inside an AccordionTrigger button that has its own focus management. Split the effect into two: - one that auto-focuses the accordion exactly once per open transition, - one that re-attaches the ESC / click-outside listeners when `onClose` flips (cheap, no focus side effect). Also stabilize `onClose` in ChapterNavigationHeader with `useCallback` so the listener-attach effect doesn't churn unnecessarily. Adds a regression test that re-renders the accordion with a new `onClose` identity while the subdivision rename input is focused and asserts focus stays on the input.
Contributor
Author
|
When testing, make sure to go into interface settings, and enable milestone editing. Otherwise, it is disabled. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Expands the subdivision editing PR to include milestone editing, with associated tests