fix(onboarding): tighten first-repo gate edge cases and address review

- Strip `_legacySoftSkipMigrationDone` from `onboarding:get` /
  `onboarding:update` IPC responses so the main-only persistence
  discriminator never crosses to the renderer.
- Fall back to in-memory legacy-eligibility suppression in App.tsx if
  the seal IPC throws, so SSH reconnect / terminal restore continue;
  the seal is retried on next launch (idempotent).
- Latch `onboarding_step4_path_revealed` only on the `'main'` substep
  so a future auto-route into the SSH form doesn't double-fire it,
  guard the `beforeunload` abandonment emitter on `closedAt === null`
  to avoid spurious events between `closeWith` and React unmount, and
  register it with `capture: true` so it runs before App's flush.
- Keep `RemoteRepoStep`'s Add button disabled across the silent
  retry-as-folder path (`busyLabel` covers what `isAddingRemote`
  doesn't) so a second click can't dispatch a duplicate add.
- Re-entry guard `retryRemoteAsFolder` on `busyLabel` and clear the
  stale `activeWorktreeId` when `activateRepoForUser` falls into the
  empty-worktree-list branch, so users land on the new repo's home
  view rather than a previous repo's terminal.
- Make `completedPath` required (not optional) in `useCloseWith` so
  the funnel `onboarding_completed` event always fires on completion.
- Narrow `onboardingStep4PathRevealedSchema` to `path: z.literal('ssh')`
  to match the only emitter; widening is a one-line follow-up if we
  ever reveal `open_folder` / `clone_url`.
- Add an explicit `aria-label` to the SSH CTA button so role+name
  locators (and assistive tech) get a clean computed name; update the
  e2e gate test to use the exact name and poll `closedAt` after Esc.
- Rewrite the design doc to align with the as-shipped surface.

Co-authored-by: Orca <help@stably.ai>
This commit is contained in:
brennanb2025
2026-05-11 10:25:50 -07:00
co-authored by Orca
parent f6f460d743
commit 69f2fdc61a
11 changed files with 187 additions and 158 deletions
+71 -113
View File
@@ -1,176 +1,134 @@
# Onboarding "First Repo Required" — Pre-Merge Review
# Onboarding "First Repo Required" — Design Rationale
**Status: PR #1677 is OPEN, not yet merged.** This doc is a pre-merge review of the changes on branch `brennanb2025/onboarding-first-repo-required` (commits `eac61212` "feat(onboarding): require first project before closing wizard" and `d2448f82` "fix(onboarding): suppress composer for folder repos and prune dead dismissed path"). It is NOT a follow-up tracker for an already-shipped change.
This doc captures the design decisions behind the onboarding repo gate (step 4 of the wizard) as it ships on `brennanb2025/onboarding-first-repo-required`. It is post-implementation: it explains *why* the shipped surface looks the way it does, what alternatives were rejected, and which loose ends are deliberately deferred.
**What the PR does:** makes the repo step (step 4 of the onboarding wizard) a hard gate. Specifically, it removes the "I'll add one later" affordance from the repo step and removes every renderer-side `closeWith('dismissed', ...)` writer on that step, so onboarding only closes once the user adds a project. `useCloseWith` is also narrowed to `outcome: 'completed'` and the unreachable `onboarding_dismissed` track call is dropped.
**What the gate does.** Step 4 (the repo step) is hard-required: the wizard only closes after the user adds a project (folder, clone, or SSH-remote repo). The "I'll add one later" affordance is gone, every renderer-side `closeWith('dismissed', ...)` writer was removed, and `useCloseWith` is narrowed to `outcome: 'completed'`.
**What this doc is:** a punch list of concrete gaps uncovered while reviewing the proposed gate against Orca's actual user paths — most importantly, the SSH-only user — and against the existing repo-step controller logic in `src/renderer/src/components/onboarding/use-onboarding-flow.ts`. The macro decision to keep the gate (vs. restoring a soft-skip) is settled per the PR author and is out of scope. Each item below is either a P0 that should block the merge, a P1 that should land before merge or as an immediate follow-up, or a P2 that is genuinely optional polish.
**For a reviewer reading this cold:** the PR's diff is small (`use-onboarding-flow-persistence.ts`, `use-onboarding-flow.ts`, `tests/e2e/onboarding.spec.ts` plus `OnboardingFlow.tsx` from the earlier commit), but the affected user surface is large because the gate has no escape. Most items below describe gaps in *recovery paths* that the soft-skip used to provide. The fixes are mostly additive — no item below requires undoing the gate itself.
**What this doc is not.** It does not relitigate "should we gate at all" — that decision is settled. It does not track every bug found during review; bugs are filed separately. It is the design rationale a future contributor needs to understand the gate without re-deriving the constraints.
## Reviewer's quick map
**Branch state.** `brennanb2025/onboarding-first-repo-required`, two commits ahead of `main`:
- `eac61212` `feat(onboarding): require first project before closing wizard`
- `d2448f82` `fix(onboarding): suppress composer for folder repos and prune dead dismissed path`
**The four onboarding steps** (`use-onboarding-flow.ts`):
1. `agent` (step index 0)
2. `theme` (step index 1)
3. `notifications` (step index 2)
4. `repo` (step index 3) ← this is the gate
4. `repo` (step index 3) — the gate
So `lastCompletedStep === 3` means "user finished notifications and is sitting on the repo gate." This matters for #3.
`lastCompletedStep === 3` therefore means "user finished notifications and is sitting on the repo gate." This matters for the legacy migration (item 3).
**Key files referenced repeatedly below** (paths relative to repo root):
- `src/renderer/src/components/onboarding/RepoStep.tsx` — the step UI (Open folder / Clone repo CTAs).
- `src/renderer/src/components/onboarding/OnboardingFlow.tsx` — wizard shell, owns the `z-[100]` overlay.
- `src/renderer/src/components/onboarding/use-onboarding-flow.ts` — the flow controller (`completeRepo`, `clone`, `skip`).
- `src/renderer/src/components/onboarding/use-onboarding-flow-persistence.ts` — `useCloseWith` (now `'completed'`-only) and `persistStep`.
- `src/renderer/src/components/onboarding/should-show-onboarding.ts` — gate predicate.
- `src/renderer/src/components/sidebar/AddRepoDialog.tsx` and `AddRepoSteps.tsx` — the existing SSH-aware add-repo dialog. Source of `useRemoteRepo` (the hook #1 wants to extract).
- `src/main/ipc/worktrees.ts` and `src/main/repo-worktrees.ts` — folder-worktree synthesis. Relevant to #2.
- `src/main/ipc/repos.ts` — `repos:add` / `repos:addRemote` IPC.
- `src/main/persistence.ts` — `OnboardingState` parse boundary. Relevant to #3 and #5.
- `src/shared/types.ts:1372` — `OnboardingOutcome` type. Relevant to #5.
- `src/shared/telemetry-events.ts` — onboarding event registry. Relevant to #4 and #5.
- `src/preload/index.ts:167-244` — native file-drop preload bridge. Relevant to #9.
**Files that carry the design weight:**
**Competitor patterns referenced** (for context on the SSH-as-peer and empty-state-as-gate ideas):
- emdash, t3code: SSH/remote as a peer of local in their first-run.
- Superset: empty-state home view as gate, drag-drop folder onto the empty surface.
- `src/renderer/src/components/onboarding/RepoStep.tsx` — the step UI, three CTAs (Open folder / Clone repo / Connect a remote).
- `src/renderer/src/components/onboarding/RemoteRepoStep.tsx` — in-wizard SSH sub-step, mounted in place of `RepoStep` when the user clicks "Connect a remote (SSH)".
- `src/renderer/src/components/onboarding/OnboardingFlow.tsx` — wizard shell. Owns the `z-[100]` overlay, the `repoSubstep` `'main' | 'remote'` toggle, the `beforeunload` abandonment emitter, and the SSH-CTA reveal latch.
- `src/renderer/src/components/onboarding/use-onboarding-flow.ts` — the flow controller. Hosts `activateRepoForUser`, `completeRepo`, `completeRepoFromRemote`, `retryRemoteAsFolder`.
- `src/renderer/src/components/onboarding/use-onboarding-flow-persistence.ts` — `useCloseWith`, narrowed to `'completed'`-only.
- `src/renderer/src/components/onboarding/should-show-onboarding.ts` — gate predicate; suppresses the wizard for `legacySoftSkipEligible` rows.
- `src/renderer/src/components/sidebar/AddRepoSteps.tsx` — source of `useRemoteRepo` and `RemoteStepBody`, both reused (not duplicated) by the wizard's SSH sub-step.
- `src/main/persistence.ts` — `OnboardingState` parse boundary. Holds `sanitizeOnboardingUpdate` (with the `allowInternal` gate) and the legacy soft-skip migration in `Store.load`.
- `src/main/ipc/onboarding.ts` — strips `_legacySoftSkipMigrationDone` from `onboarding:get` / `onboarding:update` responses so the renderer cannot read it. `legacySoftSkipEligible` is intentionally left in the response because `shouldShowOnboarding` reads it; renderer writes of either field are blocked by the sanitizer's `allowInternal` gate.
- `src/shared/types.ts` — `OnboardingOutcome` (narrowed to `'completed'`), `OnboardingState` (with `legacySoftSkipEligible` and `_legacySoftSkipMigrationDone`).
- `src/shared/constants.ts` — `getDefaultOnboardingState`, which stamps `_legacySoftSkipMigrationDone: true` on new rows.
- `src/shared/telemetry-events.ts` — onboarding event registry (`onboardingPathSchema` includes `'ssh'`; new `onboardingStep4PathRevealedSchema` and `onboardingStep4AbandonedSchema`).
**How to read the priorities.**
- **P0** = should block this PR's merge.
- **P1** = should land before merge OR as an immediate follow-up; explicitly call out which in the PR description.
- **P2** = nice to have, no merge dependency.
**Decisions a reviewer should make / push back on:**
1. Is the SSH path (#1) in-scope for this PR or a fast follow-up? It's the only P0 in the list besides nothing.
2. For #3, which of the three migration options? Option 1 (legacy marker) is the safest, option 3 (accept the trap) is the cheapest — but the doc currently flags option 3 as risky without PostHog sizing.
3. Is shipping #4 (telemetry) before GA acceptable, or does it need to land in this PR?
**Architectural note (z-index, applies to future work in this surface).** The wizard at `z-[100]` sits above Radix `Dialog` / `Sheet` / `Command` / `Tooltip` / `HoverCard` (all `z-50`) and `Popover` (`z-[60]`). Sonner toasts at `z-index: 999999999` render above the wizard and are unaffected. Anything that wants to "open a thing inside the wizard" must either bring the wizard into the modal-stack family with explicit z-ordering and Radix portal sibling-DOM-order awareness, or extract portable internals — which is what the SSH sub-step does (item 1). The shipped SSH sub-step is mounted in place of `RepoStep` rather than as a Radix `Dialog` for exactly this reason.
---
**Architectural note (problem statement only).** All "modal during onboarding" needs hit a z-index issue. The wizard at `z-[100]` (`OnboardingFlow.tsx:76`) sits above Radix `Dialog` (`z-50`, `components/ui/dialog.tsx:34,56`), `Sheet`, `Command`, `Tooltip`, and `HoverCard` (all `z-50`), and `Popover` (`z-[60]`, `popover.tsx:72`). Sonner toasts use `z-index: 999999999` and render above the wizard — they're not z-blocked. Any future "open a thing inside the wizard" work needs to either: (a) bring the wizard into the modal-stack family with explicit z-ordering and Radix portal sibling-DOM-order awareness, or (b) extract portable internals (as #1 does for `useRemoteRepo`). Out of scope for this fix list, but flag it so the next "I want to open a thing from inside the wizard" task doesn't re-discover it.
## Items
## P0 — Block merge
### 1. SSH path: in-wizard `RemoteRepoStep`, not a deep-link
### 1. SSH-only users have no in-wizard path
**Implementation.** `RepoStep.tsx` renders three CTAs: Open a folder, Clone a repo, and "Connect a remote (SSH)". Clicking the third CTA flips `repoSubstep` to `'remote'` in `OnboardingFlow.tsx`, swapping `RepoStep` out for `RemoteRepoStep` in the same wizard shell (no overlay change, progress dots untouched). The e2e test in `tests/e2e/onboarding.spec.ts` pins the SSH CTA's presence on the gate page so a regression that drops the third path is caught alongside the no-skip assertions.
The repo step (`RepoStep.tsx`) offers two CTAs: "Open a folder" (local picker) and "Clone a repo" (clones to `settings.workspaceDir` on the local machine). An SSH-only user — one whose code lives on a remote dev box and not on the local laptop — cannot complete either action. The only acknowledgement that SSH exists today is a small footnote ("SSH? Set hosts up in Settings") at the bottom of the step.
**Why an in-wizard sub-step, not a Radix dialog.** Mounting `AddRepoDialog` from inside the wizard would render the dialog UNDER the wizard overlay (z-index trap, see the architectural note). Lifting the wizard's z-index plus rewiring Radix portal stacking is risky; extracting the portable internals is not. `RemoteRepoStep` reuses `useRemoteRepo` and `RemoteStepBody` from `AddRepoSteps.tsx` — same code path, no JSX duplication. The dialog-only chrome (`<DialogHeader>` / `<DialogTitle>` / `<DialogDescription>`) is replaced with plain `<h2>` / `<p>` since Radix's title/description primitives require a `Dialog.Root` ancestor.
Without the gate, this user could click "I'll add one later", configure their SSH target via Settings, then use the sidebar's `AddRepoDialog` to pick a repo on the remote. With the gate as currently proposed in PR #1677, that escape is gone, and the wizard has no equivalent flow of its own.
**Empty-SSH-targets case.** When the user has no configured SSH targets, `RemoteRepoStep` renders `SshTargetForm` inline (`showAddForm` branch) instead of deep-linking to Settings. A Settings deep-link would render under the wizard overlay; an inline form does not.
The desktop SSH flow Orca already supports:
**Not-a-git-repo branch.** `retryRemoteAsFolder` in `use-onboarding-flow.ts` mirrors the local "Open a folder" silent fallback — re-adds with `kind: 'folder'`, no confirmation dialog. The user already chose "Connect a remote" on a step they cannot dismiss; a Radix confirm dialog would render under the overlay anyway. The sidebar `AddRepoDialog` keeps its existing confirmation flow because that surface is outside the wizard; the asymmetry is intentional.
1. **Settings → SSH** (`SshPane.tsx`, `SshTargetForm.tsx`) — add label, host, username, identity, optional proxy/jump host, grace period; verify. (Passwords/passphrases are prompted at connection time, not configured in the form.)
2. **Sidebar → Add Repo → "SSH connected target"** (`AddRepoDialog.tsx:363-402`) — pick a connected target, then type or browse (`RemoteFileBrowser.tsx`) to a git repo path on the remote.
**Why this matters.** Without an in-wizard SSH path, the gate excludes Orca's SSH-only audience entirely (their code lives on a remote dev box, not on the laptop). The previous soft-skip used to be their escape hatch; with the gate in place, the SSH sub-step is the substitute. Matches how emdash and t3code present remote/SSH as a peer of local in their first-run flows.
Recommended fix: add a third CTA on the repo step — "Connect a remote (SSH)" — that navigates *in-place* inside the wizard's content area to a wizard-renderable `RemoteRepoStep` (with a Back arrow back to the two-CTA RepoStep). For the empty-SSH-targets case, render a nested `SshTargetForm` view inside `RemoteRepoStep` — not a Settings deep-link.
### 2. `activateRepoForUser` primitive
Why not reuse `AddRepoDialog` directly: `OnboardingFlow` is `z-[100]` (`OnboardingFlow.tsx:76`); Radix `Dialog` overlay and content are both `z-50` (`components/ui/dialog.tsx:34,56`). Mounting `AddRepoDialog` from inside the wizard renders the dialog UNDER the wizard overlay. Fixing that requires either lifting the wizard's z-index *and* updating Radix portal stacking (risky) or extracting the dialog's portable internals — which is what this fix does.
**Implementation.** `activateRepoForUser(repoId)` in `use-onboarding-flow.ts` is the single primitive `completeRepo` calls before `closeWith('completed', ...)`. It branches:
The hook is already factored out: extract `useRemoteRepo` (`AddRepoSteps.tsx:22-27`) and the `RemoteStep` JSX (`AddRepoSteps.tsx:181-299`) into a wizard-renderable `RemoteRepoStep` component. On `repos.addRemote` success, treat the resulting repo as the activation event the gate is waiting on (same code path as Open Folder / Clone today, routed through the activation primitive in #2). `RemoteStep` JSX uses `<DialogHeader>` / `<DialogTitle>` / `<DialogDescription>` (`AddRepoSteps.tsx:12, 198-224`); the wizard-renderable variant must replace these with plain `<h2>` / `<p>` (or wizard-step heading components) since Radix's title/description primitives require a `Dialog.Root` ancestor and will throw a11y warnings otherwise.
- **Worktree present** (folders via the IPC's synthesized worktree, local git, connected SSH-remote git): `activateAndRevealWorktree(worktreesByRepo[repoId][0].id)`.
- **Empty worktree list** (transient `worktrees:list` failure, SSH disconnect mid-`fetchWorktrees`): `setActiveRepo(repoId)` plus clear `activeWorktreeId`. The user lands on the home view with the repo selected; worktrees populate on reconnect.
One gotcha: `useRemoteRepo` currently accepts parent state mutators (`setStep`, `setAddedRepo`, `closeModal`) by reference (`AddRepoSteps.tsx:22-27`). Refactor it to return a result object instead of pushing into parent state — ~30 minutes of work. This is the only meaningful precondition. In the wizard caller, the success branch replaces `setStep('setup') + setAddedRepo(repo) + closeModal()` with a single `onRepoAdded(repo)` callback that the wizard wires to its `completeRepo`/`activateRepoForUser` flow. The dialog's standalone 'setup' confirmation view has no wizard equivalent — the wizard's exit IS the confirmation.
**Ordering.** `activateRepoForUser` runs BEFORE `closeWith`. The primitive only mutates store state; it MUST NOT open Radix surfaces directly. Post-`closeWith` openers (the new-workspace composer for git repos) read the freshly-set `activeRepoId` and target the right surface. Opening modals from inside `activateRepoForUser` would re-trigger the wizard z-index trap.
On the not-a-git-repo branch (`AddRepoSteps.tsx:130-148`), mirror the local 'Open a folder' silent-fallback behavior at `use-onboarding-flow.ts:349-352` — silently retry the add with `kind: 'folder'`. Skip the `'confirm-non-git-folder'` Radix dialog: it would render under the wizard's `z-[100]` overlay, and the wizard already provides the user's consent context (they explicitly chose 'Connect a remote' on a wizard step they cannot dismiss). The sidebar `AddRepoDialog` keeps its existing dialog flow — that surface is outside the wizard and the asymmetry is intentional.
**Why not a per-kind branch.** Folders, local git, and SSH-remote git all hit the worktree-present branch; the empty-list branch is the single degenerate case. Framing this as one primitive (rather than `if (folder) { ... } else if (git) { ... }`) keeps the contract honest: the gate's promise — "add a project and you're in" — has to deliver a usable surface for every kind.
Effort is the same as or less than mounting `AddRepoDialog` itself, and it doesn't collide with the wizard overlay. This keeps the gate honest — "you must add a project" includes remote repos — and matches how emdash and t3code present remote/SSH as a peer of local in their first-run flows. Without this, the gate effectively excludes Orca's SSH audience.
**Why no composer for folders.** `useComposerState` filters to git repos and creates worktrees, which non-git folders can't do. Opening the composer for a folder repo would land the user on a stuck modal with no selectable repo. The composer's git-only contract is intentional; commit `d2448f82` chose to suppress the composer for folders rather than loosen it.
## P1 — Should fix before merge or in a fast follow-up
### 3. Legacy soft-skip migration
### 2. `completeRepo` strands the user when `worktrees:list` returns `[]`
**The trap.** `shouldShowOnboarding()` previously only checked `closedAt === null`. Anyone who was partway through onboarding under the previous "I'll add one later" build would hit the new hard gate on next launch with no warning. The natural-seeming predicate `lastCompletedStep >= 3 && closedAt === null` does NOT identify legacy users — it is exactly the state of any new-build user sitting on the gate. Auto-skipping on that predicate would let any new user bypass the gate by quitting and relaunching during step 4.
`completeRepo()` in `use-onboarding-flow.ts:241-284` (as modified by `d2448f82`) calls `activateAndRevealWorktree(worktreesByRepo[repoId]?.[0])` and then `closeWith('completed', ...)`. The `worktrees:list` IPC handler at `src/main/ipc/worktrees.ts:172-220` synthesizes a folder worktree via `createFolderWorktree(repo)` for `kind: 'folder'` repos (`worktrees.ts:180-181`, `repo-worktrees.ts:5-16`), so folders activate normally — the `d2448f82` comment that folders "complete onboarding via the activateAndRevealWorktree call above" is correct.
**Implementation.** Two persisted fields, both main-write-only (renderer cannot forge either via the sanitizer's `allowInternal` gate; renderer-read access differs per field — see the threat model below):
The real edge case is SSH-remote git: when the SSH provider is unavailable mid-`fetchWorktrees` (reconnect, network blip, transient `listWorktrees` failure caught at `worktrees.ts:210-219`), the handler returns `[]`. If that lands while `completeRepo` is awaiting `fetchWorktrees`, `worktreesByRepo[repoId]?.[0]` is `undefined` and `activateAndRevealWorktree(undefined)` is a silent no-op — user closes the wizard onto the empty home view despite a successful add. This is a transient race, not a guaranteed user-facing bug, which is why this is P1 rather than P0.
- `legacySoftSkipEligible?: boolean` on `OnboardingState` (in `src/shared/types.ts`) marks rows the migration identified as pre-gate. `shouldShowOnboarding` returns `false` for these rows so the wizard is auto-suppressed rather than re-opening on the now-unskippable gate.
- `_legacySoftSkipMigrationDone?: boolean` is the one-shot discriminator. The migration in `Store.load` (in `src/main/persistence.ts`) runs only when this flag is absent. New rows created under the gate-required build are stamped `_legacySoftSkipMigrationDone: true` by `getDefaultOnboardingState` (in `src/shared/constants.ts`), so a user who closes the wizard mid-flow on this build is never marked legacy-eligible on next launch.
Recommended fix: introduce an `activateRepoForUser(repo)` primitive that handles two cases:
**Threat model: the renderer.** Both fields are gated behind an `allowInternal` parameter on `sanitizeOnboardingUpdate` (in `src/main/persistence.ts`). The load path passes `allowInternal: true` to round-trip values off disk; the IPC handler does not, so a renderer that could write `legacySoftSkipEligible: true` cannot auto-suppress the gate, and a renderer that could write `_legacySoftSkipMigrationDone: false` cannot replay the migration. On the read side, the IPC handler in `src/main/ipc/onboarding.ts` strips `_legacySoftSkipMigrationDone` from `onboarding:get` / `onboarding:update` responses (it is a pure main-only discriminator). `legacySoftSkipEligible` is intentionally left exposed because `shouldShowOnboarding` (in `src/renderer/src/components/onboarding/should-show-onboarding.ts`) reads it to drive the auto-suppression branch; the sanitizer's `allowInternal` gate is what keeps the renderer from forging it on writes.
- **Has worktree** (git + worktree present, or any folder repo via the synthesized worktree): `activateAndRevealWorktree(worktreesByRepo[repoId][0].id)` (existing behavior).
- **Empty worktree list** (SSH disconnect / remote unavailable / transient `worktrees:list` failure): `setActiveRepo(repoId)`. The user lands on the home view with the repo selected. When SSH reconnects, the next `fetchAllWorktrees` cycle populates worktrees and the home view reflects them. The gate's "you added a repo" promise is honored at minimum-viable-fidelity.
**Why a marker, not a version gate.** A `lastSeenVersion`-based predicate would have required adding `lastSeenVersion` to `settings.json`, which doesn't exist yet. The marker approach is ~10 LOC + two schema fields, contained entirely in the onboarding state.
Frame this as one primitive, not a per-kind branch — `completeRepo` calls `activateRepoForUser(repo)` and trusts it. The empty-list fallback is the only degenerate branch; the worktree-present branch covers folders, local git, and connected SSH-remote git via the same code path.
**Why not just accept the trap.** The asymmetry bites: one trapped SSH-only power user who upgrades mid-flow files the issue that defines the release. PostHog sizing of the affected cohort would have been the precondition for accepting the trap; the marker approach makes that sizing unnecessary.
**Ordering contract:** `activateRepoForUser` runs BEFORE `closeWith` (preserving today's order in `completeRepo` at `use-onboarding-flow.ts:245-256`). The primitive may queue UI side-effects that only manifest after wizard unmount (the existing `openModal('new-workspace-composer', ...)` after `closeWith` already does this). The primitive itself MUST NOT open Radix surfaces directly — it only mutates store state (`setActiveRepo`, etc.) so the post-`closeWith` openers see the right active repo. This avoids the wizard-z-index trap.
### 4. Telemetry coverage for the gate
Rejected alternative: opening the new-workspace composer for folders. The composer's git-only contract is intentional and loosening it is the larger refactor commit `d2448f82` on this branch already chose to avoid (see its commit message — "suppress composer for folder repos").
**Implementation.** Two new events on the existing `onboarding_step4_*` namespace, plus `'ssh'` added to `onboardingPathSchema`:
The gate's promise — "add a project and you're in" — has to actually deliver a usable surface. Verify by adding an SSH-remote repo with the SSH connection deliberately broken between add and worktree-list, to repro the empty-worktree-list edge case. Local folder, fresh-clone git, and SSH-remote-with-good-connection all activate normally via the existing `activateAndRevealWorktree` path.
- `onboarding_step4_path_revealed { path: 'ssh' }` — the SSH CTA is currently always rendered on `RepoStep`, so first arrival on step 4 in the `'main'` substep counts as the reveal. Latched via `sshRevealedRef` in `OnboardingFlow.tsx` so the event fires once per wizard run, not on every render. Schema is `path: z.literal('ssh')` rather than the full `onboardingPathSchema` because today's only emitter hardcodes `'ssh'`; widening is one line if a future reveal of `open_folder` / `clone_url` is wanted.
- `onboarding_step4_abandoned { duration_ms?, path_revealed_ssh }` — emitted from a `beforeunload` listener inside `OnboardingFlow.tsx` via sync IPC (`trackSync`). Async fire-and-forget would be cancelled before delivery during a real shutdown. The listener is gated on `currentStep.id === 'repo'` (so completed wizards don't emit a spurious abandonment) and `onboarding.closedAt !== null` (covers the microsecond gap between `closeWith` resolving and React unmounting). Registered with `capture: true` to run before `App.tsx`'s captureAndFlush listener regardless of registration order.
### 3. In-flight users get retroactively gated on next launch (post-merge)
**SSH path failures.** `retryRemoteAsFolder` emits `onboarding_step4_path_failed { path: 'ssh', reason: 'unknown' }` on the not-a-git-repo retry. The `'unknown'` tag (rather than `'invalid_path'`) is deliberate: this branch only runs once the original git-add already failed, and the `kind: 'folder'` retry skips the git check, so any failure here is an SSH connect drop, perm error, or disk fault — not path-shape.
`shouldShowOnboarding()` in `should-show-onboarding.ts:6` returns true for any user with `onboarding !== null && closedAt === null`. Once PR #1677 ships, anyone who was partway through onboarding under the previous "I'll add one later" build will hit the new hard gate the next time they launch, with no warning and no migration.
**SSH path clicks.** `onboarding_step4_path_clicked { path: 'ssh' }` rides on the existing `onboardingStep4PathClickedSchema` once `'ssh'` is in `onboardingPathSchema`; no new event needed.
**No safe migration without a new discriminator.** The natural-seeming predicate `lastCompletedStep >= 3 && closedAt === null` does NOT uniquely identify old-build users — it is exactly the state of any new-build user who reached the repo gate without adding a project yet. Verified:
**What this captures, what it doesn't.** Cmd+Q, native window close, and renderer reload all fire `beforeunload`. App crashes, force-kill, and OS shutdown do not — those need a main-process recovery check on next launch (read persisted `lastCompletedStep === 3 && closedAt === null` and emit a delayed event). That recovery path is design intent only; not implemented in this PR.
- `use-onboarding-flow-persistence.ts:142-159` — completing the notifications step (step index 3) calls `persistStep(3)` and persists `lastCompletedStep === 3` with no project added yet.
- `tests/e2e/onboarding.spec.ts:344-356` — the e2e for the gate explicitly asserts `lastCompletedStep === 3 && closedAt === null && outcome === null` as the expected state of a new user sitting on the repo gate.
**Why extend the existing namespace.** Keeps PostHog dashboards and `cohort` injection (in `src/shared/telemetry-events.ts`) working without schema-registry churn. All new events inherit the `cohort` discriminator already present on every onboarding event.
Auto-skipping that state would let any new user bypass the required first repo by quitting and relaunching during step 4 — exactly the regression the gate was added to prevent.
### 5. `OnboardingOutcome` narrowed to `'completed'`
Recommended fix: do not ship a migration based on the existing fields. Three viable options, in order of preference:
**Implementation.** `OnboardingOutcome` in `src/shared/types.ts` is narrowed to the single literal `'completed'`. `OnboardingState['outcome']` is `OnboardingOutcome | null`, and new writes are `'completed'` or `null`. `useCloseWith` accepts only `outcome: 'completed'` at the type level.
1. **Add a persisted "saw soft-skip" marker.** Pre-merge, write a one-time migration in `persistence.ts` that sets `legacySoftSkipEligible: true` on any existing `OnboardingState` row at first-load under the new build. Then the predicate becomes `legacySoftSkipEligible && closedAt === null` and is unambiguous. ~10 LOC + two schema fields (the eligibility flag + a one-shot discriminator `_legacySoftSkipMigrationDone` so the migration runs exactly once per install — defaulted `true` on new rows in `getDefaultOnboardingState` so a new user closing mid-flow on this build cannot retroactively be marked legacy; absent on legacy rows, which is what triggers the stamp at parse time in `Store.load`).
2. **Gate on app version.** If `settings.json` (or wherever app metadata is persisted) gains a `lastSeenVersion` field this release, treat any user whose `lastSeenVersion` is below the gate-merge version AND whose `closedAt === null` as legacy-skipped. Larger change because `lastSeenVersion` likely doesn't exist yet.
3. **Accept the trap, communicate via release notes.** Cheapest, but the asymmetry the original framing pointed out still bites: one trapped SSH-only power user who upgraded mid-flow files the issue that defines the release. Acceptable only if the cohort is empirically near-zero (size via PostHog before deciding).
**Legacy `'dismissed'` rows on disk.** The `outcome` branch of `sanitizeOnboardingUpdate` in `src/main/persistence.ts` accepts both `null` and `'dismissed'` at the parse boundary and coerces both to `null`. Pre-#1677 users who dismissed the wizard still count as "wizard closed" via `closedAt`; they just don't carry a winning outcome into the narrowed union. This keeps the type honest without breaking old data.
PostHog sizing (Orca project 406068, count of `closedAt === null && onboarding !== null` users without a recent `onboarding_completed`) is now a precondition for option 3, not optional. For options 1 and 2 it's only useful as instrumentation.
**Telemetry registry retained.** `onboardingDismissedSchema` and the `onboarding_dismissed` registration in `src/shared/telemetry-events.ts` are intentionally NOT removed yet. Old clients with queued events from before the upgrade need the schema to validate when they retry. Removal is a follow-up release after queued-event drain — tracked as design intent, not a current-PR todo.
The previous version of this section recommended setting `closedAt = Date.now()` on `lastCompletedStep >= 3 && closedAt === null` "with no date gate needed." That recommendation was wrong — the predicate is satisfied by every new-build user on the gate, not just legacy ones. Do not implement it.
**Why narrow at the type level.** Leaving the union as `'completed' | 'dismissed'` while removing every writer invites accidental resurrection. The narrowed type is the contract that the persistence layer's coercion enforces at runtime.
### 4. Telemetry blind spot for users stuck on step 4 — ship before GA
---
Commit `d2448f82` on this branch removes `closeWith('dismissed', ...)` entirely. With PR #1677, the repo step's outcome is `completed`-or-nothing — gate failure is silent. Without explicit signals, you cannot distinguish "user closed the wizard cleanly" from "user gave up and quit the app." If the gate ships to GA without telemetry, the first signal of a failure mode will be a support ticket.
## Deferred / not implemented in this PR
Ship before GA / public release of the gate. Extend the existing `onboarding_step4_*` namespace (`telemetry-events.ts:290-291, 374-383, 541-544`) rather than coining a parallel one — keeps PostHog dashboards and `cohort` injection (`telemetry-events.ts:619`) working without schema-registry churn. All new events inherit the `cohort` discriminator already present on every onboarding event.
### 6. Clone-failure escape
- **Wizard abandoned on step 4 (NEW).** The skip-blocked guard at `use-onboarding-flow.ts:398-401` is defensive only — `OnboardingFlow.tsx:179, 197` gates the Skip and Continue buttons on `currentStep.id !== 'repo'`, and `OnboardingFlow.tsx:65-69` routes Cmd/Ctrl+Enter on the repo step to `flowOpenFolder` instead of `next`/`skip`. So `setError('Add a project to continue.')` at `use-onboarding-flow.ts:401` never fires in normal usage — instrumenting it captures nothing. Instead, emit `onboarding_step4_abandoned` from a `beforeunload` listener inside `OnboardingFlow.tsx` (synchronous `beforeunload` + sync IPC, the only reliable shutdown signal in Orca). Mounting the listener inside `OnboardingFlow.tsx` (rather than `App.tsx`) keeps it scoped to the wizard's lifetime and gates on `currentStep.id === 'repo'` so completed wizards don't emit a spurious abandonment. Properties: `duration_ms` (time on step 4, via `consumeStepDurationMs()`), `path_revealed_ssh: boolean` (whether the SSH CTA was visible). What this captures: Cmd+Q, native window close, and renderer reload. What it does NOT capture: app crashes, force-kill, OS shutdown — renderer telemetry is fire-and-forget async IPC (`src/renderer/src/lib/telemetry.ts:50`) and the renderer process is gone before any of those land. For crash coverage you'd need a main-process check on next launch (read persisted `lastCompletedStep === 3 && closedAt === null && lastQuitWasClean === false` and emit a delayed `onboarding_step4_abandoned_recovered` event) — out of scope here, but flag it if crash rates on step 4 turn out to matter. Add the new event to the registry alongside the existing `onboarding_step4_*` schemas (`telemetry-events.ts:374-383`). A renderer `useEffect` cleanup is NOT a substitute — async IPC will be cancelled before delivery during a real shutdown.
- **Clone failure.** Already exists as `onboarding_step4_path_failed { path: 'clone_url', reason: 'clone_failed' }` (`use-onboarding-flow.ts:385`). No new event. Optionally extend `onboardingFailureReasonSchema` to add `'auth_failed' | 'network_error'` if a finer breakdown is desired.
- **SSH CTA revealed (NEW, depends on #1 landing).** Add `onboarding_step4_path_revealed { path: 'ssh' }` so we can see whether SSH-only users actually discover the third CTA. Add `'ssh'` to `onboardingPathSchema` (`telemetry-events.ts:290`). The same schema addition lights up the next bullet.
- **SSH CTA clicked.** Reuse `onboarding_step4_path_clicked { path: 'ssh' }` — clicked-on-SSH parallels clicked-on-clone. Lands automatically once `'ssh'` is added to `onboardingPathSchema`.
- **`time_on_step` distribution for step 4.** `onboarding_step_completed` already emits `duration_ms` (`use-onboarding-flow.ts:264-267`) but only on success — long-tail and abandonment cases are exactly the ones that don't fire it. The new `onboarding_step4_abandoned` event above carries `duration_ms`, which closes the long-tail gap without a heartbeat or per-event opt-in. No timer plumbing.
A bad URL, expired credentials, a slow/large repo, or a network blip surfaces as an inline error on `RepoStep` and that's it. The SSH CTA covers most of these users (they can pivot to remote), but a residual case remains: user with no SSH target, only repo they want is failing to clone. A "Having trouble? Open Settings" link or a back-to-previous-step affordance would close the gap. Not implemented; the SSH CTA absorbs most of the population.
Schema-edit pointer: `onboardingStep4PathFailedSchema` (`telemetry-events.ts:377-383`) requires `path: onboardingPathSchema`. It does not need to change for the abandonment event (which is its own schema, no `path` property). If a future failure-shaped event has no natural `path`, make `path` optional on `onboardingStep4PathFailedSchema` rather than adding a sentinel like `'none'` to `onboardingPathSchema` — smaller change, no fallout on existing dashboards.
### 7. Inline workspace-directory change on RepoStep
### 5. `OnboardingOutcome` shared type is stale on this branch
"Clone a repo" clones into `settings.workspaceDir`; the path is shown in small grey text but not editable inline. Predates the gate, but the hard gate means a user who dislikes the workspace dir can no longer say "skip, I'll do this manually later." A "Change…" link next to the workspace path would cost little. Not implemented; low value-per-user, design intent only.
`src/shared/types.ts:1372` still defines `OnboardingOutcome = 'completed' | 'dismissed'` even though commit `d2448f82` on this branch removes every renderer-side `'dismissed'` writer. Either restore the soft-skip (out of scope per "Out of scope" below) or narrow the type to `'completed'`. Leaving it half-alive invites accidental resurrection.
### 8. Drag-and-drop folders onto RepoStep
The dead branch is wider than just the type alias: `src/main/persistence.ts:144` still accepts both literals at the parse boundary; `src/shared/telemetry-events.ts:392` defines `onboardingDismissedSchema`; `:544` registers `onboarding_dismissed` in `eventSchemas`; and `:638` lists it in the cohort roster. ~10–15 LOC across 3 files (`src/shared/types.ts`, `src/main/persistence.ts`, `src/shared/telemetry-events.ts`) plus a one-line policy decision.
Superset's `StartView` accepts folder drops directly; Orca's `RepoStep` does not. React `onDrop` handlers do NOT fire for OS file drops in Orca — the preload calls `e.preventDefault()` on the document root and routes via the `terminal:file-dropped-from-preload` IPC bridge. Implementation would extend `NativeDropResolution`'s target union, add a `data-native-file-drop-target='onboarding-repo-step'` marker on the drop zone, subscribe via `window.api.ui.onFileDrop` from the wizard, and route the first directory through `repos.add({ kind: 'folder' })`. Pairs naturally with `activateRepoForUser`. ~25 LOC; not implemented.
Policy decision: on disk the persistence loader at `src/main/persistence.ts:144` will see legacy `outcome: 'dismissed'` rows from users who dismissed under the old build. Recommended: keep accepting the legacy value at the parse boundary (don't break old users), but stop writing new ones — narrow `OnboardingOutcome` to `'completed'` at the type level and let `persistence.ts` coerce legacy `'dismissed'` to `null` (or to a new `'legacy_dismissed'` literal if downstream wants to distinguish). Don't remove `onboardingDismissedSchema` from the telemetry registry until a release after this — keeps old clients valid if they retry queued events.
### 9. Crash-recovery telemetry for step 4
## P2 — Nice to have
`onboarding_step4_abandoned` covers Cmd+Q and renderer reload. Crashes / force-kill / OS shutdown need a main-process recovery check on next launch (persisted `lastCompletedStep === 3 && closedAt === null && lastQuitWasClean === false` → emit a delayed `onboarding_step4_abandoned_recovered`). Design intent only; not implemented.
### 6. Clone failures have no escape
### 10. Empty-state home view as the gate (long-term direction)
A bad URL, expired credentials, a large/slow repo, or a network blip surfaces as an inline error on the repo step (`RepoStep.tsx:97-101`) and that's it. Without the gate these users could click "I'll add one later" and recover; with PR #1677 they cannot.
Adding the SSH CTA (issue 1) gives most of these users another viable path. A "Having trouble? Open Settings" link or a back-to-previous-step affordance would cover the residual case where a user has no SSH target and the only repo they want is failing to clone.
### 7. Workspace directory is fixed silently
"Clone a repo" clones into `settings.workspaceDir` (`use-onboarding-flow.ts:380`). The repo step shows the workspace path in small grey text at the bottom but does not let you change it inline. Users on multiple disks, or with strong opinions about where their code lives, may want to clone elsewhere.
This predates PR #1677, but the new hard gate means a user who dislikes the workspace dir can no longer say "skip, I'll do this manually later". A "Change…" link next to the workspace path on this step would cost little and remove a small friction point.
### 8. App-quit during onboarding (verify-then-close)
Manual smoke test: open Orca → step 4 → quit (Cmd+Q) → reopen. Verify the wizard resumes at step 4 (not step 1) and that `lastCompletedStep` clamping at `src/main/persistence.ts` and `use-onboarding-flow.ts:36` (`Math.min(Math.max(onboarding.lastCompletedStep, 0), STEPS.length - 1)`) preserves the resume point. Expected to work today; this item closes when verified. 5-minute test, not a design item.
### 9. Drag-and-drop folders onto RepoStep
Superset's `StartView` accepts folder drops directly on the empty surface. Orca's `RepoStep` does not. For a hard gate, the lowest-friction "I have a folder, take it" path is unblocked by ~25 LOC, hooking into the existing preload `terminal:file-dropped-from-preload` bridge with a new wizard-step resolver target. React `onDrop` handlers do NOT fire for OS file drops in Orca — the preload at `src/preload/index.ts:167-244` calls `e.preventDefault()` on the document root and routes via IPC. Implementation: extend `NativeDropResolution`'s target union, add a `data-native-file-drop-target='onboarding-repo-step'` marker on the drop zone, subscribe via `window.api.ui.onFileDrop` from the wizard, and route the first directory through `repos.add({ kind: 'folder' })`. Pairs naturally with the `activateRepoForUser` primitive from #2. Optional, but high value-per-line.
## Future direction (out of scope for this PR)
**Empty-state home view as the gate.** Superset (the closest comparable Electron desktop competitor) ships exactly this pattern: wizard for *preferences* (agent, theme, notifications), a persistent first-project empty-state on the home view. This dissolves four classes of bug at once: modal stacking inside the wizard (the architectural note above), the empty-worktree-list activation gap (#2), clone-fail trap (#6), and the in-flight upgrade trap (#3). It also matches the standard macOS app pattern. This is the right long-term direction, but it's a structural rewrite of the wizard's terminal step — not "the same fix, refactored." Track separately.
Superset (the closest comparable Electron desktop competitor) ships a wizard for *preferences* (agent, theme, notifications) and a persistent first-project empty-state on the home view as the gate. This dissolves four classes of issue at once: modal stacking inside the wizard (the architectural note), the empty-worktree-list branch in `activateRepoForUser` (item 2), clone-fail trap (item 6), and the in-flight upgrade trap (item 3). It also matches the standard macOS app pattern. Right long-term direction; structural rewrite of the wizard's terminal step, not "the same fix, refactored." Tracked separately.
## Out of scope
- Whether to gate at all. The decision to require a first project stands; this doc only addresses gaps in the current gate's coverage and recovery paths.
- Soft-skip with reminder. Considered and rejected once the SSH path is added — the strongest "I want to skip" case (SSH user trapped) goes away with issue 1, and the remaining "let me poke around" case is weak for a workspace-orchestrator app.
- A4 / empty-state home view. See "Future direction" section above. Different in kind from the patches in this doc.
- **Whether to gate at all.** Settled. This doc explains the shipped gate; it does not relitigate the decision.
- **Soft-skip with reminder.** Considered and rejected once the SSH path landed — the strongest "I want to skip" case (SSH user trapped) is gone, and the remaining "let me poke around" case is weak for a workspace-orchestrator app.
- **Empty-state home view.** See item 10 in the deferred section.
+12 -2
View File
@@ -2,15 +2,25 @@ import { ipcMain } from 'electron'
import { sanitizeOnboardingUpdate, type Store } from '../persistence'
import type { OnboardingState } from '../../shared/types'
// Why: `_legacySoftSkipMigrationDone` is a main-only persistence
// discriminator. The shared type still carries it so persistence.ts can
// read/write it, but the renderer must never see it — leaking it widens
// the contract beyond what the comments promise. Strip before crossing
// the IPC boundary on both `get` and `update` responses.
function toRendererSafe(state: OnboardingState): OnboardingState {
const { _legacySoftSkipMigrationDone: _omit, ...rendererSafe } = state
return rendererSafe
}
export function registerOnboardingHandlers(store: Store): void {
ipcMain.removeHandler('onboarding:get')
ipcMain.removeHandler('onboarding:update')
ipcMain.handle('onboarding:get', (): OnboardingState => store.getOnboarding())
ipcMain.handle('onboarding:get', (): OnboardingState => toRendererSafe(store.getOnboarding()))
// Why: never trust renderer input — a compromised/buggy caller could send
// unknown keys or wrong-typed values that would poison persisted state.
// Run every update through the shared whitelist sanitizer.
ipcMain.handle('onboarding:update', (_event, updates: unknown): OnboardingState => {
return store.updateOnboarding(sanitizeOnboardingUpdate(updates))
return toRendererSafe(store.updateOnboarding(sanitizeOnboardingUpdate(updates)))
})
}
+20 -8
View File
@@ -284,18 +284,30 @@ function App(): React.JSX.Element {
// main-side updateOnboarding clears the eligibility flag
// automatically on the closedAt transition (the flag is
// rejected from renderer-supplied IPC updates by design).
// If the persist write throws, the outer try/catch logs and
// we leave onboarding null — next launch hits this path again
// (idempotent), which is benign.
// If the seal IPC fails, fall back to in-memory suppression
// (legacySoftSkipEligible already keeps shouldShowOnboarding
// false this launch) and let the rest of hydration — SSH
// reconnect, terminal restore, zoom sync — proceed. The seal
// is retried on next launch (idempotent).
if (
onboardingState.legacySoftSkipEligible === true &&
onboardingState.closedAt === null
) {
const sealed = await window.api.onboarding.update({
closedAt: Date.now()
})
if (!cancelled) {
setOnboarding(sealed)
try {
const sealed = await window.api.onboarding.update({
closedAt: Date.now()
})
if (!cancelled) {
setOnboarding(sealed)
}
} catch (err) {
console.warn(
'[onboarding] legacy migration update failed; suppressing wizard via in-memory state',
err
)
if (!cancelled) {
setOnboarding(onboardingState)
}
}
} else {
setOnboarding(onboardingState)
@@ -63,7 +63,10 @@ export default function OnboardingFlow({
if (currentStep.id === 'repo' && step4StartedAtRef.current === null) {
step4StartedAtRef.current = Date.now()
}
if (currentStep.id === 'repo' && !sshRevealedRef.current) {
// Why: enforce the documented invariant — the SSH CTA is only visible on
// the 'main' substep, so don't latch the reveal if some future code path
// auto-routes a first arrival straight into the 'remote' form.
if (currentStep.id === 'repo' && repoSubstep === 'main' && !sshRevealedRef.current) {
sshRevealedRef.current = true
track('onboarding_step4_path_revealed', { path: 'ssh' })
}
@@ -73,7 +76,7 @@ export default function OnboardingFlow({
if (currentStep.id !== 'repo') {
setRepoSubstep('main')
}
}, [currentStep.id])
}, [currentStep.id, repoSubstep])
// Why: depend on stable callbacks + step id only so the listener doesn't
// re-bind on every render of the parent (flow object identity changes).
const { next: flowNext, openFolder: flowOpenFolder } = flow
@@ -109,12 +112,23 @@ export default function OnboardingFlow({
// Why: shutdown signal for users who quit while still gated on step 4.
// Sync IPC because async fire-and-forget would be cancelled before delivery
// when the renderer exits. Only fires on the repo step, so completed
// wizards don't emit a spurious abandonment.
// wizards don't emit a spurious abandonment. Two extra guards:
// - closedAt check: between closeWith resolving and React unmounting this
// component, a quit in that microsecond gap would otherwise emit an
// abandonment alongside the legitimate completion event.
// - capture phase: App.tsx registers a captureAndFlush beforeunload
// listener; if it ran first and the BrowserWindow was killed before our
// listener fired, the event would be lost. Capture runs before non-
// capture regardless of registration order. Cleanup must mirror the
// capture flag for removeEventListener to match.
useEffect(() => {
const onBeforeUnload = (): void => {
if (currentStep.id !== 'repo') {
return
}
if (onboarding.closedAt !== null) {
return
}
const startedAt = step4StartedAtRef.current
const duration = startedAt !== null ? Math.max(0, Date.now() - startedAt) : undefined
trackSync('onboarding_step4_abandoned', {
@@ -122,9 +136,9 @@ export default function OnboardingFlow({
path_revealed_ssh: sshRevealedRef.current
})
}
window.addEventListener('beforeunload', onBeforeUnload)
return () => window.removeEventListener('beforeunload', onBeforeUnload)
}, [currentStep.id])
window.addEventListener('beforeunload', onBeforeUnload, { capture: true })
return () => window.removeEventListener('beforeunload', onBeforeUnload, { capture: true })
}, [currentStep.id, onboarding.closedAt])
return (
<div className="fixed inset-0 z-[100] overflow-auto bg-background text-foreground">
@@ -156,7 +156,11 @@ export function RemoteRepoStep({
selectedTargetId={selectedTargetId}
remotePath={remotePath}
remoteError={remoteError}
isAddingRemote={isAddingRemote}
// Why: the wizard's `busyLabel` covers the silent folder-retry path
// that runs in flow.retryRemoteAsFolder. Without this, the Add button
// re-enables during the retry's IPC round-trip and a second click
// dispatches a duplicate repos.addRemote.
isAddingRemote={isAddingRemote || busyLabel !== null}
onSelectTarget={(id) => {
setSelectedTargetId(id)
setRemoteError(null)
@@ -82,6 +82,11 @@ export function RepoStep({
<button
type="button"
// Why: explicit accessible name. Without it, the computed name is the
// concatenation of all descendant text ("Connect a remote (SSH) Open a
// project on a remote machine over SSH. Connect…"), which makes
// role+name locators (and assistive tech) unwieldy.
aria-label="Connect a remote (SSH)"
className="group flex w-full items-center gap-4 rounded-xl border border-border bg-muted/30 p-5 text-left transition hover:border-foreground/40 hover:bg-muted/60 disabled:opacity-60"
disabled={disabled}
onClick={onConnectRemote}
@@ -36,7 +36,10 @@ export function useCloseWith({
async (
outcome: 'completed',
checklist: Partial<OnboardingState['checklist']>,
completedPath?: 'open_folder' | 'clone_url' | 'ssh'
// Why: required (not optional) so the funnel `onboarding_completed` event
// always fires on wizard completion. An optional param invites a silent
// funnel-loss regression if a future caller forgets to pass it.
completedPath: 'open_folder' | 'clone_url' | 'ssh'
): Promise<boolean> => {
let nextState: OnboardingState
try {
@@ -58,29 +61,27 @@ export function useCloseWith({
return false
}
onOnboardingChange(nextState)
if (completedPath) {
const total = Math.max(0, Date.now() - startTimeRef.current)
track('onboarding_completed', {
path: completedPath,
is_git_repo: checklist.addedRepo === true,
total_duration_ms: total
const total = Math.max(0, Date.now() - startTimeRef.current)
track('onboarding_completed', {
path: completedPath,
is_git_repo: checklist.addedRepo === true,
total_duration_ms: total
})
// Why: checklist items completed by the wizard itself must fire
// `activation_checklist_item_completed` so the post-wizard panel and
// analytics agree. Other items (ranFirstAgent, triedCmdJ, …) emit
// from their own product surfaces.
if (checklist.addedRepo && !onboardingChecklist.addedRepo) {
track('activation_checklist_item_completed', {
item: 'addedRepo',
time_since_completed_ms: 0
})
}
if (checklist.addedFolder && !onboardingChecklist.addedFolder) {
track('activation_checklist_item_completed', {
item: 'addedFolder',
time_since_completed_ms: 0
})
// Why: checklist items completed by the wizard itself must fire
// `activation_checklist_item_completed` so the post-wizard panel and
// analytics agree. Other items (ranFirstAgent, triedCmdJ, …) emit
// from their own product surfaces.
if (checklist.addedRepo && !onboardingChecklist.addedRepo) {
track('activation_checklist_item_completed', {
item: 'addedRepo',
time_since_completed_ms: 0
})
}
if (checklist.addedFolder && !onboardingChecklist.addedFolder) {
track('activation_checklist_item_completed', {
item: 'addedFolder',
time_since_completed_ms: 0
})
}
}
return true
},
@@ -253,6 +253,11 @@ export function useOnboardingFlow(
return
}
setActiveRepo(repoId)
// Why: a stale activeWorktreeId from a hydrated session would otherwise
// carry over and route the user to that previous repo's terminal instead
// of the new repo's home view; clear it so worktrees populate fresh on
// reconnect.
useAppStore.setState({ activeWorktreeId: null })
},
[setActiveRepo]
)
@@ -313,6 +318,10 @@ export function useOnboardingFlow(
// already chose 'Connect a remote' on a step they cannot dismiss.
const retryRemoteAsFolder = useCallback(
async (args: { connectionId: string; remotePath: string }) => {
// Why: re-entry guard — prevents duplicate retries from the not-a-git-repo branch.
if (busyLabel !== null) {
return
}
setError(null)
setBusyLabel('Adding folder…')
try {
@@ -338,7 +347,7 @@ export function useOnboardingFlow(
setBusyLabel(null)
}
},
[completeRepo]
[busyLabel, completeRepo]
)
const persistCurrentStep = usePersistCurrentStep({
+4 -1
View File
@@ -377,8 +377,11 @@ const onboardingStep4PathClickedSchema = z
// Why: distinct from `path_clicked` because revealing the SSH CTA happens
// before any repo-add attempt — it tells us whether SSH-only users actually
// discover the third CTA, separate from whether they then succeed.
// Currently SSH-specific by design (matches design doc framing); the only
// emitter hardcodes `path: 'ssh'`. If a future reveal of `open_folder` or
// `clone_url` is wanted, widen the schema to `onboardingPathSchema` then.
const onboardingStep4PathRevealedSchema = z
.object({ path: onboardingPathSchema, cohort: cohortSchema })
.object({ path: z.literal('ssh'), cohort: cohortSchema })
.strict()
const onboardingStep4PathFailedSchema = z
.object({
+4
View File
@@ -1413,6 +1413,10 @@ export type OnboardingState = {
// Stamped true the first time the new build loads any persisted onboarding
// row, so the migration runs exactly once per install. New rows created
// by the new build are stamped at write time via the default state.
// Main-only at runtime: `src/main/ipc/onboarding.ts` strips this field
// from `onboarding:get` / `onboarding:update` responses before crossing
// the IPC boundary, so the renderer must never read it. Kept in the
// shared type so persistence.ts (which writes it) stays well-typed.
_legacySoftSkipMigrationDone?: boolean
checklist: OnboardingChecklistState
}
+12 -3
View File
@@ -348,7 +348,9 @@ test.describe('Onboarding flow', () => {
// contract — without it, SSH users would be unable to complete the
// wizard. Pin it here so a regression that drops the third path is
// caught alongside the no-skip assertion above.
await expect(orcaPage.getByRole('button', { name: /Connect a remote/i })).toBeVisible()
await expect(
orcaPage.getByRole('button', { name: 'Connect a remote (SSH)', exact: true })
).toBeVisible()
// Why: pressing Escape must not close the wizard while gated. The
// unskippable contract holds for keyboard close-vectors too, not just
@@ -356,7 +358,14 @@ test.describe('Onboarding flow', () => {
// with no backdrop, so click-outside isn't a real vector here.)
await orcaPage.keyboard.press('Escape')
await expect(orcaPage.getByRole('heading', { name: /Add your first project/i })).toBeVisible()
expect((await getOnboardingState(orcaPage)).closedAt).toBeNull()
// Why: poll instead of synchronous read so any erroneous async close
// handler has time to fire before the assertion sticks (mirrors the
// expect.poll pattern used elsewhere in this test).
await expect
.poll(async () => (await getOnboardingState(orcaPage)).closedAt, {
timeout: 1_000
})
.toBeNull()
// Why: the previous Skip click flushes persistence async via IPC, so poll
// for lastCompletedStep === 3 before snapshotting the rest of the state.
@@ -368,7 +377,7 @@ test.describe('Onboarding flow', () => {
const final = await getOnboardingState(orcaPage)
expect(final.closedAt).toBeNull()
expect(final.outcome).toBeNull()
expect(final.checklist.addedRepo).toBe(false)
expect(final.checklist.dismissed).toBe(false)
})
})