Open
Conversation
📝 WalkthroughWalkthroughIntroduce a typed Changes
Sequence Diagram(s)sequenceDiagram
participant Comp as Component / useOidc
participant OidcCore as OIDC Core
participant GetUser as createGetUser
participant TokenMgr as Token Manager
participant CreateUser as createUser
Comp->>OidcCore: request user / call getUser()
OidcCore->>GetUser: getUser()
GetUser->>TokenMgr: getCurrentTokens()
TokenMgr-->>GetUser: accessToken + decodedIdToken
GetUser->>GetUser: compute stable hash from tokens/id token
alt first fetch or hash changed
GetUser->>CreateUser: createUser({ accessToken, decodedIdToken, fetchUserInfo, issuerUri, user_current })
CreateUser-->>GetUser: User
GetUser->>OidcCore: cache User, notify subscribers
GetUser-->>Comp: return { user, refreshUser, subscribeToUserChange }
else hash unchanged
GetUser-->>Comp: return cached { user, refreshUser, subscribeToUserChange }
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Closed
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
examples/tanstack-router-file-router/src/oidc.ts (1)
50-55: Note: This example assumes Keycloak as the OIDC provider.The
createKeycloakUtilsandfetchUserProfilecalls are Keycloak-specific. This is appropriate for this example directory, but users with non-Keycloak providers should adapt accordingly. Consider adding a brief comment noting this is Keycloak-specific.💡 Suggested clarifying comment
+ // NOTE: The following is Keycloak-specific. For other providers, + // adjust or remove the userProfile fetching. const keycloakUtils = createKeycloakUtils({ issuerUri }); const [userInfo, userProfile] = await Promise.all([🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/tanstack-router-file-router/src/oidc.ts` around lines 50 - 55, Add a brief clarifying comment above the Keycloak-specific calls to indicate this example assumes Keycloak as the OIDC provider; reference the createKeycloakUtils({ issuerUri }) call and keycloakUtils.fetchUserProfile({ accessToken }) along with fetchUserInfo() so readers know to replace or adapt createKeycloakUtils and fetchUserProfile when using a non-Keycloak provider.src/react-spa/createOidcSpaUtils.ts (3)
318-340: Consider adding a debug message tocreateObjectWithSomePropertiesThatThrowIfAccessed.The function supports an optional
debugMessageparameter (second argument), but it's not being used here. Adding a helpful message would improve developer experience when accessinguserorrefreshUserbefore they're ready.♻️ Suggested improvement
return createObjectWithSomePropertiesThatThrowIfAccessed< UseOidc.Oidc.LoggedIn<DecodedIdToken, User> - >({ + >( + { isUserLoggedIn: true, // ... other properties ... user: user === undefined ? THROW_IF_ACCESSED : user, refreshUser: resultOfGetUser === undefined ? THROW_IF_ACCESSED : resultOfGetUser.refreshUser - }); + }, + createUser === undefined + ? "oidc-spa: To use user/refreshUser, you must provide createUser when calling createOidcSpaUtils" + : "oidc-spa: User is still being initialized" + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/react-spa/createOidcSpaUtils.ts` around lines 318 - 340, Pass a helpful debugMessage into createObjectWithSomePropertiesThatThrowIfAccessed so attempts to access lazy/throwing props provide context; update the call in createOidcSpaUtils (the createObjectWithSomePropertiesThatThrowIfAccessed invocation that returns UseOidc.Oidc.LoggedIn) to supply a second argument string that mentions the object and the properties (e.g., indicating that user or refreshUser are not yet available) so thrown errors from the getter placeholders (THROW_IF_ACCESSED) include that message.
204-216: MissingresultOfGetUserin useEffect dependency array.The
useEffectcapturesresultOfGetUserin its closure but uses an empty dependency array. While this works becauseresultOfGetUseris resolved before the component mounts (due to the suspense throw earlier), it would be cleaner to include it for clarity and to satisfy exhaustive-deps lint rules.♻️ Suggested improvement
useEffect(() => { if (resultOfGetUser === undefined) { return; } const { unsubscribeFromUserChange } = resultOfGetUser.subscribeToUserChange(({ user }) => setUser(user) ); return () => { unsubscribeFromUserChange(); }; - }, []); + }, [resultOfGetUser]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/react-spa/createOidcSpaUtils.ts` around lines 204 - 216, The useEffect that subscribes via resultOfGetUser.subscribeToUserChange(...) closes over resultOfGetUser but has an empty dependency array; update the hook in createOidcSpaUtils so that resultOfGetUser is included in the dependency array (e.g., useEffect(..., [resultOfGetUser])), keeping the existing early return when resultOfGetUser === undefined, and ensure the cleanup calls the unsubscribeFromUserChange returned from subscribeToUserChange (as currently implemented) so it uses the correct subscription for the current resultOfGetUser.
686-692: Type safety concern: Error passed may not be OidcInitializationError.At line 686, the check is
instanceof Error, but theErrorComponentprop expectsoidcInitializationError: OidcInitializationError. While the deferred is typed to containOidcInitializationError, the runtime check is broader. Consider usinginstanceof OidcInitializationErrorfor consistency.♻️ Suggested improvement for consistency
- if (resultOfGetUserOrError instanceof Error) { + if (resultOfGetUserOrError instanceof OidcInitializationError) { const error = resultOfGetUserOrError; return createElement(ErrorComponent, { oidcInitializationError: error }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/react-spa/createOidcSpaUtils.ts` around lines 686 - 692, The runtime check currently uses `instanceof Error` for `resultOfGetUserOrError` but `ErrorComponent` expects an `OidcInitializationError`, so update the check around `resultOfGetUserOrError` to use `instanceof OidcInitializationError` (or a custom type guard) before passing it into `createElement(ErrorComponent, { oidcInitializationError: ... })`; if the value can be a different Error at runtime, explicitly convert or construct an `OidcInitializationError` from the generic Error (or return a safe fallback) so `ErrorComponent` always receives the correct `OidcInitializationError` type.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/core/createGetUser.ts`:
- Around line 46-52: The fetchUserInfo function currently returns r.json()
without handling HTTP errors or JSON parse failures; update fetchUserInfo to
first check the Response.ok from the fetch(userinfo_endpoint, { headers: {
Authorization: `Bearer ${accessToken}` } }) call and throw a clear error
containing status and statusText (and optionally response text) for non-2xx
responses, then parse the body inside a try/catch so JSON.parse/response.json
failures are caught and rethrown with context (including the userinfo_endpoint
and status), ensuring the function returns the parsed object only on success and
surfaces clear errors on network/HTTP/parse problems.
In `@src/react-spa/types.ts`:
- Around line 142-150: Update the getUser return type so its refreshUser matches
the implementation in src/core/Oidc.ts: change the refreshUser signature from ()
=> void to () => Promise<User> within the getUser return object (referencing
getUser, User, refreshUser, and subscribeToUserChange to locate the shape).
---
Nitpick comments:
In `@examples/tanstack-router-file-router/src/oidc.ts`:
- Around line 50-55: Add a brief clarifying comment above the Keycloak-specific
calls to indicate this example assumes Keycloak as the OIDC provider; reference
the createKeycloakUtils({ issuerUri }) call and keycloakUtils.fetchUserProfile({
accessToken }) along with fetchUserInfo() so readers know to replace or adapt
createKeycloakUtils and fetchUserProfile when using a non-Keycloak provider.
In `@src/react-spa/createOidcSpaUtils.ts`:
- Around line 318-340: Pass a helpful debugMessage into
createObjectWithSomePropertiesThatThrowIfAccessed so attempts to access
lazy/throwing props provide context; update the call in createOidcSpaUtils (the
createObjectWithSomePropertiesThatThrowIfAccessed invocation that returns
UseOidc.Oidc.LoggedIn) to supply a second argument string that mentions the
object and the properties (e.g., indicating that user or refreshUser are not yet
available) so thrown errors from the getter placeholders (THROW_IF_ACCESSED)
include that message.
- Around line 204-216: The useEffect that subscribes via
resultOfGetUser.subscribeToUserChange(...) closes over resultOfGetUser but has
an empty dependency array; update the hook in createOidcSpaUtils so that
resultOfGetUser is included in the dependency array (e.g., useEffect(...,
[resultOfGetUser])), keeping the existing early return when resultOfGetUser ===
undefined, and ensure the cleanup calls the unsubscribeFromUserChange returned
from subscribeToUserChange (as currently implemented) so it uses the correct
subscription for the current resultOfGetUser.
- Around line 686-692: The runtime check currently uses `instanceof Error` for
`resultOfGetUserOrError` but `ErrorComponent` expects an
`OidcInitializationError`, so update the check around `resultOfGetUserOrError`
to use `instanceof OidcInitializationError` (or a custom type guard) before
passing it into `createElement(ErrorComponent, { oidcInitializationError: ...
})`; if the value can be a different Error at runtime, explicitly convert or
construct an `OidcInitializationError` from the generic Error (or return a safe
fallback) so `ErrorComponent` always receives the correct
`OidcInitializationError` type.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2169b100-2a8d-4150-a23c-61a27304d6f1
⛔ Files ignored due to path filters (1)
examples/tanstack-router-file-router/src/assets/avatarFallback.svgis excluded by!**/*.svg
📒 Files selected for processing (14)
examples/tanstack-router-file-router/src/components/Header.tsxexamples/tanstack-router-file-router/src/oidc.tsexamples/tanstack-router-file-router/src/routeTree.gen.tsexamples/tanstack-router-file-router/src/routes/admin-only.tsxexamples/tanstack-router-file-router/src/routes/protected.tsxsrc/angular.tssrc/core/Oidc.tssrc/core/createGetUser.tssrc/core/createMockOidc.tssrc/core/createOidc.tssrc/react-spa/createOidcSpaUtils.tssrc/react-spa/types.tssrc/react-spa/utilsBuilder.tssrc/tools/createObjectThatThrowsIfAccessed.ts
Comment on lines
+46
to
+52
| const r = await fetch(userinfo_endpoint, { | ||
| headers: { | ||
| Authorization: `Bearer ${accessToken}` | ||
| } | ||
| }); | ||
|
|
||
| return r.json(); |
There was a problem hiding this comment.
Missing error handling for fetch and JSON parsing.
The fetchUserInfo function doesn't handle HTTP errors or JSON parsing failures. If the userinfo endpoint returns a non-2xx status or invalid JSON, this will either silently fail or throw an unclear error.
🛡️ Proposed fix to add error handling
const r = await fetch(userinfo_endpoint, {
headers: {
Authorization: `Bearer ${accessToken}`
}
});
+ if (!r.ok) {
+ throw new Error(
+ `oidc-spa: Failed to fetch userinfo: ${r.status} ${r.statusText}`
+ );
+ }
+
return r.json();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/core/createGetUser.ts` around lines 46 - 52, The fetchUserInfo function
currently returns r.json() without handling HTTP errors or JSON parse failures;
update fetchUserInfo to first check the Response.ok from the
fetch(userinfo_endpoint, { headers: { Authorization: `Bearer ${accessToken}` }
}) call and throw a clear error containing status and statusText (and optionally
response text) for non-2xx responses, then parse the body inside a try/catch so
JSON.parse/response.json failures are caught and rethrown with context
(including the userinfo_endpoint and status), ensuring the function returns the
parsed object only on success and surfaces clear errors on network/HTTP/parse
problems.
garronej
force-pushed
the
user-representation-3
branch
from
7cd87e7 to
5c9773b
Compare
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)
examples/tanstack-router-file-router/src/routes/admin-only.tsx (1)
42-43:⚠️ Potential issue | 🟡 MinorMinor text inconsistency:
isRealmAdminis derived from the access token, not the ID token.Per the
oidc.tsimplementation,isRealmAdminis computed fromdecodedAccessToken?.realm_access?.roles, not the ID token.📝 Suggested fix
- Access is granted because your ID token includes the <code>realm-admin</code> role. + Access is granted because your access token includes the <code>realm-admin</code> role.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/tanstack-router-file-router/src/routes/admin-only.tsx` around lines 42 - 43, The text incorrectly states the role comes from the ID token; update the copy to say the Access Token instead. Locate the admin-only UI text (referenced near isRealmAdmin) and change the sentence that mentions "ID token" to "access token" (or "Access Token") to match how isRealmAdmin is computed from decodedAccessToken?.realm_access?.roles in oidc.ts.src/keycloak/keycloakUtils.ts (1)
225-238:⚠️ Potential issue | 🟡 MinorMissing error handling in
fetchUserProfileandfetchUserInfo.Both methods call
fetch()and returnr.json()without checkingr.ok. HTTP errors (401, 403, 500, etc.) will not throw and instead attempt to parse the error response as JSON.🛡️ Proposed fix to add error handling
fetchUserProfile: ({ accessToken }) => - fetch(`${realmUrl}/account`, { + fetch(`${realmUrl}/account`, { headers: { Accept: "application/json", Authorization: `Bearer ${accessToken}` } - }).then(r => r.json()), + }).then(r => { + if (!r.ok) { + throw new Error(`oidc-spa: Failed to fetch user profile: ${r.status} ${r.statusText}`); + } + return r.json(); + }), fetchUserInfo: ({ accessToken }) => fetch(`${realmUrl}/protocol/openid-connect/userinfo`, { headers: { Accept: "application/json", Authorization: `Bearer ${accessToken}` } - }).then(r => r.json()), + }).then(r => { + if (!r.ok) { + throw new Error(`oidc-spa: Failed to fetch userinfo: ${r.status} ${r.statusText}`); + } + return r.json(); + }),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/keycloak/keycloakUtils.ts` around lines 225 - 238, fetchUserProfile and fetchUserInfo call fetch(...).then(r => r.json()) without checking r.ok, so HTTP error responses are parsed as JSON instead of being surfaced; update both functions (fetchUserProfile and fetchUserInfo) to inspect the Response (r) after fetch, and if !r.ok read the response body (e.g., await r.text() or r.json()) and throw a new Error that includes the status and the returned error details (or statusText) so callers receive a clear, thrown error for non-2xx responses; keep successful behavior returning parsed JSON.
🧹 Nitpick comments (2)
src/tools/createObjectThatThrowsIfAccessed.ts (1)
55-57: Consider freezing the sentinel object.Since
THROW_IF_ACCESSEDis compared by reference equality (line 67), freezing it prevents accidental mutation and signals immutability intent.♻️ Proposed fix
-export const THROW_IF_ACCESSED = { - __brand: "THROW_IF_ACCESSED" -}; +export const THROW_IF_ACCESSED = Object.freeze({ + __brand: "THROW_IF_ACCESSED" +} as const);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tools/createObjectThatThrowsIfAccessed.ts` around lines 55 - 57, THROW_IF_ACCESSED sentinel is mutable; freeze it to enforce immutability and avoid accidental mutation—replace the plain object export with a frozen object (use Object.freeze on the object assigned to THROW_IF_ACCESSED while keeping the __brand property) so reference-equality checks (e.g., the code that compares to THROW_IF_ACCESSED) continue to work and the object cannot be altered at runtime.src/react-spa/utilsBuilder.ts (1)
60-72: Consider using theCreateUser<User>type directly to reduce duplication.The inline type definition duplicates the
CreateUser<User>type imported from./types. Using the imported type would improve maintainability.♻️ Suggested refactor
function createOidcSpaUtilsBuilder< AutoLogin extends boolean = false, DecodedIdToken extends Record<string, unknown> = Oidc_core.Tokens.DecodedIdToken_OidcCoreSpec, User = never >(params: { autoLogin: AutoLogin; decodedIdTokenSchema: | ZodSchemaLike<Oidc_core.Tokens.DecodedIdToken_OidcCoreSpec, DecodedIdToken> | undefined; decodedIdToken_mock: DecodedIdToken | undefined; - createUser: - | ((params: { - decodedIdToken: Oidc_core.Tokens.DecodedIdToken_OidcCoreSpec; - accessToken: string; - fetchUserInfo: () => Promise<{ - [key: string]: unknown; - sub: string; - }>; - issuerUri: string; - user_current: User | undefined; - }) => MaybeAsync<User>) - | undefined; + createUser: CreateUser<User> | undefined; user_mock: User | undefined; }): OidcSpaUtilsBuilder<AutoLogin, DecodedIdToken, User> {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/react-spa/utilsBuilder.ts` around lines 60 - 72, The inline function type for createUser duplicates the existing CreateUser<User> type; replace the long inline declaration for createUser with CreateUser<User> (keeping the same optional/union with undefined if needed) and remove the duplicated shape, and ensure the CreateUser type is imported from ./types (add the import if missing) so the declaration becomes something like createUser: CreateUser<User> | undefined while leaving user_mock as-is.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/core/createOidc.ts`:
- Line 1834: The Angular adapter currently excludes createUser from
ParamsOfProvide (the omission visible where ParamsOfProvide is defined) which
prevents Angular consumers from using the User abstraction; either add support
for createUser in the Angular adapter implementation or document the intentional
limitation: update the Angular adapter's adapter comment or README to explain
why createUser is omitted (referencing ParamsOfProvide and createUser) and mark
the feature as WIP, or implement createUser behavior in the Angular-specific
provider code (the adapter that constructs oidc_loggedIn /
Oidc.LoggedIn<DecodedIdToken, User>) so Angular consumers can use the User
abstraction.
In `@src/keycloak/keycloakUtils.ts`:
- Around line 177-180: The change of UserInfo's index signature from [key:
string]: any to [key: string]: unknown is a breaking API change; revert to a
backward-compatible type by restoring the original index signature (e.g., [key:
string]: any) on the exported type UserInfo or introduce a new non-breaking
alias (e.g., export type KeycloakUserInfo = UserInfoAny) while keeping UserInfo
as before; ensure references to UserInfo and the re-exported KeycloakUserInfo in
this file are updated to the restored type so existing consumers continue to
type-check without changes.
In `@src/react-spa/types.ts`:
- Around line 379-392: The file fails Prettier formatting; run the project's
formatter (e.g., prettier --write) on the file containing the CreateUser type
and the assert<Equals<CreateUser<{ _brand: string }>,
ParamsOfCreateOidc.CreateUser<{ _brand: string }>>> line to normalize spacing
and line breaks to the repo's Prettier config, then re-stage and commit the
formatted changes so the CI passes. Ensure the CreateUser type declaration and
the trailing assert line are preserved exactly (no semicolon/line-break changes
that alter TypeScript semantics) while applying the automatic Prettier fixes.
---
Outside diff comments:
In `@examples/tanstack-router-file-router/src/routes/admin-only.tsx`:
- Around line 42-43: The text incorrectly states the role comes from the ID
token; update the copy to say the Access Token instead. Locate the admin-only UI
text (referenced near isRealmAdmin) and change the sentence that mentions "ID
token" to "access token" (or "Access Token") to match how isRealmAdmin is
computed from decodedAccessToken?.realm_access?.roles in oidc.ts.
In `@src/keycloak/keycloakUtils.ts`:
- Around line 225-238: fetchUserProfile and fetchUserInfo call fetch(...).then(r
=> r.json()) without checking r.ok, so HTTP error responses are parsed as JSON
instead of being surfaced; update both functions (fetchUserProfile and
fetchUserInfo) to inspect the Response (r) after fetch, and if !r.ok read the
response body (e.g., await r.text() or r.json()) and throw a new Error that
includes the status and the returned error details (or statusText) so callers
receive a clear, thrown error for non-2xx responses; keep successful behavior
returning parsed JSON.
---
Nitpick comments:
In `@src/react-spa/utilsBuilder.ts`:
- Around line 60-72: The inline function type for createUser duplicates the
existing CreateUser<User> type; replace the long inline declaration for
createUser with CreateUser<User> (keeping the same optional/union with undefined
if needed) and remove the duplicated shape, and ensure the CreateUser type is
imported from ./types (add the import if missing) so the declaration becomes
something like createUser: CreateUser<User> | undefined while leaving user_mock
as-is.
In `@src/tools/createObjectThatThrowsIfAccessed.ts`:
- Around line 55-57: THROW_IF_ACCESSED sentinel is mutable; freeze it to enforce
immutability and avoid accidental mutation—replace the plain object export with
a frozen object (use Object.freeze on the object assigned to THROW_IF_ACCESSED
while keeping the __brand property) so reference-equality checks (e.g., the code
that compares to THROW_IF_ACCESSED) continue to work and the object cannot be
altered at runtime.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 91ecf919-60c8-46ed-be10-2200f42c36c2
⛔ Files ignored due to path filters (1)
examples/tanstack-router-file-router/src/assets/avatarFallback.svgis excluded by!**/*.svg
📒 Files selected for processing (16)
examples/tanstack-router-file-router/src/components/Header.tsxexamples/tanstack-router-file-router/src/oidc.tsexamples/tanstack-router-file-router/src/routeTree.gen.tsexamples/tanstack-router-file-router/src/routes/admin-only.tsxexamples/tanstack-router-file-router/src/routes/protected.tsxsrc/angular.tssrc/core/Oidc.tssrc/core/createGetUser.tssrc/core/createMockOidc.tssrc/core/createOidc.tssrc/keycloak/index.tssrc/keycloak/keycloakUtils.tssrc/react-spa/createOidcSpaUtils.tssrc/react-spa/types.tssrc/react-spa/utilsBuilder.tssrc/tools/createObjectThatThrowsIfAccessed.ts
✅ Files skipped from review due to trivial changes (2)
- src/keycloak/index.ts
- examples/tanstack-router-file-router/src/routeTree.gen.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- src/angular.ts
- examples/tanstack-router-file-router/src/components/Header.tsx
- examples/tanstack-router-file-router/src/routes/protected.tsx
- src/core/Oidc.ts
- src/react-spa/createOidcSpaUtils.ts
garronej
force-pushed
the
user-representation-3
branch
from
5c9773b to
3dc06b6
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/react-spa/createOidcSpaUtils.ts (1)
380-425:⚠️ Potential issue | 🟠 Major
getOidc()ignores the new user-initialization failure path.
useOidc()and the initialization gates already wait ondResultOfGetUserOrInitializationErrorOrUndefined, but this logged-in branch still returns as soon as core init succeeds. If the firstcreateUser()call failed during bootstrap, callers here still get a logged-in object instead of theOidcInitializationErrorthe rest of the React API sees.Also applies to: 567-585
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/react-spa/createOidcSpaUtils.ts` around lines 380 - 425, The logged-in branch returned by getOidc() currently ignores the user-initialization failure state from dResultOfGetUserOrInitializationErrorOrUndefined, so callers can receive a logged-in object even if createUser() failed; update the branch that builds the id<GetOidc.Oidc.LoggedIn...> object to first check dResultOfGetUserOrInitializationErrorOrUndefined and, if it contains an OidcInitializationError result, return the corresponding error/result path instead of the logged-in object; apply the same guard/fix to the analogous block referenced around the other occurrence (the 567-585 region) so both places honor the initialization error state used by useOidc() and the initialization gates.
🧹 Nitpick comments (1)
src/core/createOidc.ts (1)
78-79: Thread the parsedDecodedIdTokentype intoCreateUser.
CreateUsercurrently receivesDecodedIdToken_OidcCoreSpec, so callers that already supplieddecodedIdTokenSchemalose that stronger type exactly where the new user abstraction is built. Making thisCreateUser<DecodedIdToken, User>would avoid re-parsing/casting at the boundary and keepcreateUseraligned with the rest of the API.Based on learnings, when a zod schema is used via withExpectedDecodedIdTokenShape to enforce the shape of decoded ID tokens, those fields are guaranteed to exist when isUserLoggedIn is true.
Also applies to: 274-285
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/createOidc.ts` around lines 78 - 79, The CreateUser type is currently parameterized with DecodedIdToken_OidcCoreSpec causing callers who provided a custom decodedIdTokenSchema to lose its stronger parsed type; update the CreateUser generic to accept DecodedIdToken (i.e., change usages from CreateUser<DecodedIdToken_OidcCoreSpec, User> to CreateUser<DecodedIdToken, User>) so createUser callbacks receive the parsed token shape directly; apply the same change to all occurrences (notably the createUser?: declaration and the related code around the createUser invocation in the 274-285 region), and ensure type plumbing from withExpectedDecodedIdTokenShape and isUserLoggedIn carries the DecodedIdToken generic through to avoid re-parsing/casting at the boundary.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@examples/tanstack-router-file-router/src/oidc.ts`:
- Around line 43-45: The DecodedAccessToken validation currently uses a throwing
parse which lets a JWT with an unexpected realm_access shape bubble up and crash
createUser; instead, use DecodedAccessToken.safeParse(...) when validating the
decoded token and, if safeParse returns success use the parsed value, otherwise
treat realm_access as absent and keep isRealmAdmin = false; apply the same
safeParse pattern to the other validation at the second occurrence (the one
referenced around lines 59-61) so both places are best-effort and never throw on
malformed realm_access.
In `@src/core/createGetUser.ts`:
- Around line 70-101: prUser_new is assigned a promise that can reject (e.g.,
createUser/fetchUserInfo failures) and that rejected promise is cached at
state.prUser, poisoning future calls; fix by wrapping the IIFE so any rejection
clears or resets the cached state and prevents an unhandled rejection: inside
the IIFE or immediately after creating prUser_new attach a .catch(err => { state
= { hash: undefined, prUser: undefined } (or delete state.prUser); throw err; })
so failures don’t leave a rejected promise in state; apply the same pattern
where state.prUser is set (also at the other occurrence referred to around lines
116-118) and ensure callers get the original error while the cache is cleared
for retry.
In `@src/core/Oidc.ts`:
- Around line 4-6: The public OIDC user type wrongly always exposes getUser even
when no User is configured; change the exported union so that Oidc.LoggedIn
includes getUser only when the generic User parameter is not never (e.g. use a
conditional type: LoggedIn<DecodedIdToken, User> extends ... or make the union
use User extends never ? Oidc.LoggedInWithoutUser : Oidc.LoggedInWithUser), and
update the runtime implementations referenced (Oidc.LoggedIn / Oidc.NotLoggedIn
factories and the thrower implementations currently returning getUser) to omit
getUser entirely when User is never; apply the same conditional removal to the
other occurrences noted (the blocks around the symbols at lines referenced: the
LoggedIn/NotLoggedIn definitions and the runtime factories around getUser at the
other locations) so the type contract matches runtime behavior.
In `@src/keycloak/keycloakUtils.ts`:
- Around line 16-18: The userProfileMetadata.groups property is incorrectly
typed as KeycloakProfile.Attribute[]; update its type to KeycloakProfile.Group[]
so groups use the proper shape (replace KeycloakProfile.Attribute[] with
KeycloakProfile.Group[] for userProfileMetadata.groups in the
userProfileMetadata declaration) to align with the defined KeycloakProfile.Group
type and avoid attribute-only fields like validators/annotations/readOnly being
expected on group objects.
In `@src/tools/createObjectThatThrowsIfAccessed.ts`:
- Around line 55-57: Replace the plain object sentinel with a true unique symbol
so the identity check in createObjectWithSomePropertiesThatThrowIfAccessed() is
enforced by TypeScript; specifically change THROW_IF_ACCESSED from an object
literal to a unique symbol (e.g. export const THROW_IF_ACCESSED: unique symbol =
Symbol("THROW_IF_ACCESSED")) and keep the runtime comparisons (value ===
THROW_IF_ACCESSED) as-is so the trap cannot be bypassed by a structurally
identical object.
---
Outside diff comments:
In `@src/react-spa/createOidcSpaUtils.ts`:
- Around line 380-425: The logged-in branch returned by getOidc() currently
ignores the user-initialization failure state from
dResultOfGetUserOrInitializationErrorOrUndefined, so callers can receive a
logged-in object even if createUser() failed; update the branch that builds the
id<GetOidc.Oidc.LoggedIn...> object to first check
dResultOfGetUserOrInitializationErrorOrUndefined and, if it contains an
OidcInitializationError result, return the corresponding error/result path
instead of the logged-in object; apply the same guard/fix to the analogous block
referenced around the other occurrence (the 567-585 region) so both places honor
the initialization error state used by useOidc() and the initialization gates.
---
Nitpick comments:
In `@src/core/createOidc.ts`:
- Around line 78-79: The CreateUser type is currently parameterized with
DecodedIdToken_OidcCoreSpec causing callers who provided a custom
decodedIdTokenSchema to lose its stronger parsed type; update the CreateUser
generic to accept DecodedIdToken (i.e., change usages from
CreateUser<DecodedIdToken_OidcCoreSpec, User> to CreateUser<DecodedIdToken,
User>) so createUser callbacks receive the parsed token shape directly; apply
the same change to all occurrences (notably the createUser?: declaration and the
related code around the createUser invocation in the 274-285 region), and ensure
type plumbing from withExpectedDecodedIdTokenShape and isUserLoggedIn carries
the DecodedIdToken generic through to avoid re-parsing/casting at the boundary.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8ab32fb0-f32d-46a9-b30a-196cb34f220e
⛔ Files ignored due to path filters (1)
examples/tanstack-router-file-router/src/assets/avatarFallback.svgis excluded by!**/*.svg
📒 Files selected for processing (16)
examples/tanstack-router-file-router/src/components/Header.tsxexamples/tanstack-router-file-router/src/oidc.tsexamples/tanstack-router-file-router/src/routeTree.gen.tsexamples/tanstack-router-file-router/src/routes/admin-only.tsxexamples/tanstack-router-file-router/src/routes/protected.tsxsrc/angular.tssrc/core/Oidc.tssrc/core/createGetUser.tssrc/core/createMockOidc.tssrc/core/createOidc.tssrc/keycloak/index.tssrc/keycloak/keycloakUtils.tssrc/react-spa/createOidcSpaUtils.tssrc/react-spa/types.tssrc/react-spa/utilsBuilder.tssrc/tools/createObjectThatThrowsIfAccessed.ts
✅ Files skipped from review due to trivial changes (1)
- examples/tanstack-router-file-router/src/routeTree.gen.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- examples/tanstack-router-file-router/src/routes/admin-only.tsx
- src/keycloak/index.ts
- src/angular.ts
- examples/tanstack-router-file-router/src/components/Header.tsx
- src/react-spa/utilsBuilder.ts
- examples/tanstack-router-file-router/src/routes/protected.tsx
- src/react-spa/types.ts
Comment on lines
+43
to
+45
| const DecodedAccessToken = z.object({ | ||
| realm_access: z.object({ roles: z.array(z.string()) }).optional() | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n examples/tanstack-router-file-router/src/oidc.ts | head -80Repository: keycloakify/oidc-spa
Length of output: 3184
🏁 Script executed:
cat -n examples/tanstack-router-file-router/src/oidc.ts | sed -n '80,100p'Repository: keycloakify/oidc-spa
Length of output: 840
Treat the access-token shape check as best-effort all the way through.
The try/catch only handles non-JWT tokens. A JWT with an unexpected realm_access shape will still make createUser() throw and fail initialization, even though the fallback here is just isRealmAdmin = false. safeParse() would preserve that fallback.
🛠️ Proposed fix
const decodedAccessToken = !decodedAccessToken_generic
? undefined
- : DecodedAccessToken.parse(decodedAccessToken_generic);
+ : (() => {
+ const parsed = DecodedAccessToken.safeParse(decodedAccessToken_generic);
+ return parsed.success ? parsed.data : undefined;
+ })();Also applies to: 59-61
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@examples/tanstack-router-file-router/src/oidc.ts` around lines 43 - 45, The
DecodedAccessToken validation currently uses a throwing parse which lets a JWT
with an unexpected realm_access shape bubble up and crash createUser; instead,
use DecodedAccessToken.safeParse(...) when validating the decoded token and, if
safeParse returns success use the parsed value, otherwise treat realm_access as
absent and keep isRealmAdmin = false; apply the same safeParse pattern to the
other validation at the second occurrence (the one referenced around lines
59-61) so both places are best-effort and never throw on malformed realm_access.
Comment on lines
+70
to
+101
| const prUser_new = (async () => { | ||
| const prUser_current = state?.prUser; | ||
|
|
||
| if (hash_current === hash_new) { | ||
| assert(prUser_current !== undefined); | ||
| return prUser_current; | ||
| } | ||
|
|
||
| const user_current = await prUser_current; | ||
|
|
||
| const user_new = await createUser({ | ||
| accessToken: tokens.accessToken, | ||
| decodedIdToken: tokens.decodedIdToken_original, | ||
| issuerUri, | ||
| fetchUserInfo: () => fetchUserInfo({ accessToken: tokens.accessToken }), | ||
| user_current | ||
| }); | ||
|
|
||
| onUserChanges.forEach(onUserChange => | ||
| onUserChange({ | ||
| user: user_new, | ||
| user_previous: user_current | ||
| }) | ||
| ); | ||
|
|
||
| return user_new; | ||
| })(); | ||
|
|
||
| state = { | ||
| hash: hash_new, | ||
| prUser: prUser_new | ||
| }; |
There was a problem hiding this comment.
A failed recomputation poisons the user cache.
prUser_new can reject on token rotations, but this promise is cached immediately and nothing here converts that failure into a recoverable state. After the first createUser()/fetchUserInfo() failure, state.prUser stays rejected, every later getUser()/refreshUser() rethrows it, and the original rejection is likely unhandled because the subscription path doesn't await it.
Also applies to: 116-118
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/core/createGetUser.ts` around lines 70 - 101, prUser_new is assigned a
promise that can reject (e.g., createUser/fetchUserInfo failures) and that
rejected promise is cached at state.prUser, poisoning future calls; fix by
wrapping the IIFE so any rejection clears or resets the cached state and
prevents an unhandled rejection: inside the IIFE or immediately after creating
prUser_new attach a .catch(err => { state = { hash: undefined, prUser: undefined
} (or delete state.prUser); throw err; }) so failures don’t leave a rejected
promise in state; apply the same pattern where state.prUser is set (also at the
other occurrence referred to around lines 116-118) and ensure callers get the
original error while the cache is cleared for retry.
3dc06b6 to
1d9c85f
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
examples/tanstack-router-file-router/src/routes/protected.tsx (1)
1-1:⚠️ Potential issue | 🟡 MinorFix Prettier formatting.
The CI pipeline reports this file is not formatted according to project rules.
npx prettier --write examples/tanstack-router-file-router/src/routes/protected.tsx🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/tanstack-router-file-router/src/routes/protected.tsx` at line 1, The file import line in protected.tsx is not formatted to project Prettier rules; run the formatter (e.g., npx prettier --write) on examples/tanstack-router-file-router/src/routes/protected.tsx or apply your editor's Prettier formatting so the import statement (createFileRoute) and surrounding whitespace match project style, then stage the reformatted file and push the change so CI passes.
🧹 Nitpick comments (1)
src/core/createMockOidc.ts (1)
234-246: Mock getUser implementation is functional but inconsistent with type contract.The implementation throws when
mockedUserisundefined, which aligns with the runtime behavior of the real implementation. However, sincegetUseris unconditionally exposed in the type (even whenUser = never), consumers might call it unexpectedly.Consider adding documentation or a more descriptive error guiding users to provide
mockedUserin their mock configuration.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/createMockOidc.ts` around lines 234 - 246, The getUser mock currently throws a generic Error when mockedUser is undefined; update its error to a more descriptive message that directs consumers to supply mockedUser in the mock configuration (reference getUser and mockedUser) and add a concise JSDoc above getUser explaining that it will throw unless mockedUser is provided and how to configure the mock; ensure the error text and documentation mention the exact config key (mockedUser) so callers know how to fix it.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@examples/tanstack-router-file-router/src/components/Header.tsx`:
- Around line 112-121: The file fails Prettier formatting; run the project's
Prettier formatter on the component to fix it: format
examples/tanstack-router-file-router/src/components/Header.tsx (e.g., run npx
prettier --write examples/tanstack-router-file-router/src/components/Header.tsx)
so the AdminOnlyNavLink component and surrounding code (function
AdminOnlyNavLink, useOidc, user.isRealmAdmin) conform to the project's style
rules and CI will pass.
In `@src/tools/createObjectThatThrowsIfAccessed.ts`:
- Around line 63-67: The current implementation uses Object.entries(obj) which
eagerly invokes getters; change the logic in createObjectThatThrowsIfAccessed to
inspect property descriptors via Object.getOwnPropertyDescriptors(obj) and only
rewrite properties whose descriptor is a data descriptor with descriptor.value
=== THROW_IF_ACCESSED, building the map passed to Object.defineProperties from
those descriptors (not from reading the values) so getters are not executed;
reference THROW_IF_ACCESSED, createObjectThatThrowsIfAccessed,
Object.getOwnPropertyDescriptors, and Object.defineProperties when making the
change.
---
Outside diff comments:
In `@examples/tanstack-router-file-router/src/routes/protected.tsx`:
- Line 1: The file import line in protected.tsx is not formatted to project
Prettier rules; run the formatter (e.g., npx prettier --write) on
examples/tanstack-router-file-router/src/routes/protected.tsx or apply your
editor's Prettier formatting so the import statement (createFileRoute) and
surrounding whitespace match project style, then stage the reformatted file and
push the change so CI passes.
---
Nitpick comments:
In `@src/core/createMockOidc.ts`:
- Around line 234-246: The getUser mock currently throws a generic Error when
mockedUser is undefined; update its error to a more descriptive message that
directs consumers to supply mockedUser in the mock configuration (reference
getUser and mockedUser) and add a concise JSDoc above getUser explaining that it
will throw unless mockedUser is provided and how to configure the mock; ensure
the error text and documentation mention the exact config key (mockedUser) so
callers know how to fix it.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 227adefd-0ec7-4e71-a84a-aedd88abe98a
⛔ Files ignored due to path filters (1)
examples/tanstack-router-file-router/src/assets/avatarFallback.svgis excluded by!**/*.svg
📒 Files selected for processing (16)
examples/tanstack-router-file-router/src/components/Header.tsxexamples/tanstack-router-file-router/src/oidc.tsexamples/tanstack-router-file-router/src/routeTree.gen.tsexamples/tanstack-router-file-router/src/routes/admin-only.tsxexamples/tanstack-router-file-router/src/routes/protected.tsxsrc/angular.tssrc/core/Oidc.tssrc/core/createGetUser.tssrc/core/createMockOidc.tssrc/core/createOidc.tssrc/keycloak/index.tssrc/keycloak/keycloakUtils.tssrc/react-spa/createOidcSpaUtils.tssrc/react-spa/types.tssrc/react-spa/utilsBuilder.tssrc/tools/createObjectThatThrowsIfAccessed.ts
✅ Files skipped from review due to trivial changes (3)
- src/keycloak/index.ts
- examples/tanstack-router-file-router/src/routeTree.gen.ts
- src/react-spa/types.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- examples/tanstack-router-file-router/src/routes/admin-only.tsx
- src/angular.ts
- src/react-spa/createOidcSpaUtils.ts
Comment on lines
112
to
121
| function AdminOnlyNavLink() { | ||
| const { isUserLoggedIn, decodedIdToken } = useOidc(); | ||
| const { isUserLoggedIn, user } = useOidc(); | ||
|
|
||
| if (!isUserLoggedIn) { | ||
| return null; | ||
| } | ||
|
|
||
| if (!decodedIdToken.realm_access?.roles.includes("realm-admin")) { | ||
| if (!user.isRealmAdmin) { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
Fix Prettier formatting.
The CI pipeline reports this file is not formatted according to project rules. Run Prettier to fix formatting.
npx prettier --write examples/tanstack-router-file-router/src/components/Header.tsx🧰 Tools
🪛 GitHub Actions: ci
[error] Prettier ' --list-different' reported this file is not formatted according to project rules.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@examples/tanstack-router-file-router/src/components/Header.tsx` around lines
112 - 121, The file fails Prettier formatting; run the project's Prettier
formatter on the component to fix it: format
examples/tanstack-router-file-router/src/components/Header.tsx (e.g., run npx
prettier --write examples/tanstack-router-file-router/src/components/Header.tsx)
so the AdminOnlyNavLink component and surrounding code (function
AdminOnlyNavLink, useOidc, user.isRealmAdmin) conform to the project's style
rules and CI will pass.
1d9c85f to
3e61d47
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
src/tools/createObjectThatThrowsIfAccessed.ts (2)
63-80:⚠️ Potential issue | 🟠 MajorAvoid
Object.entries(obj)here; it eagerly reads values/getters.At
Line 66, scanning withObject.entries(obj)can execute accessors during construction. Use property descriptors and only rewrite data descriptors whose.value === THROW_IF_ACCESSED.Suggested fix
export function createObjectWithSomePropertiesThatThrowIfAccessed<T extends Record<string, unknown>>( obj: { [K in keyof T]: T[K] | typeof THROW_IF_ACCESSED }, debugMessage?: string ): T { + const descriptors = Object.getOwnPropertyDescriptors(obj); + return Object.defineProperties( obj, Object.fromEntries( - Object.entries(obj) - .filter(([, value]) => value === THROW_IF_ACCESSED) - .map(([key]) => { + Object.entries(descriptors) + .filter(([, descriptor]) => "value" in descriptor && descriptor.value === THROW_IF_ACCESSED) + .map(([key, descriptor]) => { const getAndSet = () => { throw new AccessError(`Cannot access ${key} yet ${debugMessage ?? ""}`); }; const pd = { get: getAndSet, set: getAndSet, - enumerable: true + enumerable: descriptor.enumerable }; return [key, pd]; }) ) ) as any; }#!/bin/bash # Verify current implementation uses Object.entries and inspect call sites. sed -n '59,90p' src/tools/createObjectThatThrowsIfAccessed.ts rg -n --type ts --type tsx 'createObjectWithSomePropertiesThatThrowIfAccessed' src/react-spa/createOidcSpaUtils.ts sed -n '300,360p' src/react-spa/createOidcSpaUtils.ts🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tools/createObjectThatThrowsIfAccessed.ts` around lines 63 - 80, The current implementation of createObjectThatThrowsIfAccessed uses Object.entries(obj), which can invoke getters and eagerly read values; instead iterate the object's own property names and use Object.getOwnPropertyDescriptor to inspect descriptors and only replace data descriptors whose value === THROW_IF_ACCESSED. Update the logic around Object.defineProperties so it calls Object.getOwnPropertyNames/Object.getOwnPropertySymbols (or Reflect.ownKeys) and for each key uses Object.getOwnPropertyDescriptor to check descriptor.value === THROW_IF_ACCESSED before creating the get/set AccessError descriptor (reusing AccessError and THROW_IF_ACCESSED names) and then pass the collected descriptors to Object.defineProperties on the original obj.
55-57:⚠️ Potential issue | 🟠 MajorMake
THROW_IF_ACCESSEDnominal (unique symbol) instead of structural.
Line 55currently exports a plain object sentinel. Because TypeScript is structural, lookalike objects can type-check but will fail the runtime identity check atLine 67, causing silent non-trapping behavior.Suggested fix
-export const THROW_IF_ACCESSED = { - __brand: "THROW_IF_ACCESSED" -}; +export const THROW_IF_ACCESSED: unique symbol = Symbol("THROW_IF_ACCESSED");#!/bin/bash # Verify sentinel declaration style and identity-check usage sites. rg -n --type ts --type tsx 'THROW_IF_ACCESSED|value === THROW_IF_ACCESSED|__brand: "THROW_IF_ACCESSED"|unique symbol|Symbol\("THROW_IF_ACCESSED"\)' \ src/tools/createObjectThatThrowsIfAccessed.ts \ src/react-spa/createOidcSpaUtils.ts🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tools/createObjectThatThrowsIfAccessed.ts` around lines 55 - 57, Replace the plain object sentinel THROW_IF_ACCESSED with a nominal unique symbol sentinel so identity checks can't be forged by lookalike objects: export a unique symbol (e.g., declare/export const THROW_IF_ACCESSED: unique symbol = Symbol("THROW_IF_ACCESSED")) and change any type annotations that used the __brand string to use typeof THROW_IF_ACCESSED (or the unique symbol type) instead; update sites that previously relied on a __brand property or structural equality to perform strict identity checks against THROW_IF_ACCESSED so they still compare value === THROW_IF_ACCESSED and consume the symbol-typed sentinel.src/core/createGetUser.ts (1)
70-119:⚠️ Potential issue | 🟠 MajorClear or roll back the cached promise on rejection.
The new fallback only handles subsequent
createUser()failures. If the firstcreateUser()/fetchUserInfo()call rejects, or if asubscribeToUserChange()callback throws inside this promise,state.prUseris left rejected and every latergetUser()keeps rethrowing until the hash changes or callers force a refresh. Restore the previous cache entry or clear it in a catch attached toprUser_newbefore storing it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/createGetUser.ts` around lines 70 - 119, prUser_new can become a permanently rejected cached promise if createUser(), fetchUserInfo(), or onUserChanges throw; before assigning it into state, attach a .catch handler to prUser_new that restores the previous cache (set state.prUser back to prUser_current or undefined) and restores state.hash to hash_current when appropriate, then rethrow the error so callers still see the failure; update references to prUser_new, state.prUser, state.hash, hash_new, hash_current, createUser, fetchUserInfo, and onUserChanges to implement this rollback-to-previous-cache behavior.src/core/Oidc.ts (1)
40-43:⚠️ Potential issue | 🟠 MajorMake
getUserconditional onUser.With
User = neveras the default,Oidc.LoggedIn<DecodedIdToken>still advertisesgetUser(). The runtime factories only have throwing placeholders in that mode, so this leaves a public type contract that callers can compile against but cannot actually use. Please make the property conditional onUserand mirror that omission in the runtime factories.Also applies to: 91-99
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/Oidc.ts` around lines 40 - 43, The LoggedIn generic type currently always includes getUser(), even when the User generic defaults to never; change the LoggedIn type so getUser is only present when User is not never (e.g., use a conditional type on the User generic to include getUser: () => User when User extends never ? never : () => User), and update the corresponding runtime factory functions (the factories that currently return throwing placeholder getUser implementations) to mirror that type change by omitting the getUser property entirely when User is never and only providing a real or placeholder getUser when User is a concrete type.examples/tanstack-router-file-router/src/oidc.ts (1)
49-52:⚠️ Potential issue | 🟠 MajorKeep Keycloak role extraction best-effort.
DecodedAccessToken.parse(decodeJwt(accessToken))still turns a malformed or opaque access token into a hardcreateUser()failure, even though this value is only used to deriveisRealmAdminwith a?? falsefallback. Treat decode/schema failures asundefinedhere so non-conforming tokens do not break login.Suggested fix
- const decodedAccessToken = !isKeycloak({ issuerUri }) - ? undefined - : DecodedAccessToken.parse(decodeJwt(accessToken)); + const decodedAccessToken = !isKeycloak({ issuerUri }) + ? undefined + : (() => { + try { + const parsed = DecodedAccessToken.safeParse(decodeJwt(accessToken)); + return parsed.success ? parsed.data : undefined; + } catch { + return undefined; + } + })();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/tanstack-router-file-router/src/oidc.ts` around lines 49 - 52, The Keycloak access token decoding/parsing must be best-effort so malformed tokens don't hard-fail user creation: update the decodedAccessToken assignment (currently using DecodedAccessToken.parse(decodeJwt(accessToken))) to treat any decode or schema validation errors as undefined; e.g., wrap decodeJwt + DecodedAccessToken.parse in a try/catch or use DecodedAccessToken.safeParse and set decodedAccessToken = undefined on failure, leaving downstream checks (isRealmAdmin ?? false) unchanged.
🧹 Nitpick comments (1)
src/react-spa/createOidcSpaUtils.ts (1)
588-593: Consider adding a clarifying comment for the mutation subscription.This subscription mutates
resultOfGetUser.userdirectly to keep the reference synchronized for any code holding onto the result object. While functionally correct (this is a singleton that lives for the app lifetime), the pattern is non-obvious and could benefit from a brief comment explaining the intent.dResultOfGetUserOrInitializationErrorOrUndefined.resolve(resultOfGetUser); + // Keep the user property on resultOfGetUser synchronized + // for any code that holds a reference to this object. resultOfGetUser.subscribeToUserChange(({ user }) => { resultOfGetUser.user = user; });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/react-spa/createOidcSpaUtils.ts` around lines 588 - 593, Add a brief clarifying comment immediately above the subscribeToUserChange callback where resultOfGetUser.user is mutated: explain that the callback intentionally updates resultOfGetUser.user to keep the original result object reference in sync for any code holding that singleton-like result object (so subscribers and callers maintain the same reference across user changes). Reference the symbols resultOfGetUser and subscribeToUserChange and place the comment next to the mutation to make the intent explicit.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/core/createGetUser.ts`:
- Around line 58-59: The refresh listener is always armed even when createUser
is undefined, causing the listener to call __updatePrUserIfHashChanged and hit
the assert; change createGetUser (the code that installs the refresh listener)
to only register the token refresh handler when createUser is provided (i.e.,
check createUser !== undefined before attaching the listener), and similarly
guard other spots that reference __updatePrUserIfHashChanged (see the other
occurrence around lines 134-136) so the listener and any calls to
__updatePrUserIfHashChanged are never invoked when createUser is absent.
---
Duplicate comments:
In `@examples/tanstack-router-file-router/src/oidc.ts`:
- Around line 49-52: The Keycloak access token decoding/parsing must be
best-effort so malformed tokens don't hard-fail user creation: update the
decodedAccessToken assignment (currently using
DecodedAccessToken.parse(decodeJwt(accessToken))) to treat any decode or schema
validation errors as undefined; e.g., wrap decodeJwt + DecodedAccessToken.parse
in a try/catch or use DecodedAccessToken.safeParse and set decodedAccessToken =
undefined on failure, leaving downstream checks (isRealmAdmin ?? false)
unchanged.
In `@src/core/createGetUser.ts`:
- Around line 70-119: prUser_new can become a permanently rejected cached
promise if createUser(), fetchUserInfo(), or onUserChanges throw; before
assigning it into state, attach a .catch handler to prUser_new that restores the
previous cache (set state.prUser back to prUser_current or undefined) and
restores state.hash to hash_current when appropriate, then rethrow the error so
callers still see the failure; update references to prUser_new, state.prUser,
state.hash, hash_new, hash_current, createUser, fetchUserInfo, and onUserChanges
to implement this rollback-to-previous-cache behavior.
In `@src/core/Oidc.ts`:
- Around line 40-43: The LoggedIn generic type currently always includes
getUser(), even when the User generic defaults to never; change the LoggedIn
type so getUser is only present when User is not never (e.g., use a conditional
type on the User generic to include getUser: () => User when User extends never
? never : () => User), and update the corresponding runtime factory functions
(the factories that currently return throwing placeholder getUser
implementations) to mirror that type change by omitting the getUser property
entirely when User is never and only providing a real or placeholder getUser
when User is a concrete type.
In `@src/tools/createObjectThatThrowsIfAccessed.ts`:
- Around line 63-80: The current implementation of
createObjectThatThrowsIfAccessed uses Object.entries(obj), which can invoke
getters and eagerly read values; instead iterate the object's own property names
and use Object.getOwnPropertyDescriptor to inspect descriptors and only replace
data descriptors whose value === THROW_IF_ACCESSED. Update the logic around
Object.defineProperties so it calls
Object.getOwnPropertyNames/Object.getOwnPropertySymbols (or Reflect.ownKeys) and
for each key uses Object.getOwnPropertyDescriptor to check descriptor.value ===
THROW_IF_ACCESSED before creating the get/set AccessError descriptor (reusing
AccessError and THROW_IF_ACCESSED names) and then pass the collected descriptors
to Object.defineProperties on the original obj.
- Around line 55-57: Replace the plain object sentinel THROW_IF_ACCESSED with a
nominal unique symbol sentinel so identity checks can't be forged by lookalike
objects: export a unique symbol (e.g., declare/export const THROW_IF_ACCESSED:
unique symbol = Symbol("THROW_IF_ACCESSED")) and change any type annotations
that used the __brand string to use typeof THROW_IF_ACCESSED (or the unique
symbol type) instead; update sites that previously relied on a __brand property
or structural equality to perform strict identity checks against
THROW_IF_ACCESSED so they still compare value === THROW_IF_ACCESSED and consume
the symbol-typed sentinel.
---
Nitpick comments:
In `@src/react-spa/createOidcSpaUtils.ts`:
- Around line 588-593: Add a brief clarifying comment immediately above the
subscribeToUserChange callback where resultOfGetUser.user is mutated: explain
that the callback intentionally updates resultOfGetUser.user to keep the
original result object reference in sync for any code holding that
singleton-like result object (so subscribers and callers maintain the same
reference across user changes). Reference the symbols resultOfGetUser and
subscribeToUserChange and place the comment next to the mutation to make the
intent explicit.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8a5814e2-565c-40cd-a659-b2b884480bea
⛔ Files ignored due to path filters (1)
examples/tanstack-router-file-router/src/assets/avatarFallback.svgis excluded by!**/*.svg
📒 Files selected for processing (16)
examples/tanstack-router-file-router/src/components/Header.tsxexamples/tanstack-router-file-router/src/oidc.tsexamples/tanstack-router-file-router/src/routeTree.gen.tsexamples/tanstack-router-file-router/src/routes/admin-only.tsxexamples/tanstack-router-file-router/src/routes/protected.tsxsrc/angular.tssrc/core/Oidc.tssrc/core/createGetUser.tssrc/core/createMockOidc.tssrc/core/createOidc.tssrc/keycloak/index.tssrc/keycloak/keycloakUtils.tssrc/react-spa/createOidcSpaUtils.tssrc/react-spa/types.tssrc/react-spa/utilsBuilder.tssrc/tools/createObjectThatThrowsIfAccessed.ts
✅ Files skipped from review due to trivial changes (4)
- src/angular.ts
- src/keycloak/index.ts
- examples/tanstack-router-file-router/src/routeTree.gen.ts
- src/react-spa/types.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- examples/tanstack-router-file-router/src/routes/admin-only.tsx
- src/react-spa/utilsBuilder.ts
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
src/core/Oidc.ts (1)
40-43:⚠️ Potential issue | 🟠 MajorDon't expose
getUser()when no user source exists.
Userdefaults tonever, butOidc.LoggedInstill exposesgetUser(). The current runtime implementations insrc/core/createGetUser.tsandsrc/core/createMockOidc.tsthrow whencreateUser/mockedUseris absent, so typed consumers can call a method that is guaranteed to fail. Make this member conditional with[User] extends [never], and make the factories require the corresponding user provider whenever a non-neverUseris requested.Also applies to: 91-99
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/Oidc.ts` around lines 40 - 43, The LoggedIn type currently always exposes getUser(), causing a mismatch when User defaults to never; change the type in Oidc.ts so getUser is conditional—e.g., only include getUser when [User] extends [never] is false—so consumers with User = never can't call it. Update the factories createGetUser and createMockOidc to require the corresponding user provider (createUser / mockedUser) in their input/signature whenever a non-never User is requested, ensuring the type-level requirement matches the runtime expectation.src/tools/createObjectThatThrowsIfAccessed.ts (1)
59-67:⚠️ Potential issue | 🟠 Major
Object.entries()is eagerly tripping the lazy getters.This helper is now used by
src/react-spa/createOidcSpaUtils.tswith getters fordecodedIdTokenandautoLogoutState. Enumeratingobjhere invokes those getters during construction, so those fields get marked as "used" even when consumers never touch them. Inspect descriptors instead and only rewrite data properties whose value isTHROW_IF_ACCESSED.Suggested fix
export function createObjectWithSomePropertiesThatThrowIfAccessed<T extends Record<string, unknown>>( obj: { [K in keyof T]: T[K] | typeof THROW_IF_ACCESSED }, debugMessage?: string ): T { + const descriptors = Object.getOwnPropertyDescriptors(obj); + return Object.defineProperties( obj, Object.fromEntries( - Object.entries(obj) - .filter(([, value]) => value === THROW_IF_ACCESSED) - .map(([key]) => { + Object.entries(descriptors) + .filter(([, descriptor]) => "value" in descriptor && descriptor.value === THROW_IF_ACCESSED) + .map(([key, descriptor]) => { const getAndSet = () => { throw new AccessError(`Cannot access ${key} yet ${debugMessage ?? ""}`); }; const pd = { get: getAndSet, set: getAndSet, - enumerable: true + enumerable: descriptor.enumerable }; return [key, pd]; }) ) ) as any; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tools/createObjectThatThrowsIfAccessed.ts` around lines 59 - 67, The function createObjectWithSomePropertiesThatThrowIfAccessed currently uses Object.entries(obj) which eagerly invokes getters on obj; instead, call Object.getOwnPropertyDescriptors(obj) and iterate descriptors, only targeting descriptors that are data properties with descriptor.value === THROW_IF_ACCESSED (do not touch accessor descriptors), and build a new descriptors map that replaces those data properties with accessor descriptors whose getter throws (including debugMessage) while preserving enumerable/configurable/writable semantics where appropriate; then call Object.defineProperties(obj, newDescriptors) so lazy getters are not executed.examples/tanstack-router-file-router/src/oidc.ts (1)
42-51:⚠️ Potential issue | 🟠 MajorKeep the access-token role parse best-effort.
isRealmAdminalready falls back tofalse, butDecodedAccessToken.parse(...)turns any unexpectedrealm_accessshape into a hard initialization failure.safeParse()preserves the existing fallback instead of breaking user creation.Suggested fix
const decodedAccessToken = !isKeycloak({ issuerUri }) ? undefined - : DecodedAccessToken.parse(decodeJwt(accessToken)); + : (() => { + const parsed = DecodedAccessToken.safeParse(decodeJwt(accessToken)); + return parsed.success ? parsed.data : undefined; + })();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/tanstack-router-file-router/src/oidc.ts` around lines 42 - 51, The code currently uses DecodedAccessToken.parse(decodeJwt(accessToken)) which will throw on unexpected shapes; change to use DecodedAccessToken.safeParse(decodeJwt(accessToken)) and set decodedAccessToken to the parsed data only when result.success is true (otherwise undefined) so the rest of the flow (e.g. isRealmAdmin fallback) remains a best-effort; update the expression where decodedAccessToken is assigned (referencing DecodedAccessToken, decodeJwt, isKeycloak, and decodedAccessToken) to use safeParse and conditional result handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/react-spa/createOidcSpaUtils.ts`:
- Around line 55-59: bootstrapOidc currently resolves readiness
(dOidcCoreOrInitializationError / bootstrapOidc_prResolved) before the initial
oidcCore.getUser()/createUser work finishes, allowing callers to proceed
prematurely; fold dResultOfGetUserOrInitializationErrorOrUndefined into the
bootstrap readiness path and mirror that wait in getOidc so both async and
imperative APIs wait for the first getUser/createUser result. Concretely: in
bootstrapOidc, await or chain dResultOfGetUserOrInitializationErrorOrUndefined
(the Deferred returned by the initial oidcCore.getUser() flow) before resolving
dOidcCoreOrInitializationError/bootstrapOidc_prResolved; and update getOidc to
await the same deferred (or the combined readiness promise) so getOidc does not
return until the first createUser/getUser has succeeded or failed. Ensure you
reference and use the existing Deferred instances
(dResultOfGetUserOrInitializationErrorOrUndefined,
dOidcCoreOrInitializationError, bootstrapOidc_prResolved) and the
oidcCore.getUser()/createUser call sites when making these changes.
---
Duplicate comments:
In `@examples/tanstack-router-file-router/src/oidc.ts`:
- Around line 42-51: The code currently uses
DecodedAccessToken.parse(decodeJwt(accessToken)) which will throw on unexpected
shapes; change to use DecodedAccessToken.safeParse(decodeJwt(accessToken)) and
set decodedAccessToken to the parsed data only when result.success is true
(otherwise undefined) so the rest of the flow (e.g. isRealmAdmin fallback)
remains a best-effort; update the expression where decodedAccessToken is
assigned (referencing DecodedAccessToken, decodeJwt, isKeycloak, and
decodedAccessToken) to use safeParse and conditional result handling.
In `@src/core/Oidc.ts`:
- Around line 40-43: The LoggedIn type currently always exposes getUser(),
causing a mismatch when User defaults to never; change the type in Oidc.ts so
getUser is conditional—e.g., only include getUser when [User] extends [never] is
false—so consumers with User = never can't call it. Update the factories
createGetUser and createMockOidc to require the corresponding user provider
(createUser / mockedUser) in their input/signature whenever a non-never User is
requested, ensuring the type-level requirement matches the runtime expectation.
In `@src/tools/createObjectThatThrowsIfAccessed.ts`:
- Around line 59-67: The function
createObjectWithSomePropertiesThatThrowIfAccessed currently uses
Object.entries(obj) which eagerly invokes getters on obj; instead, call
Object.getOwnPropertyDescriptors(obj) and iterate descriptors, only targeting
descriptors that are data properties with descriptor.value === THROW_IF_ACCESSED
(do not touch accessor descriptors), and build a new descriptors map that
replaces those data properties with accessor descriptors whose getter throws
(including debugMessage) while preserving enumerable/configurable/writable
semantics where appropriate; then call Object.defineProperties(obj,
newDescriptors) so lazy getters are not executed.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9e18b4c7-9ef5-4e99-9322-5e3247d22f8e
⛔ Files ignored due to path filters (1)
examples/tanstack-router-file-router/src/assets/avatarFallback.svgis excluded by!**/*.svg
📒 Files selected for processing (16)
examples/tanstack-router-file-router/src/components/Header.tsxexamples/tanstack-router-file-router/src/oidc.tsexamples/tanstack-router-file-router/src/routeTree.gen.tsexamples/tanstack-router-file-router/src/routes/admin-only.tsxexamples/tanstack-router-file-router/src/routes/protected.tsxsrc/angular.tssrc/core/Oidc.tssrc/core/createGetUser.tssrc/core/createMockOidc.tssrc/core/createOidc.tssrc/keycloak/index.tssrc/keycloak/keycloakUtils.tssrc/react-spa/createOidcSpaUtils.tssrc/react-spa/types.tssrc/react-spa/utilsBuilder.tssrc/tools/createObjectThatThrowsIfAccessed.ts
✅ Files skipped from review due to trivial changes (4)
- src/keycloak/index.ts
- examples/tanstack-router-file-router/src/routeTree.gen.ts
- src/core/createGetUser.ts
- src/react-spa/types.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- examples/tanstack-router-file-router/src/routes/admin-only.tsx
- src/angular.ts
- examples/tanstack-router-file-router/src/components/Header.tsx
- src/react-spa/utilsBuilder.ts
garronej
force-pushed
the
user-representation-3
branch
from
3e61d47 to
40a0767
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
src/react-spa/types.ts (1)
144-152:⚠️ Potential issue | 🟠 Major
getUser().refreshUser()regressed to a synchronous type.This signature is
() => voidhere, but the core contract and the current implementations returnPromise<User>. That breaks consumers that need to await the refreshed user value.♻️ Suggested fix
getUser: () => Promise<{ user: User; subscribeToUserChange: ( onUserChange: (params: { user: User; user_previous: User | undefined }) => void ) => { unsubscribeFromUserChange: () => void; }; - refreshUser: () => void; + refreshUser: () => Promise<User>; }>;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/react-spa/types.ts` around lines 144 - 152, The getUser return type incorrectly defines refreshUser as synchronous; update the getUser signature so refreshUser returns Promise<User> (i.e., change refreshUser: () => void to refreshUser: () => Promise<User>) to match the core contract and current implementations; ensure this change is applied inside the getUser return object type where getUser: () => Promise<{ user: User; subscribeToUserChange: (...) => { unsubscribeFromUserChange: () => void }; refreshUser: () => Promise<User>; }>.src/react-spa/createOidcSpaUtils.ts (1)
491-505:⚠️ Potential issue | 🟠 Major
bootstrapOidc()still reports readiness before the initial user load finishes.Both branches resolve the core deferred before these
getUser()blocks finish. Since bootstrap readiness is still chained only to that first deferred,await bootstrapOidc()andgetOidc()can proceed before the initial user value is available—or before acreateUser()failure is surfaced.♻️ Suggested fix
- bootstrapOidc_prResolved = dOidcCoreOrInitializationError.pr.then(() => {}); + bootstrapOidc_prResolved = (async () => { + const oidcCoreOrInitializationError = await dOidcCoreOrInitializationError.pr; + + if (!(oidcCoreOrInitializationError instanceof OidcInitializationError)) { + await dResultOfGetUserOrInitializationErrorOrUndefined.pr; + } + })();Have
getOidc()await the same combined readiness promise.Also applies to: 552-589
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/react-spa/createOidcSpaUtils.ts` around lines 491 - 505, bootstrapOidc currently resolves readiness before the initial user load/deferred (dResultOfGetUserOrInitializationErrorOrUndefined) finishes, so make bootstrapOidc and getOidc wait for the combined readiness that includes the getUser resolution: update the readiness promise used by bootstrapOidc (and the exported getOidc) to await both the original initialization deferred and dResultOfGetUserOrInitializationErrorOrUndefined (the block resolving undefined or await oidcCore.getUser()), or replace the current readiness resolution with a Promise.all/chain that includes dResultOfGetUserOrInitializationErrorOrUndefined so bootstrapOidc/getOidc do not return until oidcCore.getUser() or its error/undefined is settled.src/core/Oidc.ts (1)
3-6:⚠️ Potential issue | 🟠 MajorHide
getUser()on the no-user path.The default
Oidc.LoggedIn<DecodedIdToken>surface still advertisesgetUser(), but the current runtimes fail in that mode instead of supporting it (seesrc/core/createGetUser.ts, Line 143, andsrc/core/createMockOidc.ts, Line 235). Make this member conditional on a configuredUser, and mirror that in the SPA wrappers, so the public contract matches runtime.Also applies to: 40-43, 91-99
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/Oidc.ts` around lines 3 - 6, The Oidc public type currently exposes getUser() even when no User is configured; change the LoggedIn variant so getUser is only present when the generic User is not `never` (e.g., make Oidc.LoggedIn<DecodedIdToken, User> conditional: if User extends never then omit getUser, else include getUser(): Promise<User | null>), and update the corresponding exported types/aliases at the other declaration sites (the other LoggedIn/NotLoggedIn unions at the noted ranges) and the SPA wrapper typings to match this conditional shape; also ensure implementations referenced in createGetUser.ts and createMockOidc.ts remain compatible with the conditional signature.src/core/createGetUser.ts (2)
136-138:⚠️ Potential issue | 🔴 CriticalSkip the token-change listener when
createUseris missing.
createOidc()creates this helper even on the default no-user path. The first token rotation then calls__updatePrUserIfHashChanged(), hits the assert on Line 59, and turns a normal refresh into a runtime failure.🔒 Suggested fix
- evtTokensChange.subscribe(() => { - __updatePrUserIfHashChanged(); - }); + if (createUser !== undefined) { + evtTokensChange.subscribe(() => { + __updatePrUserIfHashChanged(); + }); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/createGetUser.ts` around lines 136 - 138, The token-change subscription should be skipped when there is no createUser handler to avoid calling __updatePrUserIfHashChanged and hitting the assertion; update the logic around evtTokensChange.subscribe to only register the callback when createUser (or the user-creation pathway from createOidc) is present/true, or alternatively make the callback a no-op when createUser is missing—locate evtTokensChange.subscribe and the __updatePrUserIfHashChanged function in createGetUser.ts and conditionally subscribe (or early-return in the handler) so the listener does not run on the default no-user path.
70-121:⚠️ Potential issue | 🟠 MajorA failed first
createUser()poisons the cache.On the initial load
user_currentisundefined, so the catch rethrows andstatekeeps the rejectedprUser_new. LatergetUser()/refreshUser()calls reuse or await that same rejected promise instead of retrying.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/createGetUser.ts` around lines 70 - 121, The issue is that a rejected prUser_new gets stored in state and poisons the cache; modify the async IIFE that defines prUser_new (referencing prUser_current, user_current, hash_new, hash_current and createUser) so that on a thrown error when user_current === undefined it clears or resets state.prUser (and state.hash if you changed it) before rethrowing; alternatively wrap the promise so rejection removes the stored prUser_new only if state.hash === hash_new to avoid races, ensuring that only successful user creations leave prUser_new in state.
🧹 Nitpick comments (1)
examples/tanstack-router-file-router/src/routes/protected.tsx (1)
43-45: RenderUsernameunconditionally.
user.usernameis now a required field in the localUsermodel, so this branch is just leftover defensive code after the migration to the user abstraction.Based on learnings, fields guaranteed at the auth boundary for a logged-in user do not need defensive checks.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/tanstack-router-file-router/src/routes/protected.tsx` around lines 43 - 45, The conditional rendering around user.username is unnecessary because username is guaranteed on the authenticated User model; remove the conditional wrapper and render <InfoRow label="Username">{user.username}</InfoRow> unconditionally in the protected.tsx component (locate the JSX using InfoRow and the user.username expression) so the Username row is always displayed for logged-in users.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/keycloak/keycloakUtils.ts`:
- Around line 15-19: The KeycloakProfile type currently requires attributes and
userProfileMetadata but the upstream Keycloak account JSON can omit or null
them; update the KeycloakProfile definition to make attributes and
userProfileMetadata optional (e.g., attributes?: Record<string, unknown>;
userProfileMetadata?: { attributes: KeycloakProfile.Attribute[]; groups:
KeycloakProfile.Group[] } | null) and adjust fetchUserProfile to handle
missing/null values safely when parsing the account endpoint response (check for
undefined/null before accessing keys or iterate). Ensure references to
KeycloakProfile, attributes, userProfileMetadata, and fetchUserProfile are
updated so callers handle the optional fields.
---
Duplicate comments:
In `@src/core/createGetUser.ts`:
- Around line 136-138: The token-change subscription should be skipped when
there is no createUser handler to avoid calling __updatePrUserIfHashChanged and
hitting the assertion; update the logic around evtTokensChange.subscribe to only
register the callback when createUser (or the user-creation pathway from
createOidc) is present/true, or alternatively make the callback a no-op when
createUser is missing—locate evtTokensChange.subscribe and the
__updatePrUserIfHashChanged function in createGetUser.ts and conditionally
subscribe (or early-return in the handler) so the listener does not run on the
default no-user path.
- Around line 70-121: The issue is that a rejected prUser_new gets stored in
state and poisons the cache; modify the async IIFE that defines prUser_new
(referencing prUser_current, user_current, hash_new, hash_current and
createUser) so that on a thrown error when user_current === undefined it clears
or resets state.prUser (and state.hash if you changed it) before rethrowing;
alternatively wrap the promise so rejection removes the stored prUser_new only
if state.hash === hash_new to avoid races, ensuring that only successful user
creations leave prUser_new in state.
In `@src/core/Oidc.ts`:
- Around line 3-6: The Oidc public type currently exposes getUser() even when no
User is configured; change the LoggedIn variant so getUser is only present when
the generic User is not `never` (e.g., make Oidc.LoggedIn<DecodedIdToken, User>
conditional: if User extends never then omit getUser, else include getUser():
Promise<User | null>), and update the corresponding exported types/aliases at
the other declaration sites (the other LoggedIn/NotLoggedIn unions at the noted
ranges) and the SPA wrapper typings to match this conditional shape; also ensure
implementations referenced in createGetUser.ts and createMockOidc.ts remain
compatible with the conditional signature.
In `@src/react-spa/createOidcSpaUtils.ts`:
- Around line 491-505: bootstrapOidc currently resolves readiness before the
initial user load/deferred (dResultOfGetUserOrInitializationErrorOrUndefined)
finishes, so make bootstrapOidc and getOidc wait for the combined readiness that
includes the getUser resolution: update the readiness promise used by
bootstrapOidc (and the exported getOidc) to await both the original
initialization deferred and dResultOfGetUserOrInitializationErrorOrUndefined
(the block resolving undefined or await oidcCore.getUser()), or replace the
current readiness resolution with a Promise.all/chain that includes
dResultOfGetUserOrInitializationErrorOrUndefined so bootstrapOidc/getOidc do not
return until oidcCore.getUser() or its error/undefined is settled.
In `@src/react-spa/types.ts`:
- Around line 144-152: The getUser return type incorrectly defines refreshUser
as synchronous; update the getUser signature so refreshUser returns
Promise<User> (i.e., change refreshUser: () => void to refreshUser: () =>
Promise<User>) to match the core contract and current implementations; ensure
this change is applied inside the getUser return object type where getUser: ()
=> Promise<{ user: User; subscribeToUserChange: (...) => {
unsubscribeFromUserChange: () => void }; refreshUser: () => Promise<User>; }>.
---
Nitpick comments:
In `@examples/tanstack-router-file-router/src/routes/protected.tsx`:
- Around line 43-45: The conditional rendering around user.username is
unnecessary because username is guaranteed on the authenticated User model;
remove the conditional wrapper and render <InfoRow
label="Username">{user.username}</InfoRow> unconditionally in the protected.tsx
component (locate the JSX using InfoRow and the user.username expression) so the
Username row is always displayed for logged-in users.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 79b7de2b-1d82-4ed6-9cc6-98c364d73976
⛔ Files ignored due to path filters (1)
examples/tanstack-router-file-router/src/assets/avatarFallback.svgis excluded by!**/*.svg
📒 Files selected for processing (16)
examples/tanstack-router-file-router/src/components/Header.tsxexamples/tanstack-router-file-router/src/oidc.tsexamples/tanstack-router-file-router/src/routeTree.gen.tsexamples/tanstack-router-file-router/src/routes/admin-only.tsxexamples/tanstack-router-file-router/src/routes/protected.tsxsrc/angular.tssrc/core/Oidc.tssrc/core/createGetUser.tssrc/core/createMockOidc.tssrc/core/createOidc.tssrc/keycloak/index.tssrc/keycloak/keycloakUtils.tssrc/react-spa/createOidcSpaUtils.tssrc/react-spa/types.tssrc/react-spa/utilsBuilder.tssrc/tools/createObjectThatThrowsIfAccessed.ts
✅ Files skipped from review due to trivial changes (3)
- examples/tanstack-router-file-router/src/routes/admin-only.tsx
- src/keycloak/index.ts
- examples/tanstack-router-file-router/src/routeTree.gen.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- src/angular.ts
- examples/tanstack-router-file-router/src/oidc.ts
- src/tools/createObjectThatThrowsIfAccessed.ts
- examples/tanstack-router-file-router/src/components/Header.tsx
Comment on lines
+15
to
+19
| attributes: Record<string, unknown>; | ||
| userProfileMetadata: { | ||
| attributes: KeycloakProfile.Attribute[]; | ||
| groups: KeycloakProfile.Group[]; | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python - <<'PY'
import urllib.request
files = {
"DefaultUserProfile.java": "https://raw.githubusercontent.com/keycloak/keycloak/main/server-spi-private/src/main/java/org/keycloak/userprofile/DefaultUserProfile.java",
"UserProfileUtil.java": "https://raw.githubusercontent.com/keycloak/keycloak/main/server-spi-private/src/main/java/org/keycloak/userprofile/UserProfileUtil.java",
"AccountRestService.java": "https://raw.githubusercontent.com/keycloak/keycloak/main/services/src/main/java/org/keycloak/services/resources/account/AccountRestService.java",
}
texts = {name: urllib.request.urlopen(url).read().decode() for name, url in files.items()}
checks = {
"DefaultUserProfile nulls empty attributes": "rep.setAttributes(attributesRep.isEmpty() ? null : attributesRep);" in texts["DefaultUserProfile.java"],
"DefaultUserProfile sets userProfileMetadata from helper": "rep.setUserProfileMetadata(createUserProfileMetadata(session, this));" in texts["DefaultUserProfile.java"],
"UserProfileUtil can return null metadata": "if(am == null) return null;" in texts["UserProfileUtil.java"],
"AccountRestService can clear metadata": "if (userProfileMetadata != null && !userProfileMetadata) { rep.setUserProfileMetadata(null); }" in texts["AccountRestService.java"],
}
for label, ok in checks.items():
print(f"{label}: {'OK' if ok else 'MISSING'}")
PYRepository: keycloakify/oidc-spa
Length of output: 2685
🏁 Script executed:
# First, let's check the current state of the file in question
cat -n src/keycloak/keycloakUtils.ts | head -30Repository: keycloakify/oidc-spa
Length of output: 1422
🏁 Script executed:
# Look for the fetchUserProfile function and how it uses KeycloakProfile
rg -n "fetchUserProfile" --type typescript -A 10 -B 2Repository: keycloakify/oidc-spa
Length of output: 98
🏁 Script executed:
# Check for any type guards or null checks related to attributes or userProfileMetadata
rg -n "userProfileMetadata|attributes" src/keycloak/ --type typescript -B 2 -A 2Repository: keycloakify/oidc-spa
Length of output: 98
🏁 Script executed:
# Let's also check if there are any tests that might reveal the expected behavior
fd -e test.ts -e spec.ts -e test.js src/keycloak/ | head -20Repository: keycloakify/oidc-spa
Length of output: 421
🏁 Script executed:
# Search for fetchUserProfile implementation with correct syntax
rg -n "fetchUserProfile" src/keycloak/ -A 15Repository: keycloakify/oidc-spa
Length of output: 3526
🏁 Script executed:
# Check the full KeycloakProfile type definition
cat -n src/keycloak/keycloakUtils.ts | head -80Repository: keycloakify/oidc-spa
Length of output: 3420
🏁 Script executed:
# Look for actual implementation of fetchUserProfile
fd . src/keycloak/ | head -20Repository: keycloakify/oidc-spa
Length of output: 356
🏁 Script executed:
# Search for where KeycloakProfile is used/consumed
rg -n "KeycloakProfile" src/ -A 2 -B 2 --max-count 20Repository: keycloakify/oidc-spa
Length of output: 3409
🏁 Script executed:
# Search for tests or mock data related to KeycloakProfile
find . -type f \( -name "*.test.ts" -o -name "*.spec.ts" -o -name "*.test.js" -o -name "*.spec.js" \) | xargs grep -l "KeycloakProfile\|fetchUserProfile" 2>/dev/null || echo "No test files found"Repository: keycloakify/oidc-spa
Length of output: 83
🏁 Script executed:
# Look for any mock data or fixtures
find . -type f -name "*mock*" -o -name "*fixture*" | head -20Repository: keycloakify/oidc-spa
Length of output: 150
🏁 Script executed:
# Check if there are any comments in the code about nullable fields
grep -r "null\|undefined" src/keycloak/keycloakUtils.ts -B 2 -A 2Repository: keycloakify/oidc-spa
Length of output: 752
🏁 Script executed:
# Look at the types.ts file to see the alternate KeycloakProfile definition
sed -n '420,500p' src/keycloak/keycloak-js/types.tsRepository: keycloakify/oidc-spa
Length of output: 1026
🏁 Script executed:
# Check what KeycloakProfile is exported/imported from keycloakUtils.ts
grep -n "KeycloakProfile" src/keycloak/keycloakUtils.ts | head -15Repository: keycloakify/oidc-spa
Length of output: 356
🏁 Script executed:
# Check what KeycloakProfile is exported from types.ts
grep -n "KeycloakProfile" src/keycloak/keycloak-js/types.ts | head -10Repository: keycloakify/oidc-spa
Length of output: 102
🏁 Script executed:
# See if types.ts KeycloakProfile is actually used
rg "from.*types.*KeycloakProfile|import.*KeycloakProfile.*types" src/ --no-headingRepository: keycloakify/oidc-spa
Length of output: 46
🏁 Script executed:
# Check index.ts to see which KeycloakProfile is publicly exported
cat src/keycloak/index.tsRepository: keycloakify/oidc-spa
Length of output: 318
🏁 Script executed:
# Check if there's any actual response transformation or validation in the broader context
cat -n src/keycloak/keycloakUtils.ts | sed -n '220,245p'Repository: keycloakify/oidc-spa
Length of output: 1254
🏁 Script executed:
# Look for any error handling or fallbacks around fetchUserProfile usage
rg -n "fetchUserProfile\|catch\|error" src/keycloak/keycloak-js/Keycloak.ts -B 2 -A 3 | head -40Repository: keycloakify/oidc-spa
Length of output: 46
Make attributes and userProfileMetadata optional in KeycloakProfile.
fetchUserProfile at lines 225-231 directly parses external JSON from the Keycloak account endpoint without validation. Both attributes and userProfileMetadata should be optional since the upstream Keycloak API can omit them: attributes is nulled when empty, userProfileMetadata can be null from the helper function, and the account endpoint can explicitly clear it.
Proposed fix
export type KeycloakProfile = KeycloakProfile.AttributeValuesMap & {
- attributes: Record<string, unknown>;
- userProfileMetadata: {
+ attributes?: Record<string, unknown> | null;
+ userProfileMetadata?: {
attributes: KeycloakProfile.Attribute[];
groups: KeycloakProfile.Group[];
- };
+ } | null;
};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/keycloak/keycloakUtils.ts` around lines 15 - 19, The KeycloakProfile type
currently requires attributes and userProfileMetadata but the upstream Keycloak
account JSON can omit or null them; update the KeycloakProfile definition to
make attributes and userProfileMetadata optional (e.g., attributes?:
Record<string, unknown>; userProfileMetadata?: { attributes:
KeycloakProfile.Attribute[]; groups: KeycloakProfile.Group[] } | null) and
adjust fetchUserProfile to handle missing/null values safely when parsing the
account endpoint response (check for undefined/null before accessing keys or
iterate). Ensure references to KeycloakProfile, attributes, userProfileMetadata,
and fetchUserProfile are updated so callers handle the optional fields.
40a0767 to
58ac11f
Compare
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)
src/keycloak/keycloakUtils.ts (1)
225-238:⚠️ Potential issue | 🟠 MajorAdd HTTP status validation before parsing JSON.
Both
fetchUserProfileandfetchUserInfocall.json()without checking if the response is OK. Non-2xx responses (e.g., 401 for expired tokens, 500 for server errors) will either fail JSON parsing or return an error object that doesn't match the expected type, causing confusing runtime errors.🛠️ Proposed fix
fetchUserProfile: ({ accessToken }) => fetch(`${realmUrl}/account`, { headers: { Accept: "application/json", Authorization: `Bearer ${accessToken}` } - }).then(r => r.json()), + }).then(r => { + if (!r.ok) { + throw new Error(`Failed to fetch user profile: ${r.status} ${r.statusText}`); + } + return r.json(); + }), fetchUserInfo: ({ accessToken }) => fetch(`${realmUrl}/protocol/openid-connect/userinfo`, { headers: { Accept: "application/json", Authorization: `Bearer ${accessToken}` } - }).then(r => r.json()), + }).then(r => { + if (!r.ok) { + throw new Error(`Failed to fetch user info: ${r.status} ${r.statusText}`); + } + return r.json(); + }),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/keycloak/keycloakUtils.ts` around lines 225 - 238, Update fetchUserProfile and fetchUserInfo to validate the HTTP response before calling .json(): after calling fetch with realmUrl, check response.ok (or response.status) and if not OK throw a meaningful error including the status and response body (or statusText) so callers don't attempt to parse or use unexpected payloads; ensure the functions still return the parsed JSON on success. Reference the existing fetchUserProfile and fetchUserInfo functions and the realmUrl variable when making the change.src/react-spa/createOidcSpaUtils.ts (1)
1-1:⚠️ Potential issue | 🟡 MinorFix Prettier formatting to pass CI.
The pipeline indicates this file differs from Prettier's expected output. Run
npx prettier --write src/react-spa/createOidcSpaUtils.tsto resolve the formatting check failure.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/react-spa/createOidcSpaUtils.ts` at line 1, The file createOidcSpaUtils.ts fails Prettier formatting; run a Prettier pass (npx prettier --write src/react-spa/createOidcSpaUtils.ts) or apply the project's Prettier rules to the file so imports and overall formatting match CI expectations; ensure the exported symbol createOidcSpaUtils (and any related functions or imports listed at the top of the file) are left unchanged functionally but reformatted to satisfy the linter/CI.
♻️ Duplicate comments (1)
src/core/createGetUser.ts (1)
35-41:⚠️ Potential issue | 🟡 MinorMissing HTTP error handling for
fetchUserInfo.The
fetchUserInfofunction doesn't checkr.okbefore parsing JSON. If the userinfo endpoint returns a non-2xx status,r.json()will either fail with unclear errors or return an error body that gets treated as valid user info.🛡️ Proposed fix to add error handling
const r = await fetch(userinfo_endpoint, { headers: { Authorization: `Bearer ${accessToken}` } }); + if (!r.ok) { + throw new Error( + `oidc-spa: Failed to fetch userinfo: ${r.status} ${r.statusText}` + ); + } + return r.json();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/createGetUser.ts` around lines 35 - 41, The fetchUserInfo flow in createGetUser.ts doesn't check the HTTP response before parsing JSON; update the fetchUserInfo call that currently does const r = await fetch(userinfo_endpoint...) / return r.json() to validate the response (check r.ok) and on non-ok throw a descriptive error that includes the HTTP status and response body (use await r.text() or attempt r.json() safely) so callers don't treat error payloads as valid user info; keep the error thrown from the same function (fetchUserInfo) so upstream callers (e.g., getUser) can handle it.
🧹 Nitpick comments (1)
src/keycloak/keycloakUtils.ts (1)
129-133: Consider makingdisplayHeaderanddisplayDescriptionoptional inGroup.The inline
Attribute.grouptype (lines 41-47) definesdisplayHeader?anddisplayDescription?as optional, but the standaloneGrouptype requires them. If these represent similar data from the Keycloak API, the inconsistency could cause type errors when the API omits these fields.♻️ Suggested alignment
export type Group = { name: string; - displayHeader: string; - displayDescription: string; + displayHeader?: string; + displayDescription?: string; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/keycloak/keycloakUtils.ts` around lines 129 - 133, The standalone type Group requires displayHeader and displayDescription but the inline Attribute.group defines them as optional; update the Group type declaration (export type Group) to make displayHeader? and displayDescription? optional so it matches Attribute.group and avoids runtime/API omissions; ensure any code consuming Group handles undefined values or provides sensible defaults where needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@examples/tanstack-router-file-router/src/routeTree.gen.ts`:
- Around line 11-95: The generated file
examples/tanstack-router-file-router/src/routeTree.gen.ts is failing Prettier
--list-different; either update the generator that produces routeTree (the code
that writes the routeTree.gen.ts output) so its template/printing matches the
repo's Prettier rules (quote style, spacing, trailing commas, semicolons, etc.)
when emitting symbols like rootRouteImport, ProtectedRoute, AdminOnlyRoute,
IndexRoute and the final routeTree._addFileChildren(...)._addFileTypes(...)
call, or have the generator prepend the agreed-upon exclusion header (e.g. the
project’s standard prettier-ignore-file comment used elsewhere) to
routeTree.gen.ts so CI skips it; pick one approach, implement it in the
generator that writes this file, and regenerate routeTree.gen.ts so CI passes.
In `@src/core/createGetUser.ts`:
- Line 1: Run Prettier on src/core/createGetUser.ts (or run the project's
formatting script) to apply the project's formatting rules; specifically
reformat the import line referencing Oidc (import type { Oidc } from "./Oidc";)
so it matches Prettier's expected output and then stage the formatted file
before committing so CI passes.
- Around line 108-113: The code always invokes subscribers via
onUserChanges.forEach after createUser even when no change occurred; update the
createUser return/notification logic (in the createGetUser implementation) to
compare user_new and user_current and only call onUserChanges.forEach(...) when
they differ (use a strict equality or a deep-equality helper as appropriate for
your user shape), i.e., check the result of createUser and skip the notification
if user_new === user_current (or deepEqual(user_new, user_current)) to avoid
unnecessary subscriber calls and re-renders.
- Around line 136-138: The evtTokensChange subscription currently calls
__updatePrUserIfHashChanged() unconditionally which asserts createUser !==
undefined; modify the code so the subscription is only registered or the
callback is no-op when createUser is undefined: either wrap the subscription
setup in a conditional check for createUser (e.g., if (createUser) {
evtTokensChange.subscribe(...) }) or change the callback to early-return if
createUser is undefined (e.g., evtTokensChange.subscribe(() => { if
(!createUser) return; __updatePrUserIfHashChanged(); })). Ensure you reference
evtTokensChange, __updatePrUserIfHashChanged, createUser and getUser accordingly
so token-rotation before getUser() no longer throws error 94302.
---
Outside diff comments:
In `@src/keycloak/keycloakUtils.ts`:
- Around line 225-238: Update fetchUserProfile and fetchUserInfo to validate the
HTTP response before calling .json(): after calling fetch with realmUrl, check
response.ok (or response.status) and if not OK throw a meaningful error
including the status and response body (or statusText) so callers don't attempt
to parse or use unexpected payloads; ensure the functions still return the
parsed JSON on success. Reference the existing fetchUserProfile and
fetchUserInfo functions and the realmUrl variable when making the change.
In `@src/react-spa/createOidcSpaUtils.ts`:
- Line 1: The file createOidcSpaUtils.ts fails Prettier formatting; run a
Prettier pass (npx prettier --write src/react-spa/createOidcSpaUtils.ts) or
apply the project's Prettier rules to the file so imports and overall formatting
match CI expectations; ensure the exported symbol createOidcSpaUtils (and any
related functions or imports listed at the top of the file) are left unchanged
functionally but reformatted to satisfy the linter/CI.
---
Duplicate comments:
In `@src/core/createGetUser.ts`:
- Around line 35-41: The fetchUserInfo flow in createGetUser.ts doesn't check
the HTTP response before parsing JSON; update the fetchUserInfo call that
currently does const r = await fetch(userinfo_endpoint...) / return r.json() to
validate the response (check r.ok) and on non-ok throw a descriptive error that
includes the HTTP status and response body (use await r.text() or attempt
r.json() safely) so callers don't treat error payloads as valid user info; keep
the error thrown from the same function (fetchUserInfo) so upstream callers
(e.g., getUser) can handle it.
---
Nitpick comments:
In `@src/keycloak/keycloakUtils.ts`:
- Around line 129-133: The standalone type Group requires displayHeader and
displayDescription but the inline Attribute.group defines them as optional;
update the Group type declaration (export type Group) to make displayHeader? and
displayDescription? optional so it matches Attribute.group and avoids
runtime/API omissions; ensure any code consuming Group handles undefined values
or provides sensible defaults where needed.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 193cb40b-f7c3-4517-932f-368523d969a8
⛔ Files ignored due to path filters (1)
examples/tanstack-router-file-router/src/assets/avatarFallback.svgis excluded by!**/*.svg
📒 Files selected for processing (16)
examples/tanstack-router-file-router/src/components/Header.tsxexamples/tanstack-router-file-router/src/oidc.tsexamples/tanstack-router-file-router/src/routeTree.gen.tsexamples/tanstack-router-file-router/src/routes/admin-only.tsxexamples/tanstack-router-file-router/src/routes/protected.tsxsrc/angular.tssrc/core/Oidc.tssrc/core/createGetUser.tssrc/core/createMockOidc.tssrc/core/createOidc.tssrc/keycloak/index.tssrc/keycloak/keycloakUtils.tssrc/react-spa/createOidcSpaUtils.tssrc/react-spa/types.tssrc/react-spa/utilsBuilder.tssrc/tools/createObjectThatThrowsIfAccessed.ts
✅ Files skipped from review due to trivial changes (2)
- src/keycloak/index.ts
- examples/tanstack-router-file-router/src/oidc.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- examples/tanstack-router-file-router/src/routes/admin-only.tsx
- examples/tanstack-router-file-router/src/components/Header.tsx
- examples/tanstack-router-file-router/src/routes/protected.tsx
- src/angular.ts
- src/tools/createObjectThatThrowsIfAccessed.ts
- src/react-spa/types.ts
| @@ -0,0 +1,235 @@ | |||
| import type { Oidc } from "./Oidc"; | |||
There was a problem hiding this comment.
Fix Prettier formatting to pass CI.
The pipeline indicates this file differs from Prettier's expected output.
🧰 Tools
🪛 GitHub Actions: ci
[error] 1-1: Prettier formatting check failed (--list-different). File differs from Prettier output.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/core/createGetUser.ts` at line 1, Run Prettier on
src/core/createGetUser.ts (or run the project's formatting script) to apply the
project's formatting rules; specifically reformat the import line referencing
Oidc (import type { Oidc } from "./Oidc";) so it matches Prettier's expected
output and then stage the formatted file before committing so CI passes.
Comment on lines
+108
to
+113
| onUserChanges.forEach(onUserChange => | ||
| onUserChange({ | ||
| user: user_new, | ||
| user_previous: user_current | ||
| }) | ||
| ); |
There was a problem hiding this comment.
Subscribers notified even when user hasn't changed.
onUserChanges.forEach is called unconditionally after createUser returns, even if user_new is identical to user_current. This can cause unnecessary re-renders in consumers.
🔧 Proposed fix to skip notification when user is unchanged
- onUserChanges.forEach(onUserChange =>
- onUserChange({
- user: user_new,
- user_previous: user_current
- })
- );
+ if (user_new !== user_current) {
+ onUserChanges.forEach(onUserChange =>
+ onUserChange({
+ user: user_new,
+ user_previous: user_current
+ })
+ );
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| onUserChanges.forEach(onUserChange => | |
| onUserChange({ | |
| user: user_new, | |
| user_previous: user_current | |
| }) | |
| ); | |
| if (user_new !== user_current) { | |
| onUserChanges.forEach(onUserChange => | |
| onUserChange({ | |
| user: user_new, | |
| user_previous: user_current | |
| }) | |
| ); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/core/createGetUser.ts` around lines 108 - 113, The code always invokes
subscribers via onUserChanges.forEach after createUser even when no change
occurred; update the createUser return/notification logic (in the createGetUser
implementation) to compare user_new and user_current and only call
onUserChanges.forEach(...) when they differ (use a strict equality or a
deep-equality helper as appropriate for your user shape), i.e., check the result
of createUser and skip the notification if user_new === user_current (or
deepEqual(user_new, user_current)) to avoid unnecessary subscriber calls and
re-renders.
Comment on lines
+136
to
+138
| evtTokensChange.subscribe(() => { | ||
| __updatePrUserIfHashChanged(); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if createGetUser is called conditionally or if the subscription is guarded
rg -n -A5 -B5 'createGetUser' src/core/createOidc.ts
# Check if there's a guard around evtTokensChange.subscribe
rg -n -B3 -A3 'evtTokensChange.subscribe' src/core/createGetUser.tsRepository: keycloakify/oidc-spa
Length of output: 1010
🏁 Script executed:
cat -n src/core/createGetUser.tsRepository: keycloakify/oidc-spa
Length of output: 8720
🏁 Script executed:
rg -n -B10 -A5 'evtTokensChange.subscribe' src/core/createGetUser.ts | head -40Repository: keycloakify/oidc-spa
Length of output: 390
🏁 Script executed:
# Also check for any guards around __updatePrUserIfHashChanged calls
rg -n -B2 '__updatePrUserIfHashChanged' src/core/createGetUser.tsRepository: keycloakify/oidc-spa
Length of output: 312
🏁 Script executed:
# Check if createGetUser is conditionally called in createOidc.ts
rg -n -B10 -A5 'createGetUser\(' src/core/createOidc.ts | head -50Repository: keycloakify/oidc-spa
Length of output: 591
🏁 Script executed:
rg -n -B20 'const { getUser } = createGetUser' src/core/createOidc.ts | head -60Repository: keycloakify/oidc-spa
Length of output: 187
🏁 Script executed:
rg -n -B20 'createGetUser' src/core/createOidc.ts | grep -A20 'const.*getUser'Repository: keycloakify/oidc-spa
Length of output: 108
🏁 Script executed:
# Better approach - look at lines around 1755
sed -n '1735,1780p' src/core/createOidc.tsRepository: keycloakify/oidc-spa
Length of output: 1313
Guard the token change subscription when createUser is undefined.
The evtTokensChange subscription (lines 136-138) unconditionally invokes __updatePrUserIfHashChanged(), which asserts createUser !== undefined (line 59). If token rotation occurs before getUser() is called on an instance without createUser, the subscription callback will throw error "94302".
Add a guard to either skip the subscription callback or prevent subscription setup when createUser is undefined.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/core/createGetUser.ts` around lines 136 - 138, The evtTokensChange
subscription currently calls __updatePrUserIfHashChanged() unconditionally which
asserts createUser !== undefined; modify the code so the subscription is only
registered or the callback is no-op when createUser is undefined: either wrap
the subscription setup in a conditional check for createUser (e.g., if
(createUser) { evtTokensChange.subscribe(...) }) or change the callback to
early-return if createUser is undefined (e.g., evtTokensChange.subscribe(() => {
if (!createUser) return; __updatePrUserIfHashChanged(); })). Ensure you
reference evtTokensChange, __updatePrUserIfHashChanged, createUser and getUser
accordingly so token-rotation before getUser() no longer throws error 94302.
garronej
force-pushed
the
user-representation-3
branch
from
58ac11f to
2b330db
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/core/createGetUser.ts (1)
155-183: Cycle detection may false-positive on concurrent calls.The
callCount_getUsercounter warns when it changes during the firstgetUser()call. However, if two components legitimately callgetUser()simultaneously at startup (before the first resolves), the counter changes and triggers a spurious deadlock warning.Since this is a diagnostic console.warn rather than an error, and concurrent initial calls are uncommon, this is minor. Consider documenting this behavior or checking if the re-entrant call originated from within the
createUserstack.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/createGetUser.ts` around lines 155 - 183, The cycle-detection warn can false-positive for concurrent external getUser() calls; add a boolean flag (e.g., isInCreateUser) that createUser sets true on entry and false on exit, then change the cycle_detection logic (which currently uses callCount_getUser, timer_cycleDetection, setTimer) to only emit the console.warn when the re-entrant increment is observed AND isInCreateUser is true (i.e., the extra call originated from inside createUser), so legitimate concurrent callers won’t trigger the deadlock warning.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/react-spa/types.ts`:
- Around line 381-392: This file fails Prettier formatting; run your project's
Prettier formatter (or npm/yarn script like format) on src/react-spa/types.ts
and commit the changes so the CreateUser type declaration and the trailing
assert<Equals<CreateUser<{ _brand: string }>, ParamsOfCreateOidc.CreateUser<{
_brand: string }>>> line are consistently formatted; ensure no lint/prettier
errors remain before pushing.
---
Nitpick comments:
In `@src/core/createGetUser.ts`:
- Around line 155-183: The cycle-detection warn can false-positive for
concurrent external getUser() calls; add a boolean flag (e.g., isInCreateUser)
that createUser sets true on entry and false on exit, then change the
cycle_detection logic (which currently uses callCount_getUser,
timer_cycleDetection, setTimer) to only emit the console.warn when the
re-entrant increment is observed AND isInCreateUser is true (i.e., the extra
call originated from inside createUser), so legitimate concurrent callers won’t
trigger the deadlock warning.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3fe375f9-9df1-4141-bfa3-b8b213e58ae8
⛔ Files ignored due to path filters (1)
examples/tanstack-router-file-router/src/assets/avatarFallback.svgis excluded by!**/*.svg
📒 Files selected for processing (16)
examples/tanstack-router-file-router/src/components/Header.tsxexamples/tanstack-router-file-router/src/oidc.tsexamples/tanstack-router-file-router/src/routeTree.gen.tsexamples/tanstack-router-file-router/src/routes/admin-only.tsxexamples/tanstack-router-file-router/src/routes/protected.tsxsrc/angular.tssrc/core/Oidc.tssrc/core/createGetUser.tssrc/core/createMockOidc.tssrc/core/createOidc.tssrc/keycloak/index.tssrc/keycloak/keycloakUtils.tssrc/react-spa/createOidcSpaUtils.tssrc/react-spa/types.tssrc/react-spa/utilsBuilder.tssrc/tools/createObjectThatThrowsIfAccessed.ts
✅ Files skipped from review due to trivial changes (2)
- src/keycloak/index.ts
- examples/tanstack-router-file-router/src/routeTree.gen.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- src/angular.ts
- examples/tanstack-router-file-router/src/components/Header.tsx
- examples/tanstack-router-file-router/src/routes/protected.tsx
- src/react-spa/utilsBuilder.ts
- src/keycloak/keycloakUtils.ts
Comment on lines
+381
to
+392
| export type CreateUser<User> = (params: { | ||
| decodedIdToken: Oidc_core.Tokens.DecodedIdToken_OidcCoreSpec; | ||
| accessToken: string; | ||
| fetchUserInfo: () => Promise<{ | ||
| [key: string]: unknown; | ||
| sub: string; | ||
| }>; | ||
| issuerUri: string; | ||
| user_current: User | undefined; | ||
| }) => MaybeAsync<User>; | ||
|
|
||
| assert<Equals<CreateUser<{ _brand: string }>, ParamsOfCreateOidc.CreateUser<{ _brand: string }>>>; No newline at end of file |
There was a problem hiding this comment.
Pipeline failure: File needs Prettier formatting.
The CI reports this file isn't formatted according to Prettier rules. Run the formatter before merging.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/react-spa/types.ts` around lines 381 - 392, This file fails Prettier
formatting; run your project's Prettier formatter (or npm/yarn script like
format) on src/react-spa/types.ts and commit the changes so the CreateUser type
declaration and the trailing assert<Equals<CreateUser<{ _brand: string }>,
ParamsOfCreateOidc.CreateUser<{ _brand: string }>>> line are consistently
formatted; ensure no lint/prettier errors remain before pushing.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes / UX
Refactor
Style
Docs/Types