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.
2026-07-09 00:46:03 -07:00 · 2026-07-09 00:46:03 -07:00 · 7efbe10394
parent 946810cfa7
commit 7efbe10394
5 changed files with 80 additions and 46 deletions
--- 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', () => {
--- 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({
            </div>

            <div className="mt-10 flex items-center gap-2 transition-[margin-top] duration-[760ms] ease-[cubic-bezier(0.22,1,0.36,1)] motion-reduce:transition-none">
-              {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)}
                      />
                    </TooltipTrigger>
--- 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<boolean> => {
+      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
      }
--- 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,
--- a/tests/e2e/onboarding.spec.ts
+++ b/tests/e2e/onboarding.spec.ts
@ -64,6 +64,10 @@ async function expectOnboardingSkipConfirmationClosed(page: Page): Promise<void>
  await expect(page.getByRole('dialog', { name: /Skip onboarding\?/i })).toHaveCount(0)
 }

+async function expectOnboardingSkipConfirmationOpen(page: Page): Promise<void> {
+  await expect(page.getByRole('dialog', { name: /Skip onboarding\?/i })).toBeVisible()
+}
+
 async function expectOnboardingNotificationSound(page: Page, name: RegExp): Promise<void> {
  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 })
  })
 })