diff --git a/packages/ui/hooks/useAnnotationHighlighter.test.tsx b/packages/ui/hooks/useAnnotationHighlighter.test.tsx index dff134cef..36e4c017d 100644 --- a/packages/ui/hooks/useAnnotationHighlighter.test.tsx +++ b/packages/ui/hooks/useAnnotationHighlighter.test.tsx @@ -353,3 +353,126 @@ describe('useAnnotationHighlighter math annotations', () => { host.remove(); }); }); + +// --- Source-markdown quotes anchoring against rendered text ----------------- + +function QuoteHarness({ annotations }: { annotations: Annotation[] }) { + const containerRef = useRef(null); + const hook = useAnnotationHighlighter({ + containerRef, + annotations: [], + selectedAnnotationId: null, + mode: 'comment', + onAddAnnotation: () => {}, + }); + + React.useEffect(() => { + // Mirrors the external-annotation restore path: no startMeta/endMeta, so + // the quote is the only address the annotation has. + const timer = setTimeout(() => hook.applyAnnotations(annotations), 0); + return () => clearTimeout(timer); + }, [annotations]); + + return ( +
+ {/* What the renderer produces: no backticks, no asterisks, and table + cells with nothing between them. */} +

+ The config.ts switch is required here. +

+

A duplicated phrase.

+

A duplicated phrase.

