# Review: `admin-visual-fixes` implementation

Status: done -- review of shipped work

**What it was reviewing:** the 8-todo `admin-visual-fixes` plan
(`plans/.omo/plans/admin-visual-fixes.md`), after its orchestration
reported 8/8 complete and F1-F4 all APPROVE. Verified against the actual
diff and the plan's own saved QA evidence
(`plans/.omo/evidence/task-8-admin-visual-fixes-console.txt`), not the
orchestration summary.

## Verdict: 6 of 8 todos are genuinely fixed; Todo 2 is not, and the plan's own evidence file proves it

### Confirmed correct
- Todo 1 (quota bar): `gauge-canvas` fully removed, `.quota-bar` markup in place.
- Todo 3 (history chart): canvas id fixed to `history-chart` **and**
  `historyChart = renderChart(...)` correctly captures the instance
  (`admin/frontend/index.html:610`).
- Todo 4: `knobDisplayValue()` (`index.html:687-696`) correctly unwraps
  `{enabled: bool}` before display.
- Todo 5: `renderWarnings` (`index.html:538-547`) rebuilds the `#no-warnings`
  placeholder inline instead of referencing a detached node — no more stale
  reference.
- Todo 6 (XSS): `task_category` and `modelStr` (selected_model/
  selected_provider) are now `escapeHtml()`-wrapped in both
  `renderDecisions` and `addDecision`. `rejected_reason`/`runner_up_models`
  are never rendered anywhere in the file, so there was nothing to escape
  there.
- Todo 7 (config race): correct and thorough.
  `_persist_config_value` (`admin.py:349-374`) takes a module-level
  `threading.Lock` around the whole load→validate→backup→write→replace
  sequence (the right primitive — `admin_config_write` is a *sync* handler,
  which FastAPI runs via `anyio.to_thread`, so `asyncio.Lock` would not have
  serialized anything), validates before backing up, and writes via
  `.tmp` + `os.replace` so a reader never observes a truncated file.
  `tests/test_admin_config.py::test_config_concurrent_writes_are_atomic_no_zero_byte_backups`
  is a real regression test — a live reader thread asserts the file is
  never empty while two writer threads race. `saveAllConfig()`
  (`index.html:774-804`) is now a sequential `for...of await` loop, not
  `Promise.all`. All 57 admin tests pass.

### Not fixed: Todo 2 (verdict doughnut) — and the evidence already proves it

`renderVerdict` (`index.html:508-536`) fixed the canvas id
(`verdict-canvas` → `verdict-chart`) but never assigns the created chart
back to the `verdictChart` variable:

```js
if (verdictChart) { updateChart('verdict', labels, values, 'doughnut', colors); }
else { renderChart('verdict-chart', labels, values, 'doughnut', colors); }   // return value dropped
```

`verdictChart` stays `null` forever, so every 30-second poll
(`REFRESH_MS`) takes the `else` branch again and tries to create a second
Chart.js instance on a canvas that already has one attached. This is
already recorded in the plan's own saved evidence,
`task-8-admin-visual-fixes-console.txt`:

```
Uncaught (in promise) Error: Canvas is already in use. Chart with ID '1' must be
destroyed before the canvas with ID 'verdict-chart' can be reused.
    at renderChart (http://127.0.0.1:8091/admin/:831:11)
    at renderVerdict (http://127.0.0.1:8091/admin/:531:9)
    at loadSnapshot (http://127.0.0.1:8091/admin/:332:4)
```

— repeated 6 times in the log. The doughnut renders once on first load,
then throws and stops updating for the rest of the page's life. This
directly contradicts F3's "clean console" sign-off and Todo 2's own
acceptance criteria ("the verdict doughnut renders"); a single screenshot
taken right after page load wouldn't catch it, but the saved console log
already had it and nobody read it before approving.

**Fix**: mirror what `historyChart` already does correctly two functions
over — capture the return value:

```js
else { verdictChart = renderChart('verdict-chart', labels, values, 'doughnut', colors); }
```

Also update the dead-code branch at `index.html:512`
(`renderChart('verdict-chart', null, 'verdict')` — missing arguments,
never actually reassigns anything either) to just `verdictChart = null;`
without the pointless re-render call, since there's no data to draw.

### Secondary, out-of-scope-but-worth-a-look: `objective.max_energy_per_request` 422s on every "Save All"

Same evidence file shows two `422` responses for
`POST /admin/api/config/objective.max_energy_per_request` during the
save-all QA pass. `config.yaml` has this key as `null`; the config table's
text input renders it as an empty string, and `saveAllConfig`'s
number-coercion (`Number('') → 0`, but the `val.trim() !== ''` guard
correctly keeps it as `''` rather than coercing to `0`) ends up POSTing an
empty string against an `Optional[float]` field, which `RouterConfig`
rejects. Not data-destructive (validation fails before the backup/write
step, exactly as designed) and not part of any of the 8 todos' scope, but
worth a follow-up: null-valued numeric config fields can't currently be
round-tripped through "Save All" without a spurious failure toast.

### Process gap, not a code defect

No commits landed for any of the 8 todos despite the plan's explicit "one
commit per todo (8 total)" commit strategy — `git log` still ends at
`b63234a`, and `admin.py` / `admin/frontend/index.html` remain untracked.
Separately, the live `llm-router.service` has not restarted since 02:35:45
(same PID throughout), so the Todo 7 fix — the one that matters for
production — is sitting on disk but not yet protecting the running
service.

## Recommendation

Fix the one-line `verdictChart` assignment (plus the dead branch at
`:512`), re-run the same Playwright QA against a fresh page load *and* at
least one 30s+ idle period to confirm the console actually stays clean
across a poll cycle this time — a single screenshot won't catch this class
of bug again. Then commit per the plan's own commit strategy. Restarting
`llm-router.service` to pick up the Todo 7 fix is a separate, explicit step
to take afterward, not bundled into the same action.