refactor(js): extract the refresh scheduler from TokenCache#9042
Open
jacekradko wants to merge 1 commit into
Open
refactor(js): extract the refresh scheduler from TokenCache#9042jacekradko wants to merge 1 commit into
jacekradko wants to merge 1 commit into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🦋 Changeset detectedLatest commit: 46fcf73 The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 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 |
Contributor
📝 WalkthroughWalkthroughAdds a new ChangesRefreshScheduler abstraction and token cache integration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
Comment |
@clerk/astro
@clerk/backend
@clerk/chrome-extension
@clerk/clerk-js
@clerk/electron
@clerk/electron-passkeys
@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: |
Contributor
API Changes Report
Summary
No API Changes DetectedAll packages have stable APIs with no detected changes. Report generated by Break Check Last ran on |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/clerk-js/src/core/refreshScheduler.ts (1)
65-69: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid
anyin theunref()path.This helper can stay cross-runtime without dropping type safety; a small guard is enough to call
unref()only on Node timer handles.♻️ Proposed refactor
+interface UnrefTimer { + unref(): void; +} + +const hasUnref = ( + value: ReturnType<typeof setTimeout>, +): value is ReturnType<typeof setTimeout> & UnrefTimer => + typeof value === 'object' && value !== null && 'unref' in value && typeof value.unref === 'function'; + const armTimer = (callback: () => void, delayMs: number): ReturnType<typeof setTimeout> => { const id = setTimeout(callback, delayMs); - if (typeof (id as any).unref === 'function') { - (id as any).unref(); + if (hasUnref(id)) { + id.unref(); } return id; };As per coding guidelines, "Avoid
anytype - preferunknownwhen type is uncertain, then narrow with type guards" and "Noanytypes without justification in code review".🤖 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/clerk-js/src/core/refreshScheduler.ts` around lines 65 - 69, The armTimer helper in refreshScheduler currently uses an any cast to call unref() on the timer handle, which violates the no-any guideline. Update armTimer to use a type-safe narrowing approach (for example, a small guard on the setTimeout result) so unref() is only called when the handle actually supports it, while keeping the cross-runtime behavior intact.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/clerk-js/src/core/refreshScheduler.ts`:
- Around line 65-69: The armTimer helper in refreshScheduler currently uses an
any cast to call unref() on the timer handle, which violates the no-any
guideline. Update armTimer to use a type-safe narrowing approach (for example, a
small guard on the setTimeout result) so unref() is only called when the handle
actually supports it, while keeping the cross-runtime behavior intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Repository UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: bf4cce79-ae41-43b1-9ece-833e22a0cc19
📒 Files selected for processing (5)
.changeset/sdk-140-refresh-scheduler.mdpackages/clerk-js/src/core/__tests__/refreshScheduler.test.tspackages/clerk-js/src/core/__tests__/tokenCache.test.tspackages/clerk-js/src/core/refreshScheduler.tspackages/clerk-js/src/core/tokenCache.ts
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
1 participant
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.
Step 4 of the TokenCache decouple (SDK-117): lifts both
setTimeouttimers (expiration cleanup + proactive refresh) out oftokenCache.tsinto acreateRefreshSchedulercollaborator with an injected clock, matching thetokenStore/keyResolvershape from the earlier steps. Timers staysetTimeout-based and the clock only suppliesnow(), so the existing ~30 fake-timer tests pass unchanged.Two things worth a close read:
Refresh delays are now recomputed from the token's absolute
exp(exp - now() - leeway - lead) rather than a fixedexpiresInmeasured from cache-set time. Identical for freshly fetched tokens (iat == now), but for a token hydrated from the session cookie before the tab loaded, today's code schedules the proactive refresh a full lifetime out (past expiry, so it never fires proactively); the recompute fixes that.refreshScheduler.test.tspins it down (a past-issuance token fires at 13s, not 43s).deleteKeyis both the expiration-timer callback and the resolver chain's.catch, so itsscheduler.cancel(key)stays inside thestore.get(key) === valueidentity guard. Otherwise a stale rejected resolver could disarm the replacement entry's live timers. A new tokenCache test covers that path.Deliberately deferred to a follow-up: the startup catch-up sweep, re-arm on visibility/online (overlaps
AuthCookieService's existing focus/visibility refresh), and failure backoff.Closes SDK-140.
Summary by CodeRabbit
New Features
Bug Fixes