Skip to content

refactor(push): resolve a tapped notification in Go and route it - #29656

Draft
chrisnojima wants to merge 6 commits into
nojima/HOTPOT-as-07-livelocationfrom
nojima/HOTPOT-as-08-pushtap
Draft

chrisnojima wants to merge 6 commits into
nojima/HOTPOT-as-07-livelocationfrom
nojima/HOTPOT-as-08-pushtap

Conversation

@chrisnojima

@chrisnojima chrisnojima commented Sep 21, 2026

Copy link
Copy Markdown
Contributor

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

  • A tap can be lost because JS is not up to parse it. The payload switch lives in stores/push.tsx; nothing resolves the tap before JS is running.
  • A cold-start tap does not reliably open the conversation.
  • A tap is retired when it is read, not when it is acted on, so a tap consumed by a client that then fails to navigate is simply gone.
  • Account switching on tap is handled ad hoc through a pendingPushNotification store field plus an HMR-guarded subscribe hook, rather than as a property of the pending intent.
  • Non-tap pushes are still handed to JS, doing work for notifications the user never touched.
  • Dead bridge surface: getInitialNotification, onPushNotification and engineReset still exist on the native module.

What this changes

libkb.ResolvePushTap turns a push payload into a keybase:// route with a target uid and an id. The service holds it and exposes peekPushTapRoute/ackPushTapRoute plus a pushTapRouteAvailable notification. Android taps go to a PushTapActivity that hands the payload straight to Go; iOS does the same from didReceiveRemoteNotification. 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 to account-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.tsx and the dead bridge surface are deleted, and config.startup.followUser goes with them — it only becomes dead now.

chat.extension pushes 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.buildPendingIntent points at PushTapActivity, 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.

  • Logging out could log you straight back in. dropOnFailure asked tapForOtherAccount(), 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 what clearPendingPushNotification-on-logout used to do.
  • A tap could arrive with no reader. The connect-time peek ran alongside subscribe() rather than after it, and pushTapRouteAvailable is 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 master 0 issues; every test binary in the repo compiles. Android builds — javap confirms deliverPushTap in the rebuilt AAR. iOS builds, with yarn sync:kb-modules run so the compiled copy of the native module matches the source. yarn lint:all clean, unit suite green.

Worth confirming on device: chat.1.local.unboxMobilePushNotification now 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.

@chrisnojima
chrisnojima added this pull request to stack #29658 September 21, 2026 18:56
@chrisnojima chrisnojima changed the title nojima/HOTPOT as 08 pushtap refactor(push): resolve a tapped notification in Go and route it 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
chrisnojima force-pushed the nojima/HOTPOT-as-08-pushtap branch from 86a99df to 121d00b Compare September 22, 2026 23:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant