Model structured code-quality findings in diff review

[LarryHu0217]

Summary

• define a provider-neutral QualityFinding contract, fixture provider, and deterministic stale-location reconciliation
• render current findings inline and all open findings in the existing review sidebar with textual severity/category metadata
• support dismiss, single-send, and multi-select structured feedback to the selected task agent

Validation

• npm run check
• npm run lint:dead
• npm run lint:arch
• npm test (1636 passed, 23 skipped)
• npm run build:frontend

Closes #239

[johannesjo]

Review feedback

I rechecked the complete change at d61c90b against #239, its discussion, the exact-head tests, and the current CI state. The provider-neutral seam and selected-agent routing look appropriate, and CI is green, but two correctness issues still need attention.

Important

• src/components/DiffViewerDialog.tsx:165 — Reconciliation returns early while diffLoaded() is false, but provider results are installed with their supplied freshness and finding actions remain gated only by task/agent availability. If the provider resolves before the diff, during commit navigation, or when the diff request fails, a finding marked current can remain navigable and sendable before it has been validated against the selected diff—or indefinitely after failure. Keep findings pending/non-actionable whenever the current diff generation is not successfully loaded, then enable them only after reconciliation. Please cover provider-first loading, commit navigation, and rejected diff loading.

• src/lib/quality-findings.ts:59 — A ranged finding is considered current when any rendered line falls within startLine…endLine, while navigation later queries only the exact startLine (ScrollingDiffView.tsx:842). For example, a finding spanning lines 8–10 is enabled when only lines 9–11 are rendered, but clicking it searches for line 8 and silently does nothing. Use one shared navigable anchor: either require startLine itself to be rendered or resolve and retain the first rendered line in the range. Add a ranged-location navigation test.

Minor

• src/components/ReviewProvider.tsx:262 — Every successful submission clears the entire selected-ID set, including single-card sends made with [id]. If B and C are selected and A is sent individually, B and C are silently deselected even though they were not submitted. Remove only the successfully submitted IDs; reserve clearing the whole set for the bulk-send path.

Verdict: Needs changes.

[LarryHu0217]

Addressed the review findings in be02a9c:

• findings remain pending and non-actionable until the current diff generation loads successfully
• commit navigation and failed diff loads return/keep findings pending
• ranged findings require their navigable startLine to be rendered
• single-card sends remove only submitted IDs; bulk sends clear the selection

Added lifecycle, ranged-navigation, selection, and pending-sidebar regression tests. Local check:static, security-rule tests, and the full suite pass (1643 passed, 23 skipped).

[johannesjo]

Review feedback

I rechecked the complete change at be02a9c3 against #239, the prior review response, the exact-head tests, and current CI. The previously reported pending-diff, ranged-anchor, single-card selection, and selected-agent issues are fixed. The provider-neutral split is proportionate; a KISS/YAGNI pass did not find an architectural rewrite warranted. Four current interaction defects remain.

Important

• src/components/ReviewProvider.tsx:252 — The new single- and bulk-finding Send controls have no shared in-flight guard, either with each other or with human-comment submission. Rapid clicks can run sendPrompt concurrently. Because sendPrompt writes focus and prompt text, waits 50–500 ms, and only then writes Enter, two calls can concatenate prompts in the selected agent terminal before either submission completes. The smallest fix is one provider-level submitting guard shared by both submission methods, with an early return and all Send controls disabled until finally; no queue abstraction is needed. Please add a concurrent-click regression test.

• src/components/ReviewSidebarPanel.tsx:92 — Finding navigation silently does nothing after its file section is collapsed. Collapsing unmounts the line nodes at ScrollingDiffView.tsx:599, while the navigation effect at line 841 only queries an already-mounted startLine. This violates #239's navigation acceptance criterion in a normal viewer state. Expand the matching file first, then query/scroll after the line remounts, and cover that state with a regression test.

• src/components/ReviewProvider.tsx:266 — An inline finding Send can be initiated while the sidebar is closed, but failure only sets submitError; its sole banner is behind sidebarOpen() and hasReviewState() in ReviewSidebarPanel. The action therefore fails with no visible feedback. Reopen the sidebar on failure and include submitError in the panel/button state so the error remains reachable even if review items change while the request is pending.

Minor

• src/lib/quality-findings.ts:115 — Bulk completion clears the selection as it exists after the awaited send. If A/B are being sent and C is selected during that interval, C is silently deselected although it was not submitted. The simpler behavior is also the correct one: always subtract the snapshotted submitted IDs from the latest selection and remove the bulkSubmission special case.

Exact-head verification passed the 18 focused tests, the full suite (1,643 passing), typecheck, ESLint/Knip/dependency-cruiser, Prettier, frontend and remote builds, Electron compilation, and MCP bundling. Both required GitHub checks are green.

Verdict: Needs changes.

[LarryHu0217]

Addressed all four follow-up findings in 0b483ad:

• Added one provider-level submission guard shared by human, single-finding, and bulk-finding sends; all Send controls are disabled while it is active.
• Finding navigation now expands a collapsed target file before querying and scrolling to its remounted line.
• Submission failures reopen the sidebar, and submitError keeps both the panel and Review button reachable.
• Successful sends subtract only the snapshotted submitted finding IDs from the latest selection.

Validation: 1,649 tests passed; typecheck, ESLint, Knip, dependency-cruiser, security-rule tests, frontend/remote builds, Electron compilation, and MCP bundling passed.

[johannesjo]

Review feedback

Rechecked the complete change at 0b483ad against #239, both earlier rounds, and the current head. Every previously reported finding is genuinely fixed — pending-diff gating, the ranged-anchor mismatch, single-card selection, selected-agent routing, the shared submission guard, collapsed-file expansion, error reachability, and the bulk-selection snapshot. tsc --noEmit is clean locally and both required checks are green.

