diff --git a/openspec/changes/add-onboarding-tour/design.md b/openspec/changes/add-onboarding-tour/design.md new file mode 100644 index 00000000..acab70b6 --- /dev/null +++ b/openspec/changes/add-onboarding-tour/design.md @@ -0,0 +1,157 @@ +# Design — Add Onboarding Tour + +## Trigger and gating + +The tour is gated by a single persisted field, `tourCompletedAt: number | +null`, stored alongside other UI state in `src/store/persistence.ts`. + +`tourCompletedAt` is `null` after `loadState()` for both fresh installs and +existing users who upgrade. To avoid hijacking the UI for users who already +know the app, the activation pass first inspects the loaded state for any +prior project or task. If at least one exists, the tour is treated as +already completed: `tourCompletedAt` is set to the current timestamp and no +overlay is shown. Only installs with zero projects and zero tasks proceed to +actual activation. + +When activation does proceed, the tour starts from `App.tsx` `onMount` after +`loadState()` resolves. If a modal dialog is open at that moment (unlikely +on first launch, but possible after `restartTour` from settings), activation +is deferred: a Solid effect watches the modal-flag signals (`showHelpDialog`, +`showSettingsDialog`, `showNewTaskDialog`, `showArena`) and triggers +activation as soon as they are all false, provided `tourCompletedAt` is +still `null`. The keybinding migration banner is suppressed while the tour +is active and re-evaluates after completion or skip. + +## Architecture + +A new store slice `src/store/tour.ts` exposes: + +- `tourActive: boolean` +- `tourStep: number` (0-indexed) +- `startTour()`, `nextStep()`, `prevStep()`, `skipTour()`, `finishTour()` +- `restartTour()` — clears `tourCompletedAt` and calls `startTour()` + +A new component `src/components/TourOverlay.tsx` renders when `tourActive` is +true. It is mounted in `App.tsx` near the existing dialogs (`HelpDialog`, +`SettingsDialog`, `ArenaOverlay`). Steps are declared as data, not JSX: + +```ts +type TourStep = { + id: string; + anchorId: string | null; // null = centered, no spotlight + title: string; + body: string; + placement: 'top' | 'right' | 'bottom' | 'left' | 'center'; + beforeEnter?: () => void; // e.g. open NewTaskDialog so its anchor exists + afterLeave?: () => void; // e.g. close it again +}; +``` + +The overlay locates its anchor via `document.querySelector('[data-tour-id="< +id>"]')`, observes its `getBoundingClientRect`, and renders: + +- A full-viewport dimmer with an SVG cutout over the anchor's bounding rect. +- A tooltip panel positioned relative to the anchor (simple heuristic; no + popper dependency). +- Prev / Next / Skip controls; step counter ("3 of 8"); Esc to skip. + +Anchor positions are recomputed on `resize` and via a `ResizeObserver` on the +anchor element so the spotlight follows window resizes and layout shifts. + +## Steps + +| # | `anchorId` | Teaches | +|---|-------------------------|------------------------------------------------------------------------| +| 1 | `null` (centered) | Welcome; one-line model: "every task = its own git worktree." | +| 2 | `tour-project-picker` | "Pick or add a project — your repo lives here." | +| 3 | `tour-new-task` | "Each task creates a branch + worktree automatically." | +| 4 | `tour-agent-selector` | "Choose Claude Code, Codex, Gemini, or a custom agent." | +| 5 | `tour-task-terminal` | "Watch the agent work live; type to interject." | +| 6 | `tour-changed-files` | "Review diffs as files change; click for full Monaco view." | +| 7 | `tour-merge-action` | "Merge back to main from the sidebar when you're happy." | +| 8 | `null` (centered) | "Press `?` anytime to see all shortcuts. You're done." | + +Step 4 needs the `NewTaskDialog` open so its anchor exists. The step uses +`beforeEnter: () => toggleNewTaskDialog(true)` and +`afterLeave: () => toggleNewTaskDialog(false)`. The dialog's normal +keybindings are suppressed while the tour is active so the user can't +accidentally submit a task during the tour. + +## First-run with no project + +If the user has no project at the time the tour starts, steps 5–7 have no +DOM anchor. We resolve this by partitioning the tour into two phases: + +- **Phase 1 (steps 1–4)** runs immediately on first launch and ends with a + prompt: "Create your first task to continue the tour, or skip." +- **Phase 2 (steps 5–8)** resumes the first time a task panel mounts after + Phase 1 completed, gated by a `tourStep` resume token persisted alongside + `tourCompletedAt`. + +Both phases share the same store and overlay; only the gating logic differs. +Skipping in either phase finalises `tourCompletedAt` so the tour does not +re-trigger. + +## Accessibility + +- Tooltip is `role="dialog"` with `aria-labelledby` (title) and + `aria-describedby` (body). +- `lib/focus-trap.ts` is reused to trap focus inside the tooltip; on close, + focus is restored to the anchor element. +- An `aria-live="polite"` region inside the overlay announces step + transitions ("Step N of M — ") so screen-reader users hear forward + / back navigation. The live region only updates on actual step changes, + not on anchor reposition. +- `prefers-reduced-motion: reduce` disables the spotlight transition and any + fade-ins. +- The overlay's dimmer has `aria-hidden="true"` so screen readers ignore it. + +## Known implementation risks + +These are not spec-level requirements but implementation decisions that need +care during the actual build. Calling them out here so they don't surprise +the implementer. + +- **Anchor lookup timing.** A flat 300 ms wait for the anchor to appear is + fragile on slow machines; prefer a `MutationObserver` that resolves as + soon as the element appears, with a longer absolute fallback (e.g. 3 s) + before skipping. Skipping a step should be logged. +- **Anchor disappearance mid-step.** If the anchor unmounts while a step is + visible (e.g. user collapses a panel via shortcut), advance the tour + rather than render a spotlight over an empty rectangle. +- **Anchor existence test.** Add a test that walks the step list, mounts + the relevant components, and asserts every non-null `anchorId` resolves + to a DOM node — so a future refactor that drops a `data-tour-id` fails + loudly instead of producing a silently broken tour. +- **`aria-live` region implementation.** The live region must be keyed on + `tourStep` (not re-rendered on every overlay tick) so that anchor + repositions or focus restorations don't re-announce the step. A naive + implementation that sets `textContent` inside an effect with broader + dependencies will violate the spec's de-duplication scenario. +- **Single-window scope.** Activation logic assumes a single renderer; if + the app ever opens a second window, only the first window to read + `tourCompletedAt === null` runs the tour. The implementation may guard + against this with a per-process flag, but the spec does not promise + multi-window behavior. +- **Hook failure logging.** `beforeEnter` and `afterLeave` hooks (e.g. + opening / closing `NewTaskDialog`) can throw; anchor lookups can time + out and skip. These catch sites must route through the structured + logger added by the `add-structured-logging` proposal under category + `tour`, not `console.warn`. If logging lands first, this is a hard + dependency at implementation time; if tour lands first, expect a + follow-up sweep. +- **Cross-proposal: dialog stack.** The spec lists four named modal + signals to defer activation against. The `improve-dialog-accessibility` + proposal introduces a stack-counted store of open dialogs; if it lands + first the tour implementation should consume that stack instead of + enumerating named signals, so a future fifth dialog doesn't silently + let the tour activate over it. The spec deliberately states the + observable behaviour (defer while a modal is open) without committing + to either source of truth. + +## Out of scope + +- Per-OS or per-agent tour variants. +- Telemetry on tour completion (no infra exists today). +- Animations, video, or interactive demo data. +- Replacing or restructuring `HelpDialog`. diff --git a/openspec/changes/add-onboarding-tour/proposal.md b/openspec/changes/add-onboarding-tour/proposal.md new file mode 100644 index 00000000..f4abb102 --- /dev/null +++ b/openspec/changes/add-onboarding-tour/proposal.md @@ -0,0 +1,43 @@ +# Add Onboarding Tour + +## Why + +The app's mental model — every task is a git worktree on its own branch, and +multiple agents can run in parallel — is unusual enough that first-time users +hit dead ends before they understand the workflow. `HelpDialog` lists the +shortcuts but never explains what the worktree-per-task model is, or how to get +from "fresh install" to "merged change". A short, skippable, first-launch tour +that points at the real UI elements (project picker, new task button, agent +selector, terminal panel, changed-files panel, merge action, help) closes that +gap without forcing users to read documentation. + +## What changes + +- Add a guided tour overlay that runs once on first launch and walks the user + through the project → spawn → review → merge flow. +- Mount the overlay at the top level of the app shell so it can dim the page + and spotlight real DOM anchors via `data-tour-id` attributes added to + existing components (no DOM restructuring). +- Persist a single `tourCompletedAt` timestamp so the tour does not re-trigger + after dismissal or completion. +- Auto-mark the tour completed for users who upgrade with prior projects or + tasks already in their persisted state, so existing installs do not get + hijacked by a tour they did not ask for. +- Add a "Restart tour" button to `SettingsDialog` so users can replay it on + demand. +- Defer the existing keybinding-migration banner until the tour is dismissed + or completed, so the two onboarding surfaces never overlap. + +## Impact + +- New capability `onboarding-tour`. +- New persisted store field `tourCompletedAt: number | null` (handled by the + existing autosave persistence path; no migration code needed for additive + optional fields). +- Additions to `App.tsx` (mount the overlay, gate first-launch activation), + `SettingsDialog.tsx` ("Restart tour" button), and a small set of existing + components (`Sidebar`, `NewTaskDialog`, `TaskPanel`) which gain + `data-tour-id` attributes. The final step is centered with no anchor since + the app does not surface a visible help button — help is invoked via the + `?` shortcut, which the step text mentions. +- No new IPC channels, no new backend work. diff --git a/openspec/changes/add-onboarding-tour/specs/onboarding-tour/spec.md b/openspec/changes/add-onboarding-tour/specs/onboarding-tour/spec.md new file mode 100644 index 00000000..a8aa1be1 --- /dev/null +++ b/openspec/changes/add-onboarding-tour/specs/onboarding-tour/spec.md @@ -0,0 +1,268 @@ +# Onboarding Tour Specification + +## ADDED Requirements + +### Requirement: First-launch activation + +The app SHALL run the onboarding tour exactly once per install, on the first +launch where persisted state has no record of the tour having completed or +been skipped, and SHALL skip the tour for users who already have prior task +or project data so existing installs are not interrupted by a tour they did +not ask for. + +#### Scenario: Fresh install starts the tour + +- **WHEN** the app boots and persisted state's `tourCompletedAt` is `null` +- **AND** the persisted state has no projects and no tasks +- **THEN** the app activates the tour at step 0 once `loadState()` has + resolved and no modal dialog is currently open + +#### Scenario: Existing user with prior data skips the tour + +- **WHEN** the app boots and `tourCompletedAt` is `null` +- **AND** the persisted state contains at least one project or one task +- **THEN** the app sets `tourCompletedAt` to the current timestamp without + activating the tour +- **AND** the tour does not activate on this or any subsequent launch unless + the user invokes "Restart tour" + +#### Scenario: Returning user does not see the tour + +- **WHEN** the app boots and `tourCompletedAt` is a non-null number +- **THEN** the tour does not activate +- **AND** no overlay is rendered + +#### Scenario: Activation defers when a modal is open + +- **WHEN** the activation conditions are otherwise met +- **AND** a modal dialog (`HelpDialog`, `SettingsDialog`, `NewTaskDialog`, + or `ArenaOverlay`) is open at the moment activation would occur +- **THEN** the app holds activation +- **AND** activates the tour as soon as all such modals close, provided + `tourCompletedAt` is still `null` + +#### Scenario: Tour suppresses the keybinding migration banner + +- **WHEN** the tour is active +- **AND** the keybinding migration banner would otherwise be shown +- **THEN** the banner is hidden until the tour finishes or is skipped + +### Requirement: Step navigation + +The tour SHALL let the user move forward, move backward, and skip out of the +flow at any step. + +#### Scenario: Advance to next step + +- **WHEN** the user clicks "Next" or presses Enter on a non-final step +- **THEN** `tourStep` increments by 1 +- **AND** any `afterLeave` hook of the previous step runs +- **AND** any `beforeEnter` hook of the next step runs before the new anchor + is queried + +#### Scenario: Move to previous step + +- **WHEN** the user clicks "Back" on any step except step 0 +- **THEN** `tourStep` decrements by 1 +- **AND** the step being left runs its `afterLeave` hook before the new + current step's `beforeEnter` hook runs and the new anchor is queried, + so a hook that opens a dialog for one step is paired with a hook that + closes it on the way back + +#### Scenario: Skip the tour + +- **WHEN** the user clicks "Skip" or presses Esc at any step +- **THEN** the tour deactivates +- **AND** `tourCompletedAt` is set to the current timestamp +- **AND** the tour does not re-activate on subsequent launches + +#### Scenario: Finish the tour + +- **WHEN** the user clicks "Done" on the final step +- **THEN** the tour deactivates +- **AND** `tourCompletedAt` is set to the current timestamp + +### Requirement: Spotlight follows real DOM anchors + +The tour SHALL spotlight existing UI elements via `data-tour-id` attributes +without restructuring the DOM, and SHALL keep the spotlight aligned as the +window resizes or layout changes. + +#### Scenario: Anchor exists when step activates + +- **WHEN** a step with a non-null `anchorId` activates +- **AND** an element with `data-tour-id="<anchorId>"` is in the DOM +- **THEN** the overlay renders a cutout matching that element's bounding + rectangle +- **AND** the tooltip is positioned according to the step's `placement` + +#### Scenario: Anchor missing when step activates + +- **WHEN** a step with a non-null `anchorId` activates +- **AND** no matching element appears in the DOM before the + implementation-defined wait expires +- **THEN** the tour skips the step and advances to the next one +- **AND** the skip is logged + +#### Scenario: Anchor disappears mid-step + +- **WHEN** the anchor element of the currently displayed step is removed + from the DOM (e.g. the user collapses a panel via shortcut) +- **THEN** the tour advances to the next step rather than rendering a + spotlight over an empty rectangle + +#### Scenario: Window resize updates the spotlight + +- **WHEN** the window resizes or the anchor element's bounding rectangle + changes +- **THEN** the spotlight cutout and tooltip position update to match without + closing the overlay + +#### Scenario: Centered step has no spotlight + +- **WHEN** a step's `anchorId` is `null` or the empty string `""` +- **THEN** the overlay renders a uniform dimmer with no cutout +- **AND** the tooltip is centered in the viewport +- **AND** no anchor lookup is attempted + +### Requirement: Two-phase flow when no project exists + +The tour SHALL split into two phases so a fresh user with no project can +still see all steps without the app fabricating demo data. `tourStep` is +a single global 0-indexed counter across both phases; phase 2 resumes at +the step index immediately following the last phase-1 step rather than +restarting at zero. + +#### Scenario: First launch with no project + +- **WHEN** the tour activates on a fresh install with zero projects +- **THEN** the tour runs phase 1 (steps that explain projects, the new-task + button, and the agent selector) +- **AND** the final phase-1 step prompts the user to create their first task + or skip +- **AND** the `tourStep` resume token is persisted so phase 2 can resume + from the next global step index + +#### Scenario: Phase 2 resumes after first task + +- **WHEN** phase 1 completed without skipping +- **AND** a task panel mounts for the first time afterward +- **THEN** the tour activates phase 2 at the global step index that + immediately follows the last phase-1 step (not phase-relative zero) + +#### Scenario: Phase 2 resume token persists across launches + +- **WHEN** phase 1 completed without skipping in a previous session +- **AND** no task panel has yet mounted +- **THEN** the resume token persists across app restarts +- **AND** phase 2 activates the next time a task panel mounts in any future + session, regardless of how many launches have passed + +#### Scenario: Mid-phase-1 quit restarts phase 1 + +- **WHEN** the app quits while phase 1 is mid-flight (the user neither + finished phase 1 nor explicitly skipped) +- **THEN** the next launch treats the tour as not yet started +- **AND** phase 1 begins again at step 0 +- **AND** any partial `tourStep` value from the prior session is discarded + +#### Scenario: Mid-phase-2 quit resumes at the same step + +- **WHEN** the app quits while phase 2 is mid-flight (after phase 1 + finished, before phase 2 was completed or skipped) +- **THEN** the persisted `tourStep` retains the global index of the + current step +- **AND** the next launch resumes phase 2 at that step the next time a + task panel mounts + +#### Scenario: Existing user restarts the tour with anchors already in scope + +- **WHEN** an existing user (one with at least one prior project or task) + invokes "Restart tour" from `SettingsDialog` +- **AND** at least one task panel is already mounted at the moment phase 1 + finishes +- **THEN** phase 2 activates immediately when phase 1 ends, without + waiting for a new task-panel mount + +#### Scenario: Skipping in either phase finalises the tour + +- **WHEN** the user skips during phase 1 or phase 2 +- **THEN** `tourCompletedAt` is set +- **AND** any phase-2 resume token is cleared +- **AND** the tour does not re-activate on subsequent launches + +### Requirement: Restart from settings + +The app SHALL let the user replay the tour from the settings dialog +regardless of whether the tour is currently active. + +#### Scenario: Restart tour clears completion + +- **WHEN** the user clicks "Restart tour" in `SettingsDialog` +- **AND** the tour is not currently active +- **THEN** `tourCompletedAt` is set to `null` +- **AND** the `tourStep` resume token is reset +- **AND** the tour activates immediately at step 0 once `SettingsDialog` + closes (deferred via the same modal-aware activation rules) + +#### Scenario: Restart tour while the tour is already active + +- **WHEN** the user clicks "Restart tour" while the tour is currently + active (e.g. in phase 2 step 6) +- **THEN** the current step is abandoned and any pending `afterLeave` + hook of that step still runs +- **AND** `tourCompletedAt` is set to `null` +- **AND** `tourStep` is reset to 0 +- **AND** the tour resumes at step 0 once `SettingsDialog` closes + +### Requirement: Global shortcuts are suppressed during the tour + +The app SHALL suppress its global keybindings while the tour is active so +a stray key press cannot open another modal on top of the tour. + +#### Scenario: Global keybindings do nothing while the tour is active + +- **WHEN** `tourActive` is `true` +- **AND** the user presses a keybinding that would normally trigger a + global action (new task, focus mode, help, settings, etc.) +- **THEN** the action does not run +- **AND** only Esc (skip) and Enter (advance) are honoured by the tour + tooltip itself + +#### Scenario: Global keybindings resume after the tour ends + +- **WHEN** the tour deactivates via Done, Skip, or Restart +- **THEN** subsequent global keybindings run normally + +### Requirement: Accessibility + +The tour SHALL be operable by keyboard alone, announce its tooltip to +assistive technology, and respect the user's reduced-motion preference. + +#### Scenario: Keyboard focus is trapped in the tooltip + +- **WHEN** the tour is active +- **THEN** Tab and Shift-Tab cycle focus among the tooltip's interactive + controls only +- **AND** focus does not escape into the dimmed background + +#### Scenario: Tooltip is announced as a dialog + +- **WHEN** the tooltip renders +- **THEN** it has `role="dialog"` +- **AND** its title is referenced by `aria-labelledby` +- **AND** its body text is referenced by `aria-describedby` + +#### Scenario: Reduced motion disables transitions + +- **WHEN** the user agent reports `prefers-reduced-motion: reduce` +- **THEN** the spotlight and tooltip render without fade-in or movement + transitions + +#### Scenario: Step transitions are announced to assistive technology + +- **WHEN** the active step changes (forward, back, or initial activation) +- **THEN** an `aria-live="polite"` region announces the new step's position + ("Step N of M") and its title +- **AND** the announcement does not duplicate when the same step re-renders + for non-step reasons such as an anchor reposition diff --git a/openspec/changes/add-onboarding-tour/tasks.md b/openspec/changes/add-onboarding-tour/tasks.md new file mode 100644 index 00000000..6c4cf1a2 --- /dev/null +++ b/openspec/changes/add-onboarding-tour/tasks.md @@ -0,0 +1,56 @@ +# Tasks — Add Onboarding Tour + +- [ ] Add `tourCompletedAt: number | null` and `tourStep: number | null` to + the persisted state in `src/store/persistence.ts` (and the matching + `PersistedState` type). +- [ ] New store slice `src/store/tour.ts` with `tourActive`, `tourStep`, + `startTour`, `nextStep`, `prevStep`, `skipTour`, `finishTour`, + `restartTour`. Re-export from `src/store/store.ts`. +- [ ] First-launch carve-out: in the activation pass, if the loaded state + already contains at least one project or task, set `tourCompletedAt` + and skip activation so existing users are not interrupted. +- [ ] Defer activation while any modal flag (`showHelpDialog`, + `showSettingsDialog`, `showNewTaskDialog`, `showArena`) is true; a + Solid effect retriggers activation when they all close. +- [ ] On mid-phase-1 quit (`tourActive` was true at quit, tour never + finalised), discard any persisted `tourStep` on next launch so phase 1 + restarts cleanly from step 0. +- [ ] New component `src/components/TourOverlay.tsx`: dimmer with SVG cutout, + tooltip panel, prev/next/skip controls, focus trap, Esc-to-skip, + `aria-live="polite"` region for step announcements. +- [ ] Add `data-tour-id` anchors to `Sidebar` (project picker, new-task + button, merge action), `NewTaskDialog` (agent selector), and + `TaskPanel` (terminal region + changed-files region). Final step is + centered with no anchor. +- [ ] Anchor existence test: assert every non-null `anchorId` in the step + registry resolves to a DOM node when the relevant components are + mounted, so a future rename fails loudly. +- [ ] Anchor lookup uses `MutationObserver` with a 3 s absolute fallback; + a step whose anchor never appears is logged and skipped, and a step + whose anchor unmounts mid-display advances rather than rendering an + empty spotlight. +- [ ] Suppress global keybindings while `tourActive` is true (only Esc and + Enter remain live inside the tooltip). +- [ ] Mount `<TourOverlay />` in `App.tsx` next to the existing dialogs and + gate first-launch activation in `onMount` after `loadState()`. +- [ ] Suppress the keybinding-migration banner while `tourActive` is true. +- [ ] Add a "Restart tour" button to `SettingsDialog`. +- [ ] Resume Phase 2 of the tour the first time a task panel mounts after + Phase 1 completed (driven by the persisted `tourStep` resume token). +- [ ] Restart-tour while `tourActive` is true must run the current step's + `afterLeave` hook (if any), reset `tourStep` to 0, clear + `tourCompletedAt`, and re-activate after `SettingsDialog` closes. +- [ ] Treat empty-string `anchorId` as `null` (centered, no lookup) so a + coding error producing `data-tour-id=""` does not waste the + MutationObserver wait. +- [ ] Route hook failures and anchor-skip events through the structured + logger (category `tour`) rather than `console.warn`; depends on or + coexists with the `add-structured-logging` proposal. +- [ ] Tests: colocated `src/store/tour.test.ts` (matching the repo's + colocated test convention) covering start, navigate (forward + + back, including hook ordering), skip, finish, restart, + restart-while-active, mid-phase-2 quit, existing-user carve-out, + deferred-activation-while-modal-open, and the resume-after-task + -spawn path. +- [ ] Validate with `npm run typecheck`, `npm test`, and + `openspec validate --all --strict`. diff --git a/openspec/changes/add-structured-logging/design.md b/openspec/changes/add-structured-logging/design.md new file mode 100644 index 00000000..e6fe356f --- /dev/null +++ b/openspec/changes/add-structured-logging/design.md @@ -0,0 +1,139 @@ +# Design — Add Structured Logging + +## Two modules, one shape + +The renderer and main process each get their own logger because they live +in separate JS contexts and Electron's IPC boundary is the natural seam. +Both modules expose the same shape so call sites read identically: + +```ts +type LogContext = Record<string, unknown>; + +export function debug(category: string, msg: string, ctx?: LogContext): void; +export function info(category: string, msg: string, ctx?: LogContext): void; +export function warn(category: string, msg: string, ctx?: LogContext): void; +export function error(category: string, msg: string, err?: unknown, ctx?: LogContext): void; +``` + +`category` is a short kebab tag (e.g. `'tasks.spawn'`, `'git.merge'`, +`'pty.fork'`). `ctx` is an optional object — typically `{ taskId, ... }` +— that gets JSON-stringified into the output line. + +## Output format + +A single line per log entry, prefixed with level + category + timestamp: + +``` +[14:23:01.412] WARN tasks.spawn — failed to symlink node_modules {"taskId":"t_abc","reason":"EEXIST"} +``` + +Stack traces from `error()` are appended on a second line. The format is +intentionally `console`-friendly so existing devtools still surface logs. + +## Level gating + +Default minimum level by build: + +| Build | Renderer | Main | +|---------------|------------------------------------------------------|---------------------------------------| +| dev | `debug` | `debug` | +| production | `warn` | `warn` | +| `verbose` on | `debug` regardless of build | `debug` (set via `LogFromRenderer`) | + +The dev / prod determination uses `import.meta.env.DEV` in the renderer +and `process.env.NODE_ENV !== 'production'` in main. `verboseLogging` is +a persisted setting; on change, the renderer pushes the new minimum level +to main via `LogFromRenderer` so both sides stay aligned. + +## Renderer → main forwarding + +Every `warn` and `error` call in the renderer also fires off a +fire-and-forget `LogFromRenderer` IPC with the serialized payload. The +goal is to give main a single timeline that future work (file output, +crash bundles) can consume. The forward is best-effort — if IPC is +unavailable the renderer still logs to its own console. + +`debug` and `info` are NOT forwarded by default; they would dominate the +channel and add no value at production levels. With verbose mode on, +forwarding extends to `info` (still not `debug`, to keep IPC volume +sane). + +## Catch-block sweep policy + +The sweep replaces three patterns: + +1. `.catch(() => {})` and `try { ... } catch {}` → `.catch((err) => + warn('<category>', '<context>', { err }))` if recoverable; + `error(...)` if not. +2. `console.error('msg', err)` → `error('<category>', 'msg', err)`. +3. `console.warn('msg', ...)` → `warn('<category>', 'msg', { ... })`. + +Every callsite picks a category. The expectation is one category per +file or feature; this is enforced by review, not by lint. Existing +`console.warn`/`console.error` calls in tests are left alone. + +## Settings UI + +A "Verbose logging" toggle in `SettingsDialog`'s diagnostics section, +with a one-line explainer. The toggle persists via the existing autosave +path; it does not require a restart — the logger reads the setting +reactively. + +## Known implementation risks + +These are not spec-level requirements but implementation decisions that +need care during the actual build. Calling them out here so they don't +surprise the implementer. + +- **Rate-cap implementation.** The spec's "Forwarding is rate-capped per + category" scenario pins 50 entries per rolling second per category. + The implementation can use a simple ring buffer keyed on category, + with a single timer per category for the suppression notice. The + renderer's own `console` output stays uncapped — only the IPC forward + is bounded. +- **Verbose toggle synchronisation.** The IPC that pushes the new level + to main has no ack and no ordering guarantee. Quick toggling could + briefly leave main at a different level than the renderer. The + implementation should reconcile main's level on each `LogFromRenderer` + payload (the level travels alongside the entry) so drift converges + within one round-trip. +- **Lifecycle gaps.** `LogFromRenderer` is unavailable during preload + init, after `beforeunload`, and during a renderer reload. In each of + these windows the renderer logger MUST still emit to its own console + so startup / shutdown diagnosis is possible without a working IPC. +- **Sweep phasing.** The catch-block sweep is split into per-directory + phases in `tasks.md` (`src/store/`, `src/components/`, + `electron/ipc/`) so each phase is reviewable on its own. The spec's + "Compliance is per-swept-directory" scenario explicitly admits the + transitional state. +- **No-silent-swallow enforcement.** The spec requires the rule but does + not enforce it. A follow-up may add a custom ESLint rule that flags + empty arrow functions in `.catch()` and empty `catch {}` blocks. Until + then, code review is the only check. +- **Sensitive-channel taxonomy.** The spec's IPC trace scenario gates + payload logging on a `SENSITIVE_CHANNELS` set. The initial + implementation populates that set with at minimum: any channel that + forwards user input from a pty, any channel that round-trips a token + or auth header (e.g. ask-code provider channels), and any channel + whose payload includes a path under the user's home directory. The + set is module-level and reviewed alongside new channels. +- **Token / secret leakage beyond IPC.** Verbose mode also exposes git + command arguments and pty events. These are not gated by + `SENSITIVE_CHANNELS` because they are not IPC. Users who turn verbose + on for bug reports may inadvertently include paths, remote URLs, or + env-derived tokens in shared logs. A redaction layer for these + surfaces is out of scope for this proposal but should be flagged in + the verbose toggle's explainer copy. +- **Category sprawl.** Categories are kebab strings with no registry. + Without a follow-up registry or lint rule, near-duplicates (`tasks.spawn` + vs `task-spawn`) will appear. Initial implementation should keep the + list short and document existing categories in `electron/log.ts`. + +## Out of scope + +- Writing logs to a file on disk (deliberately deferred; the timeline + exists in main, future work can add a file sink). +- Remote / crash reporting. +- Log redaction beyond what callers pass in (callers must not put paths + containing tokens or secrets into `ctx`). +- Replacing `console.warn` / `console.error` in test files. diff --git a/openspec/changes/add-structured-logging/proposal.md b/openspec/changes/add-structured-logging/proposal.md new file mode 100644 index 00000000..8df548da --- /dev/null +++ b/openspec/changes/add-structured-logging/proposal.md @@ -0,0 +1,46 @@ +# Add Structured Logging + +## Why + +Errors are handled inconsistently across the renderer and main process. +Some catch blocks call `console.error`, others `console.warn`, and several +swallow the error silently with `.catch(() => {})` (e.g. +`src/store/tasks.ts:539, 552`). When an agent spawn, worktree symlink, or +pty fork fails there is often no log at all — making bug reports +unactionable. There is also no debug-level instrumentation for the IPC, +git, and pty layers in development, where it would shorten diagnosis +cycles substantially. + +## What changes + +- Introduce a small logger module (`src/lib/log.ts` for renderer, + `electron/log.ts` for main) exposing `debug | info | warn | error` with + category tags and an optional structured context object. +- Sweep every catch in `src/store/`, `src/components/`, and + `electron/ipc/` so no error is silently swallowed; each catch routes + to at least `warn` (recoverable) or `error` (user-impacting). +- In development (`import.meta.env.DEV` in the renderer, + `NODE_ENV !== 'production'` in main), enable `debug` and `info` levels + and emit IPC / git / pty traces tagged by category. +- In production, only `warn` and `error` reach the console by default. +- Add a `verboseLogging` toggle in `SettingsDialog` so users helping with + bug reports can flip dev-level output on at runtime. +- Renderer logs at `warn` and above are forwarded to main via a new + `LogFromRenderer` IPC channel so main can hold a single timeline (and + later write it to a file if the implementation chooses). + +## Impact + +- New capability `logging`. +- New modules `src/lib/log.ts` and `electron/log.ts`. +- New IPC channel `LogFromRenderer` (renderer → main, fire-and-forget). + As with every other channel, it must be added both to the `IPC` enum + in `electron/ipc/channels.ts` and to the preload script's allowlist + before the renderer can call it. +- Sweep touches dozens of files split into per-directory phases. The + spec admits a transitional period where earlier-swept directories + are compliant and later ones still hold legacy patterns. +- New persisted field `verboseLogging: boolean` (default `false`); the + loader coerces non-boolean values to `false` to avoid corrupted state + silently enabling verbose mode in production. +- No new runtime dependencies. diff --git a/openspec/changes/add-structured-logging/specs/logging/spec.md b/openspec/changes/add-structured-logging/specs/logging/spec.md new file mode 100644 index 00000000..6e5bccfb --- /dev/null +++ b/openspec/changes/add-structured-logging/specs/logging/spec.md @@ -0,0 +1,257 @@ +# Logging Specification + +## ADDED Requirements + +### Requirement: Unified logger surface + +The app SHALL expose a single logger surface in both the renderer and the +main process with four levels — `debug`, `info`, `warn`, and `error`. The +`debug`, `info`, and `warn` functions accept a category tag, a message, +and an optional structured context object. The `error` function takes the +underlying error or thrown value as a required argument so it cannot be +confused with the optional context object. + +#### Scenario: Logger module is callable from any module + +- **WHEN** a module in `src/` or `electron/` imports the logger and calls + any of the four level functions +- **THEN** the call returns synchronously without throwing +- **AND** the caller does not need to construct any logger instance + +#### Scenario: error has a required error argument + +- **WHEN** the `error` function is called +- **THEN** its signature is `error(category: string, msg: string, err: + unknown, ctx?: Record<string, unknown>)` +- **AND** the third argument is required so a caller cannot accidentally + pass a context object in the error slot + +#### Scenario: Category tag prefixes every entry + +- **WHEN** a logger function is called with a category like `'tasks.spawn'` +- **THEN** the emitted line includes the level, the category, the message, + and a serialised representation of the context object if one was passed + +#### Scenario: Error includes stack trace + +- **WHEN** `error(category, msg, err)` is called with an `Error` instance +- **THEN** the emitted output includes both the message line and the stack + trace from `err` + +#### Scenario: Non-Error throwables are normalised + +- **WHEN** `error(category, msg, err)` is called and `err` is not an + `Error` instance (e.g. a string, a plain object, a rejected non-Error + value, or `undefined`) +- **THEN** the logger normalises the value to a stable string + representation in the emitted output without throwing +- **AND** if the value carries a `stack` property whose own type is a + string the stack is included; non-string `stack` values (functions, + objects, etc.) are ignored + +#### Scenario: error called without a real error value + +- **WHEN** the caller has no underlying error but wants to record an + error-shaped event +- **THEN** the caller passes `undefined` as the third argument +- **AND** the emitted output still carries the `error` level and + category but omits the stack section + +#### Scenario: Unserialisable context falls back safely + +- **WHEN** any logger function is called with a `ctx` object that cannot + be safely JSON-serialised (e.g. it contains circular references, Solid + signals, or DOM nodes) +- **THEN** the logger emits the entry with a placeholder representation + for the offending fields rather than throwing +- **AND** the rest of the entry (level, category, message) still appears +- **AND** if the safe-fallback path itself throws (e.g. `Object.keys()` + hits a Proxy with a throwing trap) the logger emits the entry with + `ctx` omitted entirely rather than propagating the error + +### Requirement: Level gating by build and verbose flag + +The logger SHALL gate output by level according to the current build mode +and the user's `verboseLogging` setting. + +#### Scenario: Production build hides debug and info + +- **WHEN** the build is production and `verboseLogging` is `false` +- **THEN** `debug(...)` and `info(...)` calls produce no output +- **AND** `warn(...)` and `error(...)` calls produce output + +#### Scenario: Development build shows all levels + +- **WHEN** the build is development +- **THEN** all four levels produce output regardless of `verboseLogging` + +#### Scenario: Verbose flag elevates production to debug level + +- **WHEN** the build is production and `verboseLogging` is `true` +- **THEN** all four levels produce output +- **AND** the renderer pushes the elevated level to the main process so + both sides log at the same minimum level + +#### Scenario: Toggling verbose at runtime applies immediately + +- **WHEN** the user toggles `verboseLogging` in `SettingsDialog` +- **THEN** subsequent log calls reflect the new minimum level without + requiring an app restart + +### Requirement: No silent error swallowing + +The codebase SHALL route every caught error through the logger; silent +swallows (e.g. `.catch(() => {})`) are not allowed in production code +paths. Compliance is measured per-directory: a directory is compliant +once its sweep task in `tasks.md` has landed. The proposal explicitly +admits a transitional period in which earlier-swept directories are +compliant and later-swept ones still hold legacy `.catch(() => {})` +calls. + +#### Scenario: Compliance is per-swept-directory during the transition + +- **WHEN** one of the sweep tasks in `tasks.md` has landed but later + ones have not +- **THEN** the swept directory contains no silent swallows +- **AND** the unswept directory may still contain legacy patterns + without violating this requirement until its sweep also lands + +#### Scenario: Recoverable failure logs at warn level + +- **WHEN** a caught error is recoverable and the calling code can continue + with a degraded result +- **THEN** the catch routes through `warn(category, msg, { err })` rather + than discarding the error + +#### Scenario: User-impacting failure logs at error level + +- **WHEN** a caught error prevents the operation from completing in a way + the user can see (e.g. agent spawn fails, worktree setup fails) +- **THEN** the catch routes through `error(category, msg, err)` + +#### Scenario: Test files are exempt + +- **WHEN** the catch lives in a test file (any file under `__tests__` or + matching `*.test.ts`) +- **THEN** the no-silent-swallow rule does not apply + +### Requirement: Renderer logs forward to main + +The renderer logger SHALL forward `warn` and `error` calls (and `info` +calls when verbose mode is on) to the main process so the main process +holds a single timeline of the session. + +#### Scenario: Warn and error forward unconditionally + +- **WHEN** the renderer logger emits a `warn` or `error` entry +- **THEN** the renderer also fires `LogFromRenderer` with the same level, + category, message, and serialised context + +#### Scenario: Info forwards only when verbose + +- **WHEN** the renderer logger emits an `info` entry +- **AND** `verboseLogging` is `true` +- **THEN** the renderer also fires `LogFromRenderer` +- **AND** when `verboseLogging` is `false`, no IPC call is made + +#### Scenario: Debug never forwards + +- **WHEN** the renderer logger emits a `debug` entry under any conditions +- **THEN** no `LogFromRenderer` IPC call is made + +#### Scenario: IPC forwarding is best-effort + +- **WHEN** `LogFromRenderer` cannot be delivered (e.g. preload not yet + initialised) +- **THEN** the renderer still emits the entry to its own `console` +- **AND** the failure does not throw or block the calling code + +#### Scenario: Forwarding is rate-capped per category + +- **WHEN** the renderer logger emits more than 50 forwardable entries + in any rolling one-second window for a single category +- **THEN** further entries in that window for that category are not + forwarded over `LogFromRenderer` +- **AND** at the end of the window a single entry is forwarded with the + same category at level `warn` summarising how many entries were + suppressed +- **AND** the renderer's own `console` continues to receive every entry + +#### Scenario: Forwarding rules apply equally in development + +- **WHEN** the build is development +- **THEN** the renderer still forwards `warn` and `error` entries (and + `info` entries iff `verboseLogging` is `true`) over `LogFromRenderer` +- **AND** forwarding behaviour does not change merely because the build + is dev — the dev build only changes which entries pass the + level-gate, not which ones forward once they pass it + +### Requirement: Verbose logging setting + +The app SHALL expose a `verboseLogging` toggle in settings, persist it +across launches, and default it to `false` for new installs. + +#### Scenario: Default is off + +- **WHEN** persisted state has no value for `verboseLogging` +- **THEN** the loaded state treats it as `false` + +#### Scenario: Non-boolean persisted value coerces to false + +- **WHEN** the persisted state contains `verboseLogging` with any value + that is not a JavaScript boolean (e.g. `"true"`, `1`, `null`, `{}`, or + any other truthy non-boolean) +- **THEN** the loader treats `verboseLogging` as `false` +- **AND** does not allow corrupted persisted state to silently enable + verbose mode in production + +#### Scenario: Toggle persists + +- **WHEN** the user enables the toggle in `SettingsDialog` +- **THEN** the next app launch starts with `verboseLogging` enabled + +#### Scenario: Toggle is visible in settings + +- **WHEN** the user opens `SettingsDialog` +- **THEN** a "Verbose logging" toggle is shown in a diagnostics section +- **AND** a one-line explainer describes that it makes the app log debug + output to the developer console + +### Requirement: Dev-mode IPC, git, and pty traces + +In development builds (and whenever `verboseLogging` is `true`) the app +SHALL emit `debug` traces from the IPC layer, the git helpers, and the +pty layer so a developer can reconstruct what happened without adding +ad-hoc `console.log` calls. + +#### Scenario: IPC handlers trace at debug level + +- **WHEN** a renderer-to-main IPC call is dispatched in dev or with + verbose on +- **THEN** the main process logs a `debug` entry under category `ipc` + with the channel name +- **AND** the payload is included only for channels that are not in the + module-level `SENSITIVE_CHANNELS` set (channels carrying tokens, + paths under the user's home directory, or shell input) +- **AND** an entry is logged on completion with the result kind + (success / failure) + +#### Scenario: Git helpers trace command and exit code + +- **WHEN** a git helper in `electron/ipc/git.ts` runs a git command in + dev or with verbose on +- **THEN** the main process logs a `debug` entry under category `git` + including the command (with arguments) and the exit code + +#### Scenario: Pty lifecycle traces at debug level + +- **WHEN** a pty is spawned, exits, or receives a signal in dev or with + verbose on +- **THEN** the main process logs a `debug` entry under category `pty` + describing the event + +#### Scenario: Production without verbose has no debug traces + +- **WHEN** the build is production and `verboseLogging` is `false` +- **THEN** none of the `ipc`, `git`, or `pty` debug traces produce + output diff --git a/openspec/changes/add-structured-logging/tasks.md b/openspec/changes/add-structured-logging/tasks.md new file mode 100644 index 00000000..699bdede --- /dev/null +++ b/openspec/changes/add-structured-logging/tasks.md @@ -0,0 +1,50 @@ +# Tasks — Add Structured Logging + +- [ ] Add new IPC channel `LogFromRenderer` to `electron/ipc/channels.ts` + and the preload allowlist. +- [ ] Implement `electron/log.ts`: `debug | info | warn | error` with + category tags, level gating by build, and a handler for + `LogFromRenderer` that funnels renderer entries into the same + output stream. The `error` function takes a required `err: unknown` + argument before the optional `ctx`, so it cannot be confused with + the optional context object on the other levels. +- [ ] Implement `src/lib/log.ts`: same surface as the main logger; emits + to `console` and forwards `warn`/`error` (and `info` when verbose) + to main via `LogFromRenderer`. Forwarding includes a per-second + rate cap per category to protect against flood loops. +- [ ] Implement safe serialisation: non-`Error` throwables are normalised + to a stable string; `ctx` objects with circular references or + unserialisable members fall back to a placeholder representation + so the logger never throws. +- [ ] Add persisted field `verboseLogging: boolean` (default `false`) to + `PersistedState` in `src/store/types.ts` and the loader/saver in + `src/store/persistence.ts`. The loader must coerce non-boolean + values to `false` so corrupted persisted state cannot silently + enable verbose mode in production. +- [ ] Wire the renderer logger to read `verboseLogging` reactively so + toggling the setting takes effect without a restart; piggy-back + the current level on each `LogFromRenderer` payload so main + reconciles within one round-trip. +- [ ] Add a "Verbose logging" toggle in `SettingsDialog` under a + diagnostics section, with explainer copy that warns logs may + include paths and command arguments before sharing them. +- [ ] Phase 1 — sweep `src/store/`: route every catch through the + renderer logger; replace silent swallows with `warn` or `error`. +- [ ] Phase 2 — sweep `src/components/`: same treatment. +- [ ] Phase 3 — sweep `electron/ipc/`: same treatment via the main + logger. +- [ ] Add debug traces at category `ipc` (every IPC handler entry/exit), + `git` (every git command + exit code), `pty` (spawn, exit, + signal). These are `debug` level so they only show in dev or with + verbose on. +- [ ] Tests colocated next to the modules (the repo's convention is + `*.test.ts` next to the file, not a `__tests__/` directory): + `src/lib/log.test.ts` and `electron/log.test.ts` covering level + gating, category formatting, forwarding behavior, the per-category + rate cap and suppression notice, the `verbose` runtime flip, + circular-ctx safety (including a Proxy whose trap throws), + non-Error normalisation (including non-string `stack`), + `error(cat, msg, undefined)` omitting the stack section, and + corrupted `verboseLogging` coercion to `false`. +- [ ] Validate with `npm run typecheck`, `npm test`, and + `openspec validate --all --strict`. diff --git a/openspec/changes/improve-dialog-accessibility/design.md b/openspec/changes/improve-dialog-accessibility/design.md new file mode 100644 index 00000000..224ad6ac --- /dev/null +++ b/openspec/changes/improve-dialog-accessibility/design.md @@ -0,0 +1,147 @@ +# Design — Improve Dialog Accessibility + +## Why a design doc + +The change is mostly mechanical (add ARIA attributes, wire title ids), but +two things are subtle enough to warrant a written design: the +`ConfirmDialog` API extension that `MergeDialog` depends on, and the +Portal-aware CSS scope for `:focus-visible`. Everything else lives in the +spec. + +## ConfirmDialog API extension + +`ConfirmDialog` currently accepts `title: string` and renders its own +untagged `<h2>`. `MergeDialog` (and any future wrapper) cannot point +`aria-labelledby` at that heading because it has no id. + +The extension: + +- Add optional `labelledBy?: string` and `describedBy?: string` props. +- Always generate a title id via Solid's `createUniqueId()` and apply it + to the rendered `<h2>` so the heading is reliably referenceable. +- When the consumer does NOT supply `labelledBy`, forward + `ConfirmDialog`'s own generated id to `Dialog` so the link works + out of the box. +- When the consumer DOES supply `labelledBy`, forward that value + unchanged; `ConfirmDialog`'s generated id remains on the `<h2>` but + is unused as a label reference. This avoids overwriting the + consumer's intent and the duplicate id is harmless. +- Forward `describedBy` to `Dialog` if the consumer passes one; + `ConfirmDialog` does not synthesise a description id. + +`MergeDialog` keeps its `<ConfirmDialog title="..."/>` call-site +unchanged; the link is set up automatically because the generated id is +forwarded. + +Existing `ConfirmDialog` call-sites continue to work unmodified — the +extension is purely additive. + +## Stack-aware `aria-modal` + +The app already supports nested dialogs (e.g. confirm-on-close on top of +`MergeDialog`). Two simultaneously-open `aria-modal="true"` panels confuse +some assistive technologies because both claim to trap navigation. + +The implementation chooses one of: + +- **A — DOM order:** the panel that ends up topmost in document order at + render time keeps `aria-modal`; the others render without it. +- **B — Ref-counted store:** a tiny module-level array tracks open + panels by id; only the last entry's panel renders `aria-modal="true"`. + +Option B is more deterministic and survives portals (which Option A's DOM +ordering doesn't reliably handle). Recommend B. + +## `:focus-visible` scope through a Portal + +`Dialog` mounts via Solid `<Portal>` to `document.body`, so a CSS rule +written as a descendant of the app shell (e.g. +`#app .panel button:focus-visible`) does not match. + +The fix is a class hook on the panel itself: + +```css +.dialog-panel + :is( + button, + input, + select, + textarea, + a[href], + [tabindex]:not([tabindex='-1']), + [role='button'], + [role='switch'], + [role='checkbox'], + [role='link'] + ):focus-visible { + outline: 2px solid var(--accent); + outline-offset: 2px; +} +``` + +Adding the `dialog-panel` class on the panel element in `Dialog.tsx` +makes the rule portable to portaled and non-portaled mounts alike. The +selector list mirrors the spec's enumeration of interactive elements so +custom widgets (toggles, icon buttons, etc.) pick up the indicator. + +## DiffViewerDialog visually hidden title + +`DiffViewerDialog` currently has no heading. The spec mandates a clip / +sr-only pattern so the heading is in the accessibility tree but +invisible. The repo does not currently ship an `.sr-only` utility; the +implementation should add one alongside the focus-visible rule: + +```css +.dialog-sr-only { + position: absolute; + width: 1px; + height: 1px; + padding: 0; + margin: -1px; + overflow: hidden; + clip: rect(0, 0, 0, 0); + white-space: nowrap; + border: 0; +} +``` + +Naming it `.dialog-sr-only` (rather than the more common `.sr-only`) +keeps the new utility scoped to this proposal so it cannot collide with +a future general-purpose visually-hidden helper. + +## `describedBy` usage guidance + +`describedBy` is optional per the spec; this section is **advisory** +only and does not bind implementations. The guidance below describes +the editorial choice the initial implementation should make: + +- `ConfirmDialog` — pass `describedBy` when `message` is non-empty. +- `MergeDialog` — pass `describedBy`; the merge-destination paragraph + is a natural description. +- `SettingsDialog`, `HelpDialog` — do not pass `describedBy`; they have + sectioned content with multiple sub-headings, and pointing the + description at one section misleads the user. +- `NewTaskDialog`, `DiffViewerDialog` — same; do not pass. + +A future change can revisit any of these without violating the spec, +which only requires `describedBy` to be optional and to render correctly +when supplied. + +## Test surface + +Tests assert structure only. jsdom does not run accessible-name +computation, so a green test does **not** prove a screen reader will +announce the title — only that the markup is shaped correctly. +Manual verification with VoiceOver / NVDA is recommended for at least +one dialog per category before this proposal is archived. + +## Out of scope + +- Live regions for dialog state changes (e.g. "saved", "error"). +- Reduced-motion handling for dialog open/close animations. +- Touch / mobile screen reader testing (the app is desktop-only). +- Replacing the focus-trap implementation; the existing + `lib/focus-trap.ts` is reused. +- A redaction or fallback `aria-label` path for future dialogs without + a title element. This is noted as a forward-compatibility item; the + current spec assumes every consuming dialog provides a title. diff --git a/openspec/changes/improve-dialog-accessibility/proposal.md b/openspec/changes/improve-dialog-accessibility/proposal.md new file mode 100644 index 00000000..7a8d7252 --- /dev/null +++ b/openspec/changes/improve-dialog-accessibility/proposal.md @@ -0,0 +1,51 @@ +# Improve Dialog Accessibility + +## Why + +The base `Dialog` wrapper in `src/components/Dialog.tsx` provides a focus +trap and ESC-to-close, but it does not set `role="dialog"`, +`aria-modal="true"`, or link a title via `aria-labelledby`. Every dialog in +the app inherits this gap: `SettingsDialog`, `NewTaskDialog`, `HelpDialog`, +`ConfirmDialog`, `MergeDialog`, `DiffViewerDialog`. Screen-reader users +cannot tell they have entered a modal, the dialog title is not announced, +and several icon-only close buttons lack `aria-label`. Visible focus +indicators on interactive elements inside dialogs are inconsistent (the +panel itself has `outline: 'none'` at `Dialog.tsx:99`). The app is +otherwise keyboard-first, so closing this gap is high-leverage and +low-cost. + +## What changes + +- Extend `Dialog` to set `role="dialog"` and `aria-modal="true"` on its + panel, and to accept new optional props `labelledBy` and `describedBy` + that render as `aria-labelledby` / `aria-describedby` on the panel. + When a dialog opens on top of another dialog, only the topmost panel + carries `aria-modal="true"`. +- Extend `ConfirmDialog`'s API to accept and forward `labelledBy` / + `describedBy` (today it only accepts a `title: string` and renders its + own untagged `<h2>`), and to generate a unique id for that title via + `createUniqueId` so consumers like `MergeDialog` get a working link + without restructuring. +- Update each consuming dialog (`SettingsDialog`, `NewTaskDialog`, + `HelpDialog`, `ConfirmDialog`, `DiffViewerDialog`) to attach a unique + id to its title element and pass that id as `labelledBy`. `MergeDialog` + inherits via `ConfirmDialog`. +- Give `DiffViewerDialog` a heading (visible or visually hidden via the + clip / sr-only pattern) so it has something to link to. +- Add `aria-label` to icon-only close buttons that exist today + (`SettingsDialog`, `HelpDialog`). +- Add a visible `:focus-visible` outline rule on interactive elements + inside dialogs (`button`, `input`, `select`, `textarea`), expressed via + a class hook on the panel so it survives `<Portal>` mounting outside + the app root. + +## Impact + +- New capability `dialog-accessibility`. +- API change to `Dialog`: two new optional props, additive, no callers + break. +- API change to `ConfirmDialog`: two new optional props plus an + internally-generated title id; existing call-sites keep working. +- Touches each consuming dialog component to attach a title id and pass + `labelledBy`. +- No new IPC channels, no persisted state changes, no new dependencies. diff --git a/openspec/changes/improve-dialog-accessibility/specs/dialog-accessibility/spec.md b/openspec/changes/improve-dialog-accessibility/specs/dialog-accessibility/spec.md new file mode 100644 index 00000000..1c90b2ba --- /dev/null +++ b/openspec/changes/improve-dialog-accessibility/specs/dialog-accessibility/spec.md @@ -0,0 +1,148 @@ +# Dialog Accessibility Specification + +## ADDED Requirements + +### Requirement: Dialog panels declare themselves as modal dialogs + +Every modal dialog mounted via the shared `Dialog` wrapper SHALL render +its panel with `role="dialog"` and `aria-modal="true"` so assistive +technology can recognise it as a modal context, and the focus trap that +makes that promise true SHALL remain wired up. + +#### Scenario: Base Dialog sets the modal role + +- **WHEN** any dialog renders via the shared `Dialog` wrapper +- **THEN** the panel element has both `role="dialog"` and + `aria-modal="true"` + +#### Scenario: aria-modal is paired with a working focus trap + +- **WHEN** a `Dialog` is open with `aria-modal="true"` +- **THEN** keyboard focus is trapped to descendants of the panel +- **AND** Tab and Shift-Tab cycle within the panel without escaping to + the dimmed background + +#### Scenario: Only one aria-modal panel is active at a time + +- **WHEN** a second `Dialog` opens on top of an already-open dialog (e.g. + a confirm-on-close over `MergeDialog`) +- **THEN** only the topmost panel has `aria-modal="true"` +- **AND** the underlying panel's `aria-modal` is removed (or the panel + is re-rendered without it) until the topmost dialog closes + +#### Scenario: aria-modal is restored when the topmost dialog closes + +- **WHEN** the topmost of two stacked dialogs closes while the + underlying dialog is still open +- **THEN** the underlying dialog regains `aria-modal="true"` on its + panel +- **AND** subsequent dialog opens see it as the new top of the stack + +### Requirement: Dialog panels link to their title + +Every dialog SHALL link its panel to a visible (or visually hidden) title +element via `aria-labelledby` so the title is announced when the dialog +opens, and the linked title element SHALL carry non-empty accessible +text. + +#### Scenario: Dialog accepts a labelledBy prop + +- **WHEN** a consumer passes `labelledBy="some-id"` to `Dialog` +- **THEN** the panel renders `aria-labelledby="some-id"` + +#### Scenario: Each consuming dialog provides a title id + +- **WHEN** any of `SettingsDialog`, `NewTaskDialog`, `HelpDialog`, + `ConfirmDialog`, `MergeDialog`, or `DiffViewerDialog` renders +- **THEN** its title element has an id that is unique within the + document for the lifetime of the dialog's open mount (produced by + `createUniqueId` so two simultaneously-open dialogs cannot collide, + and stable across re-renders of the same component instance) +- **AND** the dialog passes that id to `Dialog` as `labelledBy` +- **AND** the referenced element exists in the rendered DOM + +#### Scenario: Consumer-supplied labelledBy wins over ConfirmDialog's title + +- **WHEN** a consumer passes a `labelledBy` to `ConfirmDialog` while + also providing a `title` string +- **THEN** the value forwarded to `Dialog` as `aria-labelledby` is the + consumer-supplied id +- **AND** `ConfirmDialog`'s own internally-generated title id is left + on the rendered `<h2>` but is not used as `aria-labelledby` + +#### Scenario: Title element has accessible text + +- **WHEN** an element referenced by a dialog's `aria-labelledby` is in + the DOM +- **THEN** its `textContent` after trimming whitespace is non-empty +- **AND** it is not itself hidden via `aria-hidden="true"`, + `display: none`, or `visibility: hidden` +- **AND** the spec does not attempt to police further CSS-based + hiding tricks (e.g. `font-size: 0`, `color: transparent`); the + proposal relies on author discipline for those + +#### Scenario: DiffViewerDialog provides a visually hidden title + +- **WHEN** `DiffViewerDialog` renders +- **THEN** it includes a heading whose id is passed as `labelledBy` +- **AND** if the heading is visually hidden it uses the clip / sr-only + pattern (which leaves the node in the accessibility tree) rather than + `display: none` or `visibility: hidden` (which removes it) + +### Requirement: Dialog panels can describe themselves + +The shared `Dialog` SHALL accept an optional `describedBy` prop and render +it as `aria-describedby` on the panel so longer dialog bodies can be +announced by assistive technology. + +#### Scenario: Dialog accepts a describedBy prop + +- **WHEN** a consumer passes `describedBy="some-id"` to `Dialog` +- **THEN** the panel renders `aria-describedby="some-id"` + +#### Scenario: describedBy is optional + +- **WHEN** a consumer omits `describedBy` +- **THEN** the panel renders without an `aria-describedby` attribute + +### Requirement: Icon-only close buttons have an accessible name + +Every icon-only close button rendered by a dialog SHALL expose an +accessible name via `aria-label` so screen-reader users can identify it. + +#### Scenario: Close button has aria-label + +- **WHEN** a dialog renders an icon-only close button (no visible text) +- **THEN** the button has an `aria-label` whose value clearly identifies + it as the close action (e.g. `"Close dialog"` or `"Close settings"`) + +### Requirement: Visible focus indicators inside dialogs + +Interactive elements inside dialog panels SHALL show a visible focus +indicator when focused via keyboard, distinct from the hover or active +state. The focus-style scope SHALL be expressed in a way that survives +the panel being mounted in a Solid `<Portal>` outside the app root. + +#### Scenario: Interactive elements inside dialogs show a focus ring + +- **WHEN** an interactive element inside a dialog panel receives focus + via Tab or Shift-Tab — including any of `button`, `input`, `select`, + `textarea`, `a[href]`, `[tabindex]:not([tabindex="-1"])`, + `[role="button"]`, `[role="switch"]`, `[role="checkbox"]`, and + `[role="link"]` +- **THEN** a visible focus indicator (e.g. an outline or ring matching + the app's accent colour) is rendered + +#### Scenario: Indicator does not appear on mouse interaction alone + +- **WHEN** the same element receives focus via mouse click +- **THEN** the focus indicator follows the standard `:focus-visible` + semantics so it does not appear for pointer-only interaction + +#### Scenario: Focus styles work despite Portal-mounted panels + +- **WHEN** a dialog panel is rendered inside a Solid `<Portal>` and is + not a descendant of the app root container +- **THEN** the focus-visible rule still applies via a class hook + (e.g. `.dialog-panel`) on the panel itself rather than via a + descendant selector rooted at the app shell diff --git a/openspec/changes/improve-dialog-accessibility/tasks.md b/openspec/changes/improve-dialog-accessibility/tasks.md new file mode 100644 index 00000000..e6e54399 --- /dev/null +++ b/openspec/changes/improve-dialog-accessibility/tasks.md @@ -0,0 +1,60 @@ +# Tasks — Improve Dialog Accessibility + +- [ ] Extend `src/components/Dialog.tsx`: set `role="dialog"` and + `aria-modal="true"` on the panel; accept optional `labelledBy` and + `describedBy` props; render them as `aria-labelledby` / + `aria-describedby` on the panel; add a stable class hook (e.g. + `.dialog-panel`) on the panel so global focus styles can target + it through a `<Portal>`. +- [ ] Stack-aware `aria-modal`: when more than one `Dialog` is open at + once, only the topmost carries `aria-modal="true"`. Implement via + a tiny ref-counted store of open-dialog ids, or by reading the + DOM order of mounted panels. +- [ ] Extend `src/components/ConfirmDialog.tsx` API: accept optional + `labelledBy` and `describedBy` and forward to `Dialog`; generate + its own title id via `createUniqueId` and forward that as + `labelledBy` when a consumer does not supply one. Existing + call-sites keep working. +- [ ] Update `SettingsDialog.tsx`: give the title an id (via + `createUniqueId`) and pass it as `labelledBy`; add `aria-label` to + the existing icon-only close button. +- [ ] Update `HelpDialog.tsx`: same — unique title id + `labelledBy`; + add `aria-label` to the existing icon-only close button. +- [ ] Update `NewTaskDialog.tsx`: unique title id + `labelledBy`. (The + dialog has no icon-only close button — its dismissal is via the + footer "Cancel" button — so no `aria-label` work is needed here.) +- [ ] Update `MergeDialog.tsx`: pass-through of `labelledBy` is now + handled by the extended `ConfirmDialog` API; verify the link + resolves at render time. (`MergeDialog` does not render its own + close button, so no `aria-label` work is needed here.) +- [ ] Update `DiffViewerDialog.tsx`: add a heading element using the + `.dialog-sr-only` utility class (clip-based hiding that keeps the + node in the accessibility tree, **not** `display: none` or + `visibility: hidden`) and wire its id as `labelledBy`. +- [ ] Add a `:focus-visible` outline rule in `src/styles.css` keyed on + `.dialog-panel` and matching the spec's broader interactive + enumeration (`button`, `input`, `select`, `textarea`, `a[href]`, + `[tabindex]:not([tabindex="-1"])`, `[role="button"]`, + `[role="switch"]`, `[role="checkbox"]`, `[role="link"]`) so the + rule covers icon buttons, toggles, and links inside dialogs. +- [ ] Add the `.dialog-sr-only` utility (clip / sr-only) to + `src/styles.css` so `DiffViewerDialog`'s hidden heading can use + it without inventing a recipe. +- [ ] Stack-aware `aria-modal` must restore the underlying panel's + `aria-modal="true"` when a topmost dialog closes, so reopening a + third dialog still finds the underlying as the next-top. Cover + this in tests. +- [ ] Tests: add `Dialog.test.tsx`, `ConfirmDialog.test.tsx`, and + per-dialog assertions colocated next to each component (the + repo's convention is colocated `*.test.ts` / `*.test.tsx`, not a + `__tests__/` subdirectory) covering: panel has `role="dialog"` + and `aria-modal="true"`; the focus trap holds Tab inside the + panel; `aria-labelledby` resolves to a node whose trimmed + `textContent` is non-empty; stack-aware `aria-modal` removes the + attribute from the underlying panel when a second dialog opens + and restores it when the topmost closes; consumer-supplied + `labelledBy` to `ConfirmDialog` wins over the internal generated + id. Note that jsdom does not run AT name computation, so these + tests verify structure, not announcement. +- [ ] Validate with `npm run typecheck`, `npm test`, and + `openspec validate --all --strict`.