From 0c7e3651f00b1710c0a39af053da80b341c3a045 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 6 Nov 2026 01:15:03 +0000 Subject: [PATCH] fix(bookmarks): keep bookmarks through whole-paragraph edit; pair fresh raw bookmark ids A whole-paragraph `edit` dropped the paragraph's bookmark (`--markdown` rebuild) or just its `` when it sat inside a rebuilt `` (`--text`), silently breaking every cross-reference to it ("Error! Bookmark not defined." in print). The edit now lifts the paragraph's bookmark markers and puts them back around the new content (outside the ``/`` pair under tracking), and the `--text` word diff restores any marker it rebuilt away. `raw insert` split every fresh bookmark pair: `remintBookmarkIds` treated the end marker as colliding with its own start and re-minted it. The first sight of a free id now maps to itself, so both halves share it. First, field-independent slice of #40. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01LkctENj7B2nTmUttTgfBhs Triage-Routine: true --- src/core/bookmarks/markers.ts | 90 +++++++++++++++++++ src/core/edit/index.tsx | 29 ++++++ src/core/raw/references.ts | 9 +- tests/cli/edit-bookmarks.test.ts | 149 +++++++++++++++++++++++++++++++ tests/core/raw.test.ts | 18 ++++ 5 files changed, 293 insertions(+), 2 deletions(-) create mode 100644 src/core/bookmarks/markers.ts create mode 100644 tests/cli/edit-bookmarks.test.ts diff --git a/src/core/bookmarks/markers.ts b/src/core/bookmarks/markers.ts new file mode 100644 index 0000000..e02059a --- /dev/null +++ b/src/core/bookmarks/markers.ts @@ -0,0 +1,90 @@ +import { + isRunBearingWrapper, + wrapperContentNode, + type XmlNode, +} from "../parser"; + +/** Lift a paragraph's own ``/`` markers out of + * it (directly or inside a run-bearing wrapper such as ``), + * REMOVING them, so a whole-paragraph rebuild can't take them along — + * `edit --markdown` used to drop every bookmark on the paragraph, and with + * it every cross-reference that pointed there. Hand the result to + * {@link restoreBookmarkMarkers} once the new content is live. Markers inside + * a run (a text box's story) are not the paragraph's and stay with the run. */ +export function liftBookmarkMarkers(paragraph: XmlNode): BookmarkMarkers { + const markers = collectBookmarkMarkers(paragraph); + const lifted = new Set([...markers.starts, ...markers.ends]); + if (lifted.size > 0) removeMarkers(paragraph, lifted); + return markers; +} + +/** Snapshot the same markers WITHOUT removing them — for the in-place + * `--text` word diff, which keeps markers at their exact offsets unless the + * run container holding one (a ``) is rebuilt away. */ +export function collectBookmarkMarkers(paragraph: XmlNode): BookmarkMarkers { + const markers: BookmarkMarkers = { starts: [], ends: [] }; + visitMarkerScope(paragraph, (child) => { + if (child.tag === "w:bookmarkStart") markers.starts.push(child); + if (child.tag === "w:bookmarkEnd") markers.ends.push(child); + }); + return markers; +} + +/** Put back every snapshotted marker that is no longer inside `paragraph`: + * a start right after the ``, an end after the last child — so a + * bookmark that covered the paragraph covers its new content, and a + * cross-paragraph bookmark keeps the half that lived here (its partner in the + * neighbor stays paired). Markers still in place are left where they are. */ +export function restoreBookmarkMarkers( + paragraph: XmlNode, + markers: BookmarkMarkers, +): void { + // Keyed by tag + id, not node identity: a rebuild that CLONED a wrapper + // (a split ``) still carries the marker, just a copy of it. + const current = collectBookmarkMarkers(paragraph); + const present = new Set([...current.starts, ...current.ends].map(markerKey)); + const missing = (marker: XmlNode) => !present.has(markerKey(marker)); + const starts = markers.starts.filter(missing); + const ends = markers.ends.filter(missing); + if (starts.length === 0 && ends.length === 0) return; + const insertAt = paragraph.children[0]?.tag === "w:pPr" ? 1 : 0; + paragraph.children.splice(insertAt, 0, ...starts); + paragraph.children.push(...ends); +} + +function markerKey(marker: XmlNode): string { + return `${marker.tag}#${marker.getAttribute("w:id") ?? ""}`; +} + +/** Walk the paragraph's own marker scope — its children plus the contents of + * run-bearing wrappers (via `wrapperContentNode`, so an + * `` is visited through its chosen branch only). */ +function visitMarkerScope( + node: XmlNode, + visit: (child: XmlNode) => void, +): void { + for (const child of node.children) { + visit(child); + if (isRunBearingWrapper(child.tag)) { + visitMarkerScope(wrapperContentNode(child), visit); + } + } +} + +/** Drop `markers` from the same scope {@link visitMarkerScope} walks. */ +function removeMarkers(node: XmlNode, markers: Set): void { + // In place: a caller may hold this child array (a live block reference). + for (let index = node.children.length - 1; index >= 0; index--) { + const child = node.children[index]; + if (child && markers.has(child)) node.children.splice(index, 1); + } + for (const child of node.children) { + if (isRunBearingWrapper(child.tag)) { + removeMarkers(wrapperContentNode(child), markers); + } + } +} + +/** The original marker NODES (not copies), so every attribute — `w:name`, + * `w:colFirst`/`w:colLast`, `w:displacedByCustomXml` — survives the move. */ +export type BookmarkMarkers = { starts: XmlNode[]; ends: XmlNode[] }; diff --git a/src/core/edit/index.tsx b/src/core/edit/index.tsx index e60e403..50c00f2 100644 --- a/src/core/edit/index.tsx +++ b/src/core/edit/index.tsx @@ -14,6 +14,12 @@ import { priorPprChildren, wrapPprChange, } from "../blocks"; +import { + type BookmarkMarkers, + collectBookmarkMarkers, + liftBookmarkMarkers, + restoreBookmarkMarkers, +} from "../bookmarks/markers"; import { buildCodeBlockParagraphs, ensureCodeBlockStyles } from "../code-block"; import { Comments } from "../comments"; import { clearFormatting as clearRunFormatting } from "./clear-formatting"; @@ -211,6 +217,10 @@ export class Edit { const commentMarkers = extractCommentMarkers(blockRef.node); if (textEdit !== null && equationPlan) { + // The word diff keeps bookmark markers at their offsets, except one + // inside a run container it rebuilds away (a ``'s end + // marker) — put any such casualty back at the paragraph boundary. + const bookmarks = collectBookmarkMarkers(blockRef.node); applyFormattingPreservingEdit( this.document, blockRef.node, @@ -220,6 +230,7 @@ export class Edit { tracked, equationPlan.inserted, ); + restoreBookmarkMarkers(blockRef.node, bookmarks); this.reanchorComments(blockRef.node, commentMarkers); // Mutated in place — the same node is the result. return { node: blockRef.node, resolvedAuthors: listResolvedAuthors() }; @@ -241,6 +252,9 @@ export class Edit { blockRef.node, restatedPictures(newParagraphs), ); + // Bookmarks ride along the same way: the replacement never saw them, and + // a dropped bookmark silently breaks every cross-reference to it. + const bookmarks = liftBookmarkMarkers(blockRef.node); // Run inheritance goes FIRST: its `bringsNewBlockStructure` guard must // see only the structure the new content brought (a markdown `#` heading, // a list item), not the old paragraph's `` that the pPr @@ -310,7 +324,11 @@ export class Edit { // (new content appended inside ``), so the untracked carriers go // back onto it — after the replace, or they'd be swept into the ``. reattachObjectRuns(blockRef.node, objectRuns); + // Outside the ``/`` pair, so the bookmark brackets + // whichever content survives accept or reject. + restoreBookmarkMarkers(blockRef.node, bookmarks); } else { + restoreBookmarkCarrier(newParagraphs, bookmarks); if (objectRuns.leading.length > 0 || objectRuns.trailing.length > 0) { reattachObjectRuns(objectCarrierTarget(newParagraphs), objectRuns); } @@ -1037,6 +1055,17 @@ function inheritCommonRunFormatting( } } +/** Put an untracked rebuild's lifted bookmarks on the replacement's first + * paragraph — the same carrier the lifted object runs use (minting an empty + * one when the new content is all tables), so the bookmark never vanishes. */ +function restoreBookmarkCarrier( + newParagraphs: XmlNode[], + bookmarks: BookmarkMarkers, +): void { + if (bookmarks.starts.length === 0 && bookmarks.ends.length === 0) return; + restoreBookmarkMarkers(objectCarrierTarget(newParagraphs), bookmarks); +} + /** The paragraph lifted object runs go back onto for an untracked replace: the * first `` of the replacement. When the replacement has none (markdown * that built only a table, or nothing at all), a bare paragraph is prepended diff --git a/src/core/raw/references.ts b/src/core/raw/references.ts index 7905c76..cd8447e 100644 --- a/src/core/raw/references.ts +++ b/src/core/raw/references.ts @@ -164,8 +164,9 @@ function remintDrawingIds( } /** Bookmark ids are document-unique; `w:bookmarkStart`/`w:bookmarkEnd` pair by - * id, so collisions re-map through one old→new table (scoped to the fragment, - * so its pairs stay linked). */ + * id, so every id the fragment uses maps through one old→new table (scoped to + * the fragment, so its pairs stay linked) — identity for a free id, a fresh + * mint for one the document already uses. */ function remintBookmarkIds( nodes: XmlNode[], context: ReferenceAuditContext, @@ -182,7 +183,11 @@ function remintBookmarkIds( return; } if (!context.bookmarkIds.has(id)) { + // Record the first sight as an identity mapping, so the pair's + // other half reuses it instead of "colliding" with its own start + // (which split every fresh pair: start 100, end re-minted to 0). context.bookmarkIds.add(id); + remapped.set(id, id); return; } const minted = String(context.nextBookmarkId++); diff --git a/tests/cli/edit-bookmarks.test.ts b/tests/cli/edit-bookmarks.test.ts new file mode 100644 index 0000000..d559ceb --- /dev/null +++ b/tests/cli/edit-bookmarks.test.ts @@ -0,0 +1,149 @@ +import { describe, expect, test } from "bun:test"; +import { XmlNode } from "@core/parser"; +import { runCli } from "./harness"; +import { + buildRawDoc, + expectValidClean, + freshFixture, + readDocumentXml, +} from "./helpers"; + +// Issue #40 table 2: a whole-paragraph `edit` used to drop the paragraph's +// bookmark (markdown), or its `` alone when that sat inside a +// `` the `--text` rebuild removed — silently breaking every +// cross-reference to it ("Error! Bookmark not defined." in print). + +const ACM = "tests/fixtures/multi-column.docx"; +// p103 (reference [8]) holds `_Ref279752219` (w:id 13), cited from p37 + p93; +// its bookmarkEnd sits inside the paragraph's trailing ``. +const CITED = "_Ref279752219"; + +/** Body-paragraph index (among ``'s direct `` children) holding + * each half of bookmark `name`, or -1 for a missing half. */ +async function bookmarkSpan( + path: string, + name: string, +): Promise<{ + start: number; + end: number; + startCount: number; + endCount: number; +}> { + const root = XmlNode.parse(await readDocumentXml(path)); + const body = XmlNode.findRoot(root, "w:document")?.findChild("w:body"); + const paragraphs = (body?.children ?? []).filter( + (child) => child.tag === "w:p", + ); + const starts = paragraphs.flatMap((paragraph, index) => + descendants(paragraph, "w:bookmarkStart") + .filter((marker) => marker.getAttribute("w:name") === name) + .map((marker) => ({ index, id: marker.getAttribute("w:id") })), + ); + const id = starts[0]?.id; + const ends = paragraphs.flatMap((paragraph, index) => + descendants(paragraph, "w:bookmarkEnd") + .filter((marker) => marker.getAttribute("w:id") === id) + .map(() => index), + ); + return { + start: starts[0]?.index ?? -1, + end: ends[0] ?? -1, + startCount: starts.length, + endCount: ends.length, + }; +} + +function descendants(node: XmlNode, tag: string): XmlNode[] { + return node.children.flatMap((child) => [ + ...(child.tag === tag ? [child] : []), + ...descendants(child, tag), + ]); +} + +describe("whole-paragraph edit keeps the paragraph's bookmarks", () => { + test("--markdown rebuild keeps a cited bookmark paired on the paragraph", async () => { + const path = await freshFixture("bm-markdown", ACM); + const before = await bookmarkSpan(path, CITED); + const result = await runCli( + "edit", + path, + "--at", + "p103", + "Psy. 2013. *Gangnam Style*. Video.", + ); + expect(result.exitCode).toBe(0); + const after = await bookmarkSpan(path, CITED); + expect(after).toEqual(before); + expect(after.start).toBe(after.end); + await expectValidClean(path); + }); + + test("--text keeps the bookmarkEnd that lived inside the rebuilt hyperlink", async () => { + const path = await freshFixture("bm-text", ACM); + const before = await bookmarkSpan(path, CITED); + const result = await runCli( + "edit", + path, + "--at", + "p103", + "--text", + "Psy. 2013. Gangnam Style. Video.", + ); + expect(result.exitCode).toBe(0); + expect(await bookmarkSpan(path, CITED)).toEqual(before); + await expectValidClean(path); + }); + + test("the bookmark brackets the new text, so the edit reads unchanged", async () => { + const path = await buildRawDoc( + 'Old methods.', + "bm-bracket", + ); + const result = await runCli("edit", path, "--at", "p0", "New **methods**."); + expect(result.exitCode).toBe(0); + const xml = await readDocumentXml(path); + expect(xml).toMatch( + /.*New .*methods.*<\/w:p>/, + ); + const read = await runCli("read", path); + expect(read.stdout).toContain("New **methods**."); + }); + + test("a cross-paragraph bookmark keeps its half on the edited paragraph", async () => { + const path = await buildRawDoc( + 'First.Second.', + "bm-cross", + ); + const result = await runCli( + "edit", + path, + "--at", + "p0", + "*First*, revised.", + ); + expect(result.exitCode).toBe(0); + const span = await bookmarkSpan(path, "Span"); + expect(span).toEqual({ start: 0, end: 1, startCount: 1, endCount: 1 }); + }); + + for (const verdict of ["accept", "reject"] as const) { + test(`tracked --markdown edit keeps the bookmark after ${verdict}`, async () => { + const path = await freshFixture(`bm-tracked-${verdict}`, ACM); + const before = await bookmarkSpan(path, CITED); + const edited = await runCli( + "edit", + path, + "--track", + "--at", + "p103", + "Psy. 2013. *Gangnam Style*. Video.", + ); + expect(edited.exitCode).toBe(0); + expect(await bookmarkSpan(path, CITED)).toEqual(before); + const settled = await runCli("track-changes", verdict, path, "--all"); + expect(settled.exitCode).toBe(0); + expect(await bookmarkSpan(path, CITED)).toEqual(before); + await expectValidClean(path); + }); + } +}); diff --git a/tests/core/raw.test.ts b/tests/core/raw.test.ts index d50e6cd..6102396 100644 --- a/tests/core/raw.test.ts +++ b/tests/core/raw.test.ts @@ -294,6 +294,24 @@ describe("gate 5 — reference/id integrity", () => { expect(end).toBe(start); }); + test("a fresh (non-colliding) bookmark pair keeps one shared id", async () => { + const document = await openFixture( + "minimal.docx", + "ref-bookmark-fresh.docx", + ); + const prepared = new Raw(document).prepareFragment( + 'Methods', + "blocks", + ); + const paragraph = prepared.nodes[0]; + expect( + paragraph?.findDescendant("w:bookmarkStart")?.getAttribute("w:id"), + ).toBe("100"); + expect( + paragraph?.findDescendant("w:bookmarkEnd")?.getAttribute("w:id"), + ).toBe("100"); + }); + test("unknown style and numId warn instead of rejecting", async () => { const document = await openFixture("minimal.docx", "ref-warn.docx"); const prepared = new Raw(document).prepareFragment(