Skip to content

Commit 8837a3c

Browse files
loganjclaude
andcommitted
fix(penpal): render pending comment highlights via rehype plugin instead of DOM mutation
The pending highlight (shown while composing a new comment) was applied by directly mutating React-managed DOM — wrapping text nodes in <mark> elements. When an SSE event raced with comment submission, React would re-render the MarkdownViewer and encounter unexpected nodes during reconciliation, crashing with "The object can not be found here." Move the pending highlight into the existing rehypeCommentHighlights plugin so React owns the full lifecycle. When pendingAnchor is set, it's added to the threadHighlights array with a `pending` flag that adds the `pending-highlight` CSS class alongside `comment-highlight`. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 parent e126595 commit 8837a3c

6 files changed

Lines changed: 393 additions & 54 deletions

File tree

apps/penpal/frontend/src/components/MarkdownViewer.test.tsx

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -76,4 +76,31 @@ describe('MarkdownViewer', () => {
7676
expect(mermaidContainer).toBeDefined();
7777
expect(mermaidContainer?.getAttribute('data-mermaid-source')).toContain('graph TD');
7878
});
79+
80+
it('renders comment highlights via rehype plugin', () => {
81+
const md = 'Hello world';
82+
const highlights = [
83+
{ threadId: 't1', selectedText: 'world', startLine: 1 },
84+
];
85+
const { container } = render(
86+
<MarkdownViewer content={md} rawMarkdown={md} highlights={highlights} />,
87+
);
88+
const mark = container.querySelector('mark.comment-highlight');
89+
expect(mark).not.toBeNull();
90+
expect(mark?.textContent).toBe('world');
91+
expect(mark?.getAttribute('data-thread-id')).toBe('t1');
92+
});
93+
94+
it('renders pending highlights with pending-highlight class', () => {
95+
const md = 'Hello world';
96+
const highlights = [
97+
{ threadId: 'pending', selectedText: 'world', startLine: 1, pending: true },
98+
];
99+
const { container } = render(
100+
<MarkdownViewer content={md} rawMarkdown={md} highlights={highlights} />,
101+
);
102+
const mark = container.querySelector('mark.pending-highlight');
103+
expect(mark).not.toBeNull();
104+
expect(mark?.classList.contains('comment-highlight')).toBe(true);
105+
});
79106
});

apps/penpal/frontend/src/components/SelectionToolbar.tsx

Lines changed: 1 addition & 49 deletions
Original file line numberDiff line numberDiff line change
@@ -128,54 +128,6 @@ function computeAnchor(
128128
return anchor;
129129
}
130130

