refactor(js): extract a generic store from TokenCache#8860
Conversation
Lock in current cross-tab, lifecycle, degradation, and audience-keying behavior as the regression bar for the TokenCache decoupling (SDK-117). The broadcast postMessage-failure case is a known pre-existing bug (SDK-119), captured as an it.fails tripwire.
Lift the raw key/value storage out of MemoryTokenCache into a small generic createTokenStore<V> (pure storage: no timers, no BroadcastChannel, no JWT knowledge) and rewire the cache onto it. No behavior change; the identity-based deleteKey guard and overwrite check are preserved. First structural step of SDK-117.
🦋 Changeset detectedLatest commit: b8c8d55 The changes in this PR will be included in the next version bump. This PR includes changesets to release 0 packagesWhen changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types 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.
|
📝 WalkthroughWalkthroughAdds a generic ChangesTokenCache Store Decoupling
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 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: |
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}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/clerk-js/src/core/tokenStore.ts (1)
22-24: ⚡ Quick winAdd
@returnstag to JSDoc for customer-facing documentation.Since this is a public/reference-facing API, the JSDoc should include a
@returnstag. Clerk Docs generates reference documentation from JSDoc comments in this repository, so completeness is important for customer-facing clarity.📝 Suggested JSDoc enhancement
/** * Creates an empty in-memory {`@link` TokenStore} backed by a Map. + * + * `@returns` A new TokenStore instance with no entries */As per coding guidelines: "Document functions with JSDoc comments including
@param,@returns,@throws, and@exampletags" and "Treat JSDoc on public/reference-facing APIs as customer-facing documentation."🤖 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/tokenStore.ts` around lines 22 - 24, The JSDoc comment for the function that creates an empty in-memory TokenStore is missing the `@returns` tag. Add a `@returns` tag to the JSDoc block that documents what type of object is returned by this function, describing that it returns a TokenStore instance. This will ensure the auto-generated customer-facing documentation is complete and provides clear information about the function's return value.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/tokenStore.ts`:
- Around line 22-24: The JSDoc comment for the function that creates an empty
in-memory TokenStore is missing the `@returns` tag. Add a `@returns` tag to the
JSDoc block that documents what type of object is returned by this function,
describing that it returns a TokenStore instance. This will ensure the
auto-generated customer-facing documentation is complete and provides clear
information about the function's return value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Repository UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 9c96bed2-1827-46a8-8152-0445c5ffc9ad
📒 Files selected for processing (5)
.changeset/decouple-tokencache-store.mdpackages/clerk-js/src/core/__tests__/tokenCache.test.tspackages/clerk-js/src/core/__tests__/tokenStore.test.tspackages/clerk-js/src/core/tokenCache.tspackages/clerk-js/src/core/tokenStore.ts
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/clerk-js/src/core/tokenCache.ts`:
- Around line 437-480: The SDK-119 regression tripwire is now stale because the
broadcast failure is intentionally swallowed in tokenCache’s token update path,
so update the related test to pass normally or remove it if it no longer adds
coverage, and delete the “known bug”/`it.fails` expectation tied to this
behavior. Make sure the test still exercises the token broadcast flow around the
tokenCache update logic and reflects the new best-effort behavior, and update
the changeset/release note to describe the fix instead of referencing a known
failure.
- Line 443: The broadcast token id lookup in tokenCache should not rely on an
unsafe any cast for claims.o; keep claims.org_id as-is, but narrow claims.o to a
valid object with a string id before passing it into TokenId.build. Update the
organizationId assignment in tokenCache so it only uses claims.o.id when the
shape is verified, avoiding non-string values from reaching the broadcast match
logic.
🪄 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 Plus
Run ID: e4a4eed5-733c-41cb-a954-a7eab78d61d7
📒 Files selected for processing (2)
packages/clerk-js/src/core/__tests__/tokenCache.test.tspackages/clerk-js/src/core/tokenCache.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/clerk-js/src/core/tests/tokenCache.test.ts
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
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/clerk-js/src/core/tokenCache.ts`:
- Around line 437-480: The SDK-119 regression tripwire is now stale because the
broadcast failure is intentionally swallowed in tokenCache’s token update path,
so update the related test to pass normally or remove it if it no longer adds
coverage, and delete the “known bug”/`it.fails` expectation tied to this
behavior. Make sure the test still exercises the token broadcast flow around the
tokenCache update logic and reflects the new best-effort behavior, and update
the changeset/release note to describe the fix instead of referencing a known
failure.
- Line 443: The broadcast token id lookup in tokenCache should not rely on an
unsafe any cast for claims.o; keep claims.org_id as-is, but narrow claims.o to a
valid object with a string id before passing it into TokenId.build. Update the
organizationId assignment in tokenCache so it only uses claims.o.id when the
shape is verified, avoiding non-string values from reaching the broadcast match
logic.
🪄 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 Plus
Run ID: e4a4eed5-733c-41cb-a954-a7eab78d61d7
📒 Files selected for processing (2)
packages/clerk-js/src/core/__tests__/tokenCache.test.tspackages/clerk-js/src/core/tokenCache.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/clerk-js/src/core/tests/tokenCache.test.ts
🛑 Comments failed to post (2)
packages/clerk-js/src/core/tokenCache.ts (2)
437-480: 📐 Maintainability & Code Quality | 🟡 Minor
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Verify whether SDK-119 is still documented as an expected failure while tokenCache.ts fixes it. rg -n "SDK-119|it\.fails|postMessage|Failed to broadcast" \ packages/clerk-js/src/core/__tests__/tokenCache.test.ts \ packages/clerk-js/src/core/tokenCache.ts \ .changeset || trueRepository: clerk/javascript
Length of output: 3995
🏁 Script executed:
#!/bin/bash set -euo pipefail # Inspect the relevant tokenCache tests and any changeset references to SDK-119. sed -n '480,560p' packages/clerk-js/src/core/__tests__/tokenCache.test.ts printf '\n---\n' sed -n '1660,1705p' packages/clerk-js/src/core/__tests__/tokenCache.test.ts printf '\n---\n' rg -n "SDK-119|tokenCache|postMessage" .changeset packages/clerk-js/src/core/__tests__/tokenCache.test.ts packages/clerk-js/src/core/tokenCache.ts || trueRepository: clerk/javascript
Length of output: 9301
🏁 Script executed:
#!/bin/bash set -euo pipefail # Check whether SDK-119 is tracked in a changeset or release note. fd -a .changeset -t f | xargs -r rg -n "SDK-119|tokenCache|postMessage" || trueRepository: clerk/javascript
Length of output: 154
🏁 Script executed:
#!/bin/bash set -euo pipefail # Inspect the relevant tokenCache tests and any changeset references to SDK-119. sed -n '480,560p' packages/clerk-js/src/core/__tests__/tokenCache.test.ts printf '\n---\n' sed -n '1660,1705p' packages/clerk-js/src/core/__tests__/tokenCache.test.ts printf '\n---\n' rg -n "SDK-119|tokenCache|postMessage" .changeset packages/clerk-js/src/core/__tests__/tokenCache.test.ts packages/clerk-js/src/core/tokenCache.ts || trueRepository: clerk/javascript
Length of output: 9301
🏁 Script executed:
#!/bin/bash set -euo pipefail # Check whether SDK-119 is tracked in a changeset or release note. fd -a .changeset -t f | xargs -r rg -n "SDK-119|tokenCache|postMessage" || trueRepository: clerk/javascript
Length of output: 154
Remove the stale SDK-119 tripwire. This code now swallows broadcast failures, so the
it.failsregression test and its “known bug” comment should be flipped to a passing test or removed if it’s redundant, and the changeset/release note should describe the fix.🧰 Tools
🪛 ast-grep (0.44.0)
[warning] 447-447: Do not use Math.random() to generate security-sensitive values such as tokens, secrets, passwords, API keys, salts, nonces, OTPs, or session IDs. Math.random() is not cryptographically secure and is predictable. Use crypto.randomBytes()/crypto.randomUUID() (Node) or crypto.getRandomValues() (Web Crypto) instead.
Context: Math.random()
Note: [CWE-330] Use of Insufficiently Random Values.(insecure-random-security-token-typescript)
🤖 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/tokenCache.ts` around lines 437 - 480, The SDK-119 regression tripwire is now stale because the broadcast failure is intentionally swallowed in tokenCache’s token update path, so update the related test to pass normally or remove it if it no longer adds coverage, and delete the “known bug”/`it.fails` expectation tied to this behavior. Make sure the test still exercises the token broadcast flow around the tokenCache update logic and reflects the new best-effort behavior, and update the changeset/release note to describe the fix instead of referencing a known failure.
443-443: 🎯 Functional Correctness | 🟡 Minor
🧩 Analysis chain
🏁 Script executed:
sed -n '380,470p' packages/clerk-js/src/core/tokenCache.tsRepository: clerk/javascript
Length of output: 3948
🏁 Script executed:
sed -n '380,470p' packages/clerk-js/src/core/tokenCache.tsRepository: clerk/javascript
Length of output: 3948
🏁 Script executed:
rg -n "claims\.o|org_id|TokenId\.build|extractTemplate|interface .*claims|type .*claims" packages/clerk-js/src/core/tokenCache.ts packages/clerk-js/src -g '!**/*.map'Repository: clerk/javascript
Length of output: 9739
🏁 Script executed:
sed -n '1,220p' packages/clerk-js/src/core/tokenCache.ts && printf '\n---\n' && rg -n "claims\.o|org_id|TokenId\.build|extractTemplate|interface .*claims|type .*claims" packages/clerk-js/src -g '!**/*.map'Repository: clerk/javascript
Length of output: 15873
🏁 Script executed:
sed -n '1,220p' packages/clerk-js/src/core/tokenCache.ts && printf '\n---\n' && rg -n "claims\.o|org_id|TokenId\.build|extractTemplate|interface .*claims|type .*claims" packages/clerk-js/src -g '!**/*.map'Repository: clerk/javascript
Length of output: 15873
🏁 Script executed:
sed -n '1,220p' packages/clerk-js/src/core/tokenCache.ts && printf '\n---\n' && rg -n "claims\.o|org_id|TokenId\.build|extractTemplate|interface .*claims|type .*claims" packages/clerk-js/src -g '!**/*.map'Repository: clerk/javascript
Length of output: 15873
🏁 Script executed:
rg -n "jwt\?\.(claims|claims:)|interface .*claims|type .*claims|claims\.o|org_id" packages/clerk-js/src packages/shared/src packages -g '!**/*.map' | sed -n '1,200p'Repository: clerk/javascript
Length of output: 14077
🏁 Script executed:
sed -n '1,220p' packages/clerk-js/src/utils/tokenId.ts && printf '\n---\n' && sed -n '1,220p' packages/clerk-js/src/core/tokenFreshness.ts && printf '\n---\n' && rg -n "jwt\?\.(claims|claims:)|interface .*claims|type .*claims|claims\.o|org_id" packages/clerk-js/src packages/shared/src packages -g '!**/*.map' | sed -n '1,220p'Repository: clerk/javascript
Length of output: 18631
🏁 Script executed:
sed -n '1,220p' packages/clerk-js/src/utils/tokenId.ts && printf '\n---\n' && sed -n '1,220p' packages/clerk-js/src/core/tokenFreshness.ts && printf '\n---\n' && rg -n "jwt\?\.(claims|claims:)|interface .*claims|type .*claims|claims\.o|org_id" packages/clerk-js/src packages/shared/src packages -g '!**/*.map' | sed -n '1,220p'Repository: clerk/javascript
Length of output: 18631
🏁 Script executed:
sed -n '1,220p' packages/clerk-js/src/utils/tokenId.ts && printf '\n---\n' && sed -n '1,220p' packages/clerk-js/src/core/tokenFreshness.ts && printf '\n---\n' && rg -n "jwt\?\.(claims|claims:)|interface .*claims|type .*claims|claims\.o|org_id" packages/clerk-js/src packages/shared/src packages -g '!**/*.map' | sed -n '1,220p'Repository: clerk/javascript
Length of output: 18631
🏁 Script executed:
sed -n '1,260p' packages/shared/src/types/jwt.ts && printf '\n---\n' && sed -n '1,260p' packages/shared/src/types/jwtv2.ts && printf '\n---\n' && sed -n '420,500p' packages/clerk-js/src/core/tokenCache.tsRepository: clerk/javascript
Length of output: 10192
🏁 Script executed:
sed -n '1,260p' packages/shared/src/types/jwt.ts && printf '\n---\n' && sed -n '1,260p' packages/shared/src/types/jwtv2.ts && printf '\n---\n' && sed -n '420,500p' packages/clerk-js/src/core/tokenCache.tsRepository: clerk/javascript
Length of output: 10192
Narrow
claims.obefore building the broadcast token id.
claims.org_idis already safe, but(claims.o as any)?.idcan still pass a non-string value intoTokenId.build, which can break the broadcast match. Use an object/string guard here instead ofany.🤖 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/tokenCache.ts` at line 443, The broadcast token id lookup in tokenCache should not rely on an unsafe any cast for claims.o; keep claims.org_id as-is, but narrow claims.o to a valid object with a string id before passing it into TokenId.build. Update the organizationId assignment in tokenCache so it only uses claims.o.id when the shape is verified, avoiding non-string values from reaching the broadcast match logic.Source: Coding guidelines
…cy budget
The broadcast try/catch fix for SDK-119 landed on main, so merging it in made the duplicated `it.fails` tripwire flip red ("Expect test to fail" once the body started passing). Remove it; the regular assertion already added on main covers the same contract.
Bump the legacy browser bundle budget 114KB -> 115KB so the extracted token store module fits. main sat at 113.99KB with ~10 bytes of headroom, and any separate store module costs ~50 bytes regardless of style.
2 participants
First slice of the TokenCache decoupling (SDK-117), no behavior change. It backfills characterization tests that pin down current cache behavior, then lifts the raw key/value storage out of
MemoryTokenCacheinto a genericcreateTokenStore<V>and rewires the cache onto it, so storage is testable on its own without timers or channel mocks.The part worth a careful read is the
cache.* -> store.*rewire intokenCache.ts. ThedeleteKeyidentity check and the overwrite guard both rely onstore.get(key) !== valuepreserving reference identity, which the Map-backed store does. The full clerk-js suite stays green with Session and Client as the real consumers.The legacy browser bundle budget ticks up from 114KB to 115KB because main already sits at 113.99KB and the extracted store module costs about 50 bytes. Empty changeset, since nothing here is user-facing.
Summary by CodeRabbit
Refactor
Tests
Chores