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 676dd22e..2184481f 100644 --- a/src/components/DiffViewerDialog.tsx +++ b/src/components/DiffViewerDialog.tsx @@ -4,10 +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 { type QualityFindingProvider } from '../lib/quality-findings'; import { windowChromeTopInset } from '../lib/platform'; import { ScrollingDiffView } from './ScrollingDiffView'; import { @@ -50,6 +55,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. */ @@ -68,33 +75,43 @@ 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 ( - -

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

- - + +

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

+ - - - + + + ); } @@ -128,7 +145,7 @@ function DiffViewerContent(props: DiffViewerDialogProps) { 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; @@ -166,8 +183,20 @@ 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(''); setLoading(true); setError(''); @@ -204,18 +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)); + 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/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..b8535cec --- /dev/null +++ b/src/components/QualityFindingCard.tsx @@ -0,0 +1,92 @@ +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 QualityFindingCardProps { + finding: QualityFinding; + onDismiss: () => void; +} + +export function QualityFindingCard(props: QualityFindingCardProps) { + const color = () => qualityFindingSeverityColor(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/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.client.test.tsx b/src/components/ReviewProvider.client.test.tsx new file mode 100644 index 00000000..8559314b --- /dev/null +++ b/src/components/ReviewProvider.client.test.tsx @@ -0,0 +1,385 @@ +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 { sendPrompt } from '../store/tasks'; +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(); + vi.mocked(sendPrompt).mockReset(); +}); + +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, + onSubmitted?: () => void, +): 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( + () => ( + ''} + onSubmitted={onSubmitted} + > + + + ), + 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('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; + 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' }, + ]); + }); + + 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 { + 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 new file mode 100644 index 00000000..6a47773f --- /dev/null +++ b/src/components/ReviewProvider.test.ts @@ -0,0 +1,165 @@ +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'; + +vi.mock('../store/tasks', () => ({ + sendPrompt: vi.fn(), +})); + +function finding(overrides: Partial = {}): 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', + ...overrides, + }; +} + +function renderReviewProvider(onSubmitted = vi.fn()): ReviewContextValue { + let captured: ReviewContextValue | undefined; + + function CaptureContext() { + captured = useReview(); + return ''; + } + + renderToString(() => + createComponent(ReviewProvider, { + taskId: 'task-1', + agentId: 'agent-1', + reviewIdentity: 'task-1:/worktree', + 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; + vi.mocked(sendPrompt).mockImplementation( + () => + new Promise((resolve) => { + release = resolve; + }), + ); + 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 = review.submitReview(); + const duplicate = review.submitReview(); + + 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 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('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 review.submitReview(); + + expect(review.submitting()).toBe(false); + expect(review.submitError()).toBe('terminal unavailable'); + expect(review.sidebarOpen()).toBe(true); + 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(); + 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'); + review.clearSubmitError(); + + expect(review.submitError()).toBe(''); + }); +}); diff --git a/src/components/ReviewProvider.tsx b/src/components/ReviewProvider.tsx index 5f88e128..04c378c9 100644 --- a/src/components/ReviewProvider.tsx +++ b/src/components/ReviewProvider.tsx @@ -1,6 +1,30 @@ -import { createContext, createSignal, createEffect, useContext } from 'solid-js'; +import { + createContext, + createSignal, + createEffect, + createMemo, + onCleanup, + untrack, + useContext, +} from 'solid-js'; import type { JSX } from 'solid-js'; import { sendPrompt } from '../store/tasks'; +import { + compileQualityFindingPrompt, + dismissQualityFinding, + reconcileQualityFindingsForDiff, + resolveQualityFindings, + selectedFindingIdsAfterSubmission, + selectSubmittableFindings, + type QualityFinding, + type QualityFindingProvider, +} from '../lib/quality-findings'; +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'; /** Generic selection info used to create annotations or questions. */ @@ -22,6 +46,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,11 +63,26 @@ 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; + findingsLoading: () => boolean; + findingsError: () => string; + clearFindingsError: () => void; + + beginDiffLoad: () => void; + completeDiffLoad: (diffIdentity: string, files: FileDiff[]) => void; + suspendDiffLoad: () => void; + + scrollTarget: () => ReviewScrollTarget | null; + setScrollTarget: (target: ReviewScrollTarget | null) => void; submitReview: () => Promise; canSubmit: () => boolean; + submitting: () => boolean; pendingSelection: () => ContentSelection | null; handleSelection: (selection: ContentSelection) => void; @@ -48,11 +94,17 @@ export interface ReviewContextValue { dismissQuestion: (id: string) => void; submitError: () => string; + clearSubmitError: () => void; } 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; @@ -60,19 +112,115 @@ interface ReviewProviderProps { const ReviewContext = createContext(); +function createReviewSubmissionGuard() { + const [submitting, setSubmitting] = createSignal(false); + + async function run(action: () => Promise): Promise { + if (submitting()) return false; + setSubmitting(true); + try { + await action(); + return true; + } finally { + setSubmitting(false); + } + } + + return { submitting, run }; +} + +function canSubmitReview( + taskId: string | undefined, + agentId: string | undefined, + submitting: boolean, +): boolean { + return Boolean(taskId && agentId && !submitting); +} + 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 submission = createReviewSubmissionGuard(); + const openFindings = createMemo(() => findings().filter((finding) => finding.state === 'open')); + let findingLoadGeneration = 0; + 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++; + setFindingsLoading(false); + } + + function resetTransientState() { + setSelectedFindingIds(new Set()); + setSidebarOpen(false); + setScrollTarget(null); + setPendingSelection(null); + setActiveQuestions([]); + setSubmitError(''); + setFindingsError(''); + } + + function clearReviewState() { + reviewLifecycleGeneration++; + 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 annotations are added + // Auto-open sidebar when human comments or provider findings are added. createEffect(() => { - if (annotations().length > 0) setSidebarOpen(true); + 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 +239,149 @@ 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 sameReviewDiff( + left: ReviewDiffIdentity | null, + right: ReviewDiffIdentity | null, + ): boolean { + return Boolean( + left && + right && + left.reviewIdentity === right.reviewIdentity && + left.diffIdentity === right.diffIdentity, + ); + } + + function beginDiffLoad() { + reviewLifecycleGeneration++; + 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: ReviewDiffSnapshot = { + reviewIdentity: props.reviewIdentity ?? '', + diffIdentity, + files, + }; + const sameDiff = sameReviewDiff(activeReviewDiff, next); + + if (!sameDiff) reviewLifecycleGeneration++; + + setAnnotations((prev) => transitionReviewAnnotations(prev, activeReviewDiff, next)); + 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() { + reviewLifecycleGeneration++; + invalidateFindingLoad(); + setFindings((prev) => reconcileQualityFindingsForDiff(prev, [], false)); + resetTransientState(); + } + function handleSelection(selection: ContentSelection) { setPendingSelection(selection); } @@ -139,24 +430,54 @@ export function ReviewProvider(props: ReviewProviderProps) { } function canSubmit(): boolean { - return !!props.taskId && !!props.agentId; + return canSubmitReview(props.taskId, props.agentId, submission.submitting()); } async function submitReview(): Promise { const taskId = props.taskId; const agentId = props.agentId; if (!taskId || !agentId) return; + const submittedAnnotations = annotations(); + const submittedFindings = selectSubmittableFindings(findings(), selectedFindingIds()); + if (submittedAnnotations.length === 0 && submittedFindings.length === 0) return; - setSubmitError(''); - const prompt = props.compilePrompt(annotations()); - try { - await sendPrompt(taskId, agentId, prompt); - setAnnotations([]); - setSidebarOpen(false); - props.onSubmitted?.(); - } catch (err: unknown) { - setSubmitError(err instanceof Error ? err.message : 'Failed to send review'); - } + 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; + 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), + ); + const updatedFindings = resolveQualityFindings(untrack(findings), submittedFindings); + setAnnotations(remainingAnnotations); + setFindings(updatedFindings); + setSelectedFindingIds((previous) => + selectedFindingIdsAfterSubmission(previous, submittedFindings), + ); + const hasRemainingActionableReview = + remainingAnnotations.length > 0 || + updatedFindings.some( + (finding) => finding.state === 'open' && finding.freshness === 'current', + ); + setSidebarOpen(hasRemainingActionableReview); + if (!hasRemainingActionableReview) onSubmitted?.(); + } catch (err: unknown) { + if (submittedReviewLifecycle !== reviewLifecycleGeneration) return; + setSubmitError(err instanceof Error ? err.message : 'Failed to send review'); + setSidebarOpen(true); + } + }); } const value: ReviewContextValue = { @@ -165,6 +486,18 @@ export function ReviewProvider(props: ReviewProviderProps) { dismissAnnotation, updateAnnotation, replaceAnnotations, + findings, + openFindings, + selectedFindingIds, + setFindingSelected, + dismissFinding, + replaceFindings, + findingsLoading, + findingsError, + clearFindingsError: () => setFindingsError(''), + beginDiffLoad, + completeDiffLoad, + suspendDiffLoad, sidebarOpen, setSidebarOpen, scrollTarget, @@ -176,8 +509,10 @@ export function ReviewProvider(props: ReviewProviderProps) { activeQuestions, dismissQuestion, canSubmit, + submitting: submission.submitting, submitReview, submitError, + clearSubmitError: () => setSubmitError(''), }; return {props.children}; diff --git a/src/components/ReviewSidebar.test.ts b/src/components/ReviewSidebar.test.ts new file mode 100644 index 00000000..d93f3eac --- /dev/null +++ b/src/components/ReviewSidebar.test.ts @@ -0,0 +1,93 @@ +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', + 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[], + submission: { canSubmit: boolean; submitting: boolean } = { + canSubmit: true, + submitting: false, + }, +) { + 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: submission.canSubmit, + submitting: submission.submitting, + onDismiss: vi.fn(), + onUpdate: vi.fn(), + onScrollTo: vi.fn(), + onSubmit: vi.fn(), + onFindingSelected: vi.fn(), + onFindingDismiss: vi.fn(), + onFindingScrollTo: 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 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).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).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 5ec87704..53af8ad7 100644 --- a/src/components/ReviewSidebar.tsx +++ b/src/components/ReviewSidebar.tsx @@ -1,15 +1,23 @@ import { For, Show, createSignal } from 'solid-js'; import { theme } from '../lib/theme'; import { sf } from '../lib/fontScale'; +import type { QualityFinding } from '../lib/quality-findings'; import type { ReviewAnnotation } from './review-types'; +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; onSubmit: () => void; + onFindingSelected: (id: string, selected: boolean) => void; + onFindingDismiss: (id: string) => void; + onFindingScrollTo: (finding: QualityFinding) => void; } function truncate(text: string, max: number): string { @@ -157,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 (
- Review Comments ({props.annotations.length}) + Review ({props.annotations.length + props.findings.length})
{/* Scrollable list */} @@ -189,6 +206,43 @@ 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)} + /> + )} + + + 0}> +
0 ? '12px 0 6px' : '0 0 6px', + }} + > + Human comments ({props.annotations.length}) +
+ {(annotation) => ( diff --git a/src/components/ReviewSidebarPanel.tsx b/src/components/ReviewSidebarPanel.tsx index 640f4a4f..c2cec042 100644 --- a/src/components/ReviewSidebarPanel.tsx +++ b/src/components/ReviewSidebarPanel.tsx @@ -1,15 +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'; + +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 state = () => currentReviewSidebarState(review); 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 state = () => currentReviewSidebarState(review); return ( - 0}> +
+ {review.submitError()} + +
+ + +
+ Loading quality findings... +
+ + +
- {review.submitError()} + + Quality findings unavailable: {review.findingsError()} + +
+ review.setScrollTarget({ + id: finding.id, + filePath: finding.location.filePath, + startLine: finding.location.startLine, + endLine: finding.location.endLine, + }) + } />
diff --git a/src/components/ScrollingDiffView.tsx b/src/components/ScrollingDiffView.tsx index 00f9d6b1..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'; @@ -14,9 +14,15 @@ 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'; +import { + expandCollapsedFileForNavigation, + scheduleReviewNavigationHighlightClear, +} from './review-navigation'; interface ScrollingDiffViewProps { files: FileDiff[]; @@ -25,7 +31,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; } @@ -449,6 +455,8 @@ function FileSection(props: { worktreePath: string; baseBranch?: string; ref: (el: HTMLDivElement) => void; + collapsed: boolean; + onCollapsedChange: (collapsed: boolean) => void; dimmed: boolean; searchQuery?: string; activeQuestions: ActiveQuestion[]; @@ -456,12 +464,13 @@ function FileSection(props: { reviewAnnotations: ReviewAnnotation[]; onDismissAnnotation: (id: string) => void; onAnnotationUpdate: (id: string, comment: string) => void; + qualityFindings: QualityFinding[]; + onDismissFinding: (id: string) => void; highlightedRange?: HighlightRange | null; pendingInput?: { filePath: string; afterLine: number } | null; onSubmit: (text: string, mode: 'review' | 'ask') => void; onDismiss: () => void; }) { - const [collapsed, setCollapsed] = createSignal(false); const lang = () => detectLang(props.file.path); const added = () => props.file.hunks.reduce((s, h) => s + h.lines.filter((l) => l.type === 'add').length, 0); @@ -483,7 +492,7 @@ function FileSection(props: { > {/* Sticky file header */}
setCollapsed(!collapsed())} + onClick={() => props.onCollapsedChange(!props.collapsed)} style={{ position: 'sticky', top: '0', @@ -505,7 +514,7 @@ function FileSection(props: { 'font-size': sf(12), 'user-select': 'none', transition: 'transform 0.15s', - transform: collapsed() ? 'rotate(-90deg)' : 'rotate(0deg)', + transform: props.collapsed ? 'rotate(-90deg)' : 'rotate(0deg)', display: 'inline-block', }} > @@ -590,7 +599,7 @@ function FileSection(props: {
{/* File body */} - +
)} + finding.location.filePath, + (finding) => finding.location.startLine, + hunk.newStart, + nextStart, + )} + > + {(finding) => ( + props.onDismissFinding(finding.id)} + /> + )} + ); })()} @@ -749,28 +775,54 @@ function FileSection(props: { export function ScrollingDiffView(props: ScrollingDiffViewProps) { const review = useReview(); 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( + on( + () => props.files, + () => setCollapsedFiles(new Set()), + ), + ); + 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; }; - 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)); @@ -804,15 +856,31 @@ 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 el = containerRef?.querySelector( - `[data-file-path="${CSS.escape(target.filePath)}"][data-new-line="${target.startLine}"]`, - ); - if (el && containerRef) { - const containerTop = containerRef.getBoundingClientRect().top; - const elTop = el.getBoundingClientRect().top; - containerRef.scrollTop = elTop - containerTop + containerRef.scrollTop - 80; - } + const currentCollapsed = untrack(collapsedFiles); + const expanded = expandCollapsedFileForNavigation(currentCollapsed, target.filePath); + if (expanded !== currentCollapsed) setCollapsedFiles(expanded); + + navigationFrame = requestAnimationFrame(() => { + navigationFrame = undefined; + navigationLineFrame = requestAnimationFrame(() => { + navigationLineFrame = undefined; + const el = containerRef?.querySelector( + `[data-file-path="${CSS.escape(target.filePath)}"][data-new-line="${target.startLine}"]`, + ); + if (el && containerRef) { + const containerTop = containerRef.getBoundingClientRect().top; + const elTop = el.getBoundingClientRect().top; + containerRef.scrollTop = elTop - containerTop + containerRef.scrollTop - 80; + } + navigationHighlightTimer = scheduleReviewNavigationHighlightClear( + target, + () => untrack(() => props.scrollToAnnotation), + () => review.setScrollTarget(null), + ); + }); + }); }); onMount(() => { @@ -891,6 +959,15 @@ export function ScrollingDiffView(props: ScrollingDiffViewProps) { worktreePath={props.worktreePath} baseBranch={props.baseBranch} ref={(el) => sectionRefs.set(file.path, el)} + collapsed={collapsedFiles().has(file.path)} + onCollapsedChange={(collapsed) => + setCollapsedFiles((previous) => { + const next = new Set(previous); + if (collapsed) next.add(file.path); + else next.delete(file.path); + return next; + }) + } dimmed={dimOthers() && file.path !== props.scrollToPath} searchQuery={props.searchQuery} activeQuestions={review.activeQuestions()} @@ -898,6 +975,10 @@ export function ScrollingDiffView(props: ScrollingDiffViewProps) { reviewAnnotations={review.annotations()} onDismissAnnotation={review.dismissAnnotation} onAnnotationUpdate={review.updateAnnotation} + qualityFindings={review + .openFindings() + .filter((finding) => finding.freshness === 'current')} + onDismissFinding={review.dismissFinding} highlightedRange={highlightedRange()} pendingInput={(() => { const pi = review.pendingSelection(); diff --git a/src/components/TaskPanel.tsx b/src/components/TaskPanel.tsx index 7657fade..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; @@ -651,11 +652,12 @@ 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} 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/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); +} diff --git a/src/lib/diff-review-lifecycle.test.ts b/src/lib/diff-review-lifecycle.test.ts new file mode 100644 index 00000000..7ab3a0bf --- /dev/null +++ b/src/lib/diff-review-lifecycle.test.ts @@ -0,0 +1,155 @@ +import { describe, expect, it } from 'vitest'; +import type { ReviewAnnotation } from '../components/review-types'; +import { + createDiffIdentity, + createRequestGenerationGuard, + transitionReviewAnnotations, + type ReviewDiffSnapshot, +} 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 renderedDiff(content = 'before();'): FileDiff[] { + return [ + { + path: 'src/app.ts', + status: 'M', + binary: false, + hunks: [ + { + oldStart: 10, + oldCount: 1, + newStart: 10, + newCount: 1, + lines: [ + { type: 'remove', content: 'base();', oldLine: 10, newLine: null }, + { type: 'add', content, oldLine: null, newLine: 10 }, + ], + }, + ], + }, + ]; +} + +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 = snapshot('diff-a', renderedDiff()); + + expect(transitionReviewAnnotations(annotations, identity, identity)).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 an annotation when its anchored range disappears from the diff', () => { + expect( + transitionReviewAnnotations( + [annotation()], + snapshot('diff-a', renderedDiff()), + snapshot('diff-b', [unrelatedDiff()]), + ), + ).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( + [annotation()], + snapshot('same-diff', renderedDiff(), 'worktree-a'), + snapshot('same-diff', renderedDiff(), 'worktree-b'), + ), + ).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..dc2b3d02 --- /dev/null +++ b/src/lib/diff-review-lifecycle.ts @@ -0,0 +1,89 @@ +import type { ReviewAnnotation } from '../components/review-types'; +import type { FileDiff } from './unified-diff-parser'; + +export interface ReviewDiffIdentity { + reviewIdentity: string; + diffIdentity: string; +} + +export interface ReviewDiffSnapshot extends ReviewDiffIdentity { + files: FileDiff[]; +} + +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(''); +} + +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: ReviewDiffSnapshot | null, + next: ReviewDiffSnapshot, +): ReviewAnnotation[] { + if (!previous) return annotations; + if (previous.reviewIdentity !== next.reviewIdentity) return []; + if (previous.diffIdentity === next.diffIdentity) return annotations; + + const retained = annotations.filter((annotation) => { + const previousAnchor = annotationContentAnchor(annotation, previous.files); + if (!previousAnchor) return false; + const nextAnchor = annotationContentAnchor(annotation, next.files); + return nextAnchor !== null && sameContentAnchor(previousAnchor, nextAnchor); + }); + return retained.length === annotations.length ? annotations : retained; +} diff --git a/src/lib/quality-findings.test.ts b/src/lib/quality-findings.test.ts new file mode 100644 index 00000000..409e0a4a --- /dev/null +++ b/src/lib/quality-findings.test.ts @@ -0,0 +1,258 @@ +import { describe, expect, it } from 'vitest'; +import type { FileDiff } from './unified-diff-parser'; +import { + compileQualityFindingPrompt, + createFixtureQualityFindingProvider, + dismissQualityFinding, + reconcileQualityFindings, + reconcileQualityFindingsForDiff, + resolveQualityFindings, + selectedFindingIdsAfterSubmission, + selectSubmittableFindings, + type QualityFinding, +} from './quality-findings'; + +function finding(overrides: Partial = {}): QualityFinding { + return { + id: 'finding-1', + 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 context = { reviewIdentity: 'task-1', diffIdentity: 'diff-1', files: [diff()] }; + + const first = await provider.loadFindings(context); + first[0].location.startLine = 99; + const second = await provider.loadFindings(context); + + 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'); + }); + + it('keeps provider-first findings pending until the diff loads', () => { + const result = reconcileQualityFindingsForDiff([finding()], [], false); + + expect(result[0].freshness).toBe('pending'); + expect(selectSubmittableFindings(result, ['finding-1'])).toEqual([]); + }); + + it('returns current findings to pending during commit navigation', () => { + const current = reconcileQualityFindingsForDiff([finding()], [diff()], true); + const navigating = reconcileQualityFindingsForDiff(current, [], false); + + expect(current[0].freshness).toBe('current'); + expect(navigating[0].freshness).toBe('pending'); + }); + + it('keeps findings pending after rejected diff loading', () => { + const loading = reconcileQualityFindingsForDiff([finding()], [], false); + const rejected = reconcileQualityFindingsForDiff(loading, [], false); + + expect(rejected).toBe(loading); + 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 }, + }); + + expect(reconcileQualityFindings([ranged], [diff()])[0].freshness).toBe('stale'); + expect( + reconcileQualityFindings( + [ranged], + [ + diff({ + hunks: [ + { + oldStart: 8, + oldCount: 1, + newStart: 8, + newCount: 1, + lines: [{ type: 'add', content: 'runAsync();', oldLine: null, newLine: 8 }], + }, + ], + }), + ], + )[0].freshness, + ).toBe('current'); + }); +}); + +describe('compileQualityFindingPrompt', () => { + it('includes structured remediation fields for one or multiple findings', () => { + const prompt = compileQualityFindingPrompt([ + finding(), + finding({ + id: 'finding-2', + 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).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 the stable provider ID', () => { + const original = finding(); + const dismissed = dismissQualityFinding([original], original.id); + + expect(dismissed[0]).toMatchObject({ + id: original.id, + 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', freshness: 'stale' }); + const resolved = finding({ id: 'resolved', state: 'resolved' }); + + expect( + selectSubmittableFindings([current, stale, resolved], ['finding-1', 'stale', 'resolved']), + ).toEqual([current]); + }); + + it('preserves unrelated selections after a single-card submission', () => { + const remaining = selectedFindingIdsAfterSubmission(new Set(['finding-b', 'finding-c']), [ + finding(), + ]); + + expect([...remaining]).toEqual(['finding-b', 'finding-c']); + }); + + 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' })], + ); + + expect([...remaining]).toEqual(['finding-c']); + }); +}); diff --git a/src/lib/quality-findings.ts b/src/lib/quality-findings.ts new file mode 100644 index 00000000..15c87878 --- /dev/null +++ b/src/lib/quality-findings.ts @@ -0,0 +1,171 @@ +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 = 'pending' | 'current' | 'stale'; + +export interface QualityFindingLocation { + filePath: string; + startLine: number; + startColumn?: number; + endLine?: number; + endColumn?: number; +} + +export interface QualityFinding { + /** Stable provider-defined identifier or fingerprint. */ + id: string; + source: string; + ruleId: string; + category: QualityFindingCategory; + severity: QualityFindingSeverity; + location: QualityFindingLocation; + explanation: string; + state: QualityFindingState; + 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(context: QualityFindingLoadContext): 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(_context) { + 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; + + return file.hunks.some((hunk) => + hunk.lines.some((line) => line.newLine === finding.location.startLine), + ); +} + +export function reconcileQualityFindingsForDiff( + findings: QualityFinding[], + files: FileDiff[], + diffLoaded: boolean, + findingsDiffIdentity?: string, + currentDiffIdentity?: string, +): QualityFinding[] { + const identityMatches = + findingsDiffIdentity === undefined && currentDiffIdentity === undefined + ? true + : findingsDiffIdentity === currentDiffIdentity; + if (diffLoaded && identityMatches) return reconcileQualityFindings(findings, files); + + let changed = false; + const pending = findings.map((finding) => { + if (finding.freshness === 'pending') return finding; + changed = true; + return { ...finding, freshness: 'pending' as const }; + }); + return changed ? pending : findings; +} + +/** 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 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, +): QualityFinding[] { + const requested = new Set(ids); + return findings.filter( + (finding) => + requested.has(finding.id) && finding.state === 'open' && finding.freshness === 'current', + ); +} + +export function selectedFindingIdsAfterSubmission( + selectedIds: ReadonlySet, + submittedFindings: QualityFinding[], +): ReadonlySet { + const submittedIds = new Set(submittedFindings.map((finding) => finding.id)); + const remaining = new Set([...selectedIds].filter((id) => !submittedIds.has(id))); + return remaining.size === selectedIds.size ? selectedIds : remaining; +} + +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.startLine}:${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(finding.explanation); + lines.push(''); + } + return lines.join('\n'); +} 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; +} 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'], + }, +});