Two correctness issues remain, plus a KISS/YAGNI pass with concrete deletions.

Premise

Nothing in the running app can produce a finding. findingProvider has no production call site — all three DiffViewerDialog renderers omit it (TaskPanel.tsx:644, arena/BattleScreen.tsx:151, arena/ResultsScreen.tsx:418) — and createFixtureQualityFindingProvider is referenced only from its own test. The ~1,000 lines of UI therefore land dark: no inline card, sidebar row, checkbox, or Send button is reachable, and #239's first acceptance criterion ("at least one fixture provider can supply findings to the diff viewer") holds in unit tests only.

This does not need a redesign. One line at a single call site behind a dev-only flag satisfies the criterion and makes everything below verifiable by hand. The alternative is to land src/lib/quality-findings.ts plus its tests now and hold the UI for #238, where a real provider exists. Either works; shipping an interactive surface with no reachable data source is what I'd rather avoid.

Important

• ScrollingDiffView.tsx:793 — highlightedRange() now falls back to props.scrollToAnnotation, but scrollTarget is never cleared: there is no setScrollTarget(null) anywhere in src/, and the signal is declared { equals: false }. Clicking any sidebar row therefore paints its lines with CURRENT_MATCH_BG — the active-search-match colour defined at line 111 — for the remainder of the dialog session, with no way to dismiss it. Two consequences: after clicking a finding and then searching, two separate ranges render as "the current match"; and it regresses existing human-comment navigation, which previously only scrolled, since the old accessor derived its range from pendingSelection alone. Clear the target once the scroll lands, or time-box the highlight the way dimOthers already does at line 815.

• ReviewProvider.tsx:282 — the two send paths complete asymmetrically. submitReview calls onSubmitted?.(), wired to props.onClose at DiffViewerDialog.tsx:102; submitFindings signals nothing. Sending a finding from the inline card therefore produces no observable change whatsoever — the card stays, counts are unchanged, the dialog stays open, and the target terminal may not be visible — so the user cannot tell it worked and may click again. Conversely, "Send human comments" closes the dialog, unmounts ReviewProvider, and silently discards loaded findings and their selection. QualityFindingState already declares 'resolved' and nothing anywhere produces it; marking submitted findings resolved fixes the first half and gives that union member a purpose.

KISS / YAGNI

• 'resolved' is dead — quality-findings.ts:5 is the only occurrence outside a test. Use it (above) or drop it from the union.
• fingerprint is a second identity field that identifies nothing — declared at line 18 and printed into the prompt at line 138; reconciliation, selection, and dismissal all key off id. #239 asks for "stable ID/fingerprint" as one concept. Either key reconciliation off it or drop it.
• severityColor is byte-identical in two files — QualityFindingCard.tsx:17 and ReviewSidebar.tsx:31.
• Three send paths where one would do — per-card Send, "Send selected findings", and "Send human comments" produce two separate agent prompts that the shared guard now forces the user to issue sequentially. A single "Send to agent (N)" covering selected findings plus comments removes one button, the double prompt, and the asymmetric close above.
• quality-findings.ts:130 — the endColumn-only branch of formatQualityFindingLocation renders src/app.ts:10:3-7, ambiguous against the ranged form src/util.ts:4:2-8:7 where the trailing number is a column, not a line. It is untested and no provider emits columns yet.
• ReviewSidebar.tsx is now 471 lines (was ~213). QualityFindingSidebarItem is a natural extraction, mirroring how QualityFindingCard already got its own file.

Minor

• quality-findings.ts:51 vs ScrollingDiffView.tsx:731 — the shared-anchor fix from round 1 did not reach inline card placement: reconciliation and navigation key off startLine, but itemsForHunk anchors the card at endLine ?? startLine. A finding spanning lines 10–30, with hunk A covering 5–15 and hunk B starting at 28, is current via line 10 and scrolls to line 10, but its card renders under hunk B. This matches the human-annotation convention on line 717, which is why it is minor — but human annotations come from a selection inside one hunk, and provider ranges have no such constraint.

• quality-findings.ts:51 — mirror of the same mismatch: findingMatchesDiff scans only file.hunks, but ContextGapView renders expanded gap lines through the same DiffLineView (line 443), so they carry data-file-path/data-new-line and are perfectly navigable. A finding on such a line is marked stale and disabled even though clicking it would work. It fails closed, unlike the round-1 bug, but it is the same root cause.

• ReviewSidebarPanel.tsx:7 — findingsError is set on provider rejection and only ever cleared when the findingProvider prop changes, and it now keeps hasReviewSidebarState true. With no annotations that leaves a permanent, undismissable Review (0) button over an empty sidebar for the whole dialog session. Also: four positional parameters, two of them adjacent strings that are easy to transpose, and the reviewCount/hasReviewState pair is duplicated verbatim in both components of the same file.

• Test shape — all three new component tests target helpers that were made export solely so a test could import them (expandCollapsedFileForNavigation, createReviewSubmissionGuard, hasReviewSidebarState; each is used only inside its own module). The behaviour the last round actually asked for is still uncovered: nothing exercises "navigating to a finding in a collapsed file scrolls after the remount" — the double-requestAnimationFrame path at ScrollingDiffView.tsx:850 — or "two rapid Send clicks yield one sendPrompt". Separately, ReviewSidebar.test.ts:56 asserts expect(html).toContain('disabled') against the entire document, which passes if any element anywhere is disabled and would not catch the Send button losing its guard.

• QualityFindingCard.tsx:107, ReviewSidebar.tsx:442 — canSubmit() now folds in submitting, so during an in-flight send every Send control's tooltip reads "No agent available". Distinguish "sending…" from "no agent".

