Fixes 1-3 deployed and verified live: fresh trivial Q&A sessions now reach done immediately, and a goal-bearing session that stalled was correctly nudged by the idle sweep. Fix 4 (backfill) was replaced with deletion after the operator's call — verified against the DB first that zero knowledge notes were linked to or written by any of the 53 removed sessions, so nothing was lost. Documents the pagination gap in listSessions (hardcoded LIMIT 50, no total count) that hid 6 of those sessions from the original audit. Also fixes relative links in this plan and in the UI-review plan that broke when both moved from plans/ to plans/done/ (one directory level deeper). 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.
|