Skip to content

Commit df94bb0

Browse files
committed
Count decisions as decisions, and land the badge where it points
The dashboard's Needs-you widget said "0 of 12 decided" to someone who had dismissed five findings and could only ever see eight: it counted confirmations, called them decisions, and sized the queue with findings the quotation check had already hidden. It now counts confirmed plus dismissed, out of the queue a person is actually handed. The awaiting badge sat on every screen and landed on every review ever run, because the reviews list held its filter in component state where no link could reach it. The list reads the filter from the URL now and the badge points at it, which the toggle says out loud through aria-pressed rather than through colour alone. The projects widget was fetching the last review's date, its id and the review count and drawing none of them. All three are on screen, the outcome opens the review it describes, and every row offers the branch review the plan asked for.
1 parent c09b2ad commit df94bb0

7 files changed

Lines changed: 157 additions & 26 deletions

File tree

docs/DECISIONS.md

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1921,3 +1921,31 @@ verified evidence, in writing, here.
19211921
dashboard is the app's front door and was not in it, and the queue's
19221922
caption claimed a stage timeline and a coverage panel that the shot no
19231923
longer contains, both having moved inside the Run detail fold.
1924+
- 2026-08-18 D-84 (review pass over the dashboard shell, M1-M6): six numbered
1925+
findings, all fixed in one diff before any new work started.
1926+
1. The Needs-you widget said "N of M decided" and counted neither. N was the
1927+
confirmed findings, so someone who had dismissed an entire queue was told
1928+
they had not started it; M included findings the quotation check killed,
1929+
which the queue never shows, so the target could not be reached. The
1930+
scoreboard now carries `decided` (confirmed plus dismissed) and a total that
1931+
is the size of the queue a person will actually be handed.
1932+
2. The awaiting badge in the top bar linked to `/reviews`, unfiltered, which
1933+
is the list someone pressing a count of three wants least. The filter lived
1934+
only in component state, so nothing could link to it. The reviews list now
1935+
seeds its filter from `?status=awaiting` and the badge points there.
1936+
3. The projects widget fetched the last review's timestamp and id and the
1937+
per-project review count, and rendered none of the three, while the plan's
1938+
own row asked for "when" and a way to review a branch. All three are on
1939+
screen now: the age, the count when there is more than one, an outcome chip
1940+
that opens the review it describes, and a Review link per row.
1941+
4. That widget's tally read "3 of 12" with no word saying what the numbers
1942+
were, where every other surface names its number. It says confirmed.
1943+
5. The dashboard's one-second clock was keyed to the polled object rather
1944+
than to whether anything is running, so every four-second poll tore the
1945+
interval down and started it again.
1946+
6. The e2e assertion for finding 2 was written first as a row scrape over
1947+
`ul > li`, which matches the projects list on the page it was navigating
1948+
away from: it passed against the old DOM and then read an empty list, which
1949+
is a flake and stop-the-line. Root-caused rather than retried, and replaced
1950+
with a wait on the URL plus the filter toggle's own `aria-pressed`, which
1951+
the toggle now carries because it is a toggle.

e2e/failure-paths.spec.ts

Lines changed: 16 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -145,7 +145,21 @@ test("the top bar says how many reviews are waiting on a person", async ({ page
145145
await page.goto("/projects");
146146
const badge = page.getByLabel(/awaiting your decision/);
147147
await expect(badge).toBeVisible({ timeout: 30_000 });
148-
// A link, not an ornament: the count is also the way to the queue.
149-
await expect(badge).toHaveAttribute("href", "/reviews");
148+
// A link, not an ornament: the count is also the way to the queue. It
149+
// asserted a bare "/reviews" until 2026-08-18, which landed on every review
150+
// ever run: the list someone pressing a count of three wants least.
151+
await expect(badge).toHaveAttribute("href", "/reviews?status=awaiting");
150152
await expect(badge).toContainText(/\d+/);
153+
154+
// And the filter is actually applied on arrival, rather than the parameter
155+
// being decoration on the href. Asserted through the toggle's own pressed
156+
// state rather than by counting rows: how many reviews are waiting depends
157+
// on what the tests before this one left behind, and the question here is
158+
// whether the link arrives filtered.
159+
await badge.click();
160+
await expect(page).toHaveURL(/status=awaiting/);
161+
await expect(page.getByRole("button", { name: /awaiting your decision/ })).toHaveAttribute(
162+
"aria-pressed",
163+
"true",
164+
);
151165
});

src/app/page.tsx

Lines changed: 37 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -40,7 +40,7 @@ interface AwaitingRow {
4040
projectName: string;
4141
fromBranch: string;
4242
intoBranch: string;
43-
confirmed: number;
43+
decided: number;
4444
total: number;
4545
since: string;
4646
}
@@ -202,12 +202,15 @@ export default function HomePage() {
202202
}, []);
203203