+ + + + + + + +
WP5mcpAuth.ts
+
+ ); +} + +const quoteAnnotation = (id: string, originalText: string): Annotation => ({ + id, + blockId: 'external', + startOffset: 0, + endOffset: 0, + type: AnnotationType.COMMENT, + text: 'a remark', + originalText, + createdA: 1, +}); + +async function mountQuotes(annotations: Annotation[]) { + const host = document.createElement('div'); + document.body.appendChild(host); + const root = createRoot(host); + await act(async () => { + root.render(); + }); + await act(async () => { + await new Promise(resolve => setTimeout(resolve, 20)); + }); + return { + host, + cleanup: () => { + act(() => root.unmount()); + host.remove(); + }, + }; +} + +describe.if(hasDom)('useAnnotationHighlighter — source-markdown quotes', () => { + test('anchors a quote whose inline markdown the renderer consumed', async () => { + // The quote is verbatim from the SOURCE, where the code span still has + // its backticks and the emphasis its asterisks. Before the strip tiers + // this found nothing and the annotation stayed sidebar-only. + const { host, cleanup } = await mountQuotes([ + quoteAnnotation('a1', 'The `config.ts` switch is **required** here.'), + ]); + try { + expect(host.querySelectorAll('[data-bind-id="a1"]').length).toBeGreaterThan(0); + } finally { + cleanup(); + } + }); + + test('anchors a table row, whose cells render with nothing between them', async () => { + const { host, cleanup } = await mountQuotes([ + quoteAnnotation('a2', '| WP5 | `mcpAuth.ts` |'), + ]); + try { + expect(host.querySelectorAll('[data-bind-id="a2"]').length).toBeGreaterThan(0); + } finally { + cleanup(); + } + }); + + test('refuses to guess when the stripped quote is no longer unique', async () => { + // Stripping shortens the needle, and the search takes the first hit — so a + // quote that becomes ambiguous must anchor nowhere rather than highlight + // whichever paragraph happens to come first. + const { host, cleanup } = await mountQuotes([ + quoteAnnotation('a3', 'A **duplicated** phrase.'), + ]); + try { + expect(host.querySelectorAll('[data-bind-id="a3"]').length).toBe(0); + } finally { + cleanup(); + } + }); + + test('still anchors a quote that matches the page verbatim', async () => { + // The literal tier is unchanged: first-match-wins, no uniqueness demand. + const { host, cleanup } = await mountQuotes([ + quoteAnnotation('a4', 'A duplicated phrase.'), + ]); + try { + expect(host.querySelectorAll('[data-bind-id="a4"]').length).toBeGreaterThan(0); + } finally { + cleanup(); + } + }); +}); diff --git a/packages/ui/hooks/useAnnotationHighlighter.ts b/packages/ui/hooks/useAnnotationHighlighter.ts index c32df6fcb..1d52b23b9 100644 --- a/packages/ui/hooks/useAnnotationHighlighter.ts +++ b/packages/ui/hooks/useAnnotationHighlighter.ts @@ -11,7 +11,7 @@ import type { Annotation, EditorMode, ImageAttachment } from '../types'; import { AnnotationType } from '../types'; import type { QuickLabel } from '../utils/quickLabels'; import { getIdentity } from '../utils/identity'; -import { transformPlainText } from '../utils/inlineTransforms'; +import { stripInlineMarkdown, stripTableCellDelimiters, transformPlainText } from '../utils/inlineTransforms'; // --- Exported state types --- @@ -469,16 +469,61 @@ export function useAnnotationHighlighter({ return null; }; - // First try the literal text. If that misses, re-try with the same - // transform the renderer applies to plain text (emoji shortcodes + - // smart punctuation) so annotations made before those transforms - // shipped can still re-bind to their target after reload. + // The tiers below make the needle shorter and more generic, and + // `searchOnce` takes the FIRST hit — so a stripped quote that now appears + // in two places would silently highlight the wrong one. A highlight on + // text the comment was never about is worse than no highlight, so the + // permissive tiers refuse to guess: they anchor only when the transformed + // needle is unique. The literal tiers keep first-match-wins, which is the + // behavior that already shipped. + const searchIfUnique = (needle: string): Range | null => { + const container = containerRef.current; + if (!needle || !container) return null; + const collapse = (v: string) => v.replace(/\s+/g, ' ').trim(); + const haystack = collapse(container.textContent || ''); + const target = collapse(needle); + if (!target) return null; + let count = 0; + let from = haystack.indexOf(target); + while (from !== -1 && count < 2) { + count += 1; + from = haystack.indexOf(target, from + target.length); + } + if (count !== 1) return null; + return searchOnce(needle); + }; + + // Tiers, least destructive first. Each re-tries the search with one more + // of the transforms the renderer applies, so a quote taken from the + // document's SOURCE can still find its RENDERED target. A quote that + // matches the page verbatim returns on the first tier and never reaches + // the rest. const direct = searchOnce(searchText); if (direct) return direct; + // Emoji shortcodes + smart punctuation, so annotations made before those + // transforms shipped still re-bind to their target after reload. const transformed = transformPlainText(searchText); if (transformed !== searchText) { - return searchOnce(transformed); + const viaTransform = searchOnce(transformed); + if (viaTransform) return viaTransform; + } + + // Inline markdown the renderer consumes. An external annotation quotes + // the markdown a tool read from the server, where `code` still carries + // its backticks; the DOM has none of that syntax. + const stripped = stripInlineMarkdown(searchText); + if (stripped !== searchText) { + const viaStrip = searchIfUnique(stripped); + if (viaStrip) return viaStrip; + } + + // Last: a table row, whose cells render as adjacent elements with nothing + // between them. Deleting the padding around a `|` mangles ordinary prose, + // which is why it is only reached once everything else has failed. + const withoutCells = stripTableCellDelimiters(searchText); + if (withoutCells !== stripped) { + return searchIfUnique(withoutCells); } return null; diff --git a/packages/ui/utils/inlineTransforms.test.ts b/packages/ui/utils/inlineTransforms.test.ts index 99c273ddc..5c8b7c753 100644 --- a/packages/ui/utils/inlineTransforms.test.ts +++ b/packages/ui/utils/inlineTransforms.test.ts @@ -1,5 +1,5 @@ import { describe, test, expect } from 'bun:test'; -import { transformPlainText } from './inlineTransforms'; +import { stripInlineMarkdown, stripTableCellDelimiters, transformPlainText } from './inlineTransforms'; describe('transformPlainText — emoji shortcodes', () => { test('replaces known shortcode with unicode emoji', () => { @@ -46,3 +46,54 @@ describe('transformPlainText — smart punctuation', () => { expect(transformPlainText("he said 'hi'")).toBe('he said ‘hi’'); }); }); + +describe('stripInlineMarkdown', () => { + test('unwraps code spans, which is what an external quote carries', () => { + expect(stripInlineMarkdown('`config.ts` — `ENABLED_TOOLS` branch')).toBe( + 'config.ts — ENABLED_TOOLS branch', + ); + expect(stripInlineMarkdown('``a `b` c``')).toBe('a `b` c'); + }); + + test('unwraps emphasis and strikethrough', () => { + expect(stripInlineMarkdown('**R2.** the ~~old~~ __new__ path')).toBe('R2. the old new path'); + expect(stripInlineMarkdown('***both***')).toBe('both'); + }); + + test('keeps link and image text, drops the target', () => { + expect(stripInlineMarkdown('see [the plan](./plan.md)')).toBe('see the plan'); + expect(stripInlineMarkdown('![a diagram](x.png)')).toBe('a diagram'); + expect(stripInlineMarkdown('[[Design]] and [[Design|alias]]')).toBe('Design and Design'); + }); + + test('does not unwrap emphasis that lives inside a code span', () => { + // Backticks are consumed first, so the underscores in a code span are + // literal content and must survive as themselves. + expect(stripInlineMarkdown('`a__b__c`')).toBe('a__b__c'); + }); + + test('leaves prose without inline syntax untouched', () => { + expect(stripInlineMarkdown('plain sentence, nothing to strip')).toBe( + 'plain sentence, nothing to strip', + ); + }); + + test('leaves table cell delimiters alone', () => { + // Separate tier: deleting the padding around a pipe mangles prose, so it + // is not part of the general strip. + expect(stripInlineMarkdown('| a | b |')).toBe('| a | b |'); + }); +}); + +describe('stripTableCellDelimiters', () => { + test('renders a row the way adjacent cells concatenate', () => { + // `| a | b |` renders as ab — no text between the cells, + // so the padding has to go with the pipe, not just the pipe. + expect(stripTableCellDelimiters('| a | b |')).toBe('ab'); + expect(stripTableCellDelimiters('| `x.ts` | **WP5** |')).toBe('x.tsWP5'); + }); + + test('includes the inline strip', () => { + expect(stripTableCellDelimiters('`a`')).toBe('a'); + }); +}); diff --git a/packages/ui/utils/inlineTransforms.ts b/packages/ui/utils/inlineTransforms.ts index 9c33b7036..0efe92386 100644 --- a/packages/ui/utils/inlineTransforms.ts +++ b/packages/ui/utils/inlineTransforms.ts @@ -39,3 +39,59 @@ function smartypants(s: string): string { export function transformPlainText(text: string): string { return smartypants(replaceEmoji(text)); } + +/** + * Strip the inline markdown syntax the renderer consumes, so a quote taken + * from a document's SOURCE can be matched against its RENDERED text. + * + * An external annotation's `originalText` is a quote, and its natural source + * is the markdown a tool read from the server — where `` `config.ts` `` still + * carries its backticks. The rendered DOM has none of that syntax, so a + * faithful quote of a line containing any inline markup finds nothing and the + * annotation degrades to sidebar-only. + * + * Deliberately conservative and lossy in one direction only: it removes + * delimiters, never content. Both helpers are last-resort restore tiers, tried + * only after a literal search has already failed, so a quote that matches the + * page verbatim never reaches them. + */ +export function stripInlineMarkdown(text: string): string { + // Code-span content is literal to the renderer, so emphasis delimiters + // inside one are ordinary characters and must survive. Unwrapping the + // backticks first would expose them to the rules below, so the content is + // parked out of reach and restored at the end. + const spans: string[] = []; + const SENTINEL = '\u0000'; + const parked = text.replace(/``([\s\S]+?)``|`([^`]+)`/g, (_match, double, single) => { + spans.push(double ?? single); + return `${SENTINEL}${spans.length - 1}${SENTINEL}`; + }); + + const stripped = parked + .replace(/!\[([^\]]*)\]\([^)]*\)/g, '$1') + .replace(/\[\[([^\]|]+)(?:\|[^\]]*)?\]\]/g, '$1') + .replace(/\[([^\]]+)\]\([^)]*\)/g, '$1') + .replace(/\*\*\*([^*]+)\*\*\*/g, '$1') + .replace(/\*\*([^*]+)\*\*/g, '$1') + .replace(/__([^_]+)__/g, '$1') + .replace(/~~([^~]+)~~/g, '$1'); + + return stripped.replace( + new RegExp(`${SENTINEL}(\\d+)${SENTINEL}`, 'g'), + (_match, index) => spans[Number(index)] ?? '', + ); +} + +/** + * `stripInlineMarkdown` plus the cell delimiters of a table row. + * + * A row renders as sibling `` elements with no text between them, so the + * rendered text of `| a | b |` is `ab` — the pipes AND the padding around them + * have to go, not just the pipes. Kept separate from `stripInlineMarkdown` + * because deleting the whitespace around a `|` mangles ordinary prose that + * happens to contain one; it is only ever worth trying when everything else + * has already failed. + */ +export function stripTableCellDelimiters(text: string): string { + return stripInlineMarkdown(text).replace(/\s*\|\s*/g, ''); +}