From de41bf8ec71250c2faffe6d00f5a813d047108a3 Mon Sep 17 00:00:00 2001 From: Cyrus Gray <144336577+graycyrus@users.noreply.github.com> Date: Fri, 3 Apr 2026 11:38:39 +0530 Subject: [PATCH] fix(onboarding): resolve overlay race condition between RPC and Redux state (#284) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit OnboardingOverlay could reappear for already-onboarded users when the core config RPC call failed (sidecar not ready, timeout). The catch block hardcoded `false`, ignoring the persisted Redux `isOnboardedByUser` flag. Now reads `selectIsOnboarded` as a fallback in the catch block and combines both flags in shouldShow — either being true prevents the overlay. Also fixes DEV_FORCE_ONBOARDING which was a no-op (identical ternary branches). Closes #197 --- .claude/memory.md | 3 ++ app/src/components/OnboardingOverlay.tsx | 8 ++-- .../__tests__/OnboardingOverlay.test.tsx | 45 +++++++++++++++++++ 3 files changed, 53 insertions(+), 3 deletions(-) diff --git a/.claude/memory.md b/.claude/memory.md index 3d035ff3f..2fffb186c 100644 --- a/.claude/memory.md +++ b/.claude/memory.md @@ -50,6 +50,9 @@ Quick reference for anyone starting with Claude on this project. Updated by the - **Recovery Phrase moved to Settings** — MnemonicStep was removed from onboarding (was step 5). The same BIP39 generate/import functionality now lives in `app/src/components/settings/panels/RecoveryPhrasePanel.tsx`, accessible via Settings > Recovery Phrase. Onboarding completion logic moved into `handleSkillsNext` in `Onboarding.tsx`. - **E2E tests find onboarding buttons by label text** — `shared-flows.ts`, `login-flow.spec.ts`, `auth-access-control.spec.ts`, and `voice-mode.spec.ts` locate buttons by their visible label. Changing button labels requires updating all four files. Note: `voice-mode.spec.ts` still references legacy labels that don't match current steps (pre-existing tech debt). - **`ScreenPermissionsStep` always shows Continue** — The Continue button is always visible regardless of permission grant status, allowing users to skip the permissions step (#274). +- **OnboardingOverlay RPC/Redux race condition** — `getOnboardingCompleted()` RPC can fail (sidecar not ready, timeout); the old catch block hardcoded `setOnboardingCompleted(false)`, ignoring the persisted `isOnboardedByUser` Redux flag. Fix: read `selectIsOnboarded` from `authSelectors.ts` in the catch block as fallback, and combine both flags in `shouldShow`: `!onboardingCompleted && !isOnboardedRedux`. Either flag being `true` is sufficient to skip onboarding (#197). +- **`DEV_FORCE_ONBOARDING` was a no-op** — The old ternary had identical branches; fixed to actually force-show when the flag is set. +- **`isOnboardedRedux` must be in useEffect deps** — When reading a selector value inside a useEffect, add it to the dependency array or the effect won't re-run when Redux state changes. ## Build Blockers: macOS Tahoe + whisper-rs diff --git a/app/src/components/OnboardingOverlay.tsx b/app/src/components/OnboardingOverlay.tsx index 4f5c20795..ff928b8ad 100644 --- a/app/src/components/OnboardingOverlay.tsx +++ b/app/src/components/OnboardingOverlay.tsx @@ -2,6 +2,7 @@ import { useCallback, useEffect, useState } from 'react'; import { createPortal } from 'react-dom'; import Onboarding from '../pages/onboarding/Onboarding'; +import { selectIsOnboarded } from '../store/authSelectors'; import { useAppSelector } from '../store/hooks'; import { DEV_FORCE_ONBOARDING } from '../utils/config'; import { @@ -19,6 +20,7 @@ const OnboardingOverlay = () => { const token = useAppSelector(state => state.auth.token); const isAuthBootstrapComplete = useAppSelector(state => state.auth.isAuthBootstrapComplete); const user = useAppSelector(state => state.user.user); + const isOnboardedRedux = useAppSelector(selectIsOnboarded); /** null = still loading, true/false = resolved from core config */ const [onboardingCompleted, setOnboardingCompleted] = useState(null); @@ -54,14 +56,14 @@ const OnboardingOverlay = () => { const completed = await getOnboardingCompleted(); if (mounted) setOnboardingCompleted(completed); } catch { - if (mounted) setOnboardingCompleted(false); + if (mounted) setOnboardingCompleted(isOnboardedRedux); } }; void check(); return () => { mounted = false; }; - }, [token, isAuthBootstrapComplete, userReady]); + }, [token, isAuthBootstrapComplete, userReady, isOnboardedRedux]); const handleDone = useCallback(async () => { setOnboardingCompleted(true); @@ -78,7 +80,7 @@ const OnboardingOverlay = () => { // Still loading the flag from core if (onboardingCompleted === null) return null; - const shouldShow = DEV_FORCE_ONBOARDING ? !onboardingCompleted : !onboardingCompleted; + const shouldShow = DEV_FORCE_ONBOARDING || (!onboardingCompleted && !isOnboardedRedux); if (!shouldShow) return null; diff --git a/app/src/components/__tests__/OnboardingOverlay.test.tsx b/app/src/components/__tests__/OnboardingOverlay.test.tsx index f81b45927..bc66680ba 100644 --- a/app/src/components/__tests__/OnboardingOverlay.test.tsx +++ b/app/src/components/__tests__/OnboardingOverlay.test.tsx @@ -89,4 +89,49 @@ describe('OnboardingOverlay', () => { expect(screen.queryByText('Skip')).not.toBeInTheDocument(); }); + + it('does not render when RPC fails but Redux says onboarded', async () => { + const { getOnboardingCompleted } = await import('../../utils/tauriCommands'); + vi.mocked(getOnboardingCompleted).mockRejectedValue(new Error('RPC error')); + + renderWithProviders(, { + preloadedState: { + auth: { ...baseAuth, isOnboardedByUser: { 'user-1': true } }, + user: baseUser, + }, + }); + + await vi.waitFor(() => { + expect(screen.queryByText('Skip')).not.toBeInTheDocument(); + }); + }); + + it('does not render when RPC returns false but Redux says onboarded', async () => { + const { getOnboardingCompleted } = await import('../../utils/tauriCommands'); + vi.mocked(getOnboardingCompleted).mockResolvedValue(false); + + renderWithProviders(, { + preloadedState: { + auth: { ...baseAuth, isOnboardedByUser: { 'user-1': true } }, + user: baseUser, + }, + }); + + await vi.waitFor(() => { + expect(screen.queryByText('Skip')).not.toBeInTheDocument(); + }); + }); + + it('renders when both RPC fails and Redux says not onboarded', async () => { + const { getOnboardingCompleted } = await import('../../utils/tauriCommands'); + vi.mocked(getOnboardingCompleted).mockRejectedValue(new Error('RPC error')); + + renderWithProviders(, { + preloadedState: { auth: { ...baseAuth, isOnboardedByUser: {} }, user: baseUser }, + }); + + await vi.waitFor(() => { + expect(screen.queryByText('Skip')).toBeInTheDocument(); + }); + }); });