Skip to content

Commit 1187586

Browse files
backnotpropclaude
andcommitted
cleanup: memoize annotation filters, fix timer leaks, use blockId for highlight rings
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 parent 75cbc93 commit 1187586

3 files changed

Lines changed: 48 additions & 35 deletions

File tree

packages/editor/App.tsx

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -97,7 +97,9 @@ const App: React.FC = () => {
9797
const [showExportDropdown, setShowExportDropdown] = useState(false);
9898
const [initialExportTab, setInitialExportTab] = useState<'share' | 'annotations' | 'notes'>();
9999
const [noteSaveToast, setNoteSaveToast] = useState<{ type: 'success' | 'error'; message: string } | null>(null);
100-
// Plan diff state
100+
// Plan diff state — memoize filtered annotation lists to avoid new references per render
101+
const diffAnnotations = useMemo(() => annotations.filter(a => !!a.diffContext), [annotations]);
102+
const viewerAnnotations = useMemo(() => annotations.filter(a => !a.diffContext), [annotations]);
101103
const [isPlanDiffActive, setIsPlanDiffActive] = useState(false);
102104
const [planDiffMode, setPlanDiffMode] = useState<PlanDiffMode>('clean');
103105
const [previousPlan, setPreviousPlan] = useState<string | null>(null);
@@ -1266,7 +1268,7 @@ const App: React.FC = () => {
12661268
baseVersionLabel={planDiff.diffBaseVersion != null ? `v${planDiff.diffBaseVersion}` : undefined}
12671269
baseVersion={planDiff.diffBaseVersion ?? undefined}
12681270
maxWidth={planMaxWidth}
1269-
annotations={annotations.filter(a => !!a.diffContext)}
1271+
annotations={diffAnnotations}
12701272
onAddAnnotation={handleAddAnnotation}
12711273
onSelectAnnotation={handleSelectAnnotation}
12721274
selectedAnnotationId={selectedAnnotationId}
@@ -1279,7 +1281,7 @@ const App: React.FC = () => {
12791281
blocks={blocks}
12801282
markdown={markdown}
12811283
frontmatter={frontmatter}
1282-
annotations={annotations.filter(a => !a.diffContext)}
1284+
annotations={viewerAnnotations}
12831285
onAddAnnotation={handleAddAnnotation}
12841286
onSelectAnnotation={handleSelectAnnotation}
12851287
selectedAnnotationId={selectedAnnotationId}

packages/ui/components/plan-diff/PlanCleanDiffView.tsx

Lines changed: 42 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,7 @@ export const PlanCleanDiffView: React.FC<PlanCleanDiffViewProps> = ({
3838
const modeRef = useRef<EditorMode>(mode);
3939
const onAddAnnotationRef = useRef(onAddAnnotation);
4040
const hoverTimeoutRef = useRef<ReturnType<typeof setTimeout> | null>(null);
41+
const exitTimerRef = useRef<ReturnType<typeof setTimeout> | null>(null);
4142

4243
const [hoveredBlock, setHoveredBlock] = useState<{
4344
element: HTMLElement;
@@ -66,19 +67,23 @@ export const PlanCleanDiffView: React.FC<PlanCleanDiffViewProps> = ({
6667
useEffect(() => { modeRef.current = mode; }, [mode]);
6768
useEffect(() => { onAddAnnotationRef.current = onAddAnnotation; }, [onAddAnnotation]);
6869

69-
// Clean up hover timeout on unmount
70+
// Clean up timers on unmount
7071
useEffect(() => {
7172
return () => {
7273
if (hoverTimeoutRef.current) clearTimeout(hoverTimeoutRef.current);
74+
if (exitTimerRef.current) clearTimeout(exitTimerRef.current);
7375
};
7476
}, []);
7577

7678
// Scroll to selected annotation's diff block
79+
// Only depends on selectedAnnotationId — annotations ref is read but not a trigger
80+
const annotationsRef = useRef(annotations);
81+
annotationsRef.current = annotations;
82+
7783
useEffect(() => {
7884
if (!selectedAnnotationId) return;
7985

80-
// Find the annotation to get its blockId
81-
const ann = annotations.find(a => a.id === selectedAnnotationId);
86+
const ann = annotationsRef.current.find(a => a.id === selectedAnnotationId);
8287
if (!ann?.blockId?.startsWith('diff-block-')) return;
8388

8489
const idx = ann.blockId.replace('diff-block-', '');
@@ -92,19 +97,26 @@ export const PlanCleanDiffView: React.FC<PlanCleanDiffViewProps> = ({
9297
el.classList.remove('annotation-highlight', 'focused');
9398
}, 2000);
9499
return () => clearTimeout(timer);
95-
}, [selectedAnnotationId, annotations]);
100+
}, [selectedAnnotationId]);
96101

97-
// Build set of annotated block contents for highlight rings
98-
const annotatedContents = React.useMemo(() => {
102+
// Build set of annotated block IDs for highlight rings
103+
const annotatedBlockIds = React.useMemo(() => {
99104
const set = new Set<string>();
100105
annotations.forEach(ann => {
101-
if (ann.diffContext && ann.originalText) {
102-
set.add(ann.originalText);
106+
if (ann.diffContext && ann.blockId) {
107+
set.add(ann.blockId);
103108
}
104109
});
105110
return set;
106111
}, [annotations]);
107112

113+
/** Resolve content for a diff block section (handles modified blocks with old/new sides) */
114+
const getBlockContent = useCallback((block: PlanDiffBlock, diffContext: Annotation['diffContext']) =>
115+
block.type === 'modified' && diffContext === 'removed'
116+
? block.oldContent || block.content
117+
: block.content
118+
, []);
119+
108120
const createDiffAnnotation = useCallback((
109121
block: PlanDiffBlock,
110122
index: number,
@@ -115,19 +127,18 @@ export const PlanCleanDiffView: React.FC<PlanCleanDiffViewProps> = ({
115127
isQuickLabel?: boolean,
116128
quickLabelTip?: string,
117129
) => {
118-
const content = block.type === 'modified' && diffContext === 'removed'
119-
? block.oldContent || block.content
120-
: block.content;
130+
const content = getBlockContent(block, diffContext);
131+
const now = Date.now();
121132

122133
const newAnnotation: Annotation = {
123-
id: `diff-${Date.now()}-${index}`,
134+
id: `diff-${now}-${index}`,
124135
blockId: `diff-block-${index}`,
125136
startOffset: 0,
126137
endOffset: content.length,
127138
type,
128139
text,
129140
originalText: content.slice(0, 500),
130-
createdA: Date.now(),
141+
createdA: now,
131142
author: getIdentity(),
132143
images,
133144
diffContext,
@@ -136,7 +147,7 @@ export const PlanCleanDiffView: React.FC<PlanCleanDiffViewProps> = ({
136147
};
137148

138149
onAddAnnotationRef.current?.(newAnnotation);
139-
}, []);
150+
}, [getBlockContent]);
140151

141152
// Hover handlers
142153
const handleHover = useCallback((element: HTMLElement, block: PlanDiffBlock, index: number, diffContext: Annotation['diffContext']) => {
@@ -153,9 +164,10 @@ export const PlanCleanDiffView: React.FC<PlanCleanDiffViewProps> = ({
153164
const handleLeave = useCallback(() => {
154165
hoverTimeoutRef.current = setTimeout(() => {
155166
setIsExiting(true);
156-
setTimeout(() => {
167+
exitTimerRef.current = setTimeout(() => {
157168
setHoveredBlock(null);
158169
setIsExiting(false);
170+
exitTimerRef.current = null;
159171
}, 150);
160172
}, 100);
161173
}, []);
@@ -165,6 +177,7 @@ export const PlanCleanDiffView: React.FC<PlanCleanDiffViewProps> = ({
165177
if (!hoveredBlock) return;
166178
createDiffAnnotation(hoveredBlock.block, hoveredBlock.index, hoveredBlock.diffContext, type);
167179
setHoveredBlock(null);
180+
setIsExiting(false);
168181
};
169182

170183
const handleQuickLabel = (label: QuickLabel) => {
@@ -174,17 +187,17 @@ export const PlanCleanDiffView: React.FC<PlanCleanDiffViewProps> = ({
174187
AnnotationType.COMMENT, `${label.emoji} ${label.text}`, undefined, true, label.tip
175188
);
176189
setHoveredBlock(null);
190+
setIsExiting(false);
177191
};
178192

179193
const handleToolbarClose = () => {
180194
setHoveredBlock(null);
195+
setIsExiting(false);
181196
};
182197

183198
const handleRequestComment = (initialChar?: string) => {
184199
if (!hoveredBlock) return;
185-
const content = hoveredBlock.block.type === 'modified' && hoveredBlock.diffContext === 'removed'
186-
? hoveredBlock.block.oldContent || hoveredBlock.block.content
187-
: hoveredBlock.block.content;
200+
const content = getBlockContent(hoveredBlock.block, hoveredBlock.diffContext);
188201
setCommentPopover({
189202
anchorEl: hoveredBlock.element,
190203
contextText: content.slice(0, 80),
@@ -227,9 +240,7 @@ export const PlanCleanDiffView: React.FC<PlanCleanDiffViewProps> = ({
227240
if (modeRef.current === 'redline') {
228241
createDiffAnnotation(block, index, diffContext, AnnotationType.DELETION);
229242
} else if (modeRef.current === 'comment') {
230-
const content = block.type === 'modified' && diffContext === 'removed'
231-
? block.oldContent || block.content
232-
: block.content;
243+
const content = getBlockContent(block, diffContext);
233244
setCommentPopover({
234245
anchorEl: element,
235246
contextText: content.slice(0, 80),
@@ -240,10 +251,10 @@ export const PlanCleanDiffView: React.FC<PlanCleanDiffViewProps> = ({
240251
} else if (modeRef.current === 'quickLabel') {
241252
setQuickLabelPicker({ anchorEl: element, block, index, diffContext });
242253
}
243-
}, [createDiffAnnotation]);
254+
}, [createDiffAnnotation, getBlockContent]);
244255

245-
// Check if a block's content has been annotated (for highlight ring)
246-
const isBlockAnnotated = (content: string) => annotatedContents.has(content.slice(0, 500));
256+
// Check if a block index has been annotated (for highlight ring)
257+
const isBlockAnnotated = (index: number) => annotatedBlockIds.has(`diff-block-${index}`);
247258

248259
return (
249260
<div className="space-y-1">
@@ -313,7 +324,7 @@ interface DiffBlockRendererProps {
313324
index: number;
314325
hoveredIndex: number | null;
315326
hoveredDiffContext?: Annotation['diffContext'];
316-
isBlockAnnotated: (content: string) => boolean;
327+
isBlockAnnotated: (index: number) => boolean;
317328
onHover?: (element: HTMLElement, diffContext: Annotation['diffContext']) => void;
318329
onLeave?: () => void;
319330
onClick?: (element: HTMLElement, diffContext: Annotation['diffContext']) => void;
@@ -332,9 +343,9 @@ const DiffBlockRenderer: React.FC<DiffBlockRendererProps> = ({
332343
const isHovered = (diffContext: Annotation['diffContext']) =>
333344
hoveredIndex === index && hoveredDiffContext === diffContext;
334345

335-
const ringClass = (diffContext: Annotation['diffContext'], content: string) => {
346+
const ringClass = (diffContext: Annotation['diffContext']) => {
336347
if (isHovered(diffContext)) return 'ring-1 ring-primary/30 rounded';
337-
if (isBlockAnnotated(content)) return 'ring-2 ring-accent rounded outline-offset-2';
348+
if (isBlockAnnotated(index)) return 'ring-2 ring-accent rounded outline-offset-2';
338349
return '';
339350
};
340351

@@ -349,7 +360,7 @@ const DiffBlockRenderer: React.FC<DiffBlockRendererProps> = ({
349360
case "added":
350361
return (
351362
<div
352-
className={`plan-diff-added transition-shadow ${ringClass('added', block.content)}`}
363+
className={`plan-diff-added transition-shadow ${ringClass('added')}`}
353364
data-diff-block-index={index}
354365
{...hoverProps('added')}
355366
>
@@ -360,7 +371,7 @@ const DiffBlockRenderer: React.FC<DiffBlockRendererProps> = ({
360371
case "removed":
361372
return (
362373
<div
363-
className={`plan-diff-removed line-through decoration-destructive/30 opacity-70 transition-shadow ${ringClass('removed', block.content)}`}
374+
className={`plan-diff-removed line-through decoration-destructive/30 opacity-70 transition-shadow ${ringClass('removed')}`}
364375
data-diff-block-index={index}
365376
{...hoverProps('removed')}
366377
>
@@ -372,13 +383,13 @@ const DiffBlockRenderer: React.FC<DiffBlockRendererProps> = ({
372383
return (
373384
<div data-diff-block-index={index}>
374385
<div
375-
className={`plan-diff-removed line-through decoration-destructive/30 opacity-60 transition-shadow ${ringClass('removed', block.oldContent!)}`}
386+
className={`plan-diff-removed line-through decoration-destructive/30 opacity-60 transition-shadow ${ringClass('removed')}`}
376387
{...hoverProps('removed')}
377388
>
378389
<MarkdownChunk content={block.oldContent!} />
379390
</div>
380391
<div
381-
className={`plan-diff-added transition-shadow ${ringClass('modified', block.content)}`}
392+
className={`plan-diff-added transition-shadow ${ringClass('modified')}`}
382393
{...hoverProps('modified')}
383394
>
384395
<MarkdownChunk content={block.content} />

packages/ui/hooks/useAnnotationHighlighter.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -447,7 +447,7 @@ export function useAnnotationHighlighter({
447447
// Apply CSS classes to existing annotations
448448
useEffect(() => {
449449
const highlighter = highlighterRef.current;
450-
if (!highlighter || !annotations) return;
450+
if (!highlighter) return;
451451

452452
annotations.forEach(ann => {
453453
try {

0 commit comments

Comments
 (0)