• ScrollingDiffView.tsx:788 — collapse state moved from a per-FileSection signal to a path-keyed set in the parent and is not reset when props.files changes, so a file collapsed before commit navigation stays collapsed after. Probably an improvement, but it is an unmentioned behaviour change.

What's good

The reconciliation helpers are pure and reference-stable — the changed flag returning the original array is what lets the read-write effect at DiffViewerDialog.tsx:167 converge cleanly, and it is easy to get that wrong. The pending state plus the diffLoaded gate is the right shape for the round-1 problem. Severity, category, and freshness are all conveyed as text alongside colour and the checkbox carries a proper aria-label, which satisfies #239's accessibility criterion.

Verdict: Needs changes — the sticky highlight and the missing send feedback are the two code blockers; the premise question above decides how much of the rest is worth doing in this PR.

[LarryHu0217]

Addressed the latest review in b22e2f0:

• added an opt-in dev fixture at the real TaskPanel diff-viewer call site; it can be exercised with VITE_QUALITY_FINDING_FIXTURE=true VITE_QUALITY_FINDING_FIXTURE_PATH=src/components/TaskPanel.tsx VITE_QUALITY_FINDING_FIXTURE_LINE=660 npm run dev
• clear the review navigation target after the scroll attempt, reset collapsed-file state when the diff changes, and anchor inline findings consistently at startLine
• replaced the three send controls with one combined submission for selected findings and human comments; successful findings become resolved, and rapid duplicate submissions remain guarded
• removed the redundant fingerprint, shared the severity-color mapping, extracted the sidebar finding item, made provider errors dismissible, clarified the in-flight label, and made same-line column ranges explicit
• replaced helper-only submission tests with direct ReviewProvider behavior coverage

Validation: npm run check, npm run check:static, frontend build, and the full suite pass (1,648 passed, 23 skipped).

[johannesjo]

Review feedback

Rechecked the complete change at b22e2f0 against #239 and all three earlier rounds, with each finding below independently verified by a separate adversarial pass.

Verification at head: tsc --noEmit, eslint --max-warnings 0, knip all clean; full suite 1,644 passed / 25 skipped. Both required checks green.

Every round-3 item is genuinely addressed: the sticky highlight is cleared, fingerprint is gone, severityColor is shared via quality-finding-colors.ts, the three send paths are one, QualityFindingSidebarItem is extracted (ReviewSidebar.tsx back to 289 lines), the inline card anchors at startLine, the endColumn-only format is unambiguous and tested, and the dev fixture gives findingProvider a real call site. ReviewProvider.test.ts replacing the helper-only tests is exactly the right move — it fires submitReview() twice without awaiting and asserts one sendPrompt, which is the concurrency test round 2 asked for.

Two new correctness issues, both introduced by this round's changes.

Important

• ReviewProvider.tsx:123 — dropping { equals: false } from scrollTarget breaks repeat navigation in the plan viewer. The compensating setScrollTarget(null) exists only at ScrollingDiffView.tsx:868, and PlanViewerDialog never renders ScrollingDiffView — it only reads the target at PlanViewerDialog.tsx:126-133 and scrolls. ReviewSidebar passes the annotation object itself (ReviewSidebarPanel.tsx:133 → ReviewSidebar.tsx:252), and those objects are reference-stable unless the comment is edited. So: click annotation A → scrolls; scroll away; click A again → the signal already holds A, === suppresses the write, no scroll — for the rest of the dialog session.

  Measured against the installed solid-js 1.9.11 client build, effect replicating PlanViewerDialog.tsx:126-133, flushing between sets, sequence A, A, B, B, null, B: default equality → 3 effect runs; { equals: false } → 5. dist/dev.js:689 (if (!node.comparator || !node.comparator(current, value))) is the mechanism. The { equals: false } on main was load-bearing.

  Findings are unaffected — ReviewSidebarPanel.tsx:138 builds a fresh object per click. Restoring { equals: false } is the one-line fix and is compatible with the existing clear (the effect early-returns on null).

• ReviewProvider.tsx:265-267 — submitError can reach a state that cannot be cleared for the life of the dialog. setSubmitError('') lives at line 279, after the new empty-submission early return at line 267. Reachable sequence:

  1. One annotation (or one selected finding); Send; sendPrompt rejects → submitError set, sidebar force-opened (lines 292-293).
  2. Dismiss that annotation/finding. reviewCount → 0, but hasReviewSidebarState stays true on submitError alone (ReviewSidebarPanel.tsx:15-22).
  3. Permanent Review (0) button (line 52) over an empty sidebar with a stale error banner (lines 66-78, no dismiss control). Send is disabled because canSend() needs submissionCount() > 0 (ReviewSidebar.tsx:169), and clicking it would hit the line-267 return anyway.

  This is the exact shape you flagged for findingsError last round — that one got a dismiss button at lines 106-122; submitError did not. It is also a regression: on main both the button and the panel were gated on annotations().length > 0, so a stale submitError was simply invisible, and setSubmitError('') ran unconditionally with no early return. Same dead end is reachable without dismissing anything — fail a submit, then navigate commits: DiffViewerDialog.tsx:187 forces every finding pending, the pruning effect empties the selection, and evictStaleAnnotations can drop the annotations. Either move setSubmitError('') above the early return, or add a clearSubmitError control alongside the one findingsError already has.

Minor

