-
Notifications
You must be signed in to change notification settings - Fork 0
⚡ Optimize DiffViewer parsing performance #134
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
ea5527a
a75127f
d4260b5
ed6a8f2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,7 +5,7 @@ | |
| * Supports unified and split view modes. | ||
| */ | ||
|
|
||
| import { useState } from 'react'; | ||
| import { useMemo, useState } from 'react'; | ||
| import { ChevronDownIcon } from '@/components/icons'; | ||
| import { Text } from '@/components/ui'; | ||
| import { organicBorderRadius } from '@/lib/organic-styles'; | ||
|
|
@@ -95,9 +95,12 @@ export function DiffViewer({ | |
| }: Readonly<DiffViewerProps>) { | ||
| const [collapsed, setCollapsed] = useState(false); | ||
|
|
||
| const lines = diff || (oldContent && newContent ? parseDiff(oldContent, newContent) : []); | ||
| const additions = lines.filter((l) => l.type === 'add').length; | ||
| const deletions = lines.filter((l) => l.type === 'remove').length; | ||
| const lines = useMemo( | ||
| () => diff || (oldContent && newContent ? parseDiff(oldContent, newContent) : []), | ||
| [diff, oldContent, newContent] | ||
| ); | ||
| const additions = useMemo(() => lines.filter((l) => l.type === 'add').length, [lines]); | ||
| const deletions = useMemo(() => lines.filter((l) => l.type === 'remove').length, [lines]); | ||
|
Comment on lines
+102
to
+103
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To optimize further, you can calculate |
||
|
|
||
| const getLineStyle = (type: DiffLine['type']) => { | ||
| switch (type) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,50 @@ | ||
| import { act, fireEvent, render, screen } from '@testing-library/react'; | ||
| import { DiffViewer } from '../DiffViewer'; | ||
|
|
||
| // Mock dependencies to isolate DiffViewer performance | ||
| vi.mock('@/components/icons', () => ({ | ||
| ChevronDownIcon: () => <span>ChevronDown</span>, | ||
| })); | ||
|
|
||
| vi.mock('@/components/ui', () => ({ | ||
| // biome-ignore lint/suspicious/noExplicitAny: mock | ||
| Text: ({ children, ...props }: any) => <span {...props}>{children}</span>, | ||
| })); | ||
|
|
||
| vi.mock('@/lib/organic-styles', () => ({ | ||
| organicBorderRadius: { card: {} }, | ||
| })); | ||
|
|
||
| describe('DiffViewer Performance', () => { | ||
| it('measures re-render time with large diffs', async () => { | ||
| // Generate large content (~5000 lines) to make parsing expensive | ||
| const linesCount = 5000; | ||
| const oldContent = Array.from({ length: linesCount }, (_, i) => `original line ${i}`).join( | ||
| '\n' | ||
| ); | ||
| const newContent = Array.from({ length: linesCount }, (_, i) => `modified line ${i}`).join( | ||
| '\n' | ||
| ); | ||
|
|
||
| // Initial Render | ||
| const startRender = performance.now(); | ||
| render(<DiffViewer oldContent={oldContent} newContent={newContent} filename="benchmark.ts" />); | ||
| const endRender = performance.now(); | ||
| console.log(`[Benchmark] Initial render time: ${(endRender - startRender).toFixed(2)}ms`); | ||
|
|
||
| const button = screen.getByRole('button'); | ||
|
|
||
| // Trigger re-render by collapsing | ||
| const startUpdate = performance.now(); | ||
| await act(async () => { | ||
| fireEvent.click(button); | ||
| }); | ||
| const endUpdate = performance.now(); | ||
|
|
||
| const updateTime = endUpdate - startUpdate; | ||
| console.log(`[Benchmark] Re-render time (collapse): ${updateTime.toFixed(2)}ms`); | ||
|
|
||
| // Sanity check | ||
| expect(updateTime).toBeGreaterThan(0); | ||
jbdevprimary marked this conversation as resolved.
Show resolved
Hide resolved
Comment on lines
44
to
48
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This performance test is a great addition! However, it currently relies on To make this test more robust, I recommend verifying the behavior of the optimization directly: ensure the expensive This would involve:
This change would make the test a true regression guard for the memoization logic. |
||
| }, 10000); | ||
| }); | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
🏁 Script executed:
Repository: agentic-dev-library/thumbcode
Length of output: 150
🏁 Script executed:
Repository: agentic-dev-library/thumbcode
Length of output: 98
🏁 Script executed:
Repository: agentic-dev-library/thumbcode
Length of output: 1085
🏁 Script executed:
Repository: agentic-dev-library/thumbcode
Length of output: 273
🏁 Script executed:
Repository: agentic-dev-library/thumbcode
Length of output: 185
🏁 Script executed:
Repository: agentic-dev-library/thumbcode
Length of output: 346
🏁 Script executed:
Repository: agentic-dev-library/thumbcode
Length of output: 346
🏁 Script executed:
Repository: agentic-dev-library/thumbcode
Length of output: 112
🏁 Script executed:
Repository: agentic-dev-library/thumbcode
Length of output: 837
🏁 Script executed:
Repository: agentic-dev-library/thumbcode
Length of output: 279
🏁 Script executed:
Repository: agentic-dev-library/thumbcode
Length of output: 873
🏁 Script executed:
Repository: agentic-dev-library/thumbcode
Length of output: 112
🏁 Script executed:
Repository: agentic-dev-library/thumbcode
Length of output: 185
🏁 Script executed:
Repository: agentic-dev-library/thumbcode
Length of output: 1383
🏁 Script executed:
Repository: agentic-dev-library/thumbcode
Length of output: 55
🏁 Script executed:
Repository: agentic-dev-library/thumbcode
Length of output: 1698
🏁 Script executed:
Repository: agentic-dev-library/thumbcode
Length of output: 55
🏁 Script executed:
Repository: agentic-dev-library/thumbcode
Length of output: 388
🏁 Script executed:
Repository: agentic-dev-library/thumbcode
Length of output: 2001
🏁 Script executed:
Repository: agentic-dev-library/thumbcode
Length of output: 678
🏁 Script executed:
Repository: agentic-dev-library/thumbcode
Length of output: 455
🏁 Script executed:
Repository: agentic-dev-library/thumbcode
Length of output: 1267
🏁 Script executed:
Repository: agentic-dev-library/thumbcode
Length of output: 813
🏁 Script executed:
Repository: agentic-dev-library/thumbcode
Length of output: 813
Use
getTask()helper to search bothtaskQueueandcompletedTaskswhen looking up task dependencies.The refactoring removed the local
taskMapoptimization but incompletely—lines 88 and 95 now only searchtaskQueue, whereas the previoustaskMapcombined bothtaskQueueandcompletedTasks. This introduces a correctness bug.Line 95 is critical: When a task is cancelled, its status is set to
'cancelled'(AgentCoordinator.ts:136) and then moved tocompletedTasks(line 143). However, line 95 only searchestaskQueue, so cancelled dependencies incompletedTasksare never found. The filter evaluatestask?.status === 'cancelled'tofalse(sincetaskisundefined), and the dependent task is incorrectly placed inwaitinginstead ofblocked, causing incorrect execution plan behavior.The solution is simple: both lines 88 and 95 should use the existing
getTask()helper (lines 180–183), which already correctly searches both arrays. This maintains consistency and correctness.Proposed fix
if (pendingDeps.length === 0) { - const task = this.state.taskQueue.find((t) => t.id === taskId); + const task = this.getTask(taskId); if (task?.assignee) { plan.ready.push(taskId); } } else { const failedDeps = pendingDeps.filter((depId) => { - const task = this.state.taskQueue.find((t) => t.id === depId); + const task = this.getTask(depId); return task?.status === 'cancelled'; });🤖 Prompt for AI Agents