From 7efbe1039421484f5bba8fefa16c4c52bbd433e7 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Thu, 9 Jul 2026 00:46:03 -0700 Subject: [PATCH] fix(onboarding): dismiss on notifications step + drop skipped steps from stepper (#7909) Two onboarding-screen bugs: - The final "notifications" step blocked click-off/Escape dismissal, unlike every other step. Remove the notifications-only guard so the skip confirmation opens on all steps; the footer "Skip to project setup" stays hidden there since the primary button already hands off to Add Project. - A skipped "integrations" step (GitHub CLI already installed) still rendered as a dead, disabled stepper dot the user skipped past on Continue. The stepper now drops all skipped steps (integrations + Windows terminal) entirely instead of showing an unreachable dot. Allowing dismissal on the last step let a click-off race the "Add your first project" completion handoff (both call closeWith) and double-write onboarding state / double-fire telemetry. Make closeWith idempotent with a first-wins latch. Also map the displayed step index through resolveStepIndex so a momentarily-skipped resume step can't flash "1 of N". Verified: onboarding unit tests, full onboarding e2e spec (rewritten notifications test locks in the new dismiss behavior), typecheck, lint, and live Electron. --- .../onboarding/OnboardingFlow.test.tsx | 12 +++- .../components/onboarding/OnboardingFlow.tsx | 15 ++--- .../use-onboarding-flow-persistence.ts | 16 +++++- .../onboarding/use-onboarding-flow.ts | 28 ++++------ tests/e2e/onboarding.spec.ts | 55 +++++++++++++------ 5 files changed, 80 insertions(+), 46 deletions(-) diff --git a/src/renderer/src/components/onboarding/OnboardingFlow.test.tsx b/src/renderer/src/components/onboarding/OnboardingFlow.test.tsx index ce8603dbd..5b4f95e1b 100644 --- a/src/renderer/src/components/onboarding/OnboardingFlow.test.tsx +++ b/src/renderer/src/components/onboarding/OnboardingFlow.test.tsx @@ -129,7 +129,7 @@ describe('OnboardingFlow', () => { expect(html).toContain('4 of 5') }) - it('keeps Windows terminal defaults in the fourth progress slot when integrations are skipped', () => { + it('drops the skipped integrations step from the stepper on Windows', () => { vi.stubGlobal('navigator', { userAgent: 'Windows' }) useAppStore.setState({ preflightStatus: { @@ -148,8 +148,12 @@ describe('OnboardingFlow', () => { }) expect(html).toContain('Set Windows terminal defaults') - expect(html).toContain('4 of 5') + // Why: integrations is skipped (gh already installed), so it is not a + // stepper dot at all — the four real steps are agent, theme, Windows + // terminal, notifications, and Windows terminal is the third of four. + expect(html).toContain('3 of 4') expect(html).not.toContain('Set up GitHub tasks') + expect(html).not.toContain('Integrations') }) it('skips GitHub task setup when the GitHub CLI is already detected', () => { @@ -174,6 +178,10 @@ describe('OnboardingFlow', () => { expect(html).not.toContain('Set up GitHub tasks') expect(html).not.toContain('Connect your task sources') expect(html).not.toContain('Point Orca at some code') + // Why: with both integrations (gh installed) and Windows terminal (Mac) + // skipped, the stepper shows only the three real steps — no dead dots. + expect(html).toContain('3 of 3') + expect(html).not.toContain('Integrations') }) it('shows only GitHub on the task setup page when the GitHub CLI is missing', () => { diff --git a/src/renderer/src/components/onboarding/OnboardingFlow.tsx b/src/renderer/src/components/onboarding/OnboardingFlow.tsx index 5d0d9eb10..c5546aa95 100644 --- a/src/renderer/src/components/onboarding/OnboardingFlow.tsx +++ b/src/renderer/src/components/onboarding/OnboardingFlow.tsx @@ -116,22 +116,21 @@ export default function OnboardingFlow({ const shouldShowFooterBusy = Boolean(busyLabel) const footerPrimaryLabel = busyLabel ?? (currentStep.id === 'notifications' ? 'Add your first project' : 'Continue') - const canDismissCurrentStep = currentStep.id !== 'notifications' const [skipConfirmOpen, setSkipConfirmOpen] = useState(false) const skipConfirmAdvancedViaRef = useRef<'button' | 'keyboard'>('button') const { next: flowNext, dismissOnboarding: flowDismissOnboarding } = flow const requestSkipConfirmation = useCallback( (advancedVia: 'button' | 'keyboard') => { - // Why: the final notifications step hands off to Add Project, so all - // dismiss paths are disabled there, not just the visible Skip button. - if (!canDismissCurrentStep || busyLabel || skipConfirmOpen) { + // Why: click-off / Escape dismissal stays available on every step, + // including the final notifications step, so the modal never feels stuck. + if (busyLabel || skipConfirmOpen) { return } skipConfirmAdvancedViaRef.current = advancedVia setSkipConfirmOpen(true) }, - [busyLabel, canDismissCurrentStep, skipConfirmOpen] + [busyLabel, skipConfirmOpen] ) const confirmSkipOnboarding = useCallback(() => { @@ -218,7 +217,7 @@ export default function OnboardingFlow({
- {flow.progressSteps.map(({ step, index: realStepIndex, isSkipped }, progressIdx) => { + {flow.progressSteps.map(({ step, index: realStepIndex }, progressIdx) => { const isActive = realStepIndex === stepIndex const isDone = realStepIndex < stepIndex return ( @@ -234,8 +233,7 @@ export default function OnboardingFlow({ ? 'w-10 bg-foreground' : isDone ? 'w-6 bg-muted-foreground/70 hover:bg-foreground/80' - : 'w-6 bg-muted-foreground/25 hover:bg-muted-foreground/45', - isSkipped && 'cursor-default hover:bg-muted-foreground/25' + : 'w-6 bg-muted-foreground/25 hover:bg-muted-foreground/45' )} aria-label={translate( 'auto.components.onboarding.OnboardingFlow.adaa0aa627', @@ -243,7 +241,6 @@ export default function OnboardingFlow({ { value0: progressIdx + 1, value1: stepTooltipLabels[step.id] } )} aria-current={isActive ? 'step' : undefined} - disabled={isSkipped} onClick={() => flow.jumpToStep(realStepIndex)} /> 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 10c8c6cd6..12fe7d1bf 100644 --- a/src/renderer/src/components/onboarding/use-onboarding-flow-persistence.ts +++ b/src/renderer/src/components/onboarding/use-onboarding-flow-persistence.ts @@ -1,4 +1,4 @@ -import { useCallback } from 'react' +import { useCallback, useRef } from 'react' import { track } from '@/lib/telemetry' import { useAppStore } from '@/store' import { ONBOARDING_FINAL_STEP, ONBOARDING_FLOW_VERSION } from '../../../../shared/constants' @@ -73,6 +73,13 @@ export function useCloseWith({ startTimeRef, setError }: CloseWithDeps) { + // Why: onboarding closes exactly once. On the final notifications step both + // the "Add your first project" handoff (completed) and a click-off/Escape + // dismissal (dismissed) can reach closeWith, and next()'s persist window + // leaves the modal interactive with no busy flag. This latch makes closeWith + // idempotent so the first close wins — no double onboarding.update write and + // no double completed/dismissed telemetry. + const closedRef = useRef(false) return useCallback( async ( outcome: 'completed' | 'dismissed', @@ -81,6 +88,10 @@ export function useCloseWith({ completedPath?: 'open_folder' | 'clone_url' | 'add_project_modal', dismissedExtras?: DismissedExtras ): Promise => { + if (closedRef.current) { + return false + } + closedRef.current = true let nextState: OnboardingState try { // Why: main-process updateOnboarding already merges with current state, @@ -97,6 +108,9 @@ export function useCloseWith({ } }) } catch (err) { + // Why: the persist failed, so onboarding did not actually close — clear + // the latch so the user can retry the close action. + closedRef.current = false setError(err instanceof Error ? err.message : String(err)) return false } diff --git a/src/renderer/src/components/onboarding/use-onboarding-flow.ts b/src/renderer/src/components/onboarding/use-onboarding-flow.ts index 29017f901..2917c9075 100644 --- a/src/renderer/src/components/onboarding/use-onboarding-flow.ts +++ b/src/renderer/src/components/onboarding/use-onboarding-flow.ts @@ -386,29 +386,25 @@ export function useOnboardingFlow( const detectedSet = useMemo(() => new Set(detectedAgentIds ?? []), [detectedAgentIds]) const currentStep = STEPS[stepIndex] - const visibleSteps = useMemo( + // Why: the stepper shows exactly the steps the user will land on. Skipped + // optional steps — Windows terminal off Windows, integrations when the GitHub + // CLI is already installed — are dropped entirely rather than rendered as + // dead, unreachable dots the user silently skips past on Continue. + const progressSteps = useMemo( () => STEPS.map((step, index) => ({ step, index })).filter( ({ index }) => !isSkippedStepIndex(index, skipOptions) ), [skipOptions] ) - const progressSteps = useMemo( - () => - STEPS.map((step, index) => ({ - step, - index, - isSkipped: isSkippedStepIndex(index, skipOptions) - })).filter(({ step }) => step.id !== 'windows_terminal' || !skipWindowsTerminal), - [skipOptions, skipWindowsTerminal] - ) - const visibleStepIndex = Math.max( - 0, - visibleSteps.findIndex(({ index }) => index === stepIndex) - ) + // Why: while resuming, stepIndex can momentarily point at a step that just + // became skipped (preflight resolving to gh-installed) before the auto-skip + // effect advances it. Resolve forward so the count reflects the step we're + // about to land on instead of flashing "1 of N" with no active dot. + const displayedStepIndex = resolveStepIndex(stepIndex, skipOptions, 'forward') const progressStepIndex = Math.max( 0, - progressSteps.findIndex(({ index }) => index === stepIndex) + progressSteps.findIndex(({ index }) => index === displayedStepIndex) ) const hasExistingProject = repos.length > 0 @@ -1291,8 +1287,6 @@ export function useOnboardingFlow( settings, updateSettings, stepIndex, - visibleSteps, - visibleStepIndex, progressSteps, progressStepIndex, currentStep, diff --git a/tests/e2e/onboarding.spec.ts b/tests/e2e/onboarding.spec.ts index 5a93391f2..86ed4b9ce 100644 --- a/tests/e2e/onboarding.spec.ts +++ b/tests/e2e/onboarding.spec.ts @@ -64,6 +64,10 @@ async function expectOnboardingSkipConfirmationClosed(page: Page): Promise await expect(page.getByRole('dialog', { name: /Skip onboarding\?/i })).toHaveCount(0) } +async function expectOnboardingSkipConfirmationOpen(page: Page): Promise { + await expect(page.getByRole('dialog', { name: /Skip onboarding\?/i })).toBeVisible() +} + async function expectOnboardingNotificationSound(page: Page, name: RegExp): Promise { await expect(onboardingNotificationSoundSelect(page)).toContainText(name) } @@ -617,32 +621,49 @@ test.describe('Onboarding flow', () => { .toBe(1) }) - test('final notification step does not offer a skip or dismiss action', async ({ orcaPage }) => { + test('final notification step can be dismissed via Escape or click-off', async ({ orcaPage }) => { await expect(orcaPage.getByRole('heading', { name: /Pick your default agent/i })).toBeVisible({ timeout: 15_000 }) - // Advance through the optional preference step. The final notification step - // finishes onboarding, so no skip/dismiss path should be available there. + // Advance to the final notification step. Its primary button hands off to + // Add Project, so the footer offers no "Skip to project setup" shortcut — + // but click-off and Escape must still open the skip-confirmation dialog like + // every other step, so the modal never feels stuck. await continueOnboarding(orcaPage) await expect(orcaPage.getByRole('heading', { name: /Make it feel like home/i })).toBeVisible() await continueFromThemeToNotifications(orcaPage) + await expect(orcaPage.getByRole('heading', { name: /Set up notifications/i })).toBeVisible() await expect(onboardingFooterButton(orcaPage, SKIP_TO_PROJECT_SETUP_BUTTON)).toHaveCount(0) - await expect(onboardingFooterButton(orcaPage, /Skip all onboarding/i)).toHaveCount(0) - await orcaPage.keyboard.press('Escape') - await expectOnboardingSkipConfirmationClosed(orcaPage) - await expect(orcaPage.getByRole('heading', { name: /Set up notifications/i })).toBeVisible() - await orcaPage.locator('[data-onboarding-overlay]').click({ position: { x: 8, y: 40 } }) - await expectOnboardingSkipConfirmationClosed(orcaPage) - await expect(orcaPage.getByRole('heading', { name: /Set up notifications/i })).toBeVisible() - await continueOnboarding(orcaPage) - await expectAddProjectDialog(orcaPage) - const final = await getOnboardingState(orcaPage) - expect(final.closedAt).not.toBeNull() - expect(final.outcome).toBe('completed') - expect(final.checklist.dismissed).toBe(false) - expect(final.lastCompletedStep).toBe(ONBOARDING_FINAL_STEP) + // Escape opens the confirmation; "No, keep going" returns to the step with + // onboarding still open. + await orcaPage.keyboard.press('Escape') + await expectOnboardingSkipConfirmationOpen(orcaPage) + await orcaPage.getByRole('button', { name: /No, keep going/i }).click() + await expectOnboardingSkipConfirmationClosed(orcaPage) + await expect(orcaPage.getByRole('heading', { name: /Set up notifications/i })).toBeVisible() + expect((await getOnboardingState(orcaPage)).closedAt).toBeNull() + + // Click-off opens the confirmation; Skip dismisses onboarding outright (no + // Add Project handoff — that is the primary button's job). + await orcaPage.locator('[data-onboarding-overlay]').click({ position: { x: 8, y: 40 } }) + await expectOnboardingSkipConfirmationOpen(orcaPage) + await orcaPage.getByRole('button', { name: /^Skip$/ }).click() + + await expect + .poll( + async () => { + const state = await getOnboardingState(orcaPage) + return { + closedAt: state.closedAt === null ? null : 'set', + outcome: state.outcome, + dismissed: state.checklist.dismissed + } + }, + { timeout: 5_000 } + ) + .toEqual({ closedAt: 'set', outcome: 'dismissed', dismissed: true }) }) })