• ScrollingDiffView.tsx:794-811 — the scrollToAnnotation highlight fallback is now self-cancelling. It was added in 0b483ad; the setScrollTarget(null) added one commit later in b22e2f0 removes it before the next paint. rAF callbacks registered inside a rAF callback run in the following frame, so the highlight paints for exactly one frame — and that frame is the pre-scroll viewport, so the highlighted line is usually still off-screen. The clear also sits outside the if (el && containerRef) guard, so it fires even when the element isn't found. Worth noting the fix isn't "delete the fallback": the clear is what makes repeat-click navigation work at all now, so if the highlight is wanted it has to be deferred (a timer, as dimOthers does at line 819) rather than tied to the scroll. If it isn't wanted, dropping the fallback restores main exactly.

• Selected findings are silently wiped by commit navigation. DiffViewerDialog.tsx:187 sets diffLoaded(false) → everything goes pending → the pruning effect at ReviewProvider.tsx:164-174 empties selectedFindingIds. When the diff reloads and the findings return to current, the selection is not restored. Select five findings, step one commit back to check context, and all five are deselected with no feedback. The pending gate itself is right; only the discard is.

• Two test files were deleted rather than upgraded. ScrollingDiffView.test.ts (added in 0b483ad, gone in b22e2f0) and ReviewSidebarPanel.test.ts — the latter asserted hasReviewSidebarState(..., 'terminal unavailable') === true, i.e. precisely the behaviour that is now broken above. Both helpers were un-exported in the same commit. Fair enough that helper-only tests were the wrong shape, but the replacement covers ReviewProvider only: the expandCollapsedFileForNavigation → double-rAF → query path at lines 851-871 — which is where both of this round's regressions live — now has zero coverage. Grepping collapsedFiles|scrollToAnnotation|setScrollTarget across src/**/*.test.ts returns nothing.

• Three counts that can disagree. Header Review (annotations + openFindings) (ReviewSidebarPanel.tsx:26), sidebar heading the same (ReviewSidebar.tsx:198), Send button annotations + selectedFindingIds.size (line 168). Three stale findings and no comments reads Review (3) over a disabled Send to agent (0).

• ScrollingDiffView.tsx:789-792 — this collapse reset can't currently observe a change: both setParsedFiles calls happen while loading() is true, so the <Show> at line 495 has already disposed the component and collapsedFiles is recreated on remount. It isn't pointless, though — lifting collapse state out of FileSection removed the automatic reset main got for free, so this restores it, and it only looks redundant because of a non-local invariant in DiffViewerDialog. A comment naming that invariant would be worth more than deleting it. Separately, return props.files as the dependency declaration is obscure — it tracks only because reading the prop subscribes; the return value is createEffect's previous-value channel and is discarded.

• quality-findings.ts:55-57 — carry-over, and on reflection milder than I put it last round. findingMatchesDiff scanning only file.hunks means a finding on an expanded ContextGapView line is labelled Stale. Gap lines are never merged into parsedFiles(), so the marking is at least deterministic and never flips on expansion, and scoping a diff review to changed lines is defensible. It's the word Stale that's wrong for "outside the diff" — leave the behaviour, reconsider the label.

KISS / YAGNI

resolveQualityFindings, selectedFindingIdsAfterSubmission, and 'resolved' never affect anything a user sees. On success, ReviewProvider.tsx:285-290 applies them and then calls onSubmitted?.() in the same synchronous block — wired to props.onClose at DiffViewerDialog.tsx:103, which unmounts ReviewProvider through the <Show> at line 97. No paint can occur in between, and 'resolved' has zero readers anywhere — both consumers filter on state === 'open', so it's indistinguishable from 'dismissed'. To be clear, close-on-send is pre-existing behaviour from main, not something this PR introduced; the point is only that the new bookkeeping layered on top of it can never be observed.

selectedFindingIdsAfterSubmission is redundant twice over: the pruning effect at ReviewProvider.tsx:164-174 flushes synchronously inside the setFindings on the preceding line and has already emptied the set by the time line 286 runs.

Simplest resolution for this PR: delete all three and drop 'resolved' from the union. The alternative — making the close conditional on nothing reviewable remaining — is the better end state but belongs with #238, where a real provider makes "send two of five findings, keep reviewing" an actual workflow. Worth flagging for that issue: because the provider reloads on every open and the fixture returns state: 'open' clones, there is no dedupe — the same finding can be resent indefinitely.

What's good

createReviewSubmissionGuard is the minimal correct answer to the concurrency finding — one signal, early return, finally release, no queue abstraction — and the test that proves it fires both submissions without awaiting, which is the only way that test is worth anything. Collapsing three Send controls into Send to agent (N) removed a class of problems rather than patching them, and it took the double prompt with it. The quality-findings.ts helpers staying pure and reference-stable is what keeps the read-write reconciliation effect at DiffViewerDialog.tsx:167 converging. Anchoring the inline card at startLine now genuinely unifies all three code paths — reconciliation, navigation, and placement — on one line number.

Verdict: Needs changes — the plan-viewer navigation regression and the unclearable submitError are the two blockers; both are small fixes. Everything else is discretionary, though the missing coverage over ScrollingDiffView's navigation path is what let both of them through.

[LarryHu0217]

Addressed the two blocking regressions in f7942ee:

• restored forced updates for scroll targets, so repeat navigation to the same annotation still notifies the plan viewer
• clear a previous submission error before the empty-submission return, with a regression test for clearing the final annotation after a failed send

Validation: targeted provider tests, static checks, and the full suite pass (1,649 passed, 23 skipped).

[johannesjo]

Review feedback

Rechecked the complete change at f7942ee against #239 and all four earlier rounds. Every finding below was independently re-verified by a separate adversarial pass, each instructed to refute rather than confirm. Where the question was reactive behaviour I measured it against the real solid-js client build rather than reasoning about it — the repo's test config runs Solid in SSR mode, where createEffect is a no-op, so vitest cannot observe any of this.

Verification at head: tsc --noEmit and eslint --max-warnings 0 clean; full suite 1,645 passed / 25 skipped. Both required checks green.