131-
/**
132-
* Applies pending highlight marks to the current selection.
133-
*/
134-
export function applyPendingHighlight(sel: Selection, contentEl: HTMLElement) {
135-
removePendingHighlight();
136-
if (!sel.rangeCount) return;
137-
const range = sel.getRangeAt(0);
138-
if (!contentEl.contains(range.commonAncestorContainer)) return;
139-
140-
const treeWalker = document.createTreeWalker(contentEl, NodeFilter.SHOW_TEXT);
141-
const textNodes: Text[] = [];
142-
while (treeWalker.nextNode()) {
143-
if (range.intersectsNode(treeWalker.currentNode)) {
144-
textNodes.push(treeWalker.currentNode as Text);
145-
}
146-
}
147-
148-
for (let i = textNodes.length - 1; i >= 0; i--) {
149-
const node = textNodes[i];
150-
const start = node === range.startContainer ? range.startOffset : 0;
151-
const end = node === range.endContainer ? range.endOffset : node.nodeValue!.length;
152-
if (start >= end) continue;
153-
154-
const mark = document.createElement('mark');
155-
mark.className = 'pending-highlight';
156-
157-
if (start === 0 && end === node.nodeValue!.length) {
158-
node.parentNode!.insertBefore(mark, node);
159-
mark.appendChild(node);
160-
} else {
161-
const subRange = document.createRange();
162-
subRange.setStart(node, start);
163-
subRange.setEnd(node, end);
164-
subRange.surroundContents(mark);
165-
}
166-
}
167-
sel.removeAllRanges();
168-
}
169-
170-
export function removePendingHighlight() {
171-
document.querySelectorAll('.pending-highlight').forEach((mark) => {
172-
const parent = mark.parentNode!;
173-
while (mark.firstChild) parent.insertBefore(mark.firstChild, mark);
174-
parent.removeChild(mark);
175-
parent.normalize();
176-
});
177-
}
178-
179131
/**
180132
* Finds which mermaid container index corresponds to a given startLine
181133
* by counting ```mermaid fences in the raw markdown.
@@ -322,7 +274,7 @@ export default function SelectionToolbar({
322274
if (!selectedText) return;
323275

324276
const anchor = computeAnchor(sel, selectedText, rawMarkdown, contentRef.current);
325-
applyPendingHighlight(sel, contentRef.current);
277+
sel.removeAllRanges();
326278
setVisible(false);
327279
onComment(anchor, selectedText);
328280
};
Lines changed: 142 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,142 @@
1+
import { describe, it, expect } from 'vitest';
2+
import rehypeCommentHighlights from './rehypeCommentHighlights';
3+
import type { ThreadHighlight } from './rehypeCommentHighlights';
4+
import type { Root, Element, Text } from 'hast';
5+
6+
/** Build a minimal HAST tree: <p data-source-line="N">...text...</p> */
7+
function makeTree(line: number, text: string): Root {
8+
return {
9+
type: 'root',
10+
children: [
11+
{
12+
type: 'element',
13+
tagName: 'p',
14+
properties: {},
15+
children: [{ type: 'text', value: text } as Text],
16+
position: { start: { line, column: 1, offset: 0 }, end: { line, column: 1 + text.length, offset: text.length } },
17+
} as Element,
18+
],
19+
};
20+
}
21+
22+
/** Find all <mark> elements in a tree */
23+
function findMarks(node: Root | Element): Element[] {
24+
const marks: Element[] = [];
25+
for (const child of ('children' in node ? node.children : [])) {
26+
if (child.type === 'element') {
27+
if (child.tagName === 'mark') marks.push(child);
28+
marks.push(...findMarks(child));
29+
}
30+
}
31+
return marks;
32+
}
33+
34+
describe('rehypeCommentHighlights', () => {
35+
it('wraps matching text in a <mark> with comment-highlight class', () => {
36+
const tree = makeTree(1, 'Hello world');
37+
const transform = rehypeCommentHighlights({
38+
highlights: [{ threadId: 't1', selectedText: 'world', startLine: 1 }],
39+
});
40+
transform(tree);
41+
const marks = findMarks(tree);
42+
expect(marks).toHaveLength(1);
43+
expect((marks[0].properties as Record<string, unknown>).className).toEqual(['comment-highlight']);
44+
expect((marks[0].properties as Record<string, unknown>).dataThreadId).toBe('t1');
45+
expect((marks[0].children[0] as Text).value).toBe('world');
46+
});
47+
48+
it('does not wrap text when selectedText is not found', () => {
49+
const tree = makeTree(1, 'Hello world');
50+
const transform = rehypeCommentHighlights({
51+
highlights: [{ threadId: 't1', selectedText: 'missing', startLine: 1 }],
52+
});
53+
transform(tree);
54+
expect(findMarks(tree)).toHaveLength(0);
55+
});
56+
57+
it('does not wrap text when startLine does not match', () => {
58+
const tree = makeTree(1, 'Hello world');
59+
const transform = rehypeCommentHighlights({
60+
highlights: [{ threadId: 't1', selectedText: 'world', startLine: 99 }],
61+
});
62+
transform(tree);
63+
expect(findMarks(tree)).toHaveLength(0);
64+
});
65+
66+
it('adds pending-highlight class when pending is true', () => {
67+
const tree = makeTree(1, 'Hello world');
68+
const transform = rehypeCommentHighlights({
69+
highlights: [{ threadId: 'pending', selectedText: 'world', startLine: 1, pending: true }],
70+
});
71+
transform(tree);
72+
const marks = findMarks(tree);
73+
expect(marks).toHaveLength(1);
74+
expect((marks[0].properties as Record<string, unknown>).className).toEqual(['comment-highlight', 'pending-highlight']);
75+
});
76+
77+
it('does not add pending-highlight class when pending is false/undefined', () => {
78+
const tree = makeTree(1, 'Hello world');
79+
const transform = rehypeCommentHighlights({
80+
highlights: [{ threadId: 't1', selectedText: 'world', startLine: 1 }],
81+
});
82+
transform(tree);
83+
const marks = findMarks(tree);
84+
expect(marks).toHaveLength(1);
85+
expect((marks[0].properties as Record<string, unknown>).className).toEqual(['comment-highlight']);
86+
});
87+
88+
it('splits text node correctly around the match', () => {
89+
const tree = makeTree(1, 'The quick brown fox');
90+
const transform = rehypeCommentHighlights({
91+
highlights: [{ threadId: 't1', selectedText: 'quick', startLine: 1 }],
92+
});
93+
transform(tree);
94+
const p = tree.children[0] as Element;
95+
// Should be: "The " + <mark>quick</mark> + " brown fox"
96+
expect(p.children).toHaveLength(3);
97+
expect((p.children[0] as Text).value).toBe('The ');
98+
expect((p.children[1] as Element).tagName).toBe('mark');
99+
expect((p.children[2] as Text).value).toBe(' brown fox');
100+
});
101+
102+
it('returns tree unchanged when highlights array is empty', () => {
103+
const tree = makeTree(1, 'Hello world');
104+
const original = JSON.stringify(tree);
105+
const transform = rehypeCommentHighlights({ highlights: [] });
106+
transform(tree);
107+
expect(JSON.stringify(tree)).toBe(original);
108+
});
109+
110+
it('handles multiple highlights on different lines', () => {
111+
const tree: Root = {
112+
type: 'root',
113+
children: [
114+
{
115+
type: 'element',
116+
tagName: 'p',
117+
properties: {},
118+
children: [{ type: 'text', value: 'First line' } as Text],
119+
position: { start: { line: 1, column: 1, offset: 0 }, end: { line: 1, column: 11, offset: 10 } },
120+
} as Element,
121+
{
122+
type: 'element',
123+
tagName: 'p',
124+
properties: {},
125+
children: [{ type: 'text', value: 'Second line' } as Text],
126+
position: { start: { line: 3, column: 1, offset: 12 }, end: { line: 3, column: 12, offset: 23 } },
127+
} as Element,
128+
],
129+
};
130+
const transform = rehypeCommentHighlights({
131+
highlights: [
132+
{ threadId: 't1', selectedText: 'First', startLine: 1 },
133+
{ threadId: 't2', selectedText: 'Second', startLine: 3 },
134+
],
135+
});
136+
transform(tree);
137+
const marks = findMarks(tree);
138+
expect(marks).toHaveLength(2);
139+
expect((marks[0].properties as Record<string, unknown>).dataThreadId).toBe('t1');
140+
expect((marks[1].properties as Record<string, unknown>).dataThreadId).toBe('t2');
141+
});
142+
});

apps/penpal/frontend/src/components/rehypeCommentHighlights.ts

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ export interface ThreadHighlight {
55
threadId: string;
66
selectedText: string;
77
startLine: number;
8+
pending?: boolean;
89
}
910

1011
interface Options {
@@ -139,7 +140,7 @@ function applyHighlight(element: Element, highlight: ThreadHighlight) {
139140
type: 'element',
140141
tagName: 'mark',
141142
properties: {
142-
className: ['comment-highlight'],
143+
className: highlight.pending ? ['comment-highlight', 'pending-highlight'] : ['comment-highlight'],
143144
dataThreadId: highlight.threadId,
144145
},
145146
children: [

0 commit comments

Comments
 (0)