fix(links): route devices and /phone-app through the linking config - #29650
Draft
chrisnojima wants to merge 3 commits into
Draft
chrisnojima wants to merge 3 commits into
chrisnojima wants to merge 3 commits into
Conversation
chrisnojima
added this pull request to stack #29658
September 21, 2026 18:56
chrisnojima
changed the title
This was referenced Sep 21, 2026
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.
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.
…ount 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.
chrisnojima
force-pushed
the
nojima/HOTPOT-as-02-links
branch
from
September 22, 2026 23:08
cc424ab to
99f9748
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stack 2/9, splitting #29637. Parent: #29649.
Why
Push taps are about to be resolved into
keybase://routes by the service (#29656). For that to work, the routes those taps produce have to actually be handled by the linking config. Today several are not. This PR gets the link handling correct first, so the push-tap change lands on a working foundation rather than changing two things at once.Master bugs this fixes
keybase://devicesis not handled at all.linking.tsx:237matchesTabs.devicesTab, whose value is the stringtabs.devicesTab— notdevices.deeplinks.tsxhas nodevicescase whatsoever. The link silently does nothing./phone-appis handled outside the linking config, by an ad-hocmaybeLoadAppLinkin the settings loader, so it does not participate in normal link routing.load-settings.tsxchecksloggedInbefore itsawaitand then writes at line 35 with no identity check at all, so after an account switch account A's emails and phones land in account B's stores. Fixed here by capturing the uid before the RPC and re-checking after.One fix beyond a straight port of #29637
keybase://deviceswould have landed on the wrong screen on a phone. #29637 nestssettingsTabs.devicesTabinside the settings tab stack for all of mobile. That is correct on tablet, butrouter.tsxregisters only each tab's root screen on phones — everything else lives on the root stack above the tab bar — so React Navigation silently filters the unknown route and the user lands onsettingsRoot. Now three-way: desktop → devices tab, tablet → nested, phone → root stack, mirroring how the KBFS case already splits.The existing tests could not have caught this:
isSplitis evaluated at module load and jest runs as desktop, so settingglobal.isMobilesimulates a tablet. Those tests are renamed to say so, andlinking-phone.test.tsmocksisSplitto cover the real phone path.Deliberately not here
config.startup.followUserstays. It only becomes dead once push stops producing a follow-user startup, which is #29656.Verification
yarn lint:allclean, full unit suite green. The devices and uid-guard fixes are mutation-checked — reverting either fails its test.Note nothing emits
keybase://devicesuntil #29656;stores/push.tsxstill navigates directly, and that path has the same phone bug on master. #29656 deletes it.