fix: keep comment anchors within Markdown block content

README.md

@@ -10,6 +10,12 @@ suggested edits are written into the Markdown file as
 
 Collaborators' comments appear in your note as they write them.
 
+Comments on lists, tasks, headings, and quotes preserve their Markdown prefixes.
+For these blocks, select content on one source line at a time; selections across
+multiple source lines within a list item are refused. Selections starting in
+ordinary prose that span a later list, task, quote, or heading line are also
+refused.
+
 ![A comment arrives in a daily note, is read from the hover preview, and gets a reply](docs/comment-thread.gif)
 
 ## Suggestions

src/critic/anchor-range.ts

@@ -5,21 +5,28 @@ export interface AnchorRange {
 	to: number;
 }
 
-/** Length of the block syntax a line starts with: quote markers, then a
-    heading marker or a list marker with its optional task box. */
+/** Length of a line's quote/list containers, task box, and heading marker. */
 export function structuralPrefixLength(line: string): number {
-	let length = 0;
-	let rest = line;
-	for (;;) {
-		const quote = /^[\t ]*>[\t ]?/.exec(rest);
-		if (!quote) break;
-		length += quote[0].length;
-		rest = rest.slice(quote[0].length);
+	let cursor = /^[ \t]*/.exec(line)![0].length;
+	let hasPrefix = false;
+	while (cursor < line.length) {
+		const quote = /^>[ \t]*/.exec(line.slice(cursor));
+		const list = /^(?:[-+*]|\d{1,9}[.)])[ \t]+/.exec(line.slice(cursor));
+		const marker = quote ?? list;
+		if (!marker) break;
+		if (list && /^(?:\*[ \t]*){3,}$|^(?:-[ \t]*){3,}$/.test(line.slice(cursor))) break;
+		cursor += marker[0].length;
+		hasPrefix = true;
+		if (list) {
+			cursor += /^\[[ xX]\][ \t]+/.exec(line.slice(cursor))?.[0].length ?? 0;
+		}
+	}
+	const heading = /^#{1,6}[ \t]+/.exec(line.slice(cursor));
+	if (heading) {
+		cursor += heading[0].length;
+		hasPrefix = true;
 	}
-	const block =
-		/^[\t ]*(?:#{1,6}[\t ]+|(?:[-+*]|\d+[.)])[\t ]+(?:\[[ xX]\][\t ]+)?)/.exec(rest);
-	if (block) length += block[0].length;
-	return length;
+	return hasPrefix ? cursor : 0;
 }
 
 /**

src/editor/comment-draft-range.ts

@@ -0,0 +1,35 @@
+import { structuralPrefixLength, type AnchorRange } from "../critic/anchor-range";
+
+/** Null means the selection cannot safely be wrapped without moving block syntax. */
+export function commentDraftContentRange(
+	documentText: string,
+	from: number,
+	to: number,
+): AnchorRange | null {
+	if (from === to) return { from, to };
+	const lineFrom = from === 0 ? 0 : documentText.lastIndexOf("\n", from - 1) + 1;
+	const newline = documentText.indexOf("\n", from);
+	const lineTo = newline < 0 ? documentText.length : newline;
+	const line = documentText.slice(lineFrom, lineTo).replace(/\r$/, "");
+	const prefixLength = structuralPrefixLength(line);
+	if (newline >= 0 && to > newline + 1) {
+		let laterFrom = newline + 1;
+		while (laterFrom < to) {
+			const laterNewline = documentText.indexOf("\n", laterFrom);
+			const laterTo = laterNewline < 0 ? documentText.length : laterNewline;
+			if (structuralPrefixLength(documentText.slice(laterFrom, laterTo)) > 0) return null;
+			if (laterNewline < 0) break;
+			laterFrom = laterNewline + 1;
+		}
+	}
+	if (!prefixLength) return { from, to };
+	// Triple-click includes the line break. Keep review markup before it.
+	if (to > lineTo + (newline < 0 ? 0 : 1)) return null;
+	const contentFrom = Math.max(from, lineFrom + prefixLength);
+	let contentTo = Math.min(to, lineFrom + line.length);
+	if (/#{1,6}[ \t]+$/.test(line.slice(0, prefixLength))) {
+		const closing = /[ \t]+#+[ \t]*$/.exec(line);
+		if (closing) contentTo = Math.min(contentTo, lineFrom + closing.index);
+	}
+	return contentFrom < contentTo ? { from: contentFrom, to: contentTo } : null;
+}

src/main.ts

@@ -80,6 +80,7 @@ import {
 	getAddCommentTarget,
 } from "./editor/hotkeys";
 import { buildCommentDraftInsertion } from "./editor/comment-draft-anchor";
+import { commentDraftContentRange } from "./editor/comment-draft-range";
 import {
 	findCanvasEditorSurface,
 	findFocusedCanvasEditorSurface,
@@ -1402,6 +1403,15 @@ export default class RelayCommentsPlugin
 			return;
 		}
 		const editorText = surface?.editor.getValue();
+		if (editorText !== undefined) {
+			const range = commentDraftContentRange(editorText, from, to);
+			if (!range) {
+				new Notice("Select text within one block line to comment on it.");
+				return;
+			}
+			({ from, to } = range);
+			selectedText = editorText.slice(from, to);
+		}
 		const marks = editorText ? parseCriticMarkup(editorText) : [];
 		const nativeHighlights = editorText
 			? parseNativeHighlights(editorText, marks)
@@ -1550,6 +1560,13 @@ export default class RelayCommentsPlugin
 					draft.to,
 				)
 			: null;
