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>
263 lines
14 KiB
Markdown
263 lines
14 KiB
Markdown
# 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.
|