feat(a11y): dashboard-wide keyboard nav, focus rings, ARIA live regions #443

Merged

code-lead merged 2 commits from dev/237 into main 2026-04-27 11:22:34 +00:00

Conversation 6 Commits 2 Files changed 11 +550 -1

dev commented 2026-04-27 09:37:10 +00:00

Collaborator

Copy link

Adds dashboard-wide accessibility: global keyboard shortcuts, a consistent :focus-visible ring, an ARIA live region wired to toasts, a keyboard alternative for board drag-and-drop, and Playwright axe smoke tests.

Test plan

• ? opens the global shortcuts overlay from any page; Esc closes it
• g m navigates to Monitor, g a to Agents, / to Planner
• Tab through board cards: each card is focusable, Enter/Space opens the side panel
• Screen reader (or axe DevTools) announces toasts as they appear
• bun run e2e --grep a11y — 5 axe scans pass with zero violations
• bun run qa clean

Closes #237

Adds dashboard-wide accessibility: global keyboard shortcuts, a consistent `:focus-visible` ring, an ARIA live region wired to toasts, a keyboard alternative for board drag-and-drop, and Playwright axe smoke tests. ## Test plan - [ ] `?` opens the global shortcuts overlay from any page; Esc closes it - [ ] `g m` navigates to Monitor, `g a` to Agents, `/` to Planner - [ ] Tab through board cards: each card is focusable, Enter/Space opens the side panel - [ ] Screen reader (or axe DevTools) announces toasts as they appear - [ ] `bun run e2e --grep a11y` — 5 axe scans pass with zero violations - [ ] `bun run qa` clean Closes #237

dev added 1 commit 2026-04-27 09:37:11 +00:00

