From c7dc64e9a0bd3019096fbe4f1c36af009ca65d03 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 26 Apr 2026 22:09:43 +0000 Subject: [PATCH 1/5] docs(openspec): propose first-launch onboarding tour MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Drafts a change proposal for a skippable, two-phase guided tour that walks first-time users through the worktree-per-task model — project picker, new task, agent selector, terminal, changed files, merge, help. Uses data-tour-id anchors on existing components, persists a single tourCompletedAt timestamp, and adds a Restart-tour entry to settings. https://claude.ai/code/session_016gGqJbqCixVHiJK7tU5YGs --- .../changes/add-onboarding-tour/design.md | 102 ++++++++++++ .../changes/add-onboarding-tour/proposal.md | 38 +++++ .../specs/onboarding-tour/spec.md | 156 ++++++++++++++++++ openspec/changes/add-onboarding-tour/tasks.md | 24 +++ 4 files changed, 320 insertions(+) create mode 100644 openspec/changes/add-onboarding-tour/design.md create mode 100644 openspec/changes/add-onboarding-tour/proposal.md create mode 100644 openspec/changes/add-onboarding-tour/specs/onboarding-tour/spec.md create mode 100644 openspec/changes/add-onboarding-tour/tasks.md diff --git a/openspec/changes/add-onboarding-tour/design.md b/openspec/changes/add-onboarding-tour/design.md new file mode 100644 index 00000000..51647b34 --- /dev/null +++ b/openspec/changes/add-onboarding-tour/design.md @@ -0,0 +1,102 @@ +# 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`. The +field is `null` for fresh installs and existing users alike — meaning existing +users who upgrade also see the tour once. This is intentional: the tour +explains the worktree model, which existing users may have figured out +imperfectly. + +The tour activates from `App.tsx` `onMount`, after `loadState()` resolves, if +`tourCompletedAt === null` and no other modal is open. If the keybinding +migration banner would also have been shown, the tour suppresses it; on +completion or skip, the banner can render on the next state change. + +## 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 | `tour-help-button` | "Press `?` anytime — full shortcut list lives here. 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. +- `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. + +## 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..7c5e5ba9 --- /dev/null +++ b/openspec/changes/add-onboarding-tour/proposal.md @@ -0,0 +1,38 @@ +# 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. +- 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), `SettingsDialog.tsx` ("Restart + tour" button), and a small set of existing components (`Sidebar`, + `NewTaskDialog`, `TaskPanel`, `HelpDialog`) which gain `data-tour-id` + attributes. +- 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..67767d3e --- /dev/null +++ b/openspec/changes/add-onboarding-tour/specs/onboarding-tour/spec.md @@ -0,0 +1,156 @@ +# 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. + +#### Scenario: Fresh install starts the tour + +- **WHEN** the app boots and persisted state's `tourCompletedAt` is `null` +- **AND** no other modal dialog is open +- **THEN** the app activates the tour at step 0 after `loadState()` resolves + +#### 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: 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 `afterLeave` and `beforeEnter` hooks run in the same order as + forward navigation + +#### 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=""` 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 is found within 300 ms +- **THEN** the tour skips the step and advances to the next one + +#### 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` +- **THEN** the overlay renders a uniform dimmer with no cutout +- **AND** the tooltip is centered in the viewport + +### 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. + +#### 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 + +#### 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 (steps that explain the terminal, + changed-files panel, merge action, and help) + +#### Scenario: Skipping in either phase finalises the tour + +- **WHEN** the user skips during phase 1 or phase 2 +- **THEN** `tourCompletedAt` is set +- **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. + +#### Scenario: Restart tour clears completion + +- **WHEN** the user clicks "Restart tour" in `SettingsDialog` +- **THEN** `tourCompletedAt` is set to `null` +- **AND** the `tourStep` resume token is reset +- **AND** the tour activates immediately at step 0 + +### 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 diff --git a/openspec/changes/add-onboarding-tour/tasks.md b/openspec/changes/add-onboarding-tour/tasks.md new file mode 100644 index 00000000..724ec635 --- /dev/null +++ b/openspec/changes/add-onboarding-tour/tasks.md @@ -0,0 +1,24 @@ +# 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`. +- [ ] New component `src/components/TourOverlay.tsx`: dimmer with SVG cutout, + tooltip panel, prev/next/skip controls, focus trap, Esc-to-skip. +- [ ] Add `data-tour-id` anchors to `Sidebar` (project picker, new-task + button, merge action), `NewTaskDialog` (agent selector), `TaskPanel` + (terminal + changed-files), and `WindowTitleBar` or `Sidebar` (help + button). +- [ ] Mount `` 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). +- [ ] Tests: `src/store/__tests__/tour.test.ts` covering start, navigate, + skip, finish, and the resume-after-task-spawn path. +- [ ] Validate with `npm run typecheck`, `npm test`, and + `openspec validate --all --strict`. From f078bf675f400d4b0b020e61c7572e5334a9744f Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 26 Apr 2026 22:21:13 +0000 Subject: [PATCH 2/5] docs(openspec): incorporate review feedback on onboarding tour Patches the onboarding-tour proposal after a multi-agent review surfaced spec-level bugs and missing scenarios. Spec changes: - Fix back-navigation hook ordering wording so afterLeave/beforeEnter pairing is unambiguous (prevents closing a dialog the prior step needs). - Replace step 8's missing tour-help-button anchor with a centered final step; the app has no visible help button to point at. - Add scenarios for: existing-user upgrade (auto-mark complete if prior data exists), modal-open-at-activation (defer instead of abort), mid-phase-1 quit (restart phase 1 cleanly), phase-2 token persisting across launches, anchor disappearing mid-step, and aria-live step announcements. - Make the anchor-missing wait timeout-agnostic in the spec; the implementation-side choice (MutationObserver + 3 s fallback) is documented as a known risk in design.md. Design and tasks updated with: existing-user carve-out, deferred activation while modals are open, anchor existence test, global-shortcut suppression while tourActive, and single-window scoping note. https://claude.ai/code/session_016gGqJbqCixVHiJK7tU5YGs --- .../changes/add-onboarding-tour/design.md | 61 +++++++++++++--- .../changes/add-onboarding-tour/proposal.md | 13 ++-- .../specs/onboarding-tour/spec.md | 69 +++++++++++++++++-- openspec/changes/add-onboarding-tour/tasks.md | 33 +++++++-- 4 files changed, 149 insertions(+), 27 deletions(-) diff --git a/openspec/changes/add-onboarding-tour/design.md b/openspec/changes/add-onboarding-tour/design.md index 51647b34..0f5b815b 100644 --- a/openspec/changes/add-onboarding-tour/design.md +++ b/openspec/changes/add-onboarding-tour/design.md @@ -3,16 +3,24 @@ ## 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`. The -field is `null` for fresh installs and existing users alike — meaning existing -users who upgrade also see the tour once. This is intentional: the tour -explains the worktree model, which existing users may have figured out -imperfectly. - -The tour activates from `App.tsx` `onMount`, after `loadState()` resolves, if -`tourCompletedAt === null` and no other modal is open. If the keybinding -migration banner would also have been shown, the tour suppresses it; on -completion or skip, the banner can render on the next state change. +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 @@ -61,7 +69,7 @@ anchor element so the spotlight follows window resizes and layout shifts. | 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 | `tour-help-button` | "Press `?` anytime — full shortcut list lives here. You're done." | +| 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 @@ -90,10 +98,41 @@ re-trigger. `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. +- **Global shortcut suppression.** While `tourActive` is true, suppress + global keybindings (new task, focus mode, help, settings, etc.) so a key + press does not open another modal on top of the tour. Only Esc (skip) + and Enter (next) should be live. +- **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. + ## Out of scope - Per-OS or per-agent tour variants. diff --git a/openspec/changes/add-onboarding-tour/proposal.md b/openspec/changes/add-onboarding-tour/proposal.md index 7c5e5ba9..f4abb102 100644 --- a/openspec/changes/add-onboarding-tour/proposal.md +++ b/openspec/changes/add-onboarding-tour/proposal.md @@ -20,6 +20,9 @@ gap without forcing users to read documentation. 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 @@ -31,8 +34,10 @@ gap without forcing users to read documentation. - 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), `SettingsDialog.tsx` ("Restart - tour" button), and a small set of existing components (`Sidebar`, - `NewTaskDialog`, `TaskPanel`, `HelpDialog`) which gain `data-tour-id` - attributes. +- 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 index 67767d3e..bd8f02e8 100644 --- a/openspec/changes/add-onboarding-tour/specs/onboarding-tour/spec.md +++ b/openspec/changes/add-onboarding-tour/specs/onboarding-tour/spec.md @@ -6,13 +6,25 @@ 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. +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** no other modal dialog is open -- **THEN** the app activates the tour at step 0 after `loadState()` resolves +- **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 @@ -20,6 +32,15 @@ been skipped. - **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 @@ -43,8 +64,10 @@ flow at any step. - **WHEN** the user clicks "Back" on any step except step 0 - **THEN** `tourStep` decrements by 1 -- **AND** the `afterLeave` and `beforeEnter` hooks run in the same order as - forward navigation +- **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 @@ -76,8 +99,17 @@ window resizes or layout changes. #### Scenario: Anchor missing when step activates - **WHEN** a step with a non-null `anchorId` activates -- **AND** no matching element is found within 300 ms +- **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 @@ -113,10 +145,27 @@ still see all steps without the app fabricating demo data. - **THEN** the tour activates phase 2 (steps that explain the terminal, changed-files panel, merge action, and help) +#### 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: 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 @@ -154,3 +203,11 @@ assistive technology, and respect the user's reduced-motion preference. - **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 index 724ec635..d1b23351 100644 --- a/openspec/changes/add-onboarding-tour/tasks.md +++ b/openspec/changes/add-onboarding-tour/tasks.md @@ -6,19 +6,40 @@ - [ ] 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. + 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), `TaskPanel` - (terminal + changed-files), and `WindowTitleBar` or `Sidebar` (help - button). + 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). -- [ ] Tests: `src/store/__tests__/tour.test.ts` covering start, navigate, - skip, finish, and the resume-after-task-spawn path. +- [ ] Tests: `src/store/__tests__/tour.test.ts` covering start, navigate + (forward + back, including hook ordering), skip, finish, restart, + 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`. From 075bfea5614a28bbd7e5612455fbd00ed071389f Mon Sep 17 00:00:00 2001 From: Claude <noreply@anthropic.com> Date: Sun, 26 Apr 2026 22:29:27 +0000 Subject: [PATCH 3/5] docs(openspec): propose structured logging and dialog accessibility Two follow-up proposals from the brainstorm review. add-structured-logging: introduces a renderer/main logger pair, sweeps all catch blocks so no error is silently swallowed, gates output by build mode, adds a verboseLogging toggle to settings, and forwards renderer warns/errors to main via a new IPC channel for a single timeline. improve-dialog-accessibility: adds role="dialog", aria-modal, and labelledBy / describedBy props to the shared Dialog wrapper; updates each consuming dialog (Settings, NewTask, Help, Confirm, Merge, DiffViewer) to wire a stable title id; adds aria-label to icon-only close buttons; adds visible focus-visible indicators inside dialog panels. Both proposals are purely additive (single ## ADDED Requirements section) and ready for `openspec validate --all --strict` once the CLI is available locally. https://claude.ai/code/session_016gGqJbqCixVHiJK7tU5YGs --- .../changes/add-structured-logging/design.md | 89 ++++++++++++ .../add-structured-logging/proposal.md | 40 ++++++ .../specs/logging/spec.md | 136 ++++++++++++++++++ .../changes/add-structured-logging/tasks.md | 33 +++++ .../improve-dialog-accessibility/proposal.md | 35 +++++ .../specs/dialog-accessibility/spec.md | 91 ++++++++++++ .../improve-dialog-accessibility/tasks.md | 26 ++++ 7 files changed, 450 insertions(+) create mode 100644 openspec/changes/add-structured-logging/design.md create mode 100644 openspec/changes/add-structured-logging/proposal.md create mode 100644 openspec/changes/add-structured-logging/specs/logging/spec.md create mode 100644 openspec/changes/add-structured-logging/tasks.md create mode 100644 openspec/changes/improve-dialog-accessibility/proposal.md create mode 100644 openspec/changes/improve-dialog-accessibility/specs/dialog-accessibility/spec.md create mode 100644 openspec/changes/improve-dialog-accessibility/tasks.md diff --git a/openspec/changes/add-structured-logging/design.md b/openspec/changes/add-structured-logging/design.md new file mode 100644 index 00000000..e95dc577 --- /dev/null +++ b/openspec/changes/add-structured-logging/design.md @@ -0,0 +1,89 @@ +# 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 and a "Reveal log location" link in dev (no-op +in prod for now). Toggle persists via the existing autosave path; it +does not require a restart — the logger reads the setting reactively. + +## 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..97d47ab3 --- /dev/null +++ b/openspec/changes/add-structured-logging/proposal.md @@ -0,0 +1,40 @@ +# 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). +- Sweep touches dozens of files; no behavioral change beyond replacing + `console.*` and silent swallows with logger calls. +- New persisted field `verboseLogging: boolean` (default `false`). +- 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..86894da5 --- /dev/null +++ b/openspec/changes/add-structured-logging/specs/logging/spec.md @@ -0,0 +1,136 @@ +# 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` — that +accept a category tag, a message, an optional structured context object, +and (for `error`) the underlying error or thrown value. + +#### 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: 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` + +### 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. + +#### 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 + +### 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: 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 diff --git a/openspec/changes/add-structured-logging/tasks.md b/openspec/changes/add-structured-logging/tasks.md new file mode 100644 index 00000000..0b893c0a --- /dev/null +++ b/openspec/changes/add-structured-logging/tasks.md @@ -0,0 +1,33 @@ +# 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. +- [ ] 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`. +- [ ] Add persisted field `verboseLogging: boolean` (default `false`) to + `PersistedState` in `src/store/types.ts` and the loader/saver in + `src/store/persistence.ts`. +- [ ] Wire the renderer logger to read `verboseLogging` reactively so + toggling the setting takes effect without a restart; push the new + level to main on each change. +- [ ] Add a "Verbose logging" toggle in `SettingsDialog` under a + diagnostics section. +- [ ] Sweep every catch in `src/store/` and `src/components/` and route + through the renderer logger; replace silent swallows with `warn` + or `error` as appropriate. +- [ ] Sweep every catch in `electron/ipc/` and route through the main + logger; replace silent swallows similarly. +- [ ] Add dev-only debug traces at category `ipc` (every IPC handler + entry/exit), `git` (every git command + exit code), `pty` (spawn, + exit, signal). These should be `debug` level so they only show + with verbose on or in dev. +- [ ] Tests: `src/lib/__tests__/log.test.ts` and + `electron/__tests__/log.test.ts` covering level gating, category + formatting, forwarding behavior, and the `verbose` runtime flip. +- [ ] Validate with `npm run typecheck`, `npm test`, and + `openspec validate --all --strict`. diff --git a/openspec/changes/improve-dialog-accessibility/proposal.md b/openspec/changes/improve-dialog-accessibility/proposal.md new file mode 100644 index 00000000..6a103d84 --- /dev/null +++ b/openspec/changes/improve-dialog-accessibility/proposal.md @@ -0,0 +1,35 @@ +# 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. +- Update each consuming dialog to pass a stable id for its title element + (and description where the dialog has a body lead paragraph). +- Add `aria-label` to every icon-only close button across dialogs. +- Add a visible `:focus-visible` outline rule on interactive elements + inside dialogs (`button`, `input`, `select`, `textarea`). + +## Impact + +- New capability `dialog-accessibility`. +- API change to `Dialog`: two new optional props, additive, no callers + break. +- Touches each consuming dialog component to add 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..52abb7c7 --- /dev/null +++ b/openspec/changes/improve-dialog-accessibility/specs/dialog-accessibility/spec.md @@ -0,0 +1,91 @@ +# 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. + +#### Scenario: Base Dialog sets the modal role + +- **WHEN** any dialog renders via the shared `Dialog` wrapper +- **THEN** the panel element has `role="dialog"` and `aria-modal="true"` + +#### Scenario: Both attributes are present together + +- **WHEN** the panel renders +- **THEN** neither `role="dialog"` nor `aria-modal="true"` may be omitted + on the panel element + +### 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. + +#### 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 a stable id +- **AND** the dialog passes that id to `Dialog` as `labelledBy` +- **AND** the referenced element exists in the rendered DOM + +#### Scenario: DiffViewerDialog provides a title even if visually minimal + +- **WHEN** `DiffViewerDialog` renders +- **THEN** it includes either a visible heading or a visually hidden + heading whose id is passed as `labelledBy` + +### 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. + +#### Scenario: Buttons inside dialogs show a focus ring + +- **WHEN** a `button`, `input`, `select`, or `textarea` inside a dialog + panel receives focus via Tab or Shift-Tab +- **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 diff --git a/openspec/changes/improve-dialog-accessibility/tasks.md b/openspec/changes/improve-dialog-accessibility/tasks.md new file mode 100644 index 00000000..76846d9d --- /dev/null +++ b/openspec/changes/improve-dialog-accessibility/tasks.md @@ -0,0 +1,26 @@ +# 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. +- [ ] Update `SettingsDialog.tsx`: give the title an id and pass it as + `labelledBy`; add `aria-label` to the icon-only close button. +- [ ] Update `NewTaskDialog.tsx`: same — title id + `labelledBy`; add + `aria-label` to the close button. +- [ ] Update `HelpDialog.tsx`: same. +- [ ] Update `ConfirmDialog.tsx`: same; the dialog already has an h2 title + (lines 40–48) — wire it to `labelledBy`. +- [ ] Update `MergeDialog.tsx`: passes through `ConfirmDialog`, so the + title id flows up via `ConfirmDialog`'s prop pass-through; verify + the link is preserved. +- [ ] Update `DiffViewerDialog.tsx`: add a visually present or visually + hidden title element so `labelledBy` has something to point at. +- [ ] Add a global `:focus-visible` outline rule scoped to interactive + elements inside dialogs (likely a class added to the dialog panel + and a CSS rule in `src/styles.css` or a colocated style block). +- [ ] Tests: extend `src/components/__tests__/` to assert each dialog + panel renders with `role="dialog"`, `aria-modal="true"`, and a + resolvable `aria-labelledby` reference. +- [ ] Validate with `npm run typecheck`, `npm test`, and + `openspec validate --all --strict`. From 0bbfe63ba0a8da00afd99c83a1744cc6409d5338 Mon Sep 17 00:00:00 2001 From: Claude <noreply@anthropic.com> Date: Sun, 26 Apr 2026 22:58:11 +0000 Subject: [PATCH 4/5] docs(openspec): incorporate review feedback on logging and a11y MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Multi-agent review surfaced spec-level bugs and missing scenarios in both proposals. Spec changes: add-structured-logging: - Make error()'s err argument required, not optional, so a context object cannot land in the error slot and stringify as [object Object]. - Add scenarios for non-Error throwables (string, plain object, undefined) and unserialisable ctx (circular references, Solid signals, DOM nodes) so the logger never throws. - Add scenario coercing non-boolean persisted verboseLogging to false so corrupted state cannot silently enable verbose in production. - Add new requirement covering the dev-mode ipc/git/pty debug traces that were in tasks.md but not in spec. - Drop the "Reveal log location" link mention that wasn't in spec. - Document rate limiting, verbose-sync reconciliation, lifecycle fallbacks, sweep batching, no-silent-swallow enforcement, token leakage, and category sprawl as known implementation risks. - Split the catch-block sweep into per-directory phases. improve-dialog-accessibility: - Fix the false claim that MergeDialog inherits labelledBy via ConfirmDialog's prop pass-through (it doesn't — ConfirmDialog only takes title:string today). Add explicit task to extend ConfirmDialog's API and generate a unique title id. - Drop the redundant "Both attributes are present together" scenario and its non-normative "may" wording. - Require document-unique title ids (createUniqueId) so nested dialogs cannot collide on aria-labelledby. - Require labelledBy targets to carry non-empty accessible text and not be hidden via aria-hidden / display:none / visibility:hidden. - Add stack-aware aria-modal scenario so only the topmost panel claims modal status when dialogs are nested. - Add focus-trap scenario paired with aria-modal. - Pin the clip / sr-only pattern for DiffViewerDialog's hidden title. - Express :focus-visible scope via a class hook on the panel so the rule survives Solid <Portal> mounting outside the app root. - Remove tasks for non-existent close buttons (NewTaskDialog, MergeDialog) and fix the test path to colocated *.test.tsx. - Add design.md capturing ConfirmDialog API extension, stack-aware aria-modal options, Portal-aware CSS, describedBy guidance, and jsdom test limits. https://claude.ai/code/session_016gGqJbqCixVHiJK7tU5YGs --- .../changes/add-structured-logging/design.md | 49 ++++++++- .../specs/logging/spec.md | 79 +++++++++++++- .../changes/add-structured-logging/tasks.md | 45 +++++--- .../improve-dialog-accessibility/design.md | 100 ++++++++++++++++++ .../improve-dialog-accessibility/proposal.md | 26 ++++- .../specs/dialog-accessibility/spec.md | 57 +++++++--- .../improve-dialog-accessibility/tasks.md | 60 +++++++---- 7 files changed, 358 insertions(+), 58 deletions(-) create mode 100644 openspec/changes/improve-dialog-accessibility/design.md diff --git a/openspec/changes/add-structured-logging/design.md b/openspec/changes/add-structured-logging/design.md index e95dc577..32025f65 100644 --- a/openspec/changes/add-structured-logging/design.md +++ b/openspec/changes/add-structured-logging/design.md @@ -75,9 +75,52 @@ file or feature; this is enforced by review, not by lint. Existing ## Settings UI A "Verbose logging" toggle in `SettingsDialog`'s diagnostics section, -with a one-line explainer and a "Reveal log location" link in dev (no-op -in prod for now). Toggle persists via the existing autosave path; it -does not require a restart — the logger reads the setting reactively. +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 limiting `LogFromRenderer`.** Fire-and-forget IPC with no cap + could flood main if a render-loop bug emits a `warn` per frame. The + implementation should coalesce or rate-limit forwarded entries (e.g. + drop after N entries per second per category and emit a single + "suppressed N entries" notice). The renderer's own `console` output + is not rate-limited. +- **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 batching.** The catch-block sweep touches dozens of files. It + is split into per-directory phases in `tasks.md` (`src/store/`, + `src/components/`, `electron/ipc/`) so each phase is reviewable on + its own. Production code paths between phases retain their existing + silent-swallow behaviour until the final sweep lands; no scenario + promises full compliance until then. +- **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. +- **Token / secret leakage.** Verbose mode exposes IPC payloads, git + command arguments, and pty events. None of these are redacted. Users + who turn verbose on for bug reports may inadvertently include paths, + remote URLs, or env-derived tokens in shared logs. A future + redaction layer 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 diff --git a/openspec/changes/add-structured-logging/specs/logging/spec.md b/openspec/changes/add-structured-logging/specs/logging/spec.md index 86894da5..7f2679a7 100644 --- a/openspec/changes/add-structured-logging/specs/logging/spec.md +++ b/openspec/changes/add-structured-logging/specs/logging/spec.md @@ -5,9 +5,11 @@ ### 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` — that -accept a category tag, a message, an optional structured context object, -and (for `error`) the underlying error or thrown value. +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 @@ -16,6 +18,14 @@ and (for `error`) the underlying error or thrown value. - **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'` @@ -28,6 +38,24 @@ and (for `error`) the underlying error or thrown value. - **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 the stack is included + +#### 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 + of the offending fields rather than throwing +- **AND** the rest of the entry (level, category, message) still appears + ### Requirement: Level gating by build and verbose flag The logger SHALL gate output by level according to the current build mode @@ -123,6 +151,15 @@ across launches, and default it to `false` for new installs. - **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` @@ -134,3 +171,39 @@ across launches, and default it to `false` for new installs. - **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 (where safe) a summary of the payload +- **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 index 0b893c0a..baed5632 100644 --- a/openspec/changes/add-structured-logging/tasks.md +++ b/openspec/changes/add-structured-logging/tasks.md @@ -5,29 +5,42 @@ - [ ] 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. + 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`. + 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`. + `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; push the new - level to main on each change. + 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. -- [ ] Sweep every catch in `src/store/` and `src/components/` and route - through the renderer logger; replace silent swallows with `warn` - or `error` as appropriate. -- [ ] Sweep every catch in `electron/ipc/` and route through the main - logger; replace silent swallows similarly. -- [ ] Add dev-only debug traces at category `ipc` (every IPC handler - entry/exit), `git` (every git command + exit code), `pty` (spawn, - exit, signal). These should be `debug` level so they only show - with verbose on or in dev. + 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: `src/lib/__tests__/log.test.ts` and `electron/__tests__/log.test.ts` covering level gating, category - formatting, forwarding behavior, and the `verbose` runtime flip. + formatting, forwarding behavior, the `verbose` runtime flip, + circular-ctx safety, non-Error normalisation, and corrupted + `verboseLogging` coercion. - [ ] 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..9fcdb857 --- /dev/null +++ b/openspec/changes/improve-dialog-accessibility/design.md @@ -0,0 +1,100 @@ +# 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. +- Generate a fallback title id via Solid's `createUniqueId()` when the + consumer does not supply one. Apply that id to the rendered `<h2>`. +- Forward whichever id is in effect (consumer-supplied or generated) to + `Dialog` as `labelledBy`, and forward `describedBy` if the consumer + passes one. + +`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):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. + +## `describedBy` usage guidance + +`describedBy` should be supplied when the dialog has a meaningful body +lead — typically a one-paragraph explanation under the title: + +- `ConfirmDialog` — yes, when `message` is non-empty. +- `MergeDialog` — yes, the merge-destination paragraph is a natural + description. +- `SettingsDialog`, `HelpDialog` — no, they have sectioned content with + multiple sub-headings; pointing `describedBy` at one section misleads + the user. +- `NewTaskDialog`, `DiffViewerDialog` — no for the same reason. + +This guidance lives here, not in the spec, because it is a styling / +content-pattern call rather than a normative requirement. + +## 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 index 6a103d84..7a8d7252 100644 --- a/openspec/changes/improve-dialog-accessibility/proposal.md +++ b/openspec/changes/improve-dialog-accessibility/proposal.md @@ -19,17 +19,33 @@ low-cost. - 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. -- Update each consuming dialog to pass a stable id for its title element - (and description where the dialog has a body lead paragraph). -- Add `aria-label` to every icon-only close button across dialogs. + 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`). + 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. -- Touches each consuming dialog component to add a title id and pass +- 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 index 52abb7c7..2fb009b3 100644 --- a/openspec/changes/improve-dialog-accessibility/specs/dialog-accessibility/spec.md +++ b/openspec/changes/improve-dialog-accessibility/specs/dialog-accessibility/spec.md @@ -6,24 +6,36 @@ 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. +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 `role="dialog"` and `aria-modal="true"` +- **THEN** the panel element has both `role="dialog"` and + `aria-modal="true"` -#### Scenario: Both attributes are present together +#### Scenario: aria-modal is paired with a working focus trap -- **WHEN** the panel renders -- **THEN** neither `role="dialog"` nor `aria-modal="true"` may be omitted - on the panel element +- **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 ### 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. +opens, and the linked title element SHALL carry non-empty accessible +text. #### Scenario: Dialog accepts a labelledBy prop @@ -34,15 +46,27 @@ opens. - **WHEN** any of `SettingsDialog`, `NewTaskDialog`, `HelpDialog`, `ConfirmDialog`, `MergeDialog`, or `DiffViewerDialog` renders -- **THEN** its title element has a stable id +- **THEN** its title element has an id that is unique within the + document for the lifetime of that render (e.g. produced by + `createUniqueId` so two open dialogs cannot collide) - **AND** the dialog passes that id to `Dialog` as `labelledBy` - **AND** the referenced element exists in the rendered DOM -#### Scenario: DiffViewerDialog provides a title even if visually minimal +#### Scenario: Title element has accessible text + +- **WHEN** an element referenced by a dialog's `aria-labelledby` is in + the DOM +- **THEN** it contains non-empty text content +- **AND** it is not itself hidden via `aria-hidden="true"`, + `display: none`, or `visibility: hidden` + +#### Scenario: DiffViewerDialog provides a visually hidden title - **WHEN** `DiffViewerDialog` renders -- **THEN** it includes either a visible heading or a visually hidden - heading whose id is passed as `labelledBy` +- **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 @@ -75,7 +99,8 @@ accessible name via `aria-label` so screen-reader users can identify it. Interactive elements inside dialog panels SHALL show a visible focus indicator when focused via keyboard, distinct from the hover or active -state. +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: Buttons inside dialogs show a focus ring @@ -89,3 +114,11 @@ state. - **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 index 76846d9d..f78536c4 100644 --- a/openspec/changes/improve-dialog-accessibility/tasks.md +++ b/openspec/changes/improve-dialog-accessibility/tasks.md @@ -3,24 +3,46 @@ - [ ] 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. -- [ ] Update `SettingsDialog.tsx`: give the title an id and pass it as - `labelledBy`; add `aria-label` to the icon-only close button. -- [ ] Update `NewTaskDialog.tsx`: same — title id + `labelledBy`; add - `aria-label` to the close button. -- [ ] Update `HelpDialog.tsx`: same. -- [ ] Update `ConfirmDialog.tsx`: same; the dialog already has an h2 title - (lines 40–48) — wire it to `labelledBy`. -- [ ] Update `MergeDialog.tsx`: passes through `ConfirmDialog`, so the - title id flows up via `ConfirmDialog`'s prop pass-through; verify - the link is preserved. -- [ ] Update `DiffViewerDialog.tsx`: add a visually present or visually - hidden title element so `labelledBy` has something to point at. -- [ ] Add a global `:focus-visible` outline rule scoped to interactive - elements inside dialogs (likely a class added to the dialog panel - and a CSS rule in `src/styles.css` or a colocated style block). -- [ ] Tests: extend `src/components/__tests__/` to assert each dialog - panel renders with `role="dialog"`, `aria-modal="true"`, and a - resolvable `aria-labelledby` reference. + `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 (visible or + visually hidden via the `clip` / sr-only pattern, **not** + `display: none`) and wire its id as `labelledBy`. +- [ ] Add a `:focus-visible` outline rule in `src/styles.css` keyed on + a class on the dialog panel (e.g. + `.dialog-panel button:focus-visible`) so the rule applies to + portaled panels. +- [ ] 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 containing non-empty + text, and stack-aware `aria-modal` removes the attribute from the + underlying panel when a second dialog opens. 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`. From 701b0b46a1bab62f74e8633723e3ac4d0e04338e Mon Sep 17 00:00:00 2001 From: Claude <noreply@anthropic.com> Date: Sun, 26 Apr 2026 23:14:12 +0000 Subject: [PATCH 5/5] docs(openspec): apply final-pass review fixes across all proposals Third review pass surfaced spec gaps and one cross-proposal issue. Fixes applied: Cross-proposal: - Tour and logging both used a __tests__/ subdirectory in their test paths; repo convention is colocated *.test.ts(x). Both fixed. Tour (specs/onboarding-tour/spec.md): - tourStep is now explicitly a single global 0-indexed counter; phase 2 resumes at the global index after the last phase-1 step, not phase-relative zero. - New scenario covering mid-phase-2 quit (resumes at the same global step on the next launch). - New scenario covering Restart-tour while the tour is already active (current step's afterLeave runs, tourStep resets to 0, re-activates after SettingsDialog closes). - New scenario covering existing-user restart whose anchors are already in scope (phase 2 chains immediately). - Empty-string anchorId is treated the same as null (centered, no lookup) so a coding error doesn't waste the MutationObserver wait. - Promote global-shortcut suppression from "implementation risk" in design.md to a real spec requirement, since it is observable user behaviour. - Design.md: note hook-failure logging routes through the structured logger, and note that the implementation may consume the a11y dialog stack instead of enumerating named modal flags. Logging (specs/logging/spec.md): - error()'s third argument: explicit scenario for calling with `undefined` when there is no real error value (level + category emit, stack section omitted). - Hand-wavy "where safe" replaced by a concrete SENSITIVE_CHANNELS set covering pty input, auth/token round-trips, and home-relative paths. - Per-second per-category rate cap (50 entries/window) is now a spec requirement, with a single suppression-notice forward at window end. Aligns spec with what design and tasks already promised. - Forwarding rules clarified to apply equally in dev: dev only changes which entries pass the level gate, not which forward. - "No silent error swallowing" requirement now explicitly admits per-directory phased compliance during the sweep transition. - Robust handling pinned for non-string `stack` properties and for the safe-fallback path itself failing (outer-try guarantees ctx is omitted entirely rather than throwing). - Proposal.md: introduces the preload allowlist alongside the IPC channel mention so a reader unfamiliar with the codebase has context. - Design.md: rate-cap framing rewritten to match the spec requirement; sweep batching framed as "phasing" matching the new spec scenario. A11y (specs/dialog-accessibility/spec.md): - Stack-aware aria-modal: new scenario for the underlying panel regaining aria-modal="true" when the topmost dialog closes. - Consumer-supplied labelledBy to ConfirmDialog wins over the internally-generated id; design.md describes the contract. - Title-id stability tightened to "for the lifetime of the dialog's open mount" to forestall confusion about createUniqueId across re-renders. - Accessible-text rule now says trimmed textContent, and explicitly notes the spec does not police font-size:0 / color:transparent tricks (jsdom can't catch them). - Focus-indicator selector broadened to cover icon buttons, links, toggles, and ARIA widgets ([role="button"], [role="switch"], [role="checkbox"], [role="link"], a[href], [tabindex]). - Design.md: pinned `.dialog-sr-only` clip-based utility recipe alongside the focus-visible CSS so DiffViewerDialog's hidden heading does not have to invent a pattern. - Design.md: describedBy guidance section explicitly marked advisory so future readers don't treat its yes/no list as binding. - Tasks.md: add the .dialog-sr-only utility, broaden the focus-visible selector, add tests for stack-aware restoration on close and consumer-supplied labelledBy precedence. https://claude.ai/code/session_016gGqJbqCixVHiJK7tU5YGs --- .../changes/add-onboarding-tour/design.md | 24 +++++- .../specs/onboarding-tour/spec.md | 67 +++++++++++++-- openspec/changes/add-onboarding-tour/tasks.md | 19 ++++- .../changes/add-structured-logging/design.md | 43 ++++++---- .../add-structured-logging/proposal.md | 12 ++- .../specs/logging/spec.md | 56 +++++++++++- .../changes/add-structured-logging/tasks.md | 14 +-- .../improve-dialog-accessibility/design.md | 85 ++++++++++++++----- .../specs/dialog-accessibility/spec.md | 36 ++++++-- .../improve-dialog-accessibility/tasks.md | 36 +++++--- 10 files changed, 311 insertions(+), 81 deletions(-) diff --git a/openspec/changes/add-onboarding-tour/design.md b/openspec/changes/add-onboarding-tour/design.md index 0f5b815b..acab70b6 100644 --- a/openspec/changes/add-onboarding-tour/design.md +++ b/openspec/changes/add-onboarding-tour/design.md @@ -123,15 +123,31 @@ the implementer. 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. -- **Global shortcut suppression.** While `tourActive` is true, suppress - global keybindings (new task, focus mode, help, settings, etc.) so a key - press does not open another modal on top of the tour. Only Esc (skip) - and Enter (next) should be live. +- **`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 diff --git a/openspec/changes/add-onboarding-tour/specs/onboarding-tour/spec.md b/openspec/changes/add-onboarding-tour/specs/onboarding-tour/spec.md index bd8f02e8..a8aa1be1 100644 --- a/openspec/changes/add-onboarding-tour/specs/onboarding-tour/spec.md +++ b/openspec/changes/add-onboarding-tour/specs/onboarding-tour/spec.md @@ -120,14 +120,18 @@ window resizes or layout changes. #### Scenario: Centered step has no spotlight -- **WHEN** a step's `anchorId` is `null` +- **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. +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 @@ -137,13 +141,14 @@ still see all steps without the app fabricating demo data. - **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 (steps that explain the terminal, - changed-files panel, merge action, and help) +- **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 @@ -161,6 +166,24 @@ still see all steps without the app fabricating demo data. - **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 @@ -170,14 +193,46 @@ still see all steps without the app fabricating demo data. ### Requirement: Restart from settings -The app SHALL let the user replay the tour from the settings dialog. +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 +- **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 diff --git a/openspec/changes/add-onboarding-tour/tasks.md b/openspec/changes/add-onboarding-tour/tasks.md index d1b23351..6c4cf1a2 100644 --- a/openspec/changes/add-onboarding-tour/tasks.md +++ b/openspec/changes/add-onboarding-tour/tasks.md @@ -37,9 +37,20 @@ - [ ] 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). -- [ ] Tests: `src/store/__tests__/tour.test.ts` covering start, navigate - (forward + back, including hook ordering), skip, finish, restart, - existing-user carve-out, deferred-activation-while-modal-open, and - the resume-after-task-spawn path. +- [ ] 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 index 32025f65..e6fe356f 100644 --- a/openspec/changes/add-structured-logging/design.md +++ b/openspec/changes/add-structured-logging/design.md @@ -85,12 +85,12 @@ 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 limiting `LogFromRenderer`.** Fire-and-forget IPC with no cap - could flood main if a render-loop bug emits a `warn` per frame. The - implementation should coalesce or rate-limit forwarded entries (e.g. - drop after N entries per second per category and emit a single - "suppressed N entries" notice). The renderer's own `console` output - is not rate-limited. +- **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 @@ -101,22 +101,29 @@ surprise the implementer. 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 batching.** The catch-block sweep touches dozens of files. It - is split into per-directory phases in `tasks.md` (`src/store/`, - `src/components/`, `electron/ipc/`) so each phase is reviewable on - its own. Production code paths between phases retain their existing - silent-swallow behaviour until the final sweep lands; no scenario - promises full compliance until then. +- **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. -- **Token / secret leakage.** Verbose mode exposes IPC payloads, git - command arguments, and pty events. None of these are redacted. Users - who turn verbose on for bug reports may inadvertently include paths, - remote URLs, or env-derived tokens in shared logs. A future - redaction layer is out of scope for this proposal but should be - flagged in the verbose toggle's explainer copy. +- **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 diff --git a/openspec/changes/add-structured-logging/proposal.md b/openspec/changes/add-structured-logging/proposal.md index 97d47ab3..8df548da 100644 --- a/openspec/changes/add-structured-logging/proposal.md +++ b/openspec/changes/add-structured-logging/proposal.md @@ -34,7 +34,13 @@ cycles substantially. - New capability `logging`. - New modules `src/lib/log.ts` and `electron/log.ts`. - New IPC channel `LogFromRenderer` (renderer → main, fire-and-forget). -- Sweep touches dozens of files; no behavioral change beyond replacing - `console.*` and silent swallows with logger calls. -- New persisted field `verboseLogging: boolean` (default `false`). + 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 index 7f2679a7..6e5bccfb 100644 --- a/openspec/changes/add-structured-logging/specs/logging/spec.md +++ b/openspec/changes/add-structured-logging/specs/logging/spec.md @@ -45,7 +45,17 @@ confused with the optional context object. 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 the stack is included +- **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 @@ -53,8 +63,11 @@ confused with the optional context object. 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 - of the offending fields rather than throwing + 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 @@ -89,7 +102,19 @@ and the user's `verboseLogging` setting. The codebase SHALL route every caught error through the logger; silent swallows (e.g. `.catch(() => {})`) are not allowed in production code -paths. +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 @@ -141,6 +166,26 @@ holds a single timeline of the session. - **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 @@ -184,7 +229,10 @@ ad-hoc `console.log` calls. - **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 (where safe) a summary of the payload + 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) diff --git a/openspec/changes/add-structured-logging/tasks.md b/openspec/changes/add-structured-logging/tasks.md index baed5632..699bdede 100644 --- a/openspec/changes/add-structured-logging/tasks.md +++ b/openspec/changes/add-structured-logging/tasks.md @@ -37,10 +37,14 @@ `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: `src/lib/__tests__/log.test.ts` and - `electron/__tests__/log.test.ts` covering level gating, category - formatting, forwarding behavior, the `verbose` runtime flip, - circular-ctx safety, non-Error normalisation, and corrupted - `verboseLogging` coercion. +- [ ] 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 index 9fcdb857..224ad6ac 100644 --- a/openspec/changes/improve-dialog-accessibility/design.md +++ b/openspec/changes/improve-dialog-accessibility/design.md @@ -17,11 +17,17 @@ untagged `<h2>`. `MergeDialog` (and any future wrapper) cannot point The extension: - Add optional `labelledBy?: string` and `describedBy?: string` props. -- Generate a fallback title id via Solid's `createUniqueId()` when the - consumer does not supply one. Apply that id to the rendered `<h2>`. -- Forward whichever id is in effect (consumer-supplied or generated) to - `Dialog` as `labelledBy`, and forward `describedBy` if the consumer - passes one. +- 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 @@ -55,30 +61,71 @@ written as a descendant of the app shell (e.g. The fix is a class hook on the panel itself: ```css -.dialog-panel :is(button, input, select, textarea):focus-visible { +.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. +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. -## `describedBy` usage guidance +## DiffViewerDialog visually hidden title -`describedBy` should be supplied when the dialog has a meaningful body -lead — typically a one-paragraph explanation under the 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: -- `ConfirmDialog` — yes, when `message` is non-empty. -- `MergeDialog` — yes, the merge-destination paragraph is a natural - description. -- `SettingsDialog`, `HelpDialog` — no, they have sectioned content with - multiple sub-headings; pointing `describedBy` at one section misleads - the user. -- `NewTaskDialog`, `DiffViewerDialog` — no for the same reason. +```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 -This guidance lives here, not in the spec, because it is a styling / -content-pattern call rather than a normative requirement. +`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 diff --git a/openspec/changes/improve-dialog-accessibility/specs/dialog-accessibility/spec.md b/openspec/changes/improve-dialog-accessibility/specs/dialog-accessibility/spec.md index 2fb009b3..1c90b2ba 100644 --- a/openspec/changes/improve-dialog-accessibility/specs/dialog-accessibility/spec.md +++ b/openspec/changes/improve-dialog-accessibility/specs/dialog-accessibility/spec.md @@ -30,6 +30,14 @@ makes that promise true SHALL remain wired up. - **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 @@ -47,18 +55,31 @@ text. - **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 that render (e.g. produced by - `createUniqueId` so two open dialogs cannot collide) + 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** it contains non-empty text content +- **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 @@ -102,10 +123,13 @@ 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: Buttons inside dialogs show a focus ring +#### Scenario: Interactive elements inside dialogs show a focus ring -- **WHEN** a `button`, `input`, `select`, or `textarea` inside a dialog - panel receives focus via Tab or Shift-Tab +- **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 diff --git a/openspec/changes/improve-dialog-accessibility/tasks.md b/openspec/changes/improve-dialog-accessibility/tasks.md index f78536c4..e6e54399 100644 --- a/openspec/changes/improve-dialog-accessibility/tasks.md +++ b/openspec/changes/improve-dialog-accessibility/tasks.md @@ -27,22 +27,34 @@ 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 (visible or - visually hidden via the `clip` / sr-only pattern, **not** - `display: none`) and wire its id as `labelledBy`. +- [ ] 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 - a class on the dialog panel (e.g. - `.dialog-panel button:focus-visible`) so the rule applies to - portaled panels. + `.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 containing non-empty - text, and stack-aware `aria-modal` removes the attribute from the - underlying panel when a second dialog opens. Note that jsdom does - not run AT name computation, so these tests verify structure, not - announcement. + 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`.