Repository navigation
feat: flag what changed since the reviewer's last look - #89
Merged
Merged
Conversation
Each review the reviewer opens records its head commit in the pull request's cache folder. On a later open, the engine compares the change at that commit with the change now, each against its own merge base and by each hunk's changed lines alone, so a rebase or a force-push mixes in nothing from upstream; the result carries the pieces that changed. When GitHub no longer has that commit, the result says so and every part counts as changed. With no local record, the last look is the commit of the reviewer's last submitted GitHub review. The tree flags each part changed since the last look, a filter button shows only those parts, and the line above the tree and the overview say which commit the last look was at and how many parts changed. Closes #46
…reword not-compared outcome
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.
Intent
Implements "Since your last look" (closes #46). The companion records the head commit each time the reviewer opens a review. On a later open, the parts changed since that last look are flagged, and a filter shows only those parts. After a force-push or rebase, the change itself is compared rather than the raw commits, so upstream changes are not mixed in. If the old commit can no longer be compared, the companion says so and treats every part as changed. Without a local record, the last look falls back to the commit of the reviewer's last submitted GitHub review.
Acceptance criteria: tests cover a plain push, a rebase, a force-push with an edit, and a missing old commit; the filter shows only changed parts, and the panel says which commit the last look was at.
What Changed
sinceLastLook(schema version 16).Risk Assessment
✅ Low: The feature is well-bounded, mirrors the existing reviewed-marks store patterns, is covered by behavioral tests against real git repositories and a fake GitHub, keeps all new GitHub access read-only under the reviewer's token with only local commit-id/timestamp storage, and every substantiated finding is an info-level edge case or docs nit.
Testing
Drove the engine's last-look scenarios live earlier in this run (recorded evidence covers plain push, rebase with/without an edit, force-push that rebased and edited, old commit gone, 422 compare, and the GitHub-review fallback) and confirmed them again through the engine unit tests; the extension side was validated per the operator's decision through its unit and stub-based integration tests plus rendered output, showing the flagged part, the filter showing only it, the commit-naming line above the tree and in the overview panel, and the corrected could-not-be-compared wording; everything passed and the worktree is clean. Per the live-validation contract, the scenarios covered only by unit, stub, or rendered-output evidence (no live product drive in this run) are reported as untested, not pass.
Evidence: Live engine protocol: first look records, plain push flags only the edit
Evidence: Live engine protocol: rebase then one real edit flags only the edit
Evidence: Live engine protocol: rebase onto newer master flags nothing
Evidence: Live engine protocol: force-push that rebased and edited flags only the edit
Evidence: Live engine protocol: old commit gone counts everything as changed
Evidence: Live engine protocol: compare answers 422 counts everything as changed
Evidence: Live engine protocol: no local record falls back to last GitHub review
Evidence: Rendered extension tree: flagged part, filter showing only it, message lines naming the last-look commit, not-compared wording
message: Since your last look at abcdef0 on 2026-10-01: 1 of 7 parts changed. ▸ Must review src/retry.py — changed since your last look · New code the send path now runs on every delivery. … filtered: ▸ Must review src/retry.py — changed since your last look · New code the send path now runs on every delivery. message: … Showing only those.Evidence: Rendered overview panel HTML: since line under the meta, compared and not-compared variants
Since your last look at abcdef0 on 2026-10-01: 1 of 7 parts changed. / The change could not be compared with your last look at abcdef0 on 2026-10-01, because that commit is gone or no longer related: every part counts as changed.Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
packages/extension/src/overview.ts:298- The doc comment "A chip for each stage done, and one for the stage still running." now sits stacked above sinceLine()'s own doc instead of on stageChips() (overview.ts:305), so it documents the wrong function and stageChips is left undocumented. Mechanical move of the comment to stageChips; no behavior change.packages/extension/src/extension.ts:260- ReviewSession.onlyChanged is never reset when a review of a different pull request shows its first result (show()'s !update branch, extension.ts:391-396). Concrete sequence: the reviewer toggles the filter on PR A, then reviews PR B; on PR B's second look sinceLastLook is defined, so the tree silently starts filtered (buildTree at extension.ts:414 and the "Showing only those." message at extension.ts:426) without the reviewer ever pressing the button for PR B. The message line does disclose it, so impact is mild; the remedy — clearing onlyChanged when a new review's first result arrives — is a user-visible behavior decision, so it needs the author's call rather than an automated change.packages/engine/src/last-look.ts:101- changePieces hashes with apart=false, so two identical hunks of one file share a single hash. Concrete sequence: a file already holds a hunk adding lines A at the last look; the author then adds a second, identical hunk elsewhere in the same file. compareWith (last-look.ts:153-154) dedupes both to one piece that the old change already held, so sinceLastLook.changed is empty, the panel reads "0 parts changed" and the filter hides the genuinely new addition. This is the documented tradeoff for regrouping stability (a part holding only one of the identical hunks must hash it the same as the whole-file computation), and the same content was already reviewed once; per-file occurrence numbering cannot be reconstructed from a part subset. Noted as an accepted limitation, not a defect to harden.packages/engine/src/github.ts:243- getChangeDiff maps any 422 from compareCommitsWithBasehead to the 'commit gone' outcome, but 422 also occurs when base and head have no common ancestor (for example after the pull request's base branch was switched to an unrelated branch) — a state where the old head commit still exists on GitHub. The panel and tooltips (packages/extension/src/tree.ts:302-305, 337-339) then assert "GitHub no longer has <commit>", which is factually wrong, though the everything-counts-as-changed fallback is the safe direction. Rare edge; distinguishing 422 causes from the error body would be the only remedy.🔧 Fix applied.
1 info still open:
packages/engine/src/last-look.ts:101- changePieces hashes with apart=false, so two identical hunks of one file share a single hash. Concrete sequence: a file already holds a hunk adding lines A at the last look; the author then adds a second, identical hunk elsewhere in the same file. compareWith (last-look.ts:153-154) dedupes both to one piece that the old change already held, so sinceLastLook.changed is empty, the panel reads "0 parts changed" and the filter hides the genuinely new addition. This is the documented tradeoff for regrouping stability (a part holding only one of the identical hunks must hash it the same as the whole-file computation), and the same content was already reviewed once; per-file occurrence numbering cannot be reconstructed from a part subset. Noted as an accepted limitation, not a defect to harden.git -C ~/.no-mistakes/worktrees/a09930ebf9a5/01M4AQ2XR00WWP1NZ0YK180005 statusandgit -C ~/.no-mistakes/worktrees/a09930ebf9a5/01M4AQ2XR00WWP1NZ0YK180005 diff). Respond with fix to validate it, or abort.🔧 No changes applied.
1 warning still open:
npx vitest run packages/engine/test/last-look.test.ts— 10 passed (first look records, plain push, rebase, force-push+edit, old commit gone, 422 no-common-history, GitHub-review fallback, same-commit re-open, corrupt record, hunk hashing)npx vitest run packages/engine/test/server.test.ts packages/engine/test/review.test.ts packages/engine/test/cli.test.ts— 67 passed (protocol carries sinceLastLook; looks recorded beside reviewed marks)npx vitest run packages/extension/test/tree.test.ts packages/extension/test/overview.test.ts packages/extension/test/review-result.test.ts packages/extension/test/engine-client.test.ts— 151 passed (flag descriptions/tooltips, filter, since line, overview meta line, protocol guard, wiring)npx vitest run --config vitest.integration.config.ts -t 'since your last look'— 3 passed against the vscode stub and a spawned fake engine: flag+filter+message line, first-look message, filter kept across re-reviews of one PR and cleared for another PRRendered-output check: bundled the realoverviewHtml,buildTreeandtreeMessagewith the realmixedResultfixture via repo-local esbuild and wrote rendered artifacts (overview HTML with the since line in compared and not-compared variants; tree text unfiltered, filtered, and not-compared) into the evidence directoryCleanup per operator instruction:rm -rf .live-tmp .vscode-test— the worktree is clean with no untracked filesdocs/ux/README.md:22- The frozen design record's 'Second visit' screen says 'The diff shows only what changed since the last look, and the companion says which findings were resolved and who replied'; the shipped feature filters the review tree (not the diff) and does not report resolved findings or replies. docs/ux is a record of the design comparison, like an ADR, and README step 7 now owns the current behavior, so I left it untouched; whether to annotate the divergence in the design record is the author's call.✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.