diff --git a/CLAUDE.md b/CLAUDE.md index 4a56fa46ceab..a9a8e7a3cab1 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -17,6 +17,7 @@ - After editing `protocol/avdl/` or `protocol/bin/enabled-calls.json`: from `protocol/`, run `node ./bin/generate-ts.ts && cp ./js/rpc*.tsx ../shared/constants/rpc` and commit the regenerated `shared/constants/rpc/rpc-gen.tsx`. - Never hand-edit generated code (rpc-gen, protocol output, mocks, codegen'd files of any kind). Edit the source it's generated from and rerun the generator. CI regenerates and fails on any diff. - When updating `electron`: run `shared/desktop/extract-electron-shasums.sh `. +- Keep an open PR's description in step with its branch. Whenever new commits change what the PR does or how (a new fix, a changed approach, a removed piece, new tests or evidence), rewrite the affected sections with `gh pr edit --body-file`, and the title if the scope moved. It should read as a description of the current diff, not a changelog. Skip it for commits that don't change the story (lint, renames, test placeholders). - Never patch `react-native` itself (patch-package or node_modules edits): we use prebuilt RN core and don't compile its source, so native-side patches never take effect. Work around RN core bugs in app code. ## Working Directory @@ -29,4 +30,4 @@ Repo root is `client/`. TS source lives in `shared/`. Always use absolute paths ## Validation After TS changes (from `shared/`): `yarn lint:all` (= `yarn lint` && `yarn lint:bailouts` && `yarn tsc`). Plain `yarn lint` is eslint only and does NOT catch react-compiler bailouts — no compiler rule is wired into `eslint.config.mjs`, so bailouts only surface via `lint:bailouts`. `lint:bailouts` also flags components the compiler cannot name (an `isMobile ? arrow : arrow` ternary is never compiled at all, so nothing in it is memoized — name both branches instead), and memo scopes keyed on the whole props object (a `props.x` read inside a callback, or a destructure below one, makes the compiler key on `props` itself, so the cache never hits — read every prop through one destructure at the top, above every callback). Repo baseline is 0 bailouts and 0 whole-props deps; keep it there. When debugging visually, skip until fix is confirmed. Never delete the ESLint cache. -Before reporting any TS change complete: run `yarn lint:all` and get it clean. Do NOT run `/code-review` while iterating, building, testing, or debugging — only once the change is about to be pushed (commit for a PR, push, or open a PR). At that point, if the diff has real logic in it, run `/code-review high` against the diff and fix what it finds; if a finding is wrong, say why instead of applying it. Skip the review for trivial diffs (a config/JSON line, a codegen resync, a typo) and say you skipped it. +Before reporting any TS change complete: run `yarn lint:all` and get it clean. Do NOT run `/code-review` while iterating, building, testing, or debugging — only once the change is about to be pushed (commit for a PR, push, or open a PR). At that point, first get both `yarn lint:all` and `yarn test:unit` passing — never review, push, or open a PR with either failing. Then, if the diff has real logic in it, run `/code-review high` against the diff and fix what it finds; if a finding is wrong, say why instead of applying it. Skip the review for trivial diffs (a config/JSON line, a codegen resync, a typo) and say you skipped it. diff --git a/shared/chat/conversation/input-area/input-state.test.tsx b/shared/chat/conversation/input-area/input-state.test.tsx index f61618eadeca..577575506065 100644 --- a/shared/chat/conversation/input-area/input-state.test.tsx +++ b/shared/chat/conversation/input-area/input-state.test.tsx @@ -962,3 +962,85 @@ test('a commandStatus written while the provider is frozen is applied on thaw', expect(inputState?.commandStatus).toEqual(commandStatusInfo) }) + +describe('a pending draft save', () => { + const typeThenWait = (switchAccount: boolean) => { + jest.useFakeTimers() + try { + const saveDraft = jest.spyOn(T.RPCChat, 'localUpdateUnsentTextRpcPromise').mockResolvedValue(undefined) + jest.spyOn(T.RPCChat, 'localUpdateTypingRpcPromise').mockResolvedValue(undefined) + renderComposer() + act(() => { + mockPlatformInputProps?.onChangeText('a') + }) + // inside the 200ms throttle, so this save waits for its trailing edge + act(() => { + mockPlatformInputProps?.onChangeText('ab') + }) + if (switchAccount) { + act(() => { + useCurrentUserState.getState().dispatch.setBootstrap({ + deviceID: 'device-id-2', + deviceName: 'test-device-2', + uid: 'uid-2', + username: 'testuser-mac', + }) + }) + } + act(() => { + jest.advanceTimersByTime(250) + }) + return saveDraft.mock.calls.map(c => c[0].text) + } finally { + jest.useRealTimers() + } + } + + test('is saved for the account that typed it', () => { + expect(typeThenWait(false)).toContain('ab') + }) + + test('is not saved for the next account when a switch lands first', () => { + expect(typeThenWait(true)).not.toContain('ab') + }) +}) + +describe('a draft typed just before leaving the conversation', () => { + const typeThenUnmount = (switchAccount: boolean) => { + jest.useFakeTimers() + try { + const saveDraft = jest.spyOn(T.RPCChat, 'localUpdateUnsentTextRpcPromise').mockResolvedValue(undefined) + jest.spyOn(T.RPCChat, 'localUpdateTypingRpcPromise').mockResolvedValue(undefined) + const {unmount} = renderComposer() + act(() => { + mockPlatformInputProps?.onChangeText('a') + }) + // inside the 200ms throttle, so this save is still pending at unmount + act(() => { + mockPlatformInputProps?.onChangeText('ab') + }) + if (switchAccount) { + act(() => { + useCurrentUserState.getState().dispatch.setBootstrap({ + deviceID: 'device-id-2', + deviceName: 'test-device-2', + uid: 'uid-2', + username: 'testuser-mac', + }) + }) + } + unmount() + return saveDraft.mock.calls.map(c => c[0].text) + } finally { + jest.useRealTimers() + } + } + + test('is saved when the composer unmounts', () => { + expect(typeThenUnmount(false)).toContain('ab') + }) + + test('is not saved for the next account when the unmount comes from a switch', () => { + expect(typeThenUnmount(true)).not.toContain('ab') + }) +}) diff --git a/shared/chat/conversation/input-area/normal/index.tsx b/shared/chat/conversation/input-area/normal/index.tsx index 71802d18ce9f..e59eaa69eaf0 100644 --- a/shared/chat/conversation/input-area/normal/index.tsx +++ b/shared/chat/conversation/input-area/normal/index.tsx @@ -241,7 +241,13 @@ const ConnectedPlatformInput = function ConnectedPlatformInput() { // throttled draft-save path rather than from onChangeText, so the composer does not // re-render on every keystroke. The preview debounces another 500ms downstream anyway. const [previewText, setPreviewText] = React.useState('') + // The account this composer was mounted for. After an account switch the service saves drafts + // for the next account, so the unmount flush of a draft typed here must not save it there. + const [composerUid] = React.useState(() => useCurrentUserState.getState().uid) const updateDraftRaw = (text: string) => { + if (useCurrentUserState.getState().uid !== composerUid) { + return + } // Immediately update local meta.draft so switching back to this thread // before the async unbox completes won't re-inject the old stale draft. // Merges from the current meta (same inbox version), so force past gating. @@ -259,13 +265,8 @@ const ConnectedPlatformInput = function ConnectedPlatformInput() { } C.ignorePromise(f()) } - const updateDraft = C.useThrottledCallback(updateDraftRaw, 200, {trailing: true}) - // Flush any pending draft save before cancel fires on unmount (hooks cleanup runs in reverse order) - React.useLayoutEffect(() => { - return () => { - updateDraft.flush() - } - }, [updateDraft]) + // flushOnUnmount: leaving the conversation must still save what was typed in the last 200ms + const updateDraft = C.useThrottledCallback(updateDraftRaw, 200, {flushOnUnmount: true, trailing: true}) const textValueRef = React.useRef('') const onChangeText = (text: string) => { diff --git a/shared/chat/conversation/thread-context.test.tsx b/shared/chat/conversation/thread-context.test.tsx index abcf188d74e0..27f079411140 100644 --- a/shared/chat/conversation/thread-context.test.tsx +++ b/shared/chat/conversation/thread-context.test.tsx @@ -246,6 +246,7 @@ const separatePlainThreadWrapper = ({children}: {children: React.ReactNode}) => ) beforeEach(() => { + useConfigState.setState({loggedIn: true}) useCurrentUserState.getState().dispatch.setBootstrap({ deviceID: 'device-id', deviceName: 'test-device', @@ -844,6 +845,48 @@ test('active change marks read after an eligible mounted thread load', async () }) }) +test('a thread still on screen after an account switch does not mark read for the next account', async () => { + useConfigState.setState({loggedIn: true}) + useShellState.getState().dispatch.setActive(false) + jest + .spyOn(Common, 'isUserActivelyLookingAtThisThread') + .mockImplementation(() => useShellState.getState().active) + const markAsRead = jest + .spyOn(T.RPCChat, 'localMarkAsReadLocalRpcPromise') + .mockResolvedValue({offline: false}) + jest.spyOn(T.RPCChat, 'localGetThreadNonblockRpcListener').mockImplementation(async p => { + p.incomingCallMap['chat.1.chatUi.chatThreadFull']?.({ + thread: JSON.stringify({ + messages: [makeValidTextUIMessage(T.Chat.numberToMessageID(603), 'loaded inactive')], + pagination: {last: true, next: '', num: 100, previous: ''}, + }), + }) + await Promise.resolve() + return {offline: false} + }) + const {result} = renderHook(() => useConversationThreadLoadMoreMessages(), {wrapper}) + act(() => { + result.current({reason: 'tab selected'}) + }) + await act(async () => { + await flushPromises() + }) + + act(() => { + useCurrentUserState.getState().dispatch.setBootstrap({ + deviceID: 'device-id-2', + deviceName: 'test-device-2', + uid: 'uid-2', + username: 'testuser-mac', + }) + useShellState.getState().dispatch.setActive(true) + }) + await act(async () => { + await flushPromises() + }) + expect(markAsRead).not.toHaveBeenCalled() +}) + test('active change does not mark read after a centered thread load', async () => { useConfigState.setState({loggedIn: true}) useShellState.getState().dispatch.setActive(false) diff --git a/shared/chat/conversation/thread-context.tsx b/shared/chat/conversation/thread-context.tsx index 0a7abbe52b08..6b766b867545 100644 --- a/shared/chat/conversation/thread-context.tsx +++ b/shared/chat/conversation/thread-context.tsx @@ -403,6 +403,9 @@ const ConversationThreadProviderInner = (p: ConversationThreadProviderProps) => const lookingAtThread = active && appFocused && routeFocused const previousLookingAtThreadRef = React.useRef(lookingAtThread) const activeMarkReadEnabledRef = React.useRef(false) + // The account this thread was loaded for. Its screen outlives an account switch by a few renders, + // and a mark-read sent then would mark the next account's read position. + const [threadUid] = React.useState(() => useCurrentUserState.getState().uid) const markReadBlockedRef = React.useRef(false) const getSnapshot = React.useEffectEvent(() => threadStore.getState()) @@ -422,6 +425,10 @@ const ConversationThreadProviderInner = (p: ConversationThreadProviderProps) => logger.info('mark read bail on not logged in') return } + if (useCurrentUserState.getState().uid !== threadUid) { + logger.info('mark read bail on thread loaded for another account') + return + } if (!T.Chat.isValidConversationIDKey(id)) { logger.info('mark read bail on no selected conversation') return diff --git a/shared/chat/conversation/thread-load-status-context.test.tsx b/shared/chat/conversation/thread-load-status-context.test.tsx index 797ac75ffebc..a0c74f73dd4e 100644 --- a/shared/chat/conversation/thread-load-status-context.test.tsx +++ b/shared/chat/conversation/thread-load-status-context.test.tsx @@ -5,6 +5,7 @@ import type * as React from 'react' import * as T from '@/constants/types' import {notifyEngineActionListeners} from '@/engine/action-listener' import {resetAllStores} from '@/util/zustand' +import {useConfigState} from '@/stores/config' import {useCurrentUserState} from '@/stores/current-user' import { ConversationThreadLoadStatusProvider, @@ -25,6 +26,7 @@ const flushPromises = async () => { beforeEach(() => { jest.spyOn(T.RPCChat, 'localRequestInboxUnboxRpcPromise').mockResolvedValue(undefined) + useConfigState.setState({loggedIn: true}) useCurrentUserState.getState().dispatch.setBootstrap({ deviceID: 'device-id', deviceName: 'test-device', diff --git a/shared/chat/inbox/engine.test.tsx b/shared/chat/inbox/engine.test.tsx index 60ed618248fc..28879d27fc71 100644 --- a/shared/chat/inbox/engine.test.tsx +++ b/shared/chat/inbox/engine.test.tsx @@ -4,6 +4,7 @@ import {resetAllStores} from '@/util/zustand' import {handleConvoEngineIncoming} from './engine' import {getInboxConversationMeta, getInboxConversationParticipants} from './metadata' import {useConfigState} from '@/stores/config' +import {useCurrentUserState} from '@/stores/current-user' import {updateInboxTyping} from '@/chat/inbox/typing-state' jest.mock('@/chat/inbox/badge-state', () => ({ @@ -260,6 +261,12 @@ test('global message activity routing preserves returned global data', () => { test('read message activity without attached inbox item refreshes service-owned metadata', () => { useConfigState.setState({loggedIn: true}) + useCurrentUserState.getState().dispatch.setBootstrap({ + deviceID: 'device-id', + deviceName: 'test-device', + uid: 'uid', + username: 'alice', + }) const unbox = jest.spyOn(T.RPCChat, 'localRequestInboxUnboxRpcPromise').mockResolvedValue(undefined) expect( diff --git a/shared/chat/inbox/metadata.test.tsx b/shared/chat/inbox/metadata.test.tsx index 69af67da1ea4..9bca7c03915a 100644 --- a/shared/chat/inbox/metadata.test.tsx +++ b/shared/chat/inbox/metadata.test.tsx @@ -330,7 +330,7 @@ test('setUserSwitching abandons further unbox until switch completes', async () await flushPromises() expect(T.RPCChat.localRequestInboxUnboxRpcPromise).toHaveBeenCalledTimes(1) - useConfigState.getState().dispatch.setUserSwitching(true) + useConfigState.getState().dispatch.setUserSwitching(true, 'testuser') resolvers[0]?.() await flushPromises() diff --git a/shared/chat/inbox/use-inbox-state.test.ts b/shared/chat/inbox/use-inbox-state.test.ts index 75fb6bf366fa..da6315adb600 100644 --- a/shared/chat/inbox/use-inbox-state.test.ts +++ b/shared/chat/inbox/use-inbox-state.test.ts @@ -12,6 +12,7 @@ type MockInboxLayoutState = { } let mockInboxLayoutState: MockInboxLayoutState +let mockConfigState = {loggedIn: true, userSwitching: false} jest.mock('@/constants', () => { const React = require('react') @@ -60,7 +61,7 @@ jest.mock('./metadata', () => ({ })) jest.mock('@/stores/config', () => ({ - useConfigState: (selector: (state: {loggedIn: boolean}) => T) => selector({loggedIn: true}), + useConfigState: (selector: (state: typeof mockConfigState) => T) => selector(mockConfigState), })) jest.mock('@/stores/current-user', () => ({ @@ -85,6 +86,7 @@ let mockSetInboxRetriedOnCurrentEmpty: jest.Mock beforeEach(() => { mockLoadInboxNumSmallRows = jest.fn() mockInboxRefresh = jest.fn() + mockConfigState = {loggedIn: true, userSwitching: false} mockSetInboxRetriedOnCurrentEmpty = jest.fn() mockInboxLayoutState = { dispatch: { @@ -140,3 +142,18 @@ test('useInboxState updates inbox row count without persisting when persist is f expect(result.current.inboxNumSmallRows).toBe(7) expect(T.RPCGen.configGuiSetValueRpcPromise).not.toHaveBeenCalled() }) + +// The inbox RPCs refuse to run while a switch is under way, so the first load has to wait for it. +test('useInboxState loads the inbox once an account switch ends', () => { + mockInboxRefresh.mockReturnValue(Promise.resolve()) + mockInboxLayoutState.hasLoaded = false + mockConfigState = {loggedIn: true, userSwitching: true} + const {rerender} = renderHook(() => useInboxState()) + // what fired on mount ran into the switch and was refused + mockInboxRefresh.mockClear() + + mockConfigState = {loggedIn: true, userSwitching: false} + rerender() + + expect(mockInboxRefresh).toHaveBeenCalledWith('componentNeverLoaded') +}) diff --git a/shared/chat/inbox/use-inbox-state.tsx b/shared/chat/inbox/use-inbox-state.tsx index 83d55f37e629..a4065f3e7abf 100644 --- a/shared/chat/inbox/use-inbox-state.tsx +++ b/shared/chat/inbox/use-inbox-state.tsx @@ -62,7 +62,9 @@ export function useInboxState( refreshInbox?: T.Chat.ChatRootInboxRefresh ) { const isFocused = useIsFocused() - const loggedIn = useConfigState(s => s.loggedIn) + // Matches isChatSessionReady, which gates the inbox RPCs: loads skipped while a switch runs have + // to fire again once it ends. + const sessionReady = useConfigState(s => s.loggedIn && !s.userSwitching) const username = useCurrentUserState(s => s.username) const loadInboxNumSmallRows = C.useRPC(T.RPCGen.configGuiGetValueRpcPromise) @@ -130,14 +132,14 @@ export function useInboxState( }) React.useEffect(() => { - const ready = loggedIn && !!username && (!isMobile || isFocused) + const ready = sessionReady && !!username && (!isMobile || isFocused) if (!ready || !refreshInbox || handledRefreshNonceRef.current === refreshInbox.nonce) { return } handledRefreshNonceRef.current = refreshInbox.nonce C.ignorePromise(inboxRefresh(refreshInbox.reason)) C.Router2.setChatRootParams({refreshInbox: undefined}) - }, [inboxRefresh, isFocused, loggedIn, refreshInbox, username]) + }, [inboxRefresh, isFocused, sessionReady, refreshInbox, username]) C.Router2.useSafeFocusEffect( React.useCallback(() => { @@ -148,15 +150,15 @@ export function useInboxState( ) React.useEffect(() => { - const ready = loggedIn && !!username + const ready = sessionReady && !!username const shouldRetry = !inboxHasLoaded && ready && (!isMobile || isFocused) if (shouldRetry) { C.ignorePromise(inboxRefresh('componentNeverLoaded')) } - }, [inboxHasLoaded, inboxRefresh, isFocused, loggedIn, username]) + }, [inboxHasLoaded, inboxRefresh, isFocused, sessionReady, username]) React.useEffect(() => { - const ready = loggedIn && !!username + const ready = sessionReady && !!username if (!ready) { return } @@ -200,10 +202,10 @@ export function useInboxState( inboxNumSmallRowsLoadVersionRef.current++ } } - }, [inboxNumSmallRowsLoaded, loadInboxNumSmallRows, loggedIn, username]) + }, [inboxNumSmallRowsLoaded, loadInboxNumSmallRows, sessionReady, username]) React.useEffect(() => { - const ready = loggedIn && !!username && (!isMobile || isFocused) + const ready = sessionReady && !!username && (!isMobile || isFocused) if (!ready || isSearching || !inboxHasLoaded || inboxRows.length > 0 || inboxRetriedOnCurrentEmpty) { return } @@ -216,7 +218,7 @@ export function useInboxState( inboxRows.length, isFocused, isSearching, - loggedIn, + sessionReady, setRetriedOnCurrentEmpty, username, ]) diff --git a/shared/constants/init/index.tsx b/shared/constants/init/index.tsx index 6897f61f080d..b078193bc891 100644 --- a/shared/constants/init/index.tsx +++ b/shared/constants/init/index.tsx @@ -14,7 +14,7 @@ import logger from '@/logger' import {getEngine} from '@/engine' import {afterKbfsDaemonRpcStatusChanged} from '@/fs/common/lifecycle' import {logState, setThreadInputCommandStatus} from '@/constants/router' -import {initSharedSubscriptions, _onEngineIncoming, onEngineConnected as onSharedEngineConnected} from './shared' +import {initSharedSubscriptions, _onEngineIncoming} from './shared' import {noConversationIDKey} from '../types/chat/common' import {dumpLogs, persistRoute} from '@/util/storeless-actions' @@ -362,21 +362,6 @@ export const onEngineIncoming = (action: EngineGen.Actions) => { .dispatch.setOutOfDate({critical: true, message: upgradeMsg, outOfDate: true, updating: false}) break } - case 'keybase.1.NotifySession.loggedOut': { - if (useConfigState.getState().userSwitching) { - logger.info('Resetting renderer engine for account switch logout') - getEngine().reset() - } - break - } - case 'keybase.1.NotifySession.loggedIn': { - if (useConfigState.getState().userSwitching) { - logger.info('Refreshing renderer session registration for account switch login') - getEngine().reset() - onSharedEngineConnected() - } - break - } default: } } diff --git a/shared/constants/init/shared.test.ts b/shared/constants/init/shared.test.ts index 6ed968b2b26e..0b5f1d7a6c62 100644 --- a/shared/constants/init/shared.test.ts +++ b/shared/constants/init/shared.test.ts @@ -33,7 +33,7 @@ describe('loadAccountsStep', () => { test('does not wait for accounts while switching', async () => { withDeferredRefreshAccounts() - useConfigState.getState().dispatch.setUserSwitching(true) + useConfigState.getState().dispatch.setUserSwitching(true, 'testuser') useDaemonState.setState(s => { s.bootstrapStatus = {loggedIn: false} as any }) @@ -107,7 +107,7 @@ describe('onNetworkOnlineChanged', () => { test('does not re-read during an account switch', () => { const reRead = spyOnReRead() - useConfigState.getState().dispatch.setUserSwitching(true) + useConfigState.getState().dispatch.setUserSwitching(true, 'testuser') onNetworkOnlineChanged(true, false) expect(reRead).not.toHaveBeenCalled() }) @@ -254,27 +254,61 @@ describe('the session comes from the daemon; notifications only say to read it', expect(useCurrentUserState.getState().uid).toBe('u1') }) - test('during an account switch a logged-out reply is ignored, and the new user still replaces the old', async () => { + test('starting a switch logs the old account out of our stores', async () => { await readReplying(userA) markAccountState() - useConfigState.getState().dispatch.setUserSwitching(true) - const {changes, unsub} = loginChanges() + useConfigState.getState().dispatch.setUserSwitching(true, 'testuser2') + + expect(useConfigState.getState().loggedIn).toBe(false) + expect(accountStateCleared()).toBe(true) + }) + + test("once the switch's target is logged in, a logged-out reply mid-switch is ignored", async () => { + await readReplying(userA) + useConfigState.getState().dispatch.setUserSwitching(true, 'testuser2') + await readReplying(userB) + expect(useConfigState.getState().loggedIn).toBe(true) await readReplying(loggedOut) + expect(useConfigState.getState().loggedIn).toBe(true) + expect(useCurrentUserState.getState().username).toBe('testuser2') + expect(useConfigState.getState().userSwitching).toBe(true) + }) + + // A read of the old account in flight when the switch began replies after the reset. + test("mid-switch, the old account's reply is ignored, and the target's still applies", async () => { + await readReplying(userA) + useConfigState.getState().dispatch.setUserSwitching(true, 'testuser2') + const {changes, unsub} = loginChanges() + + await readReplying(userA) + expect(useConfigState.getState().loggedIn).toBe(false) + expect(useCurrentUserState.getState().uid).toBe('') await readReplying(userB) unsub() - expect(changes).toEqual([false, true]) - expect(accountStateCleared()).toBe(true) + expect(changes).toEqual([true]) + expect(useCurrentUserState.getState().username).toBe('testuser2') + expect(useConfigState.getState().userSwitching).toBe(true) + }) + + // The navigator for the new account ends the switch (endUserSwitchLandedOn), not its bootstrap. + test("the switch's target logging in leaves the switch for its navigator to end", async () => { + await readReplying(userA) + useConfigState.getState().dispatch.setUserSwitching(true, 'testuser2') + + await readReplying(userB) + + expect(useConfigState.getState().loggedIn).toBe(true) expect(useCurrentUserState.getState().username).toBe('testuser2') expect(useConfigState.getState().userSwitching).toBe(true) }) test('a switch whose login fails ends logged out, no longer switching', async () => { await readReplying(userA) - useConfigState.getState().dispatch.setUserSwitching(true) + useConfigState.getState().dispatch.setUserSwitching(true, 'testuser') await readReplying(loggedOut) useConfigState.getState().dispatch.setLoginError(new Error('bad password') as never) @@ -289,9 +323,9 @@ describe('the session comes from the daemon; notifications only say to read it', ['ended by a non-RPC error', new Error('engine reset')], ])('a switch whose login is %s ends logged out, no longer switching', async (_, error) => { await readReplying(userA) - useConfigState.getState().dispatch.setUserSwitching(true) + useConfigState.getState().dispatch.setUserSwitching(true, 'testuser') await readReplying(loggedOut) - expect(useConfigState.getState().loggedIn).toBe(true) + expect(useConfigState.getState().userSwitching).toBe(true) jest.spyOn(T.RPCGen, 'loginLoginRpcListener').mockRejectedValue(error) useConfigState.getState().dispatch.login('testuser2', 'password') diff --git a/shared/constants/init/shared.tsx b/shared/constants/init/shared.tsx index bb82d9078529..31ded8d2e135 100644 --- a/shared/constants/init/shared.tsx +++ b/shared/constants/init/shared.tsx @@ -256,28 +256,26 @@ const onBootstrapStatusChanged = ( } const { deviceID, deviceName, loggedIn, uid, username } = bootstrap; - useCurrentUserState - .getState() - .dispatch.setBootstrap({ deviceID, deviceName, uid, username }); const { dispatch: configDispatch, - defaultUsername: intendedUsername, userSwitching, + userSwitchingTo, } = useConfigState.getState(); - if (username && (!userSwitching || username === intendedUsername)) { - configDispatch.setDefaultUsername(username); - } - if (!loggedIn && userSwitching) { + // Mid-switch, only the target's session applies: a logged-out status, or one for the account + // being left (a read in flight when the switch began). onUserSwitchingChanged applies the + // latest status once the switch ends. + if (userSwitching && (!loggedIn || username !== userSwitchingTo)) { logger.info( - "[Bootstrap] ignoring loggedIn=false result during account switch", + "[Bootstrap] ignoring a status other than the switch target's during account switch", ); return; } // Logged in as someone else than the user we hold is a logout and then a login, however the // notifications in between reached us. Logging out clears the previous account's stores, the - // daemon's status among them, so put this status back and let that change apply it. + // daemon's status among them, so put this status back and let that change apply it. Read the + // held uid before anything below writes the current user. const currentUid = useCurrentUserState.getState().uid; if ( loggedIn && @@ -301,10 +299,6 @@ const onBootstrapStatusChanged = ( } configDispatch.setLoggedIn(loggedIn); - if (loggedIn && username && username === intendedUsername) { - configDispatch.setUserSwitching(false); - } - if (bootstrap.httpSrvInfo) { configDispatch.setHTTPSrvInfo( bootstrap.httpSrvInfo.address, diff --git a/shared/constants/navigate-append-once-root-has.test.ts b/shared/constants/navigate-append-once-root-has.test.ts new file mode 100644 index 000000000000..62b817186142 --- /dev/null +++ b/shared/constants/navigate-append-once-root-has.test.ts @@ -0,0 +1,102 @@ +/// +import logger from '@/logger' +import {navigateAppendOnceRootHas, navigationRef} from '@/constants/router' + +const dispatch = jest.fn() +const listeners = new Set<() => void>() +let rootState: unknown + +const loggedIn = {key: 'loggedIn-1', name: 'loggedIn'} +const loggedOut = { + key: 'loggedOut-1', + name: 'loggedOut', + state: {index: 0, key: 'loggedOutStack-1', routes: [{key: 'login-1', name: 'login'}], type: 'stack'}, +} + +const setRootRoutes = (routes: Array) => { + rootState = {index: routes.length - 1, key: 'root-1', routeNames: [], routes, stale: false, type: 'stack'} +} +const emitState = () => { + for (const l of [...listeners]) { + l() + } +} + +beforeEach(() => { + dispatch.mockReset() + listeners.clear() + // the jest mock's container ref is a plain object, so stub its methods directly + const nr = navigationRef as unknown as Record + nr['current'] = {} + nr['dispatch'] = dispatch + nr['getRootState'] = () => rootState + nr['isReady'] = () => true + nr['addListener'] = (_: string, cb: () => void) => { + listeners.add(cb) + return () => listeners.delete(cb) + } +}) + +afterEach(() => { + jest.useRealTimers() + jest.restoreAllMocks() +}) + +// Each test pushes distinct params: navigateAppend's module-private `_pendingAppend` dupe cache +// would otherwise swallow a same-shaped push from an earlier test. +const pushOf = (username: string) => + expect.objectContaining({payload: {name: 'username', params: {username}}, type: 'PUSH'}) + +test('pushes right away when the root already has the route', () => { + setRootRoutes([loggedOut]) + + navigateAppendOnceRootHas('loggedOut', {name: 'username', params: {username: 'testuser-a'}} as never) + + expect(dispatch).toHaveBeenCalledTimes(1) + expect(dispatch).toHaveBeenCalledWith(pushOf('testuser-a')) +}) + +test('waits for the root route to mount, then pushes once', () => { + setRootRoutes([loggedIn]) + + navigateAppendOnceRootHas('loggedOut', {name: 'username', params: {username: 'testuser-b'}} as never) + expect(dispatch).not.toHaveBeenCalled() + + emitState() + expect(dispatch).not.toHaveBeenCalled() + + setRootRoutes([loggedOut]) + emitState() + expect(dispatch).toHaveBeenCalledTimes(1) + expect(dispatch).toHaveBeenCalledWith(pushOf('testuser-b')) + + emitState() + expect(dispatch).toHaveBeenCalledTimes(1) +}) + +test('gives up if the root route does not mount before the timeout', () => { + jest.useFakeTimers() + setRootRoutes([loggedIn]) + + const warn = jest.spyOn(logger, 'warn').mockImplementation(() => {}) + + navigateAppendOnceRootHas('loggedOut', {name: 'username', params: {username: 'testuser-c'}} as never, 5000) + jest.advanceTimersByTime(5000) + expect(warn).toHaveBeenCalledWith(expect.stringContaining('loggedOut never mounted, dropping username')) + + setRootRoutes([loggedOut]) + emitState() + expect(dispatch).not.toHaveBeenCalled() +}) + +test('logs the push it drops when there is no navigator', () => { + setRootRoutes([loggedIn]) + const nr = navigationRef as unknown as Record + nr['isReady'] = () => false + const warn = jest.spyOn(logger, 'warn').mockImplementation(() => {}) + + navigateAppendOnceRootHas('loggedOut', {name: 'username', params: {username: 'testuser-d'}} as never) + + expect(warn).toHaveBeenCalledWith(expect.stringContaining('no navigator, dropping username')) + expect(dispatch).not.toHaveBeenCalled() +}) diff --git a/shared/constants/router.tsx b/shared/constants/router.tsx index 583a2edb7505..84865b086506 100644 --- a/shared/constants/router.tsx +++ b/shared/constants/router.tsx @@ -452,6 +452,37 @@ export function navigateAppend(path: NavigateAppendType, replace?: boolean): boo return true } +// Push once the root stack has a `rootRouteName` route. For a push whose target lives in a +// conditional root group that a store change is about to mount (e.g. the logged-out stack): a push +// dispatched before the group mounts reaches no navigator that can handle it and is dropped. Gives +// up after `timeoutMs` so a group that never mounts can't fire the push at some unrelated later time. +export const navigateAppendOnceRootHas = ( + rootRouteName: string, + path: NavigateAppendType, + timeoutMs = 5000 +) => { + const rootHas = () => getRootState()?.routes?.some(r => r.name === rootRouteName) ?? false + if (rootHas()) { + navigateAppend(path) + return + } + const n = _getNavigator() + if (!n) { + logger.warn(`[Nav] navigateAppendOnceRootHas: no navigator, dropping ${path.name}`) + return + } + const timer = setTimeout(() => { + unsub() + logger.warn(`[Nav] navigateAppendOnceRootHas: ${rootRouteName} never mounted, dropping ${path.name}`) + }, timeoutMs) + const unsub = n.addListener('state', () => { + if (!rootHas()) return + clearTimeout(timer) + unsub() + navigateAppend(path) + }) +} + export const switchTab = (name: Tabs.AppTab) => { if (DEBUG_NAV) { console.log('[Nav] switchTab', {name}) diff --git a/shared/engine/account-generation.tsx b/shared/engine/account-generation.tsx new file mode 100644 index 000000000000..3afef3c7b65b --- /dev/null +++ b/shared/engine/account-generation.tsx @@ -0,0 +1,37 @@ +import type {MethodKey} from './types' + +// Goes up when the session logs out: every logout, and the old account's side of an account switch. +// A call started before it belongs to an account that is gone, so its reply and any prompts the +// service sends on it are refused (see Session) instead of landing in the next account's stores. +let accountGeneration = 0 +export const getAccountGeneration = () => accountGeneration +// Called before anything reacts to the logout, so calls the logout itself starts are the new +// generation's. +export const startNewAccountGeneration = () => { + accountGeneration++ +} + +// Calls that change the logged-in account on purpose, so their replies arrive after the logout +// they cause, and calls whose answer belongs to the process rather than an account. +const spansAccountChange: ReadonlySet = new Set([ + 'keybase.1.account.cancelReset', + 'keybase.1.account.enterResetPipeline', + 'keybase.1.config.appendGUILogs', + 'keybase.1.config.getBootstrapStatus', + 'keybase.1.config.guiGetValue', + 'keybase.1.config.guiSetValue', + 'keybase.1.config.helloIAm', + 'keybase.1.config.logSend', + 'keybase.1.config.waitForClient', + 'keybase.1.login.accountDelete', + 'keybase.1.login.deprovision', + 'keybase.1.login.getConfiguredAccounts', + 'keybase.1.login.login', + 'keybase.1.login.logout', + 'keybase.1.login.recoverPassphrase', + 'keybase.1.signup.signup', +]) +// Registering UIs and notification channels is per connection, not per account. +const processWidePrefixes = ['keybase.1.delegateUiCtl.', 'keybase.1.notifyCtl.'] +export const survivesAccountChange = (method: MethodKey) => + spansAccountChange.has(method) || processWidePrefixes.some(p => method.startsWith(p)) diff --git a/shared/engine/session.test.tsx b/shared/engine/session.test.tsx index 1f37838c158c..8b74f063c358 100644 --- a/shared/engine/session.test.tsx +++ b/shared/engine/session.test.tsx @@ -2,6 +2,7 @@ import Session from './session' import {RPCError} from '@/util/errors' import * as T from '@/constants/types' +import {startNewAccountGeneration, survivesAccountChange} from './account-generation' const mockDispatchWaitingAction = jest.fn() jest.mock('./require', () => ({ @@ -66,3 +67,77 @@ test('a late server response after cancel does not fire the callback twice', () invokeCallback(undefined, {}) expect(callback).toHaveBeenCalledTimes(1) }) + +describe('a call that outlives its account', () => { + const startCall = (method: string) => { + const invoke = jest.fn() + const callback = jest.fn() + const session = new Session({ + customResponseIncomingCallMap: {'keybase.1.secretUi.getPassphrase': jest.fn()} as never, + endHandler: jest.fn(), + invoke, + sessionID: 9, + waitingKey: 'waiting-key', + }) + session.start(method, undefined, callback) + mockDispatchWaitingAction.mockReset() // drop the +1 from start + const reply = invoke.mock.calls[0]![2] as (err: unknown, data: unknown) => void + return {callback, reply, session} + } + const logOut = () => { + startNewAccountGeneration() + } + + test('its reply is refused after a logout, and still releases its waiting count', () => { + const {callback, reply} = startCall('keybase.1.user.getUserBlocks') + logOut() + + reply(undefined, [{username: 'testuser-mac'}]) + + expect(callback).toHaveBeenCalledTimes(1) + const [err, data] = callback.mock.calls[0]! as [RPCError, unknown] + expect(err).toBeInstanceOf(RPCError) + expect(err.code).toBe(T.RPCGen.StatusCode.sccanceled) + expect(data).toBeUndefined() + expect(mockDispatchWaitingAction).toHaveBeenCalledWith('waiting-key', false, undefined) + }) + + test('a prompt the service sends on it is answered with an error, not handed to its handler', () => { + const {session} = startCall('keybase.1.identify3.identify3') + logOut() + const error = jest.fn() + const handler = (session as unknown as {_customResponseIncomingCallMap: Record}) + ._customResponseIncomingCallMap['keybase.1.secretUi.getPassphrase']! + + expect(session.incomingCall('keybase.1.secretUi.getPassphrase', {}, {error, seqid: 3} as never)).toBe(true) + + expect(handler).not.toHaveBeenCalled() + expect(error).toHaveBeenCalledWith(expect.objectContaining({code: T.RPCGen.StatusCode.sccanceled})) + expect(mockDispatchWaitingAction).not.toHaveBeenCalled() + }) + + test('a call that changes the account on purpose still gets its reply', () => { + const {callback, reply} = startCall('keybase.1.login.login') + logOut() + + reply(undefined, undefined) + + expect(callback).toHaveBeenCalledWith(undefined, undefined) + expect(mockDispatchWaitingAction).toHaveBeenCalledWith('waiting-key', false, undefined) + }) + + test('registering with the service outlives an account', () => { + expect(survivesAccountChange('keybase.1.delegateUiCtl.registerChatUI')).toBe(true) + expect(survivesAccountChange('keybase.1.notifyCtl.setNotifications')).toBe(true) + expect(survivesAccountChange('keybase.1.user.getUserBlocks')).toBe(false) + }) + + test('a call started after the logout is answered normally', () => { + logOut() + const {callback, reply} = startCall('keybase.1.user.getUserBlocks') + + reply(undefined, []) + + expect(callback).toHaveBeenCalledWith(undefined, []) + }) +}) diff --git a/shared/engine/session.tsx b/shared/engine/session.tsx index 6ce67eb6c2a8..4b3a0a752f5c 100644 --- a/shared/engine/session.tsx +++ b/shared/engine/session.tsx @@ -7,6 +7,7 @@ import {printRPC} from '@/local-debug' import {rpcLog, type InvokeType} from './index.platform' import {RPCError} from '@/util/errors' import {getEngine} from './require' +import {getAccountGeneration, survivesAccountChange} from './account-generation' import type {SessionID, ResponseType, EndHandlerType, MethodKey, WaitingKey} from './types' // A session is a series of calls back and forth tied together with a single sessionID @@ -31,6 +32,8 @@ class Session { _startMethod: MethodKey | undefined // Start callback so we can cancel our own callback _startCallback: ((err?: RPCError, ...args: Array) => void) | undefined + // The account generation the session started in; undefined until start + _accountGeneration: number | undefined // Allow us to make calls _invoke: InvokeType @@ -62,6 +65,15 @@ class Session { return this._dangling } + // Started for an account that has since logged out, so nothing it receives may reach its handlers. + _belongsToPreviousAccount() { + return ( + this._accountGeneration !== undefined && + this._accountGeneration !== getAccountGeneration() && + !survivesAccountChange(this._startMethod ?? '') + ) + } + // Make a waiting handler for the request. We add additional data before calling the parent waitingHandler // and do internal bookkeeping if the request is done _makeWaitingHandler(method: MethodKey, seqid?: number) { @@ -110,6 +122,7 @@ class Session { start(method: MethodKey, param: object | undefined, callback: (() => void) | undefined) { this._startMethod = method this._startCallback = callback + this._accountGeneration = getAccountGeneration() // When this request is done the session is done const wrappedCallback = (err: RPCError | undefined, ...args: Array) => { @@ -135,6 +148,11 @@ class Session { const updateWaiting = this._makeWaitingHandler(method) updateWaiting(true) this._invoke(method, [wrappedParam], (err: unknown, data: unknown) => { + if (this._belongsToPreviousAccount()) { + updateWaiting(false) + wrappedCallback(new RPCError('The account changed during this call', StatusCode.sccanceled)) + return + } updateWaiting(false, err as RPCError | undefined) wrappedCallback(err as RPCError | undefined, data) }) @@ -168,6 +186,11 @@ class Session { return false } + if (this._belongsToPreviousAccount()) { + response?.error?.({code: StatusCode.sccanceled, desc: 'The account changed during this call'}) + return true + } + if (response?.seqid !== undefined) { this._seqIDsAwaitingResponse.add(response.seqid) } diff --git a/shared/patches/react-native-screens+4.28.0.patch b/shared/patches/react-native-screens+4.28.0.patch index 8d3960254305..a120e94b6639 100644 --- a/shared/patches/react-native-screens+4.28.0.patch +++ b/shared/patches/react-native-screens+4.28.0.patch @@ -114,6 +114,33 @@ index add33c4..8022575 100644 } } #endif // RNS_IPHONE_OS_VERSION_AVAILABLE(26_0) +diff --git a/node_modules/react-native-screens/ios/tabs/host/RNSTabBarController.mm b/node_modules/react-native-screens/ios/tabs/host/RNSTabBarController.mm +index 06c1957..222e098 100644 +--- a/node_modules/react-native-screens/ios/tabs/host/RNSTabBarController.mm ++++ b/node_modules/react-native-screens/ios/tabs/host/RNSTabBarController.mm +@@ -307,12 +307,22 @@ - (BOOL)updateSelectedViewControllerTo:(nullable UIViewController *)nextSelected + RCTAssert(![NSString rnscreens_isBlankOrNull:screenKey], + @"[RNScreens] The screenKey MUST NOT be null if the view controller is not null"); + ++ BOOL isInitialSelection = _navigationState == nil; + [self progressNavigationState:screenKey withOrigin:actionOrigin]; + + if (currSelectedViewController == nextSelectedViewController) { + return YES; + } + ++ // setViewControllers: already selected index 0; don't slide the iOS 26 glass pill to the startup tab. ++ if (isInitialSelection) { ++ [UIView performWithoutAnimation:^{ ++ [self setSelectedViewController:nextSelectedViewController]; ++ [self.tabBar layoutIfNeeded]; ++ }]; ++ return YES; ++ } ++ + [self setSelectedViewController:nextSelectedViewController]; + return YES; + } diff --git a/node_modules/react-native-screens/ios/utils/UINavigationBar+RNSUtility.h b/node_modules/react-native-screens/ios/utils/UINavigationBar+RNSUtility.h index 0e7010d..8e3af12 100644 --- a/node_modules/react-native-screens/ios/utils/UINavigationBar+RNSUtility.h diff --git a/shared/provision/waiting-overlay.test.tsx b/shared/provision/waiting-overlay.test.tsx index 798816d3d497..e03f96851427 100644 --- a/shared/provision/waiting-overlay.test.tsx +++ b/shared/provision/waiting-overlay.test.tsx @@ -64,6 +64,8 @@ describe('ProvisionWaitingOverlay', () => { mockAddListener.mockReset() mockPauseProvision.mockReset() mockNavigateUp.mockReset() + // a logout keeps in-flight waiting counts, and some tests end mid-wait + useWaitingState.getState().dispatch.clear(waitingKeyProvision) resetAllStores() }) diff --git a/shared/router-v2/account-link-switch.test.ts b/shared/router-v2/account-link-switch.test.ts index 95df1a0b3e43..6c96324acb06 100644 --- a/shared/router-v2/account-link-switch.test.ts +++ b/shared/router-v2/account-link-switch.test.ts @@ -81,6 +81,15 @@ test('a tap for a stored account switches to it once', () => { expect(login).toHaveBeenCalledTimes(1) }) +// Starting the switch resets the stores, loggedIn with them; that is not a logout to drop the tap for. +test('the store reset a switch starts with keeps the tap it is for', () => { + tapFor(otherAccount.uid) + + expect(useConfigState.getState().loggedIn).toBe(false) + expect(mockAckPushTap).not.toHaveBeenCalled() + expect(useNavigationIntentsState.getState().intent?.targetUid).toBe(otherAccount.uid) +}) + test('a tap for an account not listed yet waits for the account list', () => { setAccounts([currentAccount]) tapFor(otherAccount.uid) diff --git a/shared/router-v2/account-link-switch.tsx b/shared/router-v2/account-link-switch.tsx index 8873a6a75c95..fb57cc65ba4b 100644 --- a/shared/router-v2/account-link-switch.tsx +++ b/shared/router-v2/account-link-switch.tsx @@ -44,8 +44,7 @@ export const subscribeIntentAccountSwitch = () => { } switchingFor = intent.id logger.info('[AccountLink] switching accounts for a tapped push') - dispatch.setUserSwitching(true) - dispatch.login(account.username, '') + dispatch.switchToAccount(account.username) } const dropOnFailure = (s: ConfigState, old: ConfigState) => { const loginFailed = !!s.loginError && s.loginError !== old.loginError diff --git a/shared/router-v2/account-switch-header-avatar.native.tsx b/shared/router-v2/account-switch-header-avatar.native.tsx index b12f24b89bc1..74f461b2df2c 100644 --- a/shared/router-v2/account-switch-header-avatar.native.tsx +++ b/shared/router-v2/account-switch-header-avatar.native.tsx @@ -15,11 +15,10 @@ const openAccountSwitcher = () => { const AccountSwitchHeaderAvatar = () => { const styles = useStyles() const username = useCurrentUserState(s => s.username) - const {configuredAccounts, login, setUserSwitching, userSwitching} = useConfigState( + const {configuredAccounts, switchToAccount, userSwitching} = useConfigState( C.useShallow(s => ({ configuredAccounts: s.configuredAccounts, - login: s.dispatch.login, - setUserSwitching: s.dispatch.setUserSwitching, + switchToAccount: s.dispatch.switchToAccount, userSwitching: s.userSwitching, })) ) @@ -27,13 +26,13 @@ const AccountSwitchHeaderAvatar = () => { const handledLongPressRef = React.useRef(false) const switchToRecentAccount = () => { - if (userSwitching || !recentAccount) return + if (!recentAccount) return + const tab = C.Router2.getTab() + if (!switchToAccount(recentAccount.username)) return handledLongPressRef.current = true C.ignorePromise(Haptics.selectionAsync()) - rememberAccountSwitchTab(username, recentAccount.username, C.Router2.getTab()) - setUserSwitching(true) - login(recentAccount.username, '') + rememberAccountSwitchTab(username, recentAccount.username, tab) } const onPressIn = () => { diff --git a/shared/router-v2/account-switch.test.tsx b/shared/router-v2/account-switch.test.tsx index bd35eb979d10..e801621f902a 100644 --- a/shared/router-v2/account-switch.test.tsx +++ b/shared/router-v2/account-switch.test.tsx @@ -5,7 +5,9 @@ import { clearPendingAccountSwitch, consumePendingAccountSwitchTab, getMostRecentlyUsedAccount, + peekPendingAccountSwitchTab, rememberAccountSwitchTab, + showLoggedInScreens, } from './account-switch' const account = (username: string, hasStoredSecret = true) => ({ @@ -38,38 +40,67 @@ describe('pending account-switch tab', () => { }) test('returns the remembered tab after the username changes and consumes it once', () => { - rememberAccountSwitchTab('alice', 'bob', Tabs.chatTab) + rememberAccountSwitchTab('testuser', 'testuser-mac', Tabs.chatTab) - expect(consumePendingAccountSwitchTab('bob')).toBe(Tabs.chatTab) - expect(consumePendingAccountSwitchTab('bob')).toBeUndefined() + expect(consumePendingAccountSwitchTab('testuser-mac')).toBe(Tabs.chatTab) + expect(consumePendingAccountSwitchTab('testuser-mac')).toBeUndefined() + }) + + test('peeks the remembered tab for the target account without consuming it', () => { + rememberAccountSwitchTab('testuser', 'testuser-mac', Tabs.fsTab) + + expect(peekPendingAccountSwitchTab('testuser')).toBeUndefined() + expect(peekPendingAccountSwitchTab('testuser-mac')).toBe(Tabs.fsTab) + expect(consumePendingAccountSwitchTab('testuser-mac')).toBe(Tabs.fsTab) }) test('does not consume the tab before the account changes', () => { - rememberAccountSwitchTab('alice', 'bob', Tabs.fsTab) + rememberAccountSwitchTab('testuser', 'testuser-mac', Tabs.fsTab) - expect(consumePendingAccountSwitchTab('alice')).toBeUndefined() - expect(consumePendingAccountSwitchTab('bob')).toBe(Tabs.fsTab) + expect(consumePendingAccountSwitchTab('testuser')).toBeUndefined() + expect(consumePendingAccountSwitchTab('testuser-mac')).toBe(Tabs.fsTab) }) test('keeps the pending tab when switching ends on the target account', () => { - rememberAccountSwitchTab('alice', 'bob', Tabs.teamsTab) + rememberAccountSwitchTab('testuser', 'testuser-mac', Tabs.teamsTab) - clearPendingAccountSwitch('bob') + clearPendingAccountSwitch('testuser-mac') - expect(consumePendingAccountSwitchTab('bob')).toBe(Tabs.teamsTab) + expect(consumePendingAccountSwitchTab('testuser-mac')).toBe(Tabs.teamsTab) }) test('clears the pending tab when switching fails after blanking the username', () => { - rememberAccountSwitchTab('alice', 'bob', Tabs.teamsTab) + rememberAccountSwitchTab('testuser', 'testuser-mac', Tabs.teamsTab) clearPendingAccountSwitch('') - expect(consumePendingAccountSwitchTab('bob')).toBeUndefined() + expect(consumePendingAccountSwitchTab('testuser-mac')).toBeUndefined() }) test('ignores routes that are not application tabs', () => { - rememberAccountSwitchTab('alice', 'bob', Tabs.loginTab) + rememberAccountSwitchTab('testuser', 'testuser-mac', Tabs.loginTab) + + expect(consumePendingAccountSwitchTab('testuser-mac')).toBeUndefined() + }) +}) + +describe('showLoggedInScreens', () => { + const state = (loggedIn: boolean, userSwitching = false, userSwitchingFromLoggedIn = false) => ({ + loggedIn, + userSwitching, + userSwitchingFromLoggedIn, + }) + + test('follows loggedIn when no switch is running', () => { + expect(showLoggedInScreens(state(true))).toBe(true) + expect(showLoggedInScreens(state(false))).toBe(false) + }) + + test('holds the logged-in screens through the loggedIn flap of a switch that started logged in', () => { + expect(showLoggedInScreens(state(false, true, true))).toBe(true) + }) - expect(consumePendingAccountSwitchTab('bob')).toBeUndefined() + test('keeps the logged-out screens for a switch that started logged out', () => { + expect(showLoggedInScreens(state(false, true, false))).toBe(false) }) }) diff --git a/shared/router-v2/account-switch.tsx b/shared/router-v2/account-switch.tsx index 5568af4a57f9..8148579f141e 100644 --- a/shared/router-v2/account-switch.tsx +++ b/shared/router-v2/account-switch.tsx @@ -30,6 +30,9 @@ export const rememberAccountSwitchTab = ( : undefined } +export const peekPendingAccountSwitchTab = (currentUsername: string) => + pendingAccountSwitch?.targetUsername === currentUsername ? pendingAccountSwitch.tab : undefined + export const consumePendingAccountSwitchTab = (currentUsername: string) => { const pending = pendingAccountSwitch if (pending?.targetUsername !== currentUsername) return @@ -37,6 +40,18 @@ export const consumePendingAccountSwitchTab = (currentUsername: string) => { return pending.tab } +// Whether the root navigator shows the logged-in screens. A switch that starts while logged in flaps +// loggedIn false and back between the service's loggedOut and loggedIn notifications. Following +// that would swap the native root stack to loggedOut and back right before the navKey remount, and +// that churn leaves RNS screens from the unmounted navigator on screen, swallowing every touch. So +// hold the logged-in screens through such a switch. A switch that starts logged out (e.g. a +// notification tap on the login screen) keeps the logged-out screens until it lands. +export const showLoggedInScreens = (s: { + loggedIn: boolean + userSwitching: boolean + userSwitchingFromLoggedIn: boolean +}) => s.loggedIn || (s.userSwitching && s.userSwitchingFromLoggedIn) + export const clearPendingAccountSwitch = (currentUsername: string) => { if (pendingAccountSwitch?.targetUsername !== currentUsername) { pendingAccountSwitch = undefined diff --git a/shared/router-v2/account-switcher/index.test.tsx b/shared/router-v2/account-switcher/index.test.tsx new file mode 100644 index 000000000000..ceb1bba8e533 --- /dev/null +++ b/shared/router-v2/account-switcher/index.test.tsx @@ -0,0 +1,69 @@ +/** @jest-environment jsdom */ +/// +import type * as React from 'react' +import {act, cleanup, fireEvent, render, screen} from '@testing-library/react' +import * as T from '@/constants/types' +import {useConfigState} from '@/stores/config' +import {useCurrentUserState} from '@/stores/current-user' +import {resetAllStores} from '@/util/zustand' + +jest.mock('@/common-adapters', () => { + const React = require('react') + const Pass = ({children}: {children?: React.ReactNode}) => React.createElement('div', null, children) + return { + Avatar: () => null, + Box2: Pass, + Divider: () => null, + ListItem: ({body, onClick}: {body?: React.ReactNode; onClick?: () => void}) => + React.createElement('button', {onClick, type: 'button'}, body), + ProgressIndicator: () => null, + ScrollView: Pass, + Styles: { + createStyleHook: () => () => ({}), + platformStyles: () => ({}), + }, + Text: ({children}: {children?: React.ReactNode}) => React.createElement('span', null, children), + } +}) + +import AccountSwitcher from '.' + +beforeEach(() => { + useCurrentUserState + .getState() + .dispatch.setBootstrap({deviceID: 'd', deviceName: 'dn', uid: 'testuser', username: 'testuser'}) + useConfigState.getState().dispatch.setAccounts([ + {fullname: '', hasStoredSecret: true, uid: 'testuser-mac', username: 'testuser-mac'}, + ]) +}) + +afterEach(() => { + cleanup() + jest.restoreAllMocks() + act(() => { + resetAllStores() + }) +}) + +const loginSpy = () => jest.spyOn(T.RPCGen, 'loginLoginRpcListener').mockImplementation(async () => new Promise(() => {})) + +test('an account row starts a switch when no switch is running', () => { + const login = loginSpy() + render() + + fireEvent.click(screen.getByRole('button', {name: 'testuser-mac'})) + + expect(login).toHaveBeenCalled() +}) + +test('account rows are disabled while a switch is running, even after the reset clears the login waiting key', () => { + const login = loginSpy() + act(() => { + useConfigState.getState().dispatch.setUserSwitching(true, 'testuser-other') + }) + render() + + fireEvent.click(screen.getByRole('button', {name: 'testuser-mac'})) + + expect(login).not.toHaveBeenCalled() +}) diff --git a/shared/router-v2/account-switcher/index.tsx b/shared/router-v2/account-switcher/index.tsx index fd8a03d5fa25..b31ece36ace6 100644 --- a/shared/router-v2/account-switcher/index.tsx +++ b/shared/router-v2/account-switcher/index.tsx @@ -15,29 +15,30 @@ const AccountSwitcher = (p: {onSelected?: () => void}) => { const _fullnames = useUsersState(s => s.infoMap) const { accountRows: _accountRows, - login, logoutAndTryToLogInAs: onSelectAccountLoggedOut, logoutToLoggedOutFlow: onLoginAsAnotherUser, - setUserSwitching, + switchToAccount, + userSwitching, } = useConfigState( C.useShallow(s => ({ accountRows: s.configuredAccounts, - login: s.dispatch.login, logoutAndTryToLogInAs: s.dispatch.logoutAndTryToLogInAs, logoutToLoggedOutFlow: s.dispatch.logoutToLoggedOutFlow, - setUserSwitching: s.dispatch.setUserSwitching, + switchToAccount: s.dispatch.switchToAccount, + userSwitching: s.userSwitching, })) ) const you = useCurrentUserState(s => s.username) const fullname = _fullnames.get(you)?.fullname ?? '' - const waiting = C.Waiting.useAnyWaiting(C.waitingKeyConfigLogin) + // The mid-switch store reset clears the login waiting key while the switch is still running, so + // also hold the rows on userSwitching or a second switch can start before the first lands. + const waiting = C.Waiting.useAnyWaiting(C.waitingKeyConfigLogin) || userSwitching const onSelectAccountLoggedIn = (username: string) => { - if (isMobile) { - rememberAccountSwitchTab(you, username, C.Router2.getTab()) + const tab = C.Router2.getTab() + if (switchToAccount(username) && isMobile) { + rememberAccountSwitchTab(you, username, tab) } - setUserSwitching(true) - login(username, '') } const accountRows = _accountRows.filter(account => account.username !== you) @@ -124,6 +125,7 @@ const MobileHeader = (props: Props) => { mode="Primary" fullWidth={true} waitingKey={C.waitingKeyConfigLoginAsOther} + disabled={props.waiting} /> diff --git a/shared/router-v2/header/index.desktop.tsx b/shared/router-v2/header/index.desktop.tsx index 7595a1e3b617..1350a75c2238 100644 --- a/shared/router-v2/header/index.desktop.tsx +++ b/shared/router-v2/header/index.desktop.tsx @@ -3,7 +3,7 @@ import * as Kb from '@/common-adapters' import * as Platform from '@/constants/platform' import SyncingFolders from './syncing-folders' import KB2 from '@/util/electron' -import {useConfigState} from '@/stores/config' +import {useLoggedInScreens} from '../logged-in-screens' import {useShellState} from '@/stores/shell' import type {HeaderBackButtonProps} from '@react-navigation/elements' import type {NativeStackHeaderProps} from '@react-navigation/native-stack' @@ -397,7 +397,7 @@ type HeaderProps = Omit s.useNativeFrame) - const loggedIn = useConfigState(s => s.loggedIn) + const loggedIn = useLoggedInScreens() const isMaximized = useShellState(s => s.windowState.isMaximized) const {headerMode, title, headerTitle, headerRightActions, subHeader} = _options const {headerRight, headerTransparent, headerShadowVisible, headerBottomStyle, headerStyle, headerLeft} = diff --git a/shared/router-v2/intent-consumption.test.ts b/shared/router-v2/intent-consumption.test.ts index 9afdc67f1808..858f3bf7c162 100644 --- a/shared/router-v2/intent-consumption.test.ts +++ b/shared/router-v2/intent-consumption.test.ts @@ -65,7 +65,7 @@ test('a stale intent that is dropped without navigating still acks its tap route const ack = mockAckPushTap const now = jest.spyOn(Date, 'now') now.mockReturnValue(1_000) - useConfigState.getState().dispatch.setUserSwitching(true) + useConfigState.getState().dispatch.setUserSwitching(true, 'testuser') const listener = jest.fn() const unsubscribe = subscribeNavigationIntents(listener, jest.fn()) @@ -108,7 +108,7 @@ test('a stale intent is discarded instead of navigating', () => { now.mockReturnValue(1_000) // block consumption so the intent sits in the queue while time passes - useConfigState.getState().dispatch.setUserSwitching(true) + useConfigState.getState().dispatch.setUserSwitching(true, 'testuser') const listener = jest.fn() const handleAppLink = jest.fn() const unsubscribe = subscribeNavigationIntents(listener, handleAppLink) @@ -129,12 +129,18 @@ test('an intent that is still within its lifetime is consumed after the block cl const now = jest.spyOn(Date, 'now') now.mockReturnValue(1_000) - useConfigState.getState().dispatch.setUserSwitching(true) + useConfigState.getState().dispatch.setUserSwitching(true, 'testuser') const listener = jest.fn() const unsubscribe = subscribeNavigationIntents(listener, jest.fn()) emitDeepLink('keybase://convid/fresh-conversation') + // Starting the switch reset every store; the switched-to account logs back in and readies its router + setCurrentUser('current-uid') + useConfigState.getState().dispatch.setLoggedIn(true) + useNavigationIntentsState.getState().dispatch.setNavigationReady(true, 'current-uid') + expect(listener).not.toHaveBeenCalled() + now.mockReturnValue(1_000 + 5 * 60_000 - 1) useConfigState.getState().dispatch.setUserSwitching(false) @@ -177,7 +183,7 @@ test('an account-targeted intent survives the store reset an account switch perf const listener = jest.fn() const unsubscribe = subscribeNavigationIntents(listener, jest.fn()) - useConfigState.getState().dispatch.setUserSwitching(true) + useConfigState.getState().dispatch.setUserSwitching(true, 'testuser') enqueuePushTapRoute({id: 4444, targetUid: 'target-uid', url: 'keybase://convid/switch-target-conversation'}) expect(listener).not.toHaveBeenCalled() diff --git a/shared/router-v2/linking-initial-url.test.ts b/shared/router-v2/linking-initial-url.test.ts index 699a313e793c..5e4f96a307bb 100644 --- a/shared/router-v2/linking-initial-url.test.ts +++ b/shared/router-v2/linking-initial-url.test.ts @@ -6,6 +6,7 @@ import {useConfigState} from '@/stores/config' import {useCurrentUserState} from '@/stores/current-user' import {setPushTapAck, useNavigationIntentsState} from '@/stores/navigation-intents' import {usePushState} from '@/stores/push' +import {peekPendingAccountSwitchTab, rememberAccountSwitchTab} from './account-switch' import {createLinkingConfig} from './linking' import {enqueuePushTapRoute} from './deep-link-emitter' @@ -63,9 +64,26 @@ afterEach(() => { const {intent, dispatch} = useNavigationIntentsState.getState() if (intent) dispatch.acknowledge(intent.id) jest.restoreAllMocks() + rememberAccountSwitchTab('', '', undefined) resetAllStores() }) +test('an account switch starts on the switcher tab without consuming it before onReady', async () => { + rememberAccountSwitchTab('testuser', 'testuser-mac', Tabs.teamsTab) + setCurrentUser('testuser-mac') + setStartup({conversation: 'conv-1', conversationUid: 'testuser-mac', tab: Tabs.chatTab}) + + await expect(getInitialURL()).resolves.toBe(`keybase://${Tabs.teamsTab}`) + expect(peekPendingAccountSwitchTab('testuser-mac')).toBe(Tabs.teamsTab) +}) + +test('a switcher tab remembered for another account does not preempt the saved route', async () => { + rememberAccountSwitchTab('current-uid', 'testuser-mac', Tabs.teamsTab) + setStartup({tab: Tabs.chatTab}) + + await expect(getInitialURL()).resolves.toBe(`keybase://${Tabs.chatTab}`) +}) + test('a logged out app has no initial url', async () => { useConfigState.getState().dispatch.setLoggedIn(false) setStartup({tab: Tabs.chatTab}) diff --git a/shared/router-v2/linking.test.ts b/shared/router-v2/linking.test.ts index e26df0725ce4..11529065121d 100644 --- a/shared/router-v2/linking.test.ts +++ b/shared/router-v2/linking.test.ts @@ -90,13 +90,22 @@ test('waits until the intended account is active', () => { unsubscribe() }) +// Starting a switch resets every store, logging the old account out of them; the switched-to +// account's bootstrap then logs it back in and its router readies before the switch ends. +const landSwitchOn = (uid: string) => { + setCurrentUser(uid) + useConfigState.getState().dispatch.setLoggedIn(true) + useNavigationIntentsState.getState().dispatch.setNavigationReady(true, uid) +} + test('waits for an account switch to finish', () => { useNavigationIntentsState.getState().dispatch.setNavigationReady(true, 'current-uid') - useConfigState.getState().dispatch.setUserSwitching(true) + useConfigState.getState().dispatch.setUserSwitching(true, 'testuser') const listener = jest.fn() const unsubscribe = subscribeNavigationIntents(listener, jest.fn()) enqueuePushTapRoute({id: tapID(), targetUid: 'current-uid', url: 'keybase://convid/account-switch-conversation'}) + landSwitchOn('current-uid') expect(listener).not.toHaveBeenCalled() useConfigState.getState().dispatch.setUserSwitching(false) @@ -108,12 +117,13 @@ test('waits for an account switch to finish', () => { test('waits for the replacement router after the current account changes', () => { const navigationDispatch = useNavigationIntentsState.getState().dispatch navigationDispatch.setNavigationReady(true, 'current-uid') - useConfigState.getState().dispatch.setUserSwitching(true) + useConfigState.getState().dispatch.setUserSwitching(true, 'testuser') const listener = jest.fn() const unsubscribe = subscribeNavigationIntents(listener, jest.fn()) enqueuePushTapRoute({id: tapID(), targetUid: 'target-uid', url: 'keybase://convid/replacement-router-conversation'}) setCurrentUser('target-uid') + useConfigState.getState().dispatch.setLoggedIn(true) // The bootstrap UID can change before React commits the keyed router remount. // Even if switching is cleared early, the old account's ready router must not diff --git a/shared/router-v2/linking.tsx b/shared/router-v2/linking.tsx index 8133ba109318..7b22cedf5732 100644 --- a/shared/router-v2/linking.tsx +++ b/shared/router-v2/linking.tsx @@ -11,6 +11,7 @@ import {usePushState} from '@/stores/push' import type {LinkingOptions} from '@react-navigation/native' import type {RootParamList} from './route-params' import {Linking} from 'react-native' +import {peekPendingAccountSwitchTab} from './account-switch' import {emitDeepLink, normalizeUrl, setInitialURLOnce} from './deep-link-emitter' // Re-exported so existing importers ('@/router-v2/linking') keep working; the // definitions live in the dependency-free './deep-link-emitter' leaf. @@ -338,6 +339,13 @@ export const createLinkingConfig = ( const {loggedIn, startup, androidShare} = useConfigState.getState() if (!loggedIn) return null + // An account switch remounts the navigator. Start it on the switcher's tab: switching there + // after mount slides the iOS 26 glass tab pill over from the first tab. + const accountSwitchTab = peekPendingAccountSwitchTab(useCurrentUserState.getState().username) + if (accountSwitchTab) { + return setInitialURLOnce(`keybase://${accountSwitchTab}`) + } + const {tab: startupTab} = startup let startupConversation = startup.conversation if (!isValidConversationIDKey(startupConversation)) { diff --git a/shared/router-v2/logged-in-screens.tsx b/shared/router-v2/logged-in-screens.tsx new file mode 100644 index 000000000000..71be4a61b250 --- /dev/null +++ b/shared/router-v2/logged-in-screens.tsx @@ -0,0 +1,6 @@ +import {useConfigState} from '@/stores/config' +import {showLoggedInScreens} from './account-switch' + +// Whether the root navigator shows the logged-in screens. The routers' groups and the desktop +// header all read it here so they can't disagree. +export const useLoggedInScreens = () => useConfigState(showLoggedInScreens) diff --git a/shared/router-v2/router.tsx b/shared/router-v2/router.tsx index 33bbfed02154..a54ce526c2c1 100644 --- a/shared/router-v2/router.tsx +++ b/shared/router-v2/router.tsx @@ -33,6 +33,7 @@ import {isLiquidGlassSupported as _isLiquidGlassSupported} from '@callstack/liqu import {Platform, StatusBar, View} from 'react-native' import AccountSwitchHeaderAvatar from './account-switch-header-avatar' import {clearPendingAccountSwitch, consumePendingAccountSwitchTab} from './account-switch' +import {useLoggedInScreens} from './logged-in-screens' import {useCurrentUserState} from '@/stores/current-user' import {useNavigationIntentsState} from '@/stores/navigation-intents' const isLiquidGlassSupported = isMobile ? (_isLiquidGlassSupported as boolean) : false @@ -99,7 +100,7 @@ const setNavRef = (ref: typeof C.Router2.navigationRef.current) => { // Sticky: once the handshake finishes we never go back to the splash, even if it // restarts later (engine reconnect); the disconnected overlay covers that case. // Module-level so it survives the navigator remount on user switch (a ref would -// reset and flash the splash while the post-switch handshake is still running). +// reset and flash the splash). let handshakeEverDone = false const useHandshakeEverDone = () => { return useDaemonState(s => { @@ -187,17 +188,15 @@ if (!isMobile) { const useIsLoadingDesktop = () => !useHandshakeEverDone() - // During an account switch loggedIn flaps false between the service's loggedOut and - // loggedIn notifications; keep the app (and its left nav) mounted through that gap. const useIsLoggedInDesktop = () => { const loaded = useHandshakeEverDone() - const loggedIn = useConfigState(s => s.loggedIn || s.userSwitching) + const loggedIn = useLoggedInScreens() return loaded && loggedIn } const useIsLoggedOutDesktop = () => { const loaded = useHandshakeEverDone() - const loggedIn = useConfigState(s => s.loggedIn || s.userSwitching) + const loggedIn = useLoggedInScreens() return loaded && !loggedIn } @@ -262,7 +261,13 @@ function DesktopRouter() { const isDarkMode = useDarkModeState(s => s.isDarkMode()) const navKey = Common.useUserSwitchNavKey() - const currentUid = useCurrentUserState(s => s.uid) + const {currentUid, username} = useCurrentUserState( + C.useShallow(s => ({ + currentUid: s.uid, + username: s.username, + })) + ) + const endUserSwitchLandedOn = useConfigState(s => s.dispatch.endUserSwitchLandedOn) const setNavigationReady = useNavigationIntentsState(s => s.dispatch.setNavigationReady) React.useEffect( @@ -292,6 +297,7 @@ function DesktopRouter() { onReady={() => { onStateChange() setNavigationReady(true, currentUid) + endUserSwitchLandedOn(username) }} onStateChange={onStateChange} onUnhandledAction={onUnhandledAction} @@ -592,8 +598,8 @@ if (isMobile) { } } - const useIsLoggedInNative = () => useConfigState(s => s.loggedIn) - const useIsLoggedOutNative = () => !useConfigState(s => s.loggedIn) + const useIsLoggedInNative = () => useLoggedInScreens() + const useIsLoggedOutNative = () => !useLoggedInScreens() const nativeModalScreensConfig = routeMapToStaticScreens(modalRoutes, makeLayout, true, false, false) const nativePhoneRootScreensConfig = routeMapToStaticScreens( @@ -655,8 +661,9 @@ function NativeRouter() { const theme = Kb.Styles.useTheme() const loggedInLoaded = useHandshakeEverDone() - const {loggedIn, startupLoaded, userSwitching} = useConfigState( + const {endUserSwitchLandedOn, loggedIn, startupLoaded, userSwitching} = useConfigState( C.useShallow(s => ({ + endUserSwitchLandedOn: s.dispatch.endUserSwitchLandedOn, loggedIn: s.loggedIn, startupLoaded: s.startup.loaded, userSwitching: s.userSwitching, @@ -703,6 +710,7 @@ function NativeRouter() { C.Router2.switchTab(tab) } setNavigationReady(true, currentUid) + endUserSwitchLandedOn(username) } if (!loggedInLoaded || (loggedIn && !startupLoaded)) { diff --git a/shared/router-v2/tab-bar.desktop.tsx b/shared/router-v2/tab-bar.desktop.tsx index 4a3592c7adcf..4f769468c8d6 100644 --- a/shared/router-v2/tab-bar.desktop.tsx +++ b/shared/router-v2/tab-bar.desktop.tsx @@ -53,7 +53,12 @@ const Header = () => { const username = useCurrentUserState(s => s.username) const fullname = useUsersState(s => s.infoMap.get(username)?.fullname ?? '') - const logoutToLoggedOutFlow = useConfigState(s => s.dispatch.logoutToLoggedOutFlow) + const {logoutToLoggedOutFlow, userSwitching} = useConfigState( + C.useShallow(s => ({ + logoutToLoggedOutFlow: s.dispatch.logoutToLoggedOutFlow, + userSwitching: s.userSwitching, + })) + ) const onHelp = () => { void openURL('https://book.keybase.io') } const onQuit = () => { if (!__DEV__) { @@ -80,7 +85,8 @@ const Header = () => { const makePopup = (p: Kb.Popup2Parms) => { const {attachTo, hidePopup} = p const menuItems: Kb.MenuItems = [ - {onClick: onAddAccount, title: 'Log in as another user'}, + // the desktop menu only styles disabled items, so drop the handler too + {disabled: userSwitching, onClick: userSwitching ? undefined : onAddAccount, title: 'Log in as another user'}, {onClick: onSettings, title: 'Settings'}, {onClick: onHelp, title: 'Help'}, {danger: true, onClick: onSignOut, title: 'Sign out'}, @@ -254,19 +260,14 @@ function Tab(props: TabProps) { const isPeopleTab = index === 0 const {label} = Tabs.desktopTabMeta[tab] const current = useCurrentUserState(s => s.username) - const {login, setUserSwitching} = useConfigState( - C.useShallow(s => ({ - login: s.dispatch.login, - setUserSwitching: s.dispatch.setUserSwitching, - })) - ) + const switchToAccount = useConfigState(s => s.dispatch.switchToAccount) const onQuickSwitch = isPeopleTab ? () => { - const accountRows = useConfigState.getState().configuredAccounts + const {configuredAccounts: accountRows, userSwitching} = useConfigState.getState() + if (userSwitching) return const row = accountRows.find(a => a.username !== current && a.hasStoredSecret) if (row) { - setUserSwitching(true) - login(row.username, '') + switchToAccount(row.username) } else { onSelectTab(tab) } diff --git a/shared/router-v2/use-user-switch-nav-key.test.tsx b/shared/router-v2/use-user-switch-nav-key.test.tsx index 9acc547b40d7..4ba36b04e610 100644 --- a/shared/router-v2/use-user-switch-nav-key.test.tsx +++ b/shared/router-v2/use-user-switch-nav-key.test.tsx @@ -1,10 +1,23 @@ /** @jest-environment jsdom */ /// import {act, cleanup, renderHook} from '@testing-library/react' +import {navigationRef} from '@/constants/router' +import {useConfigState} from '@/stores/config' import {useCurrentUserState} from '@/stores/current-user' +import {useNavigationIntentsState} from '@/stores/navigation-intents' import {resetAllStores} from '@/util/zustand' import {useUserSwitchNavKey} from './use-user-switch-nav-key' +beforeEach(() => { + // the jest mock's container ref is a plain object, so stub the method the hook reads + ;(navigationRef as unknown as Record)['isReady'] = () => true +}) + +const readiness = () => { + const {navigationReady, navigationReadyForUid} = useNavigationIntentsState.getState() + return {navigationReady, navigationReadyForUid} +} + const setUsername = (username: string) => { act(() => { useCurrentUserState @@ -13,8 +26,24 @@ const setUsername = (username: string) => { }) } +const startSwitchTo = (username: string) => { + act(() => { + useConfigState.getState().dispatch.setUserSwitching(true, username) + }) +} + +// setLoggedIn(false) between the service's loggedOut and loggedIn notifications, and a logout, +// both run resetAllStores(), which blanks the current user +const blankCurrentUser = () => { + act(() => { + resetAllStores() + }) +} + afterEach(() => { cleanup() + // config's resetState carries the switch across resets, so end it explicitly + useConfigState.getState().dispatch.setUserSwitching(false) resetAllStores() }) @@ -50,3 +79,77 @@ test('an account switch that blanks username mid-flight still changes the nav ke setUsername('testuser-mac') expect(result.current).toBe('testuser-mac') }) + +test('a switch that lands back on the account the navigator shows ends the switch', () => { + setUsername('testuser') + const {result} = renderHook(() => useUserSwitchNavKey()) + blankCurrentUser() + startSwitchTo('testuser') + + setUsername('testuser') + + expect(result.current).toBe('') + expect(useConfigState.getState().userSwitching).toBe(false) +}) + +test('a first switch after starting logged out ends when its account arrives', () => { + const {result} = renderHook(() => useUserSwitchNavKey()) + startSwitchTo('testuser') + + setUsername('testuser') + + expect(result.current).toBe('') + expect(useConfigState.getState().userSwitching).toBe(false) +}) + +test('a stale username mid-switch does not end the switch, and the remount leaves it for onReady', () => { + setUsername('testuser') + const {result} = renderHook(() => useUserSwitchNavKey()) + startSwitchTo('testuser-mac') + blankCurrentUser() + + setUsername('testuser') + expect(result.current).toBe('') + expect(useConfigState.getState().userSwitching).toBe(true) + + setUsername('testuser-mac') + expect(result.current).toBe('testuser-mac') + expect(useConfigState.getState().userSwitching).toBe(true) +}) + +test('logging back in on the mounted navigator restores navigation readiness for that account', () => { + setUsername('testuser') + renderHook(() => useUserSwitchNavKey()) + // a logout's store reset clears readiness + blankCurrentUser() + expect(readiness().navigationReady).toBe(false) + + setUsername('testuser') + + expect(readiness()).toEqual({navigationReady: true, navigationReadyForUid: 'testuser'}) +}) + +test('a switch that lands on the mounted navigator ends only after readiness is back', () => { + setUsername('testuser') + renderHook(() => useUserSwitchNavKey()) + blankCurrentUser() + startSwitchTo('testuser') + let readyWhenSwitchEnded: boolean | undefined + const unsub = useConfigState.subscribe((s, p) => { + if (p.userSwitching && !s.userSwitching) { + readyWhenSwitchEnded = useNavigationIntentsState.getState().navigationReady + } + }) + + setUsername('testuser') + unsub() + + expect(readyWhenSwitchEnded).toBe(true) +}) + +test('the first render leaves navigation readiness to onReady', () => { + setUsername('testuser') + renderHook(() => useUserSwitchNavKey()) + + expect(readiness().navigationReady).toBe(false) +}) diff --git a/shared/router-v2/use-user-switch-nav-key.tsx b/shared/router-v2/use-user-switch-nav-key.tsx index 2b0cb8ed41ab..770838e73f98 100644 --- a/shared/router-v2/use-user-switch-nav-key.tsx +++ b/shared/router-v2/use-user-switch-nav-key.tsx @@ -1,21 +1,43 @@ import * as React from 'react' +import {navigationRef} from '@/constants/router' +import {useConfigState} from '@/stores/config' import {useCurrentUserState} from '@/stores/current-user' +import {useNavigationIntentsState} from '@/stores/navigation-intents' // Remount the navigator when switching between two logged-in users. // A switch arrives as 'a' → '' → 'b' because the mid-switch setLoggedIn(false) // resets all stores, so only ever compare against the last non-empty username. // Ignore '' → username (initial login) so in-flight unbox requests aren't interrupted. +// +// A remount's onReady marks navigation ready for the new account and ends the switch. A user who +// arrives without a remount gets no onReady, so this hook does both: +// - After a logout or the mid-switch reset, which clear navigation readiness, the mounted navigator +// now serves the arriving account, so mark it ready. Otherwise every deep link and notification +// intent stays queued. +// - End a switch that landed on the mounted navigator (e.g. logged out, then a notification tap for +// that same account), after readiness so the intent it replays can run. Match the switch's target +// rather than just "no remount": a stale username mid-switch must not end a switch still in flight. export const useUserSwitchNavKey = () => { const username = useCurrentUserState(s => s.username) const [navKey, setNavKey] = React.useState('') const prevUsernameRef = React.useRef(username) + const lastSeenUsernameRef = React.useRef(username) React.useEffect(() => { + const cameFromBlank = !lastSeenUsernameRef.current + lastSeenUsernameRef.current = username if (!username) return const prev = prevUsernameRef.current prevUsernameRef.current = username if (prev && prev !== username) { setNavKey(username) + return } + if (cameFromBlank && navigationRef.isReady()) { + useNavigationIntentsState + .getState() + .dispatch.setNavigationReady(true, useCurrentUserState.getState().uid) + } + useConfigState.getState().dispatch.endUserSwitchLandedOn(username) }, [username]) return navKey } diff --git a/shared/stores/config.tsx b/shared/stores/config.tsx index e2647e305a4c..6993140bd24a 100644 --- a/shared/stores/config.tsx +++ b/shared/stores/config.tsx @@ -17,8 +17,9 @@ import { niceError, } from "@/util/errors"; import { type CommonResponseHandler } from "@/engine/types"; +import { startNewAccountGeneration } from "@/engine/account-generation"; import { invalidPasswordErrorString } from "@/constants/config"; -import { navigateAppend } from "@/constants/router"; +import { navigateAppendOnceRootHas } from "@/constants/router"; import { onEngineConnected as onEngineConnectedInPlatform } from "@/util/storeless-actions"; import { useDaemonState } from "@/stores/daemon"; import { getEngine, hasEngine } from "@/engine/require"; @@ -57,6 +58,10 @@ type Store = T.Immutable<{ tab?: Tab; }; userSwitching: boolean; + // The account an in-progress switch is logging into ('' when none) + userSwitchingTo: string; + // Whether the in-progress switch started while logged in + userSwitchingFromLoggedIn: boolean; windowShownCount: Map; }>; @@ -92,12 +97,15 @@ const initialStore: Store = { loaded: false, }, userSwitching: false, + userSwitchingFromLoggedIn: false, + userSwitchingTo: "", windowShownCount: new Map(), }; export type State = Store & { dispatch: { checkForUpdate: () => void; + endUserSwitchLandedOn: (username: string) => void; initAppUpdateLoop: () => void; installerRan: () => void; loadIsOnline: () => void; @@ -124,7 +132,10 @@ export type State = Store & { setStartupDetails: (st: Omit) => void; setOutOfDate: (outOfDate: T.Config.OutOfDate) => void; setUpdating: () => void; - setUserSwitching: (sw: boolean) => void; + // Starting a switch names its target; switchToAccount is the one place that does. + setUserSwitching: (...args: [sw: true, to: string] | [sw: false]) => void; + // Starts a switch to a stored account unless one is already running. Returns whether it started. + switchToAccount: (username: string) => boolean; toggleRuntimeStats: () => void; updateGregorCategory: ( category: string, @@ -136,6 +147,10 @@ export type State = Store & { export const useConfigState = Z.createZustand("config", (set, get) => { let inflightRefreshAccounts: Promise | undefined; + // Bumped by every login. A login that fails after a newer one started (e.g. a tapped push for + // another account switching while a password login is still running) must not end the newer + // switch or show its own error. + let loginGeneration = 0; const _checkForUpdate = async () => { try { @@ -198,6 +213,14 @@ export const useConfigState = Z.createZustand("config", (set, get) => { }; ignorePromise(f()); }, + endUserSwitchLandedOn: (username) => { + // A navigator that comes up for an account a newer switch has already moved past (the user + // picked another account mid-switch) must leave that switch running. + const { userSwitching, userSwitchingTo } = get(); + if (userSwitching && userSwitchingTo === username) { + get().dispatch.setUserSwitching(false); + } + }, initAppUpdateLoop: () => { const f = async () => { while (true) { @@ -239,6 +262,8 @@ export const useConfigState = Z.createZustand("config", (set, get) => { }); }; const ignoreCallback = () => {}; + const generation = ++loginGeneration; + const superseded = () => generation !== loginGeneration; const f = async () => { try { await T.RPCGen.loginLoginRpcListener({ @@ -248,8 +273,12 @@ export const useConfigState = Z.createZustand("config", (set, get) => { "keybase.1.provisionUi.DisplayAndPromptSecret": cancelOnCallback, "keybase.1.provisionUi.PromptNewDeviceName": (_, response) => { cancelOnCallback(undefined, response); - // this account needs provisioning; hand off to the provision flow - navigateAppend({ + if (superseded()) return; + // This account needs provisioning; hand off to the provision flow. 'username' lives in + // the logged-out stack, which the routers keep unmounted while userSwitching is set, so + // end the switch and push once that stack is up. + get().dispatch.setUserSwitching(false); + navigateAppendOnceRootHas("loggedOut", { name: "username", params: { autoSubmit: true, username }, }); @@ -263,6 +292,7 @@ export const useConfigState = Z.createZustand("config", (set, get) => { // Service asking us again due to a bad passphrase? if (params.pinentry.retryLabel) { cancelOnCallback(params, response); + if (superseded()) return; let retryLabel = params.pinentry.retryLabel; if (retryLabel === invalidPasswordErrorString) { retryLabel = "Incorrect password."; @@ -299,6 +329,13 @@ export const useConfigState = Z.createZustand("config", (set, get) => { }); logger.info("login call succeeded"); } catch (error) { + if (superseded()) { + logger.info( + "login failed after a newer login started, ignoring", + error, + ); + return; + } // Nothing else ends a cancelled switch, and the logged-out status it withheld applies only then if (!(error instanceof RPCError) || error.desc === cancelDesc) { get().dispatch.setUserSwitching(false); @@ -474,6 +511,8 @@ export const useConfigState = Z.createZustand("config", (set, get) => { httpSrv: s.httpSrv, startup: { loaded: s.startup.loaded }, userSwitching: s.userSwitching, + userSwitchingFromLoggedIn: s.userSwitchingFromLoggedIn, + userSwitchingTo: s.userSwitchingTo, })); }, revoke: (name, wasCurrentDevice) => { @@ -553,6 +592,9 @@ export const useConfigState = Z.createZustand("config", (set, get) => { }, setLoggedIn: (loggedIn) => { const changed = get().loggedIn !== loggedIn; + if (changed && !loggedIn) { + startNewAccountGeneration(); + } set((s) => { s.loggedIn = loggedIn; }); @@ -589,16 +631,34 @@ export const useConfigState = Z.createZustand("config", (set, get) => { s.outOfDate.updating = true; }); }, - setUserSwitching: (sw) => { - if (sw && !get().userSwitching) { + setUserSwitching: (...args) => { + const [sw, to] = args; + // Read before the reset below, which clears loggedIn + const fromLoggedIn = sw && get().loggedIn; + const starting = sw && !get().userSwitching; + // Set before the reset, which keeps these: a subscriber that sees loggedIn go false must + // already see the switch, or it reads the reset as a logout. + set((s) => { + s.userSwitching = sw; + s.userSwitchingFromLoggedIn = fromLoggedIn; + s.userSwitchingTo = sw ? to : ""; + }); + if (starting) { + // The reset logs the old account out of our stores without going through setLoggedIn + if (fromLoggedIn) { + startNewAccountGeneration(); + } Z.resetAllStores(); if (hasEngine()) { getEngine().cancelOutstandingSessions(); } } - set((s) => { - s.userSwitching = sw; - }); + }, + switchToAccount: (username) => { + if (get().userSwitching) return false; + get().dispatch.setUserSwitching(true, username); + get().dispatch.login(username, ""); + return true; }, toggleRuntimeStats: () => { const f = async () => { diff --git a/shared/stores/settings-contacts.tsx b/shared/stores/settings-contacts.tsx index b1a871e2c09a..742facd563b7 100644 --- a/shared/stores/settings-contacts.tsx +++ b/shared/stores/settings-contacts.tsx @@ -189,6 +189,11 @@ export const useSettingsContactsState = Z.createZustand('settings-contact }, manageContactsCache: () => { const f = async () => { + // The import setting read below is this account's. Reading the address book can take + // seconds, and the upload goes to whichever account is logged in when it runs, so an + // account switch in between must not upload these contacts to the next account. + const uidBefore = useCurrentUserState.getState().uid + const accountChanged = () => useCurrentUserState.getState().uid !== uidBefore if (get().importEnabled === false) { await T.RPCGen.contactsSaveContactListRpcPromise({contacts: []}) set(s => { @@ -232,12 +237,19 @@ export const useSettingsContactsState = Z.createZustand('settings-contact } catch (_error) { const error = _error as {message: string} logger.error(`error loading contacts: ${error.message}`) + if (accountChanged()) { + return + } set(s => { s.importedCount = undefined s.importError = error.message }) return } + if (accountChanged()) { + logger.info('account changed while reading contacts, not importing') + return + } logger.info(`Importing ${mapped.length} contacts.`) try { const {newlyResolved, resolved} = await T.RPCGen.contactsSaveContactListRpcPromise({ @@ -268,6 +280,10 @@ export const useSettingsContactsState = Z.createZustand('settings-contact } catch (_error) { const error = _error as {message: string} logger.error('Error saving contacts list: ', error.message) + // includes the engine refusing the reply because a switch landed during the upload + if (accountChanged()) { + return + } set(s => { s.importedCount = undefined s.importError = error.message diff --git a/shared/stores/tests/config.test.ts b/shared/stores/tests/config.test.ts index c58f7d52c7ee..ab6ae54c5d92 100644 --- a/shared/stores/tests/config.test.ts +++ b/shared/stores/tests/config.test.ts @@ -1,7 +1,14 @@ /// +jest.mock('@/constants/router', () => ({ + ...jest.requireActual('@/constants/router'), + navigateAppendOnceRootHas: jest.fn(), +})) + import * as T from '../../constants/types' import * as Tabs from '../../constants/tabs' +import {navigateAppendOnceRootHas} from '../../constants/router' import {RPCError} from '../../util/errors' +import {getAccountGeneration} from '../../engine/account-generation' import {useDaemonState} from '../daemon' import {noConversationIDKey} from '../../constants/types/chat/common' import {useConfigState} from '../config' @@ -23,6 +30,8 @@ const resetConfigState = () => { loaded: false, }, userSwitching: false, + userSwitchingFromLoggedIn: false, + userSwitchingTo: '', } as any) dispatch.resetState() } @@ -144,6 +153,7 @@ test('loggedIn and loggedOut notifications do not set the session themselves', ( }) describe('login', () => { + const mockOnceRootHas = jest.mocked(navigateAppendOnceRootHas) const originalDaemonDispatch = useDaemonState.getState().dispatch let refresh: jest.Mock beforeEach(() => { @@ -153,6 +163,7 @@ describe('login', () => { afterEach(() => { jest.restoreAllMocks() useDaemonState.setState({dispatch: originalDaemonDispatch}) + mockOnceRootHas.mockReset() }) const flush = async () => new Promise(resolve => setImmediate(resolve)) @@ -185,11 +196,188 @@ describe('login', () => { .mockRejectedValue(new RPCError('bad password', T.RPCGen.StatusCode.scgeneric)) const switchingWhenRead: Array = [] refresh.mockImplementation(() => switchingWhenRead.push(useConfigState.getState().userSwitching)) - useConfigState.getState().dispatch.setUserSwitching(true) + useConfigState.getState().dispatch.setUserSwitching(true, 'testuser') useConfigState.getState().dispatch.login('testuser', 'password') await flush() expect(switchingWhenRead).toEqual([false]) expect(useConfigState.getState().loginError).toBeDefined() }) + + const switchWithLoginFailure = async (failure: unknown) => { + jest.spyOn(T.RPCGen, 'loginLoginRpcListener').mockRejectedValue(failure) + const {dispatch} = useConfigState.getState() + dispatch.setUserSwitching(true, 'testuser') + dispatch.login('testuser', '') + await flush() + } + + test('an account that needs provisioning ends the switch before handing off to username', async () => { + let switchingAtHandOff: boolean | undefined + mockOnceRootHas.mockImplementation(() => { + switchingAtHandOff = useConfigState.getState().userSwitching + }) + const cancelled = jest.fn().mockRejectedValue(new RPCError('Canceling RPC', T.RPCGen.StatusCode.scgeneric)) + jest.spyOn(T.RPCGen, 'loginLoginRpcListener').mockImplementation(listener => { + const prompt = (listener as any).customResponseIncomingCallMap['keybase.1.provisionUi.PromptNewDeviceName'] + prompt({}, {error: jest.fn(), result: jest.fn()}) + return cancelled() + }) + const {dispatch} = useConfigState.getState() + dispatch.setUserSwitching(true, 'testuser') + dispatch.login('testuser', '') + await flush() + + expect(mockOnceRootHas).toHaveBeenCalledWith('loggedOut', { + name: 'username', + params: {autoSubmit: true, username: 'testuser'}, + }) + expect(switchingAtHandOff).toBe(false) + }) + + test('a prompt login cancelled itself clears userSwitching without a login error', async () => { + await switchWithLoginFailure(new RPCError('Canceling RPC', T.RPCGen.StatusCode.scgeneric)) + + const state = useConfigState.getState() + expect(state.userSwitching).toBe(false) + expect(state.loginError).toBeUndefined() + }) + + test('a failure that is not an RPCError clears userSwitching', async () => { + await switchWithLoginFailure(new Error('boom')) + + expect(useConfigState.getState().userSwitching).toBe(false) + }) + + test("a login that fails after a switch started leaves the switch alone, and the newer one's failure still ends it", async () => { + const rejects: Array<(e: unknown) => void> = [] + jest.spyOn(T.RPCGen, 'loginLoginRpcListener').mockImplementation( + async () => new Promise((_resolve, reject) => rejects.push(reject)) + ) + const {dispatch} = useConfigState.getState() + dispatch.login('testuser', 'password') + await flush() + // e.g. a tapped push for another account while a password login is still running + dispatch.switchToAccount('testuser-mac') + await flush() + + rejects[0]?.(new RPCError('bad things', T.RPCGen.StatusCode.scgeneric)) + await flush() + let state = useConfigState.getState() + expect(state.userSwitching).toBe(true) + expect(state.userSwitchingTo).toBe('testuser-mac') + expect(state.loginError).toBeUndefined() + + rejects[1]?.(new RPCError('bad things', T.RPCGen.StatusCode.scgeneric)) + await flush() + state = useConfigState.getState() + expect(state.userSwitching).toBe(false) + expect(state.loginError?.desc).toBeTruthy() + }) + + test('prompts that arrive for a login after a switch started do not end the switch', async () => { + const listeners: Array = [] + jest.spyOn(T.RPCGen, 'loginLoginRpcListener').mockImplementation(async listener => { + listeners.push(listener) + return new Promise(() => {}) + }) + const {dispatch} = useConfigState.getState() + dispatch.login('testuser', 'password') + await flush() + // e.g. a tapped push for another account while a password login is still running + dispatch.switchToAccount('testuser-mac') + await flush() + + const response = () => ({error: jest.fn(), result: jest.fn()}) + const stale = listeners[0].customResponseIncomingCallMap + stale['keybase.1.provisionUi.PromptNewDeviceName']({}, response()) + stale['keybase.1.secretUi.getPassphrase']( + {pinentry: {retryLabel: 'Incorrect password.', type: T.RPCGen.PassphraseType.passPhrase}}, + response() + ) + + const state = useConfigState.getState() + expect(mockOnceRootHas).not.toHaveBeenCalled() + expect(state.userSwitching).toBe(true) + expect(state.loginError).toBeUndefined() + }) + + test('an RPC error clears userSwitching and records the login error', async () => { + await switchWithLoginFailure(new RPCError('bad things', T.RPCGen.StatusCode.scgeneric)) + + const state = useConfigState.getState() + expect(state.userSwitching).toBe(false) + expect(state.loginError?.desc).toBeTruthy() + }) +}) + +test('a navigator ready for the switch target ends the switch, one ready for a superseded target does not', () => { + const {dispatch} = useConfigState.getState() + + dispatch.setUserSwitching(true, 'testuser-mac') + dispatch.endUserSwitchLandedOn('testuser') + expect(useConfigState.getState().userSwitching).toBe(true) + + dispatch.endUserSwitchLandedOn('testuser-mac') + expect(useConfigState.getState().userSwitching).toBe(false) +}) + +test("setUserSwitching records the switch's target, clears it with the flag, and keeps it across resets", () => { + const {dispatch} = useConfigState.getState() + + dispatch.setUserSwitching(true, 'testuser') + dispatch.resetState() + expect(useConfigState.getState().userSwitchingTo).toBe('testuser') + + dispatch.setUserSwitching(false) + expect(useConfigState.getState().userSwitchingTo).toBe('') +}) + +test('setUserSwitching records whether the switch started logged in, through the mid-switch reset', () => { + const {dispatch} = useConfigState.getState() + + dispatch.setUserSwitching(true, 'testuser') + expect(useConfigState.getState().userSwitchingFromLoggedIn).toBe(false) + + dispatch.setLoggedIn(true) + dispatch.setUserSwitching(true, 'testuser') + // the service's loggedOut notification during a switch resets every store + dispatch.setLoggedIn(false) + expect(useConfigState.getState().userSwitchingFromLoggedIn).toBe(true) + + dispatch.setUserSwitching(false) + expect(useConfigState.getState().userSwitchingFromLoggedIn).toBe(false) +}) + +test('switchToAccount starts a switch to its target and logs in, and refuses while one is running', () => { + const loginSpy = jest.spyOn(T.RPCGen, 'loginLoginRpcListener').mockReturnValue(new Promise(() => {})) + const {dispatch} = useConfigState.getState() + + expect(dispatch.switchToAccount('testuser')).toBe(true) + expect(useConfigState.getState().userSwitchingTo).toBe('testuser') + expect(loginSpy).toHaveBeenCalledTimes(1) + + expect(dispatch.switchToAccount('testuser-mac')).toBe(false) + expect(useConfigState.getState().userSwitchingTo).toBe('testuser') + expect(loginSpy).toHaveBeenCalledTimes(1) + + loginSpy.mockRestore() + dispatch.setUserSwitching(false) +}) + +test('a logout starts a new account generation before anything reacts to it', () => { + const {dispatch} = useConfigState.getState() + dispatch.setLoggedIn(true) + const before = getAccountGeneration() + let seenByReaction: number | undefined + const unsub = useConfigState.subscribe((s, old) => { + if (old.loggedIn && !s.loggedIn) { + seenByReaction = getAccountGeneration() + } + }) + + dispatch.setLoggedIn(false) + unsub() + + expect(seenByReaction).toBe(before + 1) }) diff --git a/shared/stores/tests/daemon.test.ts b/shared/stores/tests/daemon.test.ts index bf5a00998a8c..9537719865a8 100644 --- a/shared/stores/tests/daemon.test.ts +++ b/shared/stores/tests/daemon.test.ts @@ -107,8 +107,8 @@ describe('daemon store', () => { }) test('startHandshake does not reuse a load orphaned by an engine reset', async () => { - // engine.reset() drops in-flight RPCs without settling their promises (user switch does - // this twice); a later handshake must start a fresh load instead of awaiting the dead one + // engine.reset() drops in-flight RPCs without settling their promises; a later handshake + // must start a fresh load instead of awaiting the dead one const spy = jest .spyOn(T.RPCGen, 'configGetBootstrapStatusRpcPromise') .mockImplementationOnce(async () => new Promise(() => {})) diff --git a/shared/stores/tests/settings-contacts.mobile.test.ts b/shared/stores/tests/settings-contacts.mobile.test.ts new file mode 100644 index 000000000000..aa9c3f4967c7 --- /dev/null +++ b/shared/stores/tests/settings-contacts.mobile.test.ts @@ -0,0 +1,75 @@ +/// +// The contacts store is a no-op on desktop, so load it as mobile. +import type * as ContactsStore from '../settings-contacts' +import type * as CurrentUser from '../current-user' +import type * as TT from '@/constants/types' + +// Jest maps every native-only package (expo-contacts, expo-localization, react-native-kb) to one +// stub, so this mock stands in for all three. +const mockGetAllDetails = jest.fn() +jest.mock('../../test/mocks/native-module', () => ({ + Contact: {getAllDetails: (...args: Array) => mockGetAllDetails(...args)}, + ContactField: {EMAILS: 'emails', FULL_NAME: 'fullName', PHONES: 'phones'}, + PermissionStatus: {GRANTED: 'granted'}, + addNotificationRequest: async () => Promise.resolve(), + getLocales: () => [{regionCode: 'US'}], + getPermissionsAsync: async () => Promise.resolve({status: 'granted'}), + requireNativeModule: () => ({}), + requireOptionalNativeModule: () => null, +})) + +const g = globalThis as {isMobile?: boolean} +let store: typeof ContactsStore +let currentUser: typeof CurrentUser +let T: typeof TT + +beforeEach(() => { + g.isMobile = true + jest.isolateModules(() => { + store = require('../settings-contacts') + currentUser = require('../current-user') + T = require('@/constants/types') + }) + currentUser.useCurrentUserState + .getState() + .dispatch.setBootstrap({deviceID: 'd', deviceName: 'dn', uid: 'uid-1', username: 'testuser'}) + store.useSettingsContactsState.setState({importEnabled: true, permissionStatus: 'granted'}) +}) + +afterEach(() => { + g.isMobile = false + jest.restoreAllMocks() +}) + +const flush = async () => { + for (let i = 0; i < 10; i++) await Promise.resolve() +} + +test('uploads the address book for the account that enabled import', async () => { + mockGetAllDetails.mockResolvedValue([{fullName: 'a', phoneNumbers: [{number: '+15555550100'}]}]) + const save = jest + .spyOn(T.RPCGen, 'contactsSaveContactListRpcPromise') + .mockResolvedValue({newlyResolved: [], resolved: []} as never) + + store.useSettingsContactsState.getState().dispatch.manageContactsCache() + await flush() + + expect(store.useSettingsContactsState.getState().importError).toBe('') + expect(save).toHaveBeenCalledTimes(1) +}) + +test('does not upload to the next account when a switch lands while the address book is read', async () => { + let finishReading: (c: unknown) => void = () => {} + mockGetAllDetails.mockImplementation(async () => new Promise(resolve => (finishReading = resolve))) + const save = jest.spyOn(T.RPCGen, 'contactsSaveContactListRpcPromise') + + store.useSettingsContactsState.getState().dispatch.manageContactsCache() + await flush() + currentUser.useCurrentUserState + .getState() + .dispatch.setBootstrap({deviceID: 'd2', deviceName: 'dn2', uid: 'uid-2', username: 'testuser-mac'}) + finishReading([{fullName: 'a', phoneNumbers: [{number: '+15555550100'}]}]) + await flush() + + expect(save).not.toHaveBeenCalled() +}) diff --git a/shared/stores/tests/waiting.test.ts b/shared/stores/tests/waiting.test.ts index 3a16967aeb3f..18e27e7a9d4c 100644 --- a/shared/stores/tests/waiting.test.ts +++ b/shared/stores/tests/waiting.test.ts @@ -36,3 +36,17 @@ test('batch applies a mixed waiting update set', () => { expect((useWaitingState.getState().counts.get('b') ?? 0) > 0).toBe(true) expect((useWaitingState.getState().counts.get('c') ?? 0) > 0).toBe(true) }) + +test('a logout keeps in-flight counts so the calls that end afterwards bring them back to zero', () => { + const {dispatch} = useWaitingState.getState() + const error = new RPCError('boom', 7) + dispatch.increment('load') + dispatch.decrement('other', error) + + resetAllStores() + + expect(useWaitingState.getState().errors.get('other')).toBeUndefined() + expect(useWaitingState.getState().counts.get('load')).toBe(1) + dispatch.decrement('load') + expect(useWaitingState.getState().counts.get('load')).toBeUndefined() +}) diff --git a/shared/stores/waiting.tsx b/shared/stores/waiting.tsx index e1bfe8efdf89..fb0aeeb71e25 100644 --- a/shared/stores/waiting.tsx +++ b/shared/stores/waiting.tsx @@ -73,7 +73,14 @@ export const useWaitingState = Z.createZustand('waiting', (set, get) => { increment: keys => { changeHelper(keys, 1) }, - resetState: Z.defaultReset, + // Counts track calls still in flight, and every one of those decrements its count when it + // settles, so a logout keeps them: clearing them would send the count negative when those calls + // end. Errors belong to the account's screens and go. + resetState: () => { + set(s => { + s.errors.clear() + }) + }, } return { diff --git a/shared/util/storeless-actions.test.ts b/shared/util/storeless-actions.test.ts new file mode 100644 index 000000000000..de75327c1c37 --- /dev/null +++ b/shared/util/storeless-actions.test.ts @@ -0,0 +1,48 @@ +/// +import * as T from '@/constants/types' +import {useCurrentUserState} from '@/stores/current-user' +import {resetAllStores} from '@/util/zustand' +import {persistRoute} from './storeless-actions' + +// persistRoute skips a route it already saved, so every test shows a different conversation +let mockConversation = '' +jest.mock('@/constants/router', () => ({ + getTab: () => 'tabs.chatTab', + getVisiblePath: () => [{name: 'chatConversation', params: {conversationIDKey: mockConversation}}], +})) + +const g = globalThis as {isMobile?: boolean} +const setUser = (uid: string, username: string) => + useCurrentUserState.getState().dispatch.setBootstrap({deviceID: 'd', deviceName: 'dn', uid, username}) + +beforeEach(() => { + g.isMobile = true + jest.useFakeTimers() +}) + +afterEach(() => { + g.isMobile = false + jest.useRealTimers() + jest.restoreAllMocks() + resetAllStores() +}) + +const persistedAfterDelay = async (switchAccount: boolean) => { + mockConversation = `conv-${String(switchAccount)}` + const setValue = jest.spyOn(T.RPCGen, 'configGuiSetValueRpcPromise').mockResolvedValue(undefined) + setUser('uid-1', 'testuser') + persistRoute(false, false, () => true) + if (switchAccount) { + setUser('uid-2', 'testuser-mac') + } + await jest.advanceTimersByTimeAsync(1000) + return setValue.mock.calls.map(c => JSON.parse(c[0].value.s ?? '') as {uid: string}) +} + +test('persists the route on screen under the account it belongs to', async () => { + expect(await persistedAfterDelay(false)).toEqual([expect.objectContaining({uid: 'uid-1'})]) +}) + +test('drops a delayed persist when an account switch lands first', async () => { + expect(await persistedAfterDelay(true)).toEqual([]) +}) diff --git a/shared/util/storeless-actions.tsx b/shared/util/storeless-actions.tsx index 0b27e52d635e..aa4056bd480a 100644 --- a/shared/util/storeless-actions.tsx +++ b/shared/util/storeless-actions.tsx @@ -82,10 +82,17 @@ export const persistRoute = (clear: boolean, immediate: boolean, isStartupLoaded } catch {} } + // The account this persist was asked for. Across an account switch the previous account's screens + // stay up briefly, so a delayed persist that runs after the switch would save their route under + // the next account's uid and restore a conversation that is not that account's on the next launch. + const uidAtRequest = useCurrentUserState.getState().uid const doPersist = async () => { if (!isStartupLoaded()) { return } + if (useCurrentUserState.getState().uid !== uidAtRequest) { + return + } let param = {} let routeName = peopleTab const cur = getTab() @@ -101,11 +108,10 @@ export const persistRoute = (clear: boolean, immediate: boolean, isStartupLoaded } return false }) - // Stamp the persisted route with the current uid. ui.routeState2 is stored + // Stamp the persisted route with its account's uid. ui.routeState2 is stored // device-globally (not per-account), so on startup we must only restore a // conversation that belongs to the account we end up logged in as. - const {uid} = useCurrentUserState.getState() - const next = JSON.stringify({param, routeName, uid}) + const next = JSON.stringify({param, routeName, uid: uidAtRequest}) if (lastPersist === next) { return } diff --git a/shared/util/use-debounce.test.tsx b/shared/util/use-debounce.test.tsx index 1b9d85c349d6..a7bd185fe96d 100644 --- a/shared/util/use-debounce.test.tsx +++ b/shared/util/use-debounce.test.tsx @@ -287,3 +287,31 @@ test('useThrottledCallback collapses repeated calls within the wait window to th expect(callback).toHaveBeenCalledTimes(2) expect(callback).toHaveBeenNthCalledWith(2, 'gamma') }) + +test('useThrottledCallback drops a pending trailing call on unmount by default', () => { + const callback = jest.fn((value: string) => value) + const {result, unmount} = renderHook(() => useThrottledCallback(callback, 100)) + act(() => { + result.current('alpha') + result.current('beta') + }) + + unmount() + advance(100) + + expect(callback.mock.calls).toEqual([['alpha']]) +}) + +test('useThrottledCallback runs a pending trailing call on unmount with flushOnUnmount', () => { + const callback = jest.fn((value: string) => value) + const {result, unmount} = renderHook(() => useThrottledCallback(callback, 100, {flushOnUnmount: true})) + act(() => { + result.current('alpha') + result.current('beta') + }) + + unmount() + advance(100) + + expect(callback.mock.calls).toEqual([['alpha'], ['beta']]) +}) diff --git a/shared/util/use-debounce.tsx b/shared/util/use-debounce.tsx index 669b31e872ae..4fcd2e4722c0 100644 --- a/shared/util/use-debounce.tsx +++ b/shared/util/use-debounce.tsx @@ -22,6 +22,12 @@ type DebounceOptions = { trailing?: boolean } +type ThrottleOptions = DebounceOptions & { + // Run a pending trailing call on unmount instead of dropping it. A component can't do this in its + // own cleanup: React runs cleanups in declaration order, so this hook's cancel would run first. + flushOnUnmount?: boolean +} + const normalizeWait = (wait?: number) => Math.max(0, wait ?? 0) export function useDebouncedCallback( @@ -149,7 +155,7 @@ export function useDebouncedCallback( export function useThrottledCallback( func: T, wait: number, - options?: DebounceOptions + options?: ThrottleOptions ): DebouncedState { const funcRef = React.useRef(func) React.useLayoutEffect(() => { @@ -165,6 +171,7 @@ export function useThrottledCallback( const waitMs = normalizeWait(wait) const leading = options?.leading ?? true const trailing = options?.trailing ?? true + const flushOnUnmount = options?.flushOnUnmount ?? false const throttled = React.useMemo(() => { const clearTimer = () => { @@ -250,9 +257,13 @@ export function useThrottledCallback( React.useLayoutEffect(() => { runtimeRef.current = {} return () => { - throttled.cancel() + if (flushOnUnmount) { + throttled.flush() + } else { + throttled.cancel() + } } - }, [throttled]) + }, [throttled, flushOnUnmount]) return throttled }