6krrt/plans/admin-visual-fixes-review.md

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:

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:

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.