Round-4 blocker 1 is genuinely fixed. Restoring { equals: false } on scrollTarget is the right one-line answer, and I confirmed the mechanism rather than taking it on trust. Modelling the plan-viewer effect (PlanViewerDialog.tsx:126-133) with reference-stable annotation objects and the click sequence A, A, B, B, null, B: default equality → 4 effect runs / 3 scrolls; { equals: false } → 6 runs / 5 scrolls. Two of five clicks were silently swallowed. Modelling the diff viewer gives identical scroll output under both variants, because the setScrollTarget(null) at ScrollingDiffView.tsx:868 supplies the intervening identity change — which is exactly why the regression was only ever visible in the plan viewer.

I also checked the pathologies the restore could plausibly introduce, and all came back negative: the null clear does not re-fire unboundedly (200 repeat clicks → exactly 2.00 effect runs per click, 200 clears, queue always settles, zero leftover drains); the props.scrollToAnnotation === target identity guard is unaffected by the equals option and still clears exactly once under a rapid double-set; and highlightedRange() recomputing more often produces byte-identical DOM style writes, since Solid diffs style properties individually. No new bug is introduced by f7942ee.

Important

• ReviewProvider.tsx:267 — moving setSubmitError('') above the empty-submission early return does not reach the dead end it was meant to fix. In the state that produces the dead end, the only control that can invoke submitReview() is disabled.

  setSubmitError('') has exactly one caller, and submitReview has exactly one UI entry point: ReviewSidebarPanel.tsx:134 → the Send button at ReviewSidebar.tsx:265-284, disabled={!canSend()} where canSend = canSubmit && submissionCount() > 0. There is no keyboard path — the only keydown handler in the dialog (DiffViewerDialog.tsx:151-161) is Ctrl/Cmd+F, and nothing in src/remote/ or electron/ touches it.

  Driving the real provider through fail-then-dismiss lands on {annotations: 0, openFindings: 0, selectedFindingIds: 0, canSubmit: true, submitting: false, submitError: 'terminal unavailable', sidebarOpen: true}. Rendering ReviewSidebar with exactly those props emits aria-label="Send review to agent" disabled ... title="Select at least one finding">Send to agent (0). Rendering the real ReviewCommentsButton + ReviewSidebarPanel in that state gives Review (0), the terminal unavailable banner, and no <button> inside the banner — versus findingsError, which got its dismiss control at ReviewSidebarPanel.tsx:106-122.

  The stronger form: the guard on that clear went from (task && agent) && (something submittable) && !submitting to (task && agent). So the delta is observable only when nothing is submittable (Send disabled) or a submission is already in flight (Send disabled). The commit changes nothing a user can trigger. ReviewProvider.test.ts:135 passes only because it calls submitReview() programmatically in precisely the state where the button is disabled.

  One thing my last round understated: reaching the dead end needs no user action at all. DiffViewerDialog.tsx:227 runs evictStaleAnnotations on every refetch, so a failed send followed by the agent pushing a commit can drop the last annotation on its own. Commit navigation does not remount the provider (selectedCommit changes, the <Show> condition at DiffViewerDialog.tsx:97 stays true), so "for the life of the dialog" is accurate.

  Minimal fix, mirroring the pattern already in the file: add clearSubmitError: () => setSubmitError('') to ReviewContextValue and the value object, and give the banner at ReviewSidebarPanel.tsx:66-78 the same close button findingsError has. Keeping the line-267 move alongside it is harmless.

Minor — carry-over, all still open at f7942ee

• ScrollingDiffView.tsx:803-810 — I need to soften my own wording from last round: the scrollToAnnotation highlight fallback is not quite never painted. The highlight applies synchronously when the sidebar sets the target, so rAF#1's frame does paint it — for exactly one frame, at the pre-scroll scroll position; rAF#2 scrolls and clears in the same callback, so the post-scroll paint never carries it. Net user-visible value is still ≈ zero. The out-of-guard placement of the clear at line 868 has a sharper consequence than I gave it credit for: it sits outside if (el && containerRef), so when the target line isn't in the DOM the target is dropped with no scroll, no error, and no retry.
• DiffViewerDialog.tsx:187 — commit navigation still discards the finding selection, now measured end-to-end against the real client build:

  after load    freshness=current×5  selected=[f0,f1,f2,f3,f4]
  during fetch  freshness=pending×5  selected=[]
  after reload  freshness=current×5  selected=[]   restored=false
  

• Counts that can disagree — correcting my last round: there are two distinct formulas, not three. ReviewSidebarPanel.tsx:26 and ReviewSidebar.tsx:198 are the same expression and can never disagree with each other; the odd one out is ReviewSidebar.tsx:168. Rendered with 3 stale findings and 0 comments: Review (3) … Automated findings (3) … <button disabled title="Select at least one finding">Send to agent (0)</button>.
• quality-findings.ts:55-57 — the Stale label for "outside the diff" is more commonly wrong than I assumed. Gap lines render through the same DiffLineView and carry data-file-path/data-new-line, so the line-861 query would match them, and gaps of ≤5 lines auto-expand on mount — so the "labelled Stale but clicking would work" case is the normal one, not a corner. For larger gaps it only holds after the user expands. Still just the label; the behaviour is fine.
• ScrollingDiffView.tsx:789-792 — return props.files still reads as a dependency declaration when it is really just the prop read that subscribes. One correction to my last round: the child isn't "already disposed" at the first setParsedFiles; both writes land in one synchronous update cycle and disposal happens during that flush. Conclusion unchanged — the reset can't observe a change, and the remount already supplies a fresh empty collapsedFiles. A comment naming that invariant is still worth more than deleting the effect.
• Coverage. grep -rn "collapsedFiles|scrollToAnnotation|setScrollTarget|expandCollapsedFileForNavigation" src --include="*.test.ts" still returns nothing, and there is no e2e/, tests/, or *.spec.ts anywhere. The 19 test lines added in f7942ee cover only the setSubmitError half — the { equals: false } repeat-navigation behaviour that this commit exists to restore has no test, and neither does the expandCollapsedFileForNavigation → double-rAF → querySelector path at ScrollingDiffView.tsx:851-871. That path is where two of the last three rounds' regressions lived. Note that a test for it has to run outside the SSR config to be worth anything.

