diff --git a/apps/web/src/hooks/__tests__/useAuth.test.ts b/apps/web/src/hooks/__tests__/useAuth.test.ts index d2e882d2ff..fb7560a087 100644 --- a/apps/web/src/hooks/__tests__/useAuth.test.ts +++ b/apps/web/src/hooks/__tests__/useAuth.test.ts @@ -25,6 +25,7 @@ type MockAuthStoreState = { isRefreshing: boolean; hasHydrated: boolean; authFailedPermanently: boolean; + _authPromise: Promise | null; setUser: ReturnType void>>; setLoading: ReturnType void>>; setHydrated: ReturnType void>>; @@ -46,6 +47,7 @@ const { mockAuthStore, mockLoadSession, mockGetSessionDuration, + mockShouldLoadSession, mockInitializeEventListeners, } = vi.hoisted(() => { const store: MockAuthStoreState = { @@ -55,6 +57,7 @@ const { isRefreshing: false, hasHydrated: true, authFailedPermanently: false, + _authPromise: null, // Simulate actual state transitions setUser: vi.fn<(user: AuthUser | null) => void>((user) => { store.user = user; @@ -88,6 +91,7 @@ const { mockAuthStore: store, mockLoadSession: vi.fn(), mockGetSessionDuration: vi.fn(() => 0), + mockShouldLoadSession: vi.fn(() => false), mockInitializeEventListeners: vi.fn(), }; }); @@ -136,6 +140,7 @@ vi.mock('@/stores/useAuthStore', () => { authStoreHelpers: { loadSession: mockLoadSession, getSessionDuration: mockGetSessionDuration, + shouldLoadSession: mockShouldLoadSession, initializeEventListeners: mockInitializeEventListeners, }, }; @@ -177,6 +182,8 @@ describe('useAuth', () => { mockAuthStore.isRefreshing = false; mockAuthStore.hasHydrated = true; mockAuthStore.authFailedPermanently = false; + mockAuthStore._authPromise = null; + mockShouldLoadSession.mockReturnValue(false); global.fetch = vi.fn(); }); @@ -427,12 +434,10 @@ describe('useAuth', () => { it('given already loading, should skip redundant auth check', async () => { mockAuthStore.isLoading = true; + mockLoadSession.mockClear(); const { result } = renderHook(() => useAuth()); - // Clear calls from the initial mount effect (loadSession always runs on mount) - mockLoadSession.mockClear(); - await act(async () => { await result.current.actions.checkAuth(); }); @@ -530,5 +535,23 @@ describe('useAuth', () => { // Observable: hydration state updated expect(mockAuthStore.setHydrated).toHaveBeenCalledWith(true); }); + + it('given no session load needed and no in-flight auth, should clear loading', () => { + mockShouldLoadSession.mockReturnValue(false); + mockAuthStore._authPromise = null; + + renderHook(() => useAuth()); + + expect(mockAuthStore.setLoading).toHaveBeenCalledWith(false); + }); + + it('given shared auth promise in flight, should not clear loading', () => { + mockShouldLoadSession.mockReturnValue(false); + mockAuthStore._authPromise = Promise.resolve(); + + renderHook(() => useAuth()); + + expect(mockAuthStore.setLoading).not.toHaveBeenCalledWith(false); + }); }); }); diff --git a/apps/web/src/hooks/__tests__/usePageTree.test.ts b/apps/web/src/hooks/__tests__/usePageTree.test.ts index 47d2004101..183347146c 100644 --- a/apps/web/src/hooks/__tests__/usePageTree.test.ts +++ b/apps/web/src/hooks/__tests__/usePageTree.test.ts @@ -200,7 +200,7 @@ describe('usePageTree', () => { expect(updatedTree).toBe(mockTree); // Same reference, unchanged }); - it('given no data loaded yet, should return undefined without error', () => { + it('given no data loaded yet, should no-op without mutating cache', () => { mockSWRState.data = undefined; const { result } = renderHook(() => usePageTree('drive-123')); @@ -211,10 +211,19 @@ describe('usePageTree', () => { }); }).not.toThrow(); - // mutate is called, and updater returns undefined when no data - expect(mockMutate).toHaveBeenCalledWith(expect.any(Function), { revalidate: false }); - const updaterFn = mockMutate.mock.calls[0][0]; - expect(updaterFn(undefined)).toBeUndefined(); + // No data means no optimistic mutation attempt. + expect(mockMutate).not.toHaveBeenCalled(); + }); + + it('given tree data changes, should keep updateNode callback stable', () => { + mockSWRState.data = [createMockTreePage({ id: 'page-1' })]; + const { result, rerender } = renderHook(() => usePageTree('drive-123')); + const firstUpdateNode = result.current.updateNode; + + mockSWRState.data = [createMockTreePage({ id: 'page-2' })]; + rerender(); + + expect(result.current.updateNode).toBe(firstUpdateNode); }); }); diff --git a/apps/web/src/hooks/useAuth.ts b/apps/web/src/hooks/useAuth.ts index c8be00ea53..0fbab614db 100644 --- a/apps/web/src/hooks/useAuth.ts +++ b/apps/web/src/hooks/useAuth.ts @@ -326,29 +326,46 @@ export function useAuth(): { } }, []); - // Initial auth check - always run loadSession() on cold start - // This ensures warmSessionCache() runs before CenterPanel mounts, - // preventing SWR stuck detection on Electron/Capacitor cold starts. - // loadSession() has internal deduplication, circuit breaker, and permanent failure guards. + // Initial auth check - simplified with store-level deduplication + // Desktop OAuth now uses secure exchange codes handled in Electron main process useEffect(() => { // Wait for hydration if (!hasHydrated) return; - const loadAndCleanup = async () => { - try { - await authStoreHelpers.loadSession(isOAuthSuccess); // Force reload for OAuth success - } finally { - // Clean up OAuth success parameter from URL after session loads - if (isOAuthSuccess && typeof window !== 'undefined') { - console.log('[AUTH_HOOK] Cleaning up OAuth success parameter from URL'); - const newUrl = new URL(window.location.href); - newUrl.searchParams.delete('auth'); - window.history.replaceState({}, '', newUrl.toString()); - setIsOAuthSuccess(false); // Clear the flag to exit loading state + // Use store helper to determine if session load is needed + const shouldLoad = authStoreHelpers.shouldLoadSession() || isOAuthSuccess; + + if (shouldLoad) { + console.log(`[AUTH_HOOK] Loading session - hasHydrated: ${hasHydrated}, isOAuthSuccess: ${isOAuthSuccess}`); + + // Await loadSession to ensure isLoading is set before clearing OAuth flag + const loadAndCleanup = async () => { + try { + await authStoreHelpers.loadSession(isOAuthSuccess); // Force reload for OAuth success + } catch (error) { + console.error('[AUTH_HOOK] Failed to load session during initial auth check:', error); + } finally { + // Clean up OAuth success parameter from URL after session loads + if (isOAuthSuccess && typeof window !== 'undefined') { + console.log('[AUTH_HOOK] Cleaning up OAuth success parameter from URL'); + const newUrl = new URL(window.location.href); + newUrl.searchParams.delete('auth'); + window.history.replaceState({}, '', newUrl.toString()); + setIsOAuthSuccess(false); // Clear the flag to exit loading state + } } - } - }; - loadAndCleanup(); + }; + void loadAndCleanup(); + } else { + // Multiple components can mount useAuth simultaneously. + // If another instance already started loadSession(), keep loading true + // until that shared auth promise settles. + const hasInFlightSessionLoad = !!useAuthStore.getState()._authPromise; + if (hasInFlightSessionLoad) return; + + // Session check not needed (e.g., lastAuthCheck is recent) — unblock the UI + useAuthStore.getState().setLoading(false); + } }, [hasHydrated, isOAuthSuccess]); // Initialize auth event listeners once (moved to store level for deduplication) diff --git a/apps/web/src/hooks/usePageTree.ts b/apps/web/src/hooks/usePageTree.ts index 9368ddb144..9bd58f7671 100644 --- a/apps/web/src/hooks/usePageTree.ts +++ b/apps/web/src/hooks/usePageTree.ts @@ -78,6 +78,8 @@ export function usePageTree(driveId?: string, trashView?: boolean) { } ); const { cache } = useSWRConfig(); + const hasTreeSnapshotRef = useRef(!!data); + hasTreeSnapshotRef.current = !!data; // Self-healing: detect when SWR gets stuck (valid key, no data, no error, not fetching). // On desktop/Capacitor, async token retrieval in fetchWithAuth can cause SWR to lose track @@ -137,6 +139,11 @@ export function usePageTree(driveId?: string, trashView?: boolean) { }, [swrKey, cache, mutate]); const updateNode = useCallback((nodeId: string, updates: Partial) => { + // Avoid optimistic mutation before the first tree snapshot exists. + // A no-op mutate on undefined data can stamp SWR mutation state and drop + // the in-flight initial fetch result, leaving the tree in a stuck skeleton state. + if (!hasTreeSnapshotRef.current) return; + mutate((currentData) => { if (!currentData) return currentData; diff --git a/apps/web/src/stores/__tests__/useAuthStore.test.ts b/apps/web/src/stores/__tests__/useAuthStore.test.ts index 0c5189e0a7..2e9d6a0ffb 100644 --- a/apps/web/src/stores/__tests__/useAuthStore.test.ts +++ b/apps/web/src/stores/__tests__/useAuthStore.test.ts @@ -511,6 +511,45 @@ describe('useAuthStore', () => { expect(global.fetch).toHaveBeenCalled(); }); + it('given overlapping force loads, should keep latest auth promise active', async () => { + const user = createMockUser(); + let resolveFirstFetch!: (value: Response) => void; + let resolveSecondFetch!: (value: Response) => void; + const firstFetch = new Promise((resolve) => { + resolveFirstFetch = resolve; + }); + const secondFetch = new Promise((resolve) => { + resolveSecondFetch = resolve; + }); + + vi.mocked(global.fetch) + .mockImplementationOnce(() => firstFetch) + .mockImplementationOnce(() => secondFetch); + + const firstLoad = useAuthStore.getState().loadSession(true); + const secondLoad = useAuthStore.getState().loadSession(true); + const activePromise = useAuthStore.getState()._authPromise; + + // Latest request should own dedupe slot. + expect(activePromise).not.toBeNull(); + + resolveFirstFetch({ + ok: true, + json: () => Promise.resolve(user), + } as Response); + await firstLoad; + + // Completing first request must not clear second in-flight promise. + expect(useAuthStore.getState()._authPromise).toBe(activePromise); + + resolveSecondFetch({ + ok: true, + json: () => Promise.resolve(user), + } as Response); + await secondLoad; + expect(useAuthStore.getState()._authPromise).toBeNull(); + }); + it('given network error, should record failed attempt', async () => { vi.mocked(global.fetch).mockRejectedValue(new Error('Network error')); const consoleError = vi.spyOn(console, 'error').mockImplementation(() => {}); @@ -814,6 +853,63 @@ describe('authStoreHelpers', () => { }); }); + describe('shouldLoadSession', () => { + it('given not hydrated, should return true', () => { + useAuthStore.setState({ hasHydrated: false }); + + expect(authStoreHelpers.shouldLoadSession()).toBe(true); + }); + + it('given no server initialization and recent auth check, should return true', () => { + useAuthStore.setState({ + hasHydrated: true, + _serverSessionInitialized: false, + lastAuthCheck: Date.now(), + }); + + expect(authStoreHelpers.shouldLoadSession()).toBe(true); + }); + + it('given circuit breaker active, should return false', () => { + useAuthStore.setState({ + hasHydrated: true, + failedAuthAttempts: 5, // MAX_FAILED_AUTH_ATTEMPTS (web threshold) + lastFailedAuthCheck: Date.now(), + }); + + expect(authStoreHelpers.shouldLoadSession()).toBe(false); + }); + + it('given already loading, should return false', () => { + useAuthStore.setState({ + hasHydrated: true, + _authPromise: Promise.resolve(), + }); + + expect(authStoreHelpers.shouldLoadSession()).toBe(false); + }); + + it('given server initialized and stale check, should return true', () => { + useAuthStore.setState({ + hasHydrated: true, + _serverSessionInitialized: true, + lastAuthCheck: Date.now() - 16 * 60 * 1000, // Stale + }); + + expect(authStoreHelpers.shouldLoadSession()).toBe(true); + }); + + it('given server initialized and recent check, should return false', () => { + useAuthStore.setState({ + hasHydrated: true, + _serverSessionInitialized: true, + lastAuthCheck: Date.now(), + }); + + expect(authStoreHelpers.shouldLoadSession()).toBe(false); + }); + }); + describe('trackActivity', () => { it('should call updateActivity on the store', () => { useAuthStore.setState({ diff --git a/apps/web/src/stores/useAuthStore.ts b/apps/web/src/stores/useAuthStore.ts index 85bba97878..64c7635d23 100644 --- a/apps/web/src/stores/useAuthStore.ts +++ b/apps/web/src/stores/useAuthStore.ts @@ -296,11 +296,8 @@ export const useAuthStore = create()( return; } - // Set loading state to prevent premature redirects (critical for OAuth flow) - set({ isLoading: true }); - - // Create new auth promise - const authPromise = (async () => { + // Create and publish promise before async work starts to avoid races. + const authPromise = Promise.resolve().then(async () => { try { const isDesktop = typeof window !== 'undefined' && window.electron?.isDesktop; const headers: Record = {}; @@ -536,13 +533,19 @@ export const useAuthStore = create()( lastFailedAuthCheck: Date.now(), }); } finally { - // Clear loading state and promise when done - set({ isLoading: false, _authPromise: null }); + // Clear loading state. Only clear _authPromise if this request is still current. + set((currentState) => { + if (currentState._authPromise !== authPromise) { + return { isLoading: false }; + } + + return { isLoading: false, _authPromise: null }; + }); } - })(); + }); - // Store promise for deduplication - set({ _authPromise: authPromise }); + // Set loading and store promise for deduplication. + set({ isLoading: true, _authPromise: authPromise }); return authPromise; }, }), @@ -628,6 +631,35 @@ export const authStoreHelpers = { return state.failedAuthAttempts >= maxAttempts; }, + // Check if auth check is needed (considering server initialization) + shouldLoadSession: (): boolean => { + const state = useAuthStore.getState(); + + // Skip if circuit breaker is active + if (authStoreHelpers.shouldSkipAuthCheck()) { + return false; + } + + // Skip if already loading + if (state._authPromise) { + return false; + } + + // Always load if not hydrated yet + if (!state.hasHydrated) { + return true; + } + + // If server session was not initialized this app boot, force one revalidation. + // Persisted auth state (user/isAuthenticated/lastAuthCheck) can be stale. + if (!state._serverSessionInitialized) { + return true; + } + + // Server session initialized on this boot, so normal staleness policy applies. + return authStoreHelpers.needsAuthCheck(); + }, + // Initialize store from server session (called during app startup) initializeFromServer: (initialUser: User | null): void => { useAuthStore.getState().initializeFromServer(initialUser);