From d2448f82e643feba509643abfb61e1ff151ca261 Mon Sep 17 00:00:00 2001 From: brennanb2025 Date: Sun, 10 May 2026 18:12:06 -0700 Subject: [PATCH] fix(onboarding): suppress composer for folder repos and prune dead dismissed path The composer is git-only (eligibleRepos filters with isGitRepoKind), so opening it after a non-git first project leaves the user on a stuck modal with no selectable repo. Gate the openModal call on isGit; folders complete onboarding via activateAndRevealWorktree above. Now that the only closeWith('dismissed', ...) caller is gone, narrow useCloseWith to outcome:'completed', drop DismissedExtras and the unused lastStepReached parameter, and remove the unreachable onboarding_dismissed track call. Tighten the new e2e test: exact match for the absent Skip button, assert the new repo-step subtitle, and poll lastCompletedStep so the prior Skip's IPC flush doesn't race the snapshot. Co-authored-by: Orca --- .../use-onboarding-flow-persistence.ts | 32 +++++-------------- .../onboarding/use-onboarding-flow.ts | 23 +++++++------ tests/e2e/onboarding.spec.ts | 16 ++++++++-- 3 files changed, 36 insertions(+), 35 deletions(-) diff --git a/src/renderer/src/components/onboarding/use-onboarding-flow-persistence.ts b/src/renderer/src/components/onboarding/use-onboarding-flow-persistence.ts index 1c668f281ec..0635a48050b 100644 --- a/src/renderer/src/components/onboarding/use-onboarding-flow-persistence.ts +++ b/src/renderer/src/components/onboarding/use-onboarding-flow-persistence.ts @@ -3,7 +3,7 @@ import { track } from '@/lib/telemetry' import { ONBOARDING_FINAL_STEP } from '../../../../shared/constants' import type { GlobalSettings, OnboardingState, TuiAgent } from '../../../../shared/types' import type { NotificationDraft } from './NotificationStep' -import type { StepId, StepNumber } from './use-onboarding-flow-types' +import type { StepId } from './use-onboarding-flow-types' export async function persistStep( stepNumber: number, @@ -26,11 +26,6 @@ type CloseWithDeps = { setError: (msg: string | null) => void } -export type DismissedExtras = { - advancedVia: 'button' | 'keyboard' - durationMs: number -} - export function useCloseWith({ onOnboardingChange, onboardingChecklist, @@ -39,24 +34,23 @@ export function useCloseWith({ }: CloseWithDeps) { return useCallback( async ( - outcome: 'completed' | 'dismissed', + outcome: 'completed', checklist: Partial, - lastStepReached: StepNumber, - completedPath?: 'open_folder' | 'clone_url', - dismissedExtras?: DismissedExtras + completedPath?: 'open_folder' | 'clone_url' ): Promise => { let nextState: OnboardingState try { // Why: main-process updateOnboarding already merges with current state, // so spreading the local (potentially stale) onboarding.checklist would - // overwrite concurrent updates. + // overwrite concurrent updates. Writing dismissed:false explicitly + // resets any prior on-disk true value via the merge. nextState = await window.api.onboarding.update({ closedAt: Date.now(), outcome, - lastCompletedStep: outcome === 'completed' ? ONBOARDING_FINAL_STEP : -1, + lastCompletedStep: ONBOARDING_FINAL_STEP, checklist: { ...checklist, - dismissed: outcome === 'dismissed' + dismissed: false } }) } catch (err) { @@ -64,7 +58,7 @@ export function useCloseWith({ return false } onOnboardingChange(nextState) - if (outcome === 'completed' && completedPath) { + if (completedPath) { const total = Math.max(0, Date.now() - startTimeRef.current) track('onboarding_completed', { path: completedPath, @@ -87,16 +81,6 @@ export function useCloseWith({ time_since_completed_ms: 0 }) } - } else if (outcome === 'dismissed') { - track('onboarding_dismissed', { - last_step: lastStepReached, - ...(dismissedExtras - ? { - duration_ms: dismissedExtras.durationMs, - advanced_via: dismissedExtras.advancedVia - } - : {}) - }) } return true }, diff --git a/src/renderer/src/components/onboarding/use-onboarding-flow.ts b/src/renderer/src/components/onboarding/use-onboarding-flow.ts index b27c7bb225f..fd1064954be 100644 --- a/src/renderer/src/components/onboarding/use-onboarding-flow.ts +++ b/src/renderer/src/components/onboarding/use-onboarding-flow.ts @@ -252,7 +252,6 @@ export function useOnboardingFlow( const closed = await closeWith( 'completed', isGit ? { addedRepo: true } : { addedFolder: true }, - 4, path ) if (!closed) { @@ -267,13 +266,19 @@ export function useOnboardingFlow( value_kind: 'repo', duration_ms: consumeStepDurationMs() }) - // Why: plain folders are valid first projects too; suppressing the - // composer for them strands users after onboarding with no workspace. - openModal('new-workspace-composer', { - initialRepoId: repoId, - prefilledName: 'onboarding', - telemetrySource: 'onboarding' - }) + // Why: plain folders are valid first projects, but the composer is + // git-only (useComposerState filters to git repos and creates worktrees, + // which non-git folders can't do). Folders complete onboarding via the + // activateAndRevealWorktree call above, which navigates the user + // straight into the folder workspace; opening the composer for them + // would land them on a stuck modal with no selectable repo. + if (isGit) { + openModal('new-workspace-composer', { + initialRepoId: repoId, + prefilledName: 'onboarding', + telemetrySource: 'onboarding' + }) + } }, [closeWith, consumeStepDurationMs, fetchRepos, fetchWorktrees, openModal] ) @@ -398,7 +403,7 @@ export function useOnboardingFlow( } // Why: skip has no keyboard path today, so `advanced_via` is always // `'button'`. Including the field keeps the shape uniform with the - // completed/dismissed events and lets a future keyboard-skip arrive + // completed events and lets a future keyboard-skip arrive // without a schema migration. const durationMs = consumeStepDurationMs() track('onboarding_step_skipped', { diff --git a/tests/e2e/onboarding.spec.ts b/tests/e2e/onboarding.spec.ts index cc86897e3e9..2e355fd9b2c 100644 --- a/tests/e2e/onboarding.spec.ts +++ b/tests/e2e/onboarding.spec.ts @@ -333,13 +333,25 @@ test.describe('Onboarding flow', () => { ).toBeVisible() await orcaPage.getByRole('button', { name: 'Skip' }).click() await expect(orcaPage.getByRole('heading', { name: /Add your first project/i })).toBeVisible() + await expect( + orcaPage.getByText(/Orca needs a folder or repo before it can create a workspace/i) + ).toBeVisible() await expect(orcaPage.getByRole('button', { name: /I'll add one later/ })).toHaveCount(0) - await expect(orcaPage.getByRole('button', { name: 'Skip' })).toHaveCount(0) + // Why: exact match — any unrelated "Skip…" button could give a false + // negative (mirrors the 'Back' exact-match rationale above). + await expect(orcaPage.getByRole('button', { name: 'Skip', exact: true })).toHaveCount(0) + + // Why: the previous Skip click flushes persistence async via IPC, so poll + // for lastCompletedStep === 3 before snapshotting the rest of the state. + await expect + .poll(async () => (await getOnboardingState(orcaPage)).lastCompletedStep, { + timeout: 5_000 + }) + .toBe(3) const final = await getOnboardingState(orcaPage) expect(final.closedAt).toBeNull() expect(final.outcome).toBeNull() expect(final.checklist.dismissed).toBe(false) - expect(final.lastCompletedStep).toBe(3) }) })