fix(bookmarks): keep bookmarks through whole-paragraph edit; pair fresh raw bookmark ids

src/core/bookmarks/markers.ts

@@ -0,0 +1,90 @@
+import {
+	isRunBearingWrapper,
+	wrapperContentNode,
+	type XmlNode,
+} from "../parser";
+
+/** Lift a paragraph's own `<w:bookmarkStart>`/`<w:bookmarkEnd>` markers out of
+ *  it (directly or inside a run-bearing wrapper such as `<w:hyperlink>`),
+ *  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 `<w:hyperlink>`) 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 `<w:pPr>`, 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 `<w:hyperlink>`) 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
+ *  `<mc:AlternateContent>` 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<XmlNode>): 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[] };

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 `<w:hyperlink>`'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 <w:p> 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 `<w:pStyle>` that the pPr
@@ -310,7 +324,11 @@ export class Edit {
 			// (new content appended inside `<w:ins>`), so the untracked carriers go
 			// back onto it — after the replace, or they'd be swept into the `<w:del>`.
 			reattachObjectRuns(blockRef.node, objectRuns);
+			// Outside the `<w:del>`/`<w:ins>` 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 `<w:p>` of the replacement. When the replacement has none (markdown
  *  that built only a table, or nothing at all), a bare paragraph is prepended

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++);

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 `<w:bookmarkEnd>` alone when that sat inside a
+// `<w:hyperlink>` 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 `<w:hyperlink>`.
+const CITED = "_Ref279752219";
+
+/** Body-paragraph index (among `<w:body>`'s direct `<w:p>` 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(
+			'<w:p><w:bookmarkStart w:id="7" w:name="Methods"/><w:r><w:t>Old methods.</w:t></w:r><w:bookmarkEnd w:id="7"/></w:p>',
+			"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(
+			/<w:bookmarkStart w:id="7" w:name="Methods"\/>.*New .*methods.*<w:bookmarkEnd w:id="7"\/><\/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(
+			'<w:p><w:bookmarkStart w:id="3" w:name="Span"/><w:r><w:t>First.</w:t></w:r></w:p><w:p><w:r><w:t>Second.</w:t></w:r><w:bookmarkEnd w:id="3"/></w:p>',
+			"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);
+		});
+	}
+});

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(
+			'<w:p><w:bookmarkStart w:id="100" w:name="_Ref100"/><w:r><w:t>Methods</w:t></w:r><w:bookmarkEnd w:id="100"/></w:p>',
+			"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(