+		const range = nativeHighlight
+			? null
+			: commentDraftContentRange(documentText, draft.from, draft.to);
+		if (!nativeHighlight && !range) {
+			new Notice("The commented text changed; select it again within one block line.");
+			return;
+		}
 		this.commentDraft = null;
 		this.clearCommentDraftAnchor();
 		if (nativeHighlight) {
@@ -1569,13 +1586,13 @@ export default class RelayCommentsPlugin
 				undefined,
 				"relay-comments",
 			);
-		} else {
-			const currentText = documentText.slice(draft.from, draft.to);
+		} else if (range) {
+			const currentText = documentText.slice(range.from, range.to);
 			const insertion = buildCommentDraftInsertion(currentText, commentMarkup);
 			refreshedEditor.replaceRange(
 				insertion,
-				refreshedEditor.offsetToPos(draft.from),
-				refreshedEditor.offsetToPos(draft.to),
+				refreshedEditor.offsetToPos(range.from),
+				refreshedEditor.offsetToPos(range.to),
 				"relay-comments",
 			);
 		}

tests/unit/critic/anchor-range.test.ts

@@ -5,6 +5,18 @@ import {
 } from "src/critic/anchor-range";
 
 describe("structuralPrefixLength", () => {
+	it.each([
+		["- > quoted", 4],
+		["> - [ ] task", 8],
+		["\t\t- [ ] task", 8],
+		["> > ### heading", 8],
+		["* * *", 0],
+		["- - -", 0],
+		["1234567890. prose", 0],
+	])("uses shared block syntax rules for %j", (line, length) => {
+		expect(structuralPrefixLength(line)).toBe(length);
+	});
+
 	it("measures heading, list, task, and quote syntax", () => {
 		expect(structuralPrefixLength("## Must fix")).toBe(3);
 		expect(structuralPrefixLength("- item")).toBe(2);

tests/unit/editor/comment-draft-range.test.ts

@@ -0,0 +1,181 @@
+import { ChangeSet, EditorSelection, EditorState } from "@codemirror/state";
+import { describe, expect, it } from "@jest/globals";
+import { parseCriticMarkup } from "src/critic/parse";
+import { narrowAnchorRange } from "src/critic/anchor-range";
+import { buildCommentDraftInsertion, mapCommentDraftAnchor } from "src/editor/comment-draft-anchor";
+import { commentDraftContentRange } from "src/editor/comment-draft-range";
+import { findNativeHighlightForSelection, parseNativeHighlights } from "src/markdown/highlights";
+
+const comment = '{{authorId="reviewer" author="Reviewer">>test<<}}';
+
+function insertComment(documentText: string, from: number, to: number): string {
+	const range = commentDraftContentRange(documentText, from, to);
+	if (!range) throw new Error("Unsupported comment range");
+	return documentText.slice(0, range.from)
+		+ buildCommentDraftInsertion(documentText.slice(range.from, range.to), comment)
+		+ documentText.slice(range.to);
+}
+
+describe("block comment content ranges", () => {
+	it.each([
+		"1. ", "12) ", "- ", "* ", "+ ",
+		"- [ ] ", "- [x] ", "- [X] ", "2. [ ] ",
+		"# ", "## ", "###### ", "> ", ">> ", "> > ",
+		"  - ", "    - ", "\t- ", "> - [ ] ", "- > ", "> ## ", "1.\t",
+	])("preserves the %j prefix and following block", (prefix) => {
+		const before = "Earlier text.\n";
+		const line = prefix + "**Selected text.**";
+		const after = "\n3. Following item.\n\nPlain paragraph.";
+		const text = before + line + after;
+		for (const trailingNewline of [0, 1]) {
+			const result = insertComment(text, before.length, before.length + line.length + trailingNewline);
+			expect(result).toBe(before + prefix + "{==**Selected text.**==}" + comment + after);
+			const marks = parseCriticMarkup(result);
+			expect(marks.map((mark) => mark.type)).toEqual(["highlight", "comment"]);
+			expect(marks.every((mark) => mark.valid)).toBe(true);
+			expect(marks[0].raw).toBe("{==**Selected text.**==}");
+			expect(marks[1].raw).toBe(comment);
+		}
+	});
+
+	it("keeps a heading's closing hashes and line break outside the comment", () => {
+		const text = "## Heading ##\nNext paragraph";
+		expect(insertComment(text, 0, text.indexOf("\n") + 1)).toBe(
+			"## {==Heading==}" + comment + " ##\nNext paragraph",
+		);
+	});
+
+	it("preserves CRLF after a whole heading selection", () => {
+		const text = "# Heading\r\nNext paragraph";
+		expect(insertComment(text, 0, text.indexOf("\n") + 1)).toBe(
+			"# {==Heading==}" + comment + "\r\nNext paragraph",
+		);
+	});
+
+	it("adjusts forward and backward CodeMirror selections identically", () => {
+		const text = "# Heading\nNext paragraph";
+		for (const [anchor, head] of [[0, 10], [10, 0]]) {
+			const state = EditorState.create({ doc: text, selection: EditorSelection.single(anchor, head) });
+			const { from, to } = state.selection.main;
+			expect(state.doc.sliceString(from, to)).toBe("# Heading\n");
+			expect(commentDraftContentRange(text, from, to)).toEqual({ from: 2, to: 9 });
+			expect(state.selection.main.anchor).toBe(anchor);
+		}
+	});
+
+	it.each([0, 1, 2, 3, 4, 5])("moves a start inside a task prefix (%i) after the box", (from) => {
+		expect(insertComment("- [ ] Task", from, 10)).toBe("- [ ] {==Task==}" + comment);
+	});
+
+	it("leaves a partial content selection at its original offsets", () => {
+		expect(insertComment("- one two three", 6, 9)).toBe("- one {==two==}" + comment + " three");
+	});
+
+	it.each(["plain text", "a\nparagraph", "#hashtag", "####### text", "1234567890. text", "-word", "\\# escaped", "---", "* * *"])(
+		"leaves ordinary or unsupported syntax unchanged: %j", (text) => {
+			expect(insertComment(text, 0, text.length)).toBe("{==" + text + "==}" + comment);
+		},
+	);
+
+	it.each(["```md\n", "~~~\n", "````\n```\n"])("matches upstream narrowing for a code-text prefix (%j)", (fence) => {
+		const text = fence + "# literal code\n";
+		expect(commentDraftContentRange(text, fence.length, text.length)).toEqual(
+			narrowAnchorRange(text, fence.length, text.length),
+		);
+		expect(insertComment(text, fence.length, text.length)).toBe(fence + "# {==literal code==}" + comment + "\n");
+	});
+
+	it("adjusts a block after a closed fence", () => {
+		const before = "```md\n# code\n```\n";
+		const text = before + "# Heading";
+		expect(insertComment(text, before.length, text.length)).toBe(before + "# {==Heading==}" + comment);
+	});
+
+	it.each(["1. first\n2. second", "- first\n  continued", "# heading\nparagraph", "> first\n> second", "- [ ] task\n- next"])(
+		"refuses a prefixed selection crossing lines: %j", (text) => {
+			expect(commentDraftContentRange(text, 0, text.length)).toBeNull();
+		},
+	);
+
+	it.each(["- ", "# ", "> ", "- [ ] "])("refuses a prefix-only selection: %j", (text) => {
+		expect(commentDraftContentRange(text, 0, text.length)).toBeNull();
+	});
+
+	it.each(["1. item", "- item", "- [ ] task", "# heading", "> quote", "> - item"])(
+		"refuses a selection starting before a later block: %j", (line) => {
+			const text = "plain text\n" + line;
+			expect(commentDraftContentRange(text, 0, text.length)).toBeNull();
+			expect(commentDraftContentRange(text, 10, text.length)).toBeNull();
+		},
+	);
+
+	it("does not include a block starting exactly at the selection end", () => {
+		const text = "plain text\n- item";
+		expect(commentDraftContentRange(text, 0, 11)).toEqual({ from: 0, to: 11 });
+	});
+
+	it("refuses even a partial later prefix using the full source line", () => {
+		const text = "plain text\n- item";
+		expect(commentDraftContentRange(text, 0, 12)).toBeNull();
+		expect(commentDraftContentRange("\n- item", 0, 2)).toBeNull();
+	});
+
+	it.each(["    - ambiguous", "\t- ambiguous", "     # ambiguous", ">    - ambiguous", "> \t# ambiguous"])(
+		"keeps indentation outside the anchor without guessing the block type: %j", (text) => {
+			expect(commentDraftContentRange(text, 0, text.length)).toEqual({ from: text.indexOf("ambiguous"), to: text.length });
+		},
+	);
+
+	it.each(["\t- ", "    - ", "\t\t- [ ] ", ">     ", "> \t- "])(
+		"allows both whole-line and content-only nested selections: %j", (prefix) => {
+			const before = "- parent\n";
+			const text = before + prefix + "b c";
+			const contentFrom = text.indexOf("b c");
+			for (const from of [before.length, contentFrom]) {
+				const range = commentDraftContentRange(text, from, text.length)!;
+				expect(range).toEqual({ from: contentFrom, to: text.length });
+				expect(narrowAnchorRange(text, range.from, range.to)).toEqual(range);
+				expect(insertComment(text, from, text.length)).toBe(before + prefix + "{==b c==}" + comment);
+			}
+		},
+	);
+
+	it.each([
+		["> ```\n", "> - literal code"],
+		["- ```\n", "  # literal code"],
+		["    ~~~\n", "    - literal code"],
+	])("keeps nested code containers outside the mark: %j", (before, line) => {
+		const text = before + line;
+		const contentFrom = text.indexOf("literal code");
+		expect(commentDraftContentRange(text, before.length, text.length)).toEqual({ from: contentFrom, to: text.length });
+		expect(insertComment(text, before.length, text.length)).toBe(text.slice(0, contentFrom) + "{==literal code==}" + comment);
+	});
+
+	it("adjusts a block after a closed nested fence", () => {
+		const before = "> ```\n> code\n> ```\n";
+		const text = before + "# Heading";
+		expect(insertComment(text, before.length, text.length)).toBe(before + "# {==Heading==}" + comment);
+	});
+
+	it("keeps empty selections at their surviving insertion point", () => {
+		expect(insertComment("# Heading", 0, 0)).toBe(comment + "# Heading");
+	});
+
+	it("finds a complete native highlight after removing the block prefix", () => {
+		const text = "# ==Heading==\nNext paragraph";
+		const range = commentDraftContentRange(text, 0, 14)!;
+		const highlight = findNativeHighlightForSelection(parseNativeHighlights(text, []), range.from, range.to);
+		expect(highlight?.raw).toBe("==Heading==");
+		const result = text.slice(0, highlight!.to) + comment + text.slice(highlight!.to);
+		expect(result).toBe("# ==Heading==" + comment + "\nNext paragraph");
+		expect(parseCriticMarkup(result).map((mark) => mark.type)).toEqual(["comment"]);
+	});
+
+	it("recomputes the content boundary after an entire mapped range is rewritten", () => {
+		const text = "old text";
+		const changes = ChangeSet.of({ from: 0, to: text.length, insert: "# New heading\n" }, text.length);
+		const draft = mapCommentDraftAnchor({ id: 7, filePath: "note.md", from: 0, to: text.length }, changes);
+		const updatedText = changes.apply(EditorState.create({ doc: text }).doc).toString();
+		expect(insertComment(updatedText, draft.from, draft.to)).toBe("# {==New heading==}" + comment + "\n");
+	});
+});