docs(openspec): propose first-launch onboarding tour#88
Closed
johannesjo wants to merge 5 commits intomainfrom
Closed
docs(openspec): propose first-launch onboarding tour#88johannesjo wants to merge 5 commits intomainfrom
johannesjo wants to merge 5 commits intomainfrom
Conversation
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
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
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
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
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
johannesjo
added a commit
that referenced
this pull request
Apr 27, 2026
Local snapshot of #88 (three OpenSpec change proposals: add-onboarding-tour, add-structured-logging, improve-dialog-accessibility) with the following review fixes applied on top: - Three docs reflowed so prettier --check passes (table padding, stray whitespace, error() inline-backtick wrapping). - Moved SHALL to the first line of the dev-mode-traces requirement so openspec validate --strict picks it up. - Tour spec: existing-user-with-prior-data carve-out now requires tourStep is null, so it does not stomp a tour mid-flight from a prior session. - Tour spec: mid-phase-1 quit explicitly detects the in-progress tour via persisted non-null tourStep and restarts phase 1 even when a project was created during the abandoned run. - Tour spec: activation-defers-when-modal-open now requires tourActive is false, so the tour cannot defer itself when step 4's beforeEnter opens NewTaskDialog.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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