From a3374617a5b6f55f986cb21d573d4682a706de36 Mon Sep 17 00:00:00 2001 From: chrisnojima Date: Mon, 21 Sep 2026 10:09:24 -0400 Subject: [PATCH 1/3] fix(links): route devices and /phone-app through the linking config keybase://devices had no handler at all: it fell through handleKeybaseLink's switch and did nothing. Give it one in both places a URL can arrive, branched per platform the way devices/index.tsx and device-revoke.tsx already do -- devices live under Settings on phone and tablet, and in their own tab on desktop. The linking config builds launch state for it like every other known link; handleKeybaseLink has to agree, because on desktop router.tsx passes it as the linking subscription's listener as well as its fallback, so every URL there lands in handleKeybaseLink. Nothing carved phone-app out of normalizeHttpUrl's single-segment username rule, so our own invite install link (https://keybase.io/phone-app) resolved to a profile for a user that does not exist. It now normalizes to keybase://settingsAddPhone, which the linking config opens as a modal over the settings tab and handleKeybaseLink opens the same way. That was config.startup.link's only consumer, so the field, its setStartupDetails sites and the bespoke once-per-process check in load-settings go with it. The launch-URL read stays: it still decides whether the saved route may be restored. Also two settings load fixes. Anything that writes the email/phone stores while userLoadMySettings is in flight knows something the reply does not, so each half of the reply is applied only to the value it was read against -- a notification that lands mid-load is no longer overwritten. And loggedIn is re-read after the await: a logout cannot trip those identity checks, because Z.defaultReset restores the values captured at store creation, so without it the reply would repopulate the stores for a logged-out app and the next account could read the previous one's settings. --- shared/constants/deeplinks.test.ts | 61 ++++++++++++++++++++ shared/constants/deeplinks.tsx | 21 ++++++- shared/constants/init/index.tsx | 12 ++-- shared/router-v2/deep-link-emitter.tsx | 9 +++ shared/router-v2/linking-initial-url.test.ts | 2 - shared/router-v2/linking-state.test.ts | 11 ++++ shared/router-v2/linking.test.ts | 57 +++++++++++++++++- shared/router-v2/linking.tsx | 32 ++++++++-- shared/router-v2/url-normalize.test.ts | 13 +++++ shared/settings/load-settings.tsx | 41 +++++++------ shared/stores/config.tsx | 2 - shared/stores/tests/config.test.ts | 4 -- shared/stores/tests/settings.test.ts | 56 ++++++++++++++++-- 13 files changed, 271 insertions(+), 50 deletions(-) create mode 100644 shared/constants/deeplinks.test.ts diff --git a/shared/constants/deeplinks.test.ts b/shared/constants/deeplinks.test.ts new file mode 100644 index 000000000000..d8c6b83c8f9b --- /dev/null +++ b/shared/constants/deeplinks.test.ts @@ -0,0 +1,61 @@ +/// +jest.mock('./router', () => ({ + navUpToScreen: jest.fn(), + navigateAppend: jest.fn(), + navigateToThread: jest.fn(), + navToProfile: jest.fn(), + previewConversation: jest.fn(), + switchTab: jest.fn(), +})) +jest.mock('@/teams/team-page-actions', () => ({showTeamByName: jest.fn()})) +import * as Router from './router' +import * as Tabs from './tabs' +import {settingsDevicesTab} from './settings' +import {handleAppLink} from './deeplinks' + +const withIsMobile = (isMobile: boolean, f: () => void) => { + const was = global.isMobile + global.isMobile = isMobile + try { + f() + } finally { + global.isMobile = was + } +} + +beforeEach(() => { + jest.clearAllMocks() +}) + +// On desktop handleAppLink IS the linking subscription's listener (router.tsx passes it as +// both listener and fallback), so this case is the whole implementation there. +test('a devices link opens the devices tab on desktop', () => { + withIsMobile(false, () => { + handleAppLink('keybase://devices') + + expect(Router.switchTab).toHaveBeenCalledWith(Tabs.devicesTab) + expect(Router.navUpToScreen).toHaveBeenCalledWith('devicesRoot') + expect(Router.navigateAppend).not.toHaveBeenCalled() + }) +}) + +test('a devices link opens the devices screen under settings on mobile', () => { + withIsMobile(true, () => { + handleAppLink('keybase://devices') + + expect(Router.switchTab).toHaveBeenCalledWith(Tabs.settingsTab) + expect(Router.navUpToScreen).toHaveBeenCalledWith(settingsDevicesTab) + expect(Router.navigateAppend).not.toHaveBeenCalled() + }) +}) + +// The invite install link normalizes to this; the linking config handles it on mobile, but +// desktop routes every URL through here, so both have to agree on where it goes. +test('an add-phone link opens the add-phone modal over settings', () => { + withIsMobile(false, () => { + handleAppLink('keybase://settingsAddPhone') + + expect(Router.switchTab).toHaveBeenCalledWith(Tabs.settingsTab) + expect(Router.navigateAppend).toHaveBeenCalledWith({name: 'settingsAddPhone', params: {}}) + }) +}) diff --git a/shared/constants/deeplinks.tsx b/shared/constants/deeplinks.tsx index df62b988a6c1..06f27fb0d213 100644 --- a/shared/constants/deeplinks.tsx +++ b/shared/constants/deeplinks.tsx @@ -1,7 +1,15 @@ import logger from '@/logger' import * as T from '@/constants/types' -import {navigateAppend, navigateToThread, navToProfile, previewConversation, switchTab} from './router' +import { + navigateAppend, + navigateToThread, + navToProfile, + navUpToScreen, + previewConversation, + switchTab, +} from './router' import * as Tabs from './tabs' +import {settingsDevicesTab} from './settings' import {showTeamByName} from '@/teams/team-page-actions' const prefix = 'keybase://' @@ -75,6 +83,17 @@ const handleKeybaseLink = (link: string) => { return } break + case 'devices': + // Devices live under Settings on phone/tablet and in their own tab on desktop. + switchTab(isMobile ? Tabs.settingsTab : Tabs.devicesTab) + navUpToScreen(isMobile ? settingsDevicesTab : 'devicesRoot') + return + case 'settingsAddPhone': + // Where the invite install link (https://keybase.io/phone-app) lands. The linking config + // also handles it; desktop routes every URL here, so this must agree with it. + switchTab(Tabs.settingsTab) + navigateAppend({name: 'settingsAddPhone', params: {}}) + return case 'private': case 'public': try { diff --git a/shared/constants/init/index.tsx b/shared/constants/init/index.tsx index 51c81055da2d..88b76432ee8e 100644 --- a/shared/constants/init/index.tsx +++ b/shared/constants/init/index.tsx @@ -158,7 +158,6 @@ const loadStartupDetails = async () => { let conversation: T.Chat.ConversationIDKey | undefined let conversationUid = '' let followUser = '' - let link = '' let tab = '' // Top priority, push @@ -166,11 +165,10 @@ const loadStartupDetails = async () => { logger.info('initialState: push', push.startupConversation, push.startupFollowUser) conversation = push.startupConversation followUser = push.startupFollowUser ?? '' - } else if (initialUrl) { - // Second priority, deep link - link = initialUrl - } else if (routeState) { - // Last priority, saved from last session + } else if (!initialUrl && routeState) { + // Last priority, saved from last session. The linking config reads the launch URL + // itself; this read only decides whether the saved route may be restored, since a + // launch URL outranks it. try { const item = JSON.parse(routeState) as | undefined @@ -203,7 +201,6 @@ const loadStartupDetails = async () => { conversation: conversation ?? noConversationIDKey, conversationUid, followUser, - link, tab: tab as Tabs.Tab, }) @@ -592,7 +589,6 @@ const _initDesktopPlatformListener = () => { useConfigState.getState().dispatch.setStartupDetails({ conversation: Chat.noConversationIDKey, followUser: '', - link: '', tab: undefined, }) } diff --git a/shared/router-v2/deep-link-emitter.tsx b/shared/router-v2/deep-link-emitter.tsx index e3a39b62f4f2..441b1cb77e80 100644 --- a/shared/router-v2/deep-link-emitter.tsx +++ b/shared/router-v2/deep-link-emitter.tsx @@ -40,6 +40,15 @@ const normalizeHttpUrl = (url: string): string | undefined => { : `keybase://team-page/${teamName}` } + // /phone-app — the install link our own chat invite banner texts to an unresolved @phone + // participant (chat/conversation/bottom-banner.tsx). It is not a username, so it has to be + // carved out ahead of the single-segment rule below, which would otherwise open a profile + // for a user that does not exist. It always opens Add Phone Number: the invitee's inviter + // wrote to a number, and nothing here knows (or waits to learn) whether they have one. + if (pathname === '/phone-app' || pathname === '/phone-app/') { + return 'keybase://settingsAddPhone' + } + // /username (single path segment) const userMatch = pathname.match(/^\/((?:[a-zA-Z0-9][a-zA-Z0-9_-]?)+)\/?$/) if (userMatch?.[1]) { diff --git a/shared/router-v2/linking-initial-url.test.ts b/shared/router-v2/linking-initial-url.test.ts index 4930318e5da8..250735e6c62c 100644 --- a/shared/router-v2/linking-initial-url.test.ts +++ b/shared/router-v2/linking-initial-url.test.ts @@ -21,7 +21,6 @@ type Startup = { conversation: T.Chat.ConversationIDKey conversationUid?: string followUser: string - link: string tab?: Tabs.Tab } @@ -32,7 +31,6 @@ const setStartup = (st: Partial) => { startup: { conversation: T.Chat.noConversationIDKey, followUser: '', - link: '', loaded: true, ...st, }, diff --git a/shared/router-v2/linking-state.test.ts b/shared/router-v2/linking-state.test.ts index 72e211644714..ce2c3ad5e3a7 100644 --- a/shared/router-v2/linking-state.test.ts +++ b/shared/router-v2/linking-state.test.ts @@ -144,6 +144,17 @@ test('the push prompt is a modal with no tab parked underneath', () => { }) }) +test('add-phone is a modal over the settings tab', () => { + expect(isHandledByLinkingConfig('keybase://settingsAddPhone')).toBe(true) + expect(getStateFromPath('settingsAddPhone')).toEqual({ + index: 1, + routes: [ + {name: 'loggedIn', state: {index: 0, routes: [{name: Tabs.settingsTab}]}}, + {name: 'settingsAddPhone'}, + ], + }) +}) + test('every app tab name is a bare tab switch', () => { for (const tab of [ Tabs.chatTab, diff --git a/shared/router-v2/linking.test.ts b/shared/router-v2/linking.test.ts index cb61c28fc083..72ea503f0edc 100644 --- a/shared/router-v2/linking.test.ts +++ b/shared/router-v2/linking.test.ts @@ -3,7 +3,9 @@ import {useConfigState} from '@/stores/config' import {useCurrentUserState} from '@/stores/current-user' import {useNavigationIntentsState} from '@/stores/navigation-intents' import {emitDeepLink} from './deep-link-emitter' -import {subscribeNavigationIntents} from './linking' +import * as Settings from '@/constants/settings' +import * as Tabs from '@/constants/tabs' +import {createLinkingConfig, isHandledByLinkingConfig, subscribeNavigationIntents} from './linking' const setCurrentUser = (uid: string) => { useCurrentUserState.getState().dispatch.setBootstrap({ @@ -148,3 +150,56 @@ test('consumes an intent after bootstrap fills in the uid the router readied wit expect(listener).toHaveBeenCalledWith('keybase://convid/post-bootstrap-conversation') unsubscribe() }) + +const getStateFromPath = (path: string) => + (createLinkingConfig(jest.fn()).getStateFromPath as (p: string) => unknown)(path) + +test('a devices link is consumed by the linking config, not by handleAppLink', () => { + useNavigationIntentsState.getState().dispatch.setNavigationReady(true, 'current-uid') + const listener = jest.fn() + const handleAppLink = jest.fn() + const unsubscribe = subscribeNavigationIntents(listener, handleAppLink) + + emitDeepLink('keybase://devices') + + expect(isHandledByLinkingConfig('keybase://devices')).toBe(true) + expect(listener).toHaveBeenCalledWith('keybase://devices') + expect(handleAppLink).not.toHaveBeenCalled() + unsubscribe() +}) + +test('a devices link opens the devices screen in the settings tab on mobile', () => { + const wasMobile = global.isMobile + global.isMobile = true + try { + expect(getStateFromPath('devices')).toEqual({ + index: 0, + routes: [ + { + name: 'loggedIn', + state: { + index: 0, + routes: [ + { + name: Tabs.settingsTab, + state: { + index: 1, + routes: [{name: 'settingsRoot'}, {name: Settings.settingsDevicesTab}], + }, + }, + ], + }, + }, + ], + }) + } finally { + global.isMobile = wasMobile + } +}) + +test('a devices link opens the devices tab on desktop', () => { + expect(getStateFromPath('devices')).toEqual({ + index: 0, + routes: [{name: 'loggedIn', state: {index: 0, routes: [{name: Tabs.devicesTab}]}}], + }) +}) diff --git a/shared/router-v2/linking.tsx b/shared/router-v2/linking.tsx index 6f3442f00728..427bd417e290 100644 --- a/shared/router-v2/linking.tsx +++ b/shared/router-v2/linking.tsx @@ -1,3 +1,4 @@ +import * as Settings from '@/constants/settings' import * as Tabs from '@/constants/tabs' import {isSplit} from '@/constants/chat/layout' import {isValidConversationIDKey, stringToConversationIDKey} from '@/constants/types/chat/common' @@ -124,6 +125,9 @@ export const subscribeNavigationIntents = ( try { // Profile links use imperative navigation to build their intermediate // back stack. Other known URLs can use React Navigation's linking state. + // This split only differs on mobile: desktop passes handleAppLink as both + // arguments (router.tsx), so every URL there lands in handleKeybaseLink, + // which must therefore stay correct for URLs the config also handles. if (intent.url.startsWith('keybase://profile/')) { handleAppLink(intent.url) } else if (isHandledByLinkingConfig(intent.url)) { @@ -183,6 +187,13 @@ const customGetStateFromPath = ( // profile/new-proof is handled by handleAppLink fallback for now break + // keybase://devices — a tap on a device push. Devices live in the Settings tab on phone and + // tablet, and in their own tab on desktop. + case 'devices': + return isMobile + ? makeTabState(Tabs.settingsTab, [{name: 'settingsRoot'}, {name: Settings.settingsDevicesTab}]) + : makeTabState(Tabs.devicesTab) + // KBFS paths: keybase://private/..., keybase://public/... case 'private': case 'public': { @@ -227,6 +238,11 @@ const customGetStateFromPath = ( case 'settingsPushPrompt': return makeModalState('settingsPushPrompt') + // keybase://settingsAddPhone — where https://keybase.io/phone-app lands. Settings sits + // under the modal so dismissing it leaves the invitee somewhere they can find it again. + case 'settingsAddPhone': + return makeModalState('settingsAddPhone', undefined, Tabs.settingsTab) + // Tab switches: keybase://tabs.chatTab, etc. case Tabs.chatTab: case Tabs.peopleTab: @@ -247,6 +263,16 @@ const customGetStateFromPath = ( // ---- Linking config ---- +// Known URLs become launch state; the rest open imperatively once the router is up. +// setInitialURLOnce also consumes: markInitialURLHandled clears a pending intent with the +// same URL, so subscribeNavigationIntents won't navigate to it a second time. +const openInitialLink = (link: string, handleAppLink: (link: string) => void) => { + if (isHandledByLinkingConfig(link)) return setInitialURLOnce(link) + setInitialURLOnce(link) + setTimeout(() => handleAppLink(link), 1) + return null +} + export const createLinkingConfig = ( handleAppLink: (link: string) => void ): LinkingOptions => { @@ -284,11 +310,7 @@ export const createLinkingConfig = ( if (deepLinkUrl) { const normalized = normalizeUrl(deepLinkUrl) if (normalized) { - if (isHandledByLinkingConfig(normalized)) return setInitialURLOnce(normalized) - // URL not handled by linking config; use imperative navigation as fallback - setInitialURLOnce(normalized) - setTimeout(() => handleAppLink(normalized), 1) - return null + return openInitialLink(normalized, handleAppLink) } } diff --git a/shared/router-v2/url-normalize.test.ts b/shared/router-v2/url-normalize.test.ts index 6f7707c9cda5..5b857c4faf1a 100644 --- a/shared/router-v2/url-normalize.test.ts +++ b/shared/router-v2/url-normalize.test.ts @@ -1,5 +1,6 @@ /// import {normalizeUrl} from './deep-link-emitter' +import {useSettingsPhoneState} from '@/stores/settings-phone' test('keybase urls pass through untouched', () => { expect(normalizeUrl('keybase://convid/conv-1')).toBe('keybase://convid/conv-1') @@ -79,3 +80,15 @@ test('a slash-separated subteam path is not a team-page link', () => { // second segment and nothing matches expect(normalizeUrl('https://keybase.io/team/keybase/sub')).toBeUndefined() }) + +test('the invite install link opens add-phone, not a profile for a user named phone-app', () => { + expect(normalizeUrl('https://keybase.io/phone-app')).toBe('keybase://settingsAddPhone') + expect(normalizeUrl('https://keybase.io/phone-app/')).toBe('keybase://settingsAddPhone') + expect(normalizeUrl('https://keybase.io/phone-app?utm=x')).toBe('keybase://settingsAddPhone') +}) + +test('the invite install link opens add-phone even when the user already has a number', () => { + useSettingsPhoneState.setState({phones: new Map([['+15555555555', {} as never]])}) + expect(normalizeUrl('https://keybase.io/phone-app')).toBe('keybase://settingsAddPhone') + useSettingsPhoneState.getState().dispatch.resetState() +}) diff --git a/shared/settings/load-settings.tsx b/shared/settings/load-settings.tsx index 8770fd103a4a..9a082d7b4c4a 100644 --- a/shared/settings/load-settings.tsx +++ b/shared/settings/load-settings.tsx @@ -1,40 +1,39 @@ -import * as Tabs from '@/constants/tabs' import * as S from '@/constants/strings' import * as T from '@/constants/types' import {ignorePromise} from '@/constants/utils' import logger from '@/logger' -import {navigateAppend, switchTab} from '@/constants/router' import {RPCError} from '@/util/errors' import {useConfigState} from '@/stores/config' import {useSettingsEmailState} from '@/stores/settings-email' import {useSettingsPhoneState} from '@/stores/settings-phone' -let maybeLoadAppLinkOnce = false - export const loadSettings = () => { - const maybeLoadAppLink = () => { - const phones = useSettingsPhoneState.getState().phones - if (!phones || phones.size > 0) { - return - } - - if (maybeLoadAppLinkOnce || !useConfigState.getState().startup.link.endsWith('/phone-app')) { - return - } - maybeLoadAppLinkOnce = true - switchTab(Tabs.settingsTab) - navigateAppend({name: 'settingsAddPhone', params: {}}) - } - const f = async () => { if (!useConfigState.getState().loggedIn) { return } + // Anything that writes these two stores while this RPC is in flight knows something the + // reply does not, so the reply must not land on top of it. Apply each half only to the + // value it was read against. The racing writer is usually an emailsChanged/phoneNumbersChanged notification, but + // notifyEmailVerified and sentVerificationEmail trip it too -- so a resend-verification + // click mid-load drops that round's server list, by design. + const emailsBefore = useSettingsEmailState.getState().emails + const phonesBefore = useSettingsPhoneState.getState().phones try { const settings = await T.RPCGen.userLoadMySettingsRpcPromise(undefined, S.waitingKeySettingsLoadSettings) - useSettingsEmailState.getState().dispatch.notifyEmailAddressEmailsChanged(settings.emails ?? []) - useSettingsPhoneState.getState().dispatch.setNumbers(settings.phoneNumbers ?? undefined) - maybeLoadAppLink() + // A logout does NOT trip the identity checks below: Z.defaultReset restores the values + // captured at store creation, so on a cold start emails is the same initial Map and + // phones the same undefined. Without this, the reply would repopulate the stores for a + // logged-out app and the next account could read the previous one's settings. + if (!useConfigState.getState().loggedIn) { + return + } + if (useSettingsEmailState.getState().emails === emailsBefore) { + useSettingsEmailState.getState().dispatch.notifyEmailAddressEmailsChanged(settings.emails ?? []) + } + if (useSettingsPhoneState.getState().phones === phonesBefore) { + useSettingsPhoneState.getState().dispatch.setNumbers(settings.phoneNumbers ?? undefined) + } } catch (error) { if (!(error instanceof RPCError)) { return diff --git a/shared/stores/config.tsx b/shared/stores/config.tsx index f0ab46999359..18715f9c7129 100644 --- a/shared/stores/config.tsx +++ b/shared/stores/config.tsx @@ -46,7 +46,6 @@ type Store = T.Immutable<{ // Used to avoid replaying a conversation under a different account. conversationUid?: string followUser: string - link: string tab?: Tab } userSwitching: boolean @@ -84,7 +83,6 @@ const initialStore: Store = { startup: { conversation: noConversationIDKey, followUser: '', - link: '', loaded: false, }, userSwitching: false, diff --git a/shared/stores/tests/config.test.ts b/shared/stores/tests/config.test.ts index 1df0b762284f..e6595a24ca60 100644 --- a/shared/stores/tests/config.test.ts +++ b/shared/stores/tests/config.test.ts @@ -17,7 +17,6 @@ const resetConfigState = () => { startup: { conversation: noConversationIDKey, followUser: '', - link: '', loaded: false, }, userSwitching: false, @@ -39,20 +38,17 @@ test('setStartupDetails only records the first startup payload', () => { dispatch.setStartupDetails({ conversation: 'first-convo' as any, followUser: 'alice', - link: 'keybase://first', tab: undefined, }) dispatch.setStartupDetails({ conversation: 'second-convo' as any, followUser: 'bob', - link: 'keybase://second', tab: undefined, }) expect(useConfigState.getState().startup).toEqual({ conversation: 'first-convo', followUser: 'alice', - link: 'keybase://first', loaded: true, tab: undefined, }) diff --git a/shared/stores/tests/settings.test.ts b/shared/stores/tests/settings.test.ts index ae699f7d314f..24d0636cd511 100644 --- a/shared/stores/tests/settings.test.ts +++ b/shared/stores/tests/settings.test.ts @@ -1,10 +1,4 @@ /// -jest.mock('../../constants/router', () => ({ - clearModals: jest.fn(), - navigateAppend: jest.fn(), - switchTab: jest.fn(), -})) - import * as T from '../../constants/types' import {loadSettings} from '../../settings/load-settings' import {resetAllStores} from '../../util/zustand' @@ -51,4 +45,54 @@ describe('settings loading', () => { expect(emailHandler).toHaveBeenCalledWith(emails) expect(phoneHandler).toHaveBeenCalledWith(phoneNumbers) }) + + test('a notification that lands while the settings load is in flight is not overwritten', async () => { + const stale = [{ctime: 0, phoneNumber: '+15550000000', superseded: false, verified: true, visibility: 0}] + const notified = [{ctime: 0, phoneNumber: '+15551111111', superseded: false, verified: true, visibility: 0}] + const staleEmails = [ + {email: 'stale@example.com', isPrimary: true, isVerified: true, lastVerifyEmailDate: 0, visibility: 0}, + ] + const notifiedEmails = [ + {email: 'fresh@example.com', isPrimary: true, isVerified: true, lastVerifyEmailDate: 0, visibility: 0}, + ] + + useConfigState.setState({loggedIn: true}) + jest.spyOn(T.RPCGen, 'userLoadMySettingsRpcPromise').mockImplementation((async () => { + // the notifications win the race: they carry the newer server state + await Promise.resolve() + useSettingsPhoneState.getState().dispatch.notifyPhoneNumberPhoneNumbersChanged(notified) + useSettingsEmailState.getState().dispatch.notifyEmailAddressEmailsChanged(notifiedEmails) + return {emails: staleEmails, phoneNumbers: stale} + }) as never) + + loadSettings() + for (let i = 0; i < 10; ++i) await Promise.resolve() + + expect([...useSettingsPhoneState.getState().phones!.keys()]).toEqual(['+15551111111']) + expect([...useSettingsEmailState.getState().emails.keys()]).toEqual(['fresh@example.com']) + }) + + test('a logout while the settings load is in flight drops the reply', async () => { + const emails = [ + {email: 'a@example.com', isPrimary: true, isVerified: true, lastVerifyEmailDate: 0, visibility: 0}, + ] + const phoneNumbers = [ + {ctime: 0, phoneNumber: '+15555555555', superseded: false, verified: true, visibility: 0}, + ] + + useConfigState.setState({loggedIn: true}) + jest.spyOn(T.RPCGen, 'userLoadMySettingsRpcPromise').mockImplementation((async () => { + // Z.defaultReset restores the identities captured at store creation, so the reference + // checks cannot see this; only the loggedIn re-read can. + await Promise.resolve() + useConfigState.getState().dispatch.setLoggedIn(false) + return {emails, phoneNumbers} + }) as never) + + loadSettings() + for (let i = 0; i < 10; ++i) await Promise.resolve() + + expect(useSettingsPhoneState.getState().phones).toBeUndefined() + expect([...useSettingsEmailState.getState().emails.keys()]).toEqual([]) + }) }) From e151313dc9fddde0eef5afb4960e2cfffa1cb2c5 Mon Sep 17 00:00:00 2001 From: chrisnojima Date: Mon, 21 Sep 2026 10:41:35 -0400 Subject: [PATCH 2/3] fix(links): open devices on the root stack on a phone keybase://devices built one nested state for all of mobile: the devices route inside the Settings tab stack. That resolves on a tablet, where every tab stack registers every route, but on a phone each tab stack holds only its own root screen and everything else is registered on the root stack above the tabs (router.tsx phoneRootRoutes). React Navigation's getRehydratedState silently drops the unknown nested route, so tapping the link landed on settingsRoot instead of Devices. Split the case three ways, the way the KBFS private/public case already does: its own tab on desktop, nested in the Settings tab stack when isSplit (tablet), and on the root stack above the tabs on a phone. The imperative fallback in deeplinks had the same root cause from the other direction: navUpToScreen pins its popTo to the deepest active stack, which at a tab root on a phone is the Settings tab stack, so StackRouter returned null and the action was dropped. Push instead -- untargeted, so it is handled by whichever navigator registers the route: the root stack on a phone, the Settings tab stack on a tablet. It is the same call the phone Settings list itself makes. The existing "on mobile" linking test was really a tablet test: isSplit is computed at module load and jest runs as desktop, so global.isMobile alone never produced the phone shape. Renamed it, and added linking-phone.test.ts, which mocks the layout module to get isSplit false and asserts the root-stack shape. --- shared/constants/deeplinks.test.ts | 14 +++++-- shared/constants/deeplinks.tsx | 14 ++++++- shared/router-v2/linking-phone.test.ts | 56 ++++++++++++++++++++++++++ shared/router-v2/linking.test.ts | 4 +- shared/router-v2/linking.tsx | 29 +++++++++++-- 5 files changed, 108 insertions(+), 9 deletions(-) create mode 100644 shared/router-v2/linking-phone.test.ts diff --git a/shared/constants/deeplinks.test.ts b/shared/constants/deeplinks.test.ts index d8c6b83c8f9b..3add9978c440 100644 --- a/shared/constants/deeplinks.test.ts +++ b/shared/constants/deeplinks.test.ts @@ -39,13 +39,21 @@ test('a devices link opens the devices tab on desktop', () => { }) }) -test('a devices link opens the devices screen under settings on mobile', () => { +// The mobile half of this switch is the fallback only: isHandledByLinkingConfig now claims +// keybase://devices, so on mobile every producer routes it to the linking config instead (the +// phone shape it builds is covered by router-v2/linking-phone.test.ts). It still has to be +// phone-correct, because the config is the thing that can stop claiming a URL. +// One call covers phone and tablet: the push carries no target, so it is handled by whichever +// navigator registers the route -- the root stack above the tabs on a phone, the Settings tab +// stack on a tablet. navUpToScreen cannot do that; it pins its popTo to the active stack, which +// at a tab root on a phone is the Settings tab stack, where the route does not exist. +test('a devices link pushes the devices screen without pinning it to the settings tab stack', () => { withIsMobile(true, () => { handleAppLink('keybase://devices') expect(Router.switchTab).toHaveBeenCalledWith(Tabs.settingsTab) - expect(Router.navUpToScreen).toHaveBeenCalledWith(settingsDevicesTab) - expect(Router.navigateAppend).not.toHaveBeenCalled() + expect(Router.navigateAppend).toHaveBeenCalledWith({name: settingsDevicesTab, params: {}}) + expect(Router.navUpToScreen).not.toHaveBeenCalled() }) }) diff --git a/shared/constants/deeplinks.tsx b/shared/constants/deeplinks.tsx index 06f27fb0d213..731dc1360116 100644 --- a/shared/constants/deeplinks.tsx +++ b/shared/constants/deeplinks.tsx @@ -85,8 +85,18 @@ const handleKeybaseLink = (link: string) => { break case 'devices': // Devices live under Settings on phone/tablet and in their own tab on desktop. - switchTab(isMobile ? Tabs.settingsTab : Tabs.devicesTab) - navUpToScreen(isMobile ? settingsDevicesTab : 'devicesRoot') + if (!isMobile) { + switchTab(Tabs.devicesTab) + navUpToScreen('devicesRoot') + return + } + switchTab(Tabs.settingsTab) + // navUpToScreen pins its popTo to the deepest active stack, which at a tab root on a + // phone is the Settings tab stack -- and that stack knows only settingsRoot there, so + // StackRouter returned null and the action was dropped. An untargeted push lands + // wherever the route is registered: the root stack above the tabs on a phone, the + // Settings tab stack on a tablet. Same call the phone settings list itself makes. + navigateAppend({name: settingsDevicesTab, params: {}}) return case 'settingsAddPhone': // Where the invite install link (https://keybase.io/phone-app) lands. The linking config diff --git a/shared/router-v2/linking-phone.test.ts b/shared/router-v2/linking-phone.test.ts new file mode 100644 index 000000000000..ef7328c218be --- /dev/null +++ b/shared/router-v2/linking-phone.test.ts @@ -0,0 +1,56 @@ +/// +// Phone shapes. isSplit is computed at module load and this suite loads as desktop, so +// global.isMobile alone yields the tablet shapes (see linking.test.ts / linking-state.test.ts); +// only mocking the module gets the phone ones. On a phone each tab stack holds just its root +// screen -- every other route is registered on the root stack, above the tabs -- so a nested +// route is silently dropped on rehydrate and the tap lands on the tab root. +jest.mock('@/constants/chat/layout', () => ({isSplit: false, threadRouteName: 'chatConversation'})) +import * as Settings from '@/constants/settings' +import * as Tabs from '@/constants/tabs' +import {createLinkingConfig} from './linking' + +const getStateFromPath = (path: string) => + (createLinkingConfig(jest.fn()).getStateFromPath as (p: string) => unknown)(path) + +const wasMobile = global.isMobile +beforeAll(() => { + global.isMobile = true +}) +afterAll(() => { + global.isMobile = wasMobile +}) + +test('a devices link opens devices on the root stack above the tabs on a phone', () => { + expect(getStateFromPath('devices')).toEqual({ + index: 1, + routes: [ + { + name: 'loggedIn', + state: { + index: 0, + routes: [{name: Tabs.settingsTab, state: {index: 0, routes: [{name: 'settingsRoot'}]}}], + }, + }, + {name: Settings.settingsDevicesTab}, + ], + }) +}) + +// A control: if the isSplit mock ever stopped taking effect, this would produce the split +// (chatRoot-with-params) shape instead, and the devices expectation above would be testing +// the tablet path while claiming to test the phone one. +test('the phone shapes are in force -- a conversation opens above the tabs, not in the chat tab', () => { + expect(getStateFromPath('convid/conv-1')).toEqual({ + index: 1, + routes: [ + { + name: 'loggedIn', + state: { + index: 0, + routes: [{name: Tabs.chatTab, state: {index: 0, routes: [{name: 'chatRoot', params: {}}]}}], + }, + }, + {name: 'chatConversation', params: {conversationIDKey: 'conv-1'}}, + ], + }) +}) diff --git a/shared/router-v2/linking.test.ts b/shared/router-v2/linking.test.ts index 72ea503f0edc..b985780ecdb4 100644 --- a/shared/router-v2/linking.test.ts +++ b/shared/router-v2/linking.test.ts @@ -168,7 +168,9 @@ test('a devices link is consumed by the linking config, not by handleAppLink', ( unsubscribe() }) -test('a devices link opens the devices screen in the settings tab on mobile', () => { +// isSplit is baked in at module load and this suite loads as desktop, so global.isMobile alone +// gets the tablet shape, not the phone one. Phone coverage lives in linking-phone.test.ts. +test('a devices link opens the devices screen inside the settings tab on tablet', () => { const wasMobile = global.isMobile global.isMobile = true try { diff --git a/shared/router-v2/linking.tsx b/shared/router-v2/linking.tsx index 427bd417e290..c6e86a3a4cf2 100644 --- a/shared/router-v2/linking.tsx +++ b/shared/router-v2/linking.tsx @@ -190,9 +190,32 @@ const customGetStateFromPath = ( // keybase://devices — a tap on a device push. Devices live in the Settings tab on phone and // tablet, and in their own tab on desktop. case 'devices': - return isMobile - ? makeTabState(Tabs.settingsTab, [{name: 'settingsRoot'}, {name: Settings.settingsDevicesTab}]) - : makeTabState(Tabs.devicesTab) + if (!isMobile) { + return makeTabState(Tabs.devicesTab) + } + if (isSplit) { + // Tablet: the Settings tab stack holds every settings route, so devices pushes + // above the tab root, inside that stack. + return makeTabState(Tabs.settingsTab, [{name: 'settingsRoot'}, {name: Settings.settingsDevicesTab}]) + } + // Phone: settingsRoot is the only screen in the Settings tab stack, so a nested devices + // route is filtered out on rehydrate and the tap lands on settingsRoot. Devices is + // registered on the root stack there, above the tabs. + return { + index: 1, + routes: [ + { + name: 'loggedIn', + state: { + index: 0, + routes: [ + {name: Tabs.settingsTab, state: {index: 0, routes: [{name: 'settingsRoot'}]}}, + ], + }, + }, + {name: Settings.settingsDevicesTab}, + ], + } // KBFS paths: keybase://private/..., keybase://public/... case 'private': From 99f974838c3eff5c9d65578e739d4a0c789d3ed5 Mon Sep 17 00:00:00 2001 From: chrisnojima Date: Mon, 21 Sep 2026 10:41:42 -0400 Subject: [PATCH 3/3] fix(settings): drop a settings reply that belongs to the previous account loadSettings guarded its reply with a loggedIn re-read and a reference check on each store, which covers a logout and a racing notification but not an account switch. resetState puts back the values captured when the stores were created -- the same initial emails Map, phones still undefined -- and setLoggedIn(true) follows, so account A's in-flight reply passes all three checks and writes A's emails and phone numbers into account B's stores. Worse, that write changes the references B's own loadSettings captured, so B's reply is then dropped as raced and B keeps showing A's data until a notification happens to arrive. Capture the uid before the RPC and compare it after: it is the only thing an account switch actually changes. --- shared/settings/load-settings.tsx | 10 +++++++ shared/stores/tests/settings.test.ts | 39 ++++++++++++++++++++++++++++ 2 files changed, 49 insertions(+) diff --git a/shared/settings/load-settings.tsx b/shared/settings/load-settings.tsx index 9a082d7b4c4a..c922fa21cde0 100644 --- a/shared/settings/load-settings.tsx +++ b/shared/settings/load-settings.tsx @@ -4,6 +4,7 @@ import {ignorePromise} from '@/constants/utils' import logger from '@/logger' import {RPCError} from '@/util/errors' import {useConfigState} from '@/stores/config' +import {useCurrentUserState} from '@/stores/current-user' import {useSettingsEmailState} from '@/stores/settings-email' import {useSettingsPhoneState} from '@/stores/settings-phone' @@ -19,6 +20,7 @@ export const loadSettings = () => { // click mid-load drops that round's server list, by design. const emailsBefore = useSettingsEmailState.getState().emails const phonesBefore = useSettingsPhoneState.getState().phones + const uidBefore = useCurrentUserState.getState().uid try { const settings = await T.RPCGen.userLoadMySettingsRpcPromise(undefined, S.waitingKeySettingsLoadSettings) // A logout does NOT trip the identity checks below: Z.defaultReset restores the values @@ -28,6 +30,14 @@ export const loadSettings = () => { if (!useConfigState.getState().loggedIn) { return } + // An account switch is invisible to both checks above: the reset restores the same + // creation-time values the reference checks compare against, and the new account is + // logged in by the time the reply lands. Only the uid separates account A's reply from + // account B's stores -- and writing it would also make B's own in-flight load look + // raced, so B would be left showing A's emails and phone numbers. + if (useCurrentUserState.getState().uid !== uidBefore) { + return + } if (useSettingsEmailState.getState().emails === emailsBefore) { useSettingsEmailState.getState().dispatch.notifyEmailAddressEmailsChanged(settings.emails ?? []) } diff --git a/shared/stores/tests/settings.test.ts b/shared/stores/tests/settings.test.ts index 24d0636cd511..6ce1eb83322c 100644 --- a/shared/stores/tests/settings.test.ts +++ b/shared/stores/tests/settings.test.ts @@ -3,6 +3,7 @@ import * as T from '../../constants/types' import {loadSettings} from '../../settings/load-settings' import {resetAllStores} from '../../util/zustand' import {useConfigState} from '../config' +import {useCurrentUserState} from '../current-user' import {useSettingsEmailState} from '../settings-email' import {useSettingsPhoneState} from '../settings-phone' @@ -95,4 +96,42 @@ describe('settings loading', () => { expect(useSettingsPhoneState.getState().phones).toBeUndefined() expect([...useSettingsEmailState.getState().emails.keys()]).toEqual([]) }) + + test('an account switch while the settings load is in flight drops the reply', async () => { + const emails = [ + {email: 'a@example.com', isPrimary: true, isVerified: true, lastVerifyEmailDate: 0, visibility: 0}, + ] + const phoneNumbers = [ + {ctime: 0, phoneNumber: '+15555555555', superseded: false, verified: true, visibility: 0}, + ] + + const bootstrap = (uid: string) => + useCurrentUserState.getState().dispatch.setBootstrap({ + deviceID: '', + deviceName: '', + uid, + username: uid, + }) + + useConfigState.setState({loggedIn: true}) + bootstrap('uid-a') + jest.spyOn(T.RPCGen, 'userLoadMySettingsRpcPromise').mockImplementation((async () => { + // The switch, as the app performs it: the stores go back to their creation-time values + // and the next account logs in. Neither the reference checks nor the loggedIn re-read + // can tell that apart from a quiet load, so only the uid stops account A's reply from + // landing in account B's stores. + await Promise.resolve() + useSettingsEmailState.getState().dispatch.resetState() + useSettingsPhoneState.getState().dispatch.resetState() + bootstrap('uid-b') + useConfigState.getState().dispatch.setLoggedIn(true) + return {emails, phoneNumbers} + }) as never) + + loadSettings() + for (let i = 0; i < 10; ++i) await Promise.resolve() + + expect(useSettingsPhoneState.getState().phones).toBeUndefined() + expect([...useSettingsEmailState.getState().emails.keys()]).toEqual([]) + }) })