Conversation
🦋 Changeset detectedLatest commit: fd118d9 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
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.
|
@clerk/agent-toolkit
@clerk/astro
@clerk/backend
@clerk/chrome-extension
@clerk/clerk-js
@clerk/dev-cli
@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: |
|
|
||
| describe('with offline browser and network failure', () => { | ||
| beforeEach(() => { | ||
| // Use real timers for offline tests to avoid unhandled rejection issues with retry logic |
bratsos
force-pushed
the
alexbratsos/user-4744-investigate-random-sign-outs-in-the-dashboard-possibly
branch
from
d134aeb to
c230146
Compare
📝 WalkthroughWalkthroughAdds a changeset entry for a patch release. Modifies 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
bratsos
force-pushed
the
alexbratsos/user-4744-investigate-random-sign-outs-in-the-dashboard-possibly
branch
from
992a3ba to
2377201
Compare
| // Throw when offline and no token so retry() in getToken() can fire. | ||
| // Without this, _getToken returns null (success) and retry() never calls shouldRetry. | ||
| if (result === null && !isValidBrowserOnline()) { | ||
| throw new ClerkRuntimeError('Network request failed while offline', { code: 'network_error' }); |
There was a problem hiding this comment.
Recommend that we re-use ClerkOfflineError here
There was a problem hiding this comment.
I'm not sure about this. At this point throwing this error doesn't mean we're 100% offline, we just throw this so that retry's shouldRetry mechanism catches the error and retries 3 times (if the browser is offline during this). After the 3rd retry, if the browser is still offline, then we throw a ClerkOfflineError.
The way I see this is that when we throw this ClerkRuntimeError we're in a state where something went wrong with the network, but will give it some time, and we only throw ClerkOfflineError when there is no more attempts.
There was a problem hiding this comment.
Got it, so this is not an error that the user would see?
There was a problem hiding this comment.
Yes exactly - if it surfaces to the user it's gonna be a ClerkOfflineError. Unless I messed something up that is
| return cachedToken.getRawString() || null; | ||
| result = cachedToken.getRawString() || null; | ||
| } else { | ||
| result = await this.#fetchToken(template, organizationId, tokenId, shouldDispatchTokenUpdate, skipCache); |
There was a problem hiding this comment.
does this throw if offline? should we avoid fetching if we're offline in the first place?
There was a problem hiding this comment.
Great question! So that sent me through a rabbit hole and tl;dr: we can do it and if we throw here when we're offline, we'll trigger the retrying mechanism without firing network requests.
Additionally because of this exploration I figured a race condition that we had where the browser could go back online while mid-request, so I added a test for it and a fix here
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/clerk-js/src/core/resources/Session.ts`:
- Around line 451-453: The token-update path currently emits and caches/clears
when isValidBrowserOnline() is true even if cachedToken.getRawString() is empty;
change the guard so eventBus.emit(events.TokenUpdate, { token: cachedToken })
and any code that writes/clears __session only run when
cachedToken.getRawString() is truthy (i.e., require a non-empty token), and do
not rely on isValidBrowserOnline() as a substitute for a missing token. Apply
this same change to the other TokenUpdate/caching occurrences that use
cachedToken.getRawString() || isValidBrowserOnline() so all places (the
eventBus.emit call and the __session write/clear logic) first check
cachedToken.getRawString() before proceeding.
ℹ️ Review info
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
packages/clerk-js/src/core/resources/Session.tspackages/clerk-js/src/core/resources/__tests__/Session.test.ts
| if (shouldDispatchTokenUpdate && (cachedToken.getRawString() || isValidBrowserOnline())) { | ||
| eventBus.emit(events.TokenUpdate, { token: cachedToken }); | ||
| } |
There was a problem hiding this comment.
Empty-token token:update can still fire after offline→online race.
The current guards allow dispatch/caching when getRawString() is empty but isValidBrowserOnline() is true. If the request failed while offline and connectivity flips before dispatch, this can still emit an empty token and clear __session.
Proposed fix
- if (shouldDispatchTokenUpdate && (cachedToken.getRawString() || isValidBrowserOnline())) {
+ if (shouldDispatchTokenUpdate && cachedToken.getRawString()) {
eventBus.emit(events.TokenUpdate, { token: cachedToken });
}
@@
- if (!token.getRawString() && !isValidBrowserOnline()) {
+ if (!token.getRawString()) {
return;
}
@@
- if (!token.getRawString() && !isValidBrowserOnline()) {
+ if (!token.getRawString()) {
return;
}Also applies to: 496-500, 565-579
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/clerk-js/src/core/resources/Session.ts` around lines 451 - 453, The
token-update path currently emits and caches/clears when isValidBrowserOnline()
is true even if cachedToken.getRawString() is empty; change the guard so
eventBus.emit(events.TokenUpdate, { token: cachedToken }) and any code that
writes/clears __session only run when cachedToken.getRawString() is truthy
(i.e., require a non-empty token), and do not rely on isValidBrowserOnline() as
a substitute for a missing token. Apply this same change to the other
TokenUpdate/caching occurrences that use cachedToken.getRawString() ||
isValidBrowserOnline() so all places (the eventBus.emit call and the __session
write/clear logic) first check cachedToken.getRawString() before proceeding.
3 participants
Description
When offline, two code paths in
Session.tsemittoken:updateevents with empty tokens.AuthCookieServiceinterprets empty tokens as signed-out and removes the__session cookie— even though the session is still valid server-side. On the next page load or visibility change, the missing cookie makes the user appear signed out.Checklist
pnpm testruns as expected.pnpm buildruns as expected.Type of change
Summary by CodeRabbit
Bug Fixes
Tests
Chores