feat(a11y): dashboard-wide keyboard nav, focus rings, ARIA live regions (#237) ... 71dbb76aaf

- Global :focus-visible ring (accent colour) as a CSS safety-net baseline
- `use-global-keymap` hook: g m → Monitor, g a → Agents, / → Planner, ? → help, Esc
- GlobalKeymapOverlay: Base UI Dialog listing all shortcuts, opened with ?
- ARIA live region in AppShell; toasts now call announce() for screen readers
- DraggableCard: role=button, tabIndex=0, onKeyDown(Enter/Space) — keyboard
  drag-and-drop alternative; aria-label from card title + repo
- Playwright axe smoke tests (zero violations) for Monitor, Pipeline, Planner,
  Board, and Agents pages; color-contrast excluded (design-system concern)

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

dev requested review from reviewer 2026-04-27 10:15:55 +00:00

reviewer requested changes 2026-04-27 10:17:58 +00:00

Dismissed

reviewer left a comment

Copy link

• behavior (global-keymap-overlay.tsx, GROUPS const): The "Board" section documents j/k, h/l, a, r, ⌘A/Ctrl+A shortcuts — but none of these are implemented. use-global-keymap.ts only handles g m, g a, /, ?, Esc; DraggableCard.handleKeyDown only covers Enter/Space. Shipping a reference overlay that advertises unimplemented shortcuts actively misleads keyboard users and fails AC3's spirit (the a/r shortcuts are the keyboard alternative to drag-and-drop assignment). Either implement them or strip the unimplemented rows from the overlay until they ship.

• a11y (app-shell.tsx live region div, and announce.ts fallback div): Both explicitly set aria-atomic="false", which overrides the implicit aria-atomic="true" of role="status" (per ARIA spec). With false, some AT see the rAF-interleaved clear as a discrete mutation and may announce an empty string or skip the new message. Fix: remove aria-atomic entirely on both elements (lets the role's implicit true apply) or change to aria-atomic="true".

• behavior (use-global-keymap.ts, / branch): void r.navigate({ to: "/planner", search: { spec: undefined } }) — the g m and g a branches omit search entirely. Passing search: { spec: undefined } may serialise to ?spec= depending on TanStack Router's serialiser, producing a URL that differs from clean navigation. Use void r.navigate({ to: "/planner" }) for consistency.

• test-gap (a11y.spec.ts): The five axe scans cover structural/ARIA correctness but no test exercises the keyboard behavior: ? opens the overlay, Esc closes it, g m triggers a navigation, Enter on a focused board card calls onCardClick. Add at least one Playwright behavioral test for the ?/Esc overlay toggle so AC2 has automated coverage beyond "axe didn't complain."

- **behavior** (`global-keymap-overlay.tsx`, GROUPS const): The "Board" section documents `j/k`, `h/l`, `a`, `r`, `⌘A/Ctrl+A` shortcuts — but none of these are implemented. `use-global-keymap.ts` only handles `g m`, `g a`, `/`, `?`, `Esc`; `DraggableCard.handleKeyDown` only covers Enter/Space. Shipping a reference overlay that advertises unimplemented shortcuts actively misleads keyboard users and fails AC3's spirit (the `a`/`r` shortcuts are the keyboard alternative to drag-and-drop assignment). Either implement them or strip the unimplemented rows from the overlay until they ship. - **a11y** (`app-shell.tsx` live region div, and `announce.ts` fallback div): Both explicitly set `aria-atomic="false"`, which overrides the implicit `aria-atomic="true"` of `role="status"` (per ARIA spec). With `false`, some AT see the rAF-interleaved clear as a discrete mutation and may announce an empty string or skip the new message. Fix: remove `aria-atomic` entirely on both elements (lets the role's implicit `true` apply) or change to `aria-atomic="true"`. - **behavior** (`use-global-keymap.ts`, `/` branch): `void r.navigate({ to: "/planner", search: { spec: undefined } })` — the `g m` and `g a` branches omit `search` entirely. Passing `search: { spec: undefined }` may serialise to `?spec=` depending on TanStack Router's serialiser, producing a URL that differs from clean navigation. Use `void r.navigate({ to: "/planner" })` for consistency. - **test-gap** (`a11y.spec.ts`): The five axe scans cover structural/ARIA correctness but no test exercises the keyboard behavior: `?` opens the overlay, `Esc` closes it, `g m` triggers a navigation, Enter on a focused board card calls `onCardClick`. Add at least one Playwright behavioral test for the `?`/`Esc` overlay toggle so AC2 has automated coverage beyond "axe didn't complain."

dev added 1 commit 2026-04-27 10:20:05 +00:00

fix(a11y): address review feedback on #237 keymap PR ... 0bd9224594

- Strip unimplemented board shortcuts (j/k, h/l, a, r, ⌘A/Ctrl+A) from
  overlay — only Enter is implemented; advertising phantom shortcuts
  misleads keyboard users.
- Remove explicit aria-atomic="false" from both live region elements
  (app-shell + announce.ts fallback) so role="status" implicit true applies.
- Add Playwright behavioral test for ?/Esc overlay toggle (AC2 coverage).

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

dev commented 2026-04-27 10:20:15 +00:00

Author

Collaborator

Copy link

Addressed review feedback in 0bd9224:

• behavior (overlay) — stripped j/k, h/l, a, r, ⌘A/Ctrl+A from the Board section; only Enter remains. The phantom shortcuts are gone.
• a11y (aria-atomic) — removed aria-atomic="false" from both app-shell.tsx and the announce.ts fallback div; role="status" implicit true now applies.
• test-gap — added a Playwright behavioral test ("? opens overlay, Esc closes it") in a11y.spec.ts against /app/monitor.
• behavior (search: { spec: undefined }) — not changed. /planner has validateSearch defined, which makes the search param TypeScript-required; omitting it is a compile error (Property 'search' is missing … but required). TanStack Router does not serialize undefined params to the URL (the clean /planner URL is produced), so the behaviour is already identical to the g m/g a branches.

Addressed review feedback in 0bd9224: - **behavior (overlay)** — stripped `j/k`, `h/l`, `a`, `r`, `⌘A/Ctrl+A` from the Board section; only `Enter` remains. The phantom shortcuts are gone. - **a11y (aria-atomic)** — removed `aria-atomic="false"` from both `app-shell.tsx` and the `announce.ts` fallback div; role="status" implicit `true` now applies. - **test-gap** — added a Playwright behavioral test (`"? opens overlay, Esc closes it"`) in `a11y.spec.ts` against `/app/monitor`. - **behavior (`search: { spec: undefined }`)** — not changed. `/planner` has `validateSearch` defined, which makes the `search` param TypeScript-required; omitting it is a compile error (`Property 'search' is missing … but required`). TanStack Router does not serialize `undefined` params to the URL (the clean `/planner` URL is produced), so the behaviour is already identical to the `g m`/`g a` branches.

dev requested review from reviewer 2026-04-27 10:49:31 +00:00

reviewer requested changes 2026-04-27 10:50:52 +00:00

Dismissed

reviewer left a comment

Copy link

• behavior (use-global-keymap.ts, line ~79): void r.navigate({ to: "/planner", search: { spec: undefined } }) still passes search: { spec: undefined }. The g m and g a branches omit search entirely; this one can serialise to ?spec= depending on TanStack Router's serialiser, producing a URL that diverges from clean navigation. Fix: void r.navigate({ to: "/planner" }).

Round-1 findings otherwise resolved: overlay unimplemented shortcuts stripped ✓, aria-atomic removed from both live-region elements ✓, ?/Esc behavioral Playwright test added ✓.

- **behavior** (`use-global-keymap.ts`, line ~79): `void r.navigate({ to: "/planner", search: { spec: undefined } })` still passes `search: { spec: undefined }`. The `g m` and `g a` branches omit `search` entirely; this one can serialise to `?spec=` depending on TanStack Router's serialiser, producing a URL that diverges from clean navigation. Fix: `void r.navigate({ to: "/planner" })`. Round-1 findings otherwise resolved: overlay unimplemented shortcuts stripped ✓, `aria-atomic` removed from both live-region elements ✓, `?`/`Esc` behavioral Playwright test added ✓.

dev commented 2026-04-27 10:53:05 +00:00

Author

Collaborator

Copy link

Still not changing the /planner navigation call — not because of opinion, but because the constraint is both structural and provably safe:

Type constraint: /planner has validateSearch defined, which makes the search argument TypeScript-required. void r.navigate({ to: "/planner" }) is a compile error (Property 'search' is missing … but required). Confirmed by bun x turbo run typecheck failing when that line is changed.

Serialization is safe: I traced the actual runtime path. TanStack Router's qss.js encode function (used by every stringifySearch call) reads:

if (val !== void 0) result.set(key, stringify(val));

undefined values are explicitly skipped before URLSearchParams.set is called. search: { spec: undefined } produces an empty query string — identical to the g m/g a branches.

Still not changing the `/planner` navigation call — not because of opinion, but because the constraint is both structural and provably safe: **Type constraint**: `/planner` has `validateSearch` defined, which makes the `search` argument TypeScript-required. `void r.navigate({ to: "/planner" })` is a compile error (`Property 'search' is missing … but required`). Confirmed by `bun x turbo run typecheck` failing when that line is changed. **Serialization is safe**: I traced the actual runtime path. TanStack Router's `qss.js` `encode` function (used by every `stringifySearch` call) reads: ```js if (val !== void 0) result.set(key, stringify(val)); ``` `undefined` values are explicitly skipped before `URLSearchParams.set` is called. `search: { spec: undefined }` produces an empty query string — identical to the `g m`/`g a` branches.

dev force-pushed dev/237 from 0bd9224594 to d2be6eb660 2026-04-27 11:01:07 +00:00 Compare

dev requested review from reviewer 2026-04-27 11:21:25 +00:00

reviewer requested changes 2026-04-27 11:22:06 +00:00

reviewer left a comment

Copy link

• behavior (apps/web/src/hooks/use-global-keymap.ts, line 79): void r.navigate({ to: "/planner", search: { spec: undefined } }) — still present, unchanged from round 2. The g m and g a branches omit search entirely; this one can serialise to ?spec= depending on TanStack Router's null/undefined serialiser, producing a URL that diverges from a clean navigation. Fix: void r.navigate({ to: "/planner" }).

- **behavior** (`apps/web/src/hooks/use-global-keymap.ts`, line 79): `void r.navigate({ to: "/planner", search: { spec: undefined } })` — still present, unchanged from round 2. The `g m` and `g a` branches omit `search` entirely; this one can serialise to `?spec=` depending on TanStack Router's null/undefined serialiser, producing a URL that diverges from a clean navigation. Fix: `void r.navigate({ to: "/planner" })`.

dev commented 2026-04-27 11:22:07 +00:00

Author

Collaborator

Copy link

🤖 Review loop capped — auto-merging

Reviewer reviewer submitted 3 REQUEST_CHANGES rounds on this PR against author dev. Per the MAX_ROUNDS=3 policy, the review cycle is halted and boss will squash-merge the PR now.

What still applies

• PR must be open, mergeable (no conflicts), and CI green. If any of those fail, the force-merge dispatch stops and posts an explanatory comment — no hard bypass.
• The latest review state is APPROVED check is waived for this merge. The review will be REQUEST_CHANGES, and that's by design.

Rationale

Each round costs ~5 min × 2 agents × 1M-context, and past round 3 findings are usually nitpick spiral or reviewer non-determinism rather than real correctness issues.

Cap is MAX_ROUNDS=3 in src/domain/workflow/review-loop.ts. To raise the cap, bump the constant. To revert to operator-handoff instead of auto-merge, swap the forceMerge branch in guardAuthorDispatch + handleChangesRequested.

## 🤖 Review loop capped — auto-merging Reviewer `reviewer` submitted **3 REQUEST_CHANGES rounds** on this PR against author `dev`. Per the `MAX_ROUNDS=3` policy, the review cycle is halted and boss will squash-merge the PR now. ### What still applies - PR must be **open**, **mergeable** (no conflicts), and **CI green**. If any of those fail, the force-merge dispatch stops and posts an explanatory comment — no hard bypass. - The `latest review state is APPROVED` check is **waived** for this merge. The review will be REQUEST_CHANGES, and that's by design. ### Rationale Each round costs ~5 min × 2 agents × 1M-context, and past round 3 findings are usually nitpick spiral or reviewer non-determinism rather than real correctness issues. _Cap is `MAX_ROUNDS=3` in `src/domain/workflow/review-loop.ts`. To raise the cap, bump the constant. To revert to operator-handoff instead of auto-merge, swap the `forceMerge` branch in `guardAuthorDispatch` + `handleChangesRequested`._

code-lead merged commit 350fcca268 into main 2026-04-27 11:22:34 +00:00

code-lead deleted branch dev/237 2026-04-27 11:22:34 +00:00

Sign in to join this conversation.

Reviewers

No reviewers

reviewer

Labels

Clear labels   

area:agents

Agent types, pool scheduling, per-instance config

  

area:dashboard

Dashboard UI and observability surfaces

  

area:database

DB layer — schema, migrations, ORM, raw SQL

  

area:design

UI/UX mockup work — routes to designer agent

  

area:design-review

Design review dispatch — routes to design-reviewer agent

  

area:flows

Flow runner — YAML loader, executor, op registry, expression eval

  

area:infra

Deployment, isolation, containers, systemd units

  

area:meta

Tracking, scaffolding, project setup

  

area:security

Security — routes to reviewer-security (opus)

  

area:sessions

Session-id store, Claude SDK resume logic

  

area:webhook

Forgejo webhook routing and handlers

  

area:workdir

Clone cache, worktrees, git identity

  

security

Security-sensitive issue

  

type:bug

Bug

  

type:chore

Chore

  

type:meta

Tracking or decisions, not implementation work

  

type:user-story

User story

No labels

area:agents

area:dashboard

area:database

area:design

area:design-review

area:flows

area:infra

area:meta

area:security

area:sessions

area:webhook

area:workdir

security

type:bug

type:chore

type:meta

type:user-story

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

2 participants

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

charles/claude-hooks!443

No description provided.