feat: Commit Review, micro AI reviews in the Changes panel (v3.7.0) - #164
Merged
Conversation
added 29 commits
August 13, 2026 15:56
Lift indexDiffFiles/parseFileDiff/parseUnifiedDiff out of usePrPanel.ts into a pure utils/unifiedDiff.ts (no Vue, no backend import), and isEditableTarget out of usePrReviewKeymap.ts into utils/editableTarget.ts, both re-exported for back-compat so every pre-existing test keeps passing unmodified. usePrPreReview's analyzeFile gains a scope: "pr" | "commit" option that swaps only the prompt's framing and dependency-signal sentences; severity scale, confidence rules and the JSON contract stay byte-identical. This lets the v3.7.0 commit-review feature reuse the same engine over the staged diff instead of a PR, with no behavior change for existing PR callers (default stays "pr").
Add useCommitReview.ts, a composable that runs the v3.6.0 pre-review engine (usePrPreReview.analyzeFile, scope: "commit") over the staged diff fetched via the existing gitExec(["diff", "--cached"]) primitive. No new Tauri command. Reuses the existing usePrReviewQueue for sequential/abortable/visibility-gated execution and the existing reviewAiConfidenceThreshold/reviewAiMaxFindings settings for the threshold and cap, rather than adding commit-specific twins. Caps the reviewed staged set at 40 files / ~400 KB (slice-order truncation) so a huge staged tree cannot fan out hundreds of LLM calls, exposing a truncated flag for the UI. Off by default: add commitReviewEnabled (opt-in) and commitReviewAutoReReview settings to both useSettings.ts and SettingsPanel.vue, with a new "Commit Review" group in the AI tab. Add commitReview.* and settings.commitReview.* i18n keys to all 5 locales.
…inline findings Wire the Commit Review engine into the UI: - RepoSidebar.vue: a "Review staged changes" button (shown only when the opt-in setting is on and something is staged) with a spinner and done/total progress, plus a findings badge that opens the summary modal. Also adds a per-file finding count chip in both the flat-list and tree-layout file renderers (RepoSidebar has two independent renderers for the file list; both needed the chip). - DiffViewer.vue: a new optional `findings` prop. Inline mode renders one finding row per anchored line (severity badge, confidence, title, detail, Dismiss), reusing the existing prAnnotations grouping so LEFT/RIGHT findings on the same line number never merge and an orphan finding (line not in the diff) renders nothing instead of throwing. Side-by-side mode gets a gutter severity marker only, no card (decision D4 - full SBS cards are a follow-up). Findings render via plain-text interpolation only, never v-html. Exposes scrollToFinding(line, side) for Task 2's navigation. - CommitReviewModal.vue: new component (modelled on SecretsFindingsModal.vue) showing the deterministic summary line and a severity-then-confidence-sorted finding list with Jump to/Dismiss per finding and an empty state. - App.vue: instantiates useCommitReview, wires the button/badge/modal, passes per-file findings to DiffViewer (index-scoped, so never painted on an unstaged diff), and extends the existing staged-set watch to reset() the review (never auto-run) whenever the staged set changes. Add commitReview.* i18n keys (all 5 locales) for the new UI strings.
Add commitReviewKeymap.ts, a pure resolver mapping n/p/x/? to next-finding/prev-finding/dismiss-finding/help. Unlike usePrReviewKeymap (which relies on its host to focus-guard), this resolver guards isEditableTarget itself: the commit summary input and description textarea live in the same view as the findings list, so bare letters must stay inert while typing there. Add useCommitReviewNav.ts, porting usePrReviewNav's jumpToFinding to switch the selected staged file via an injected selectFile callback and scroll the mounted DiffViewer to the finding's line/side. Findings are cycled in the same severity-then-confidence order the modal displays them in (sortFindingsForReview, now the single shared source of that order, extracted out of CommitReviewModal.vue). Dismissing the finding at the cursor clamps it back into range as the list shrinks. Wire both into App.vue's existing onKeyDown, early and before the mod-key ladder, active only in the Changes view with the feature on and at least one finding. "?" surfaces a one-line reminder via the existing toast affordance rather than a new help modal. The per-file finding count chips (RepoSidebar's flat-list and tree-layout file renderers) and the reviewFindingsByFile prop were implemented together with the Task 1b RepoSidebar wiring in the previous commit, since both touched the same file in the same edit pass; this commit is the rest of Task 2 (the keymap resolver, the nav composable, and their App.vue wiring).
resolveCommitReviewShortcut bailed on any held modifier, including
shiftKey, before ever reaching the "?" case. Since "?" requires Shift on
every standard keyboard layout, that branch was dead code and the help
toast could never fire.
Only meta/ctrl/alt are rejected up front now; shift is checked per key
instead, so n/p/x stay inert with Shift held (Shift+n etc. is noise) while
"?" (Shift+/) still resolves to help. Mirrors usePrReviewKeymap.ts, which
has the same shape for the same reason.
Fixed the test that asserted kd("?") (no shiftKey) resolved to help, which
was asserting a KeyboardEvent shape a real keyboard never produces, and
added coverage for the shift-blocks-letters-but-not-question-mark
distinction plus meta/ctrl/alt still blocking "?".
The <td class="diff-finding-cell"> carried display: flex directly, which takes it out of the CSS table box model: colspan is silently ignored by the browser on a flex-display cell, so the finding card rendered squeezed into the 48px line-no column instead of spanning the row. The <td> now stays a plain table-cell (no display override, colspan works again); the flex layout moved onto a new .diff-finding-body div wrapped around the same content. jsdom cannot catch layout regressions like this one, so please eyeball it manually in a browser: open a repo with staged changes, enable Commit Review, run a review, and confirm a finding row spans the full diff width in both inline modes (with and without selectable partial-staging checkboxes, since colspan differs between the two). Added a regression assertion (colSpan > 1 and the flex wrapper is a child of the td, not the td itself) so a future refactor that moves display: flex back onto the td fails a real test instead of only a manual check.
useCommitReview wrapped a single shared usePrReviewQueue() instance and
called queue.run() again on every run(). usePrReviewQueue.run() has a
try { ... } finally { running.value = false }; if run A is aborted while
its in-flight analyzeOne() is still resolving and run B has already
started, A's stale finally could fire after B and flip the shared
running/done/total refs back to A's state mid-run-B.
run() now creates a fresh usePrReviewQueue() instance every call; an
`activeQueue` shallowRef always points at the most recent one, and the
exposed running/progress/resume all read through it. A stale run's
cleanup only ever touches refs nothing else reads anymore, so it can no
longer clobber a newer run. (shallowRef, not ref: a plain ref would make
the queue object itself reactive, and Vue auto-unwraps its nested
done/total/running refs on access, which silently turned every .value
read into undefined once activeQueue became reactive - shallowRef keeps
the queue object itself as a plain, non-unwrapped value.)
Also changes run()'s return type from Promise<void> to Promise<boolean>:
true once a full attempt actually happened (even if it errored, the
staged diff was empty, or every file was filtered out), false when the
call never really tried (disabled, AI unavailable, empty cwd, or
superseded by a newer run before it got anywhere). This is what lets the
App.vue wiring (next commit) tell "ran with zero findings" apart from
"did not run" without adding a second signal.
Added tests: a stale aborted run's cleanup no longer clobbers a newer
run's running/progress state; the file-count cap test is tightened from
toBeLessThanOrEqual (which passed even at 0 calls) to an exact toBe(40);
a new byte-budget test proves a single oversized file's diff exhausts the
cap and excludes files after it; and a resume()-unblocks-a-hidden-run
test that proves the run actually completes instead of hanging.
…pass useCommitReview.resume() was exposed but nothing ever called it. Starting a review then hiding the tab (or minimizing) left the queue paused forever on document.hidden inside usePrReviewQueue.waitWhileHidden(): only a staged-set change (via commitReview.reset()) could unwedge it, and the button stayed disabled in the meantime. usePrPanel.ts already has the app's one visibilitychange listener (it resumes useReviewIntelligence's PR pre-review queue, a different queue instance, on the same hidden -> visible edge). Added an onVisibilityResume option to PrPanelOptions, called from that same handler, and wired commitReview.resume into it from App.vue's usePrPanel(...) call - no second listener. Also, two silent-feedback gaps in the same "Review staged changes" click handler: - A failed review (gitExec error) set commitReview.lastError but nothing read it, so a failure was indistinguishable from success. Reused the existing repoError error-toast banner via a watcher, matching every other place in this file that funnels a failure into repoError.value. - A clean review (zero findings) produced zero UI feedback, indistinguishable from "didn't run" or "failed". commitReview.summaryClean already existed but was unreachable outside the findings modal (which only opens when there are findings). The reviewStaged handler now checks run()'s new boolean return (only true once a full attempt actually happened) and shows a brief transient toast via the same affordance already used for the "?" shortcut-help reminder. No new test file for the App.vue wiring itself (no App.test.ts exists in this codebase); the new usePrPanel-visibility-resume.test.ts proves the visibility-triggered callback fires on the hidden -> visible edge and never throws when the option is omitted, and useCommitReview.test.ts (previous commit) proves resume() actually unblocks a paused run.
…file chip to staged rows commitReviewAutoReReview had a real Settings checkbox (SettingsPanel.vue, useSettings.ts, i18n x5) but zero consumer in this PR: its only purpose is arming the one-shot re-review after a "Fix with agent" handoff, which is Task 3 and explicitly out of scope here. Hid the toggle from the Commit Review settings group rather than ship a control that visibly does nothing yet; the setting field itself, its default, and the i18n strings stay in place for PR2 to wire up. Also gated RepoSidebar's per-file finding-count chip on section === 'staged' in both the flat-list and tree-layout renderers. Findings are index-scoped, so without this gate the chip also decorated the unstaged/untracked row of the same path, which never reflects the staged diff's content.
Records the step-by-step plan behind the Commit Review feature (ROADMAP.md v3.7.0), including the decisions on 3-way PR splitting and warn-only pre-commit hook scope.
…ch worktree Task 3 of v3.7.0: adds buildReviewFixPrompt (pure, no secrets, PTY-safe), a Fix with agent control in CommitReviewModal (tool picker + scratch worktree checkbox), and useCommitReview.armReReview/onStagedSetChanged, the real staged-set-watcher trigger for the one-shot re-review after a fix handoff (decision D7: the prompt is typed into the agent PTY without pressing Enter). Factors confirmNewAiTask's scratch-worktree sequence into a shared helper. Un-hides the previously dangling commitReviewAutoReReview setting now that it has a real consumer. disabled
Task 4 of v3.7.0: adds commitReviewState.ts, a pure, localStorage-persisted store (pattern: usePrCache.ts) tracking review->fix->review iterations and content-hash based coverage of the staged diff's added lines, so a fix that shifts line numbers never destroys coverage. Wires iterations/coverage into useCommitReview (recorded only for a non-aborted run, guarding against the same stale-run race already covered for running/progress), refreshes iterations from persisted state on repo switch, and clears state after a successful commit (a new commit starts a new review cycle). disabled
…ed/skipped decision Task 5 of v3.7.0: adds buildReviewTrailer, the pure commit-gate helpers (resolveCommitReviewGate, effectiveReviewDecision, appendReviewTrailer) and CommitReviewDecisionModal.vue, wired into App.vue's handleCommitRequest via a shared proceedToCommit path (also used by the secrets "Commit anyway" route, so both funnel through the same gate). Same non-blocking UX contract as the v3.5.0 secrets scanner: cancelling the decision modal cancels the commit rather than silently recording "skipped" (decision D8). "Review now" leaves the commit pending; a later commit click with iterations > 0 proceeds without re-prompting and records "ran". A successful commit clears the review-state cycle. disabled
…flakiness The real-timer version of the debounce/one-shot re-review test had only a few ms of margin between its waits and the debounce window, and failed once under load during full-suite verification. Switches it to fake timers (pattern: useSecretsScanner.test.ts), scoped to just this one test. disabled
….ts binary hashLineKey's template literal had a literal, embedded NUL byte instead of the \0 escape sequence, which made the whole file register as binary to git (git diff showed "Binary files differ", GitHub would render it as "Binary file not shown", and git blame/merge/diff were broken on it going forward). Replaces the raw byte with the \0 escape sequence in source, so the runtime hash values are byte-identical (verified: every existing hashLineKey test still passes unmodified) while the file itself is plain UTF-8 text. Adds a regression guard that reads the source file's own bytes and fails if a raw NUL character ever reappears. disabled
…onest between reviews Two related correctness fixes to the iterations/coverage tracking: 1. Coverage no longer asserts 100% right after a plain staged-set change. onStagedSetChanged now recomputes it against the CURRENT staged diff (a plain git diff fetch, no LLM call), guarded by a generation counter so a stale in-flight refresh can never clobber a newer one or a real run(). Previously a brand-new unreviewed file staged after a completed review kept showing coverage:100% until the next explicit review click. 2. iterations is now bound to the repo's HEAD commit, not just the review count. run() resolves and stamps HEAD via a single git rev-parse HEAD call per run, and reconcileIterationsForHead (awaited by the commit gate before it ever reads iterations) resets the count to 0 when HEAD moved since the last recorded review. Without this, a commit made outside the app (amend, terminal commit, any external tool) left a stale iterations count that let the gate skip the decision modal and write a "ran" trailer for a review that never happened against what was actually being committed. Also resets iterations when snapshot pruning empties a repo's snapshot list entirely (aged-out evidence). disabled
… now, harden the agent handoff
- proceedToCommit awaits commitReview.reconcileIterationsForHead before
resolveCommitReviewGate ever reads iterations, so a commit made outside
the app since the last review is caught before the gate decision (see
the companion commitReviewState/useCommitReview fix).
- onCommitReviewDecisionReviewNow ("Review now" in the decision modal) now
reuses onReviewStagedClicked directly instead of a weaker duplicate:
it only opens the findings modal when a review actually completed
without error, and surfaces "no AI provider configured" via repoError
when the run never even attempted. Previously it always popped an empty
"No findings" modal regardless of outcome.
- onCommitReviewFixWithAgent adds a short readiness wait before typing the
prompt into a freshly spawned agent PTY, and surfaces scratch-worktree
and terminal-session failures instead of silently doing nothing after
the review modal has already closed.
Manual QA performed against real claude and codex CLIs via the dev-server's
node-pty backend (see PR report for the full write-up): once an agent is
at its normal ready-to-chat input, writing the whole multi-line prompt as
one burst lands as unsent text with no premature submission. A first-run
"trust this directory?" onboarding screen (always hit by the scratch-
worktree path, since it is always a brand-new directory) is a separate,
unresolved risk this readiness wait does not cover, flagged in code
comments and the PR report for explicit sign-off before merge.
disabled
sortFindingsForReview lived in composables/useCommitReviewNav.ts, but utils/reviewFixPrompt.ts (a pure utils module) needed the same order and was importing it from there, inverting the established utils-do-not- depend-on-composables direction (Task 0's unifiedDiff.ts/editableTarget.ts precedent). Moves it to utils/reviewFindingsSort.ts; useCommitReviewNav.ts re-exports it verbatim for back-compat with CommitReviewModal.vue's import. disabled
Deliberate scope-narrowing for PR2, not a bug fix. Manual QA against real
claude/codex CLIs (see the previous commit's report) found that a brand-new
scratch worktree always hits a first-run "trust this directory?" onboarding
screen that misinterprets the piped fix prompt as menu navigation, which
drove a real brew upgrade --cask codex attempt in testing. The user was
shown this finding and decided to disable the scratch-worktree option for
this PR rather than ship it with that risk.
CommitReviewModal.vue's "Fix with agent" footer no longer offers a scratch
checkbox; its fix-with-agent emit payload drops the scratch flag. App.vue's
onCommitReviewFixWithAgent always targets the current repo (already
trusted, no onboarding screen) and no longer calls
createAiTaskScratchWorktree, which stays untouched for its other caller,
confirmNewAiTask ("New AI task"). Removed the now-unreachable
commitReview.fixInScratch i18n key from all 5 locales after confirming it
was not used anywhere else. Updated ROADMAP.md's Fix with agent bullet to
record the cut and the follow-up: revisit scratch-worktree support once
there is a real fix for the onboarding-trust-screen problem (pre-trusting
the directory before launching the agent, or detecting the onboarding
screen before writing).
disabled
…fore the trailer is written Second verifier pass, HIGH: coverage:100% was still reachable in two real scenarios even after the previous round's fix. Scenario B: stage a.ts, review it (coverage 100), then edit a.ts further and restage it. The staged file COUNT never changes (still one file, just more content), so App.vue's staged-set watcher (keyed on repoStats.staged, a count) never fires onStagedSetChanged, the coverage refresh never runs, and the commit gets a stale coverage:100% for content that was never reviewed. This is the single most common review-fix-review cycle. Scenario C: run()'s own coverage computation compared the just-reviewed, file/byte-capped subset against itself, a tautology that always yields 100% even when the file-count cap dropped most of a large staged tree. Fix: a new shared pure helper, coverageFromDiffText, always computes coverage against the FULL parsed staged diff, never the capped subset sent to the AI. run() now uses it for both of its own coverage snapshots. A new computeCurrentCoverage method wraps it with a fresh git diff fetch and is awaited by App.vue's proceedToCommit right before buildReviewTrailer, so the trailer's coverage number is correct at the moment of commit regardless of staged-set-watcher granularity or truncation. Also fixes a stale doc comment on createAiTaskScratchWorktree that still claimed it was shared with Commit Review's scratch-worktree option (removed in the previous commit) — it is now only used by "New AI task". disabled
…nt finding text Second verifier pass, low priority. sanitizeLine only stripped \r and collapsed newlines before a finding's title/detail got written raw into a PTY. That text is parsed from an AI response to an arbitrary diff, so a crafted diff could in principle smuggle other control bytes (ANSI escape sequences, NUL, BEL) into the raw PTY write. Broadens the strip to the full C0 control range plus DEL after the existing \r/newline handling. disabled
… 100 when disabled mid-cycle Third verifier pass, item L1: computeCurrentCoverage and refreshCoverageForCurrentDiff both defaulted next to 100 whenever the feature was disabled or the diff fetch failed, discarding whatever truthful coverage the ref already held. Toggling Commit Review off between a real review and the commit could launder a genuinely partial coverage back to a false 100% in the trailer, the same failure class already fixed twice for other code paths in this PR.
… pass Adds a commitReview block to GitWandrcConfig (enabled/minConfidence/ maxFindings/maxFiles), validated with the same defensive style as the existing secrets block: booleans type-checked, numbers range-checked, unknown keys ignored, empty block omitted. useCommitReview resolves this block the same way useSecretsScanner resolves its own gitwandrc block, cached per repo, and lets it override the app Settings in both directions (a repo can force the review pass on or off, or tune its confidence threshold / caps, regardless of the global setting). run() and the coverage-recompute helpers now gate on this resolved config instead of reading settings.commitReviewEnabled directly.
…iew sections) .git/hooks/pre-commit can only hold one script, and the v3.5.0 secrets hook already owned it outright via gitHookCreate's overwrite semantics. Introduces gitwandHook.ts: a single GitWand-managed script builder with a v2 marker and parseable section boundaries, so a warn-only Commit Review reminder section can be installed alongside (or independently of) the secrets-scanning section without either clobbering the other. parseGitwandHookSections recognizes a previously-installed v1 secrets-only script (secretsHook.ts, now marked deprecated but kept for this migration) as secrets:true/review:false, and returns null for any foreign hook. The secrets section's npx --no-install @gitwand/cli scan --staged --strict --json invocation is byte-identical to the shipped v3.5.0 script; the new review section always exits 0.
HooksPanel's single "Secrets pre-commit hook" row becomes two independent rows (Secrets / Commit review), each driven by parseGitwandHookSections and each with its own install/remove flow. Both write through the same buildGitwandHookScript builder so installing or removing one section never clobbers the other; removing the last remaining section deletes the hook file entirely. Reuses the existing askConfirm modal gate for both rows, matching the prior secrets-only confirm copy style. Adds the settings.commitReview.rcOverrideHint line to the AI tab noting that a repo's .gitwandrc can force Commit Review on or off, and adds every new user-visible string (hook row labels/confirms/errors, the rc override hint) to all 5 locales.
…threshold/cap HIGH: App.vue read settings.value.commitReviewEnabled directly at three call sites (the Review staged changes button, the n/p/x shortcut guard, and the commit-time decision gate), so a .gitwandrc override never reached the UI in either direction. A repo forcing the feature on had no reachable path into run(), and worse, a repo forcing it off still showed the button, still popped the Review/Vouch/Skip modal on every commit, and still wrote a GitWand-Review trailer. useCommitReview now exposes effectiveEnabled, a computed that resolves .gitwandrc's commitReview.enabled against the app setting for whatever repo is currently active, refreshed by onStagedSetChanged (the same per-repo refresh App.vue's existing repoFolderPath/staged-count watcher already triggers) and by run(). App.vue's three call sites now read this instead of the raw setting. MEDIUM: effectiveThreshold/effectiveCap were one-shot refs written only inside run(), so changing the Review AI confidence threshold or max findings cap in Settings after a run had no visible effect until the next full review. They are now computeds reading the same rc-override state reactively, so the findings list re-filters immediately on a Settings change. Adds effectiveEnabled direction tests (rc forces on/off against the opposite app setting), a live-refresh test via onStagedSetChanged with no LLM call, a no-repo-open fallback test, and two reactivity tests proving a live Settings change re-filters findings without a new run. Also adds the previously-missing maxFiles/maxFindings override tests.
…mit script LOW #1: hooks.secretsInstallConfirmMessage and secretsRemoveConfirmMessage in all 5 locales still described the old single-hook-file behavior (any existing hook overwritten, file always deleted on removal). Now that installing/removing writes through the shared sectioned script builder, installing over a GitWand-managed hook merges sections rather than overwriting, and removing secrets while commit review stays installed rewrites the file to review-only instead of deleting it. Updated the copy in all 5 locales to describe this accurately. LOW #3: the pre-commit hook's warn-only review reminder claimed "this commit was made from the terminal", which is wrong for a GUI commit that reaches the hook without --no-verify (harmless since GitWand discards hook stdout on a successful GUI commit, but still inaccurate). Reworded to a generic "commit review did not run for this commit" that does not assume the commit's origin. This text lives in the generated bash script itself, not in the i18n locale files, so there is no per-locale variant to update for it.
…eCommitReviewConfig Third verifier pass, LOW-2: the fix that made effectiveThreshold/ effectiveCap into computeds removed their only readers (the one-shot assignments inside run()), leaving resolveEffectiveConfig's threshold and cap fields produced but never consumed anywhere.
… fix stale zero-IPC doc Fourth verifier pass (direct review), three confirmed findings: - HIGH: rcOverride.value was written unconditionally in refreshRcOverride, unlike coverage.value which is guarded by coverageGeneration. A slow first-visit .gitwandrc read for a repo the user already left could resolve after a newer repo's read and overwrite effectiveEnabled with the wrong repo's config. Now guarded by the same coverageGeneration counter already protecting coverage.value. - MEDIUM: HooksPanel.vue's install/remove handlers for the secrets and review hook sections used two independent busy flags but all read-modify-write the same hookSections.value snapshot through a single full-file rewrite. Installing both in quick succession could let one write silently clobber the other. Both sections now share one busy gate (hookWriteBusy), checked both in the template and at each handler's entry. - The file's own doc comment claimed zero IPC when Commit Review is disabled; that's no longer true since .gitwandrc must be read to support a per-repo force-on override. Corrected the comment and the two tests that were asserting a stale "zero IPC" claim without ever checking readGitwandrcMock.
4 tasks
This was referenced Aug 19, 2026
Closed
added 18 commits
August 19, 2026 18:20
…ndings status.value gets reassigned to a brand new object on every 2s status poll tick even when nothing changed. The old repoStats computed always returned a fresh object literal, so Vue's hasChanged(new, old) was always true and notified every subscriber unconditionally, including a single-getter-array watch in App.vue whose own multi-source form is also always "changed" for a fresh array. That watch reset commit review's findings and re-ran a full secrets scan every 2s, wiping real findings within seconds with no user interaction. Move the repoStats computation into a pure utils/repoStats.ts module and memoize it: a structurally identical recomputation now returns the previous object reference, so a no-op poll no longer notifies at all. Also widen App.vue's watcher from a bare staged count to a staged fingerprint (paths + statuses) and convert it to Vue's proper multi-source array form, which additionally fixes a pre-existing gap where unstage-A + stage-B left the count unchanged and never re-fired. This also fixes a pre-existing perf bug: the v3.5.0 secrets scanner sat on the same watcher and was re-scanning every 2s in every repo regardless of whether anything changed.
…ng the last poll Task 1 removed the accidental every-2s secrets rescan (it was firing unconditionally on every status poll tick, not just on real staged-set changes). That leaves one case uncovered: editing and restaging an already-staged file changes neither the staged count nor the staged fingerprint, so a secret introduced that way would no longer be caught until the next real staged-set change. Commit review already compensates for the same gap by recomputing coverage fresh at commit time; the secrets gate did not. Add an awaitable, un-debounced scanNow() to useSecretsScanner and await it in handleCommitRequest before reading activeFindings. One IPC on the commit path, consistent with the two IPCs proceedToCommit already awaits. Session dismissals are preserved since scanNow never touches dismissedKeys.
…ate clear runs clearReviewState() cleared iterations/coverage without calling stop() first, unlike reset() which does. A background re-review armed by "Fix with agent" can still be in flight right after a commit; on completion it reaches recordReview(cwd, files, headHash) with the pre-commit HEAD hash, re-populating a store that was just cleared and showing a bogus iteration badge until the next commit self-heals it. Call stop() first in clearReviewState, mirroring reset()'s ordering.
… finding is live resolveCommitReviewGate never consulted the actual findings: once a review had run once this cycle (iterations > 0), it proceeded straight to commit regardless of what the review found. Combined with the task-1 watcher bug, a commit could silently carry a "ran" trailer while a real Risk finding was flagged and never seen. Add a required unresolvedRiskCount field and re-prompt whenever it is greater than 0, checked after the explicit-decision short-circuit (so Vouch/Skip still stop the loop) and before the iterations check. Still never a hard stop: Vouch and Skip both proceed, and dismissing the finding removes it from the count so it stops re-prompting.
…he decision modal
CommitReviewDecisionModal always rendered "{0} finding(s) in your
staged changes", so "0 finding(s)" read identically whether a review
ran and found nothing or no review ever ran at all. Task 4 makes
iterations > 0 reachable at this modal (previously the gate always
proceeded once a review had run), which is exactly why this needed to
land together with it.
Add a riskCount prop and three mutually exclusive context strings
driven by iterations/findingsCount, plus a distinct warning line when
riskCount > 0. New keys in all 5 locales: decisionNotReviewed,
decisionReviewedClean, decisionRiskWarning.
… counter run() bumped coverageGeneration but discarded the returned value, then wrote coverage.value twice later guarded only by controller.signal.aborted -- a different invalidation channel than the generation counter. A computeCurrentCoverage call that starts and resolves after run() bumps the counter (e.g. App.vue's proceedToCommit) could still be clobbered by run()'s own, now-stale write finishing later. Capture myCoverageGeneration at the bump and gate both later writes with it, same contract as refreshCoverageForCurrentDiff and computeCurrentCoverage already use.
All four success-path writes in /api/read-gitwandrc were missing ...corsHeaders(req), unlike every sibling text/plain route in this file and unlike this same route's own error path (which goes through jsonResponse(), always CORS'd). The browser blocked the fetch outright, making .gitwandrc's per-repo commitReview/secrets overrides untestable via pnpm dev:web, this repo's own sanctioned manual-QA path. Add the same ...corsHeaders(req) spread the five sibling text/plain routes already use to all four writeHead(200, ...) calls in that route. Wire format stays text/plain (frontend reads raw JSONC text with comments intact). New tests/parity/read-gitwandrc-cors.test.mjs runs under pnpm test:parity (uses only startDevServer(), no Rust binary needed since CORS is HTTP-layer only). Rust's read_gitwandrc is a Tauri IPC command, not an HTTP endpoint, so no Rust-side change is needed or possible.
…ok is installed A foreign (hand-written) pre-commit hook collapsed to the exact same "not installed" UI state as no hook at all (parseGitwandHookSections returns null for both), so Install would silently OVERWRITE the user's own script with only unconditional prose in every confirm message as a warning, shown identically when there was provably nothing to overwrite. Add classifyPreCommitHook (utils/gitwandHook.ts, unit-tested) returning a PreCommitHookKind of none/gitwand/foreign alongside the existing section flags. HooksPanel.vue renders a distinct warning row when a foreign hook is detected, and the two Install confirms swap in a foreign-specific message with an explicit overwrite warning; the non-foreign messages drop their now-false "if it is not managed by GitWand it will be overwritten" clause. New/reworded keys in all 5 locales.
COMMIT_REVIEW_MAX_BYTES's doc comment claimed a "hard cap", but the enforcement admits a file slice BEFORE decrementing the budget, so a single file whose diff already exceeds the whole budget is still admitted whole, only files after it are excluded. This is tested and intentional (see the test named "truncates by the byte budget when a single file's diff exceeds it, excluding files after it") -- only the comment was wrong. Comment-only change: pnpm test is green with zero test edits, which is itself the proof nothing behavioral moved.
A raw git diff always ends with a newline, and indexDiffFiles joins
per-file slices with "\n" too, so parseFileDiff's split("\n") yielded
one trailing "" element on the last file's slice. That element was
classified as a (deliberately, per AGENTS.md) blank context line, which
landed a phantom zero-length context row on the last hunk of the last
file and pushed its line counters past what the hunk header declared.
This PR's own doc comment had started asserting that as deliberately
correct, contradicting the Rust parser's prior explicit fix for the
identical bug class (src-tauri/src/commands/read.rs).
Drop exactly one trailing "" element (only when it is last) before any
line is classified. A genuine blank context line mid-hunk, or one right
before EOF in a diff ending in "\n\n", is untouched -- only the split
artifact from the diff's own trailing newline is removed.
Regression sweep: ran usePrPanel-lazy-diff.test.ts,
usePrPanel-findings-render.test.ts, usePrPanel-lineAnnotations.test.ts,
usePrPanel.test.ts, useReviewIntelligence.test.ts,
DiffViewer-findings.test.ts, and PrInlineDiff.test.ts -- all pass
unmodified, so no existing PR-review expectation encoded the phantom
line as expected output.
PullRequestPanel.vue carries its own private, byte-identical copy of
this parser with the same defect; left untouched here (separate
refactor, own regression surface), noted as a follow-up in the plan.
BaseModal had no focus trap, no initial focus, and no focus restore:
only a window-level Escape handler. This is the shipped foundation for
~30 modals across the app (SecretsFindingsModal, CommitReviewModal,
SettingsPanel, EditCommitOverlay, SplitCommitModal, and every other
BaseModal consumer), a pre-existing gap this PR did not introduce.
Add trapFocus/autoFocus props (both default true, both an escape hatch
for a modal that needs different behavior). On mount, synchronously
(not nextTick) remember the previously focused element and focus the
panel; synchronous is load-bearing because Vue fires a child's mounted
before its parent's, and AiTaskNameModal/CloneModal/ForkModal/
FolderPicker self-focus an input from their own onMounted + nextTick,
so a nextTick here would race them. On unmount, restore focus to the
remembered element if it is still in the document. A bubble-phase
keydown listener on the panel itself (not window) traps Tab, wrapping
at the ends via the new pure utils/focusTrap.ts (focusableWithin +
nextTrapTarget, unit tested without mounting a component), and bails
on e.defaultPrevented so an inner component that owns Tab (CodeMirror,
xterm) always wins first.
Manually reasoned through (no live browser available in this session,
recommend a dev:web pass before merge):
- CloneModal, AiTaskNameModal, ForkModal: their nextTick self-focus
still wins, confirmed by the synchronous-vs-nextTick ordering.
- FolderPicker: its own separate overlay (not a BaseModal instance),
rendered as a sibling in App.vue, not nested inside CloneModal's
panel DOM -- no double-trap interaction.
- EditCommitOverlay: NOT in the plan's originally-identified list of
self-focusing modals -- it self-focuses via
watch(entry, ..., {immediate:true}) + setTimeout(50), not
onMounted + nextTick. Verified this still ends up correct: the 50ms
macrotask reliably fires after BaseModal's synchronous focus and
after any nextTick, so the summary textarea still wins the race.
Flagging this as a plan-completeness gap, not a functional
regression.
- No modal in this codebase currently embeds CodeMirror or xterm
inside a BaseModal (TerminalPanel/FileExplorerPanel are docked
panels, MergeEditor is a full view) -- the e.defaultPrevented guard
is a forward-looking safeguard, not exercised by any existing modal
today.
- askConfirm's generic confirm modal is likewise a sibling BaseModal
instance at App.vue's root, not DOM-nested inside another modal's
panel -- each panel's own bubble-phase listener only reacts when
focus is within its own subtree.
Full desktop suite (107 files / 937 tests) run and green after this
commit specifically, not just at the end of the plan.
"Select this folder" called emit("select", currentPath.value), and
currentPath is only ever written by fetchDir (reached via
navigate/goUp/goHome/onInputEnter) -- so a path typed into the input
was silently ignored unless Enter was pressed first.
selectCurrent() now resolves the typed path through fetchDir first
when it differs from currentPath, normalizing it and surfacing a bad
path as this dialog's own inline error instead of failing downstream
in openRepo. fetchDir is already non-throwing and sets errorMsg, so no
new error handling is needed.
The "Enable AI suggestions" checkbox and its hint were conflict-only, so a user who only wanted Commit Review (or PR pre-review) had no reason to tick it and never discovered those features' settings exist -- everything below the toggle, including the Commit Review group, is gated on it. Copy-only fix, per decision O4: reword the label/hint in all 5 locales so the toggle states its full scope (conflict-resolution suggestions, commit messages, PR pre-review, Commit Review). No template, settings, or behavior change -- ungating commitReviewEnabled from aiEnabled would let the toggle be on while the provider is unavailable, showing a "Review staged changes" button that silently does nothing, a worse UX than the current discoverability gap. No new test: no key was added or removed (pnpm build's vue-tsc check is the only verification a pure value change needs), and there is no existing test asserting copy values to update.
… fallback 14a: the four Settings nav sidebar group headers were hardcoded strings (three of them French: "Dépôt", "IA & Agents", "Système"), rendered with no t() call, shown untranslated in every locale, while every sibling tab label already goes through t(). Changed settingsNavGroups to carry a labelKey instead of a raw label string, added 4 new keys (navGroupApplication/Repo/Ai/System) to all 5 locales. 14b: useAIProvider.ts intentionally falls through to the local Claude Code CLI when the selected provider is genuinely misconfigured (a confirmed, working-as-intended behavior, not a bug), but nothing in the Settings UI said so. Add one conditional hint line under the provider select, shown only when the selected provider (claude or openai-compat) is missing its required config AND the CLI is present (SettingsPanel already holds claudeCliInfo for this). Does not change which provider is picked. One new key (aiProviderCliFallbackHint) in all 5 locales. Both trivial, zero-risk, cheap i18n/indicator fixes bundled per decision O5; no new test (no test asserts copy values or nav-group rendering specifically), pnpm build's vue-tsc missing-key check is the verification.
The global constraint (and the standing house rule) is no em dash in any commit message, code comment, or i18n string written by this plan. Comments and test descriptions across all 14 tasks used em dashes; commit messages were already clean. Replace each occurrence with a colon (heading/explanation pattern) or a comma (parenthetical aside), whichever reads correctly for that specific line. Text-only change, touches only the lines this plan itself introduced: pre-existing comments elsewhere in the same files that already contained an em dash are left untouched, out of scope for this fix round.
Tracks the plan behind the 14 fixes responding to the product and code review round on PR164 (root-cause watcher fix, secrets-rescan restoration, gate/decision-modal correctness, dev-server CORS, foreign-hook UI state, focus trap, and several smaller findings).
main switched apps/desktop's Vitest default environment from jsdom to node, with DOM-touching files opting in via a file-header directive (v3.6.6 perf work). These 9 test files construct KeyboardEvent/mount components/touch document and need the same opt-in, or they fail with "document is not defined" / "KeyboardEvent is not defined" post-merge.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Combined review surface for ROADMAP.md's v3.7.0 "Commit Review" feature, superseding the three stacked PRs it was built and reviewed across:
.gitwandrcopt-in + composable pre-commit hookEach of the three went through its own implement → adversarial-verify → fix cycle (see their individual PR descriptions/history for the detailed back-and-forth). This PR exists purely to give reviewers one place to see the whole feature's diff at once, end to end, before it lands in a release.
What ships
n/p, dismiss withx, per-file finding counts in the staged list.GitWand-Review: ran|vouched|skipped (iter:N, coverage:X%)commit trailer..gitwandrccommitReviewblock can force the feature on or off independently of the app Setting, in both directions.Everything is opt-in and off by default; no new Tauri commands anywhere in the whole feature.
Test plan
pnpm --filter @gitwand/core run test -- --run— 1066/1066 passingcd apps/desktop && pnpm test -- --run— 860/860 passingcd apps/desktop && pnpm build— cleanvue-tsc --noEmit+vite buildclaude; not fully verified forcodex/opencode— documented follow-up, not a blocker