204204
// Ticks only while something is running, so a quiet dashboard is not
205-
// re-rendered once a second for ever.
205+
// re-rendered once a second for ever. Keyed to whether a run exists rather
206+
// than to the polled object, which is a new identity every four seconds and
207+
// would restart the clock each time it arrived.
208+
const running = live !== null;
206209
useEffect(() => {
207-
if (!live) return;
210+
if (!running) return;
208211
const tick = setInterval(() => setNow(Date.now()), 1000);
209212
return () => clearInterval(tick);
210-
}, [live]);
213+
}, [running]);
211214

212215
useEffect(() => {
213216
let cancelled = false;
@@ -359,9 +362,7 @@ export default function HomePage() {
359362
</span>
360363
</span>
361364
<span className="text-dense tabular-nums">
362-
{row.total > 0
363-
? `${row.confirmed} of ${row.total} decided`
364-
: "no findings"}
365+
{row.total > 0 ? `${row.decided} of ${row.total} decided` : "no findings"}
365366
<span className="ml-3 text-[var(--color-ink-muted)]">
366367
waiting {ago(row.since, now)}
367368
</span>
@@ -482,20 +483,41 @@ export default function HomePage() {
482483
{project.name}
483484
</Link>
484485
{project.latest ? (
485-
<span className="flex items-center gap-2 text-aside">
486-
<Badge tone={statusTone(project.latest.status)}>
487-
{statusLabel(project.latest.status)}
488-
</Badge>
486+
<span className="flex flex-wrap items-center gap-x-2 gap-y-1 text-aside">
487+
{/* The outcome is the way into the review that
488+
produced it: a status worth reading is a status
489+
worth opening. */}
490+
<Link href={`/reviews/${project.latest.id}`}>
491+
<Badge tone={statusTone(project.latest.status)}>
492+
{statusLabel(project.latest.status)}
493+
</Badge>
494+
</Link>
489495
<span className="text-[var(--color-ink-muted)] tabular-nums">
490496
{project.latest.total > 0
491-
? `${project.latest.confirmed} of ${project.latest.total}`
497+
? `${project.latest.confirmed} of ${project.latest.total} confirmed`
492498
: "no findings"}
493499
</span>
500+
{/* When, and how many times. A project reviewed once
501+
last March is a different thing from one reviewed
502+
nine times this week, and the widget said neither. */}
503+
<span className="text-[var(--color-ink-faint)]">
504+
{ago(project.latest.at, now)}
505+
{project.reviews > 1 ? ` · ${project.reviews} reviews` : ""}
506+
</span>
507+
<Link
508+
href={`/reviews/new?projectId=${project.id}`}
509+
className="text-[var(--color-accent)] underline underline-offset-2"
510+
>
511+
Review
512+
</Link>
494513
</span>
495514
) : (
496-
<span className="text-aside text-[var(--color-ink-faint)]">
497-
never reviewed
498-
</span>
515+
<Link
516+
href={`/reviews/new?projectId=${project.id}`}
517+
className="text-aside text-[var(--color-accent)] underline underline-offset-2"
518+
>
519+
Review a branch
520+
</Link>
499521
)}
500522
</li>
501523
))}

src/app/reviews/page.tsx

Lines changed: 32 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,8 @@
1010
*/
1111

1212
import Link from "next/link";
13-
import { useCallback, useEffect, useState } from "react";
13+
import { useSearchParams } from "next/navigation";
14+
import { Suspense, useCallback, useEffect, useState } from "react";
1415
import { PageBody, PageHeader } from "@/components/page";
1516
import {
1617
Badge,
@@ -57,10 +58,16 @@ function ranFor(startedAt: string | null, completedAt: string | null): string |
5758
/** Statuses that own a running process, so the row offers no delete. */
5859
const RUNNING = new Set(["running", "verifying"]);
5960

60-
export default function ReviewsPage() {
61+
function ReviewsList() {
62+
const search = useSearchParams();
6163
const [reviews, setReviews] = useState<Row[] | null>(null);
6264
const [project, setProject] = useState("");
63-
const [status, setStatus] = useState("");
65+
// Seeded from the URL so the awaiting badge in the top bar can land on the
66+
// filtered list rather than on every review ever run, which is the list
67+
// someone pressing a count of three is least interested in.
68+
const [status, setStatus] = useState(() =>
69+
search.get("status") === "awaiting" ? "awaiting" : "",
70+
);
6471
const [confirming, setConfirming] = useState("");
6572
const [busy, setBusy] = useState("");
6673
const [error, setError] = useState("");
@@ -129,6 +136,9 @@ export default function ReviewsPage() {
129136
{awaiting > 0 ? (
130137
<Button
131138
variant={status === "awaiting" ? "primary" : "quiet"}
139+
// It is a toggle, so it says whether it is on rather than
140+
// leaving that to colour alone.
141+
aria-pressed={status === "awaiting"}
132142
onClick={() => setStatus(status === "awaiting" ? "" : "awaiting")}
133143
>
134144
{awaiting} awaiting your decision
@@ -249,3 +259,22 @@ export default function ReviewsPage() {
249259
</>
250260
);
251261
}
262+
263+
export default function ReviewsPage() {
264+
// Required because the list reads a search param. The fallback is the header
265+
// it is about to render anyway, so nothing flashes empty.
266+
return (
267+
<Suspense
268+
fallback={
269+
<>
270+
<PageHeader title="Reviews" />
271+
<PageBody>
272+
<p className="text-body text-[var(--color-ink-muted)]">Reading the reviews...</p>
273+
</PageBody>
274+
</>
275+
}
276+
>
277+
<ReviewsList />
278+
</Suspense>
279+
);
280+
}

src/components/topbar.tsx

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -140,7 +140,7 @@ export function TopBar() {
140140
<div className="ml-auto flex shrink-0 items-center gap-2">
141141
{awaiting > 0 ? (
142142
<Link
143-
href="/reviews"
143+
href="/reviews?status=awaiting"
144144
aria-label={`${awaiting} awaiting your decision`}
145145
className="flex items-center gap-1.5 rounded-md border border-[var(--color-question)] bg-[var(--color-question-soft)] px-2 py-1 text-aside font-medium text-[var(--color-question)]"
146146
>

src/server/db/repositories/scoreboard.ts

Lines changed: 21 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,17 @@ export interface AwaitingReview {
4545
projectName: string;
4646
fromBranch: string;
4747
intoBranch: string;
48-
confirmed: number;
48+
/**
49+
* Confirmed or dismissed. Both are decisions, and counting only the
50+
* confirmed ones told someone who had dismissed a whole queue that they
51+
* had not started it.
52+
*/
53+
decided: number;
54+
/**
55+
* The size of the queue this person will actually be shown, so killed
56+
* findings are not in it: the quotation check hides those, and a total
57+
* that counts them can never be reached.
58+
*/
4959
total: number;
5060
/** When the review stopped and started waiting on a person. */
5161
since: string;
@@ -107,16 +117,22 @@ export function readScoreboard(db: Db, recentLimit = 12): Scoreboard {
107117
.from(findings)
108118
.all();
109119

110-
const byReview = new Map<string, { confirmed: number; total: number }>();
120+
const byReview = new Map<
121+
string,
122+
{ confirmed: number; decided: number; queued: number; total: number }
123+
>();
111124
const byRule = new Map<string, RuleYield>();
112125
let confirmed = 0;
113126
let dismissed = 0;
114127
let killed = 0;
115128

116129
for (const row of findingRows) {
117-
const tally = byReview.get(row.reviewId) ?? { confirmed: 0, total: 0 };
130+
const tally = byReview.get(row.reviewId) ?? { confirmed: 0, decided: 0, queued: 0, total: 0 };
118131
tally.total += 1;
119132
if (row.status === "confirmed") tally.confirmed += 1;
133+
if (row.status === "confirmed" || row.status === "dismissed") tally.decided += 1;
134+
// Everything the queue will show. A killed finding is not in it.
135+
if (row.status !== "killed") tally.queued += 1;
120136
byReview.set(row.reviewId, tally);
121137

122138
if (row.status === "confirmed") confirmed += 1;
@@ -187,8 +203,8 @@ export function readScoreboard(db: Db, recentLimit = 12): Scoreboard {
187203
projectName: review.projectName,
188204
fromBranch: review.fromBranch,
189205
intoBranch: review.intoBranch,
190-
confirmed: byReview.get(review.id)?.confirmed ?? 0,
191-
total: byReview.get(review.id)?.total ?? 0,
206+
decided: byReview.get(review.id)?.decided ?? 0,
207+
total: byReview.get(review.id)?.queued ?? 0,
192208
// When it stopped, which is when the waiting began. A review that
193209
// never finished a run has only its creation to go on.
194210
since: review.completedAt ?? review.createdAt,

tests/server/db/scoreboard.test.ts

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -158,6 +158,28 @@ describe("what the dashboard widgets need", () => {
158158
expect(board.awaitingReviews[0]?.fromBranch).toBe("feature/x");
159159
});
160160

161+
it("counts a dismissal as a decision, and leaves killed findings out of the queue", () => {
162+
// The widget's whole job is saying how much is left. Counting only the
163+
// confirmed ones told someone who had dismissed a queue that they had not
164+
// started it, and counting killed findings in the total set a target the
165+
// queue never shows and so can never reach.
166+
const kept = candidate();
167+
const dropped = candidate({ issue: "Second" });
168+
const untouched = candidate({ issue: "Third" });
169+
for (const finding of [kept, dropped, untouched]) markVerified(db, finding.id, evidence);
170+
confirmFinding(db, kept.id);
171+
dismissFinding(db, dropped.id, "Guarded upstream.");
172+
markKilled(db, candidate({ issue: "Invented" }).id, "Quoted code that was not there.");
173+
174+
transitionReview(db, reviewId, "running");
175+
transitionReview(db, reviewId, "verifying");
176+
transitionReview(db, reviewId, "awaiting_confirmation");
177+
178+
const row = readScoreboard(db).awaitingReviews[0];
179+
expect(row?.decided).toBe(2);
180+
expect(row?.total).toBe(3);
181+
});
182+
161183
it("keeps two projects with the same name apart", () => {
162184
// Two remotes can end in the same repository name. Matching on it would
163185
// report one project's last finding as another's.

0 commit comments

Comments
 (0)