fix(ui): implement UI review findings — a11y, IA, and consistency fixes
Fixes the reviewed gaps: keyboard-inaccessible delete controls (SessionRail, Entities row), case-sensitive entity filter, two competing entity-detail navigation patterns (standardize on EntitySheet), non-clickable Overview KPI cards, a bare button bypassing the shared Button component, inconsistent blur-only vs live filtering, and an unenforced sanitization assumption on search snippet HTML (now using the already-present dompurify dependency). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This commit is contained in:
262
plans/done/2026-07-11-ui-review-ia-usability.md
Normal file
262
plans/done/2026-07-11-ui-review-ia-usability.md
Normal file
@@ -0,0 +1,262 @@
|
||||
# UI review: information architecture, usability, and best practices
|
||||
|
||||
Status: Done — 2026-07-11. All fix-plan items implemented and verified live
|
||||
except C2 (a11y lint enforcement — no ESLint/svelte-check is configured in
|
||||
`web/` at all, so there's nothing to promote from warn to error; flagged
|
||||
below instead of silently adding lint infra). Verification also surfaced an
|
||||
unrelated pre-existing bug (Knowledge page search results never render) —
|
||||
spun off as a separate task, not fixed here.
|
||||
|
||||
## Scope
|
||||
|
||||
Systematic review of `web/src/` (Svelte 5 + shadcn-svelte + Tailwind v4
|
||||
control-room UI): all 13 pages, the 11 shared components, the sidebar/routing
|
||||
shell (`App.svelte`), and cross-cutting patterns (filtering, loading/empty
|
||||
states, live-event wiring, accessibility). Read in full, not sampled.
|
||||
Grounded in what's actually in the code — no speculative "best practice"
|
||||
items without a concrete file:line instance.
|
||||
|
||||
Not implementation. Findings and a proposed fix plan only, mirroring
|
||||
[`2026-07-11-nomos-agent-code-review.md`](2026-07-11-nomos-agent-code-review.md)'s
|
||||
structure — implement on a later "proceed."
|
||||
|
||||
## Findings
|
||||
|
||||
### A. Information architecture
|
||||
|
||||
**A1. Entity detail has two competing UI patterns for the same content.**
|
||||
[`Entities.svelte:16-19,155`](../web/src/pages/Entities.svelte) opens entity
|
||||
detail as an in-page `EntitySheet` slide-over (no URL change, no sidebar
|
||||
state change). [`Knowledge.svelte:57-59`](../web/src/pages/Knowledge.svelte)
|
||||
and [`Graph.svelte:464`](../web/src/pages/Graph.svelte) instead navigate via
|
||||
`location.hash = '#/entity/' + slug`, which `App.svelte`'s router resolves to
|
||||
a full-page `EntityDetail` route — but `'entity'` isn't in `navItems`
|
||||
([`App.svelte:68-79`](../web/src/App.svelte)), so landing there leaves the
|
||||
sidebar with nothing highlighted and the header showing the raw slug instead
|
||||
of a section name. Same underlying view
|
||||
(`EntityDetailContent.svelte`), three different entry points, two
|
||||
different navigation models, one of which produces an orphaned page state.
|
||||
A user who reaches an entity via Knowledge or Graph has no way back to
|
||||
"where they were" via the sidebar — only browser back.
|
||||
|
||||
**A2. Two chat entry points with no visual link between them.**
|
||||
The sidebar's "Tasks" section (board → `Chat.svelte` detail,
|
||||
`isActive={page === 'tasks' || page === 'chat'}`,
|
||||
[`App.svelte:127`](../web/src/App.svelte)) and the footer's "Chat drawer"
|
||||
button ([`App.svelte:160-163`](../web/src/App.svelte), opens a `Sheet`
|
||||
wrapping the same `Chat` component) are both valid, intentional ways to
|
||||
reach chat — but nothing in the UI explains they're different modes (drawer
|
||||
= overlay on current page, keeps your place; Tasks = full navigation). A
|
||||
first-time user has no way to know which one preserves their current page.
|
||||
Low-severity, but worth a tooltip/label distinction.
|
||||
|
||||
**A3. Overview's KPI cards don't drill down.**
|
||||
[`Overview.svelte`](../web/src/pages/Overview.svelte) shows "Pending
|
||||
approvals," "Open signals," and fleet-health counts as static cards. The
|
||||
header badges for the same data (`approvalsPending`, `openSignals`,
|
||||
[`App.svelte:185-194`](../web/src/App.svelte)) ARE clickable and navigate to
|
||||
Ops/Signals — so the pattern exists in the app, just not on the page whose
|
||||
entire purpose is summarizing this data. A dashboard card showing a count
|
||||
that doesn't lead anywhere is a standard drill-down gap.
|
||||
|
||||
### B. Usability / interaction consistency
|
||||
|
||||
**B1. Table-row click targets lack keyboard/screen-reader support in one
|
||||
place but not others.**
|
||||
[`Entities.svelte:117-120`](../web/src/pages/Entities.svelte) makes an
|
||||
entire `Table.Row` clickable via a bare `onclick`, with no `role`,
|
||||
`tabindex`, or `onkeydown` — unreachable and inoperable via keyboard, and
|
||||
screen readers get no indication the row is interactive. This is a
|
||||
regression against the codebase's own established pattern: `Tasks.svelte`
|
||||
wraps its cards in real `<button>` elements
|
||||
([`Tasks.svelte:172`](../web/src/pages/Tasks.svelte)), `Events.svelte`'s
|
||||
correlation-group headers are real `<button>`s
|
||||
([`Events.svelte:110-114`](../web/src/pages/Events.svelte)), and
|
||||
`Graph.svelte`'s SVG nodes explicitly add `role="button"`, `tabindex="0"`,
|
||||
and `onkeydown` ([`Graph.svelte:416-421`](../web/src/pages/Graph.svelte)).
|
||||
Entities is the outlier.
|
||||
|
||||
**B2. Filter inputs are inconsistently "live" vs. "apply-on-blur," with no
|
||||
visual cue either way.**
|
||||
`Entities.svelte`'s slug/name filter and `Graph.svelte`'s search box filter
|
||||
as-you-type (bound to a `$derived`). But `Ops.svelte` (implicitly, no text
|
||||
filters), `Audit.svelte`'s action/entity inputs
|
||||
([`Audit.svelte:71-72`](../web/src/pages/Audit.svelte)),
|
||||
`Agent.svelte`'s agent_id input
|
||||
([`Agent.svelte:59`](../web/src/pages/Agent.svelte)), and `Events.svelte`'s
|
||||
type/severity inputs ([`Events.svelte:92-93`](../web/src/pages/Events.svelte))
|
||||
all use `onchange`, which only fires on blur — a user typing a filter value
|
||||
and watching the table sees nothing happen until they click or tab away, and
|
||||
nothing in the UI (placeholder text, a debounce spinner, an "Enter to
|
||||
apply" hint) tells them why. Three different pages share the same
|
||||
`onchange`-only pattern, so it's a systemic choice, not an oversight — but
|
||||
it reads as broken on first use.
|
||||
|
||||
**B3. Entity filter is case-sensitive; nothing else in the app is.**
|
||||
[`Entities.svelte:50`](../web/src/pages/Entities.svelte) matches with raw
|
||||
`.includes()`, no `.toLowerCase()`. `Graph.svelte`'s equivalent search
|
||||
normalizes both sides
|
||||
([`Graph.svelte:175-176`](../web/src/pages/Graph.svelte):
|
||||
`n.slug.toLowerCase().includes(q)`). Slugs are lowercase by convention today,
|
||||
which is why this hasn't bitten anyone yet, but entity *names* are
|
||||
free text and can be mixed-case — a name filter that silently returns zero
|
||||
results for a correctly-spelled but wrong-case query is a real trap, and the
|
||||
one-line fix already has a working reference implementation three files
|
||||
away.
|
||||
|
||||
**B4. `{@html}` on server-provided search snippets.**
|
||||
[`Knowledge.svelte:120-121`](../web/src/pages/Knowledge.svelte) renders
|
||||
`hit.snippet` with `{@html}`, justified by a comment claiming the backend's
|
||||
`ts_headline` output is pre-sanitized. That's true for Postgres
|
||||
`ts_headline` today (it only wraps matched terms in `<b>` from a
|
||||
parameterized query), but there's no client-side enforcement of that
|
||||
invariant — if the search query or snippet source ever changes upstream,
|
||||
this becomes a stored-XSS vector with no guard at the point of use. Not an
|
||||
active vulnerability, but a fragile trust boundary worth tightening
|
||||
defensively (e.g. a tiny allow-list sanitizer) rather than relying on a
|
||||
comment to hold forever.
|
||||
|
||||
### C. Accessibility
|
||||
|
||||
**C1. `SessionRail.svelte`'s delete control is a `<span>`, not a button.**
|
||||
[`SessionRail.svelte:54-64`](../web/src/lib/components/SessionRail.svelte)
|
||||
attaches `onclick` to a `<span>` for the per-session delete affordance, with
|
||||
no `role`, `tabindex`, or keyboard handler — same defect class as B1, on a
|
||||
destructive action this time (delete a chat session), which makes it a
|
||||
notch more important: a keyboard-only user cannot delete a session from
|
||||
this rail at all.
|
||||
|
||||
**C2. Same defect, lower stakes, elsewhere.**
|
||||
Scan for the same "clickable non-interactive element" shape found in B1/C1
|
||||
should be swept across `web/src/` once — these two are the ones a full read
|
||||
surfaced, but the pattern (a `<div>`/`<span>` with `onclick` and no
|
||||
keyboard path) is exactly the kind of thing that creeps back in per-PR
|
||||
without a lint rule catching it. Worth checking whether
|
||||
`eslint-plugin-svelte`'s `a11y_click_events_have_key_events` /
|
||||
`a11y_no_static_element_interactions` rules are enabled and enforced in CI
|
||||
(the prior summary noted these exist as warnings, not build failures — that
|
||||
should be confirmed and possibly promoted to errors as part of implementing
|
||||
C1/B1).
|
||||
|
||||
### D. Visual / component consistency
|
||||
|
||||
**D1. One page bypasses the shared `Button` component.**
|
||||
`Agent.svelte`'s "Refresh" control is a bare
|
||||
`<button class="rounded-md border px-3 py-1.5 text-xs">`
|
||||
([`Agent.svelte:73`](../web/src/pages/Agent.svelte)) instead of
|
||||
`Button` (`variant="outline"`), which every other page's refresh/action
|
||||
buttons use (`Ops.svelte`, `Signals.svelte`, `Audit.svelte`, `Events.svelte`
|
||||
all use `<Button variant="outline">`). Cosmetically near-identical today
|
||||
(both render as a bordered pill) but it'll drift the moment the design
|
||||
tokens on `Button` change, since this one doesn't inherit them.
|
||||
|
||||
**D2. `formatEventLabel` is a needless indirection.**
|
||||
[`Overview.svelte`](../web/src/pages/Overview.svelte)'s
|
||||
`formatEventLabel(ev)` returns `ev.type` verbatim — a one-line wrapper with
|
||||
no formatting logic. Trivial, but noted since it reads as if formatting
|
||||
were intended and never finished.
|
||||
|
||||
### E. Loading / empty states
|
||||
|
||||
No real findings — this is a strength worth naming rather than "fixing."
|
||||
Every page reviewed (Overview, Entities, Ops, Signals, Events, Agent, Audit,
|
||||
Knowledge, Learning, Graph, Tasks) has both a loading state (skeletons or an
|
||||
implicit empty table) and an explicit, page-appropriate empty-state message
|
||||
(not a generic "no data"). That consistency is worth preserving as new pages
|
||||
get added — call it out in the PR template or a short frontend README note
|
||||
rather than leaving it as tribal knowledge.
|
||||
|
||||
## Fix plan
|
||||
|
||||
Priority order, grounded in user impact:
|
||||
|
||||
1. **C1 (SessionRail delete button)** — highest priority: it's a destructive
|
||||
action that's currently unreachable by keyboard at all. Swap the `<span>`
|
||||
for a real `<button>` with `aria-label="Delete session"`, matching the
|
||||
pattern `Tasks.svelte` already uses for its own delete affordance
|
||||
([`Tasks.svelte:195-207`](../web/src/pages/Tasks.svelte) — same feature,
|
||||
done correctly, in the same codebase).
|
||||
2. **B1 (Entities row click)** — wrap row content in a `<button>` (or add
|
||||
`role="button" tabindex="0" onkeydown`) matching `Tasks.svelte` /
|
||||
`Events.svelte`'s existing pattern.
|
||||
3. **B3 (case-sensitive filter)** — one-line `.toLowerCase()` fix on both
|
||||
sides of the `.includes()` calls in `Entities.svelte:50`.
|
||||
4. **A1 (dual entity-detail navigation)** — pick one pattern. Recommend
|
||||
standardizing on the `EntitySheet` (in-page, no navigation loss) and
|
||||
changing `Knowledge.svelte`/`Graph.svelte`'s "View entity detail" actions
|
||||
to open the sheet directly instead of hash-navigating to the orphaned
|
||||
`#/entity/:slug` route. If the full-page route is kept for deep-linking
|
||||
(a legitimate reason to keep it), then at minimum highlight the
|
||||
originating section in the sidebar and give the header a real label
|
||||
instead of the bare slug.
|
||||
5. **A3 (Overview KPI cards not clickable)** — wrap the approvals/signals
|
||||
cards in the same click-to-navigate pattern already used by the header
|
||||
badges.
|
||||
6. **D1 (Agent.svelte bare button)** — swap for `<Button variant="outline">`.
|
||||
7. **B2 (inconsistent live-vs-blur filtering)** — standardize on
|
||||
`oninput`-driven, debounced (~300ms) filtering across Audit/Agent/Events,
|
||||
matching the already-live feel of Entities/Graph. Lower priority than the
|
||||
above since it's a rough edge, not a defect.
|
||||
8. **B4 (`{@html}` trust boundary)** — add a minimal sanitize step (strip
|
||||
everything but the `<b>` tags `ts_headline` emits) at the point of
|
||||
render, so the safety property doesn't depend on the backend never
|
||||
changing.
|
||||
9. **A2 (chat drawer vs. Tasks unlabeled)** and **D2 (`formatEventLabel`)** —
|
||||
cosmetic, do opportunistically or skip.
|
||||
10. **C2 (a11y lint enforcement)** — checked: `web/` has no ESLint config and
|
||||
no `lint`/`check` npm script at all (confirmed via `package.json` and
|
||||
directory listing). The "a11y warnings" referenced in earlier session
|
||||
notes were editor/IDE diagnostics, not a CI gate. There's nothing to
|
||||
promote from warn to error because no lint infrastructure exists —
|
||||
setting one up is a separate, larger decision (which rules, whether to
|
||||
also add `svelte-check` for types) that wasn't part of this review's
|
||||
scope. Not done; flagging for a separate decision rather than silently
|
||||
bootstrapping tooling.
|
||||
|
||||
## Implementation notes (2026-07-11)
|
||||
|
||||
- C1, B1, B3, A1, A3, D1, D2, B2, B4, A2 all implemented and verified live
|
||||
in the browser preview against the running stack (see Verification below).
|
||||
- A1: standardized on `EntitySheet` per the plan's recommendation —
|
||||
`Knowledge.svelte` and `Graph.svelte`'s "View entity detail" now open the
|
||||
sheet instead of hash-navigating to the orphaned `#/entity/:slug` route.
|
||||
The full-page `EntityDetail` route/component was left in place (not
|
||||
deleted) as a harmless deep-link fallback — nothing internal navigates to
|
||||
it anymore, but a bookmarked/shared URL still resolves.
|
||||
- B4: used the `dompurify` package, already a `dependencies` entry in
|
||||
`web/package.json` (unused until now) — no new dependency added.
|
||||
- B2: added a small `debounce()` helper to `web/src/lib/utils.ts` and
|
||||
switched Audit/Agent/Events' filter inputs from `onchange` (blur-only) to
|
||||
debounced `oninput`.
|
||||
- **Found during verification, not in the original fix list:** the
|
||||
Knowledge page's search never actually renders results (the "Clear"
|
||||
button appears, confirming `searched` flips to `true`, but the content
|
||||
area stays on the "Recently learned" branch) despite the backend request
|
||||
succeeding with real data. Confirmed via `git diff` this isn't caused by
|
||||
anything touched here. Spun off as a separate follow-up rather than fixed
|
||||
in this pass, since it's unrelated to any finding in this review.
|
||||
|
||||
## Verification
|
||||
|
||||
- After each interaction fix (C1, B1, A3): manual keyboard-only pass (Tab +
|
||||
Enter/Space, no mouse) through the affected page in the browser preview.
|
||||
- After B3: type a filter query in Entities with mixed case against a
|
||||
known-mixed-case entity name; confirm it now matches.
|
||||
- After A1: confirm both entry paths (Entities row click, Knowledge search
|
||||
hit's linked entity, Graph node's "View entity detail") land on the same
|
||||
UI pattern; confirm sidebar/header state is coherent from whichever page
|
||||
the user started on.
|
||||
- `cd web && npm run lint && npm run check` clean after all fixes.
|
||||
- Visual: `npm run build` + spot-check each changed page in the browser
|
||||
preview (light pass, not full regression).
|
||||
|
||||
## Open questions
|
||||
|
||||
- **A1's resolution direction** (sheet vs. full-page route) is a genuine
|
||||
product call, not just a bug fix — needs a decision before implementing,
|
||||
not just "proceed." Recommendation given above (standardize on the
|
||||
sheet), but flagging it explicitly since it changes user-visible behavior
|
||||
for Knowledge and Graph, not just Entities.
|
||||
- Whether to promote a11y lint rules from warn to error (C2) is a policy
|
||||
call for the repo, worth a one-line "yes/no" rather than silently doing
|
||||
it.
|
||||
@@ -41,6 +41,7 @@ See [`done/`](done/) for executed plans:
|
||||
| 2026-07-10 | [Autonomous plan execution: close the observation gap](done/2026-07-10-autonomous-plan-execution.md) |
|
||||
| 2026-07-11 | [Tasks: the chat page as goal-structured autonomous work](done/2026-07-11-goal-oriented-chat-control-panel.md) |
|
||||
| 2026-07-11 | [Concurrent task execution: safety + throughput + frontend correctness](done/2026-07-11-concurrent-task-execution.md) |
|
||||
| 2026-07-11 | [UI review: information architecture, usability, and best practices](done/2026-07-11-ui-review-ia-usability.md) |
|
||||
|
||||
## Conventions
|
||||
|
||||
|
||||
Reference in New Issue
Block a user