fix(cockpit): ‹ no longer walks back into a PR you just skipped - #48
Merged
Conversation
…never returns you to a skipped PR The prev/next arrows walk a snapshot of the queue taken when the review opened — deliberately, so finishing one doesn't renumber the walk under you. But the snapshot still called a PR you had just skipped ready, so skipping and pressing ‹ landed you straight back on it. The snapshot stays frozen; what leaves it is what you settled while walking (skipped, reviewed, or sent). The review you have open keeps its place, because the arrows and the "n of m" count read from it and a send leaves you standing there. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR fixes queue navigation in the cockpit detail view so the ‹ › arrows no longer walk back into a PR you just settled (skipped/reviewed/sent) while still preserving the “frozen snapshot” invariant for the walk taken when the review page opens.
Changes:
- Add
walkFrom()to derive the walk from the initial snapshot while filtering out reviews settled during the current walkthrough (except the currently open review). - Track “settled during this page session” keys in
Detail.tsxand applywalkFrom()for arrow navigation / position counting. - Add focused unit tests for
walkFrom()and update spec/docs to reflect the refined walk rules.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| web/src/inbox.ts | Introduces walkFrom() to filter the walk snapshot by locally-settled keys while keeping the current review in the list. |
| web/src/inbox.test.ts | Adds four new unit tests covering the expected walkFrom() behaviors (including the reported regression). |
| web/src/Detail.tsx | Tracks settledHere and switches arrow-walk computation from walkable() to walkFrom(); marks current review settled after successful settle/send. |
| SPEC.md | Updates the normative spec to state that settled-in-walk reviews are removed from the snapshot immediately, with the current review retained. |
| docs/lifecycle.md | Updates lifecycle documentation to describe the new walk behavior and point to walkFrom usage in the detail view. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
🎉 This PR is included in version 0.27.3 🎉 The release is available on: Your semantic-release bot 📦🚀 |
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
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.
The problem
Skip a review in the cockpit and you land on the next one, as you should. Press
‹there and the PR you just skipped comes back — the arrows walk you into adecision you have already made, and there is no way to tell from the page that
this one is settled.
The fix
The
‹ ›arrows walk a snapshot of the queue, fetched once when the reviewpage opens. That is deliberate: finishing a review must not renumber the walk
under the reader or strand the arrows on a list this PR has just left. But the
snapshot still called the skipped PR
ready, sowalkable()kept it and thearrows kept offering it.
The snapshot stays frozen. What now leaves it is what you settled while walking
— skipped, marked reviewed, or sent — tracked in the page's own state and
filtered out by a new
walkFrom()inweb/src/inbox.ts.Technical details
walkFrom(list, currentKey, settledHere)=walkable(list)minus thereviews settled since the page opened, except the one you have open. That
exception is load-bearing: the current review is where the arrows and the
"n of m awaiting" count read their position from, and a send leaves you
standing on the page it settled — drop it and the send panel's
"next review: repo#123" button degrades to "back to the queue".
Detail.tsxmarks a review settled in the same place it settles it:settle()(skip / mark reviewed) and a successfuldoSend().advance()still reads
nextfrom the render that ran before the mark, so the moveforward is unchanged.
"n of m awaiting" counter stays honest for free.
Verification
web/src/inbox.test.tscoverwalkFrom: the untouchedwalk, the reported case (skip
a, stand onb,ais gone), the currentreview keeping its place when it is itself settled, and the everything-else-
settled end of the walk.
pnpm typecheck && pnpm test && pnpm buildall pass (531 tests).Docs
docs/lifecycle.mdandSPEC.md§17.3 describe the walk, so both say the newrule: the snapshot is not refetched under the reader, a review settled during
the walk leaves it at once, and the open review keeps its place whatever its
status became.
🤖 Generated with Claude Code