KISS / YAGNI

resolveQualityFindings, selectedFindingIdsAfterSubmission, and 'resolved' are still unobservable, and I verified each sub-claim rather than re-asserting it:

• Every onSubmitted is a props.onClose that synchronously flips the <Show> gating ReviewProvider (TaskPanel.tsx:652, ResultsScreen.tsx:422, BattleScreen.tsx:154), so nothing painted from the resolved state.
• 'resolved' has zero runtime readers — both consumers filter state === 'open' (ReviewProvider.tsx:130, quality-findings.ts:120); every other reference is the producer or a test.
• The synchronous-flush claim, measured with no await between the two lines:

  before submit                        selected=5
  submittable                          5
  synchronously after setFindings      selected=0  states=resolved×5   ← what line 288 sees
  

  So ReviewProvider.tsx:288-290 is a no-op. Worth noting the current tests pass only because SSR makes createEffect a no-op, so they never exercise the prune effect that makes line 288 redundant.

Deleting all three and dropping 'resolved' from the union removes ~25 lines with no behaviour change.

Scope note

One qualifier I should have made explicit earlier: the only wired provider is devQualityFindingProvider, which returns undefined unless import.meta.env.DEV && VITE_QUALITY_FINDING_FIXTURE === 'true' (dev-quality-finding-fixture.ts:8-14). In shipped builds no findings ever load, so the finding-specific minors above (selection wipe, count mismatch, Stale label) are latent until #238 lands a real provider — not user-visible today. That does not extend to the submitError blocker, which is reachable through human comments alone and therefore live in shipped builds.

What's good

The { equals: false } restore is the correct minimal fix, and it is a genuinely subtle interaction with the clear added one commit earlier — easy to get wrong in the direction of a loop or a swallowed write, and it does neither. The reconciliation helpers remain pure and reference-stable, which is what keeps the read-write effect at DiffViewerDialog.tsx:167 converging. createReviewSubmissionGuard plus a test that fires both submissions without awaiting is still the right shape.

Verdict: Needs changes — one blocker: the submitError dead end survives the fix, because the only control that would clear it is disabled in exactly the state that produces it. clearSubmitError plus a dismiss button on the banner closes it. Everything else is discretionary carry-over, though the untested navigation path remains the structural weak spot, and the SSR test config is why it keeps slipping through.

[LarryHu0217]

Updated in 1a73054. The submission-error banner now has the same dismiss pattern as the findings-error banner, backed by clearSubmitError in the review context. The regression covers dismissing the error after the final annotation has gone.

Validation: npm run check and npm test (1,649 passed, 23 skipped).

[johannesjo]

Reviewed locally on codex/quality-findings-239. Reproduced your validation: tsc --noEmit clean, eslint clean on all touched files, knip clean, full vitest run → 1649 passed / 25 skipped, no regressions.

Overall this is a well-built PR. The contract in src/lib/quality-findings.ts is genuinely provider-neutral, all reconciliation logic is pure and referentially stable (returning the same array when nothing changed is what keeps the read/write effect in DiffViewerDialog from looping), and the new components follow existing conventions closely — QualityFindingCard mirrors ReviewCommentCard's geometry, QualityFindingSidebarItem mirrors SidebarAnnotationItem's background. Every acceptance criterion in #239 is at least nominally met.

Notes below, roughly by severity.

---

1. The navigation highlight added in this PR is never visible

ScrollingDiffView.tsx:794-811 extends highlightedRange() to cover scrollToAnnotation, but ScrollingDiffView.tsx:868 nulls the target out inside the same rAF callback that performs the scroll:

containerRef.scrollTop = elTop - containerTop + containerRef.scrollTop - 80;
if (props.scrollToAnnotation === target) review.setScrollTarget(null);

Solid flushes render effects synchronously, so the highlight leaves the DOM in the same frame the scroll lands. The user sees the highlighted rows for roughly one frame at the old scroll position (off-screen), then the scrolled destination with no highlight. The new highlightedRange branch is effectively dead.

Also worth noting: scrollTarget is created with equals: false (ReviewProvider.tsx:99), so re-clicking the same finding already re-fires the effect — the null-reset isn't needed for repeat navigation.

Suggestion: keep the target set and clear it on a timer, mirroring the dimTimer pattern already in this file (ScrollingDiffView.tsx:813).

2. Dismiss and resolve don't survive closing the diff viewer

ReviewProvider is mounted inside <Show when={props.scrollToFile !== null}> (DiffViewerDialog.tsx:97), and onSubmitted={props.onClose} (:103). So:

• Send one finding → the diff viewer closes → reopen → the provider reloads → the finding you just remediated is open again.
• Dismiss a finding → close → reopen → it's back.

