3523dcf93e
plans/ held 58 documents and exactly one said whether it was open. The rest
mixed finished work, reviews of shipped work, parked specs and genuinely
pending ones, with nothing distinguishing them, so "how many plans are in
the queue" had no answer short of reading all 58.
Now `grep -H '^Status:' plans/*.md` is the answer:
50 done 3 in progress 2 planned 2 reference 1 parked
Statuses were derived rather than guessed: CLAUDE.md's own built list and
"What's NOT built yet" section, plus checking the subject exists in the
code. A review of work that shipped counts as done -- it records what was
found, it is not a request for anything. `reference` separates the two docs
that are conventions rather than work items (admin-design-standards,
admin-work-framework), which otherwise read as permanently-open plans.
The vocabulary is deliberately five words. A larger one invites "mostly
done" and "blocked-ish", which is how the directory became unreadable.
test_plans_declare_status.py keeps it from rotting: a new plan without a
marker fails, as does an unknown status, one buried below the eighth line,
or an open status with no reason -- "planned" alone is the state that rots,
since nobody can tell later whether it waits on a decision, a dependency,
or just nobody's turn.
Also updates the sweep plan with what landed and what did not, including
that #9 was not a defect.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VRQXz5SYZYVWscxS1QqF6U
99 lines
4.4 KiB
Markdown
99 lines
4.4 KiB
Markdown
# Review: verdict-bar chart breaks on its first live update
|
|
|
|
Status: done -- review of shipped work
|
|
|
|
**Scope:** just this one defect, introduced in `906c9e6`
|
|
("fix(admin): replace stretched verdict doughnut with partition bar and
|
|
split history into per-metric mini charts"), in
|
|
`admin/frontend/index.html`. Not a review of the rest of that commit (the
|
|
history mini-charts split was checked separately and is sound).
|
|
|
|
## Verdict: real regression, invisible on first load, breaks on every subsequent poll
|
|
|
|
`906c9e6` replaced the Verdict Mix doughnut with a horizontal "partition
|
|
bar" — one Chart.js `bar` dataset holding all N verdict counts, rendered as
|
|
a single row split into colored segments. That design depends on the chart
|
|
always having exactly **one** category on its index axis, with all N values
|
|
living inside that one dataset's `data` array.
|
|
|
|
Chart creation gets this right. `renderChart`'s `verdict-bar` branch
|
|
(`index.html:844-857`) ignores whatever `labels` it was called with and
|
|
hardcodes a single-element array:
|
|
|
|
```js
|
|
return new Chart(canvas, { type: 'bar', data: { labels: [''], datasets }, options: opts });
|
|
```
|
|
|
|
But `updateChart` — the path taken on every render *after* the first,
|
|
since `renderVerdict` calls it whenever `verdictChart` already exists
|
|
(`index.html:543`) — does not:
|
|
|
|
```js
|
|
function updateChart(name, labels, values, type, colors) { // index.html:898
|
|
if (name !== 'verdict' || !verdictChart) return;
|
|
verdictChart.data.labels = labels; // index.html:900
|
|
verdictChart.data.datasets[0].data = values;
|
|
verdictChart.data.datasets[0].backgroundColor = colors;
|
|
verdictChart.update();
|
|
}
|
|
```
|
|
|
|
`labels` here is `Object.keys(mix)` (`index.html:527`) — e.g.
|
|
`['pass', 'fail']`, one entry per verdict category, not the single-element
|
|
array the chart was created with. Line 900 overwrites `data.labels` with
|
|
that real array.
|
|
|
|
For a `stacked` bar chart on `indexAxis: 'y'` with a single dataset,
|
|
Chart.js uses `data.labels.length` to decide how many category rows to
|
|
draw. `stacked: true` (`index.html:854-855`) only merges multiple
|
|
*datasets* that share an index position — it does nothing to merge
|
|
multiple *values within one dataset* onto a single row. So the moment
|
|
`data.labels` goes from `['']` to `['pass', 'fail']`, the same one dataset
|
|
that used to render as one bar with two colored segments instead renders
|
|
as **two separate bars**, one per label. The "partition bar" concept only
|
|
holds together as long as `labels` stays a single blank entry, which is
|
|
exactly the invariant `updateChart` breaks.
|
|
|
|
## Why this passed the round of QA that caught the *last* verdict-chart bug
|
|
|
|
The previous regression (`verdictChart` never being assigned, causing
|
|
"Canvas is already in use") was caught by explicitly waiting 36+ seconds to
|
|
span a `REFRESH_MS` poll cycle
|
|
(`plans/.omo/evidence/task-8-admin-visual-fixes-v3-verdict.md`). That
|
|
discipline wasn't applied to `906c9e6` — it's a pure frontend diff with no
|
|
Playwright run, no screenshot, and no unit test attached
|
|
(`git show 906c9e6 --stat` touches only `admin/frontend/index.html`). A
|
|
single fresh-load screenshot would show a correct-looking single bar, since
|
|
first render always goes through `renderChart`, not `updateChart` — the
|
|
bug only shows up starting from the second render, i.e. the first 30-second
|
|
poll or SSE-triggered refresh after page load. Same blind spot as the last
|
|
bug, in a new function.
|
|
|
|
## Fix
|
|
|
|
`updateChart` is only ever called for `'verdict'` (the `name !== 'verdict'`
|
|
guard at the top makes it single-purpose), so there's no other caller
|
|
relying on the `data.labels` assignment. Drop it:
|
|
|
|
```js
|
|
function updateChart(name, labels, values, type, colors) {
|
|
if (name !== 'verdict' || !verdictChart) return;
|
|
verdictChart.data.datasets[0].data = values;
|
|
verdictChart.data.datasets[0].backgroundColor = colors;
|
|
verdictChart.update();
|
|
}
|
|
```
|
|
|
|
`labels` becomes an unused parameter at that point; either drop it from the
|
|
signature and its one call site (`index.html:543`), or leave it for
|
|
signature symmetry with `renderChart` — cosmetic, doesn't affect
|
|
correctness either way.
|
|
|
|
## Verification recommendation
|
|
|
|
A screenshot alone won't catch this class of bug twice. Confirm the fix the
|
|
same way the last one was confirmed: load `/admin/`, capture the verdict
|
|
bar on first render, wait past one `REFRESH_MS` cycle (30s+), capture again,
|
|
and diff — the bar should still be one row with the same segment count
|
|
before and after, not N separate rows.
|