From 4c514fe2be915c8c80cc6768f26b2fc514925924 Mon Sep 17 00:00:00 2001 From: Jacek Tomaszewski Date: Tue, 1 Sep 2026 13:27:21 +0200 Subject: [PATCH] =?UTF-8?q?fix(cockpit):=20the=20walk=20drops=20a=20review?= =?UTF-8?q?=20the=20moment=20you=20settle=20it,=20so=20=E2=80=B9=20never?= =?UTF-8?q?=20returns=20you=20to=20a=20skipped=20PR?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- SPEC.md | 7 +++++++ docs/lifecycle.md | 9 ++++++++- web/src/Detail.tsx | 29 ++++++++++++++++++++++++----- web/src/inbox.test.ts | 29 +++++++++++++++++++++++++++++ web/src/inbox.ts | 19 +++++++++++++++++++ 5 files changed, 87 insertions(+), 6 deletions(-) diff --git a/SPEC.md b/SPEC.md index 349dafa..f09b98c 100644 --- a/SPEC.md +++ b/SPEC.md @@ -1370,6 +1370,13 @@ within a band) — MUST back the prev/next walk, the arrival bell and the favicon dot. These are one invariant, not three coincidences: the dot on the tab, the popup, and the arrows all answer "what still wants me?". +The walk is a snapshot taken when a review opens and MUST NOT be refetched +under the reader. A review the reader settles during the walk — skipped, +marked reviewed, or sent — MUST leave that snapshot at once, so no arrow +walks back into a decision just made; the open review MUST keep its place in +it whatever its own status became, since the arrows and the position count +read from it. + ### 17.4 The Arrival Bell and Favicon The browser bell polls the queue from every screen and notifies once per new diff --git a/docs/lifecycle.md b/docs/lifecycle.md index beb22ac..77243ca 100644 --- a/docs/lifecycle.md +++ b/docs/lifecycle.md @@ -263,7 +263,14 @@ awaits you" over a poll that had just counted two. **Sort order** (`STATUS_ORDER`): `ready`, `awaiting`, `running`, `failed`, `reviewed`, `skipped`, `sent` — newest first inside each band. The `‹ ›` arrows -walk `walkable()`: open, unsettled rows only. +walk `walkable()`: open, unsettled rows only. That list is fetched once, when +the review opens — finishing one must not renumber the walk under you — so a +review you settle *during* the walk (skipped, marked reviewed, or sent) is +dropped from the snapshot in the browser instead (`walkFrom`, used by +`web/src/Detail.tsx`); otherwise `‹` off the next review would walk straight +back into the PR you just skipped. The review you have open keeps its place +whatever you did to it, because it is what the arrows and the "n of m" count +read their position from, and a send leaves you standing on it. ### Things that remove a row without you touching it diff --git a/web/src/Detail.tsx b/web/src/Detail.tsx index 82b0292..ac6f9aa 100644 --- a/web/src/Detail.tsx +++ b/web/src/Detail.tsx @@ -22,7 +22,7 @@ import { import { highlightDiff } from "./highlight"; import { Icon, IconName, Key } from "./Icon"; import { Markdown } from "./Markdown"; -import { walkable } from "./inbox"; +import { walkFrom } from "./inbox"; import { EVENT_LABEL, EVENT_TONE, @@ -1332,6 +1332,11 @@ export function Detail({ reviewKey }: { reviewKey: string }) { const [freshnessError, setFreshnessError] = useState(null); const [rerunning, setRerunning] = useState(false); const [neighbours, setNeighbours] = useState([]); + // The reviews settled since this page opened — skipped, marked reviewed, or + // sent. Kept here rather than refetched, so a decision leaves the walk the + // moment you make it without the rest of the list moving. + const [settledHere, setSettledHere] = useState>(new Set()); + const markSettled = (key: string) => setSettledHere((s) => new Set(s).add(key)); // What the user has pointed at with "discuss this", waiting to be sent with // their next message. Lives here so a button anywhere in the walkthrough can // reach the one chat panel at the bottom. @@ -1456,8 +1461,14 @@ export function Detail({ reviewKey }: { reviewKey: string }) { const readOnly = artifact?.sent != null; // The walk is the queue as it stood when this page opened. Deliberately not // refetched: finishing this review must not renumber the walk under you or - // strand the arrows on a list this PR has just left. - const walk = useMemo(() => walkable(neighbours), [neighbours]); + // strand the arrows on a list this PR has just left. What it does drop is + // the reviews you settled on the way through — the snapshot still calls them + // ready, and walking ‹ back into the PR you just skipped is cerber asking + // you to decide it twice. + const walk = useMemo( + () => walkFrom(neighbours, reviewKey, settledHere), + [neighbours, reviewKey, settledHere], + ); const at = walk.findIndex((r) => r.key === reviewKey); const prev = (at > 0 ? walk[at - 1] : null) ?? null; const next = (at >= 0 && at < walk.length - 1 ? walk[at + 1] : null) ?? null; @@ -1555,7 +1566,12 @@ export function Detail({ reviewKey }: { reviewKey: string }) { setSending(true); setSendError(null); sendReview(reviewKey, event) - .then(setArtifact) + .then((a) => { + setArtifact(a); + // A send settles this review too — it stays on screen showing what + // landed, but the arrows have no reason to come back to it. + markSettled(reviewKey); + }) .catch((e) => setSendError(String(e.message ?? e))) .finally(() => setSending(false)); }; @@ -1595,7 +1611,10 @@ export function Detail({ reviewKey }: { reviewKey: string }) { /** Settle this review locally and move on. Stays put if the write failed. */ const settle = (status: "reviewed" | "skipped") => patchReview(reviewKey, { status }) - .then(advance) + .then(() => { + markSettled(reviewKey); + advance(); + }) .catch((e) => setError(String(e))); const onUpdateComment = (id: string, patch: { body?: string; status?: string }) => apply(patchComment(reviewKey, id, patch)); diff --git a/web/src/inbox.test.ts b/web/src/inbox.test.ts index 32842b1..d7bb21d 100644 --- a/web/src/inbox.test.ts +++ b/web/src/inbox.test.ts @@ -17,6 +17,7 @@ import { tabOf, verdictCell, walkable, + walkFrom, } from "./inbox"; import { DaemonStatus, ReviewListItem } from "./types"; @@ -108,6 +109,34 @@ describe("walkable", () => { }); }); +describe("walkFrom", () => { + const three = [ + row({ key: "a", status: "ready" }), + row({ key: "b", status: "ready" }), + row({ key: "c", status: "ready" }), + ]; + + it("walks the whole snapshot while you have settled nothing", () => { + expect(walkFrom(three, "b", new Set()).map((r) => r.key)).toEqual(["a", "b", "c"]); + }); + + // The reported case: skip a review, land on the next one, press ‹ — and the + // PR you just skipped comes back. + it("steps past a review you skipped on the way here", () => { + expect(walkFrom(three, "b", new Set(["a"])).map((r) => r.key)).toEqual(["b", "c"]); + }); + + it("keeps the review you are on, so it still has a place to walk from", () => { + // Sending leaves you on the page: the walk it reads its position and its + // "next review" button from has to still contain it. + expect(walkFrom(three, "b", new Set(["b"])).map((r) => r.key)).toEqual(["a", "b", "c"]); + }); + + it("says the walk is over once everything else is settled", () => { + expect(walkFrom(three, "b", new Set(["a", "b", "c"])).map((r) => r.key)).toEqual(["b"]); + }); +}); + describe("hiddenAwaiting", () => { // The reported case: the strip counted two, the queue showed none. const settledPair = [ diff --git a/web/src/inbox.ts b/web/src/inbox.ts index 570ffb7..13f6dad 100644 --- a/web/src/inbox.ts +++ b/web/src/inbox.ts @@ -87,6 +87,25 @@ export function walkable(list: ReviewListItem[]): ReviewListItem[] { return sortReviews(list.filter((r) => !isArchived(r) && !SETTLED.includes(r.status))); } +/** + * The walk as it stands from the review you have open, given what you have + * settled since the page opened. + * + * The list is a snapshot taken on arrival — cerber deliberately doesn't + * renumber the walk under you while you read — so it still calls a PR you + * skipped a moment ago ready, and ‹ would walk straight back into the + * decision you just made. The review you are on keeps its place whatever you + * did to it: it is where both arrows and the count read their position from, + * and after a send it is still the page you are standing on. + */ +export function walkFrom( + list: ReviewListItem[], + currentKey: string, + settledHere: ReadonlySet, +): ReviewListItem[] { + return walkable(list).filter((r) => r.key === currentKey || !settledHere.has(r.key)); +} + /** * The reviews GitHub still wants from you that the queue is not showing. *