refactor(push): resolve a tapped notification in Go and route it - #29656
Draft
chrisnojima wants to merge 6 commits into
Draft
chrisnojima wants to merge 6 commits into
chrisnojima wants to merge 6 commits into
Conversation
chrisnojima
added this pull request to stack #29658
September 21, 2026 18:56
This was referenced Sep 21, 2026
appState gains PushTapRoute plus peekPushTapRoute/ackPushTapRoute, and NotifyApp gains pushTapRouteAvailable. Peek does not retire the route: a reply lost on the way to the client would take the tap with it, and only the client knows whether it acted, so the ack is what clears it. The notification carries nothing -- the route rides the peek's reply -- so a tap has exactly one reader whether it happened before a client existed or while one was connected.
ResolvePushTap turns a push payload into the keybase:// URL it opens, the mapping the client used to carry, and PendingPushTap parks the result until a client acks it. Native delivers a tap through bind.DeliverPushTap, the one door a tap comes through and the only thing that may name an account to switch to, so a URL another app opens cannot switch accounts. Each parked route carries an id, so an ack that crosses a newer tap retires nothing, and a tap not yet acked is replaced rather than queued -- the newest tap is the one the user just made.
iOS delivers the tap from UNUserNotificationCenter's didReceive; Android's notification PendingIntent now opens PushTapActivity, which is not exported, so only this app's own notifications can start it. Both pass the push through verbatim -- which fields matter is the service's business -- and neither stores, re-emits or parses it. That retires the whole JS-facing push path: getInitialNotification, onPushNotification, KbSetInitialNotification/KbEmitPushNotification and the become-active re-emit, MainActivity's notification extra and its duplicate-tap hash, and engineReset with them. The Android PendingIntent data is a digest of the payload, because extras are not part of filterEquals and two notifications would otherwise share one PendingIntent, and it is immutable so its holder cannot substitute a payload.
…yload The client peeks the service's resolved route on connect and on pushTapRouteAvailable, and queues it as a navigation intent. The router owns consumption, so a tap for another account waits for account-link-switch and the intent survives the store reset the switch performs; whatever consumes the intent -- navigating it, the linking config taking it as the startup route, or giving up on it as stale -- is what acks the tap with the service. That makes a tap exactly-once end to end. With no push payload reaching JS any more, the push normalizer, the pending-push-notification machinery and constants/types/push all go, and config.startup.followUser with them: a follow tap is now just a profile URL.
Two holes found reviewing the tap path. dropOnFailure asked tapForOtherAccount, so a tap for the account being logged out of was kept: it is not "for another account" while that uid is still set, and the teardown clearing the uid then makes it one, so check() logged the user straight back into the account they just left. The drop is account-blind now, which is what the pending-push clear it replaces did. The connect-time peek ran alongside the subscribe rather than after it. pushTapRouteAvailable is filtered per connection on the App channel, so a tap landing between the peek's reply and the service applying the subscribe had no reader left at all -- and on Android the tap really is delivered on another thread while the client is coming up.
Nothing sends a chat.extension push anymore, and an unknown type already resolves to no route, so the entry and its test case were dead.
chrisnojima
force-pushed
the
nojima/HOTPOT-as-08-pushtap
branch
from
September 22, 2026 23:08
86a99df to
121d00b
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
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 8/9, splitting #29637. Parent: #29655.
Why
Tapping a notification has to work when the app is cold — which is exactly when JS is not running. On master the push payload is parsed in JS to decide where to navigate, so the thing that decides the destination is the thing that may not exist yet. Moving resolution into the service means the route is known the moment the tap reaches Go, and JS just consumes it when it comes up.
Master bugs this fixes
stores/push.tsx; nothing resolves the tap before JS is running.pendingPushNotificationstore field plus an HMR-guarded subscribe hook, rather than as a property of the pending intent.getInitialNotification,onPushNotificationandengineResetstill exist on the native module.What this changes
libkb.ResolvePushTapturns a push payload into akeybase://route with a target uid and an id. The service holds it and exposespeekPushTapRoute/ackPushTapRouteplus apushTapRouteAvailablenotification. Android taps go to aPushTapActivitythat hands the payload straight to Go; iOS does the same fromdidReceiveRemoteNotification. JS drains the route on connect and on notification, routes it through the linking config (which #29650 made correct), and acks it once navigation actually consumes it. The account-switch behaviour moves toaccount-link-switch.tsx, driven by the intent's target uid, so it is a property of the intent rather than a side store.constants/types/push.tsxand the dead bridge surface are deleted, andconfig.startup.followUsergoes with them — it only becomes dead now.chat.extensionpushes are not routed. Master's JS mapped them to their conversation, but nothing has sent that type since the old share extension was removed, so it isn't carried over. An unknown type resolves to no route.Why this one cannot be split further
The moment
KBPushNotifier.buildPendingIntentpoints atPushTapActivity, the old JS tap path is dead. Any partial landing ships a build where notification taps do nothing.Two fixes beyond a straight port of #29637
Both are present on the source branch; both are mutation-checked here.
dropOnFailureaskedtapForOtherAccount(), but a tap for the account being logged out of is not "for another account" while that uid is still set — and the teardown clearing the uid then makes it one, so the pending tap logged the user back into the account they had just left. The drop path is now account-blind, matching whatclearPendingPushNotification-on-logout used to do.subscribe()rather than after it, andpushTapRouteAvailableis filtered per connection, so a tap landing between the peek's reply and the subscribe being applied was seen by nobody. On Android the tap genuinely arrives on another thread while the client is still coming up.Verification
go build,go vet,gofmt,golangci-lint --new-from-rev master0 issues; every test binary in the repo compiles. Android builds —javapconfirmsdeliverPushTapin the rebuilt AAR. iOS builds, withyarn sync:kb-modulesrun so the compiled copy of the native module matches the source.yarn lint:allclean, unit suite green.Worth confirming on device:
chat.1.local.unboxMobilePushNotificationnow has no caller. Go's handler covers the normal path but not the case where its own processing threw, so a tap after failed Go processing may land on a conversation whose message was never unboxed.