diff --git a/src/cli/info/schema.ts b/src/cli/info/schema.ts index 61f2240..e7fc3bd 100644 --- a/src/cli/info/schema.ts +++ b/src/cli/info/schema.ts @@ -170,6 +170,20 @@ const JSON_SCHEMA = { author: { type: "string" }, date: { type: "string" }, revisionId: { type: "string" }, + within: { + type: "array", + description: + "Enclosing revision wrappers, outermost first — present only when this wrapper is nested inside another; a run is visible in a view only when every entry is", + items: { + type: "object", + required: ["id", "kind", "author"], + properties: { + id: { type: "string" }, + kind: { enum: ["ins", "del", "moveFrom", "moveTo"] }, + author: { type: "string" }, + }, + }, + }, }, }, hyperlink: { $ref: "#/$defs/Hyperlink" }, @@ -208,6 +222,18 @@ const JSON_SCHEMA = { author: { type: "string" }, date: { type: "string" }, revisionId: { type: "string" }, + within: { + type: "array", + items: { + type: "object", + required: ["id", "kind", "author"], + properties: { + id: { type: "string" }, + kind: { enum: ["ins", "del", "moveFrom", "moveTo"] }, + author: { type: "string" }, + }, + }, + }, }, }, }, diff --git a/src/cli/read/markdown.ts b/src/cli/read/markdown.ts index 97c8881..e1d5a1b 100644 --- a/src/cli/read/markdown.ts +++ b/src/cli/read/markdown.ts @@ -26,6 +26,10 @@ import { } from "@core/code-block"; import { extensionForImageMime } from "@core/image/formats"; import { inlineEscapeMask } from "@core/markdown"; +import { + isRevisionVisible, + type RevisionView, +} from "@core/track-changes/revision-visibility"; import { emuToInches, formatNote, @@ -35,7 +39,7 @@ import { twipsToPoints, } from "./annotations"; -export type MarkdownView = "current" | "accepted" | "baseline"; +export type MarkdownView = RevisionView; export type MarkdownOptions = { from?: string; @@ -365,14 +369,11 @@ function emptyCommentIndex(): CommentIndex { }; } +/** Whether a text run renders in `view` — the shared all-ancestors rule, so a + * `` nested inside another author's `` is hidden in BOTH + * resolved views, exactly as `wc`/`find` and `accept`/`reject --all` treat it. */ function isRunVisible(run: TextRun, view: MarkdownView): boolean { - const kind = run.trackedChange?.kind; - if (!kind) return true; - if (view === "accepted" && (kind === "del" || kind === "moveFrom")) - return false; - if (view === "baseline" && (kind === "ins" || kind === "moveTo")) - return false; - return true; + return isRevisionVisible(run.trackedChange, view); } function buildCommentIndex( @@ -457,7 +458,7 @@ function renderTextBoxes(block: Block, ctx: RenderContext): string[] { // A box whose anchor run is tracked-deleted is gone in the accepted view // (and a tracked-inserted one absent from the baseline) — same rule as // `isRunVisible` for text. - if (!isTextBoxVisible(run, view)) continue; + if (!isRevisionVisible(run.trackedChange, view)) continue; const pairs: NotePair[] = [["anchor", anchorId]]; if (run.wrap) pairs.push(["wrap", run.wrap]); if (run.align) pairs.push(["align", run.align]); @@ -476,13 +477,6 @@ function renderTextBoxes(block: Block, ctx: RenderContext): string[] { return out; } -function isTextBoxVisible(run: TextBoxRun, view: MarkdownView): boolean { - const kind = run.trackedChange?.kind; - if (view === "current" || !kind) return true; - if (view === "accepted") return kind !== "del" && kind !== "moveFrom"; - return kind !== "ins" && kind !== "moveTo"; -} - /** The text-box runs anchored DIRECTLY in `block` (its own runs, or its cells' * paragraphs' runs for a table) — not the ones inside those boxes' stories, * which `renderTextBoxes` reaches recursively. */ diff --git a/src/cli/track-changes/CLAUDE.md b/src/cli/track-changes/CLAUDE.md index c6693ec..1f22b5c 100644 --- a/src/cli/track-changes/CLAUDE.md +++ b/src/cli/track-changes/CLAUDE.md @@ -59,7 +59,7 @@ What's deferred: `` inside math runs (font-property revisions on in ## Adding a tracked-change kind -Extend `TrackedChange["kind"]` (and `TrackedChangeKind`) in `core/ast/types.ts`, widen the JSON schema enum in `cli/info/schema.ts`, register it in `walkRunContainer` (`core/ast/read.ts`) so it lands in `document.trackedChangeReferences`, update `paragraphTextAccepted`/`paragraphTextBaseline` in `core/ast/text.ts` and `isRunVisible`/`criticMarkerFor`/`trackedChangeLabelFor` in `cli/read/markdown.ts`. If the kind takes part in accept/reject, handle it in `actionFor`/`applyAccept`/`applyReject` in [apply.ts](apply.ts); if its wrapper tag is ambiguous, add it to `trackedChangeKindForTag` (apply.ts) or carry an explicit `kind` on the reference. Because there's only the one reader walk, you register in exactly one place — no second walker to keep in sync. +Extend `TrackedChange["kind"]` (and `TrackedChangeKind`) in `core/ast/types.ts`, widen the JSON schema enum in `cli/info/schema.ts`, register it in `walkRunContainer` (`core/ast/read.ts`) so it lands in `document.trackedChangeReferences`, teach `isRevisionVisible` in `core/track-changes/revision-visibility.ts` which views hide it (the one predicate `paragraphTextAccepted`/`paragraphTextBaseline` in `core/ast/text.ts`, `find`, `iterateBlocks` and `cli/read/markdown.ts` all read — a nested wrapper is visible only when every enclosing one is), and update `criticMarkerFor`/`trackedChangeLabelFor` in `cli/read/markdown.ts`. If the kind takes part in accept/reject, handle it in `actionFor`/`applyAccept`/`applyReject` in [apply.ts](apply.ts); if its wrapper tag is ambiguous, add it to `trackedChangeKindForTag` (apply.ts) or carry an explicit `kind` on the reference. Because there's only the one reader walk, you register in exactly one place — no second walker to keep in sync. Table-structural kinds (rowIns/rowDel/cellIns/cellDel/tblGridChange) live outside the run/paragraph machinery: register them in `readTable` (`core/ast/read.ts`), give the `TrackedChangeReference` an explicit `kind` (their tags are ambiguous) plus any row/cell scope (`tableRow`/`tableCell` + parents) that accept/reject needs, handle them in `applyTableChange` (apply.ts), and filter whole tracked rows by view in `renderTable` (`cli/read/markdown.ts`) — they don't touch `text.ts` (no run text). GFM tables can't express cell-level changes, so only whole-row insert/delete are view-filtered. diff --git a/src/cli/track-changes/list-view.ts b/src/cli/track-changes/list-view.ts index 1c50263..054c05b 100644 --- a/src/cli/track-changes/list-view.ts +++ b/src/cli/track-changes/list-view.ts @@ -13,7 +13,7 @@ type ParagraphPropsSummary = { indent?: Record; }; -export type TrackedChangeRecord = TrackedChange & { +export type TrackedChangeRecord = Omit & { blockId: string; text: string; prior?: SectionProperties | ParagraphPropsSummary; @@ -39,8 +39,15 @@ export function collectTrackedChangeRecords( existing.text += run.text; continue; } + // Explicit fields, not a spread: `TrackedChange.within` (the enclosing + // wrapper chain of a nested revision) is an AST/view concern and stays + // off the `list` record so its shape is the same for every change. byId.set(change.id, { - ...change, + id: change.id, + kind: change.kind, + author: change.author, + date: change.date, + revisionId: change.revisionId, blockId: paragraph.id, text: run.text, }); diff --git a/src/core/ast/CLAUDE.md b/src/core/ast/CLAUDE.md index 87f0d18..0dd923e 100644 --- a/src/core/ast/CLAUDE.md +++ b/src/core/ast/CLAUDE.md @@ -28,6 +28,8 @@ This is the **only walk that assigns `tcN` ids**. `cli/track-changes/list.ts` and `core/track-changes/apply.ts` both read `Document.trackedChangeReferences` (or `new TrackChanges(document).list()`) and never re-walk — so they can't drift from the AST. +**Nested revision wrappers: a run's `trackedChange` is its INNERMOST wrapper, plus the enclosing chain in `within`.** Word nests one author's `` inside another's still-pending `` (and the mirror). `walkRunContainer` keeps `kind`/`id` as the innermost wrapper's (that's the `tcN` accept/reject acts on) and records the enclosing wrappers, outermost first, as `TrackedChange.within` (`{ id, kind, author }[]`) — absent on a top-level wrapper, so an un-nested document's AST is unchanged. **Every view consumer decides visibility with the ONE predicate `isRevisionVisible(change, view)`** ([core/track-changes/revision-visibility.ts](../track-changes/revision-visibility.ts)): a run is visible iff EVERY wrapper in the chain is (`accepted` hides `del`/`moveFrom`, `baseline` hides `ins`/`moveTo`, `current` shows all), so `read --baseline` ≡ `reject --all` and `read --accepted` ≡ `accept --all`. Consumers: `paragraphTextAccepted`/`paragraphTextBaseline` ([text.ts](text.ts), so `wc` follows), `find`'s per-view run text, `iterateBlocks({ view })` (text-box stories), and `cli/read/markdown.ts`. Never re-derive the rule from `trackedChange.kind` alone. + ## Why the reader is free functions and not a `Body` method The walk reads `document.relationships` + `document.numbering` (image/list resolution) and writes `document.trackedChangeReferences` — a Document-level map spanning body + notes. `readNoteTrackedChanges` operates on the footnote/endnote views directly. So the reader is genuinely cross-part orchestration; making it a `Body` method would force `Body` to reach sideways into its siblings, which is exactly the coupling the embedded-views split removed everywhere else. Free functions stay. @@ -38,4 +40,4 @@ Add to [types.ts](types.ts), populate in [read.ts](read.ts), then widen `cli/inf ## Adding a tracked-change kind -See [src/cli/track-changes/CLAUDE.md](../../cli/track-changes/CLAUDE.md). The summary: register it in the right place in the reader's body walk (the single source of `tcN` ids), handle it in `actionFor` / `applyAccept` / `applyReject` (`core/track-changes/apply.ts`), and surface it in `paragraphTextAccepted` / `paragraphTextBaseline` ([text.ts](text.ts)) + `cli/read/markdown.ts`. One place to register — no second walker to mirror. +See [src/cli/track-changes/CLAUDE.md](../../cli/track-changes/CLAUDE.md). The summary: register it in the right place in the reader's body walk (the single source of `tcN` ids), handle it in `actionFor` / `applyAccept` / `applyReject` (`core/track-changes/apply.ts`), and, if it affects visible text, teach `isRevisionVisible` ([core/track-changes/revision-visibility.ts](../track-changes/revision-visibility.ts)) which views hide it — `paragraphTextAccepted` / `paragraphTextBaseline` ([text.ts](text.ts)), `find`, `iterateBlocks` and `cli/read/markdown.ts` all read that one predicate. One place to register — no second walker to mirror. diff --git a/src/core/ast/document/body.ts b/src/core/ast/document/body.ts index 61788e6..2257649 100644 --- a/src/core/ast/document/body.ts +++ b/src/core/ast/document/body.ts @@ -1,4 +1,8 @@ import type { XmlNode } from "../../parser"; +import { + isRevisionVisible, + type RevisionView, +} from "../../track-changes/revision-visibility"; import { flattenImageRuns } from "../text"; import type { Block, @@ -178,7 +182,7 @@ export class Body { * `current` walks every story. */ export function* iterateBlocks( blocks: Block[], - options: { view?: "current" | "accepted" | "baseline" } = {}, + options: { view?: RevisionView } = {}, ): IterableIterator { const view = options.view ?? "current"; for (const block of blocks) { @@ -194,13 +198,7 @@ export function* iterateBlocks( if (block.type === "paragraph") { for (const run of block.runs) { if (run.type !== "textBox") continue; - const kind = run.trackedChange?.kind; - if (view === "accepted" && (kind === "del" || kind === "moveFrom")) { - continue; - } - if (view === "baseline" && (kind === "ins" || kind === "moveTo")) { - continue; - } + if (!isRevisionVisible(run.trackedChange, view)) continue; yield* iterateBlocks(run.blocks, options); } } diff --git a/src/core/ast/read.ts b/src/core/ast/read.ts index 247caba..652467a 100644 --- a/src/core/ast/read.ts +++ b/src/core/ast/read.ts @@ -627,6 +627,21 @@ function walkRunContainer( date: child.getAttribute("w:date") ?? "", revisionId: child.getAttribute("w:id") ?? "", }; + // A wrapper nested inside another (Word's `…` for a + // deletion inside a pending insertion) records its enclosing chain, + // outermost first, so every view consumer can apply the + // all-ancestors rule (`isRevisionVisible`). Top-level wrappers carry + // no `within`, so the AST of an un-nested document is unchanged. + if (trackedChange) { + change.within = [ + ...(trackedChange.within ?? []), + { + id: trackedChange.id, + kind: trackedChange.kind, + author: trackedChange.author, + }, + ]; + } context.document.trackedChangeReferences.set(trackedChangeId, { node: child, parent: container.children, diff --git a/src/core/ast/text.ts b/src/core/ast/text.ts index 3313ed4..3f8ed10 100644 --- a/src/core/ast/text.ts +++ b/src/core/ast/text.ts @@ -1,3 +1,4 @@ +import { isRevisionVisible } from "../track-changes/revision-visibility"; import { iterateBlocks } from "./document/body"; import type { Block, ImageRun, Paragraph } from "./types"; @@ -10,14 +11,13 @@ export function paragraphText(paragraph: Paragraph): string { } /** Concatenate text as it would read in the accepted view: skip runs inside - * a tracked deletion (``) or a tracked-move source (``); - * keep insertions and move destinations. */ + * a tracked deletion (``) or a tracked-move source (``) — + * or nested anywhere inside one; keep insertions and move destinations. */ export function paragraphTextAccepted(paragraph: Paragraph): string { let out = ""; for (const run of paragraph.runs) { if (run.type !== "text") continue; - const kind = run.trackedChange?.kind; - if (kind === "del" || kind === "moveFrom") continue; + if (!isRevisionVisible(run.trackedChange, "accepted")) continue; out += run.text; } return out; @@ -25,13 +25,13 @@ export function paragraphTextAccepted(paragraph: Paragraph): string { /** Concatenate text as it would read in the baseline (pre-change) view: skip * runs inside a tracked insertion (``) or a tracked-move destination - * (``); keep deletions and move sources. */ + * (``) — or nested anywhere inside one; keep deletions and move + * sources. */ export function paragraphTextBaseline(paragraph: Paragraph): string { let out = ""; for (const run of paragraph.runs) { if (run.type !== "text") continue; - const kind = run.trackedChange?.kind; - if (kind === "ins" || kind === "moveTo") continue; + if (!isRevisionVisible(run.trackedChange, "baseline")) continue; out += run.text; } return out; diff --git a/src/core/ast/types.ts b/src/core/ast/types.ts index aed022a..f5a9e4d 100644 --- a/src/core/ast/types.ts +++ b/src/core/ast/types.ts @@ -428,6 +428,14 @@ export type TrackedChange = { author: string; date: string; revisionId: string; + /** The run-level revision wrappers ENCLOSING this one, outermost first. + * Present only when the wrapper is nested inside another — Word's shape for + * one author deleting inside another's still-pending insertion + * (`…`). `kind`/`id` stay the INNERMOST + * wrapper's; a run is visible in a view only when EVERY wrapper in the chain + * is (`isRevisionVisible` in `core/track-changes/revision-visibility.ts`): rejecting + * the outer insertion removes the nested deletion with it. */ + within?: { id: string; kind: TrackedChangeKind; author: string }[]; }; /** OOXML revision-tracking wrappers we surface in the AST. diff --git a/src/core/comments/markers.tsx b/src/core/comments/markers.tsx index 5559eeb..2ccb290 100644 --- a/src/core/comments/markers.tsx +++ b/src/core/comments/markers.tsx @@ -15,9 +15,12 @@ import { export { generateParaId }; /** Whether a run-bearing wrapper's contents are visible in the chosen - * view. Mirrors `isWrapperVisibleInView` in `cli/replace/replace-span.tsx` - * and `isRunVisibleInView` in `core/find/index.ts` — they MUST agree, or - * `find → comments add` (and `find → replace`) misalign. */ + * view. Mirrors `isWrapperVisibleInView` in `core/find/replace-span.tsx` + * and the AST-side `isRevisionVisible` in + * `core/track-changes/revision-visibility.ts` (which `find` reads) — they + * MUST agree, or `find → comments add` (and `find → replace`) misalign. A + * hidden wrapper is never descended, so anything nested inside it is hidden + * too — the same all-ancestors rule. */ function isWrapperVisibleInView(tag: string, view: FindView): boolean { if (!isRunBearingWrapper(tag)) return false; if (view === "current") return true; diff --git a/src/core/find/index.ts b/src/core/find/index.ts index dd75344..da6b92c 100644 --- a/src/core/find/index.ts +++ b/src/core/find/index.ts @@ -1,5 +1,9 @@ import { type Body, iterateBlocks } from "../ast/document/body"; import type { Block, Paragraph, TrackedChange } from "../ast/types"; +import { + isRevisionVisible, + type RevisionView, +} from "../track-changes/revision-visibility"; import { replaceAcrossParagraphs } from "./replace-across"; import type { ReplacementFormatting } from "./replace-span"; @@ -19,7 +23,7 @@ export type TextMatch = { trackedChanges?: TrackedChange[]; }; -export type FindView = "accepted" | "current" | "baseline"; +export type FindView = RevisionView; export type FindOptions = { regex?: boolean; @@ -348,7 +352,7 @@ export function findFormattedSpans( offset += runViewText(run, view).length; continue; } - if (!isRunVisibleInView(run.trackedChange?.kind, view)) continue; + if (!isRevisionVisible(run.trackedChange, view)) continue; if (runMatchesFilter(run, filter)) { if (spanStart === null) spanStart = offset; spanText += run.text; @@ -484,29 +488,19 @@ function paragraphTextForView(paragraph: Paragraph, view: FindView): string { * widths from this one function, so they can't disagree. */ function runViewText(run: Paragraph["runs"][number], view: FindView): string { if (run.type === "text") { - return isRunVisibleInView(run.trackedChange?.kind, view) ? run.text : ""; + return isRevisionVisible(run.trackedChange, view) ? run.text : ""; } if (run.type === "tab") { - return isRunVisibleInView(run.trackedChange?.kind, view) ? "\t" : ""; + return isRevisionVisible(run.trackedChange, view) ? "\t" : ""; } if (run.type === "break") { - return run.kind === "line" && - isRunVisibleInView(run.trackedChange?.kind, view) + return run.kind === "line" && isRevisionVisible(run.trackedChange, view) ? "\n" : ""; } return ""; } -function isRunVisibleInView( - kind: TrackedChange["kind"] | undefined, - view: FindView, -): boolean { - if (view === "current") return true; - if (view === "accepted") return kind !== "del" && kind !== "moveFrom"; - return kind !== "ins" && kind !== "moveTo"; -} - function normalizeQuery(query: string): { normalized: string; applied: NormalizationKind[]; diff --git a/src/core/find/replace-span.tsx b/src/core/find/replace-span.tsx index da44121..1a62f60 100644 --- a/src/core/find/replace-span.tsx +++ b/src/core/find/replace-span.tsx @@ -30,8 +30,10 @@ export type ReplacementFormatting = { /** Whether a run-bearing wrapper's contents should be treated as VISIBLE in * the chosen view. Invisible wrappers pass through replace's offset * arithmetic untouched (their inner text adds nothing to the offset and - * spans don't slice into them). Mirrors `isRunVisibleInView` in - * src/core/find/index.ts so find/replace stay in sync. (Exported for + * spans don't slice into them). The tag-side twin of `isRevisionVisible` in + * core/track-changes/revision-visibility.ts (the AST-side rule `find` reads); + * because a hidden wrapper is never descended, a wrapper nested inside a + * hidden one is hidden too — the same all-ancestors rule. (Exported for * replace-across.tsx, which walks the same offset space over whole * paragraphs.) */ export function isWrapperVisibleInView(tag: string, view: FindView): boolean { diff --git a/src/core/track-changes/CLAUDE.md b/src/core/track-changes/CLAUDE.md index b6f70a5..9725645 100644 --- a/src/core/track-changes/CLAUDE.md +++ b/src/core/track-changes/CLAUDE.md @@ -1,6 +1,6 @@ # src/core/track-changes — `TrackChanges` lens, accept/reject machinery, tracked-range shapes -Five files behind the `@core/track-changes` barrel ([index.tsx](index.tsx)), plus one `core/edit` imports directly: +Five files behind the `@core/track-changes` barrel ([index.tsx](index.tsx)), plus two imported directly by their consumers (`core/edit`, and the view readers): - [index.tsx](index.tsx) — the `TrackChanges` lens + small free helpers (`resolveAuthor`, `resolveDate`, `convertTextToDelText`) - [apply.ts](apply.ts) — accept/reject state machine: `previewTrackedChanges`, `applyTrackedChanges`, `applyTrackedDecisions` (the mixed accept+reject pass behind `track-changes apply` — both lists resolved against the pre-mutation tree, applied in one reverse-preorder traversal via the shared `applyResolvedTargets` core), `collectTrackedChanges`, the `actionFor` dispatch table, body-side note pairing, table-grid resync @@ -8,6 +8,11 @@ Five files behind the `@core/track-changes` barrel ([index.tsx](index.tsx)), plu - [replace.tsx](replace.tsx) — range edit/delete shapes empirically validated against Word: `applyTrackedRangeReplace`, `applyTrackedRangeDelete`, `applyUntrackedRangeReplace`, `applyUntrackedRangeDelete`, the shared `assertParagraphOnlyTrackedRange` guard + `TrackedRangeConflictError`, `applyFormattingPreservingEdit` (the word-level diff for `edit --text`), and `removeParagraphLine` — the cell-safe single-paragraph removal shared by `docx delete --at pN` and `docx edit --at pN --text ""` (a ``'s last paragraph is blanked, not deleted, so we never emit an invalid empty ``) - [preserve-formatting.tsx](preserve-formatting.tsx) — the LCS-based word-level diff that drives `applyFormattingPreservingEdit`: `extractOldTokens`, `tokenize`, `diffTokens`, `buildTrackedRuns`, `buildUntrackedRuns` - [pending-authors.ts](pending-authors.ts) — `watchPendingRevisions`, the before/after snapshot `Edit.paragraph`/`Edit.range` use to report the other authors whose pending revisions a content rebuild resolved (issue #17; not re-exported by the barrel) +- [revision-visibility.ts](revision-visibility.ts) — `isRevisionVisible(change, view)`, the ONE rule for whether a run shows in `current`/`accepted`/`baseline` (not re-exported by the barrel; `core/ast/text.ts`, `core/ast/document/body.ts`, `core/find/index.ts` and `cli/read/markdown.ts` import it directly). See "Nested revisions" below. + +## Nested revisions — the all-ancestors visibility rule + +Word nests one author's `` inside another author's still-pending `` (`……`), and the mirror (`` inside ``). The reader keeps the run's `trackedChange` as the INNERMOST wrapper (its `tcN` is what accept/reject targets) and records the enclosing chain in `TrackedChange.within` (see [ast/CLAUDE.md](../ast/CLAUDE.md)). **A run is visible in a view iff EVERY enclosing wrapper is visible in that view** — `accepted` hides `del`/`moveFrom`, `baseline` hides `ins`/`moveTo`, `current` shows all — because rejecting the outer insertion removes the nested deletion with it, and accepting the outer deletion removes the nested insertion. That is exactly what the reverse-pre-order `accept --all`/`reject --all` produces, so `read --baseline` ≡ `reject --all` and `read --accepted` ≡ `accept --all` (and `wc`/`find` agree). Classifying by the innermost wrapper alone showed the nested deletion's text in `--baseline`. The XML-side LENGTH/PLACEMENT walkers (`sumVisibleTextLength`/`isWrapperVisibleInView` in [find/replace-span.tsx](../find/replace-span.tsx), [find/replace-across.tsx](../find/replace-across.tsx) and [comments/markers.tsx](../comments/markers.tsx)) never descend a wrapper hidden in the view, so a wrapper nested inside a hidden one contributes nothing — the same rule, structurally; keep it that way. (Known gap, separate from views: `splitWrapperAcrossSpan` in replace-span.tsx does not recurse into a wrapper nested inside the wrapper it splits — a `replace` span that crosses a nested ``/`` inside a `` slices at the wrong offsets.) ## TrackChanges lens API @@ -83,4 +88,4 @@ rPr equality is structural (XML-string compare on cloned nodes). The codebase's ## Adding a new tracked-change kind -See [cli/track-changes/CLAUDE.md "Adding a tracked-change kind"](../../cli/track-changes/CLAUDE.md). The summary: extend `TrackedChange["kind"]` in `core/ast/types.ts`, widen `cli/info/schema.ts`, register in `walkRunContainer` in `core/ast/read.ts` (the single source of `tcN` ids), and handle in `actionFor` / `applyAccept` / `applyReject` here. If the wrapper tag is ambiguous (table-structural, checkboxToggle), set `reference.kind` explicitly in the reader so `changeFoundFromReference` doesn't have to guess; otherwise `trackedChangeKindForTag` covers it. Surface in `paragraphTextAccepted` / `paragraphTextBaseline` ([core/ast/text.ts](../ast/text.ts)) and `cli/read/markdown.ts` if the kind affects visible text. +See [cli/track-changes/CLAUDE.md "Adding a tracked-change kind"](../../cli/track-changes/CLAUDE.md). The summary: extend `TrackedChange["kind"]` in `core/ast/types.ts`, widen `cli/info/schema.ts`, register in `walkRunContainer` in `core/ast/read.ts` (the single source of `tcN` ids), and handle in `actionFor` / `applyAccept` / `applyReject` here. If the wrapper tag is ambiguous (table-structural, checkboxToggle), set `reference.kind` explicitly in the reader so `changeFoundFromReference` doesn't have to guess; otherwise `trackedChangeKindForTag` covers it. If the kind affects visible text, teach `isRevisionVisible` ([revision-visibility.ts](revision-visibility.ts)) which views hide it — `paragraphTextAccepted` / `paragraphTextBaseline` ([core/ast/text.ts](../ast/text.ts)), `find`, `iterateBlocks` and `cli/read/markdown.ts` all read that one predicate. diff --git a/src/core/track-changes/revision-visibility.ts b/src/core/track-changes/revision-visibility.ts new file mode 100644 index 0000000..561084f --- /dev/null +++ b/src/core/track-changes/revision-visibility.ts @@ -0,0 +1,35 @@ +import type { TrackedChange, TrackedChangeKind } from "../ast/types"; + +/** Whether a run carrying `change` is visible in `view` — the ONE rule every + * view consumer shares (`paragraphTextAccepted`/`paragraphTextBaseline`, so + * `wc` follows; `find`'s per-view text; `iterateBlocks({ view })`; the + * markdown renderer). `accepted` hides `del`/`moveFrom`, `baseline` hides + * `ins`/`moveTo`, `current` shows everything — and a run is visible only when + * EVERY enclosing wrapper is: a `` nested inside another author's + * `` is hidden in BOTH resolved views, because rejecting the insertion + * removes everything inside it (the deletion included) and accepting it still + * leaves the deletion to be accepted. The XML-side length/placement walkers + * (`sumVisibleTextLength` in find/replace-span.tsx, find/replace-across.tsx + * and comments/markers.tsx) never descend a hidden wrapper, so they follow + * the same rule structurally. */ +export function isRevisionVisible( + change: TrackedChange | undefined, + view: RevisionView, +): boolean { + if (view === "current" || !change) return true; + if (!isKindVisible(change.kind, view)) return false; + for (const outer of change.within ?? []) { + if (!isKindVisible(outer.kind, view)) return false; + } + return true; +} + +export type RevisionView = "current" | "accepted" | "baseline"; + +function isKindVisible( + kind: TrackedChangeKind, + view: Exclude, +): boolean { + if (view === "accepted") return kind !== "del" && kind !== "moveFrom"; + return kind !== "ins" && kind !== "moveTo"; +} diff --git a/tests/cli/find.test.ts b/tests/cli/find.test.ts index ae900a0..ea7c953 100644 --- a/tests/cli/find.test.ts +++ b/tests/cli/find.test.ts @@ -1,7 +1,7 @@ import { beforeEach, describe, expect, test } from "bun:test"; import { join } from "node:path"; import { runCli, spawnCli, tempWorkspace } from "./harness"; -import { freshFixture } from "./helpers"; +import { buildRawDoc, freshFixture, NESTED_REVISION_BODY } from "./helpers"; describe("docx find", () => { let docPath: string; @@ -769,3 +769,63 @@ describe("docx find — bold / italic / underline formatting filters", () => { ]); }); }); + +describe("docx find — nested revisions follow the all-ancestors rule", () => { + // A `` nested inside another author's `` is hidden in BOTH + // resolved views (rejecting the insertion removes the deletion with it), so + // `--baseline` must not match its text and the offsets of what follows must + // not count it. `--current` still sees everything. + async function locatorLines( + docPath: string, + ...args: string[] + ): Promise { + const result = await runCli("find", docPath, ...args); // harness injects --json + return ( + result.parsed as { matches: Array<{ locator: string }> } + ).matches.map((match) => match.locator); + } + + test("--baseline does not match the nested-deleted text", async () => { + const docPath = await buildRawDoc( + NESTED_REVISION_BODY.editorDelInIns, + "find-nested-editor", + ); + expect(await locatorLines(docPath, "disputed", "--baseline")).toEqual([]); + // Accepted (the default) sees Editor's replacement text only. + expect(await locatorLines(docPath, "disputed")).toEqual(["p0:26-34"]); + // Current sees both the nested deletion and the replacement. + expect(await locatorLines(docPath, "disputed", "--current")).toEqual([ + "p0:26-34", + "p0:41-49", + ]); + }); + + test("the mirror (ins inside del) is invisible to both resolved views", async () => { + const docPath = await buildRawDoc( + NESTED_REVISION_BODY.insInDel, + "find-nested-ins-in-del", + ); + expect(await locatorLines(docPath, "added")).toEqual([]); + expect(await locatorLines(docPath, "added", "--baseline")).toEqual([]); + expect(await locatorLines(docPath, "added", "--current")).toEqual([ + "p0:22-27", + ]); + }); + + test("a inside the nested deletion loses its offset slot too", async () => { + const docPath = await buildRawDoc( + NESTED_REVISION_BODY.breakInNestedDel, + "find-nested-break", + ); + // baseline "Start end": the hidden break no longer pushes "end" to 7. + expect(await locatorLines(docPath, "end", "--baseline")).toEqual([ + "p0:6-9", + ]); + expect(await locatorLines(docPath, "beta", "--baseline")).toEqual([]); + // accepted "Start alphaend"; current "Start alpha\nbetaend". + expect(await locatorLines(docPath, "end")).toEqual(["p0:11-14"]); + expect(await locatorLines(docPath, "end", "--current")).toEqual([ + "p0:16-19", + ]); + }); +}); diff --git a/tests/cli/helpers.ts b/tests/cli/helpers.ts index fc68c79..87e4f41 100644 --- a/tests/cli/helpers.ts +++ b/tests/cli/helpers.ts @@ -153,3 +153,52 @@ export function wordTextBoxParagraphXml( `` ); } + +/** `` paragraphs with one revision wrapper NESTED inside another — the + * shape Word writes when a second author deletes inside a still-pending + * insertion (or inserts inside a pending deletion). Shared by the + * `read`/`find`/`wc` view tests, which assert the all-ancestors rule: a run is + * visible in a view only when EVERY enclosing wrapper is (`isRevisionVisible`), + * so `--baseline` ≡ `reject --all` and `--accepted` ≡ `accept --all`. Feed one + * to `buildRawDoc`. */ +export const NESTED_REVISION_BODY = { + /** Editor's `` inside Reviewer A's ``, then Editor's own + * ``. baseline "The Client " / accepted "The Client may withhold a + * disputed sum." */ + editorDelInIns: + 'The Client ' + + 'may withhold a ' + + 'disputed amount' + + 'disputed sum' + + '.', + /** Reviewer B's `` inside Reviewer A's ``. baseline "Fees are + * due. " / accepted "Fees are due. The Client may withhold disputed amount." */ + bDelInAIns: + 'Fees are due. ' + + 'The Client may withhold ' + + 'any ' + + "disputed amount.", + /** The mirror: Reviewer B's `` inside Reviewer A's ``. + * baseline "Keep this. Old clause goes away." / accepted "Keep this. " */ + insInDel: + 'Keep this. ' + + 'Old clause ' + + 'added inside ' + + "goes away.", + /** A `` inside the nested ``: the break's offset slot must + * vanish with it. baseline "Start end" / accepted "Start alphaend". */ + breakInNestedDel: + 'Start ' + + 'alpha' + + 'beta' + + "end", +} as const; + +/** The prose of a `read` (markdown) output: `docx:`/locator comments and blank + * lines dropped, whitespace collapsed — for comparing two views' TEXT. */ +export function markdownProse(markdown: string): string { + return markdown + .replace(//g, "") + .replace(/\s+/g, " ") + .trim(); +} diff --git a/tests/cli/track-changes.test.ts b/tests/cli/track-changes.test.ts index ba08dd5..1b156e5 100644 --- a/tests/cli/track-changes.test.ts +++ b/tests/cli/track-changes.test.ts @@ -2,7 +2,12 @@ import { describe, expect, test } from "bun:test"; import { join } from "node:path"; import { Pkg } from "@core/ast/document/package"; import { runCli, spawnCli, tempWorkspace } from "./harness"; -import { freshFixture as copyFixture } from "./helpers"; +import { + buildRawDoc, + freshFixture as copyFixture, + markdownProse, + NESTED_REVISION_BODY, +} from "./helpers"; describe("docx track-changes", () => { test("on creates settings.xml and registers it", async () => { @@ -1791,3 +1796,143 @@ describe("docx track-changes apply (mixed accept+reject)", () => { expect(await listCount(docPath)).toBe(2); }); }); + +describe("nested revisions — a run is visible only when EVERY enclosing wrapper is", () => { + // Word nests one author's `` inside another's still-pending + // ``. Classifying the run by its innermost wrapper alone showed the + // deleted text in `--baseline` (rejecting the insertion removes everything + // inside it). The resolved views must equal `reject --all` / `accept --all`. + async function resolvedViews( + body: string, + label: string, + ): Promise<{ + baseline: string; + accepted: string; + rejectedAll: string; + acceptedAll: string; + }> { + const docPath = await buildRawDoc(body, `${label}-views`); + const baseline = markdownProse( + (await runCli("read", docPath, "--baseline")).stdout, + ); + const accepted = markdownProse( + (await runCli("read", docPath, "--accepted")).stdout, + ); + const rejectPath = await buildRawDoc(body, `${label}-reject`); + expect( + (await runCli("track-changes", "reject", rejectPath, "--all")).exitCode, + ).toBe(0); + const rejectedAll = markdownProse( + (await runCli("read", rejectPath)).stdout, + ); + const acceptPath = await buildRawDoc(body, `${label}-accept`); + expect( + (await runCli("track-changes", "accept", acceptPath, "--all")).exitCode, + ).toBe(0); + const acceptedAll = markdownProse( + (await runCli("read", acceptPath)).stdout, + ); + return { baseline, accepted, rejectedAll, acceptedAll }; + } + + test("Editor's del inside Reviewer A's ins: baseline ≡ reject --all, accepted ≡ accept --all", async () => { + const views = await resolvedViews( + NESTED_REVISION_BODY.editorDelInIns, + "nested-editor", + ); + expect(views.baseline).toBe("The Client"); + expect(views.baseline).toBe(views.rejectedAll); + expect(views.accepted).toBe("The Client may withhold a disputed sum."); + expect(views.accepted).toBe(views.acceptedAll); + }); + + test("Reviewer B's del inside Reviewer A's ins: baseline ≡ reject --all, accepted ≡ accept --all", async () => { + const views = await resolvedViews( + NESTED_REVISION_BODY.bDelInAIns, + "nested-b-in-a", + ); + expect(views.baseline).toBe("Fees are due."); + expect(views.baseline).toBe(views.rejectedAll); + expect(views.accepted).toBe( + "Fees are due. The Client may withhold disputed amount.", + ); + expect(views.accepted).toBe(views.acceptedAll); + }); + + test("the mirror — ins inside del — is hidden in BOTH resolved views", async () => { + const views = await resolvedViews( + NESTED_REVISION_BODY.insInDel, + "nested-ins-in-del", + ); + expect(views.baseline).toBe("Keep this. Old clause goes away."); + expect(views.baseline).toBe(views.rejectedAll); + expect(views.accepted).toBe("Keep this."); + expect(views.accepted).toBe(views.acceptedAll); + }); + + test("--current still shows every nested run as CriticMarkup", async () => { + const docPath = await buildRawDoc( + NESTED_REVISION_BODY.editorDelInIns, + "nested-current", + ); + const out = (await runCli("read", docPath, "--current")).stdout; + expect(out).toContain( + "{++may withhold a ++}[^tc0]{--disputed amount--}[^tc1]{++disputed sum++}[^tc2]{++.++}[^tc3]", + ); + }); + + test("AST: the nested run keeps its innermost kind/id and carries the enclosing chain in `within`", async () => { + const docPath = await buildRawDoc( + NESTED_REVISION_BODY.editorDelInIns, + "nested-ast", + ); + const result = await runCli("read", docPath, "--ast"); + expect(result.exitCode).toBe(0); + const doc = result.parsed as { + blocks: Array<{ + runs?: Array<{ + text?: string; + trackedChange?: { + id: string; + kind: string; + within?: Array<{ id: string; kind: string; author: string }>; + }; + }>; + }>; + }; + const runs = doc.blocks[0]?.runs ?? []; + const nested = runs.find((run) => run.text === "disputed amount"); + expect(nested?.trackedChange).toMatchObject({ + id: "tc1", + kind: "del", + author: "Editor", + within: [{ id: "tc0", kind: "ins", author: "Reviewer A" }], + }); + // Top-level wrappers carry no `within` at all, so an un-nested document's + // AST is byte-identical to before. + const outer = runs.find((run) => run.text === "may withhold a "); + expect(outer?.trackedChange).toMatchObject({ id: "tc0", kind: "ins" }); + expect(outer?.trackedChange).not.toHaveProperty("within"); + const plain = runs.find((run) => run.text === "The Client "); + expect(plain?.trackedChange).toBeUndefined(); + }); + + test("`track-changes list` keeps its record shape (no `within`) for nested changes", async () => { + const docPath = await buildRawDoc( + NESTED_REVISION_BODY.editorDelInIns, + "nested-list", + ); + const result = await runCli("track-changes", "list", docPath, "--json"); + expect(result.exitCode).toBe(0); + const records = result.parsed as Array>; + expect(records.map((record) => record.id)).toEqual([ + "tc0", + "tc1", + "tc2", + "tc3", + ]); + for (const record of records) { + expect(record).not.toHaveProperty("within"); + } + }); +}); diff --git a/tests/cli/wc.test.ts b/tests/cli/wc.test.ts index ce160d6..0f5dfb4 100644 --- a/tests/cli/wc.test.ts +++ b/tests/cli/wc.test.ts @@ -1,6 +1,7 @@ import { beforeEach, describe, expect, test } from "bun:test"; import { join } from "node:path"; import { runCli, tempWorkspace } from "./harness"; +import { buildRawDoc, NESTED_REVISION_BODY } from "./helpers"; describe("docx wc", () => { let docPath: string; @@ -217,3 +218,64 @@ describe("docx wc — tracked changes", () => { expect((baseline.parsed as { words: number }).words).toBe(6); }); }); + +describe("docx wc — nested revisions follow the all-ancestors rule", () => { + // `--baseline` counts what `reject --all` leaves, `--accepted` what + // `accept --all` leaves — a `` nested in another author's `` + // is hidden in both. + async function words(docPath: string, ...args: string[]): Promise { + const result = await runCli("wc", docPath, ...args); + expect(result.exitCode).toBe(0); + return (result.parsed as { words: number }).words; + } + + async function resolvedCounts( + body: string, + label: string, + ): Promise<{ afterRejectAll: number; afterAcceptAll: number }> { + const rejectPath = await buildRawDoc(body, `${label}-reject`); + await runCli("track-changes", "reject", rejectPath, "--all"); + const acceptPath = await buildRawDoc(body, `${label}-accept`); + await runCli("track-changes", "accept", acceptPath, "--all"); + return { + afterRejectAll: await words(rejectPath), + afterAcceptAll: await words(acceptPath), + }; + } + + test("Editor's del inside Reviewer A's ins", async () => { + const body = NESTED_REVISION_BODY.editorDelInIns; + const docPath = await buildRawDoc(body, "wc-nested-editor"); + const resolved = await resolvedCounts(body, "wc-nested-editor"); + // baseline "The Client " = 2; accepted "The Client may withhold a + // disputed sum." = 7; current counts every run = 8. + expect(await words(docPath, "--baseline")).toBe(2); + expect(await words(docPath, "--baseline")).toBe(resolved.afterRejectAll); + expect(await words(docPath, "--accepted")).toBe(7); + expect(await words(docPath, "--accepted")).toBe(resolved.afterAcceptAll); + expect(await words(docPath, "--current")).toBe(8); + }); + + test("Reviewer B's del inside Reviewer A's ins", async () => { + const body = NESTED_REVISION_BODY.bDelInAIns; + const docPath = await buildRawDoc(body, "wc-nested-b-in-a"); + const resolved = await resolvedCounts(body, "wc-nested-b-in-a"); + // baseline "Fees are due. " = 3; accepted "Fees are due. The Client may + // withhold disputed amount." = 9. + expect(await words(docPath, "--baseline")).toBe(3); + expect(await words(docPath, "--baseline")).toBe(resolved.afterRejectAll); + expect(await words(docPath, "--accepted")).toBe(9); + expect(await words(docPath, "--accepted")).toBe(resolved.afterAcceptAll); + }); + + test("the mirror — ins inside del", async () => { + const body = NESTED_REVISION_BODY.insInDel; + const docPath = await buildRawDoc(body, "wc-nested-ins-in-del"); + const resolved = await resolvedCounts(body, "wc-nested-ins-in-del"); + // baseline "Keep this. Old clause goes away." = 6; accepted "Keep this. " = 2. + expect(await words(docPath, "--baseline")).toBe(6); + expect(await words(docPath, "--baseline")).toBe(resolved.afterRejectAll); + expect(await words(docPath, "--accepted")).toBe(2); + expect(await words(docPath, "--accepted")).toBe(resolved.afterAcceptAll); + }); +});