Both round-trips are lost. The AC "a user can dismiss a finding" holds within a session only. For human annotations that was harmless — they're user-created and never re-supplied — but a provider re-supplies the same IDs on every load, so it's user-visible here. Either hoist finding state above the dialog, or stop auto-closing on submit (see #3).

3. Sending one finding closes the entire review UI

Same onSubmitted={props.onClose} wiring. That made sense when submitting meant "I've sent all my comments, I'm done", but the new multi-select flow invites incremental sends — and each one tears down the viewer you're working in. Consider only closing when nothing actionable remains.

4. Selections are silently dropped on diff refetch

DiffViewerDialog.tsx:186 sets diffLoaded(false) at the start of every fetch → reconcileQualityFindingsForDiff forces every finding to pending → the pruning effect at ReviewProvider.tsx:170-181 clears the selection set. So navigating commits via CommitNavBar wipes checked findings with no feedback. Defensible as "don't act on stale data", but it currently reads as incidental rather than deliberate.

5. Smaller items

• ScrollingDiffView.tsx:788-791 — createEffect(() => { setCollapsedFiles(new Set()); return props.files; }). This works (the props.files read registers the dependency regardless of its position in the body), but relying on a returned value for tracking is easy to break with a later edit. createEffect(on(() => props.files, () => setCollapsedFiles(new Set()))) states the intent.
• DiffViewerDialog.tsx:120 — findingProvider is passed down to DiffViewerContent but never read there. Dead prop.
• quality-finding-colors.ts — severity: 'warning' resolves to theme.warning, which is exactly the border color ReviewCommentCard uses for human comments. Inline, a warning finding and a human comment are the same color. The "Automated" text label satisfies the AC, but the color channel works against "visually distinguish automated findings from human comments".
• QualityFindingSidebarItem.tsx:26-29 — clickable <div> with no role/tabindex/key handler, so findings aren't keyboard-navigable. SidebarAnnotationItem has the same gap, so it's consistent rather than a regression — but it is new surface.
• ReviewSidebar.tsx:170 — sendTitle() returns "Select at least one finding" when the count is zero, which is also the state where the user simply has no comments yet. "Add a comment or select a finding" would be accurate.
• compileQualityFindingPrompt — provider-supplied explanation text is interpolated into an agent prompt verbatim with no delimiting. Fine for the fixture; a real scanner emitting Markdown headings or instruction-shaped text will blend into the prompt structure. Worth a note in the contract before a non-fixture provider lands.
• ReviewProvider.tsx:281 — setSubmitError('') runs before the in-flight guard, so a double-click clears the first attempt's error. Cosmetic.

6. Test coverage

The pure contract functions are well covered — quality-findings.test.ts is thorough on reconciliation, selection pruning, and prompt shape. Gaps against the ACs:

• "Findings navigate to the correct file and line" has no test. expandCollapsedFileForNavigation is pure and trivially testable but isn't exported or exercised — and that's exactly the path where issue #1 lives.
• The DiffViewerDialog reconcile-on-load wiring (the diffLoaded gate) is untested; only the pure function underneath is.

Scope note

TaskPanel.tsx:655 changes agentId from props.task.agentIds[0] to selectedAgentId(). This also reroutes plain human review comments, not just findings. It's the right change and #239 asks for "the selected task agent", but it's a behavior change to existing functionality that isn't mentioned in the PR description and has no test.

---

Issue #1 is reasoned from Solid's synchronous render-effect flush plus the rAF ordering — I read it rather than watching it in the running app, so worth confirming. #2 and #3 follow directly from the <Show> boundary and the onSubmitted={props.onClose} wiring.

[LarryHu0217]

Addressed the latest review notes in 9cf20d3:

• Navigation targets now remain highlighted briefly after scrolling, with focused coverage for collapsed-file expansion and stale timer cleanup.
• Review state stays mounted across viewer close/reopen, preserving dismissed and resolved findings.
• Incremental sends keep the viewer open while actionable comments or findings remain, then close after the final item is handled.
• Duplicate submissions no longer clear an existing submission error before acquiring the shared guard.

Validation: npm run check, npm run check:static, 7 focused tests, and the full suite (1,653 passed, 23 skipped). Hosted quality and GitGuardian checks are green.

[johannesjo]

Review feedback

Rechecked the exact head 9cf20d3 with six isolated fresh-context passes (correctness/tests, security, architecture, alternatives, performance, and KISS/YAGNI), then manually verified every retained item against the immutable diff and its callers. The prior highlight, error-dismissal, and incremental-send fixes are present. Both required checks are green, the PR is clean/mergeable, and there are no unresolved inline threads.

Important

• DiffViewerDialog.tsx:221-227 — the new persistence boundary can delete unsent comments. ReviewProvider now survives close/reopen, but every new DiffViewerContent refetch runs evictStaleAnnotations against that shared state. Reopening the same diff therefore drops a retained comment on an added/modified line, because the same hunk still “touches” its range. There is also a race: fetchGeneration belongs to each content instance and is never invalidated on cleanup, so an old request from a closed viewer can resolve after a newer reopen and evict comments created in the new session. Invalidate/abort outstanding loads on cleanup, and only evict against an actual diff-version transition from the version the annotation was created on. Please cover same-diff reopen and out-of-order close/reopen requests with client-lifecycle tests.

• DiffViewerDialog.tsx:78-104 — hoisting the whole provider persists transient state across review sessions and worktrees. This now retains pendingSelection, activeQuestions, scrollTarget, and errors, not only finding decisions. Closing unmounts an AskCodeCard and cancels its request, but the question remains in the outer provider; reopening remounts the card and starts the request again, losing the prior response. More seriously, the Arena callers reuse one dialog while changing diffWorktree (BattleScreen.tsx:83-85), so a pending selection/question from one competitor can reappear in another competitor’s diff when path/line coordinates overlap. Persist only durable review decisions, key them by a review identity (task/worktree/revision), and reset transient interaction state on close or identity change.

• quality-findings.ts:51-57 — reconciliation can silently mark an unrelated occurrence current. Matching uses only filePath plus startLine, while the provider is not reloaded when the selected diff/commit changes. If the offending code is fixed but another line remains at that coordinate—or another commit modifies the same path/line—the old finding becomes current, navigable, and sendable against unrelated content. That violates #239’s stale-reconciliation requirement. Scope results to an immutable diff identity and/or carry a provider-supplied content/context anchor that can be checked against the rendered line; without a matching anchor, conservatively mark the finding stale. Add a same-path/same-line/content-changed test.

Production-provider boundary

• quality-findings.ts:145-153 forwards provider fields verbatim as agent instructions and ultimately raw PTY input. This is latent with the fixed dev fixture, not a production exploit in this PR, but it becomes a real boundary as soon as #238 supplies external findings: Markdown can blend into trusted remediation instructions, and embedded control/bracketed-paste sequences reach the existing sendPrompt terminal write. Before a non-fixture provider lands, serialize findings as explicitly untrusted data and strip/encode terminal control characters at the shared PTY-write boundary; add hostile payload tests.

Verdict: Needs changes.

[LarryHu0217]

Addressed the latest review in 2f70bc1:

• Durable annotations and finding decisions are now scoped to the task/worktree plus an exact SHA-256 diff identity. Exact-diff reopen preserves them; actual revision changes trigger reconciliation, and closed-viewer loads are invalidated before they can mutate a reopened session.
• Transient selection, questions, scroll targets, sidebar state, and errors reset on close or review-identity change. Provider responses are also generation-guarded across close/reopen.
• Finding providers now receive the immutable review/diff context, and findings cannot become current or submittable unless their result identity matches the rendered diff. The same-path/same-line/content-changed case remains pending.
• Added a dedicated client-runtime lifecycle suite for same-diff reopen, worktree switching, and out-of-order close/reopen requests, alongside focused unit coverage.

Validation on the rebased head: npm run check, npm run check:static, npm test (1,661 unit tests plus 4 client lifecycle tests; 23 skipped), security-rule tests, frontend/remote builds, and MCP bundling pass. Both hosted checks are green.

[johannesjo]

Review feedback

Rechecked the exact head 2f70bc1 against #239, the full discussion, and the current diff. The 30 focused lifecycle/finding/navigation tests and tsc --noEmit pass locally, and both hosted checks are green. Two targeted lifecycle counterexamples still reproduce:

Important

• src/lib/diff-review-lifecycle.ts:56 — an unrelated diff change can still silently delete a retained draft annotation. files is the complete current base diff, not the delta since previous. For example, annotate an added line in diff A, close the viewer, change only another file, and reopen. The raw-diff hash changes, so this branch runs; the unchanged added line is still present in the cumulative diff and evictStaleAnnotations treats it as touched, removing the annotation. Please retain a previous content/diff anchor and evict only when the annotated range actually changes or disappears. Add a regression where diff B preserves A's annotated addition and changes only an unrelated file.

• src/components/ReviewProvider.tsx:445-461 — a submission from diff A can mutate or close diff B. submitReview snapshots A, then awaits the asynchronous PTY send. Commit navigation remains enabled and keeps this provider mounted, so B can complete loading meanwhile. The continuation then reads live B annotations/findings, resolves provider-stable IDs by ID alone, rewrites B's selection/sidebar, and may call onSubmitted, which closes B. A rejected A send can likewise surface its error in B. Capture the active review/diff lifecycle generation before sendPrompt and, if it changed while awaiting, do not mutate the current diff or call its completion callback. Please cover a deferred send followed by A→B navigation.

Verdict: Needs changes.

[LarryHu0217]

Addressed both lifecycle blockers in 7eb916d:

• retained a previous parsed-diff snapshot and now keep annotations only when their exact content anchor is unchanged in the next cumulative diff; added regressions for unrelated-file changes plus changed and disappeared ranges
• captured the active review lifecycle before sendPrompt and ignore stale success or failure continuations after diff navigation; added client regressions for both deferred resolve and reject across A -> B navigation

Validation:

• npm test: 1,663 unit tests passed, 23 skipped; 6 client-lifecycle tests passed
• npm run check
• npm run check:static
• npm run test:security-rules
• hosted quality and GitGuardian checks are green

Ready for re-review.

[johannesjo]

Review feedback

Rechecked the exact head 7eb916d against #239, the complete discussion and diff, current thread state, and CI. There are no inline review threads; both required checks are green. Locally, all 1,663 unit tests and 6 client-runtime tests pass, as do npm run check, npm run check:static, and both frontend and remote builds.

Important

• src/lib/diff-review-lifecycle.ts:84 — annotations created on expanded context-gap lines survive after their location disappears. ContextGapView loads and renders those lines separately from the parsed unified-diff hunks, but annotationContentAnchor only searches previous.files[*].hunks. For a valid annotation created on such a rendered line, previousAnchor is therefore null, and line 84 retains it unconditionally across every same-worktree diff transition—even when the next diff deletes the file. The stale annotation remains in the sidebar, navigation has no destination, and submitReview still treats it as sendable feedback. I reproduced this on the exact head with both a focused lifecycle regression and a mounted ReviewProvider client-runtime regression. Conservatively evict annotations whose previous anchor cannot be reconstructed when the diff identity changes, or capture their exact line-content anchor when they are created; please add the expanded-gap → changed/deleted location regression.

Verdict: Needs changes.

[LarryHu0217]

Addressed the expanded-context annotation lifecycle blocker in f8502ab:

• When the diff identity changes, annotations whose previous content anchor cannot be reconstructed are now conservatively evicted instead of being retained indefinitely. Exact-diff reopen behavior is unchanged.
• Added both a focused lifecycle regression and a mounted ReviewProvider regression for an expanded context-gap annotation whose location disappears.

Validation: npm test (1,664 unit tests plus 7 client-runtime tests), npm run check, npm run check:static, and npm run build:frontend pass.