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. *