feat: add support for sync helpers #186
Conversation
WalkthroughAdds synchronous counterparts for many token utilities and re-exports them; marks stores with an Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant TokenUtils
participant SessionStore
rect rgb(221,238,255)
Note over TokenUtils,SessionStore: synchronous helpers (new)
Caller->>TokenUtils: getDecodedTokenSync(tokenType)
TokenUtils->>SessionStore: getSessionItem(tokenKey) [sync]
alt no storage or no token
SessionStore-->>TokenUtils: null
TokenUtils-->>Caller: null
else token string
SessionStore-->>TokenUtils: tokenString
TokenUtils->>TokenUtils: _decodeTokenCore(tokenString)
TokenUtils-->>Caller: decodedToken
end
end
sequenceDiagram
participant Caller
participant TokenUtils
participant SessionStore
rect rgb(255,245,235)
Note over TokenUtils,SessionStore: existing async helpers
Caller->>TokenUtils: await getDecodedToken(tokenType)
TokenUtils->>SessionStore: await getItems(tokenKey) [async]
alt no storage or no token
SessionStore-->>TokenUtils: null
TokenUtils-->>Caller: null
else token string
SessionStore-->>TokenUtils: tokenString
TokenUtils->>TokenUtils: _decodeTokenCore(tokenString)
TokenUtils-->>Caller: decodedToken
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
lib/utils/token/getUserProfile.ts (1)
12-38: Return type claims& Tbut T is never included in the object._core builds only UserProfile fields; consumers seeing
(UserProfile & T)will assume extra fields exist. Either drop the generic or actually merge T from the token.Option A (simplest): return only UserProfile.
-const _getUserProfileCore = <T>( - idToken: - | (T & { - sub: string; - given_name: string; - family_name: string; - email: string; - picture: string; - }) - | null, -): (UserProfile & T) | null => { +const _getUserProfileCore = ( + idToken: + | { + sub?: string; + given_name?: string; + family_name?: string; + email?: string; + picture?: string; + } + | null, +): UserProfile | null => { if (!idToken) { return null; } - const { sub } = idToken; - if (!sub) { + const { sub } = idToken; + if (!sub) { console.error("No sub in idToken"); return null; } return { - id: idToken.sub, - givenName: idToken.given_name, - familyName: idToken.family_name, - email: idToken.email, - picture: idToken.picture, - } as UserProfile & T; + id: idToken.sub, + givenName: idToken.given_name, + familyName: idToken.family_name, + email: idToken.email, + picture: idToken.picture, + } as UserProfile; };lib/utils/token/getFlag.ts (2)
25-32: API lookup usesnameinstead ofkey; returns null erroneously.
AccountFeatureFlagsResultentries are keyed bykey(see getFlags.ts usage). Matching onnamelikely never hits.- const flag = data.feature_flags.find((flag) => flag.name === name); + const flag = data.feature_flags.find((flag) => flag.key === name);Add a unit test for the
forceApipath to prevent regressions.
17-20: JSDoc return type is outdated.The return type comment excludes
boolean/objectand mentionsstring[]. Update to reflect genericT | null.- * @returns { Promise<string | number | string[] | null> } + * @returns { Promise<T | null> }
🧹 Nitpick comments (15)
lib/utils/token/getDecodedToken.test.ts (1)
53-65: Consider testing the async-only store error case.The tests cover the happy path and null case well. However, consider adding a test that verifies
getDecodedTokenSyncthrows the expected error when called with an async-only store (e.g., ChromeStore or ExpoSecureStore).Example test to add:
it("throws error when storage is async-only", () => { const asyncStorage = new ChromeStore(); setActiveStorage(asyncStorage); expect(() => getDecodedTokenSync("accessToken")).toThrow( "Active storage is async-only. Use the async helpers." ); });lib/utils/token/getRawToken.test.ts (1)
60-72: Consider testing the async-only store error case.The tests cover the happy path and null case well. However, consider adding a test that verifies
getRawTokenSyncreturns null (based on line 26 of getRawToken.ts) when called with an async-only store, for consistency with the sync API behavior.Example test to add:
it("returns null when storage is async-only", () => { const asyncStorage = new ChromeStore(); setActiveStorage(asyncStorage); expect(getRawTokenSync("accessToken")).toBe(null); });lib/utils/token/getUserProfile.test.ts (1)
86-117: LGTM! Core scenarios covered.The synchronous tests validate null handling and basic profile mapping. The test coverage is appropriate for the sync variant given that the core logic is shared via
_getUserProfileCore.Optionally, you could add a test for the "no sub" scenario (like the async version at lines 57-83) to ensure the sync path also handles missing subject identifiers correctly.
lib/utils/token/getUserOrganizationsHasura.test.ts (1)
49-53: Reset store state between tests.Call destroySession in beforeEach to avoid leaking tokens from prior tests.
describe("getUserOrganizationsSync - Hasura", () => { beforeEach(() => { - setActiveStorage(storage); + storage.destroySession(); + setActiveStorage(storage); });lib/utils/token/getPermission.test.ts (2)
18-26: Prefer removing the token over storing null.Setting null is stored as a value in MemoryStorage; remove the item to represent “no token”.
- storage.setSessionItem(StorageKeys.accessToken, null); + storage.removeSessionItem(StorageKeys.accessToken);
8-10: Unnecessary mock for this file.The sync tests don’t call the account API; consider dropping this mock to reduce noise.
-vi.mock("./accountApi/callAccountApi", () => ({ - callAccountApi: vi.fn(), -})); +// No API calls in sync tests; mock not required here.lib/utils/token/getRoles.test.ts (1)
364-384: Harden the sync suite and cover error path.
- Reset storage per test to avoid state coupling.
- Remove the token instead of storing null.
- Add a test asserting getRolesSync({ forceApi: true }) throws.
describe("getRolesSync", () => { beforeEach(() => { - setActiveStorage(storage); + storage.destroySession(); + setActiveStorage(storage); }); it("returns [] when no token", () => { - storage.setSessionItem(StorageKeys.accessToken, null); + storage.removeSessionItem(StorageKeys.accessToken); expect(getRolesSync()).toStrictEqual([]); }); + it("throws when forceApi is true", () => { + expect(() => getRolesSync({ forceApi: true })).toThrow( + "forceApi cannot be used in sync mode", + ); + });lib/utils/token/getCurrentOrganizationHasura.test.ts (2)
37-40: Reset store state between tests.Add destroySession in beforeEach to guarantee isolation.
describe("getCurrentOrganizationSync - Hasura", () => { beforeEach(() => { - setActiveStorage(storage); + storage.destroySession(); + setActiveStorage(storage); });
41-51: Add a no-token sync case for parity.Mirror the async “no token” scenario to assert null in sync mode.
it("with access", () => { // existing test... }); + + it("when no token", () => { + storage.removeSessionItem(StorageKeys.accessToken); + expect(getCurrentOrganizationSync()).toBeNull(); + });lib/utils/token/getUserProfile.ts (1)
28-29: Library logging footprint.console.error in core can be noisy; consider warn/debug or remove.
lib/utils/token/getRoles.ts (1)
15-18: Reuse existing JWTDecoded type; avoid local alias drift.JWTDecoded already models both roles and x-hasura-roles; type _getRolesCore with it to prevent divergence. Based on learnings.
-import { getDecodedTokenSync } from "./getDecodedToken"; +import { getDecodedTokenSync } from "./getDecodedToken"; +import type { JWTDecoded } from "./getDecodedToken"; @@ -type TokenWithRoles = { - roles?: Role[]; - "x-hasura-roles"?: Role[]; -} | null; - -const _getRolesCore = (token: TokenWithRoles): Role[] => { +const _getRolesCore = (token: JWTDecoded | null): Role[] => { if (!token) { return []; } if (!token.roles && !token["x-hasura-roles"]) { console.warn( "No roles found in token, ensure roles have been included in the token customisation within the application settings", ); return []; } return (token.roles || token["x-hasura-roles"]) as Role[]; };lib/utils/token/getFlags.ts (1)
47-62: Core extraction is solid; consider dedup with getFlag.
_getFlagsCoremirrors logic ingetFlag._getFlagCore. Consider a single shared extractor to avoid drift.lib/utils/token/getFlag.ts (1)
50-63: Core extractor is fine; optional dedup with getFlags.Consider sharing a single flag-extraction helper with
getFlagsto reduce duplication.lib/utils/token/getUserOrganizations.ts (1)
1-31: LGTM! Excellent refactoring.The extraction of
_getUserOrganizationsCoreis a clean refactoring that eliminates code duplication between the async and sync variants while maintaining the same behavior for both paths.lib/utils/token/index.ts (1)
11-33: LGTM! Well-organized exports.The reorganization to group async and sync variants together improves readability and maintainability. All synchronous helper variants are properly exported.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (35)
lib/main.test.ts(1 hunks)lib/main.ts(1 hunks)lib/sessionManager/stores/chromeStore.ts(1 hunks)lib/sessionManager/stores/expoSecureStore.ts(1 hunks)lib/sessionManager/stores/localStorage.test.ts(1 hunks)lib/sessionManager/stores/localStorage.ts(5 hunks)lib/sessionManager/stores/memory.ts(4 hunks)lib/sessionManager/types.ts(6 hunks)lib/utils/activityTracking.test.ts(1 hunks)lib/utils/token/getClaim.test.ts(2 hunks)lib/utils/token/getClaim.ts(2 hunks)lib/utils/token/getClaims.ts(2 hunks)lib/utils/token/getCurrentOrganization.test.ts(3 hunks)lib/utils/token/getCurrentOrganization.ts(2 hunks)lib/utils/token/getCurrentOrganizationHasura.test.ts(2 hunks)lib/utils/token/getDecodedToken.test.ts(2 hunks)lib/utils/token/getDecodedToken.ts(1 hunks)lib/utils/token/getFlag.test.ts(2 hunks)lib/utils/token/getFlag.ts(2 hunks)lib/utils/token/getFlags.test.ts(2 hunks)lib/utils/token/getFlags.ts(2 hunks)lib/utils/token/getPermission.test.ts(1 hunks)lib/utils/token/getPermission.ts(3 hunks)lib/utils/token/getPermissions.test.ts(2 hunks)lib/utils/token/getPermissions.ts(3 hunks)lib/utils/token/getRawToken.test.ts(2 hunks)lib/utils/token/getRawToken.ts(1 hunks)lib/utils/token/getRoles.test.ts(2 hunks)lib/utils/token/getRoles.ts(3 hunks)lib/utils/token/getUserOrganizations.test.ts(3 hunks)lib/utils/token/getUserOrganizations.ts(1 hunks)lib/utils/token/getUserOrganizationsHasura.test.ts(2 hunks)lib/utils/token/getUserProfile.test.ts(2 hunks)lib/utils/token/getUserProfile.ts(3 hunks)lib/utils/token/index.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-02-24T16:16:36.437Z
Learnt from: DanielRivers
PR: kinde-oss/js-utils#86
File: lib/utils/token/getRoles.ts:8-23
Timestamp: 2025-02-24T16:16:36.437Z
Learning: The getDecodedToken function's return type (JWTDecoded) already includes type definitions for both traditional roles (roles) and Hasura roles (x-hasura-roles) through its union of JWTExtra and JWTExtraHasura interfaces.
Applied to files:
lib/utils/token/getClaims.tslib/utils/token/getCurrentOrganization.tslib/utils/token/getRoles.tslib/utils/token/getUserOrganizations.tslib/utils/token/getDecodedToken.ts
📚 Learning: 2024-11-19T20:31:59.197Z
Learnt from: DanielRivers
PR: kinde-oss/js-utils#11
File: lib/utils/token/index.test.ts:59-62
Timestamp: 2024-11-19T20:31:59.197Z
Learning: In `lib/utils/token/index.test.ts`, the test "getInsecureStorage returns active storage when no insecure storage is set" is intentional. It verifies that when insecure storage is not set, `getInsecureStorage()` returns the active storage as a fallback, preferring secure storage but allowing for insecure storage when needed by consumers of the library.
Applied to files:
lib/utils/token/getCurrentOrganization.test.tslib/utils/token/getDecodedToken.test.tslib/utils/token/getClaim.test.tslib/utils/token/getFlag.test.tslib/utils/token/getRawToken.test.ts
📚 Learning: 2024-10-25T14:24:05.260Z
Learnt from: DanielRivers
PR: kinde-oss/js-utils#16
File: lib/sessionManager/types.ts:75-80
Timestamp: 2024-10-25T14:24:05.260Z
Learning: All storage classes (`MemoryStorage`, `LocalStorage`, `ChromeStore`, `ExpoSecureStore`) extend `SessionBase`, inheriting the `setItems` method, so they do not need to implement `setItems` explicitly.
Applied to files:
lib/sessionManager/stores/expoSecureStore.ts
🧬 Code graph analysis (25)
lib/utils/token/getClaims.ts (1)
lib/utils/token/getDecodedToken.ts (2)
JWTDecoded(45-45)getDecodedTokenSync(76-91)
lib/utils/token/getCurrentOrganization.test.ts (3)
lib/utils/token/index.ts (2)
setActiveStorage(49-55)getCurrentOrganizationSync(14-14)lib/utils/token/getCurrentOrganization.ts (1)
getCurrentOrganizationSync(17-20)lib/utils/token/testUtils/index.ts (1)
createMockAccessToken(23-44)
lib/utils/token/getDecodedToken.test.ts (3)
lib/utils/token/getDecodedToken.ts (1)
getDecodedTokenSync(76-91)lib/sessionManager/stores/memory.ts (1)
MemoryStorage(9-91)lib/utils/token/testUtils/index.ts (1)
createMockAccessToken(23-44)
lib/utils/token/getPermission.test.ts (4)
lib/sessionManager/stores/memory.ts (1)
MemoryStorage(9-91)lib/utils/token/index.ts (2)
setActiveStorage(49-55)getPermissionSync(25-25)lib/utils/token/getPermission.ts (1)
getPermissionSync(50-60)lib/utils/token/testUtils/index.ts (1)
createMockAccessToken(23-44)
lib/utils/token/getRawToken.ts (1)
lib/utils/token/index.ts (1)
getActiveStorage(61-63)
lib/utils/token/getClaim.test.ts (2)
lib/utils/token/index.ts (2)
setActiveStorage(49-55)getClaimSync(11-11)lib/utils/token/testUtils/index.ts (1)
createMockAccessToken(23-44)
lib/utils/token/getUserOrganizationsHasura.test.ts (3)
lib/utils/token/index.ts (2)
setActiveStorage(49-55)getUserOrganizationsSync(31-31)lib/utils/token/testUtils/index.ts (1)
createMockAccessToken(23-44)lib/utils/token/getUserOrganizations.ts (1)
getUserOrganizationsSync(13-16)
lib/utils/token/getCurrentOrganization.ts (1)
lib/utils/token/getDecodedToken.ts (2)
getDecodedTokenSync(76-91)JWTDecoded(45-45)
lib/utils/token/getClaim.ts (1)
lib/utils/token/getClaims.ts (1)
getClaimsSync(15-19)
lib/utils/token/getPermissions.ts (2)
lib/utils/token/getDecodedToken.ts (2)
JWTDecoded(45-45)getDecodedTokenSync(76-91)lib/types.ts (1)
GetPermissionsOptions(273-273)
lib/utils/token/getFlag.test.ts (3)
lib/utils/token/index.ts (2)
setActiveStorage(49-55)getFlagSync(18-18)lib/utils/token/getFlag.ts (1)
getFlagSync(38-48)lib/utils/token/testUtils/index.ts (1)
createMockAccessToken(23-44)
lib/utils/token/getFlag.ts (2)
lib/types.ts (1)
GetFeatureFlagsOptions(275-275)lib/utils/token/getDecodedToken.ts (1)
getDecodedTokenSync(76-91)
lib/utils/token/getCurrentOrganizationHasura.test.ts (3)
lib/utils/token/index.ts (2)
setActiveStorage(49-55)getCurrentOrganizationSync(14-14)lib/utils/token/testUtils/index.ts (1)
createMockAccessToken(23-44)lib/utils/token/getCurrentOrganization.ts (1)
getCurrentOrganizationSync(17-20)
lib/utils/token/getFlags.test.ts (3)
lib/utils/token/index.ts (2)
setActiveStorage(49-55)getFlagsSync(20-20)lib/utils/token/getFlags.ts (1)
getFlagsSync(36-45)lib/utils/token/testUtils/index.ts (1)
createMockAccessToken(23-44)
lib/utils/token/getUserProfile.test.ts (3)
lib/utils/token/index.ts (2)
setActiveStorage(49-55)getUserProfileSync(22-22)lib/utils/token/getUserProfile.ts (1)
getUserProfileSync(53-62)lib/utils/token/testUtils/index.ts (1)
createMockAccessToken(23-44)
lib/utils/token/getRoles.ts (2)
lib/types.ts (1)
GetRolesOptions(274-274)lib/utils/token/getDecodedToken.ts (1)
getDecodedTokenSync(76-91)
lib/utils/token/getRawToken.test.ts (4)
lib/utils/token/index.ts (2)
getRawTokenSync(16-16)setActiveStorage(49-55)lib/utils/token/getRawToken.ts (1)
getRawTokenSync(24-42)lib/sessionManager/stores/memory.ts (1)
MemoryStorage(9-91)lib/utils/token/testUtils/index.ts (1)
createMockAccessToken(23-44)
lib/utils/token/getUserOrganizations.test.ts (3)
lib/utils/token/index.ts (2)
setActiveStorage(49-55)getUserOrganizationsSync(31-31)lib/utils/token/getUserOrganizations.ts (1)
getUserOrganizationsSync(13-16)lib/utils/token/testUtils/index.ts (1)
createMockAccessToken(23-44)
lib/utils/token/getUserOrganizations.ts (1)
lib/utils/token/getDecodedToken.ts (3)
getDecodedToken(61-74)getDecodedTokenSync(76-91)JWTDecoded(45-45)
lib/utils/token/getUserProfile.ts (1)
lib/utils/token/getClaims.ts (2)
getClaims(9-13)getClaimsSync(15-19)
lib/utils/token/getRoles.test.ts (3)
lib/utils/token/index.ts (2)
setActiveStorage(49-55)getRolesSync(33-33)lib/utils/token/getRoles.ts (1)
getRolesSync(57-64)lib/utils/token/testUtils/index.ts (1)
createMockAccessToken(23-44)
lib/utils/token/getPermission.ts (1)
lib/utils/token/getDecodedToken.ts (2)
JWTDecoded(45-45)getDecodedTokenSync(76-91)
lib/utils/token/getPermissions.test.ts (3)
lib/utils/token/index.ts (2)
setActiveStorage(49-55)getPermissionsSync(28-28)lib/utils/token/getPermissions.ts (1)
getPermissionsSync(58-67)lib/utils/token/testUtils/index.ts (1)
createMockAccessToken(23-44)
lib/utils/token/getDecodedToken.ts (1)
lib/utils/token/index.ts (1)
getActiveStorage(61-63)
lib/utils/token/getFlags.ts (2)
lib/types.ts (1)
GetFeatureFlagsOptions(275-275)lib/utils/token/getDecodedToken.ts (2)
getDecodedTokenSync(76-91)JWTDecoded(45-45)
🔇 Additional comments (38)
lib/utils/activityTracking.test.ts (1)
216-226: LGTM! Test assertions simplified.The test assertions have been refactored from the
.resolvesmatcher pattern to directly awaiting promises and checking their resolved values. Both approaches are valid, and the new pattern is more straightforward.lib/utils/token/getDecodedToken.ts (2)
47-59: LGTM! Good refactoring to extract common logic.The
_decodeTokenCorehelper eliminates duplication between the async and sync variants while maintaining clear error handling.
76-91: LGTM! Sync implementation follows correct pattern.The synchronous variant properly:
- Validates storage existence
- Checks
asyncStoreflag and throws a clear error for async-only stores- Calls
getSessionItemsynchronously (without await)- Delegates decoding to the shared
_decodeTokenCorehelperThe type cast on line 88 to
string | nullis acceptable given that token storage always contains string values.lib/utils/token/getClaims.ts (1)
15-19: LGTM! Sync variant follows established pattern.The
getClaimsSyncfunction correctly delegates togetDecodedTokenSyncand maintains type parameter consistency with the async variant.lib/sessionManager/stores/expoSecureStore.ts (1)
22-22: LGTM! Correctly marks store as async-only.Setting
asyncStore = trueis appropriate for ExpoSecureStore since all its storage methods (getSessionItem,setSessionItem,removeSessionItem,destroySession) are asynchronous.lib/sessionManager/stores/localStorage.test.ts (1)
291-291: LGTM! Reduces potential test flakiness.Doubling the wait time from 10ms to 20ms provides additional buffer for the asynchronous listener (which has a 5ms delay on line 281) to complete before assertions run. This should improve test reliability.
lib/sessionManager/stores/chromeStore.ts (1)
25-25: LGTM! Correctly marks store as async-only.Setting
asyncStore = trueis appropriate for ChromeStore since all its storage methods (getSessionItem,setSessionItem,removeSessionItem,destroySession) are asynchronous.lib/utils/token/getCurrentOrganization.ts (1)
1-29: LGTM! Clean refactoring with shared core logic.The extraction of
_getCurrentOrganizationCoreproperly centralizes the organization code retrieval logic, and both async and sync variants correctly delegate to it. The synchronous variant follows the established pattern by callinggetDecodedTokenSync.lib/utils/token/getFlags.test.ts (1)
341-372: LGTM! Adequate test coverage for synchronous variant.The test suite covers the essential scenarios: no token returns null, single flag extraction works, and
forceApicorrectly throws an error. The lighter coverage compared to the async tests is reasonable since the core extraction logic is shared via_getFlagsCore.lib/utils/token/getClaim.test.ts (1)
32-51: LGTM! Essential scenarios covered.The synchronous tests appropriately verify null handling and basic claim retrieval. Since the core logic is shared with the async variant via
_getClaimCore, this coverage is sufficient.lib/utils/token/getPermissions.test.ts (1)
164-191: LGTM! Comprehensive sync test coverage.The test suite properly validates the synchronous behavior: returning empty results when no token is present, correctly extracting org code and permissions from the token, and throwing an error when
forceApiis used in sync mode.lib/utils/token/getFlag.test.ts (1)
224-266: LGTM! Key scenarios validated.The test suite covers the critical paths for the synchronous API: null token handling, missing flags, successful flag retrieval, and the expected error when
forceApiis used in sync mode.lib/utils/token/getClaim.ts (2)
4-15: LGTM! Well-designed core helper.The
_getClaimCorefunction properly centralizes the claim extraction logic with correct null handling and type preservation. The type parametersTandVmaintain flexibility for callers.
34-43: LGTM! Synchronous variant follows the pattern.The
getClaimSyncfunction correctly delegates togetClaimsSyncfor synchronous claim retrieval and uses the shared_getClaimCorefor formatting.lib/utils/token/getUserOrganizationsHasura.test.ts (1)
54-60: LGTM on Hasura claim usage.Correct bracket notation for x-hasura-org-codes and expected array result.
lib/utils/token/getPermission.test.ts (2)
13-16: Good isolation.destroySession + setActiveStorage ensures clean sync test setup.
41-45: LGTM: sync-only option guard.forceApi error path is properly tested for the sync helper.
lib/utils/token/getRoles.ts (2)
20-31: Core extractor looks good.Clear, side-effect-limited roles resolution with sensible warnings.
57-63: Test the sync error branch.Add a unit test asserting getRolesSync({ forceApi: true }) throws.
You can append this to getRoles.test.ts “getRolesSync” block:
it("throws when forceApi is true", () => { expect(() => getRolesSync({ forceApi: true })).toThrow( "forceApi cannot be used in sync mode", ); });lib/utils/token/getUserProfile.ts (1)
53-62: Remove generics from both async and sync versions, or keep both as-is—the proposed asymmetric change is incorrect.The review's proposed diff has two critical issues:
Asymmetric API: It removes
<T>fromgetUserProfileSynconly, leavinggetUserProfile(async) unchanged. Both functions share identical generic patterns through_getUserProfileCoreand should be consistent.Incorrect return type: The diff claims the return type becomes
UserProfile | null, but without an explicitT, TypeScript infers it fromidToken. SincegetClaimsSyncreturns the intersection of those claim fields, the actual return would beUserProfile & { sub, given_name, family_name, email, picture } | null—not the simpler type shown.Test coverage shows generics are unused (no calls pass type arguments), suggesting either intentional API design for extensibility or dead code. If dead code, remove
<T>from both functions. Otherwise, keep both unchanged.Likely an incorrect or invalid review comment.
lib/utils/token/getPermission.ts (3)
2-2: Import update looks good.Directly importing
getDecodedTokenSyncandJWTDecodedavoids unnecessary index indirection.
47-48: Good reuse of shared core.Async path now funnels through
_getPermissionCore; behavior remains consistent.
50-60: Sync guard and flow LGTM.Throwing on
forceApiin sync mode is consistent with other sync helpers; sync path correctly reuses the core.lib/utils/token/getPermissions.ts (4)
18-34: Core helper handles Hasura/standard claims properly.Nice normalization of
permissionsandorgCodeacross token variants.
55-56: Async path parity via core helper looks good.
58-67: Sync guard and reuse LGTM.Throws on
forceApiand reuses_getPermissionsCore. Consistent with other sync helpers.
6-11: The review comment is incorrect. The typing is correct as written.The
callAccountApiPaginatedfunction returnsPromise<T["data"]>, which uses indexed access to extract thedataproperty fromT. When called withAccountPermissionsResult, it returnsPromise<AccountPermissionsResult["data"]>, which resolves to{ org_code: string; permissions: [...] }.At line 48, accessing
data.org_codeis valid because the variabledatahas the unwrapped type withorg_codeat the top level. The type definition and usage align correctly—there is no mismatch between nested and unwrapped shapes. The code is type-safe as-is.Likely an incorrect or invalid review comment.
lib/utils/token/getFlags.ts (3)
2-2: Import addition is appropriate.Direct import of
getDecodedTokenSyncandJWTDecodedis consistent with the new sync pattern.
33-34: Async path parity via_getFlagsCore.Good consolidation; keeps behavior aligned.
36-45: Sync guard and flow LGTM.Error on
forceApiis consistent; sync path reuses core.lib/utils/token/getFlag.ts (1)
38-48: Sync guard and reuse LGTM.The sync path mirrors async logic and blocks
forceApias expected.lib/main.ts (1)
29-70: All Sync helpers are properly re-exported; imports will not break.The verification confirms that every Sync helper exported in
lib/main.ts(lines 29-70) is correctly re-exported fromlib/utils/token/index.ts. All 11 Sync functions are present and accessible. The public API expansion is consistent and complete.lib/utils/token/getUserOrganizations.test.ts (2)
3-7: LGTM!The import additions and test setup changes are appropriate. The change on line 18 to use
removeItemsinstead of setting to null aligns better with the synchronous test path behavior.Also applies to: 18-18
67-74: LGTM!The test correctly validates the multiple organization codes scenario for the synchronous variant.
lib/sessionManager/types.ts (1)
47-47: LGTM! Good addition for runtime sync/async detection.The
asyncStoreproperty enables callers to determine whether a storage implementation operates synchronously or asynchronously, which is essential for the sync helper variants.Also applies to: 122-122
lib/sessionManager/stores/memory.ts (1)
13-13: LGTM! Clean conversion to synchronous storage.The MemoryStorage class has been consistently converted to synchronous operations with
asyncStore = falsecorrectly indicating the synchronous behavior. All method signatures and implementations are aligned.Also applies to: 20-23, 31-49, 56-74, 81-90
lib/main.test.ts (1)
86-109: LGTM! Complete export coverage.All synchronous helper variants are properly included in the expected exports list, ensuring the public API surface is correctly validated.
lib/sessionManager/stores/localStorage.ts (1)
13-13: LGTM! Clean conversion to synchronous storage.The LocalStorage class has been consistently converted to synchronous operations with
asyncStore = falsecorrectly indicating the synchronous behavior. The change fromPromise.alltoforEachindestroySessionis appropriate for the synchronous flow.Also applies to: 27-33, 41-64, 71-88, 95-112
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
lib/utils/token/getUserProfile.ts (2)
12-38: Remove misleading generic parameter and unsafe cast.The generic
Tparameter suggests that additional properties fromTwould be preserved in the return value, but the implementation only returnsUserProfilefields (lines 31-37). The cast on line 37 is unsafe—it claims the returned object includes properties fromTthat aren't actually present.Since neither caller uses the generic parameter meaningfully, simplify by removing it.
Apply this diff to simplify the core helper:
-const _getUserProfileCore = <T>( +const _getUserProfileCore = ( idToken: - | (T & { + | ({ sub: string; given_name: string; family_name: string; email: string; picture: string; }) | null, -): (UserProfile & T) | null => { +): UserProfile | null => { if (!idToken) { return null; } const { sub } = idToken; if (!sub) { console.error("No sub in idToken"); return null; } return { id: idToken.sub, givenName: idToken.given_name, familyName: idToken.family_name, email: idToken.email, picture: idToken.picture, - } as UserProfile & T; + }; };
1-62: Fix Prettier formatting issues before merging.The pipeline detected Prettier formatting issues. Run
prettier --write lib/utils/token/getUserProfile.tsto fix them.#!/bin/bash # Run Prettier to fix formatting issues prettier --write lib/utils/token/getUserProfile.ts
🧹 Nitpick comments (2)
lib/utils/exchangeAuthCode.ts (1)
190-202: Excellent error handling improvement for token persistence.The try/catch block properly handles storage failures and returns a clear error instead of silently continuing. This prevents the function from returning success when tokens couldn't be persisted.
Optional enhancement for consistency:
Consider calling
clearTempStore()before returning the error to clean up temporary authentication values (state, nonce, codeVerifier). While these values become stale after the authorization code is consumed, cleaning them maintains consistency with the error path at line 173.} catch (error) { console.error("Failed to persist tokens to secure storage:", error); + await clearTempStore(); return { success: false, error: `Failed to persist tokens: ${error}`, }; }lib/utils/token/getCurrentOrganization.test.ts (1)
34-54: Consider consolidating token cleanup in beforeEach for better isolation.The synchronous test coverage looks solid and correctly exercises the sync path. I notice the previous review feedback has been addressed (no console.log, variable correctly named
orgCode).However, the suggestion to consolidate token cleanup into
beforeEachis still relevant. This would improve test isolation and reduce duplication:describe("getCurrentOrganizationSync", () => { beforeEach(() => { setActiveStorage(storage); + storage.removeSessionItem(StorageKeys.idToken); + storage.removeSessionItem(StorageKeys.accessToken); }); it("when no token", () => { - storage.removeSessionItem(StorageKeys.idToken); - storage.removeSessionItem(StorageKeys.accessToken); const orgCode = getCurrentOrganizationSync(); - expect(orgCode).toStrictEqual(null); }); it("with access", () => { storage.setSessionItem( StorageKeys.accessToken, createMockAccessToken({ org_code: "org_123456" }), ); const orgCode = getCurrentOrganizationSync(); expect(orgCode).toStrictEqual(null); }); });The same pattern could optionally be applied to the async describe block at lines 12-32.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
lib/utils/exchangeAuthCode.ts(1 hunks)lib/utils/token/getCurrentOrganization.test.ts(3 hunks)lib/utils/token/getPermission.ts(3 hunks)lib/utils/token/getRawToken.ts(1 hunks)lib/utils/token/getUserOrganizations.test.ts(3 hunks)lib/utils/token/getUserProfile.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/utils/token/getRawToken.ts
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2024-11-10T23:29:36.293Z
Learnt from: DanielRivers
PR: kinde-oss/js-utils#11
File: lib/utils/exchangeAuthCode.ts:51-51
Timestamp: 2024-11-10T23:29:36.293Z
Learning: In `lib/utils/exchangeAuthCode.ts`, the `exchangeAuthCode` function intentionally uses `getInsecureStorage()` to store temporary authentication flow values locally between sessions. This is necessary for storing data needed for the return trip. The `getInsecureStorage()` function returns the secure storage if no active insecure storage is defined.
Applied to files:
lib/utils/exchangeAuthCode.ts
📚 Learning: 2024-11-19T20:31:59.197Z
Learnt from: DanielRivers
PR: kinde-oss/js-utils#11
File: lib/utils/token/index.test.ts:59-62
Timestamp: 2024-11-19T20:31:59.197Z
Learning: In `lib/utils/token/index.test.ts`, the test "getInsecureStorage returns active storage when no insecure storage is set" is intentional. It verifies that when insecure storage is not set, `getInsecureStorage()` returns the active storage as a fallback, preferring secure storage but allowing for insecure storage when needed by consumers of the library.
Applied to files:
lib/utils/token/getCurrentOrganization.test.ts
🧬 Code graph analysis (5)
lib/utils/exchangeAuthCode.ts (2)
lib/main.ts (1)
StorageKeys(84-84)lib/sessionManager/index.ts (1)
StorageKeys(51-51)
lib/utils/token/getCurrentOrganization.test.ts (3)
lib/utils/token/index.ts (2)
setActiveStorage(49-55)getCurrentOrganizationSync(14-14)lib/utils/token/getCurrentOrganization.ts (1)
getCurrentOrganizationSync(17-20)lib/utils/token/testUtils/index.ts (1)
createMockAccessToken(23-44)
lib/utils/token/getUserOrganizations.test.ts (3)
lib/utils/token/index.ts (2)
setActiveStorage(49-55)getUserOrganizationsSync(31-31)lib/utils/token/getUserOrganizations.ts (1)
getUserOrganizationsSync(13-16)lib/utils/token/testUtils/index.ts (1)
createMockAccessToken(23-44)
lib/utils/token/getPermission.ts (2)
lib/utils/token/getDecodedToken.ts (2)
JWTDecoded(45-45)getDecodedTokenSync(76-91)lib/types.ts (1)
GetPermissionOptions(272-272)
lib/utils/token/getUserProfile.ts (1)
lib/utils/token/getClaims.ts (2)
getClaims(9-13)getClaimsSync(15-19)
🪛 GitHub Actions: Build and test
lib/utils/token/getUserProfile.ts
[warning] 1-1: Prettier formatting issues detected in this file. Run 'prettier --write' to fix.
🔇 Additional comments (7)
lib/utils/token/getCurrentOrganization.test.ts (2)
3-7: LGTM! Clean import addition.The import of
getCurrentOrganizationSyncis structured correctly and aligns with the new synchronous test coverage.
17-18: Good fix: properly awaiting async storage operations.Awaiting the
removeSessionItemcalls ensures the tokens are fully removed before the test assertion runs, improving test reliability.lib/utils/token/getPermission.ts (3)
47-48: LGTM: Clean refactor to use shared core helper.The async path correctly delegates to
_getPermissionCoreafter retrieving the token. This keeps the sync and async implementations consistent.
51-61: LGTM: Sync API correctly guards against async operations.The implementation properly prevents
forceApiusage in sync mode with a clear error message, then delegates to the same core helper as the async path. This maintains consistency across both APIs.
12-30: Hasura claims handling verified and type-safe; no changes needed.The critical issue from the previous review has been successfully addressed. The
_getPermissionCorehelper now correctly handles both standard (permissions,org_code) and Hasura-specific (x-hasura-permissions,x-hasura-org-code) claims. TypeScript compilation confirms that the property accesses are type-safe—no type assertions are required. The code is correct as-is.lib/utils/token/getUserProfile.ts (2)
1-2: LGTM!The import of
getClaimsSyncis correctly added to support the new synchronous API.
40-42: Remove unnecessary parentheses from return type.The parentheses around
UserProfileon lines 40-41 are unnecessary. The return type should bePromise<UserProfile | null>.Apply this diff:
-export const getUserProfile = async (): Promise< - (UserProfile) | null -> => { +export const getUserProfile = async (): Promise<UserProfile | null> => {Likely an incorrect or invalid review comment.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
lib/utils/token/getUserProfile.ts (1)
12-38: Remove unnecessary genericTand fix return type.The generic parameter
Tand its inclusion in the return type(UserProfile & T) | nullis misleading. The implementation (lines 31-37) only extracts and returns the fiveUserProfilefields—it doesn't preserve or spread any additional properties fromT. The castas UserProfile & Ton line 37 is incorrect.Since both public functions call this helper without needing the generic and only expect
UserProfile | null, simplify the core helper to be non-generic.Apply this diff:
-const _getUserProfileCore = <T>( +const _getUserProfileCore = ( idToken: - | (T & { + | ({ sub: string; given_name: string; family_name: string; email: string; picture: string; }) | null, -): (UserProfile & T) | null => { +): UserProfile | null => { if (!idToken) { return null; } const { sub } = idToken; if (!sub) { console.error("No sub in idToken"); return null; } return { id: idToken.sub, givenName: idToken.given_name, familyName: idToken.family_name, email: idToken.email, picture: idToken.picture, - } as UserProfile & T; + }; };lib/utils/token/getCurrentOrganizationHasura.test.ts (1)
16-21: RemoveStorageKeys.idTokenand replace withStorageKeys.accessToken(line 17).
getCurrentOrganization()reads the access token by default (viagetDecodedToken()which defaults toStorageKeys.accessToken). The "when no token" test should remove the access token to properly test the no-token scenario, not the ID token. The "with access" test confirms this by settingStorageKeys.accessToken(lines 25-26).
🧹 Nitpick comments (1)
lib/utils/token/getFlag.test.ts (1)
224-266: Expand test coverage forgetFlagSyncto matchgetFlag.The sync test suite has good structure but incomplete coverage compared to the async version. Consider adding these test cases to ensure the sync variant has equivalent coverage:
when name missing(see lines 38-55 for reference)boolean false(see lines 74-89)string(see lines 91-106)integer(see lines 108-123)no existing flag(see lines 125-140)All of these scenarios are independent of async/API behavior and should work identically in sync mode.
Example implementation for the missing test cases:
it("when name missing", () => { storage.setSessionItem( StorageKeys.accessToken, createMockAccessToken({ feature_flags: { test: { v: true, t: "b" }, }, }), ); // @ts-expect-error - we want to test the case where no flag name is provided const flagValue = getFlagSync(); expect(flagValue).toStrictEqual(null); }); it("boolean false", () => { storage.setSessionItem( StorageKeys.accessToken, createMockAccessToken({ feature_flags: { test: { v: false, t: "b" }, }, }), ); const flagValue = getFlagSync<boolean>("test"); expect(flagValue).toStrictEqual(false); }); it("string", () => { storage.setSessionItem( StorageKeys.accessToken, createMockAccessToken({ feature_flags: { test: { v: "hello", t: "s" }, }, }), ); const flagValue = getFlagSync<string>("test"); expect(flagValue).toStrictEqual("hello"); }); it("integer", () => { storage.setSessionItem( StorageKeys.accessToken, createMockAccessToken({ feature_flags: { test: { v: 5, t: "i" }, }, }), ); const flagValue = getFlagSync<number>("test"); expect(flagValue).toStrictEqual(5); }); it("no existing flag", () => { storage.setSessionItem( StorageKeys.accessToken, createMockAccessToken({ feature_flags: { test: { v: 5, t: "i" }, }, }), ); const flagValue = getFlagSync<number>("noexist"); expect(flagValue).toStrictEqual(null); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
lib/utils/token/getCurrentOrganizationHasura.test.ts(3 hunks)lib/utils/token/getFlag.test.ts(3 hunks)lib/utils/token/getPermission.test.ts(3 hunks)lib/utils/token/getPermissions.test.ts(3 hunks)lib/utils/token/getRawToken.ts(1 hunks)lib/utils/token/getUserOrganizations.test.ts(3 hunks)lib/utils/token/getUserProfile.test.ts(3 hunks)lib/utils/token/getUserProfile.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- lib/utils/token/getPermissions.test.ts
- lib/utils/token/getUserOrganizations.test.ts
- lib/utils/token/getUserProfile.test.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-11-19T20:31:59.197Z
Learnt from: DanielRivers
PR: kinde-oss/js-utils#11
File: lib/utils/token/index.test.ts:59-62
Timestamp: 2024-11-19T20:31:59.197Z
Learning: In `lib/utils/token/index.test.ts`, the test "getInsecureStorage returns active storage when no insecure storage is set" is intentional. It verifies that when insecure storage is not set, `getInsecureStorage()` returns the active storage as a fallback, preferring secure storage but allowing for insecure storage when needed by consumers of the library.
Applied to files:
lib/utils/token/getFlag.test.tslib/utils/token/getRawToken.tslib/utils/token/getPermission.test.tslib/utils/token/getCurrentOrganizationHasura.test.ts
🧬 Code graph analysis (5)
lib/utils/token/getFlag.test.ts (3)
lib/utils/token/index.ts (2)
setActiveStorage(49-55)getFlagSync(18-18)lib/utils/token/getFlag.ts (1)
getFlagSync(38-48)lib/utils/token/testUtils/index.ts (1)
createMockAccessToken(23-44)
lib/utils/token/getRawToken.ts (1)
lib/utils/token/index.ts (1)
getActiveStorage(61-63)
lib/utils/token/getPermission.test.ts (4)
lib/sessionManager/stores/memory.ts (1)
MemoryStorage(9-91)lib/utils/token/index.ts (2)
setActiveStorage(49-55)getPermissionSync(25-25)lib/utils/token/getPermission.ts (1)
getPermissionSync(51-61)lib/utils/token/testUtils/index.ts (1)
createMockAccessToken(23-44)
lib/utils/token/getUserProfile.ts (1)
lib/utils/token/getClaims.ts (2)
getClaims(9-13)getClaimsSync(15-19)
lib/utils/token/getCurrentOrganizationHasura.test.ts (3)
lib/utils/token/index.ts (2)
setActiveStorage(49-55)getCurrentOrganizationSync(14-14)lib/utils/token/testUtils/index.ts (1)
createMockAccessToken(23-44)lib/utils/token/getCurrentOrganization.ts (1)
getCurrentOrganizationSync(17-20)
🔇 Additional comments (8)
lib/utils/token/getRawToken.ts (1)
24-46: LGTM! Past review concern has been properly addressed.The implementation correctly throws an error when
activeStorage.asyncStoreis true (lines 33-35), which resolves the inconsistency flagged in the previous review. The function now follows the established error-handling pattern used bygetDecodedTokenSyncand other sync helpers, preventing silent failures when async-only storage is encountered.The overall implementation is solid:
- Appropriate null handling for missing storage and tokens
- Correct synchronous token retrieval (no await)
- Consistent error messaging across sync helpers
lib/utils/token/getUserProfile.ts (2)
40-49: LGTM!The async implementation correctly awaits
getClaimsand forwards to the core helper. The non-generic signature is appropriate.
51-60: LGTM!The synchronous variant correctly mirrors the async implementation using
getClaimsSync. The pattern is consistent with other sync helpers added in this PR.lib/utils/token/getFlag.test.ts (1)
3-3: LGTM: Import updated for sync variant.The addition of
getFlagSyncto the imports is correct and necessary for testing the new synchronous API.lib/utils/token/getPermission.test.ts (3)
28-39: LGTM!The test correctly verifies that
getPermissionSyncreads permission status and organization code from the token.
41-45: LGTM!Correctly verifies that
getPermissionSyncthrows an error whenforceApiis used, since synchronous mode cannot make API calls.
8-10: The mock is necessary but tests don't exercise the API code path.The
callAccountApimock is correctly defined and necessary—the implementation imports and uses it whenforceApioption is true ingetPermission(). However, the current tests never setforceApi, so they don't exercise the code path that calls the API. The mock prevents real API calls (which is good), but there's no test coverage for theforceApibehavior. This may be intentional, but consider adding a test case forgetPermission()withforceApi: trueto verify that code path.lib/utils/token/getCurrentOrganizationHasura.test.ts (1)
3-7: LGTM!The import of
getCurrentOrganizationSyncaligns with the PR's objective to add synchronous helper support.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
lib/utils/token/getRawToken.test.ts (1)
60-82: Good test coverage for the sync API.The test suite covers the essential scenarios for
getRawTokenSync: no storage, successful retrieval, and async storage error handling. The tests follow existing patterns well.Consider adding these optional test cases for completeness:
- Sync storage with no token set (similar to the async test at lines 17-20)
- idToken retrieval with sync API (currently only accessToken is tested)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
lib/utils/exchangeAuthCode.test.ts(1 hunks)lib/utils/token/getDecodedToken.test.ts(2 hunks)lib/utils/token/getRawToken.test.ts(2 hunks)lib/utils/token/getRoles.test.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/utils/token/getDecodedToken.test.ts
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2024-11-19T20:31:59.197Z
Learnt from: DanielRivers
PR: kinde-oss/js-utils#11
File: lib/utils/token/index.test.ts:59-62
Timestamp: 2024-11-19T20:31:59.197Z
Learning: In `lib/utils/token/index.test.ts`, the test "getInsecureStorage returns active storage when no insecure storage is set" is intentional. It verifies that when insecure storage is not set, `getInsecureStorage()` returns the active storage as a fallback, preferring secure storage but allowing for insecure storage when needed by consumers of the library.
Applied to files:
lib/utils/token/getRawToken.test.tslib/utils/exchangeAuthCode.test.ts
📚 Learning: 2024-11-10T23:29:36.293Z
Learnt from: DanielRivers
PR: kinde-oss/js-utils#11
File: lib/utils/exchangeAuthCode.ts:51-51
Timestamp: 2024-11-10T23:29:36.293Z
Learning: In `lib/utils/exchangeAuthCode.ts`, the `exchangeAuthCode` function intentionally uses `getInsecureStorage()` to store temporary authentication flow values locally between sessions. This is necessary for storing data needed for the return trip. The `getInsecureStorage()` function returns the secure storage if no active insecure storage is defined.
Applied to files:
lib/utils/exchangeAuthCode.test.ts
🧬 Code graph analysis (3)
lib/utils/token/getRawToken.test.ts (4)
lib/utils/token/index.ts (3)
clearActiveStorage(76-78)getRawTokenSync(16-16)setActiveStorage(49-55)lib/utils/token/getRawToken.ts (1)
getRawTokenSync(24-46)lib/sessionManager/stores/memory.ts (1)
MemoryStorage(9-91)lib/utils/token/testUtils/index.ts (1)
createMockAccessToken(23-44)
lib/utils/exchangeAuthCode.test.ts (1)
lib/utils/exchangeAuthCode.ts (1)
exchangeAuthCode(67-241)
lib/utils/token/getRoles.test.ts (3)
lib/utils/token/index.ts (2)
setActiveStorage(49-55)getRolesSync(33-33)lib/utils/token/getRoles.ts (1)
getRolesSync(57-64)lib/utils/token/testUtils/index.ts (1)
createMockAccessToken(23-44)
🔇 Additional comments (5)
lib/utils/token/getRoles.test.ts (2)
3-3: LGTM: Import correctly updated for sync API.The addition of
getRolesSyncto the imports aligns with the PR's objective of adding synchronous counterparts for token utilities.
364-391: Verify behavioral inconsistency between async and sync versions.The test coverage for
getRolesSyncis well-structured and covers key scenarios. However, there's a notable behavioral difference:
- Async version (Line 24):
getRoles()throws"Authentication token not found."when no token exists.- Sync version (Line 371):
getRolesSync()returns[]when no token exists.Additionally, the sync test suite lacks an
afterEachcleanup block (present in the async suite at lines 17-20), which could lead to test pollution if storage state persists across tests.Consider adding cleanup to prevent state leakage:
describe("getRolesSync", () => { beforeEach(() => { setActiveStorage(storage); }); + + afterEach(() => { + vi.restoreAllMocks(); + });Please verify that the behavioral difference (throw vs. return
[]) is intentional and aligns with the expected user experience for sync vs. async APIs.lib/utils/token/getRawToken.test.ts (2)
2-4: LGTM - Import additions support the new sync API tests.The addition of
getRawTokenSyncandclearActiveStorageimports are necessary and correctly placed for the new test suite.
74-81: Error message verification complete — no issues found.The error message in the test matches the implementation exactly. The test correctly verifies that using async storage with the sync API throws the expected error.
lib/utils/exchangeAuthCode.test.ts (1)
580-620: Confirm whether temporary storage cleanup on persistence failure is intentional.The verification reveals that when
secureStore.setItems()fails at line 196, the catch block returns immediately without callingclearTempStore(). This means state and codeVerifier remain in storage after a persistence failure.The test correctly validates the error path, but does not verify the state of temporary storage post-failure. Clarify:
- Is leaving temporary storage intact intentional (to support retry scenarios)?
- Or should
clearTempStore()be called in the catch block before returning?If temporary storage should be cleared, the implementation needs a fix and the test should verify that cleanup occurs.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
lib/utils/token/getEntitlements.test.ts (1)
33-47: Consider fixing the naming convention.The variable names use "Plansand" which should be "PlansAnd" (capital A) to follow camelCase convention properly.
Apply this diff to improve naming consistency:
-const mockEntitlementsAPINoPlansandEntitlementsResponse = { +const mockEntitlementsAPINoPlansAndEntitlementsResponse = { data: { org_code: "org_0195ac80a14e", }, metadata: { has_more: false, next_page_starting_after: "entitlement_0195ac80a14e8d71f42b98e75d3c61ad", }, }; -const expectedResponseNoPlansandEntitlements = { +const expectedResponseNoPlansAndEntitlements = { orgCode: "org_0195ac80a14e", plans: [], entitlements: [], };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
lib/utils/token/getDecodedToken.test.ts(2 hunks)lib/utils/token/getEntitlements.test.ts(2 hunks)lib/utils/token/getPermission.test.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- lib/utils/token/getDecodedToken.test.ts
- lib/utils/token/getPermission.test.ts
🧰 Additional context used
🧬 Code graph analysis (1)
lib/utils/token/getEntitlements.test.ts (1)
lib/utils/token/getEntitlements.ts (1)
getEntitlements(18-41)
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/utils/token/getPermissions.test.ts (1)
24-32: Fix token removal: change StorageKeys.idToken to StorageKeys.accessToken on line 25.The
getPermissions()function defaults to reading fromStorageKeys.accessToken(viagetDecodedToken), notidToken. The test at line 25 removes the wrong token. The sync test at line 170 correctly removesaccessToken.- await storage.removeSessionItem(StorageKeys.idToken); + await storage.removeSessionItem(StorageKeys.accessToken);
🧹 Nitpick comments (5)
lib/utils/token/getRoles.test.ts (1)
364-391: Consider expanding test coverage for edge cases.The sync test suite has minimal coverage (3 tests) compared to the async version (~20+ tests). Consider adding tests for:
- When
getDecodedTokenSync()returnsnull(similar to async test at line 345)- When token has
x-hasura-rolesinstead ofroles(similar to async tests at lines 283, 310)- When token has
roles: undefinedor other edge casesThis would ensure parity in robustness between sync and async variants.
lib/utils/token/getClaim.test.ts (1)
32-51: LGTM! Synchronous test suite correctly mirrors async tests.The new
getClaimSynctest suite properly tests the synchronous API by:
- Correctly omitting
awaitkeywords for synchronous operations- Mirroring the async test cases for consistency
- Covering both no-token and claim-retrieval scenarios
The test structure is sound and should validate the synchronous helper functionality effectively.
Optional: Consider reducing duplication.
The sync and async test suites have significant code duplication. While this is typical for testing both variants, you could optionally refactor to use a shared test factory or helper function if this pattern repeats across other test files. However, the current approach is clear and maintainable as-is.
lib/utils/token/getFlags.test.ts (1)
341-372: Expand test coverage for sync variant.The sync test suite currently covers only 3 scenarios (no token, single boolean flag, forceApi error), while the async suite covers 11+ scenarios. Although
getFlagsSyncshares core logic via_getFlagsCore, the sync code path should be independently verified.Consider adding sync tests for:
- No flags in token (null
feature_flags)- Empty flags object (
feature_flags: {})- Different flag types: string, integer, object
- Multiple flags with mixed types
- Hasura-style flags (
x-hasura-feature-flags)- Preference of
feature_flagsoverx-hasura-feature-flagsExample test to add:
+ it("when no flags in token", () => { + storage.setSessionItem( + StorageKeys.accessToken, + createMockAccessToken({ + feature_flags: null, + }), + ); + const flags = getFlagsSync(); + expect(flags).toStrictEqual(null); + }); + + it("single string flag", () => { + storage.setSessionItem( + StorageKeys.accessToken, + createMockAccessToken({ + feature_flags: { + theme: { v: "dark", t: "string" }, + }, + }), + ); + const flags = getFlagsSync(); + expect(flags).toStrictEqual([ + { key: "theme", value: "dark", type: "string" }, + ]); + });lib/utils/token/getPermission.test.ts (1)
59-59: Optional cleanup: Remove redundantidTokenclearing.Lines 59 and 72 remove
idToken, but permission functions only readaccessToken(as correctly removed on lines 60 and 73). TheidTokenremoval is harmless but unnecessary.Apply this diff if you'd like to simplify:
it("when no token", async () => { - storage.removeSessionItem(StorageKeys.idToken); storage.removeSessionItem(StorageKeys.accessToken);it("when no token with enum", async () => { - storage.removeSessionItem(StorageKeys.idToken); storage.removeSessionItem(StorageKeys.accessToken);Also applies to: 72-72
lib/utils/token/getPermissions.test.ts (1)
164-191: Consider adding typed permissions test for completeness.While the current sync test coverage validates essential scenarios, consider adding a test for typed permissions (similar to lines 47-58 in the async suite) to ensure type parameter handling works correctly in sync mode:
it("with typed permissions", () => { storage.setSessionItem( StorageKeys.accessToken, createMockAccessToken({ permissions: ["canEdit"] }), ); const result = getPermissionsSync<PermissionEnum>(); expect(result.permissions).toStrictEqual([PermissionEnum.canEdit]); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
lib/utils/token/getClaim.test.ts(3 hunks)lib/utils/token/getEntitlements.test.ts(2 hunks)lib/utils/token/getFlagHasura.test.ts(1 hunks)lib/utils/token/getFlags.test.ts(3 hunks)lib/utils/token/getPermission.test.ts(4 hunks)lib/utils/token/getPermissions.test.ts(3 hunks)lib/utils/token/getPermissionsHasura.test.ts(1 hunks)lib/utils/token/getRoles.test.ts(2 hunks)lib/utils/token/has/has.test.ts(1 hunks)lib/utils/token/has/hasRoles.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/utils/token/getEntitlements.test.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-11-19T20:31:59.197Z
Learnt from: DanielRivers
PR: kinde-oss/js-utils#11
File: lib/utils/token/index.test.ts:59-62
Timestamp: 2024-11-19T20:31:59.197Z
Learning: In `lib/utils/token/index.test.ts`, the test "getInsecureStorage returns active storage when no insecure storage is set" is intentional. It verifies that when insecure storage is not set, `getInsecureStorage()` returns the active storage as a fallback, preferring secure storage but allowing for insecure storage when needed by consumers of the library.
Applied to files:
lib/utils/token/getClaim.test.tslib/utils/token/has/has.test.tslib/utils/token/has/hasRoles.test.tslib/utils/token/getFlagHasura.test.tslib/utils/token/getPermissionsHasura.test.tslib/utils/token/getPermission.test.ts
🧬 Code graph analysis (6)
lib/utils/token/getClaim.test.ts (2)
lib/utils/token/index.ts (2)
setActiveStorage(49-55)getClaimSync(11-11)lib/utils/token/testUtils/index.ts (1)
createMockAccessToken(23-44)
lib/utils/token/has/has.test.ts (1)
lib/main.ts (1)
StorageKeys(84-84)
lib/utils/token/getPermissions.test.ts (3)
lib/utils/token/index.ts (2)
setActiveStorage(49-55)getPermissionsSync(28-28)lib/utils/token/getPermissions.ts (1)
getPermissionsSync(58-67)lib/utils/token/testUtils/index.ts (1)
createMockAccessToken(23-44)
lib/utils/token/getFlags.test.ts (3)
lib/utils/token/index.ts (2)
setActiveStorage(49-55)getFlagsSync(20-20)lib/utils/token/getFlags.ts (1)
getFlagsSync(36-45)lib/utils/token/testUtils/index.ts (1)
createMockAccessToken(23-44)
lib/utils/token/getPermission.test.ts (5)
lib/sessionManager/stores/memory.ts (1)
MemoryStorage(9-91)lib/utils/token/index.ts (3)
setActiveStorage(49-55)getPermissionSync(25-25)getPermission(24-24)lib/utils/token/getPermission.ts (2)
getPermissionSync(51-61)getPermission(37-49)lib/utils/token/testUtils/index.ts (1)
createMockAccessToken(23-44)lib/utils/token/accountApi/callAccountApi.ts (1)
callAccountApi(8-43)
lib/utils/token/getRoles.test.ts (3)
lib/utils/token/index.ts (2)
setActiveStorage(49-55)getRolesSync(33-33)lib/utils/token/getRoles.ts (1)
getRolesSync(57-64)lib/utils/token/testUtils/index.ts (1)
createMockAccessToken(23-44)
🔇 Additional comments (17)
lib/utils/token/has/has.test.ts (1)
22-32: LGTM! Improved token-absence semantics.Using
removeSessionIteminstead of setting the token to null is semantically clearer and aligns with token-absence patterns used throughout the codebase.lib/utils/token/has/hasRoles.test.ts (1)
23-23: LGTM! More explicit test setup for token absence.Using
removeSessionIteminstead of setting the token tonullis semantically clearer and better represents the "no token" scenario. This aligns with the sync helper patterns introduced in this PR.lib/utils/token/getRoles.test.ts (2)
3-3: LGTM!The import correctly adds
getRolesSyncalongside the existinggetRolesimport.
369-372: Verify behavioral difference: async throws, sync returns empty array.The async version
getRoles()throws "Authentication token not found" when no token is present (line 24), but the sync versiongetRolesSync()returns an empty array. Ensure this behavioral divergence is intentional and aligns with the design goals for synchronous APIs.lib/utils/token/getClaim.test.ts (2)
2-2: LGTM! Import addition is correct.The addition of
getClaimSyncto the imports aligns with the PR objective to add synchronous helper functions.
14-14: Good semantic improvement.Using
removeSessionItemis more explicit and semantically correct than setting the token tonull. This improves code clarity.lib/utils/token/getFlags.test.ts (2)
3-3: LGTM: Import addition is correct.The addition of
getFlagsSyncto the imports is necessary and properly structured for the new synchronous test suite.
21-21: LGTM: Clearer token removal semantics.Using
removeSessionItemexplicitly removes the token rather than setting it to null, which better reflects the intent of testing the "no token" scenario.lib/utils/token/getFlagHasura.test.ts (2)
14-14: LGTM: Semantically clearer token removal.Using
removeSessionItemis more explicit and appropriate than setting the token tonull.
1-104: Now let me search for more specific information about the test file structure and whether sync tests for Hasura scenarios should exist:Let me run a more targeted script to see the full structure of both test files:Add sync test coverage for Hasura feature flag scenarios in getFlagHasura.test.ts.
Sync tests for
getFlagSyncexist inlib/utils/token/getFlag.test.ts, but the Hasura-specific test file (getFlagHasura.test.ts) lacks corresponding sync test coverage. Since this file tests Hasura-specific flag scenarios (e.g., x-hasura-feature-flags), add adescribe("getFlagSync - Hasura", ...)block that mirrors the async test scenarios to ensure the synchronous path is validated for all Hasura flag types and edge cases.lib/utils/token/getPermission.test.ts (2)
14-48: LGTM! Comprehensive coverage of sync permission API.The test suite properly validates:
- Fallback behavior when no token exists
- Permission reading from stored access tokens
- Error handling for the unsupported
forceApioption in sync mode
133-158: LGTM! Proper validation of API-based permission fetching.The test correctly verifies:
- The
forceApipath invokescallAccountApi- Permission keys with special characters are properly URL-encoded (space →
%20, slash →%2F)- The API response is returned as expected
lib/utils/token/getPermissions.test.ts (5)
1-3: LGTM: Import changes align with sync helper additions.The addition of
getPermissionsSyncto the imports is consistent with the PR's objective to add synchronous counterparts for token utilities.
164-167: LGTM: Appropriate setup for sync tests.The
beforeEachsetup correctly initializes storage without mock fetch configuration, which is appropriate since sync operations cannot make API calls.
169-175: LGTM: Correctly tests empty state for sync mode.The test appropriately uses synchronous
removeSessionItem(withoutawait) and validates the expected empty state structure. This correctly removesaccessToken, which is the token containing permissions.
177-184: LGTM: Validates synchronous permission retrieval.The test correctly uses synchronous storage operations and validates that permissions are properly extracted from the access token in sync mode.
186-190: LGTM: Critical constraint validation for sync mode.This test is essential for ensuring that API-based operations (
forceApi) are properly rejected in synchronous mode, preventing runtime issues from attempting async operations in a sync context.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/utils/token/getCurrentOrganizationHasura.test.ts (1)
16-21: Fix token key mismatch in "when no token" test.The async test removes
StorageKeys.idToken(line 17), but both "with access" tests setStorageKeys.accessToken(lines 25, 50), and the sync "when no token" test removesStorageKeys.accessToken(line 43). SincegetCurrentOrganizationextracts the org code from the access token's Hasura claim, this test should remove the access token for consistency.Apply this diff:
it("when no token", async () => { - await storage.removeSessionItem(StorageKeys.idToken); + await storage.removeSessionItem(StorageKeys.accessToken); const idToken = await getCurrentOrganization();
🧹 Nitpick comments (1)
lib/utils/token/getRawToken.test.ts (1)
60-88: LGTM: Comprehensive test coverage for sync helper.The test suite properly covers the key scenarios:
- Absence of active storage
- Token not set (modeling absence by not setting, not by setting null—aligns with the PR's approach)
- Successful token retrieval
- Error handling for async-only storage
Optional: Consider adding explicit test for default parameter.
The default
tokenTypeparameter (which defaults to"accessToken") is implicitly tested in the error case at line 84, but a happy-path test callinggetRawTokenSync()without arguments would make the coverage more explicit:it("returns accessToken by default when no tokenType specified", () => { const storage = new MemoryStorage(); setActiveStorage(storage); const mockedToken = createMockAccessToken({ unique: "default" }); storage.setSessionItem(StorageKeys.accessToken, mockedToken); expect(getRawTokenSync()).toBe(mockedToken); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
lib/utils/token/getCurrentOrganizationHasura.test.ts(3 hunks)lib/utils/token/getPermission.test.ts(4 hunks)lib/utils/token/getRawToken.test.ts(2 hunks)lib/utils/token/getRoles.test.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- lib/utils/token/getRoles.test.ts
- lib/utils/token/getPermission.test.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-11-19T20:31:59.197Z
Learnt from: DanielRivers
PR: kinde-oss/js-utils#11
File: lib/utils/token/index.test.ts:59-62
Timestamp: 2024-11-19T20:31:59.197Z
Learning: In `lib/utils/token/index.test.ts`, the test "getInsecureStorage returns active storage when no insecure storage is set" is intentional. It verifies that when insecure storage is not set, `getInsecureStorage()` returns the active storage as a fallback, preferring secure storage but allowing for insecure storage when needed by consumers of the library.
Applied to files:
lib/utils/token/getRawToken.test.tslib/utils/token/getCurrentOrganizationHasura.test.ts
🧬 Code graph analysis (2)
lib/utils/token/getRawToken.test.ts (4)
lib/utils/token/index.ts (3)
clearActiveStorage(76-78)getRawTokenSync(16-16)setActiveStorage(49-55)lib/utils/token/getRawToken.ts (1)
getRawTokenSync(24-46)lib/sessionManager/stores/memory.ts (1)
MemoryStorage(9-91)lib/utils/token/testUtils/index.ts (1)
createMockAccessToken(23-44)
lib/utils/token/getCurrentOrganizationHasura.test.ts (3)
lib/utils/token/index.ts (2)
setActiveStorage(49-55)getCurrentOrganizationSync(14-14)lib/utils/token/getCurrentOrganization.ts (1)
getCurrentOrganizationSync(17-20)lib/utils/token/testUtils/index.ts (1)
createMockAccessToken(23-44)
🔇 Additional comments (3)
lib/utils/token/getCurrentOrganizationHasura.test.ts (2)
3-7: LGTM: Imports correctly updated for sync test support.The addition of
getCurrentOrganizationSyncandsetActiveStorageto the imports is appropriate for the new synchronous test cases.
37-59: Well-structured sync test coverage.The synchronous test block includes both "when no token" and "with access" scenarios, properly mirroring the async test structure and addressing the previous review feedback. The tests correctly use synchronous operations without
awaitand validate both null return for missing tokens and correct Hasura org code extraction.lib/utils/token/getRawToken.test.ts (1)
2-2: LGTM: Import updates are correct.The new imports
getRawTokenSyncandclearActiveStorageare properly used in the test suite below and necessary for testing the synchronous API surface.Also applies to: 4-4
DanielRivers
force-pushed
the
feat/sync-helpers
branch
from
a0d361c to
48b25f6
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/utils/token/getCurrentOrganizationHasura.test.ts (1)
16-21: Incorrect storage key removed in async "when no token" test.Line 17 removes
StorageKeys.idToken, butgetCurrentOrganization()reads the Hasura org code from the access token'sx-hasura-org-codeclaim. This is inconsistent with:
- Line 24, which sets
StorageKeys.accessTokenin the "with access" test- Line 43, which correctly removes
StorageKeys.accessTokenin the sync variant's "when no token" testApply this diff to fix the inconsistency:
it("when no token", async () => { - await storage.removeSessionItem(StorageKeys.idToken); + await storage.removeSessionItem(StorageKeys.accessToken); const idToken = await getCurrentOrganization(); expect(idToken).toStrictEqual(null); });
🧹 Nitpick comments (4)
lib/utils/token/getRoles.test.ts (2)
385-390: Remove unnecessary token removal.Line 386 removes the token before testing that
forceApithrows in sync mode. SincegetRolesSyncshould throw immediately whenforceApi: trueis passed (before checking for a token), this removal is unnecessary.Apply this diff:
it("can't forceApi in sync request", () => { - storage.removeSessionItem(StorageKeys.accessToken); expect(() => getRolesSync({ forceApi: true })).toThrow( "forceApi cannot be used in sync mode", ); });
364-391: Consider adding afterEach for pattern consistency.The async test suite includes an
afterEachblock (lines 17-20) for cleanup, but the sync test suite omits it. While the sync tests don't use mocks that require cleanup, adding a minimalafterEachwould maintain pattern consistency across the test file.Example:
afterEach(() => { vi.restoreAllMocks(); });lib/utils/token/getPermission.test.ts (2)
50-52: Addstorage.destroySession()for consistent test cleanup.The sync tests (line 11) call
storage.destroySession()inbeforeEach, but the async tests do not. While each test explicitly sets or removes tokens, omitting cleanup could lead to state bleeding between tests.Apply this diff to add cleanup:
describe("getPermission", () => { beforeEach(() => { + storage.destroySession(); setActiveStorage(storage); });
54-55: Consider removing only the relevant token.These tests remove both
idTokenandaccessToken, butgetPermissiononly reads fromaccessToken. RemovingidTokenis unnecessary (though harmless).Apply this diff to remove only the relevant token:
it("when no token", async () => { - storage.removeSessionItem(StorageKeys.idToken); storage.removeSessionItem(StorageKeys.accessToken);it("when no token with enum", async () => { - storage.removeSessionItem(StorageKeys.idToken); storage.removeSessionItem(StorageKeys.accessToken);Also applies to: 67-68
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
lib/utils/token/getCurrentOrganizationHasura.test.ts(3 hunks)lib/utils/token/getPermission.test.ts(4 hunks)lib/utils/token/getRawToken.test.ts(2 hunks)lib/utils/token/getRoles.test.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/utils/token/getRawToken.test.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-11-19T20:31:59.197Z
Learnt from: DanielRivers
PR: kinde-oss/js-utils#11
File: lib/utils/token/index.test.ts:59-62
Timestamp: 2024-11-19T20:31:59.197Z
Learning: In `lib/utils/token/index.test.ts`, the test "getInsecureStorage returns active storage when no insecure storage is set" is intentional. It verifies that when insecure storage is not set, `getInsecureStorage()` returns the active storage as a fallback, preferring secure storage but allowing for insecure storage when needed by consumers of the library.
Applied to files:
lib/utils/token/getPermission.test.tslib/utils/token/getCurrentOrganizationHasura.test.ts
🧬 Code graph analysis (3)
lib/utils/token/getPermission.test.ts (5)
lib/sessionManager/stores/memory.ts (1)
MemoryStorage(9-91)lib/utils/token/index.ts (3)
setActiveStorage(49-55)getPermissionSync(25-25)getPermission(24-24)lib/utils/token/getPermission.ts (2)
getPermissionSync(51-61)getPermission(37-49)lib/utils/token/testUtils/index.ts (1)
createMockAccessToken(23-44)lib/utils/token/accountApi/callAccountApi.ts (1)
callAccountApi(8-43)
lib/utils/token/getCurrentOrganizationHasura.test.ts (3)
lib/utils/token/index.ts (2)
setActiveStorage(49-55)getCurrentOrganizationSync(14-14)lib/utils/token/getCurrentOrganization.ts (1)
getCurrentOrganizationSync(17-20)lib/utils/token/testUtils/index.ts (1)
createMockAccessToken(23-44)
lib/utils/token/getRoles.test.ts (3)
lib/utils/token/index.ts (2)
setActiveStorage(49-55)getRolesSync(33-33)lib/utils/token/getRoles.ts (1)
getRolesSync(57-64)lib/utils/token/testUtils/index.ts (1)
createMockAccessToken(23-44)
🔇 Additional comments (6)
lib/utils/token/getCurrentOrganizationHasura.test.ts (1)
37-59: LGTM! Sync test coverage correctly implemented.The synchronous test block properly mirrors the async tests with both "when no token" and "with access" scenarios. The sync operations correctly avoid
await, and the test structure aligns with patterns established in other token utility tests.lib/utils/token/getRoles.test.ts (1)
3-3: Import addition looks good.The addition of
getRolesSyncto the imports is appropriate for the new synchronous test suite.lib/utils/token/getPermission.test.ts (4)
1-5: LGTM!The imports are properly updated to support the new synchronous tests and API mocking functionality.
9-43: LGTM!The synchronous test suite is well-structured and comprehensive. It properly covers the no-token case, successful token read, and the forceApi error path. The use of
removeSessionItemanddestroySessionensures clean test state.
124-126: LGTM!The mock setup for
callAccountApiis properly configured and will be hoisted by Vitest. This enables the forceApi tests to verify API interaction without actual network calls.
128-153: Excellent test coverage for API encoding!This test properly verifies that permission keys with special characters (spaces, slashes) are URL-encoded before being passed to the account API. The test choice of
"view reports/advanced"is particularly good as it exercises multiple encoding scenarios.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
lib/utils/token/getRoles.test.ts(2 hunks)lib/utils/token/getRoles.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/utils/token/getRoles.test.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-02-24T16:16:36.437Z
Learnt from: DanielRivers
PR: kinde-oss/js-utils#86
File: lib/utils/token/getRoles.ts:8-23
Timestamp: 2025-02-24T16:16:36.437Z
Learning: The getDecodedToken function's return type (JWTDecoded) already includes type definitions for both traditional roles (roles) and Hasura roles (x-hasura-roles) through its union of JWTExtra and JWTExtraHasura interfaces.
Applied to files:
lib/utils/token/getRoles.ts
🧬 Code graph analysis (1)
lib/utils/token/getRoles.ts (2)
lib/types.ts (1)
GetRolesOptions(274-274)lib/utils/token/getDecodedToken.ts (1)
getDecodedTokenSync(76-91)
🔇 Additional comments (3)
lib/utils/token/getRoles.ts (3)
2-2: LGTM!The import is necessary for the synchronous implementation and is correctly placed.
15-31: Well-structured core extraction logic.The
TokenWithRolestype and_getRolesCorefunction provide a clean abstraction for extracting roles from either the standardrolesfield or Hasura'sx-hasura-rolesfield. The null handling and warning message are appropriate.The cast on line 30 is safe because lines 24-29 guarantee at least one roles field is truthy.
53-54: LGTM!Good refactoring to delegate role extraction to the shared
_getRolesCorefunction, enabling code reuse between async and sync implementations.
3 participants
Explain your changes
Adds support to call the token helpers synchronously then the store is not promise based
Checklist
🛟 If you need help, consider asking for advice over in the Kinde community.