From 3a53b633569db20165e0f6c94ea5593a7b7033fb Mon Sep 17 00:00:00 2001 From: Liang Hu Date: Sat, 25 Jul 2026 18:06:05 -0400 Subject: [PATCH 01/10] feat: model structured quality findings --- src/components/DiffViewerDialog.tsx | 15 ++ src/components/PlanViewerDialog.tsx | 2 +- src/components/QualityFindingCard.tsx | 124 ++++++++++++ src/components/ReviewProvider.tsx | 127 +++++++++++- src/components/ReviewSidebar.test.ts | 70 +++++++ src/components/ReviewSidebar.tsx | 270 ++++++++++++++++++++++++-- src/components/ReviewSidebarPanel.tsx | 51 ++++- src/components/ScrollingDiffView.tsx | 51 ++++- src/components/TaskPanel.tsx | 2 +- src/lib/quality-findings.test.ts | 136 +++++++++++++ src/lib/quality-findings.ts | 122 ++++++++++++ 11 files changed, 933 insertions(+), 37 deletions(-) create mode 100644 src/components/QualityFindingCard.tsx create mode 100644 src/components/ReviewSidebar.test.ts create mode 100644 src/lib/quality-findings.test.ts create mode 100644 src/lib/quality-findings.ts diff --git a/src/components/DiffViewerDialog.tsx b/src/components/DiffViewerDialog.tsx index 676dd22e..cb0c6852 100644 --- a/src/components/DiffViewerDialog.tsx +++ b/src/components/DiffViewerDialog.tsx @@ -8,6 +8,7 @@ import { theme } from '../lib/theme'; import { sf } from '../lib/fontScale'; import { parseUnifiedDiff } from '../lib/unified-diff-parser'; import { evictStaleAnnotations } from '../lib/review-eviction'; +import { reconcileQualityFindings, type QualityFindingProvider } from '../lib/quality-findings'; import { windowChromeTopInset } from '../lib/platform'; import { ScrollingDiffView } from './ScrollingDiffView'; import { @@ -50,6 +51,8 @@ interface DiffViewerDialogProps { onCommitNavigate?: (selection: CommitSelection) => void; /** Git isolation mode — CommitNavBar is only shown for worktree-isolated tasks */ gitIsolation?: GitIsolationMode; + /** Optional structured-finding source. Providers capture their own repository context. */ + findingProvider?: QualityFindingProvider; } /** Compile review annotations into a prompt string for the agent. */ @@ -92,6 +95,7 @@ export function DiffViewerDialog(props: DiffViewerDialogProps) { @@ -110,6 +114,7 @@ export function DiffViewerDialog(props: DiffViewerDialogProps) { selectedCommit={props.selectedCommit} onCommitNavigate={props.onCommitNavigate} gitIsolation={props.gitIsolation} + findingProvider={props.findingProvider} /> @@ -123,6 +128,7 @@ function DiffViewerContent(props: DiffViewerDialogProps) { const headerPaddingTop = `${windowChromeTopInset + 12}px`; const [parsedFiles, setParsedFiles] = createSignal([]); + const [diffLoaded, setDiffLoaded] = createSignal(false); const [loading, setLoading] = createSignal(false); const [error, setError] = createSignal(''); const [searchQuery, setSearchQuery] = createSignal(''); @@ -155,6 +161,13 @@ function DiffViewerContent(props: DiffViewerDialogProps) { setActiveFilePath(props.scrollToFile); }); + createEffect(() => { + if (!diffLoaded()) return; + const current = review.findings(); + const reconciled = reconcileQualityFindings(current, parsedFiles()); + if (reconciled !== current) review.replaceFindings(() => reconciled); + }); + createEffect(() => { const scrollTarget = props.scrollToFile; // Access selectedCommit before the early return so the effect tracks it @@ -169,6 +182,7 @@ function DiffViewerContent(props: DiffViewerDialogProps) { const thisGen = ++fetchGeneration; setSearchQuery(''); + setDiffLoaded(false); setLoading(true); setError(''); setParsedFiles([]); @@ -209,6 +223,7 @@ function DiffViewerContent(props: DiffViewerDialogProps) { const newFiles = parseUnifiedDiff(rawDiff); setParsedFiles(newFiles); review.replaceAnnotations((prev) => evictStaleAnnotations(prev, newFiles)); + setDiffLoaded(true); }) .catch((err) => { if (thisGen !== fetchGeneration) return; diff --git a/src/components/PlanViewerDialog.tsx b/src/components/PlanViewerDialog.tsx index ba842fbd..af22096e 100644 --- a/src/components/PlanViewerDialog.tsx +++ b/src/components/PlanViewerDialog.tsx @@ -125,7 +125,7 @@ function PlanViewerContent(props: PlanViewerContentProps) { // Scroll to annotation when scrollTarget changes createEffect(() => { const target = review.scrollTarget(); - if (!target) return; + if (!target?.id) return; const y = cardOffsets()[target.id]; if (y !== undefined && scrollRef) { scrollRef.scrollTo({ top: Math.max(0, y - 100), behavior: 'smooth' }); diff --git a/src/components/QualityFindingCard.tsx b/src/components/QualityFindingCard.tsx new file mode 100644 index 00000000..7f3ce193 --- /dev/null +++ b/src/components/QualityFindingCard.tsx @@ -0,0 +1,124 @@ +import { theme } from '../lib/theme'; +import { sf } from '../lib/fontScale'; +import { + formatQualityFindingLocation, + type QualityFinding, + type QualityFindingSeverity, +} from '../lib/quality-findings'; +import { CloseIcon } from './icons'; + +interface QualityFindingCardProps { + finding: QualityFinding; + canSubmit: boolean; + onDismiss: () => void; + onSubmit: () => void; +} + +function severityColor(severity: QualityFindingSeverity): string { + if (severity === 'error') return theme.error; + if (severity === 'warning') return theme.warning; + return theme.accent; +} + +export function QualityFindingCard(props: QualityFindingCardProps) { + const color = () => severityColor(props.finding.severity); + + return ( +
+
+ Automated + + {props.finding.severity} + + {props.finding.category} + + +
+ +
+ {props.finding.source} + + {props.finding.ruleId} + + {formatQualityFindingLocation(props.finding)} +
+ +
+ {props.finding.explanation} +
+ +
+ +
+
+ ); +} diff --git a/src/components/ReviewProvider.tsx b/src/components/ReviewProvider.tsx index 5f88e128..7ba1aa3f 100644 --- a/src/components/ReviewProvider.tsx +++ b/src/components/ReviewProvider.tsx @@ -1,6 +1,13 @@ -import { createContext, createSignal, createEffect, useContext } from 'solid-js'; +import { createContext, createSignal, createEffect, createMemo, useContext } from 'solid-js'; import type { JSX } from 'solid-js'; import { sendPrompt } from '../store/tasks'; +import { + compileQualityFindingPrompt, + dismissQualityFinding, + selectSubmittableFindings, + type QualityFinding, + type QualityFindingProvider, +} from '../lib/quality-findings'; import type { ReviewAnnotation, DiffInteractionMode } from './review-types'; /** Generic selection info used to create annotations or questions. */ @@ -22,6 +29,13 @@ export interface ActiveQuestion { selectedText: string; } +export interface ReviewScrollTarget { + id?: string; + filePath: string; + startLine: number; + endLine?: number; +} + export interface ReviewContextValue { annotations: () => ReviewAnnotation[]; addAnnotation: (annotation: ReviewAnnotation) => void; @@ -32,8 +46,18 @@ export interface ReviewContextValue { sidebarOpen: () => boolean; setSidebarOpen: (open: boolean) => void; - scrollTarget: () => ReviewAnnotation | null; - setScrollTarget: (target: ReviewAnnotation | null) => void; + findings: () => QualityFinding[]; + openFindings: () => QualityFinding[]; + selectedFindingIds: () => ReadonlySet; + setFindingSelected: (id: string, selected: boolean) => void; + dismissFinding: (id: string) => void; + replaceFindings: (fn: (prev: QualityFinding[]) => QualityFinding[]) => void; + submitFindings: (ids?: string[]) => Promise; + findingsLoading: () => boolean; + findingsError: () => string; + + scrollTarget: () => ReviewScrollTarget | null; + setScrollTarget: (target: ReviewScrollTarget | null) => void; submitReview: () => Promise; canSubmit: () => boolean; @@ -53,6 +77,7 @@ export interface ReviewContextValue { interface ReviewProviderProps { taskId?: string; agentId?: string; + findingProvider?: QualityFindingProvider; compilePrompt: (annotations: ReviewAnnotation[]) => string; onSubmitted?: () => void; children: JSX.Element; @@ -62,17 +87,63 @@ const ReviewContext = createContext(); export function ReviewProvider(props: ReviewProviderProps) { const [annotations, setAnnotations] = createSignal([]); + const [findings, setFindings] = createSignal([]); + const [selectedFindingIds, setSelectedFindingIds] = createSignal>(new Set()); + const [findingsLoading, setFindingsLoading] = createSignal(false); + const [findingsError, setFindingsError] = createSignal(''); const [sidebarOpen, setSidebarOpen] = createSignal(false); - const [scrollTarget, setScrollTarget] = createSignal(null, { + const [scrollTarget, setScrollTarget] = createSignal(null, { equals: false, }); const [pendingSelection, setPendingSelection] = createSignal(null); const [activeQuestions, setActiveQuestions] = createSignal([]); const [submitError, setSubmitError] = createSignal(''); + const openFindings = createMemo(() => findings().filter((finding) => finding.state === 'open')); + let findingLoadGeneration = 0; - // Auto-open sidebar when annotations are added createEffect(() => { - if (annotations().length > 0) setSidebarOpen(true); + const provider = props.findingProvider; + const generation = ++findingLoadGeneration; + setFindingsError(''); + setSelectedFindingIds(new Set()); + if (!provider) { + setFindings([]); + setFindingsLoading(false); + return; + } + + setFindingsLoading(true); + void provider + .loadFindings() + .then((loaded) => { + if (generation === findingLoadGeneration) setFindings(loaded); + }) + .catch((err: unknown) => { + if (generation !== findingLoadGeneration) return; + setFindings([]); + setFindingsError(err instanceof Error ? err.message : 'Failed to load quality findings'); + setSidebarOpen(true); + }) + .finally(() => { + if (generation === findingLoadGeneration) setFindingsLoading(false); + }); + }); + + // Auto-open sidebar when human comments or provider findings are added. + createEffect(() => { + if (annotations().length > 0 || openFindings().length > 0) setSidebarOpen(true); + }); + + createEffect(() => { + const validIds = new Set( + openFindings() + .filter((finding) => finding.freshness === 'current') + .map((finding) => finding.id), + ); + setSelectedFindingIds((prev) => { + const next = new Set([...prev].filter((id) => validIds.has(id))); + return next.size === prev.size ? prev : next; + }); }); function addAnnotation(annotation: ReviewAnnotation) { @@ -91,6 +162,24 @@ export function ReviewProvider(props: ReviewProviderProps) { setAnnotations(fn); } + function setFindingSelected(id: string, selected: boolean) { + setSelectedFindingIds((prev) => { + const next = new Set(prev); + if (selected) next.add(id); + else next.delete(id); + return next; + }); + } + + function dismissFinding(id: string) { + setFindings((prev) => dismissQualityFinding(prev, id)); + setFindingSelected(id, false); + } + + function replaceFindings(fn: (prev: QualityFinding[]) => QualityFinding[]) { + setFindings(fn); + } + function handleSelection(selection: ContentSelection) { setPendingSelection(selection); } @@ -159,12 +248,38 @@ export function ReviewProvider(props: ReviewProviderProps) { } } + async function submitFindings(ids?: string[]): Promise { + const taskId = props.taskId; + const agentId = props.agentId; + if (!taskId || !agentId) return; + + const selected = selectSubmittableFindings(findings(), ids ?? selectedFindingIds()); + if (selected.length === 0) return; + + setSubmitError(''); + try { + await sendPrompt(taskId, agentId, compileQualityFindingPrompt(selected)); + setSelectedFindingIds(new Set()); + } catch (err: unknown) { + setSubmitError(err instanceof Error ? err.message : 'Failed to send quality findings'); + } + } + const value: ReviewContextValue = { annotations, addAnnotation, dismissAnnotation, updateAnnotation, replaceAnnotations, + findings, + openFindings, + selectedFindingIds, + setFindingSelected, + dismissFinding, + replaceFindings, + submitFindings, + findingsLoading, + findingsError, sidebarOpen, setSidebarOpen, scrollTarget, diff --git a/src/components/ReviewSidebar.test.ts b/src/components/ReviewSidebar.test.ts new file mode 100644 index 00000000..6336facb --- /dev/null +++ b/src/components/ReviewSidebar.test.ts @@ -0,0 +1,70 @@ +import { renderToString } from 'solid-js/web'; +import { describe, expect, it, vi } from 'vitest'; +import type { QualityFinding } from '../lib/quality-findings'; +import { ReviewSidebar } from './ReviewSidebar'; + +function finding(overrides: Partial = {}): QualityFinding { + return { + id: 'finding-1', + fingerprint: 'fixture:no-floating-promises:src/app.ts:10', + source: 'fixture', + ruleId: 'no-floating-promises', + category: 'reliability', + severity: 'warning', + location: { filePath: 'src/app.ts', startLine: 10, startColumn: 3 }, + explanation: 'Await this promise or explicitly handle its rejection.', + state: 'open', + freshness: 'current', + ...overrides, + }; +} + +function render(findings: QualityFinding[]) { + return renderToString(() => + ReviewSidebar({ + annotations: [ + { + id: 'human-1', + filePath: 'src/app.ts', + startLine: 12, + endLine: 12, + selectedText: 'return value;', + comment: 'Please clarify this return value.', + }, + ], + findings, + selectedFindingIds: new Set(['finding-1']), + canSubmit: true, + onDismiss: vi.fn(), + onUpdate: vi.fn(), + onScrollTo: vi.fn(), + onSubmit: vi.fn(), + onFindingSelected: vi.fn(), + onFindingDismiss: vi.fn(), + onFindingScrollTo: vi.fn(), + onFindingSubmit: vi.fn(), + }), + ); +} + +describe('ReviewSidebar', () => { + it('distinguishes automated findings from human comments with textual metadata', () => { + const html = render([finding()]); + const text = html.replace(//g, ''); + + expect(text).toContain('Automated findings (1)'); + expect(text).toContain('Automated · warning · reliability'); + expect(text).toContain('fixture/no-floating-promises'); + expect(text).toContain('Human comments (1)'); + expect(text).toContain('Please clarify this return value.'); + expect(text).toContain('Send selected findings (1)'); + expect(text).toContain('Send human comments (1)'); + }); + + it('marks stale findings textually and disables their remediation controls', () => { + const html = render([finding({ freshness: 'stale' })]); + + expect(html).toContain('Stale'); + expect(html).toContain('disabled'); + }); +}); diff --git a/src/components/ReviewSidebar.tsx b/src/components/ReviewSidebar.tsx index 5ec87704..bed6ae61 100644 --- a/src/components/ReviewSidebar.tsx +++ b/src/components/ReviewSidebar.tsx @@ -1,21 +1,183 @@ import { For, Show, createSignal } from 'solid-js'; import { theme } from '../lib/theme'; import { sf } from '../lib/fontScale'; +import { + formatQualityFindingLocation, + type QualityFinding, + type QualityFindingSeverity, +} from '../lib/quality-findings'; import type { ReviewAnnotation } from './review-types'; +import { CloseIcon } from './icons'; interface ReviewSidebarProps { annotations: ReviewAnnotation[]; + findings: QualityFinding[]; + selectedFindingIds: ReadonlySet; canSubmit: boolean; onDismiss: (id: string) => void; onUpdate: (id: string, comment: string) => void; onScrollTo: (annotation: ReviewAnnotation) => void; onSubmit: () => void; + onFindingSelected: (id: string, selected: boolean) => void; + onFindingDismiss: (id: string) => void; + onFindingScrollTo: (finding: QualityFinding) => void; + onFindingSubmit: (ids?: string[]) => void; } function truncate(text: string, max: number): string { return text.length > max ? text.slice(0, max) + '...' : text; } +function severityColor(severity: QualityFindingSeverity): string { + if (severity === 'error') return theme.error; + if (severity === 'warning') return theme.warning; + return theme.accent; +} + +function QualityFindingSidebarItem(props: { + finding: QualityFinding; + selected: boolean; + canSubmit: boolean; + onSelected: (selected: boolean) => void; + onDismiss: () => void; + onScrollTo: () => void; + onSubmit: () => void; +}) { + const color = () => severityColor(props.finding.severity); + const isStale = () => props.finding.freshness === 'stale'; + + return ( +
{ + if (!isStale()) props.onScrollTo(); + }} + style={{ + padding: '8px 10px', + 'margin-bottom': '6px', + 'border-left': `3px solid ${isStale() ? theme.fgSubtle : color()}`, + 'border-radius': '0 4px 4px 0', + background: 'color-mix(in srgb, var(--fg) 3%, transparent)', + cursor: isStale() ? 'default' : 'pointer', + opacity: isStale() ? '0.65' : '1', + }} + > +
+ event.stopPropagation()} + onChange={(event) => props.onSelected(event.currentTarget.checked)} + /> + + Automated · {props.finding.severity} · {props.finding.category} + + + + + Stale + + + +
+ +
+ {formatQualityFindingLocation(props.finding)} +
+ +
+ {props.finding.source}/{props.finding.ruleId} +
+ +
+ {truncate(props.finding.explanation, 180)} +
+ +
+ +
+
+ ); +} + function SidebarAnnotationItem(props: { annotation: ReviewAnnotation; onDismiss: () => void; @@ -178,7 +340,7 @@ export function ReviewSidebar(props: ReviewSidebarProps) { color: theme.fg, }} > - Review Comments ({props.annotations.length}) + Review ({props.annotations.length + props.findings.length}) {/* Scrollable list */} @@ -189,6 +351,45 @@ export function ReviewSidebar(props: ReviewSidebarProps) { padding: '8px', }} > + 0}> +
+ Automated findings ({props.findings.length}) +
+ + {(finding) => ( + props.onFindingSelected(finding.id, selected)} + onDismiss={() => props.onFindingDismiss(finding.id)} + onScrollTo={() => props.onFindingScrollTo(finding)} + onSubmit={() => props.onFindingSubmit([finding.id])} + /> + )} + + + 0}> +
0 ? '12px 0 6px' : '0 0 6px', + }} + > + Human comments ({props.annotations.length}) +
+ {(annotation) => ( - +
+ 0}> + + + 0}> + + +
); diff --git a/src/components/ReviewSidebarPanel.tsx b/src/components/ReviewSidebarPanel.tsx index 640f4a4f..23f83b0a 100644 --- a/src/components/ReviewSidebarPanel.tsx +++ b/src/components/ReviewSidebarPanel.tsx @@ -7,9 +7,12 @@ import { sf } from '../lib/fontScale'; /** Toggle button that shows annotation count and opens/closes the review sidebar. */ export function ReviewCommentsButton() { const review = useReview(); + const reviewCount = () => review.annotations().length + review.openFindings().length; + const hasReviewState = () => + reviewCount() > 0 || review.findingsLoading() || Boolean(review.findingsError()); return ( - 0}> + ); } -/** Sidebar column with error banner and annotation list. Renders nothing when closed or empty. */ +/** Sidebar column with human comments and provider findings. */ export function ReviewSidebarPanel() { const review = useReview(); + const reviewCount = () => review.annotations().length + review.openFindings().length; + const hasReviewState = () => + reviewCount() > 0 || review.findingsLoading() || Boolean(review.findingsError()); return ( - 0}> +
+ +
+ Loading quality findings... +
+ + +
+ Quality findings unavailable: {review.findingsError()} +
+ + review.setScrollTarget({ + id: finding.id, + filePath: finding.location.filePath, + startLine: finding.location.startLine, + endLine: finding.location.endLine, + }) + } + onFindingSubmit={(ids) => void review.submitFindings(ids)} />
diff --git a/src/components/ScrollingDiffView.tsx b/src/components/ScrollingDiffView.tsx index 00f9d6b1..d821bf27 100644 --- a/src/components/ScrollingDiffView.tsx +++ b/src/components/ScrollingDiffView.tsx @@ -14,8 +14,10 @@ import { getDiffSelection } from '../lib/diff-selection'; import { getContextGapLineCount, type ContextGapRange } from '../lib/diff-context-gaps'; import { AskCodeCard } from './AskCodeCard'; import { ReviewCommentCard } from './ReviewCommentCard'; +import { QualityFindingCard } from './QualityFindingCard'; import { InlineInput } from './InlineInput'; -import { useReview, type ActiveQuestion } from './ReviewProvider'; +import { useReview, type ActiveQuestion, type ReviewScrollTarget } from './ReviewProvider'; +import type { QualityFinding } from '../lib/quality-findings'; import type { ReviewAnnotation, DiffInteractionMode } from './review-types'; interface ScrollingDiffViewProps { @@ -25,7 +27,7 @@ interface ScrollingDiffViewProps { /** Base branch for diff comparison (e.g. 'main', 'develop'). Undefined = auto-detect. */ baseBranch?: string; searchQuery?: string; - scrollToAnnotation?: ReviewAnnotation | null; + scrollToAnnotation?: ReviewScrollTarget | null; onScrollRef?: (el: HTMLDivElement) => void; } @@ -456,6 +458,10 @@ function FileSection(props: { reviewAnnotations: ReviewAnnotation[]; onDismissAnnotation: (id: string) => void; onAnnotationUpdate: (id: string, comment: string) => void; + qualityFindings: QualityFinding[]; + canSubmitFindings: boolean; + onDismissFinding: (id: string) => void; + onSubmitFinding: (id: string) => void; highlightedRange?: HighlightRange | null; pendingInput?: { filePath: string; afterLine: number } | null; onSubmit: (text: string, mode: 'review' | 'ask') => void; @@ -710,6 +716,25 @@ function FileSection(props: { /> )} + finding.location.filePath, + (finding) => finding.location.endLine ?? finding.location.startLine, + hunk.newStart, + nextStart, + )} + > + {(finding) => ( + props.onDismissFinding(finding.id)} + onSubmit={() => props.onSubmitFinding(finding.id)} + /> + )} + ); })()} @@ -755,11 +780,19 @@ export function ScrollingDiffView(props: ScrollingDiffViewProps) { const highlightedRange = (): HighlightRange | null => { const selection = review.pendingSelection(); - return selection + if (selection) { + return { + filePath: selection.source, + startLine: selection.startLine, + endLine: selection.endLine, + }; + } + const target = props.scrollToAnnotation; + return target ? { - filePath: selection.source, - startLine: selection.startLine, - endLine: selection.endLine, + filePath: target.filePath, + startLine: target.startLine, + endLine: target.endLine ?? target.startLine, } : null; }; @@ -898,6 +931,12 @@ export function ScrollingDiffView(props: ScrollingDiffViewProps) { reviewAnnotations={review.annotations()} onDismissAnnotation={review.dismissAnnotation} onAnnotationUpdate={review.updateAnnotation} + qualityFindings={review + .openFindings() + .filter((finding) => finding.freshness === 'current')} + canSubmitFindings={review.canSubmit()} + onDismissFinding={review.dismissFinding} + onSubmitFinding={(id) => void review.submitFindings([id])} highlightedRange={highlightedRange()} pendingInput={(() => { const pi = review.pendingSelection(); diff --git a/src/components/TaskPanel.tsx b/src/components/TaskPanel.tsx index 7657fade..533e592c 100644 --- a/src/components/TaskPanel.tsx +++ b/src/components/TaskPanel.tsx @@ -651,7 +651,7 @@ export function TaskPanel(props: TaskPanelProps) { baseBranch={props.task.baseBranch} onClose={() => setDiffScrollTarget(null)} taskId={props.task.id} - agentId={props.task.agentIds[0]} + agentId={selectedAgentId()} commitList={commitList()} selectedCommit={selectedCommit()} onCommitNavigate={setSelectedCommit} diff --git a/src/lib/quality-findings.test.ts b/src/lib/quality-findings.test.ts new file mode 100644 index 00000000..05861189 --- /dev/null +++ b/src/lib/quality-findings.test.ts @@ -0,0 +1,136 @@ +import { describe, expect, it } from 'vitest'; +import type { FileDiff } from './unified-diff-parser'; +import { + compileQualityFindingPrompt, + createFixtureQualityFindingProvider, + dismissQualityFinding, + reconcileQualityFindings, + selectSubmittableFindings, + type QualityFinding, +} from './quality-findings'; + +function finding(overrides: Partial = {}): QualityFinding { + return { + id: 'finding-1', + fingerprint: 'fixture:no-floating-promises:src/app.ts:10', + source: 'fixture', + ruleId: 'no-floating-promises', + category: 'reliability', + severity: 'warning', + location: { filePath: 'src/app.ts', startLine: 10, startColumn: 3 }, + explanation: 'Await this promise or explicitly handle its rejection.', + state: 'open', + freshness: 'current', + ...overrides, + }; +} + +function diff(overrides: Partial = {}): FileDiff { + return { + path: 'src/app.ts', + status: 'M', + binary: false, + hunks: [ + { + oldStart: 9, + oldCount: 2, + newStart: 9, + newCount: 3, + lines: [ + { type: 'context', content: 'before', oldLine: 9, newLine: 9 }, + { type: 'add', content: 'runAsync();', oldLine: null, newLine: 10 }, + { type: 'context', content: 'after', oldLine: 10, newLine: 11 }, + ], + }, + ], + ...overrides, + }; +} + +describe('createFixtureQualityFindingProvider', () => { + it('supplies independent copies of fixture findings', async () => { + const original = finding(); + const provider = createFixtureQualityFindingProvider([original]); + + const first = await provider.loadFindings(); + first[0].location.startLine = 99; + const second = await provider.loadFindings(); + + expect(second).toEqual([original]); + }); +}); + +describe('reconcileQualityFindings', () => { + it('keeps a finding current when its location is represented in the diff', () => { + const input = [finding()]; + expect(reconcileQualityFindings(input, [diff()])).toBe(input); + }); + + it.each([ + ['file is absent', []], + ['file is deleted', [diff({ status: 'D', hunks: [] })]], + ['location no longer matches', [diff()]], + ])('marks a finding stale when the %s', (_name, files) => { + const input = + _name === 'location no longer matches' + ? [finding({ location: { filePath: 'src/app.ts', startLine: 50 } })] + : [finding()]; + expect(reconcileQualityFindings(input, files as FileDiff[])[0].freshness).toBe('stale'); + }); + + it('can mark a stale finding current again after a matching diff refresh', () => { + const stale = finding({ freshness: 'stale' }); + expect(reconcileQualityFindings([stale], [diff()])[0].freshness).toBe('current'); + }); +}); + +describe('compileQualityFindingPrompt', () => { + it('includes structured remediation fields for one or multiple findings', () => { + const prompt = compileQualityFindingPrompt([ + finding(), + finding({ + id: 'finding-2', + fingerprint: 'fixture:complexity:src/util.ts:4', + ruleId: 'complexity', + category: 'maintainability', + severity: 'note', + location: { + filePath: 'src/util.ts', + startLine: 4, + startColumn: 2, + endLine: 8, + endColumn: 7, + }, + }), + ]); + + expect(prompt).toContain('[warning] [reliability] fixture/no-floating-promises'); + expect(prompt).toContain('Location: src/app.ts:10:3'); + expect(prompt).toContain('Fingerprint: fixture:no-floating-promises:src/app.ts:10'); + expect(prompt).toContain('[note] [maintainability] fixture/complexity'); + expect(prompt).toContain('Location: src/util.ts:4:2-8:7'); + }); +}); + +describe('finding review actions', () => { + it('dismisses by state without dropping provider identity', () => { + const original = finding(); + const dismissed = dismissQualityFinding([original], original.id); + + expect(dismissed[0]).toMatchObject({ + id: original.id, + fingerprint: original.fingerprint, + state: 'dismissed', + }); + }); + + it('submits only selected open findings with current locations', () => { + const current = finding(); + const stale = finding({ id: 'stale', fingerprint: 'stale', freshness: 'stale' }); + const resolved = finding({ id: 'resolved', fingerprint: 'resolved', state: 'resolved' }); + + expect( + selectSubmittableFindings([current, stale, resolved], ['finding-1', 'stale', 'resolved']), + ).toEqual([current]); + }); +}); diff --git a/src/lib/quality-findings.ts b/src/lib/quality-findings.ts new file mode 100644 index 00000000..3f8d112a --- /dev/null +++ b/src/lib/quality-findings.ts @@ -0,0 +1,122 @@ +import type { FileDiff } from './unified-diff-parser'; + +export type QualityFindingCategory = 'reliability' | 'maintainability'; +export type QualityFindingSeverity = 'error' | 'warning' | 'note'; +export type QualityFindingState = 'open' | 'dismissed' | 'resolved'; +export type QualityFindingFreshness = 'current' | 'stale'; + +export interface QualityFindingLocation { + filePath: string; + startLine: number; + startColumn?: number; + endLine?: number; + endColumn?: number; +} + +export interface QualityFinding { + id: string; + fingerprint: string; + source: string; + ruleId: string; + category: QualityFindingCategory; + severity: QualityFindingSeverity; + location: QualityFindingLocation; + explanation: string; + state: QualityFindingState; + freshness: QualityFindingFreshness; +} + +export interface QualityFindingProvider { + loadFindings(): Promise; +} + +function cloneFinding(finding: QualityFinding): QualityFinding { + return { + ...finding, + location: { ...finding.location }, + }; +} + +/** In-memory provider for component tests and provider integration fixtures. */ +export function createFixtureQualityFindingProvider( + findings: QualityFinding[], +): QualityFindingProvider { + return { + async loadFindings() { + return findings.map(cloneFinding); + }, + }; +} + +function findingMatchesDiff(finding: QualityFinding, files: FileDiff[]): boolean { + const file = files.find((candidate) => candidate.path === finding.location.filePath); + if (!file || file.status === 'D' || file.binary) return false; + + const startLine = finding.location.startLine; + const endLine = finding.location.endLine ?? startLine; + return file.hunks.some((hunk) => + hunk.lines.some( + (line) => line.newLine !== null && line.newLine >= startLine && line.newLine <= endLine, + ), + ); +} + +/** Mark provider locations stale when they no longer map to the current rendered diff. */ +export function reconcileQualityFindings( + findings: QualityFinding[], + files: FileDiff[], +): QualityFinding[] { + let changed = false; + const reconciled = findings.map((finding) => { + const freshness: QualityFindingFreshness = findingMatchesDiff(finding, files) + ? 'current' + : 'stale'; + if (finding.freshness === freshness) return finding; + changed = true; + return { ...finding, freshness }; + }); + return changed ? reconciled : findings; +} + +export function dismissQualityFinding(findings: QualityFinding[], id: string): QualityFinding[] { + return findings.map((finding) => + finding.id === id ? { ...finding, state: 'dismissed' } : finding, + ); +} + +export function selectSubmittableFindings( + findings: QualityFinding[], + ids: Iterable, +): QualityFinding[] { + const requested = new Set(ids); + return findings.filter( + (finding) => + requested.has(finding.id) && finding.state === 'open' && finding.freshness === 'current', + ); +} + +export function formatQualityFindingLocation(finding: QualityFinding): string { + const location = finding.location; + const startColumn = location.startColumn ? `:${location.startColumn}` : ''; + let end = ''; + if (location.endLine && location.endLine !== location.startLine) { + end = `-${location.endLine}${location.endColumn ? `:${location.endColumn}` : ''}`; + } else if (location.endColumn && location.endColumn !== location.startColumn) { + end = `-${location.endColumn}`; + } + return `${location.filePath}:${location.startLine}${startColumn}${end}`; +} + +export function compileQualityFindingPrompt(findings: QualityFinding[]): string { + const lines = ['Structured code-quality findings to remediate:\n']; + for (const finding of findings) { + lines.push( + `## [${finding.severity}] [${finding.category}] ${finding.source}/${finding.ruleId}`, + ); + lines.push(`Location: ${formatQualityFindingLocation(finding)}`); + lines.push(`Fingerprint: ${finding.fingerprint}`); + lines.push(finding.explanation); + lines.push(''); + } + return lines.join('\n'); +} From b8b514c0b37474e29620af0ce092ac7a375c3bdf Mon Sep 17 00:00:00 2001 From: Liang Hu Date: Mon, 27 Jul 2026 07:58:20 -0400 Subject: [PATCH 02/10] fix: gate quality findings on loaded diffs --- src/components/DiffViewerDialog.tsx | 8 ++-- src/components/ReviewProvider.tsx | 5 +- src/components/ReviewSidebar.test.ts | 7 +++ src/components/ReviewSidebar.tsx | 51 +++++++++++--------- src/lib/quality-findings.test.ts | 71 ++++++++++++++++++++++++++++ src/lib/quality-findings.ts | 35 +++++++++++--- 6 files changed, 145 insertions(+), 32 deletions(-) diff --git a/src/components/DiffViewerDialog.tsx b/src/components/DiffViewerDialog.tsx index cb0c6852..427af609 100644 --- a/src/components/DiffViewerDialog.tsx +++ b/src/components/DiffViewerDialog.tsx @@ -8,7 +8,10 @@ import { theme } from '../lib/theme'; import { sf } from '../lib/fontScale'; import { parseUnifiedDiff } from '../lib/unified-diff-parser'; import { evictStaleAnnotations } from '../lib/review-eviction'; -import { reconcileQualityFindings, type QualityFindingProvider } from '../lib/quality-findings'; +import { + reconcileQualityFindingsForDiff, + type QualityFindingProvider, +} from '../lib/quality-findings'; import { windowChromeTopInset } from '../lib/platform'; import { ScrollingDiffView } from './ScrollingDiffView'; import { @@ -162,9 +165,8 @@ function DiffViewerContent(props: DiffViewerDialogProps) { }); createEffect(() => { - if (!diffLoaded()) return; const current = review.findings(); - const reconciled = reconcileQualityFindings(current, parsedFiles()); + const reconciled = reconcileQualityFindingsForDiff(current, parsedFiles(), diffLoaded()); if (reconciled !== current) review.replaceFindings(() => reconciled); }); diff --git a/src/components/ReviewProvider.tsx b/src/components/ReviewProvider.tsx index 7ba1aa3f..249230ad 100644 --- a/src/components/ReviewProvider.tsx +++ b/src/components/ReviewProvider.tsx @@ -4,6 +4,7 @@ import { sendPrompt } from '../store/tasks'; import { compileQualityFindingPrompt, dismissQualityFinding, + selectedFindingIdsAfterSubmission, selectSubmittableFindings, type QualityFinding, type QualityFindingProvider, @@ -259,7 +260,9 @@ export function ReviewProvider(props: ReviewProviderProps) { setSubmitError(''); try { await sendPrompt(taskId, agentId, compileQualityFindingPrompt(selected)); - setSelectedFindingIds(new Set()); + setSelectedFindingIds((previous) => + selectedFindingIdsAfterSubmission(previous, selected, ids === undefined), + ); } catch (err: unknown) { setSubmitError(err instanceof Error ? err.message : 'Failed to send quality findings'); } diff --git a/src/components/ReviewSidebar.test.ts b/src/components/ReviewSidebar.test.ts index 6336facb..5a33f150 100644 --- a/src/components/ReviewSidebar.test.ts +++ b/src/components/ReviewSidebar.test.ts @@ -67,4 +67,11 @@ describe('ReviewSidebar', () => { expect(html).toContain('Stale'); expect(html).toContain('disabled'); }); + + it('marks pending findings textually and disables their remediation controls', () => { + const html = render([finding({ freshness: 'pending' })]); + + expect(html).toContain('Pending'); + expect(html).toContain('disabled'); + }); }); diff --git a/src/components/ReviewSidebar.tsx b/src/components/ReviewSidebar.tsx index bed6ae61..ff0dac4b 100644 --- a/src/components/ReviewSidebar.tsx +++ b/src/components/ReviewSidebar.tsx @@ -44,35 +44,40 @@ function QualityFindingSidebarItem(props: { onSubmit: () => void; }) { const color = () => severityColor(props.finding.severity); - const isStale = () => props.finding.freshness === 'stale'; + const isActionable = () => props.finding.freshness === 'current'; + const freshnessLabel = () => { + if (props.finding.freshness === 'pending') return 'Pending'; + if (props.finding.freshness === 'stale') return 'Stale'; + return null; + }; return (
{ - if (!isStale()) props.onScrollTo(); + if (isActionable()) props.onScrollTo(); }} style={{ padding: '8px 10px', 'margin-bottom': '6px', - 'border-left': `3px solid ${isStale() ? theme.fgSubtle : color()}`, + 'border-left': `3px solid ${isActionable() ? color() : theme.fgSubtle}`, 'border-radius': '0 4px 4px 0', background: 'color-mix(in srgb, var(--fg) 3%, transparent)', - cursor: isStale() ? 'default' : 'pointer', - opacity: isStale() ? '0.65' : '1', + cursor: isActionable() ? 'pointer' : 'default', + opacity: isActionable() ? '1' : '0.65', }} >
event.stopPropagation()} onChange={(event) => props.onSelected(event.currentTarget.checked)} /> - - - Stale - + + {(label) => ( + + {label()} + + )} -
); } diff --git a/src/components/QualityFindingSidebarItem.tsx b/src/components/QualityFindingSidebarItem.tsx new file mode 100644 index 00000000..4b465348 --- /dev/null +++ b/src/components/QualityFindingSidebarItem.tsx @@ -0,0 +1,136 @@ +import { Show } from 'solid-js'; +import { theme } from '../lib/theme'; +import { sf } from '../lib/fontScale'; +import { formatQualityFindingLocation, type QualityFinding } from '../lib/quality-findings'; +import { CloseIcon } from './icons'; +import { qualityFindingSeverityColor } from './quality-finding-colors'; + +interface QualityFindingSidebarItemProps { + finding: QualityFinding; + selected: boolean; + onSelected: (selected: boolean) => void; + onDismiss: () => void; + onScrollTo: () => void; +} + +export function QualityFindingSidebarItem(props: QualityFindingSidebarItemProps) { + const color = () => qualityFindingSeverityColor(props.finding.severity); + const isActionable = () => props.finding.freshness === 'current'; + const freshnessLabel = () => { + if (props.finding.freshness === 'pending') return 'Pending'; + if (props.finding.freshness === 'stale') return 'Stale'; + return null; + }; + + return ( +
{ + if (isActionable()) props.onScrollTo(); + }} + style={{ + padding: '8px 10px', + 'margin-bottom': '6px', + 'border-left': `3px solid ${isActionable() ? color() : theme.fgSubtle}`, + 'border-radius': '0 4px 4px 0', + background: 'color-mix(in srgb, var(--fg) 3%, transparent)', + cursor: isActionable() ? 'pointer' : 'default', + opacity: isActionable() ? '1' : '0.65', + }} + > +
+ event.stopPropagation()} + onChange={(event) => props.onSelected(event.currentTarget.checked)} + /> + + Automated · {props.finding.severity} · {props.finding.category} + + + + {(label) => ( + + {label()} + + )} + + +
+ +
+ {formatQualityFindingLocation(props.finding)} +
+ +
+ {props.finding.source}/{props.finding.ruleId} +
+ +
+ {props.finding.explanation.length > 180 + ? `${props.finding.explanation.slice(0, 180)}...` + : props.finding.explanation} +
+
+ ); +} diff --git a/src/components/ReviewProvider.test.ts b/src/components/ReviewProvider.test.ts index e55300eb..f091f198 100644 --- a/src/components/ReviewProvider.test.ts +++ b/src/components/ReviewProvider.test.ts @@ -1,40 +1,120 @@ -import { describe, expect, it, vi } from 'vitest'; -import { canSubmitReview, createReviewSubmissionGuard } from './ReviewProvider'; +import { createComponent } from 'solid-js'; +import { renderToString } from 'solid-js/web'; +import { beforeEach, describe, expect, it, vi } from 'vitest'; +import { sendPrompt } from '../store/tasks'; +import type { QualityFinding } from '../lib/quality-findings'; +import { ReviewProvider, useReview, type ReviewContextValue } from './ReviewProvider'; -describe('createReviewSubmissionGuard', () => { - it('blocks concurrent review and finding submissions until the active send settles', async () => { +vi.mock('../store/tasks', () => ({ + sendPrompt: vi.fn(), +})); + +function finding(): QualityFinding { + return { + id: 'finding-1', + source: 'fixture', + ruleId: 'no-floating-promises', + category: 'reliability', + severity: 'warning', + location: { filePath: 'src/app.ts', startLine: 10 }, + explanation: 'Await this promise.', + state: 'open', + freshness: 'current', + }; +} + +function renderReviewProvider(onSubmitted = vi.fn()): ReviewContextValue { + let captured: ReviewContextValue | undefined; + + function CaptureContext() { + captured = useReview(); + return ''; + } + + renderToString(() => + createComponent(ReviewProvider, { + taskId: 'task-1', + agentId: 'agent-1', + compilePrompt: (annotations) => `Human comments: ${annotations.length}`, + onSubmitted, + get children() { + return createComponent(CaptureContext, {}); + }, + }), + ); + + if (!captured) throw new Error('Review context was not rendered'); + return captured; +} + +describe('ReviewProvider submission', () => { + beforeEach(() => { + vi.mocked(sendPrompt).mockReset(); + }); + + it('sends one combined prompt, blocks a rapid duplicate, and resolves submitted findings', async () => { let release: (() => void) | undefined; - const activeSend = vi.fn( + vi.mocked(sendPrompt).mockImplementation( () => new Promise((resolve) => { release = resolve; }), ); - const competingSend = vi.fn(async () => {}); - const guard = createReviewSubmissionGuard(); + const onSubmitted = vi.fn(); + const review = renderReviewProvider(onSubmitted); + review.addAnnotation({ + id: 'annotation-1', + filePath: 'src/app.ts', + startLine: 11, + endLine: 11, + selectedText: 'runAsync();', + comment: 'Handle this promise.', + }); + review.replaceFindings(() => [finding()]); + review.setFindingSelected('finding-1', true); - const first = guard.run(activeSend); - expect(guard.submitting()).toBe(true); - expect(canSubmitReview('task-1', 'agent-1', guard.submitting())).toBe(false); + const first = review.submitReview(); + const duplicate = review.submitReview(); - await expect(guard.run(competingSend)).resolves.toBe(false); - expect(competingSend).not.toHaveBeenCalled(); + expect(review.submitting()).toBe(true); + await duplicate; + expect(sendPrompt).toHaveBeenCalledTimes(1); + expect(sendPrompt).toHaveBeenCalledWith( + 'task-1', + 'agent-1', + expect.stringContaining('Human comments: 1'), + ); + expect(vi.mocked(sendPrompt).mock.calls[0][2]).toContain( + '[warning] [reliability] fixture/no-floating-promises', + ); release?.(); - await expect(first).resolves.toBe(true); - expect(guard.submitting()).toBe(false); - expect(canSubmitReview('task-1', 'agent-1', guard.submitting())).toBe(true); + await first; + + expect(review.submitting()).toBe(false); + expect(review.annotations()).toEqual([]); + expect(review.findings()[0].state).toBe('resolved'); + expect(review.selectedFindingIds().size).toBe(0); + expect(onSubmitted).toHaveBeenCalledOnce(); }); - it('releases the guard after a failed send', async () => { - const guard = createReviewSubmissionGuard(); + it('keeps submission errors visible and releases the in-flight guard', async () => { + vi.mocked(sendPrompt).mockRejectedValue(new Error('terminal unavailable')); + const review = renderReviewProvider(); + review.addAnnotation({ + id: 'annotation-1', + filePath: 'src/app.ts', + startLine: 11, + endLine: 11, + selectedText: 'runAsync();', + comment: 'Handle this promise.', + }); - await expect( - guard.run(async () => { - throw new Error('terminal unavailable'); - }), - ).rejects.toThrow('terminal unavailable'); + await review.submitReview(); - expect(guard.submitting()).toBe(false); + expect(review.submitting()).toBe(false); + expect(review.submitError()).toBe('terminal unavailable'); + expect(review.sidebarOpen()).toBe(true); + expect(review.annotations()).toHaveLength(1); }); }); diff --git a/src/components/ReviewProvider.tsx b/src/components/ReviewProvider.tsx index de9a51a3..bd2a45c0 100644 --- a/src/components/ReviewProvider.tsx +++ b/src/components/ReviewProvider.tsx @@ -4,6 +4,7 @@ import { sendPrompt } from '../store/tasks'; import { compileQualityFindingPrompt, dismissQualityFinding, + resolveQualityFindings, selectedFindingIdsAfterSubmission, selectSubmittableFindings, type QualityFinding, @@ -53,15 +54,16 @@ export interface ReviewContextValue { setFindingSelected: (id: string, selected: boolean) => void; dismissFinding: (id: string) => void; replaceFindings: (fn: (prev: QualityFinding[]) => QualityFinding[]) => void; - submitFindings: (ids?: string[]) => Promise; findingsLoading: () => boolean; findingsError: () => string; + clearFindingsError: () => void; scrollTarget: () => ReviewScrollTarget | null; setScrollTarget: (target: ReviewScrollTarget | null) => void; submitReview: () => Promise; canSubmit: () => boolean; + submitting: () => boolean; pendingSelection: () => ContentSelection | null; handleSelection: (selection: ContentSelection) => void; @@ -86,7 +88,7 @@ interface ReviewProviderProps { const ReviewContext = createContext(); -export function createReviewSubmissionGuard() { +function createReviewSubmissionGuard() { const [submitting, setSubmitting] = createSignal(false); async function run(action: () => Promise): Promise { @@ -103,7 +105,7 @@ export function createReviewSubmissionGuard() { return { submitting, run }; } -export function canSubmitReview( +function canSubmitReview( taskId: string | undefined, agentId: string | undefined, submitting: boolean, @@ -118,9 +120,7 @@ export function ReviewProvider(props: ReviewProviderProps) { const [findingsLoading, setFindingsLoading] = createSignal(false); const [findingsError, setFindingsError] = createSignal(''); const [sidebarOpen, setSidebarOpen] = createSignal(false); - const [scrollTarget, setScrollTarget] = createSignal(null, { - equals: false, - }); + const [scrollTarget, setScrollTarget] = createSignal(null); const [pendingSelection, setPendingSelection] = createSignal(null); const [activeQuestions, setActiveQuestions] = createSignal([]); const [submitError, setSubmitError] = createSignal(''); @@ -262,14 +262,30 @@ export function ReviewProvider(props: ReviewProviderProps) { const taskId = props.taskId; const agentId = props.agentId; if (!taskId || !agentId) return; - const prompt = props.compilePrompt(annotations()); + const submittedAnnotations = annotations(); + const submittedFindings = selectSubmittableFindings(findings(), selectedFindingIds()); + if (submittedAnnotations.length === 0 && submittedFindings.length === 0) return; + + const prompt = [ + submittedAnnotations.length > 0 ? props.compilePrompt(submittedAnnotations) : '', + submittedFindings.length > 0 ? compileQualityFindingPrompt(submittedFindings) : '', + ] + .filter(Boolean) + .join('\n'); + const submittedAnnotationIds = new Set(submittedAnnotations.map((annotation) => annotation.id)); const onSubmitted = props.onSubmitted; await submission.run(async () => { setSubmitError(''); try { await sendPrompt(taskId, agentId, prompt); - setAnnotations([]); + setAnnotations((previous) => + previous.filter((annotation) => !submittedAnnotationIds.has(annotation.id)), + ); + setFindings((previous) => resolveQualityFindings(previous, submittedFindings)); + setSelectedFindingIds((previous) => + selectedFindingIdsAfterSubmission(previous, submittedFindings), + ); setSidebarOpen(false); onSubmitted?.(); } catch (err: unknown) { @@ -279,26 +295,6 @@ export function ReviewProvider(props: ReviewProviderProps) { }); } - async function submitFindings(ids?: string[]): Promise { - const taskId = props.taskId; - const agentId = props.agentId; - if (!taskId || !agentId) return; - - const selected = selectSubmittableFindings(findings(), ids ?? selectedFindingIds()); - if (selected.length === 0) return; - - await submission.run(async () => { - setSubmitError(''); - try { - await sendPrompt(taskId, agentId, compileQualityFindingPrompt(selected)); - setSelectedFindingIds((previous) => selectedFindingIdsAfterSubmission(previous, selected)); - } catch (err: unknown) { - setSubmitError(err instanceof Error ? err.message : 'Failed to send quality findings'); - setSidebarOpen(true); - } - }); - } - const value: ReviewContextValue = { annotations, addAnnotation, @@ -311,9 +307,9 @@ export function ReviewProvider(props: ReviewProviderProps) { setFindingSelected, dismissFinding, replaceFindings, - submitFindings, findingsLoading, findingsError, + clearFindingsError: () => setFindingsError(''), sidebarOpen, setSidebarOpen, scrollTarget, @@ -325,6 +321,7 @@ export function ReviewProvider(props: ReviewProviderProps) { activeQuestions, dismissQuestion, canSubmit, + submitting: submission.submitting, submitReview, submitError, }; diff --git a/src/components/ReviewSidebar.test.ts b/src/components/ReviewSidebar.test.ts index 5a33f150..d93f3eac 100644 --- a/src/components/ReviewSidebar.test.ts +++ b/src/components/ReviewSidebar.test.ts @@ -6,7 +6,6 @@ import { ReviewSidebar } from './ReviewSidebar'; function finding(overrides: Partial = {}): QualityFinding { return { id: 'finding-1', - fingerprint: 'fixture:no-floating-promises:src/app.ts:10', source: 'fixture', ruleId: 'no-floating-promises', category: 'reliability', @@ -19,7 +18,13 @@ function finding(overrides: Partial = {}): QualityFinding { }; } -function render(findings: QualityFinding[]) { +function render( + findings: QualityFinding[], + submission: { canSubmit: boolean; submitting: boolean } = { + canSubmit: true, + submitting: false, + }, +) { return renderToString(() => ReviewSidebar({ annotations: [ @@ -34,7 +39,8 @@ function render(findings: QualityFinding[]) { ], findings, selectedFindingIds: new Set(['finding-1']), - canSubmit: true, + canSubmit: submission.canSubmit, + submitting: submission.submitting, onDismiss: vi.fn(), onUpdate: vi.fn(), onScrollTo: vi.fn(), @@ -42,7 +48,6 @@ function render(findings: QualityFinding[]) { onFindingSelected: vi.fn(), onFindingDismiss: vi.fn(), onFindingScrollTo: vi.fn(), - onFindingSubmit: vi.fn(), }), ); } @@ -57,21 +62,32 @@ describe('ReviewSidebar', () => { expect(text).toContain('fixture/no-floating-promises'); expect(text).toContain('Human comments (1)'); expect(text).toContain('Please clarify this return value.'); - expect(text).toContain('Send selected findings (1)'); - expect(text).toContain('Send human comments (1)'); + expect(text).toContain('Send to agent (2)'); }); it('marks stale findings textually and disables their remediation controls', () => { const html = render([finding({ freshness: 'stale' })]); expect(html).toContain('Stale'); - expect(html).toContain('disabled'); + expect(html).toMatch( + /]*disabled[^>]*aria-label="Select no-floating-promises finding"/, + ); }); it('marks pending findings textually and disables their remediation controls', () => { const html = render([finding({ freshness: 'pending' })]); expect(html).toContain('Pending'); - expect(html).toContain('disabled'); + expect(html).toMatch( + /]*disabled[^>]*aria-label="Select no-floating-promises finding"/, + ); + }); + + it('labels the unified send action as in progress while a submission is active', () => { + const html = render([finding()], { canSubmit: false, submitting: true }); + + expect(html).toContain('title="Sending review..."'); + expect(html).toContain('Sending...'); + expect(html).not.toContain('No agent available'); }); }); diff --git a/src/components/ReviewSidebar.tsx b/src/components/ReviewSidebar.tsx index ff0dac4b..53af8ad7 100644 --- a/src/components/ReviewSidebar.tsx +++ b/src/components/ReviewSidebar.tsx @@ -1,19 +1,16 @@ import { For, Show, createSignal } from 'solid-js'; import { theme } from '../lib/theme'; import { sf } from '../lib/fontScale'; -import { - formatQualityFindingLocation, - type QualityFinding, - type QualityFindingSeverity, -} from '../lib/quality-findings'; +import type { QualityFinding } from '../lib/quality-findings'; import type { ReviewAnnotation } from './review-types'; -import { CloseIcon } from './icons'; +import { QualityFindingSidebarItem } from './QualityFindingSidebarItem'; interface ReviewSidebarProps { annotations: ReviewAnnotation[]; findings: QualityFinding[]; selectedFindingIds: ReadonlySet; canSubmit: boolean; + submitting: boolean; onDismiss: (id: string) => void; onUpdate: (id: string, comment: string) => void; onScrollTo: (annotation: ReviewAnnotation) => void; @@ -21,170 +18,12 @@ interface ReviewSidebarProps { onFindingSelected: (id: string, selected: boolean) => void; onFindingDismiss: (id: string) => void; onFindingScrollTo: (finding: QualityFinding) => void; - onFindingSubmit: (ids?: string[]) => void; } function truncate(text: string, max: number): string { return text.length > max ? text.slice(0, max) + '...' : text; } -function severityColor(severity: QualityFindingSeverity): string { - if (severity === 'error') return theme.error; - if (severity === 'warning') return theme.warning; - return theme.accent; -} - -function QualityFindingSidebarItem(props: { - finding: QualityFinding; - selected: boolean; - canSubmit: boolean; - onSelected: (selected: boolean) => void; - onDismiss: () => void; - onScrollTo: () => void; - onSubmit: () => void; -}) { - const color = () => severityColor(props.finding.severity); - const isActionable = () => props.finding.freshness === 'current'; - const freshnessLabel = () => { - if (props.finding.freshness === 'pending') return 'Pending'; - if (props.finding.freshness === 'stale') return 'Stale'; - return null; - }; - - return ( -
{ - if (isActionable()) props.onScrollTo(); - }} - style={{ - padding: '8px 10px', - 'margin-bottom': '6px', - 'border-left': `3px solid ${isActionable() ? color() : theme.fgSubtle}`, - 'border-radius': '0 4px 4px 0', - background: 'color-mix(in srgb, var(--fg) 3%, transparent)', - cursor: isActionable() ? 'pointer' : 'default', - opacity: isActionable() ? '1' : '0.65', - }} - > -
- event.stopPropagation()} - onChange={(event) => props.onSelected(event.currentTarget.checked)} - /> - - Automated · {props.finding.severity} · {props.finding.category} - - - - {(label) => ( - - {label()} - - )} - - -
- -
- {formatQualityFindingLocation(props.finding)} -
- -
- {props.finding.source}/{props.finding.ruleId} -
- -
- {truncate(props.finding.explanation, 180)} -
- -
- -
-
- ); -} - function SidebarAnnotationItem(props: { annotation: ReviewAnnotation; onDismiss: () => void; @@ -326,6 +165,15 @@ function SidebarAnnotationItem(props: { } export function ReviewSidebar(props: ReviewSidebarProps) { + const submissionCount = () => props.annotations.length + props.selectedFindingIds.size; + const canSend = () => props.canSubmit && submissionCount() > 0; + const sendTitle = () => { + if (props.submitting) return 'Sending review...'; + if (!props.canSubmit) return 'No agent available to receive review'; + if (submissionCount() === 0) return 'Select at least one finding'; + return undefined; + }; + return (
props.onFindingSelected(finding.id, selected)} onDismiss={() => props.onFindingDismiss(finding.id)} onScrollTo={() => props.onFindingScrollTo(finding)} - onSubmit={() => props.onFindingSubmit([finding.id])} /> )} @@ -416,55 +262,26 @@ export function ReviewSidebar(props: ReviewSidebarProps) { 'border-top': `1px solid ${theme.border}`, }} > -
- 0}> - - - 0}> - - -
+
); diff --git a/src/components/ReviewSidebarPanel.test.ts b/src/components/ReviewSidebarPanel.test.ts deleted file mode 100644 index 1f8694fd..00000000 --- a/src/components/ReviewSidebarPanel.test.ts +++ /dev/null @@ -1,12 +0,0 @@ -import { describe, expect, it } from 'vitest'; -import { hasReviewSidebarState } from './ReviewSidebarPanel'; - -describe('hasReviewSidebarState', () => { - it('keeps the review button and panel reachable when only submission failed', () => { - expect(hasReviewSidebarState(0, false, '', 'terminal unavailable')).toBe(true); - }); - - it('hides an empty review state without loading or errors', () => { - expect(hasReviewSidebarState(0, false, '', '')).toBe(false); - }); -}); diff --git a/src/components/ReviewSidebarPanel.tsx b/src/components/ReviewSidebarPanel.tsx index 651df94c..05d51439 100644 --- a/src/components/ReviewSidebarPanel.tsx +++ b/src/components/ReviewSidebarPanel.tsx @@ -1,32 +1,42 @@ import { Show } from 'solid-js'; -import { useReview } from './ReviewProvider'; +import { useReview, type ReviewContextValue } from './ReviewProvider'; import { ReviewSidebar } from './ReviewSidebar'; import { theme } from '../lib/theme'; import { sf } from '../lib/fontScale'; +import { CloseIcon } from './icons'; -export function hasReviewSidebarState( - reviewCount: number, - findingsLoading: boolean, - findingsError: string, - submitError: string, -): boolean { - return reviewCount > 0 || findingsLoading || Boolean(findingsError) || Boolean(submitError); +interface ReviewSidebarState { + reviewCount: number; + findingsLoading: boolean; + findingsError: string; + submitError: string; +} + +function hasReviewSidebarState(state: ReviewSidebarState): boolean { + return ( + state.reviewCount > 0 || + state.findingsLoading || + Boolean(state.findingsError) || + Boolean(state.submitError) + ); +} + +function currentReviewSidebarState(review: ReviewContextValue): ReviewSidebarState { + return { + reviewCount: review.annotations().length + review.openFindings().length, + findingsLoading: review.findingsLoading(), + findingsError: review.findingsError(), + submitError: review.submitError(), + }; } /** Toggle button that shows annotation count and opens/closes the review sidebar. */ export function ReviewCommentsButton() { const review = useReview(); - const reviewCount = () => review.annotations().length + review.openFindings().length; - const hasReviewState = () => - hasReviewSidebarState( - reviewCount(), - review.findingsLoading(), - review.findingsError(), - review.submitError(), - ); + const state = () => currentReviewSidebarState(review); return ( - + ); @@ -48,17 +58,10 @@ export function ReviewCommentsButton() { /** Sidebar column with human comments and provider findings. */ export function ReviewSidebarPanel() { const review = useReview(); - const reviewCount = () => review.annotations().length + review.openFindings().length; - const hasReviewState = () => - hasReviewSidebarState( - reviewCount(), - review.findingsLoading(), - review.findingsError(), - review.submitError(), - ); + const state = () => currentReviewSidebarState(review); return ( - +
- Quality findings unavailable: {review.findingsError()} + + Quality findings unavailable: {review.findingsError()} + +
void review.submitFindings(ids)} />
diff --git a/src/components/ScrollingDiffView.test.ts b/src/components/ScrollingDiffView.test.ts deleted file mode 100644 index 5b5ad868..00000000 --- a/src/components/ScrollingDiffView.test.ts +++ /dev/null @@ -1,19 +0,0 @@ -import { describe, expect, it } from 'vitest'; -import { expandCollapsedFileForNavigation } from './ScrollingDiffView'; - -describe('expandCollapsedFileForNavigation', () => { - it('expands the target file while preserving other collapsed sections', () => { - const collapsed = new Set(['src/target.ts', 'src/other.ts']); - - const expanded = expandCollapsedFileForNavigation(collapsed, 'src/target.ts'); - - expect([...expanded]).toEqual(['src/other.ts']); - expect([...collapsed]).toEqual(['src/target.ts', 'src/other.ts']); - }); - - it('keeps the same state when the navigation target is already expanded', () => { - const collapsed = new Set(['src/other.ts']); - - expect(expandCollapsedFileForNavigation(collapsed, 'src/target.ts')).toBe(collapsed); - }); -}); diff --git a/src/components/ScrollingDiffView.tsx b/src/components/ScrollingDiffView.tsx index 35731714..51d3b336 100644 --- a/src/components/ScrollingDiffView.tsx +++ b/src/components/ScrollingDiffView.tsx @@ -93,7 +93,7 @@ function isLineHighlighted( ); } -export function expandCollapsedFileForNavigation( +function expandCollapsedFileForNavigation( collapsedFiles: ReadonlySet, filePath: string, ): ReadonlySet { @@ -471,9 +471,7 @@ function FileSection(props: { onDismissAnnotation: (id: string) => void; onAnnotationUpdate: (id: string, comment: string) => void; qualityFindings: QualityFinding[]; - canSubmitFindings: boolean; onDismissFinding: (id: string) => void; - onSubmitFinding: (id: string) => void; highlightedRange?: HighlightRange | null; pendingInput?: { filePath: string; afterLine: number } | null; onSubmit: (text: string, mode: 'review' | 'ask') => void; @@ -732,7 +730,7 @@ function FileSection(props: { props.qualityFindings, props.file.path, (finding) => finding.location.filePath, - (finding) => finding.location.endLine ?? finding.location.startLine, + (finding) => finding.location.startLine, hunk.newStart, nextStart, )} @@ -740,9 +738,7 @@ function FileSection(props: { {(finding) => ( props.onDismissFinding(finding.id)} - onSubmit={() => props.onSubmitFinding(finding.id)} /> )} @@ -790,6 +786,11 @@ export function ScrollingDiffView(props: ScrollingDiffViewProps) { let dimTimer: ReturnType | undefined; let containerRef: HTMLDivElement | undefined; + createEffect(() => { + setCollapsedFiles(new Set()); + return props.files; + }); + const highlightedRange = (): HighlightRange | null => { const selection = review.pendingSelection(); if (selection) { @@ -864,6 +865,7 @@ export function ScrollingDiffView(props: ScrollingDiffViewProps) { const elTop = el.getBoundingClientRect().top; containerRef.scrollTop = elTop - containerTop + containerRef.scrollTop - 80; } + if (props.scrollToAnnotation === target) review.setScrollTarget(null); }); }); }); @@ -963,9 +965,7 @@ export function ScrollingDiffView(props: ScrollingDiffViewProps) { qualityFindings={review .openFindings() .filter((finding) => finding.freshness === 'current')} - canSubmitFindings={review.canSubmit()} onDismissFinding={review.dismissFinding} - onSubmitFinding={(id) => void review.submitFindings([id])} highlightedRange={highlightedRange()} pendingInput={(() => { const pi = review.pendingSelection(); diff --git a/src/components/TaskPanel.tsx b/src/components/TaskPanel.tsx index 533e592c..ecb967c4 100644 --- a/src/components/TaskPanel.tsx +++ b/src/components/TaskPanel.tsx @@ -42,6 +42,7 @@ import type { Task } from '../store/types'; import type { CommitInfo } from '../ipc/types'; import { isLandedTaskState } from '../store/landing'; import { shouldPollTaskCommits } from './task-commit-polling'; +import { devQualityFindingProvider } from './dev-quality-finding-fixture'; interface TaskPanelProps { task: Task; @@ -656,6 +657,7 @@ export function TaskPanel(props: TaskPanelProps) { selectedCommit={selectedCommit()} onCommitNavigate={setSelectedCommit} gitIsolation={props.task.gitIsolation} + findingProvider={devQualityFindingProvider} /> setEditingProjectId(null)} /> diff --git a/src/components/dev-quality-finding-fixture.ts b/src/components/dev-quality-finding-fixture.ts new file mode 100644 index 00000000..a10b6410 --- /dev/null +++ b/src/components/dev-quality-finding-fixture.ts @@ -0,0 +1,31 @@ +import { + createFixtureQualityFindingProvider, + type QualityFindingProvider, +} from '../lib/quality-findings'; + +function createDevQualityFindingProvider(): QualityFindingProvider | undefined { + // Opt in with a changed file and line so the fixture reconciles against the open task diff. + if (!import.meta.env.DEV || import.meta.env.VITE_QUALITY_FINDING_FIXTURE !== 'true') { + return undefined; + } + + const filePath = import.meta.env.VITE_QUALITY_FINDING_FIXTURE_PATH; + const startLine = Number(import.meta.env.VITE_QUALITY_FINDING_FIXTURE_LINE); + if (!filePath || !Number.isInteger(startLine) || startLine < 1) return undefined; + + return createFixtureQualityFindingProvider([ + { + id: `dev-fixture:${filePath}:${startLine}`, + source: 'fixture', + ruleId: 'dev-quality-finding', + category: 'maintainability', + severity: 'note', + location: { filePath, startLine }, + explanation: 'Development fixture for verifying the structured quality-finding review loop.', + state: 'open', + freshness: 'pending', + }, + ]); +} + +export const devQualityFindingProvider = createDevQualityFindingProvider(); diff --git a/src/components/quality-finding-colors.ts b/src/components/quality-finding-colors.ts new file mode 100644 index 00000000..96148e32 --- /dev/null +++ b/src/components/quality-finding-colors.ts @@ -0,0 +1,8 @@ +import { theme } from '../lib/theme'; +import type { QualityFindingSeverity } from '../lib/quality-findings'; + +export function qualityFindingSeverityColor(severity: QualityFindingSeverity): string { + if (severity === 'error') return theme.error; + if (severity === 'warning') return theme.warning; + return theme.accent; +} diff --git a/src/lib/quality-findings.test.ts b/src/lib/quality-findings.test.ts index d6cbe7b7..cf18ad5a 100644 --- a/src/lib/quality-findings.test.ts +++ b/src/lib/quality-findings.test.ts @@ -6,6 +6,7 @@ import { dismissQualityFinding, reconcileQualityFindings, reconcileQualityFindingsForDiff, + resolveQualityFindings, selectedFindingIdsAfterSubmission, selectSubmittableFindings, type QualityFinding, @@ -14,7 +15,6 @@ import { function finding(overrides: Partial = {}): QualityFinding { return { id: 'finding-1', - fingerprint: 'fixture:no-floating-promises:src/app.ts:10', source: 'fixture', ruleId: 'no-floating-promises', category: 'reliability', @@ -141,7 +141,6 @@ describe('compileQualityFindingPrompt', () => { finding(), finding({ id: 'finding-2', - fingerprint: 'fixture:complexity:src/util.ts:4', ruleId: 'complexity', category: 'maintainability', severity: 'note', @@ -157,28 +156,53 @@ describe('compileQualityFindingPrompt', () => { expect(prompt).toContain('[warning] [reliability] fixture/no-floating-promises'); expect(prompt).toContain('Location: src/app.ts:10:3'); - expect(prompt).toContain('Fingerprint: fixture:no-floating-promises:src/app.ts:10'); + expect(prompt).not.toContain('Fingerprint:'); expect(prompt).toContain('[note] [maintainability] fixture/complexity'); expect(prompt).toContain('Location: src/util.ts:4:2-8:7'); }); + + it('renders same-line column ranges without confusing the end column for a line', () => { + const prompt = compileQualityFindingPrompt([ + finding({ + location: { + filePath: 'src/app.ts', + startLine: 10, + startColumn: 3, + endColumn: 7, + }, + }), + ]); + + expect(prompt).toContain('Location: src/app.ts:10:3-10:7'); + }); }); describe('finding review actions', () => { - it('dismisses by state without dropping provider identity', () => { + it('dismisses by state without dropping the stable provider ID', () => { const original = finding(); const dismissed = dismissQualityFinding([original], original.id); expect(dismissed[0]).toMatchObject({ id: original.id, - fingerprint: original.fingerprint, state: 'dismissed', }); }); + it('marks only successfully submitted findings resolved', () => { + const submitted = finding(); + const untouched = finding({ id: 'finding-b' }); + const result = resolveQualityFindings([submitted, untouched], [submitted]); + + expect(result.map(({ id, state }) => ({ id, state }))).toEqual([ + { id: 'finding-1', state: 'resolved' }, + { id: 'finding-b', state: 'open' }, + ]); + }); + it('submits only selected open findings with current locations', () => { const current = finding(); - const stale = finding({ id: 'stale', fingerprint: 'stale', freshness: 'stale' }); - const resolved = finding({ id: 'resolved', fingerprint: 'resolved', state: 'resolved' }); + const stale = finding({ id: 'stale', freshness: 'stale' }); + const resolved = finding({ id: 'resolved', state: 'resolved' }); expect( selectSubmittableFindings([current, stale, resolved], ['finding-1', 'stale', 'resolved']), @@ -196,7 +220,7 @@ describe('finding review actions', () => { it('removes only snapshotted bulk IDs from the latest selection', () => { const remaining = selectedFindingIdsAfterSubmission( new Set(['finding-1', 'finding-b', 'finding-c']), - [finding(), finding({ id: 'finding-b', fingerprint: 'finding-b' })], + [finding(), finding({ id: 'finding-b' })], ); expect([...remaining]).toEqual(['finding-c']); diff --git a/src/lib/quality-findings.ts b/src/lib/quality-findings.ts index f61ba771..6644cd89 100644 --- a/src/lib/quality-findings.ts +++ b/src/lib/quality-findings.ts @@ -14,8 +14,8 @@ export interface QualityFindingLocation { } export interface QualityFinding { + /** Stable provider-defined identifier or fingerprint. */ id: string; - fingerprint: string; source: string; ruleId: string; category: QualityFindingCategory; @@ -96,6 +96,20 @@ export function dismissQualityFinding(findings: QualityFinding[], id: string): Q ); } +export function resolveQualityFindings( + findings: QualityFinding[], + submittedFindings: QualityFinding[], +): QualityFinding[] { + const submittedIds = new Set(submittedFindings.map((finding) => finding.id)); + let changed = false; + const resolved = findings.map((finding) => { + if (!submittedIds.has(finding.id) || finding.state === 'resolved') return finding; + changed = true; + return { ...finding, state: 'resolved' as const }; + }); + return changed ? resolved : findings; +} + export function selectSubmittableFindings( findings: QualityFinding[], ids: Iterable, @@ -123,7 +137,7 @@ export function formatQualityFindingLocation(finding: QualityFinding): string { if (location.endLine && location.endLine !== location.startLine) { end = `-${location.endLine}${location.endColumn ? `:${location.endColumn}` : ''}`; } else if (location.endColumn && location.endColumn !== location.startColumn) { - end = `-${location.endColumn}`; + end = `-${location.startLine}:${location.endColumn}`; } return `${location.filePath}:${location.startLine}${startColumn}${end}`; } @@ -135,7 +149,6 @@ export function compileQualityFindingPrompt(findings: QualityFinding[]): string `## [${finding.severity}] [${finding.category}] ${finding.source}/${finding.ruleId}`, ); lines.push(`Location: ${formatQualityFindingLocation(finding)}`); - lines.push(`Fingerprint: ${finding.fingerprint}`); lines.push(finding.explanation); lines.push(''); } diff --git a/src/vite-env.d.ts b/src/vite-env.d.ts index 11f02fe2..d63839fd 100644 --- a/src/vite-env.d.ts +++ b/src/vite-env.d.ts @@ -1 +1,7 @@ /// + +interface ImportMetaEnv { + readonly VITE_QUALITY_FINDING_FIXTURE?: string; + readonly VITE_QUALITY_FINDING_FIXTURE_PATH?: string; + readonly VITE_QUALITY_FINDING_FIXTURE_LINE?: string; +} From c5ae1d3771178af6706c162eef7bffcf962997f9 Mon Sep 17 00:00:00 2001 From: Liang Hu Date: Tue, 28 Jul 2026 14:04:34 -0400 Subject: [PATCH 05/10] fix(review): preserve repeat navigation and clear stale errors Signed-off-by: Liang Hu --- src/components/ReviewProvider.test.ts | 19 +++++++++++++++++++ src/components/ReviewProvider.tsx | 6 ++++-- 2 files changed, 23 insertions(+), 2 deletions(-) diff --git a/src/components/ReviewProvider.test.ts b/src/components/ReviewProvider.test.ts index f091f198..506288b2 100644 --- a/src/components/ReviewProvider.test.ts +++ b/src/components/ReviewProvider.test.ts @@ -117,4 +117,23 @@ describe('ReviewProvider submission', () => { expect(review.sidebarOpen()).toBe(true); expect(review.annotations()).toHaveLength(1); }); + + it('clears a previous submission error when nothing remains to send', async () => { + vi.mocked(sendPrompt).mockRejectedValue(new Error('terminal unavailable')); + const review = renderReviewProvider(); + review.addAnnotation({ + id: 'annotation-1', + filePath: 'src/app.ts', + startLine: 11, + endLine: 11, + selectedText: 'runAsync();', + comment: 'Handle this promise.', + }); + + await review.submitReview(); + review.dismissAnnotation('annotation-1'); + await review.submitReview(); + + expect(review.submitError()).toBe(''); + }); }); diff --git a/src/components/ReviewProvider.tsx b/src/components/ReviewProvider.tsx index bd2a45c0..0a52c617 100644 --- a/src/components/ReviewProvider.tsx +++ b/src/components/ReviewProvider.tsx @@ -120,7 +120,9 @@ export function ReviewProvider(props: ReviewProviderProps) { const [findingsLoading, setFindingsLoading] = createSignal(false); const [findingsError, setFindingsError] = createSignal(''); const [sidebarOpen, setSidebarOpen] = createSignal(false); - const [scrollTarget, setScrollTarget] = createSignal(null); + const [scrollTarget, setScrollTarget] = createSignal(null, { + equals: false, + }); const [pendingSelection, setPendingSelection] = createSignal(null); const [activeQuestions, setActiveQuestions] = createSignal([]); const [submitError, setSubmitError] = createSignal(''); @@ -262,6 +264,7 @@ export function ReviewProvider(props: ReviewProviderProps) { const taskId = props.taskId; const agentId = props.agentId; if (!taskId || !agentId) return; + setSubmitError(''); const submittedAnnotations = annotations(); const submittedFindings = selectSubmittableFindings(findings(), selectedFindingIds()); if (submittedAnnotations.length === 0 && submittedFindings.length === 0) return; @@ -276,7 +279,6 @@ export function ReviewProvider(props: ReviewProviderProps) { const onSubmitted = props.onSubmitted; await submission.run(async () => { - setSubmitError(''); try { await sendPrompt(taskId, agentId, prompt); setAnnotations((previous) => From 54bdd0bb2045ec78a77be29cba4b8a24d52b471b Mon Sep 17 00:00:00 2001 From: Liang Hu Date: Wed, 29 Jul 2026 16:35:45 -0400 Subject: [PATCH 06/10] fix(review): allow dismissing submission errors --- src/components/ReviewProvider.test.ts | 4 ++-- src/components/ReviewProvider.tsx | 2 ++ src/components/ReviewSidebarPanel.tsx | 22 +++++++++++++++++++++- 3 files changed, 25 insertions(+), 3 deletions(-) diff --git a/src/components/ReviewProvider.test.ts b/src/components/ReviewProvider.test.ts index 506288b2..d7df6a61 100644 --- a/src/components/ReviewProvider.test.ts +++ b/src/components/ReviewProvider.test.ts @@ -118,7 +118,7 @@ describe('ReviewProvider submission', () => { expect(review.annotations()).toHaveLength(1); }); - it('clears a previous submission error when nothing remains to send', async () => { + it('allows dismissing a submission error after the final annotation is dismissed', async () => { vi.mocked(sendPrompt).mockRejectedValue(new Error('terminal unavailable')); const review = renderReviewProvider(); review.addAnnotation({ @@ -132,7 +132,7 @@ describe('ReviewProvider submission', () => { await review.submitReview(); review.dismissAnnotation('annotation-1'); - await review.submitReview(); + review.clearSubmitError(); expect(review.submitError()).toBe(''); }); diff --git a/src/components/ReviewProvider.tsx b/src/components/ReviewProvider.tsx index 0a52c617..69b91db5 100644 --- a/src/components/ReviewProvider.tsx +++ b/src/components/ReviewProvider.tsx @@ -75,6 +75,7 @@ export interface ReviewContextValue { dismissQuestion: (id: string) => void; submitError: () => string; + clearSubmitError: () => void; } interface ReviewProviderProps { @@ -326,6 +327,7 @@ export function ReviewProvider(props: ReviewProviderProps) { submitting: submission.submitting, submitReview, submitError, + clearSubmitError: () => setSubmitError(''), }; return {props.children}; diff --git a/src/components/ReviewSidebarPanel.tsx b/src/components/ReviewSidebarPanel.tsx index 05d51439..c2cec042 100644 --- a/src/components/ReviewSidebarPanel.tsx +++ b/src/components/ReviewSidebarPanel.tsx @@ -71,9 +71,29 @@ export function ReviewSidebarPanel() { 'font-size': sf(12), 'border-bottom': `1px solid ${theme.border}`, background: 'rgba(255, 95, 115, 0.08)', + display: 'flex', + 'align-items': 'center', + gap: '8px', }} > - {review.submitError()} + {review.submitError()} + From 97fca994e1657ef9d2fe118d45f5c7c8ac0dc44e Mon Sep 17 00:00:00 2001 From: Liang Hu Date: Thu, 30 Jul 2026 19:26:34 -0400 Subject: [PATCH 07/10] fix(review): preserve incremental review state --- src/components/DiffViewerDialog.tsx | 59 ++++++++++++------------ src/components/ReviewProvider.test.ts | 27 ++++++++++- src/components/ReviewProvider.tsx | 28 ++++++++--- src/components/ScrollingDiffView.tsx | 55 +++++++++++++--------- src/components/review-navigation.test.ts | 51 ++++++++++++++++++++ src/components/review-navigation.ts | 24 ++++++++++ 6 files changed, 185 insertions(+), 59 deletions(-) create mode 100644 src/components/review-navigation.test.ts create mode 100644 src/components/review-navigation.ts diff --git a/src/components/DiffViewerDialog.tsx b/src/components/DiffViewerDialog.tsx index 427af609..a78238dc 100644 --- a/src/components/DiffViewerDialog.tsx +++ b/src/components/DiffViewerDialog.tsx @@ -75,33 +75,33 @@ export function compileDiffReview(annotations: ReviewAnnotation[]): string { export function DiffViewerDialog(props: DiffViewerDialogProps) { const titleId = createUniqueId(); return ( - -

- Diff viewer for {props.taskName ?? 'task'}: {props.scrollToFile ?? 'all changes'} -

- - + +

+ Diff viewer for {props.taskName ?? 'task'}: {props.scrollToFile ?? 'all changes'} +

+ - - - + + + ); } diff --git a/src/components/ReviewProvider.test.ts b/src/components/ReviewProvider.test.ts index d7df6a61..369b886d 100644 --- a/src/components/ReviewProvider.test.ts +++ b/src/components/ReviewProvider.test.ts @@ -9,7 +9,7 @@ vi.mock('../store/tasks', () => ({ sendPrompt: vi.fn(), })); -function finding(): QualityFinding { +function finding(overrides: Partial = {}): QualityFinding { return { id: 'finding-1', source: 'fixture', @@ -20,6 +20,7 @@ function finding(): QualityFinding { explanation: 'Await this promise.', state: 'open', freshness: 'current', + ...overrides, }; } @@ -118,6 +119,30 @@ describe('ReviewProvider submission', () => { expect(review.annotations()).toHaveLength(1); }); + it('keeps the review open until every actionable finding is submitted', async () => { + vi.mocked(sendPrompt).mockResolvedValue(); + const onSubmitted = vi.fn(); + const review = renderReviewProvider(onSubmitted); + review.replaceFindings(() => [finding(), finding({ id: 'finding-2', ruleId: 'prefer-const' })]); + review.setFindingSelected('finding-1', true); + + await review.submitReview(); + + expect(review.findings().map(({ id, state }) => ({ id, state }))).toEqual([ + { id: 'finding-1', state: 'resolved' }, + { id: 'finding-2', state: 'open' }, + ]); + expect(review.sidebarOpen()).toBe(true); + expect(onSubmitted).not.toHaveBeenCalled(); + + review.setFindingSelected('finding-2', true); + await review.submitReview(); + + expect(review.findings().every((item) => item.state === 'resolved')).toBe(true); + expect(review.sidebarOpen()).toBe(false); + expect(onSubmitted).toHaveBeenCalledOnce(); + }); + it('allows dismissing a submission error after the final annotation is dismissed', async () => { vi.mocked(sendPrompt).mockRejectedValue(new Error('terminal unavailable')); const review = renderReviewProvider(); diff --git a/src/components/ReviewProvider.tsx b/src/components/ReviewProvider.tsx index 69b91db5..a07a21ce 100644 --- a/src/components/ReviewProvider.tsx +++ b/src/components/ReviewProvider.tsx @@ -1,4 +1,11 @@ -import { createContext, createSignal, createEffect, createMemo, useContext } from 'solid-js'; +import { + createContext, + createSignal, + createEffect, + createMemo, + untrack, + useContext, +} from 'solid-js'; import type { JSX } from 'solid-js'; import { sendPrompt } from '../store/tasks'; import { @@ -265,7 +272,6 @@ export function ReviewProvider(props: ReviewProviderProps) { const taskId = props.taskId; const agentId = props.agentId; if (!taskId || !agentId) return; - setSubmitError(''); const submittedAnnotations = annotations(); const submittedFindings = selectSubmittableFindings(findings(), selectedFindingIds()); if (submittedAnnotations.length === 0 && submittedFindings.length === 0) return; @@ -280,17 +286,25 @@ export function ReviewProvider(props: ReviewProviderProps) { const onSubmitted = props.onSubmitted; await submission.run(async () => { + setSubmitError(''); try { await sendPrompt(taskId, agentId, prompt); - setAnnotations((previous) => - previous.filter((annotation) => !submittedAnnotationIds.has(annotation.id)), + const remainingAnnotations = untrack(annotations).filter( + (annotation) => !submittedAnnotationIds.has(annotation.id), ); - setFindings((previous) => resolveQualityFindings(previous, submittedFindings)); + const updatedFindings = resolveQualityFindings(untrack(findings), submittedFindings); + setAnnotations(remainingAnnotations); + setFindings(updatedFindings); setSelectedFindingIds((previous) => selectedFindingIdsAfterSubmission(previous, submittedFindings), ); - setSidebarOpen(false); - onSubmitted?.(); + const hasRemainingActionableReview = + remainingAnnotations.length > 0 || + updatedFindings.some( + (finding) => finding.state === 'open' && finding.freshness === 'current', + ); + setSidebarOpen(hasRemainingActionableReview); + if (!hasRemainingActionableReview) onSubmitted?.(); } catch (err: unknown) { setSubmitError(err instanceof Error ? err.message : 'Failed to send review'); setSidebarOpen(true); diff --git a/src/components/ScrollingDiffView.tsx b/src/components/ScrollingDiffView.tsx index 51d3b336..755554b5 100644 --- a/src/components/ScrollingDiffView.tsx +++ b/src/components/ScrollingDiffView.tsx @@ -1,4 +1,4 @@ -import { For, Show, createSignal, createEffect, onMount, onCleanup, untrack } from 'solid-js'; +import { For, Show, createSignal, createEffect, onMount, onCleanup, untrack, on } from 'solid-js'; import type { JSX } from 'solid-js'; import { theme } from '../lib/theme'; import { sf } from '../lib/fontScale'; @@ -19,6 +19,10 @@ import { InlineInput } from './InlineInput'; import { useReview, type ActiveQuestion, type ReviewScrollTarget } from './ReviewProvider'; import type { QualityFinding } from '../lib/quality-findings'; import type { ReviewAnnotation, DiffInteractionMode } from './review-types'; +import { + expandCollapsedFileForNavigation, + scheduleReviewNavigationHighlightClear, +} from './review-navigation'; interface ScrollingDiffViewProps { files: FileDiff[]; @@ -93,16 +97,6 @@ function isLineHighlighted( ); } -function expandCollapsedFileForNavigation( - collapsedFiles: ReadonlySet, - filePath: string, -): ReadonlySet { - if (!collapsedFiles.has(filePath)) return collapsedFiles; - const expanded = new Set(collapsedFiles); - expanded.delete(filePath); - return expanded; -} - // --------------------------------------------------------------------------- // Search highlight helpers // --------------------------------------------------------------------------- @@ -783,13 +777,17 @@ export function ScrollingDiffView(props: ScrollingDiffViewProps) { const sectionRefs = new Map(); const [collapsedFiles, setCollapsedFiles] = createSignal>(new Set()); const [dimOthers, setDimOthers] = createSignal(false); - let dimTimer: ReturnType | undefined; + let navigationFrame: number | undefined; + let navigationLineFrame: number | undefined; + let navigationHighlightTimer: ReturnType | undefined; let containerRef: HTMLDivElement | undefined; - createEffect(() => { - setCollapsedFiles(new Set()); - return props.files; - }); + createEffect( + on( + () => props.files, + () => setCollapsedFiles(new Set()), + ), + ); const highlightedRange = (): HighlightRange | null => { const selection = review.pendingSelection(); @@ -810,13 +808,21 @@ export function ScrollingDiffView(props: ScrollingDiffViewProps) { : null; }; - onCleanup(() => clearTimeout(dimTimer)); + function clearNavigationSchedule() { + if (navigationFrame !== undefined) cancelAnimationFrame(navigationFrame); + if (navigationLineFrame !== undefined) cancelAnimationFrame(navigationLineFrame); + clearTimeout(navigationHighlightTimer); + navigationFrame = undefined; + navigationLineFrame = undefined; + navigationHighlightTimer = undefined; + } + + onCleanup(clearNavigationSchedule); /** Scroll to a file section when scrollToPath changes. */ createEffect(() => { const target = props.scrollToPath; if (!target) return; - clearTimeout(dimTimer); setDimOthers(true); // Start fade-in on next frame so the browser registers the dimmed state first requestAnimationFrame(() => setDimOthers(false)); @@ -850,13 +856,16 @@ export function ScrollingDiffView(props: ScrollingDiffViewProps) { /** Scroll to a specific annotation (e.g. clicked in the sidebar). */ createEffect(() => { const target = props.scrollToAnnotation; + clearNavigationSchedule(); if (!target) return; const currentCollapsed = untrack(collapsedFiles); const expanded = expandCollapsedFileForNavigation(currentCollapsed, target.filePath); if (expanded !== currentCollapsed) setCollapsedFiles(expanded); - requestAnimationFrame(() => { - requestAnimationFrame(() => { + navigationFrame = requestAnimationFrame(() => { + navigationFrame = undefined; + navigationLineFrame = requestAnimationFrame(() => { + navigationLineFrame = undefined; const el = containerRef?.querySelector( `[data-file-path="${CSS.escape(target.filePath)}"][data-new-line="${target.startLine}"]`, ); @@ -865,7 +874,11 @@ export function ScrollingDiffView(props: ScrollingDiffViewProps) { const elTop = el.getBoundingClientRect().top; containerRef.scrollTop = elTop - containerTop + containerRef.scrollTop - 80; } - if (props.scrollToAnnotation === target) review.setScrollTarget(null); + navigationHighlightTimer = scheduleReviewNavigationHighlightClear( + target, + () => untrack(() => props.scrollToAnnotation), + () => review.setScrollTarget(null), + ); }); }); }); diff --git a/src/components/review-navigation.test.ts b/src/components/review-navigation.test.ts new file mode 100644 index 00000000..9acc3d1b --- /dev/null +++ b/src/components/review-navigation.test.ts @@ -0,0 +1,51 @@ +import { afterEach, describe, expect, it, vi } from 'vitest'; +import type { ReviewScrollTarget } from './ReviewProvider'; +import { + REVIEW_NAVIGATION_HIGHLIGHT_MS, + expandCollapsedFileForNavigation, + scheduleReviewNavigationHighlightClear, +} from './review-navigation'; + +afterEach(() => { + vi.useRealTimers(); +}); + +describe('review navigation', () => { + it('expands a collapsed target without changing an already-expanded set', () => { + const collapsed = new Set(['src/app.ts', 'src/other.ts']); + const expanded = expandCollapsedFileForNavigation(collapsed, 'src/app.ts'); + + expect([...expanded]).toEqual(['src/other.ts']); + expect(expandCollapsedFileForNavigation(expanded, 'src/app.ts')).toBe(expanded); + }); + + it('keeps the target active for the highlight interval before clearing it', () => { + vi.useFakeTimers(); + const target: ReviewScrollTarget = { filePath: 'src/app.ts', startLine: 10 }; + let currentTarget: ReviewScrollTarget | null = target; + const clearTarget = vi.fn(() => { + currentTarget = null; + }); + + scheduleReviewNavigationHighlightClear(target, () => currentTarget, clearTarget); + + vi.advanceTimersByTime(REVIEW_NAVIGATION_HIGHLIGHT_MS - 1); + expect(clearTarget).not.toHaveBeenCalled(); + + vi.advanceTimersByTime(1); + expect(clearTarget).toHaveBeenCalledOnce(); + expect(currentTarget).toBeNull(); + }); + + it('does not clear a newer navigation target', () => { + vi.useFakeTimers(); + const target: ReviewScrollTarget = { filePath: 'src/app.ts', startLine: 10 }; + const currentTarget: ReviewScrollTarget = { filePath: 'src/other.ts', startLine: 20 }; + const clearTarget = vi.fn(); + + scheduleReviewNavigationHighlightClear(target, () => currentTarget, clearTarget); + vi.runAllTimers(); + + expect(clearTarget).not.toHaveBeenCalled(); + }); +}); diff --git a/src/components/review-navigation.ts b/src/components/review-navigation.ts new file mode 100644 index 00000000..e442cbfc --- /dev/null +++ b/src/components/review-navigation.ts @@ -0,0 +1,24 @@ +import type { ReviewScrollTarget } from './ReviewProvider'; + +export const REVIEW_NAVIGATION_HIGHLIGHT_MS = 1200; + +export function expandCollapsedFileForNavigation( + collapsedFiles: ReadonlySet, + filePath: string, +): ReadonlySet { + if (!collapsedFiles.has(filePath)) return collapsedFiles; + const expanded = new Set(collapsedFiles); + expanded.delete(filePath); + return expanded; +} + +export function scheduleReviewNavigationHighlightClear( + target: ReviewScrollTarget, + currentTarget: () => ReviewScrollTarget | null | undefined, + clearTarget: () => void, + delay = REVIEW_NAVIGATION_HIGHLIGHT_MS, +): ReturnType { + return setTimeout(() => { + if (currentTarget() === target) clearTarget(); + }, delay); +} From 2f70bc11667b9bdee77f5a6914f05e448ab70778 Mon Sep 17 00:00:00 2001 From: Liang Hu Date: Fri, 31 Jul 2026 09:28:34 -0400 Subject: [PATCH 08/10] fix(review): scope persisted review state Signed-off-by: Liang Hu --- package-lock.json | 63 ++++ package.json | 5 +- src/components/DiffViewerDialog.tsx | 59 ++-- src/components/ReviewProvider.client.test.tsx | 292 ++++++++++++++++++ src/components/ReviewProvider.test.ts | 1 + src/components/ReviewProvider.tsx | 205 ++++++++++-- src/lib/diff-review-lifecycle.test.ts | 95 ++++++ src/lib/diff-review-lifecycle.ts | 57 ++++ src/lib/quality-findings.test.ts | 34 +- src/lib/quality-findings.ts | 21 +- vitest.client.config.ts | 10 + 11 files changed, 790 insertions(+), 52 deletions(-) create mode 100644 src/components/ReviewProvider.client.test.tsx create mode 100644 src/lib/diff-review-lifecycle.test.ts create mode 100644 src/lib/diff-review-lifecycle.ts create mode 100644 vitest.client.config.ts diff --git a/package-lock.json b/package-lock.json index 4ee8b55b..865eaf20 100644 --- a/package-lock.json +++ b/package-lock.json @@ -47,6 +47,7 @@ "eslint": "^9.39.3", "eslint-config-prettier": "^10.1.8", "eslint-plugin-solid": "^0.14.5", + "happy-dom": "^20.11.1", "husky": "^9.1.7", "knip": "^6.12.2", "lint-staged": "^16.2.7", @@ -3592,6 +3593,13 @@ "license": "MIT", "optional": true }, + "node_modules/@types/whatwg-mimetype": { + "version": "3.0.2", + "resolved": "https://registry.npmjs.org/@types/whatwg-mimetype/-/whatwg-mimetype-3.0.2.tgz", + "integrity": "sha512-c2AKvDT8ToxLIOUlN51gTiHXflsfIFisS4pO7pDPoKouJCESkhZnEy623gwP9laCy5lnLDAw1vAzu2vM2YLOrA==", + "dev": true, + "license": "MIT" + }, "node_modules/@types/ws": { "version": "8.18.1", "resolved": "https://registry.npmjs.org/@types/ws/-/ws-8.18.1.tgz", @@ -4862,6 +4870,19 @@ "dev": true, "license": "MIT" }, + "node_modules/buffer-image-size": { + "version": "0.6.4", + "resolved": "https://registry.npmjs.org/buffer-image-size/-/buffer-image-size-0.6.4.tgz", + "integrity": "sha512-nEh+kZOPY1w+gcCMobZ6ETUp9WfibndnosbpwB1iJk/8Gt5ZF2bhS6+B6bPYz424KtwsR6Rflc3tCz1/ghX2dQ==", + "dev": true, + "license": "MIT", + "dependencies": { + "@types/node": "*" + }, + "engines": { + "node": ">=4.0" + } + }, "node_modules/builder-util": { "version": "26.8.1", "resolved": "https://registry.npmjs.org/builder-util/-/builder-util-26.8.1.tgz", @@ -8294,6 +8315,38 @@ "integrity": "sha512-3GKBOn+m2LX9iq+JC1064cSFprJY4jL1jCXTcpnfER5HYE2l/4EfWSGzkPa/ZDBmYI0ZOEj5VHV/eKnPGkHuOg==", "license": "MIT" }, + "node_modules/happy-dom": { + "version": "20.11.1", + "resolved": "https://registry.npmjs.org/happy-dom/-/happy-dom-20.11.1.tgz", + "integrity": "sha512-XSt8tMzbW9ymE7687xztkO1ckR7qJNQ3LywY9vlYGhGi3zXrGBHuUo2Cl1ztZaICW+1eAGdkLbj6iwVqDT33kg==", + "dev": true, + "license": "MIT", + "dependencies": { + "@types/node": ">=20.0.0", + "@types/whatwg-mimetype": "^3.0.2", + "@types/ws": "^8.18.1", + "buffer-image-size": "^0.6.4", + "entities": "^7.0.1", + "whatwg-mimetype": "^3.0.0", + "ws": "^8.21.0" + }, + "engines": { + "node": ">=20.0.0" + } + }, + "node_modules/happy-dom/node_modules/entities": { + "version": "7.0.1", + "resolved": "https://registry.npmjs.org/entities/-/entities-7.0.1.tgz", + "integrity": "sha512-TWrgLOFUQTH994YUyl1yT4uyavY5nNB5muff+RtWaqNVCAK408b5ZnnbNAUEWLTCpum9w6arT70i1XdQ4UeOPA==", + "dev": true, + "license": "BSD-2-Clause", + "engines": { + "node": ">=0.12" + }, + "funding": { + "url": "https://github.com/fb55/entities?sponsor=1" + } + }, "node_modules/has-flag": { "version": "4.0.0", "resolved": "https://registry.npmjs.org/has-flag/-/has-flag-4.0.0.tgz", @@ -13739,6 +13792,16 @@ "defaults": "^1.0.3" } }, + "node_modules/whatwg-mimetype": { + "version": "3.0.0", + "resolved": "https://registry.npmjs.org/whatwg-mimetype/-/whatwg-mimetype-3.0.0.tgz", + "integrity": "sha512-nt+N2dzIutVRxARx1nghPKGv1xHikU7HKdfafKkLNLindmPU/ch3U31NOCGGA/dmPcmb1VlofO0vnKAcsm0o/Q==", + "dev": true, + "license": "MIT", + "engines": { + "node": ">=12" + } + }, "node_modules/which": { "version": "5.0.0", "resolved": "https://registry.npmjs.org/which/-/which-5.0.0.tgz", diff --git a/package.json b/package.json index 314e7819..37c8362c 100644 --- a/package.json +++ b/package.json @@ -26,7 +26,9 @@ "lint:secrets": "command -v gitleaks >/dev/null 2>&1 && gitleaks detect --config .gitleaks.toml || (echo 'gitleaks not installed (brew install gitleaks)' >&2; exit 1)", "format": "prettier --write .", "format:check": "prettier --check .", - "test": "vitest run", + "test": "npm run test:unit && npm run test:client", + "test:unit": "vitest run", + "test:client": "vitest run --config vitest.client.config.ts", "test:coordinator-pty": "RUN_COORDINATOR_PTY_TEST=1 vitest run electron/mcp/coordinator-real-pty.integration.test.ts", "test:coverage": "vitest run --coverage", "check:coordinator-log": "node scripts/check-coordinator-run.mjs", @@ -75,6 +77,7 @@ "eslint": "^9.39.3", "eslint-config-prettier": "^10.1.8", "eslint-plugin-solid": "^0.14.5", + "happy-dom": "^20.11.1", "husky": "^9.1.7", "knip": "^6.12.2", "lint-staged": "^16.2.7", diff --git a/src/components/DiffViewerDialog.tsx b/src/components/DiffViewerDialog.tsx index a78238dc..2184481f 100644 --- a/src/components/DiffViewerDialog.tsx +++ b/src/components/DiffViewerDialog.tsx @@ -4,14 +4,15 @@ import { errMessage } from '../lib/log'; import { invoke } from '../lib/ipc'; import { IPC } from '../../electron/ipc/channels'; import { createDialogScroll } from '../lib/dialog-scroll'; +import { + createDiffIdentity, + createRequestGenerationGuard, + createReviewIdentity, +} from '../lib/diff-review-lifecycle'; import { theme } from '../lib/theme'; import { sf } from '../lib/fontScale'; import { parseUnifiedDiff } from '../lib/unified-diff-parser'; -import { evictStaleAnnotations } from '../lib/review-eviction'; -import { - reconcileQualityFindingsForDiff, - type QualityFindingProvider, -} from '../lib/quality-findings'; +import { type QualityFindingProvider } from '../lib/quality-findings'; import { windowChromeTopInset } from '../lib/platform'; import { ScrollingDiffView } from './ScrollingDiffView'; import { @@ -74,11 +75,20 @@ export function compileDiffReview(annotations: ReviewAnnotation[]): string { export function DiffViewerDialog(props: DiffViewerDialogProps) { const titleId = createUniqueId(); + const reviewIdentity = () => + createReviewIdentity({ + taskId: props.taskId, + worktreePath: props.worktreePath, + projectRoot: props.projectRoot, + branchName: props.branchName, + }); return ( @@ -130,13 +140,12 @@ function DiffViewerContent(props: DiffViewerDialogProps) { const headerPaddingTop = `${windowChromeTopInset + 12}px`; const [parsedFiles, setParsedFiles] = createSignal([]); - const [diffLoaded, setDiffLoaded] = createSignal(false); const [loading, setLoading] = createSignal(false); const [error, setError] = createSignal(''); const [searchQuery, setSearchQuery] = createSignal(''); const [activeFilePath, setActiveFilePath] = createSignal(null); - let fetchGeneration = 0; + const fetchGeneration = createRequestGenerationGuard(); let searchInputRef: HTMLInputElement | undefined; let diffScrollRef: HTMLDivElement | undefined; let containerRef: HTMLDivElement | undefined; @@ -163,12 +172,6 @@ function DiffViewerContent(props: DiffViewerDialogProps) { setActiveFilePath(props.scrollToFile); }); - createEffect(() => { - const current = review.findings(); - const reconciled = reconcileQualityFindingsForDiff(current, parsedFiles(), diffLoaded()); - if (reconciled !== current) review.replaceFindings(() => reconciled); - }); - createEffect(() => { const scrollTarget = props.scrollToFile; // Access selectedCommit before the early return so the effect tracks it @@ -180,10 +183,21 @@ function DiffViewerContent(props: DiffViewerDialogProps) { const projectRoot = props.projectRoot; const branchName = props.branchName; const baseBranch = props.baseBranch; - const thisGen = ++fetchGeneration; - + const reviewIdentity = createReviewIdentity({ + taskId: props.taskId, + worktreePath, + projectRoot, + branchName, + }); + const thisGen = fetchGeneration.begin(); + + onCleanup(() => { + fetchGeneration.invalidate(); + review.suspendDiffLoad(); + }); + + review.beginDiffLoad(); setSearchQuery(''); - setDiffLoaded(false); setLoading(true); setError(''); setParsedFiles([]); @@ -219,19 +233,20 @@ function DiffViewerContent(props: DiffViewerDialogProps) { } diffPromise - .then((rawDiff) => { - if (thisGen !== fetchGeneration) return; + .then(async (rawDiff) => { + if (!fetchGeneration.isCurrent(thisGen)) return; const newFiles = parseUnifiedDiff(rawDiff); + const diffIdentity = await createDiffIdentity(reviewIdentity, rawDiff); + if (!fetchGeneration.isCurrent(thisGen)) return; setParsedFiles(newFiles); - review.replaceAnnotations((prev) => evictStaleAnnotations(prev, newFiles)); - setDiffLoaded(true); + review.completeDiffLoad(diffIdentity, newFiles); }) .catch((err) => { - if (thisGen !== fetchGeneration) return; + if (!fetchGeneration.isCurrent(thisGen)) return; setError(errMessage(err)); }) .finally(() => { - if (thisGen === fetchGeneration) setLoading(false); + if (fetchGeneration.isCurrent(thisGen)) setLoading(false); }); }); diff --git a/src/components/ReviewProvider.client.test.tsx b/src/components/ReviewProvider.client.test.tsx new file mode 100644 index 00000000..acf1a8ce --- /dev/null +++ b/src/components/ReviewProvider.client.test.tsx @@ -0,0 +1,292 @@ +import { Show, createEffect, createSignal, onCleanup, type Setter } from 'solid-js'; +import { render } from 'solid-js/web'; +import { afterEach, describe, expect, it, vi } from 'vitest'; +import { + createRequestGenerationGuard, + type RequestGenerationGuard, +} from '../lib/diff-review-lifecycle'; +import type { + QualityFinding, + QualityFindingLoadContext, + QualityFindingProvider, +} from '../lib/quality-findings'; +import type { FileDiff } from '../lib/unified-diff-parser'; +import { ReviewProvider, useReview, type ReviewContextValue } from './ReviewProvider'; + +vi.mock('../store/tasks', () => ({ + sendPrompt: vi.fn(), +})); + +const disposers: Array<() => void> = []; + +afterEach(() => { + while (disposers.length > 0) disposers.pop()?.(); + document.body.replaceChildren(); +}); + +function finding(id: string): QualityFinding { + return { + id, + source: 'fixture', + ruleId: 'no-floating-promises', + category: 'reliability', + severity: 'warning', + location: { filePath: 'src/app.ts', startLine: 10 }, + explanation: 'Await this promise.', + state: 'open', + freshness: 'current', + }; +} + +function renderedDiff(content = 'runAsync();'): FileDiff[] { + return [ + { + path: 'src/app.ts', + status: 'M', + binary: false, + hunks: [ + { + oldStart: 9, + oldCount: 2, + newStart: 9, + newCount: 3, + lines: [ + { type: 'context', content: 'before', oldLine: 9, newLine: 9 }, + { type: 'add', content, oldLine: null, newLine: 10 }, + { type: 'context', content: 'after', oldLine: 10, newLine: 11 }, + ], + }, + ], + }, + ]; +} + +interface MountedReview { + review: ReviewContextValue; + setOpen: Setter; + setReviewIdentity: Setter; +} + +function mountReview(findingProvider?: QualityFindingProvider): MountedReview { + const [open, setOpen] = createSignal(true); + const [reviewIdentity, setReviewIdentity] = createSignal('task-a:/worktree-a'); + let review: ReviewContextValue | undefined; + + function CaptureReview() { + review = useReview(); + return null; + } + + const host = document.createElement('div'); + document.body.append(host); + disposers.push( + render( + () => ( + ''} + > + + + ), + host, + ), + ); + + if (!review) throw new Error('Review context was not rendered'); + return { review, setOpen, setReviewIdentity }; +} + +async function flushAsyncWork(): Promise { + await Promise.resolve(); + await Promise.resolve(); +} + +describe('ReviewProvider client lifecycle', () => { + it('keeps durable comments on same-diff reopen and clears transient interaction state', async () => { + const loadFindings = vi.fn(async () => [finding('finding-1')]); + const { review, setOpen } = mountReview({ loadFindings }); + review.completeDiffLoad('diff-a', renderedDiff()); + await flushAsyncWork(); + review.dismissFinding('finding-1'); + review.addAnnotation({ + id: 'annotation-1', + filePath: 'src/app.ts', + startLine: 10, + endLine: 10, + selectedText: 'runAsync();', + comment: 'Handle this promise.', + }); + review.handleSelection({ + source: 'src/app.ts', + startLine: 10, + endLine: 10, + selectedText: 'runAsync();', + }); + review.handleSubmit('Why is this not awaited?', 'ask'); + review.handleSelection({ + source: 'src/app.ts', + startLine: 11, + endLine: 11, + selectedText: 'after', + }); + review.setScrollTarget({ filePath: 'src/app.ts', startLine: 10 }); + + setOpen(false); + await flushAsyncWork(); + + expect(review.annotations()).toHaveLength(1); + expect(review.findings()).toMatchObject([{ id: 'finding-1', state: 'dismissed' }]); + expect(review.pendingSelection()).toBeNull(); + expect(review.activeQuestions()).toEqual([]); + expect(review.scrollTarget()).toBeNull(); + expect(review.sidebarOpen()).toBe(false); + + setOpen(true); + await flushAsyncWork(); + review.beginDiffLoad(); + review.completeDiffLoad('diff-a', renderedDiff()); + + expect(review.annotations()).toHaveLength(1); + expect(review.findings()).toMatchObject([ + { id: 'finding-1', state: 'dismissed', freshness: 'current' }, + ]); + expect(loadFindings).toHaveBeenCalledOnce(); + }); + + it('clears durable and transient state when the worktree identity changes', async () => { + const { review, setReviewIdentity } = mountReview(); + review.completeDiffLoad('diff-a', renderedDiff()); + review.addAnnotation({ + id: 'annotation-1', + filePath: 'src/app.ts', + startLine: 10, + endLine: 10, + selectedText: 'runAsync();', + comment: 'Handle this promise.', + }); + review.handleSelection({ + source: 'src/app.ts', + startLine: 10, + endLine: 10, + selectedText: 'runAsync();', + }); + + setReviewIdentity('task-b:/worktree-b'); + await flushAsyncWork(); + + expect(review.annotations()).toEqual([]); + expect(review.pendingSelection()).toBeNull(); + expect(review.findings()).toEqual([]); + }); + + it('ignores a provider response from a closed session after the same diff reopens', async () => { + const requests: Array<{ + context: QualityFindingLoadContext; + resolve: (findings: QualityFinding[]) => void; + }> = []; + const provider: QualityFindingProvider = { + loadFindings: vi.fn( + (context) => + new Promise((resolve) => { + requests.push({ context, resolve }); + }), + ), + }; + const { review, setOpen } = mountReview(provider); + + review.beginDiffLoad(); + review.completeDiffLoad('diff-a', renderedDiff()); + setOpen(false); + await flushAsyncWork(); + setOpen(true); + await flushAsyncWork(); + review.beginDiffLoad(); + review.completeDiffLoad('diff-a', renderedDiff()); + + expect(requests.map((request) => request.context.diffIdentity)).toEqual(['diff-a', 'diff-a']); + + requests[0].resolve([finding('late-finding')]); + await flushAsyncWork(); + expect(review.findings()).toEqual([]); + + requests[1].resolve([finding('current-finding')]); + await flushAsyncWork(); + expect(review.findings()).toMatchObject([ + { id: 'current-finding', freshness: 'current', state: 'open' }, + ]); + }); +}); + +interface PendingRequest { + promise: Promise; + resolve: (value: string) => void; +} + +function pendingRequest(): PendingRequest { + let resolve: ((value: string) => void) | undefined; + const promise = new Promise((complete) => { + resolve = complete; + }); + if (!resolve) throw new Error('Request resolver was not initialized'); + return { promise, resolve }; +} + +function RequestOwner(props: { + request: PendingRequest; + guard: RequestGenerationGuard; + onApply: (value: string) => void; +}) { + createEffect(() => { + const guard = props.guard; + const onApply = props.onApply; + const request = props.request; + const generation = guard.begin(); + onCleanup(() => guard.invalidate()); + void request.promise.then((value) => { + if (guard.isCurrent(generation)) onApply(value); + }); + }); + return null; +} + +describe('diff request client lifecycle', () => { + it('rejects an old request after close and reopen even when it resolves last', async () => { + const first = pendingRequest(); + const second = pendingRequest(); + const applied: string[] = []; + const [open, setOpen] = createSignal(true); + const [request, setRequest] = createSignal(first); + const guard = createRequestGenerationGuard(); + const host = document.createElement('div'); + document.body.append(host); + disposers.push( + render( + () => ( + + applied.push(value)} + /> + + ), + host, + ), + ); + + setOpen(false); + setRequest(second); + setOpen(true); + second.resolve('new'); + await flushAsyncWork(); + first.resolve('old'); + await flushAsyncWork(); + + expect(applied).toEqual(['new']); + }); +}); diff --git a/src/components/ReviewProvider.test.ts b/src/components/ReviewProvider.test.ts index 369b886d..6a47773f 100644 --- a/src/components/ReviewProvider.test.ts +++ b/src/components/ReviewProvider.test.ts @@ -36,6 +36,7 @@ function renderReviewProvider(onSubmitted = vi.fn()): ReviewContextValue { createComponent(ReviewProvider, { taskId: 'task-1', agentId: 'agent-1', + reviewIdentity: 'task-1:/worktree', compilePrompt: (annotations) => `Human comments: ${annotations.length}`, onSubmitted, get children() { diff --git a/src/components/ReviewProvider.tsx b/src/components/ReviewProvider.tsx index a07a21ce..e7c2aa75 100644 --- a/src/components/ReviewProvider.tsx +++ b/src/components/ReviewProvider.tsx @@ -3,6 +3,7 @@ import { createSignal, createEffect, createMemo, + onCleanup, untrack, useContext, } from 'solid-js'; @@ -11,12 +12,15 @@ import { sendPrompt } from '../store/tasks'; import { compileQualityFindingPrompt, dismissQualityFinding, + reconcileQualityFindingsForDiff, resolveQualityFindings, selectedFindingIdsAfterSubmission, selectSubmittableFindings, type QualityFinding, type QualityFindingProvider, } from '../lib/quality-findings'; +import { transitionReviewAnnotations, type ReviewDiffIdentity } from '../lib/diff-review-lifecycle'; +import type { FileDiff } from '../lib/unified-diff-parser'; import type { ReviewAnnotation, DiffInteractionMode } from './review-types'; /** Generic selection info used to create annotations or questions. */ @@ -65,6 +69,10 @@ export interface ReviewContextValue { findingsError: () => string; clearFindingsError: () => void; + beginDiffLoad: () => void; + completeDiffLoad: (diffIdentity: string, files: FileDiff[]) => void; + suspendDiffLoad: () => void; + scrollTarget: () => ReviewScrollTarget | null; setScrollTarget: (target: ReviewScrollTarget | null) => void; @@ -89,6 +97,10 @@ interface ReviewProviderProps { taskId?: string; agentId?: string; findingProvider?: QualityFindingProvider; + /** Stable task/worktree identity. Durable review state never crosses this boundary. */ + reviewIdentity?: string; + /** Whether the review surface is currently mounted and interactive. */ + open?: boolean; compilePrompt: (annotations: ReviewAnnotation[]) => string; onSubmitted?: () => void; children: JSX.Element; @@ -137,35 +149,57 @@ export function ReviewProvider(props: ReviewProviderProps) { const submission = createReviewSubmissionGuard(); const openFindings = createMemo(() => findings().filter((finding) => finding.state === 'open')); let findingLoadGeneration = 0; + let activeReviewDiff: ReviewDiffIdentity | null = null; + let findingsLoadedFor: ReviewDiffIdentity | null = null; + let findingsLoadedProvider: QualityFindingProvider | undefined; + let trackedReviewIdentity = untrack(() => props.reviewIdentity ?? ''); + let wasOpen = untrack(() => props.open ?? true); + + function invalidateFindingLoad() { + findingLoadGeneration++; + setFindingsLoading(false); + } - createEffect(() => { - const provider = props.findingProvider; - const generation = ++findingLoadGeneration; - setFindingsError(''); + function resetTransientState() { setSelectedFindingIds(new Set()); - if (!provider) { - setFindings([]); - setFindingsLoading(false); - return; - } + setSidebarOpen(false); + setScrollTarget(null); + setPendingSelection(null); + setActiveQuestions([]); + setSubmitError(''); + setFindingsError(''); + } - setFindingsLoading(true); - void provider - .loadFindings() - .then((loaded) => { - if (generation === findingLoadGeneration) setFindings(loaded); - }) - .catch((err: unknown) => { - if (generation !== findingLoadGeneration) return; - setFindings([]); - setFindingsError(err instanceof Error ? err.message : 'Failed to load quality findings'); - setSidebarOpen(true); - }) - .finally(() => { - if (generation === findingLoadGeneration) setFindingsLoading(false); - }); + function clearReviewState() { + invalidateFindingLoad(); + setAnnotations([]); + setFindings([]); + activeReviewDiff = null; + findingsLoadedFor = null; + findingsLoadedProvider = undefined; + resetTransientState(); + } + + createEffect(() => { + const reviewIdentity = props.reviewIdentity ?? ''; + if (reviewIdentity === trackedReviewIdentity) return; + trackedReviewIdentity = reviewIdentity; + clearReviewState(); }); + createEffect(() => { + const open = props.open ?? true; + if (open === wasOpen) return; + wasOpen = open; + if (open) { + resetTransientState(); + } else { + suspendDiffLoad(); + } + }); + + onCleanup(invalidateFindingLoad); + // Auto-open sidebar when human comments or provider findings are added. createEffect(() => { if (annotations().length > 0 || openFindings().length > 0) setSidebarOpen(true); @@ -217,6 +251,126 @@ export function ReviewProvider(props: ReviewProviderProps) { setFindings(fn); } + function sameReviewDiff( + left: ReviewDiffIdentity | null, + right: ReviewDiffIdentity | null, + ): boolean { + return Boolean( + left && + right && + left.reviewIdentity === right.reviewIdentity && + left.diffIdentity === right.diffIdentity, + ); + } + + function beginDiffLoad() { + invalidateFindingLoad(); + setFindings((prev) => reconcileQualityFindingsForDiff(prev, [], false)); + resetTransientState(); + } + + function loadFindingsForDiff(next: ReviewDiffIdentity, files: FileDiff[]) { + const provider = props.findingProvider; + if (!provider) { + setFindings([]); + setFindingsLoading(false); + findingsLoadedFor = next; + findingsLoadedProvider = undefined; + return; + } + + const generation = ++findingLoadGeneration; + setFindingsError(''); + setFindingsLoading(true); + void provider + .loadFindings({ + reviewIdentity: next.reviewIdentity, + diffIdentity: next.diffIdentity, + files, + }) + .then((loaded) => { + if ( + generation !== findingLoadGeneration || + !sameReviewDiff(activeReviewDiff, next) || + !wasOpen + ) { + return; + } + const reconciled = reconcileQualityFindingsForDiff( + loaded, + files, + true, + next.diffIdentity, + activeReviewDiff?.diffIdentity, + ); + findingsLoadedFor = next; + findingsLoadedProvider = provider; + setFindings(reconciled); + setSidebarOpen( + untrack(annotations).length > 0 || + reconciled.some( + (finding) => finding.state === 'open' && finding.freshness === 'current', + ), + ); + }) + .catch((err: unknown) => { + if (generation !== findingLoadGeneration || !sameReviewDiff(activeReviewDiff, next)) { + return; + } + setFindings([]); + setFindingsError(err instanceof Error ? err.message : 'Failed to load quality findings'); + setSidebarOpen(true); + }) + .finally(() => { + if (generation === findingLoadGeneration) setFindingsLoading(false); + }); + } + + function completeDiffLoad(diffIdentity: string, files: FileDiff[]) { + const next: ReviewDiffIdentity = { + reviewIdentity: props.reviewIdentity ?? '', + diffIdentity, + }; + const sameDiff = sameReviewDiff(activeReviewDiff, next); + + setAnnotations((prev) => transitionReviewAnnotations(prev, activeReviewDiff, next, files)); + activeReviewDiff = next; + + if (!sameDiff) { + invalidateFindingLoad(); + setFindings([]); + setSelectedFindingIds(new Set()); + findingsLoadedFor = null; + findingsLoadedProvider = undefined; + } else if ( + sameReviewDiff(findingsLoadedFor, next) && + findingsLoadedProvider === props.findingProvider + ) { + setFindings((prev) => + reconcileQualityFindingsForDiff( + prev, + files, + true, + findingsLoadedFor?.diffIdentity, + diffIdentity, + ), + ); + setSidebarOpen( + annotations().length > 0 || + findings().some((finding) => finding.state === 'open' && finding.freshness === 'current'), + ); + return; + } + + loadFindingsForDiff(next, files); + } + + function suspendDiffLoad() { + invalidateFindingLoad(); + setFindings((prev) => reconcileQualityFindingsForDiff(prev, [], false)); + resetTransientState(); + } + function handleSelection(selection: ContentSelection) { setPendingSelection(selection); } @@ -327,6 +481,9 @@ export function ReviewProvider(props: ReviewProviderProps) { findingsLoading, findingsError, clearFindingsError: () => setFindingsError(''), + beginDiffLoad, + completeDiffLoad, + suspendDiffLoad, sidebarOpen, setSidebarOpen, scrollTarget, diff --git a/src/lib/diff-review-lifecycle.test.ts b/src/lib/diff-review-lifecycle.test.ts new file mode 100644 index 00000000..ce82c2e4 --- /dev/null +++ b/src/lib/diff-review-lifecycle.test.ts @@ -0,0 +1,95 @@ +import { describe, expect, it } from 'vitest'; +import type { ReviewAnnotation } from '../components/review-types'; +import { + createDiffIdentity, + createRequestGenerationGuard, + transitionReviewAnnotations, +} from './diff-review-lifecycle'; +import type { FileDiff } from './unified-diff-parser'; + +function annotation(): ReviewAnnotation { + return { + id: 'annotation-1', + filePath: 'src/app.ts', + startLine: 10, + endLine: 10, + selectedText: 'before();', + comment: 'Keep this behavior.', + }; +} + +function touchingDiff(): FileDiff[] { + return [ + { + path: 'src/app.ts', + status: 'M', + binary: false, + hunks: [ + { + oldStart: 10, + oldCount: 1, + newStart: 10, + newCount: 1, + lines: [ + { type: 'remove', content: 'before();', oldLine: 10, newLine: null }, + { type: 'add', content: 'after();', oldLine: null, newLine: 10 }, + ], + }, + ], + }, + ]; +} + +describe('diff review lifecycle', () => { + it('keeps durable comments when the exact same diff is reopened', () => { + const annotations = [annotation()]; + const identity = { reviewIdentity: 'task-a', diffIdentity: 'diff-a' }; + + expect(transitionReviewAnnotations(annotations, identity, identity, touchingDiff())).toBe( + annotations, + ); + }); + + it('evicts touched comments only on an actual diff transition', () => { + expect( + transitionReviewAnnotations( + [annotation()], + { reviewIdentity: 'task-a', diffIdentity: 'diff-a' }, + { reviewIdentity: 'task-a', diffIdentity: 'diff-b' }, + touchingDiff(), + ), + ).toEqual([]); + }); + + it('never carries durable comments into another worktree review', () => { + expect( + transitionReviewAnnotations( + [annotation()], + { reviewIdentity: 'worktree-a', diffIdentity: 'same-diff' }, + { reviewIdentity: 'worktree-b', diffIdentity: 'same-diff' }, + [], + ), + ).toEqual([]); + }); + + it('invalidates a closed viewer request before a reopened request starts', () => { + const guard = createRequestGenerationGuard(); + const closedRequest = guard.begin(); + guard.invalidate(); + const reopenedRequest = guard.begin(); + + expect(guard.isCurrent(closedRequest)).toBe(false); + expect(guard.isCurrent(reopenedRequest)).toBe(true); + }); + + it('derives stable identities from the exact review scope and diff content', async () => { + const first = await createDiffIdentity('task-a', 'diff content'); + const same = await createDiffIdentity('task-a', 'diff content'); + const changedContent = await createDiffIdentity('task-a', 'different content'); + const changedScope = await createDiffIdentity('task-b', 'diff content'); + + expect(same).toBe(first); + expect(changedContent).not.toBe(first); + expect(changedScope).not.toBe(first); + }); +}); diff --git a/src/lib/diff-review-lifecycle.ts b/src/lib/diff-review-lifecycle.ts new file mode 100644 index 00000000..7e64286f --- /dev/null +++ b/src/lib/diff-review-lifecycle.ts @@ -0,0 +1,57 @@ +import type { ReviewAnnotation } from '../components/review-types'; +import { evictStaleAnnotations } from './review-eviction'; +import type { FileDiff } from './unified-diff-parser'; + +export interface ReviewDiffIdentity { + reviewIdentity: string; + diffIdentity: string; +} + +export interface RequestGenerationGuard { + begin: () => number; + invalidate: () => void; + isCurrent: (generation: number) => boolean; +} + +export function createRequestGenerationGuard(): RequestGenerationGuard { + let current = 0; + return { + begin: () => ++current, + invalidate: () => { + current++; + }, + isCurrent: (generation) => generation === current, + }; +} + +export function createReviewIdentity(parts: { + taskId?: string; + worktreePath: string; + projectRoot?: string; + branchName?: string | null; +}): string { + return JSON.stringify([ + parts.taskId ?? null, + parts.worktreePath, + parts.projectRoot ?? null, + parts.branchName ?? null, + ]); +} + +export async function createDiffIdentity(reviewIdentity: string, rawDiff: string): Promise { + const bytes = new TextEncoder().encode(`${reviewIdentity}\0${rawDiff}`); + const digest = await crypto.subtle.digest('SHA-256', bytes); + return Array.from(new Uint8Array(digest), (byte) => byte.toString(16).padStart(2, '0')).join(''); +} + +export function transitionReviewAnnotations( + annotations: ReviewAnnotation[], + previous: ReviewDiffIdentity | null, + next: ReviewDiffIdentity, + files: FileDiff[], +): ReviewAnnotation[] { + if (!previous) return annotations; + if (previous.reviewIdentity !== next.reviewIdentity) return []; + if (previous.diffIdentity === next.diffIdentity) return annotations; + return evictStaleAnnotations(annotations, files); +} diff --git a/src/lib/quality-findings.test.ts b/src/lib/quality-findings.test.ts index cf18ad5a..409e0a4a 100644 --- a/src/lib/quality-findings.test.ts +++ b/src/lib/quality-findings.test.ts @@ -53,10 +53,11 @@ describe('createFixtureQualityFindingProvider', () => { it('supplies independent copies of fixture findings', async () => { const original = finding(); const provider = createFixtureQualityFindingProvider([original]); + const context = { reviewIdentity: 'task-1', diffIdentity: 'diff-1', files: [diff()] }; - const first = await provider.loadFindings(); + const first = await provider.loadFindings(context); first[0].location.startLine = 99; - const second = await provider.loadFindings(); + const second = await provider.loadFindings(context); expect(second).toEqual([original]); }); @@ -108,6 +109,35 @@ describe('reconcileQualityFindings', () => { expect(rejected[0].freshness).toBe('pending'); }); + it('does not reuse a finding from another immutable diff at the same path and line', () => { + const changedContent = diff({ + hunks: [ + { + oldStart: 9, + oldCount: 2, + newStart: 9, + newCount: 3, + lines: [ + { type: 'context', content: 'before', oldLine: 9, newLine: 9 }, + { type: 'add', content: 'differentCall();', oldLine: null, newLine: 10 }, + { type: 'context', content: 'after', oldLine: 10, newLine: 11 }, + ], + }, + ], + }); + + const reconciled = reconcileQualityFindingsForDiff( + [finding()], + [changedContent], + true, + 'diff-before', + 'diff-after', + ); + + expect(reconciled[0].freshness).toBe('pending'); + expect(selectSubmittableFindings(reconciled, ['finding-1'])).toEqual([]); + }); + it('requires the navigable start line for a ranged finding', () => { const ranged = finding({ location: { filePath: 'src/app.ts', startLine: 8, endLine: 10 }, diff --git a/src/lib/quality-findings.ts b/src/lib/quality-findings.ts index 6644cd89..15c87878 100644 --- a/src/lib/quality-findings.ts +++ b/src/lib/quality-findings.ts @@ -26,8 +26,17 @@ export interface QualityFinding { freshness: QualityFindingFreshness; } +export interface QualityFindingLoadContext { + /** Stable identity for the task/worktree being reviewed. */ + reviewIdentity: string; + /** Immutable identity derived from the exact rendered diff. */ + diffIdentity: string; + /** Parsed files for providers that need to scope or validate fixture data. */ + files: readonly FileDiff[]; +} + export interface QualityFindingProvider { - loadFindings(): Promise; + loadFindings(context: QualityFindingLoadContext): Promise; } function cloneFinding(finding: QualityFinding): QualityFinding { @@ -42,7 +51,7 @@ export function createFixtureQualityFindingProvider( findings: QualityFinding[], ): QualityFindingProvider { return { - async loadFindings() { + async loadFindings(_context) { return findings.map(cloneFinding); }, }; @@ -61,8 +70,14 @@ export function reconcileQualityFindingsForDiff( findings: QualityFinding[], files: FileDiff[], diffLoaded: boolean, + findingsDiffIdentity?: string, + currentDiffIdentity?: string, ): QualityFinding[] { - if (diffLoaded) return reconcileQualityFindings(findings, files); + const identityMatches = + findingsDiffIdentity === undefined && currentDiffIdentity === undefined + ? true + : findingsDiffIdentity === currentDiffIdentity; + if (diffLoaded && identityMatches) return reconcileQualityFindings(findings, files); let changed = false; const pending = findings.map((finding) => { diff --git a/vitest.client.config.ts b/vitest.client.config.ts new file mode 100644 index 00000000..aba8afc5 --- /dev/null +++ b/vitest.client.config.ts @@ -0,0 +1,10 @@ +import { defineConfig } from 'vitest/config'; +import solidPlugin from 'vite-plugin-solid'; + +export default defineConfig({ + plugins: [solidPlugin({ ssr: false })], + test: { + environment: 'happy-dom', + include: ['src/**/*.client.test.tsx'], + }, +}); From 7eb916deb0367d8583e99c6096ff809d3a5f6c51 Mon Sep 17 00:00:00 2001 From: Liang Hu Date: Fri, 31 Jul 2026 10:35:52 -0400 Subject: [PATCH 09/10] fix(review): guard diff lifecycle transitions Signed-off-by: Liang Hu --- src/components/ReviewProvider.client.test.tsx | 77 ++++++++++++++++++- src/components/ReviewProvider.tsx | 22 +++++- src/lib/diff-review-lifecycle.test.ts | 71 +++++++++++++---- src/lib/diff-review-lifecycle.ts | 42 ++++++++-- 4 files changed, 188 insertions(+), 24 deletions(-) diff --git a/src/components/ReviewProvider.client.test.tsx b/src/components/ReviewProvider.client.test.tsx index acf1a8ce..859ef2f0 100644 --- a/src/components/ReviewProvider.client.test.tsx +++ b/src/components/ReviewProvider.client.test.tsx @@ -5,6 +5,7 @@ import { createRequestGenerationGuard, type RequestGenerationGuard, } from '../lib/diff-review-lifecycle'; +import { sendPrompt } from '../store/tasks'; import type { QualityFinding, QualityFindingLoadContext, @@ -22,6 +23,7 @@ const disposers: Array<() => void> = []; afterEach(() => { while (disposers.length > 0) disposers.pop()?.(); document.body.replaceChildren(); + vi.mocked(sendPrompt).mockReset(); }); function finding(id: string): QualityFinding { @@ -67,7 +69,10 @@ interface MountedReview { setReviewIdentity: Setter; } -function mountReview(findingProvider?: QualityFindingProvider): MountedReview { +function mountReview( + findingProvider?: QualityFindingProvider, + onSubmitted?: () => void, +): MountedReview { const [open, setOpen] = createSignal(true); const [reviewIdentity, setReviewIdentity] = createSignal('task-a:/worktree-a'); let review: ReviewContextValue | undefined; @@ -89,6 +94,7 @@ function mountReview(findingProvider?: QualityFindingProvider): MountedReview { reviewIdentity={reviewIdentity()} open={open()} compilePrompt={() => ''} + onSubmitted={onSubmitted} > @@ -220,6 +226,75 @@ describe('ReviewProvider client lifecycle', () => { { id: 'current-finding', freshness: 'current', state: 'open' }, ]); }); + + it('ignores a completed submission after navigation moves to another diff', async () => { + let resolveSend: (() => void) | undefined; + vi.mocked(sendPrompt).mockImplementation( + () => + new Promise((resolve) => { + resolveSend = resolve; + }), + ); + const onSubmitted = vi.fn(); + const { review } = mountReview(undefined, onSubmitted); + review.beginDiffLoad(); + review.completeDiffLoad('diff-a', renderedDiff('runA();')); + review.addAnnotation({ + id: 'annotation-a', + filePath: 'src/app.ts', + startLine: 10, + endLine: 10, + selectedText: 'runA();', + comment: 'Review diff A.', + }); + + const submission = review.submitReview(); + review.beginDiffLoad(); + review.completeDiffLoad('diff-b', renderedDiff('runB();')); + review.addAnnotation({ + id: 'annotation-b', + filePath: 'src/app.ts', + startLine: 10, + endLine: 10, + selectedText: 'runB();', + comment: 'Review diff B.', + }); + resolveSend?.(); + await submission; + + expect(review.annotations().map((annotation) => annotation.id)).toEqual(['annotation-b']); + expect(review.submitError()).toBe(''); + expect(onSubmitted).not.toHaveBeenCalled(); + }); + + it('ignores a rejected submission after navigation moves to another diff', async () => { + let rejectSend: ((error: Error) => void) | undefined; + vi.mocked(sendPrompt).mockImplementation( + () => + new Promise((_resolve, reject) => { + rejectSend = reject; + }), + ); + const { review } = mountReview(); + review.beginDiffLoad(); + review.completeDiffLoad('diff-a', renderedDiff('runA();')); + review.addAnnotation({ + id: 'annotation-a', + filePath: 'src/app.ts', + startLine: 10, + endLine: 10, + selectedText: 'runA();', + comment: 'Review diff A.', + }); + + const submission = review.submitReview(); + review.beginDiffLoad(); + review.completeDiffLoad('diff-b', renderedDiff('runB();')); + rejectSend?.(new Error('diff A terminal failure')); + await submission; + + expect(review.submitError()).toBe(''); + }); }); interface PendingRequest { diff --git a/src/components/ReviewProvider.tsx b/src/components/ReviewProvider.tsx index e7c2aa75..04c378c9 100644 --- a/src/components/ReviewProvider.tsx +++ b/src/components/ReviewProvider.tsx @@ -19,7 +19,11 @@ import { type QualityFinding, type QualityFindingProvider, } from '../lib/quality-findings'; -import { transitionReviewAnnotations, type ReviewDiffIdentity } from '../lib/diff-review-lifecycle'; +import { + transitionReviewAnnotations, + type ReviewDiffIdentity, + type ReviewDiffSnapshot, +} from '../lib/diff-review-lifecycle'; import type { FileDiff } from '../lib/unified-diff-parser'; import type { ReviewAnnotation, DiffInteractionMode } from './review-types'; @@ -149,11 +153,12 @@ export function ReviewProvider(props: ReviewProviderProps) { const submission = createReviewSubmissionGuard(); const openFindings = createMemo(() => findings().filter((finding) => finding.state === 'open')); let findingLoadGeneration = 0; - let activeReviewDiff: ReviewDiffIdentity | null = null; + let activeReviewDiff: ReviewDiffSnapshot | null = null; let findingsLoadedFor: ReviewDiffIdentity | null = null; let findingsLoadedProvider: QualityFindingProvider | undefined; let trackedReviewIdentity = untrack(() => props.reviewIdentity ?? ''); let wasOpen = untrack(() => props.open ?? true); + let reviewLifecycleGeneration = 0; function invalidateFindingLoad() { findingLoadGeneration++; @@ -171,6 +176,7 @@ export function ReviewProvider(props: ReviewProviderProps) { } function clearReviewState() { + reviewLifecycleGeneration++; invalidateFindingLoad(); setAnnotations([]); setFindings([]); @@ -264,6 +270,7 @@ export function ReviewProvider(props: ReviewProviderProps) { } function beginDiffLoad() { + reviewLifecycleGeneration++; invalidateFindingLoad(); setFindings((prev) => reconcileQualityFindingsForDiff(prev, [], false)); resetTransientState(); @@ -327,13 +334,16 @@ export function ReviewProvider(props: ReviewProviderProps) { } function completeDiffLoad(diffIdentity: string, files: FileDiff[]) { - const next: ReviewDiffIdentity = { + const next: ReviewDiffSnapshot = { reviewIdentity: props.reviewIdentity ?? '', diffIdentity, + files, }; const sameDiff = sameReviewDiff(activeReviewDiff, next); - setAnnotations((prev) => transitionReviewAnnotations(prev, activeReviewDiff, next, files)); + if (!sameDiff) reviewLifecycleGeneration++; + + setAnnotations((prev) => transitionReviewAnnotations(prev, activeReviewDiff, next)); activeReviewDiff = next; if (!sameDiff) { @@ -366,6 +376,7 @@ export function ReviewProvider(props: ReviewProviderProps) { } function suspendDiffLoad() { + reviewLifecycleGeneration++; invalidateFindingLoad(); setFindings((prev) => reconcileQualityFindingsForDiff(prev, [], false)); resetTransientState(); @@ -438,11 +449,13 @@ export function ReviewProvider(props: ReviewProviderProps) { .join('\n'); const submittedAnnotationIds = new Set(submittedAnnotations.map((annotation) => annotation.id)); const onSubmitted = props.onSubmitted; + const submittedReviewLifecycle = reviewLifecycleGeneration; await submission.run(async () => { setSubmitError(''); try { await sendPrompt(taskId, agentId, prompt); + if (submittedReviewLifecycle !== reviewLifecycleGeneration) return; const remainingAnnotations = untrack(annotations).filter( (annotation) => !submittedAnnotationIds.has(annotation.id), ); @@ -460,6 +473,7 @@ export function ReviewProvider(props: ReviewProviderProps) { setSidebarOpen(hasRemainingActionableReview); if (!hasRemainingActionableReview) onSubmitted?.(); } catch (err: unknown) { + if (submittedReviewLifecycle !== reviewLifecycleGeneration) return; setSubmitError(err instanceof Error ? err.message : 'Failed to send review'); setSidebarOpen(true); } diff --git a/src/lib/diff-review-lifecycle.test.ts b/src/lib/diff-review-lifecycle.test.ts index ce82c2e4..77f50272 100644 --- a/src/lib/diff-review-lifecycle.test.ts +++ b/src/lib/diff-review-lifecycle.test.ts @@ -4,6 +4,7 @@ import { createDiffIdentity, createRequestGenerationGuard, transitionReviewAnnotations, + type ReviewDiffSnapshot, } from './diff-review-lifecycle'; import type { FileDiff } from './unified-diff-parser'; @@ -18,7 +19,7 @@ function annotation(): ReviewAnnotation { }; } -function touchingDiff(): FileDiff[] { +function renderedDiff(content = 'before();'): FileDiff[] { return [ { path: 'src/app.ts', @@ -31,8 +32,8 @@ function touchingDiff(): FileDiff[] { newStart: 10, newCount: 1, lines: [ - { type: 'remove', content: 'before();', oldLine: 10, newLine: null }, - { type: 'add', content: 'after();', oldLine: null, newLine: 10 }, + { type: 'remove', content: 'base();', oldLine: 10, newLine: null }, + { type: 'add', content, oldLine: null, newLine: 10 }, ], }, ], @@ -40,23 +41,66 @@ function touchingDiff(): FileDiff[] { ]; } +function unrelatedDiff(): FileDiff { + return { + path: 'src/other.ts', + status: 'M', + binary: false, + hunks: [ + { + oldStart: 2, + oldCount: 1, + newStart: 2, + newCount: 1, + lines: [ + { type: 'remove', content: 'old();', oldLine: 2, newLine: null }, + { type: 'add', content: 'newer();', oldLine: null, newLine: 2 }, + ], + }, + ], + }; +} + +function snapshot( + diffIdentity: string, + files: FileDiff[], + reviewIdentity = 'task-a', +): ReviewDiffSnapshot { + return { reviewIdentity, diffIdentity, files }; +} + describe('diff review lifecycle', () => { it('keeps durable comments when the exact same diff is reopened', () => { const annotations = [annotation()]; - const identity = { reviewIdentity: 'task-a', diffIdentity: 'diff-a' }; + const identity = snapshot('diff-a', renderedDiff()); + + expect(transitionReviewAnnotations(annotations, identity, identity)).toBe(annotations); + }); - expect(transitionReviewAnnotations(annotations, identity, identity, touchingDiff())).toBe( - annotations, - ); + it('keeps an annotation when a cumulative diff changes only an unrelated file', () => { + const annotations = [annotation()]; + const previous = snapshot('diff-a', renderedDiff()); + const next = snapshot('diff-b', [...renderedDiff(), unrelatedDiff()]); + + expect(transitionReviewAnnotations(annotations, previous, next)).toBe(annotations); + }); + + it('evicts an annotation when its anchored content changes', () => { + expect( + transitionReviewAnnotations( + [annotation()], + snapshot('diff-a', renderedDiff()), + snapshot('diff-b', renderedDiff('after();')), + ), + ).toEqual([]); }); - it('evicts touched comments only on an actual diff transition', () => { + it('evicts an annotation when its anchored range disappears from the diff', () => { expect( transitionReviewAnnotations( [annotation()], - { reviewIdentity: 'task-a', diffIdentity: 'diff-a' }, - { reviewIdentity: 'task-a', diffIdentity: 'diff-b' }, - touchingDiff(), + snapshot('diff-a', renderedDiff()), + snapshot('diff-b', [unrelatedDiff()]), ), ).toEqual([]); }); @@ -65,9 +109,8 @@ describe('diff review lifecycle', () => { expect( transitionReviewAnnotations( [annotation()], - { reviewIdentity: 'worktree-a', diffIdentity: 'same-diff' }, - { reviewIdentity: 'worktree-b', diffIdentity: 'same-diff' }, - [], + snapshot('same-diff', renderedDiff(), 'worktree-a'), + snapshot('same-diff', renderedDiff(), 'worktree-b'), ), ).toEqual([]); }); diff --git a/src/lib/diff-review-lifecycle.ts b/src/lib/diff-review-lifecycle.ts index 7e64286f..240f1944 100644 --- a/src/lib/diff-review-lifecycle.ts +++ b/src/lib/diff-review-lifecycle.ts @@ -1,5 +1,4 @@ import type { ReviewAnnotation } from '../components/review-types'; -import { evictStaleAnnotations } from './review-eviction'; import type { FileDiff } from './unified-diff-parser'; export interface ReviewDiffIdentity { @@ -7,6 +6,10 @@ export interface ReviewDiffIdentity { diffIdentity: string; } +export interface ReviewDiffSnapshot extends ReviewDiffIdentity { + files: FileDiff[]; +} + export interface RequestGenerationGuard { begin: () => number; invalidate: () => void; @@ -44,14 +47,43 @@ export async function createDiffIdentity(reviewIdentity: string, rawDiff: string return Array.from(new Uint8Array(digest), (byte) => byte.toString(16).padStart(2, '0')).join(''); } +function annotationContentAnchor(annotation: ReviewAnnotation, files: FileDiff[]): string[] | null { + const file = files.find((candidate) => candidate.path === annotation.filePath); + if (!file || file.binary || file.status === 'D') return null; + + const contentByLine = new Map(); + for (const hunk of file.hunks) { + for (const line of hunk.lines) { + if (line.newLine !== null) contentByLine.set(line.newLine, line.content); + } + } + + const content: string[] = []; + for (let line = annotation.startLine; line <= annotation.endLine; line++) { + if (!contentByLine.has(line)) return null; + content.push(contentByLine.get(line) ?? ''); + } + return content; +} + +function sameContentAnchor(previous: string[], next: string[]): boolean { + return previous.length === next.length && previous.every((line, index) => line === next[index]); +} + export function transitionReviewAnnotations( annotations: ReviewAnnotation[], - previous: ReviewDiffIdentity | null, - next: ReviewDiffIdentity, - files: FileDiff[], + previous: ReviewDiffSnapshot | null, + next: ReviewDiffSnapshot, ): ReviewAnnotation[] { if (!previous) return annotations; if (previous.reviewIdentity !== next.reviewIdentity) return []; if (previous.diffIdentity === next.diffIdentity) return annotations; - return evictStaleAnnotations(annotations, files); + + const retained = annotations.filter((annotation) => { + const previousAnchor = annotationContentAnchor(annotation, previous.files); + if (!previousAnchor) return true; + const nextAnchor = annotationContentAnchor(annotation, next.files); + return nextAnchor !== null && sameContentAnchor(previousAnchor, nextAnchor); + }); + return retained.length === annotations.length ? annotations : retained; } From f8502abc1865017495a74983114285a7000b06e5 Mon Sep 17 00:00:00 2001 From: Liang Hu Date: Sat, 1 Aug 2026 11:00:21 -0400 Subject: [PATCH 10/10] fix(review): evict unanchored annotations on diff change Signed-off-by: Liang Hu --- src/components/ReviewProvider.client.test.tsx | 18 ++++++++++++++++++ src/lib/diff-review-lifecycle.test.ts | 17 +++++++++++++++++ src/lib/diff-review-lifecycle.ts | 2 +- 3 files changed, 36 insertions(+), 1 deletion(-) diff --git a/src/components/ReviewProvider.client.test.tsx b/src/components/ReviewProvider.client.test.tsx index 859ef2f0..8559314b 100644 --- a/src/components/ReviewProvider.client.test.tsx +++ b/src/components/ReviewProvider.client.test.tsx @@ -190,6 +190,24 @@ describe('ReviewProvider client lifecycle', () => { expect(review.findings()).toEqual([]); }); + it('evicts an expanded-context annotation after its diff location disappears', () => { + const { review } = mountReview(); + review.completeDiffLoad('diff-a', renderedDiff()); + review.addAnnotation({ + id: 'expanded-context-annotation', + filePath: 'src/app.ts', + startLine: 50, + endLine: 50, + selectedText: 'expandedContext();', + comment: 'Review the expanded context line.', + }); + + review.beginDiffLoad(); + review.completeDiffLoad('diff-b', []); + + expect(review.annotations()).toEqual([]); + }); + it('ignores a provider response from a closed session after the same diff reopens', async () => { const requests: Array<{ context: QualityFindingLoadContext; diff --git a/src/lib/diff-review-lifecycle.test.ts b/src/lib/diff-review-lifecycle.test.ts index 77f50272..7ab3a0bf 100644 --- a/src/lib/diff-review-lifecycle.test.ts +++ b/src/lib/diff-review-lifecycle.test.ts @@ -105,6 +105,23 @@ describe('diff review lifecycle', () => { ).toEqual([]); }); + it('evicts an expanded-context annotation when its previous anchor cannot be reconstructed', () => { + const expandedContextAnnotation = { + ...annotation(), + startLine: 50, + endLine: 50, + selectedText: 'expandedContext();', + }; + + expect( + transitionReviewAnnotations( + [expandedContextAnnotation], + snapshot('diff-a', renderedDiff()), + snapshot('diff-b', []), + ), + ).toEqual([]); + }); + it('never carries durable comments into another worktree review', () => { expect( transitionReviewAnnotations( diff --git a/src/lib/diff-review-lifecycle.ts b/src/lib/diff-review-lifecycle.ts index 240f1944..dc2b3d02 100644 --- a/src/lib/diff-review-lifecycle.ts +++ b/src/lib/diff-review-lifecycle.ts @@ -81,7 +81,7 @@ export function transitionReviewAnnotations( const retained = annotations.filter((annotation) => { const previousAnchor = annotationContentAnchor(annotation, previous.files); - if (!previousAnchor) return true; + if (!previousAnchor) return false; const nextAnchor = annotationContentAnchor(annotation, next.files); return nextAnchor !== null && sameContentAnchor(previousAnchor, nextAnchor); });