BA-3147-fe-auth-refactor-phase-1-allauth-strategy-foundation-and-sess…#352
Conversation
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
lucas-caribe
force-pushed
the
feature/BA-3147-fe-auth-refactor-phase-1-allauth-strategy-foundation-and-session-architecture
branch
from
ecb0753 to
804dbec
Compare
lucas-caribe
requested review from
anicioalexandre and
priscilladeroode
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/authentication/modules/user/useUpdateUser/__tests__/useUpdateUser.test.tsx (1)
43-47:⚠️ Potential issue | 🟡 MinorAlign mocked response status with the test’s 401 scenario.
Line 30says this validates the 401 onSettled behavior, butLine 45mocks a201response. That can miss the unauthorized branch and produce a false positive.Suggested fix
mockFetch('/users/1', { method: 'PATCH', - status: 201, + status: 401, response: request, })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/authentication/modules/user/useUpdateUser/__tests__/useUpdateUser.test.tsx` around lines 43 - 47, The mocked PATCH response in the useUpdateUser test currently returns status: 201 which contradicts the test intent to validate the 401 onSettled branch; update the mockFetch call used in useUpdateUser.test.tsx (the mockFetch('/users/1', { method: 'PATCH', ... }) invocation) to return status: 401 (and an appropriate unauthorized response body if needed) so the test exercises the unauthorized branch and onSettled behavior for 401.packages/utils/functions/axios/createAxiosInstance/__tests__/createAxiosInstance.test.ts (1)
406-432:⚠️ Potential issue | 🟡 MinorTest name may be misleading.
The test is named "should not use getTokenSSR when no auth is required in SSR", but line 430 asserts
expect(getTokenSSRMock).toHaveBeenCalled(). This appears to verify thatgetTokenSSRis called (to check for a token), but theAuthorizationheader is not set for public endpoints. Consider renaming the test to clarify the actual behavior being tested, e.g., "should not set Authorization header for public endpoints in SSR even if token exists".Proposed test name fix
- it('should not use getTokenSSR when no auth is required in SSR', async () => { + it('should not set Authorization header for public endpoints in SSR', async () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/utils/functions/axios/createAxiosInstance/__tests__/createAxiosInstance.test.ts` around lines 406 - 432, The test name is misleading: it asserts getTokenSSR is called but Authorization remains unset for public endpoints; rename the test to reflect that behavior (for example, "should not set Authorization header for public endpoints in SSR even if token exists") so it matches the assertions. Locate the test using createAxiosInstance and the mocked getTokenSSR (getTokenSSRMock) and update the it(...) description accordingly without changing the test logic that invokes interceptorFn and checks request.headers.Authorization remains undefined when servicesWithoutToken includes /\/public-endpoint/.packages/authentication/modules/access/allauth/useAllAuthGoogleLoginHandler/index.ts (1)
30-54:⚠️ Potential issue | 🟡 MinorNon-success auth results (MFA, redirect) trigger a misleading error message.
If
mapLoginResponsereturnsmfa_requiredorredirect_required, the else branch at line 48-50 shows "Failed to complete login - tokens not found", which is technically incorrect and may confuse users. Consider handling these cases explicitly:Proposed improvement
const result = mapLoginResponse(response) if ( result.kind === 'success' && result.session?.accessToken && result.session?.refreshToken ) { await writeSessionFromAuthResult(result) const profile = resolveProfileFromAuthResult(result) if (profile) { setCurrentProfile(profile) await setProfileExpoStorage(profile) } await options?.onSuccess?.(result.user ?? null) + } else if (result.kind === 'mfa_required') { + sendApiErrorToast({ message: 'MFA is required but not supported for Google login' }) + } else if (result.kind === 'redirect_required') { + // Handle redirect if needed, or log for debugging + sendApiErrorToast({ message: 'Additional steps required to complete login' }) } else { sendApiErrorToast({ message: 'Failed to complete login - tokens not found' }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/authentication/modules/access/allauth/useAllAuthGoogleLoginHandler/index.ts` around lines 30 - 54, The else branch in handleGoogleLoginSuccess treats any non-success mapLoginResponse result as "tokens not found" which mislabels cases like 'mfa_required' or 'redirect_required'; update handleGoogleLoginSuccess to explicitly branch on result.kind (e.g., 'mfa_required', 'redirect_required', 'error') and call appropriate user feedback handlers (e.g., sendApiErrorToast with specific messages or invoke an MFA/redirect flow) instead of the generic sendApiErrorToast message, while keeping the existing tokens-not-found message only for the case where kind === 'success' but tokens are missing.
🧹 Nitpick comments (33)
packages/authentication/modules/access/useRecoverPassword/__tests__/useRecoverPassword.test.tsx (1)
53-82: Consider adding assertion that mock was called in error test.The error test verifies
onErrorran but doesn't confirm the mock was actually invoked. This could mask issues where the error comes from elsewhere (e.g., validation).💡 Suggested addition
await result.current.form.handleSubmit() + expect(mockRecoverPassword).toHaveBeenCalledWith({ email }) expect(hasOnErrorRan).toBe(true)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/authentication/modules/access/useRecoverPassword/__tests__/useRecoverPassword.test.tsx` around lines 53 - 82, The test "should run onError when strategy rejects" for useRecoverPassword doesn't assert the mockRecoverPassword was invoked; add an assertion after await result.current.form.handleSubmit() to verify mockRecoverPassword was called (e.g., toHaveBeenCalledTimes(1) or toHaveBeenCalledWith the expected payload) so the test ensures the error came from the mocked strategy rather than other sources.packages/authentication/services/auth.ts (2)
31-46: Inconsistent API pattern and redundant type check.
Line 41: The
response instanceof Responsecheck is redundant—fetchalways returns aResponseobject (or throws on network failure).Consistency: Other methods in this class use
baseAppFetch, but this method uses rawfetch. Consider usingbaseAppFetchfor consistent error handling and request configuration across the API layer.🔧 Proposed simplification
- if (response instanceof Response && !response.ok) { + if (!response.ok) { throw new Error('Failed to pre-authenticate.') }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/authentication/services/auth.ts` around lines 31 - 46, The preAuthenticate method redundantly checks `response instanceof Response` and uses raw fetch instead of the shared API helper; update `preAuthenticate(token: string): Promise<JWTResponse>` to call the existing `baseAppFetch` helper (same pattern as other methods in this class) so you get consistent headers/error handling, remove the `instanceof Response` check, and throw or propagate errors based on `baseAppFetch` results while returning the parsed `JWTResponse`.
48-50: Consider removing the alias method.
preAuthenticateJWTsimply delegates topreAuthenticatewithout adding any value. If both names are needed for API clarity, consider keeping just one to reduce surface area, or document why both exist.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/authentication/services/auth.ts` around lines 48 - 50, preAuthenticateJWT is a redundant alias of preAuthenticate; remove the unnecessary method to reduce API surface and update callers to call AuthService.preAuthenticate(token) directly. Locate the static method preAuthenticateJWT on the AuthService (or class containing preAuthenticate/preAuthenticateJWT), delete that method, search and replace any call sites to use preAuthenticate instead, and update any related exports, type declarations, or tests that referenced preAuthenticateJWT to avoid breaking imports.packages/components/modules/navigations/web/Header/AccountMenu/AccountPopover/LogoutItem/index.tsx (1)
20-23: Consider handling logout errors gracefully.If
logout()fails (e.g., network error), the user has already seen the popover close but may still be logged in. Consider adding error handling or makinglogoutreturn a promise that can be awaited.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/components/modules/navigations/web/Header/AccountMenu/AccountPopover/LogoutItem/index.tsx` around lines 20 - 23, The onClick handler currently calls handlePopoverOnClose() then logout() without awaiting or handling failures, which can hide the UI while logout fails; update the onClick to call an async handler that awaits logout() (or handles its returned promise) and wraps it in try/catch, only closing the popover via handlePopoverOnClose() after a successful logout or, on error, show an error message (toast/modal) and keep the popover open; reference the existing onClick, handlePopoverOnClose, and logout identifiers when making the change.packages/authentication/modules/access/useResetPassword/index.ts (1)
38-42: Remove unnecessary type assertion.The assertion
(err as AuthError).fieldErrorsis redundant since the'fieldErrors' in errcheck already narrows the type. SonarCloud flagged this.♻️ Simplified error handling
if (enableFormApiErrors) { const errorWithFieldErrors = err && typeof err === 'object' && 'fieldErrors' in err - ? { response: { data: (err as AuthError).fieldErrors } } + ? { response: { data: err.fieldErrors } } : err setFormApiErrors(form, errorWithFieldErrors) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/authentication/modules/access/useResetPassword/index.ts` around lines 38 - 42, The code creates errorWithFieldErrors using a redundant type assertion (err as AuthError). Remove the unnecessary cast: when checking err && typeof err === 'object' && 'fieldErrors' in err, access err.fieldErrors directly instead of (err as AuthError). Update the expression that builds errorWithFieldErrors and keep the call to setFormApiErrors(form, errorWithFieldErrors) unchanged; reference symbols: err, AuthError, errorWithFieldErrors, setFormApiErrors, form.packages/components/modules/navigations/web/Header/AccountMenu/index.tsx (1)
35-41: Consider adding a loading indicator.The loading state currently renders only
{renderHeaderContent()}, which may appear empty to users. A subtle skeleton or spinner could improve perceived performance.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/components/modules/navigations/web/Header/AccountMenu/index.tsx` around lines 35 - 41, The isLoading branch in AccountMenu currently returns only renderHeaderContent(), which can look blank; update the AccountMenu component so when isLoading is true it renders a lightweight loading indicator (e.g., a small spinner or skeleton blocks) alongside or in place of renderHeaderContent() while preserving the existing wrapper div and grid classes; use an accessible pattern (aria-busy="true" and aria-label or sr-only text) and reuse any existing UI Spinner/Skeleton component if available, otherwise add minimal markup (divs with background shimmer or a centered spinner) so users see progress while renderHeaderContent() is resolving.packages/authentication/modules/auth-strategy/utils.ts (1)
4-8: Consider early validation of the environment variable value.While the type assertion
as AuthStrategyId | undefinedbypasses runtime validation, invalid values are actually caught ingetActiveAuthModule()via theSUPPORTED_STRATEGIES.has()check (lines 21-24), which throws a clear error before the strategy is used. However, validating ingetActiveStrategyId()would provide earlier feedback and better separation of concerns.If implementing validation, note that
AUTH_STRATEGY_IDSdoesn't exist in the codebase—useSUPPORTED_STRATEGIESinstead. The Set is exported from constants and contains the valid strategy IDs:♻️ Proposed early validation
import { SUPPORTED_STRATEGIES } from './constants' import { DEFAULT_AUTH_STRATEGY } from './constants' import type { AuthStrategyId } from './types' export function getActiveStrategyId(): AuthStrategyId { - return ( - (process.env.NEXT_PUBLIC_AUTH_STRATEGY as AuthStrategyId | undefined) ?? DEFAULT_AUTH_STRATEGY - ) + const envValue = process.env.NEXT_PUBLIC_AUTH_STRATEGY + if (envValue && SUPPORTED_STRATEGIES.has(envValue as AuthStrategyId)) { + return envValue as AuthStrategyId + } + return DEFAULT_AUTH_STRATEGY }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/authentication/modules/auth-strategy/utils.ts` around lines 4 - 8, The getActiveStrategyId function currently asserts process.env.NEXT_PUBLIC_AUTH_STRATEGY as AuthStrategyId without runtime validation; update getActiveStrategyId to read the env var, check it against SUPPORTED_STRATEGIES (imported from constants) and return it only if present in the Set, otherwise return DEFAULT_AUTH_STRATEGY (or throw a clear error if you prefer consistent behavior with getActiveAuthModule); reference getActiveStrategyId, SUPPORTED_STRATEGIES, DEFAULT_AUTH_STRATEGY and AuthStrategyId when making the change so validation happens earlier and avoids relying solely on getActiveAuthModule.packages/utils/functions/auth/awaitSessionRecovery/__tests__/awaitSessionRecovery.test.ts (1)
53-53: Assert handler registration explicitly before invoking it.At
Line 53, optional chaining can mask setup failures. An explicit expectation makes failures immediate and easier to diagnose.Suggested test hardening
- refreshedHandler?.() + expect(refreshedHandler).toBeDefined() + refreshedHandler!()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/utils/functions/auth/awaitSessionRecovery/__tests__/awaitSessionRecovery.test.ts` at line 53, The test currently calls refreshedHandler?.() which can hide registration failures; before invoking refreshedHandler in the test for awaitSessionRecovery add an explicit assertion that refreshedHandler is defined and callable (e.g., expect(refreshedHandler).toBeDefined() or expect(typeof refreshedHandler).toBe('function')) so any setup error surfaces immediately; update the test around the refreshedHandler reference to assert its presence (refreshedHandler) before calling it.packages/authentication/modules/access/useLogout/__tests__/useLogout.test.tsx (1)
44-52: Assert cache invalidation in the rejected-logout path too.This new test only checks
clear('logout'). IfuseLogoutstops resettingUSER_API_KEY/MFA_API_KEYwhenstrategy.logout()rejects, this failure-path regression will still pass.🧪 Suggested assertion additions
test('should clear session even if strategy logout fails', async () => { mockStrategyLogout.mockRejectedValueOnce(new Error('network failure')) const { result } = renderHook(() => useLogout(), { wrapper: ComponentWithProviders }) await result.current.logout() expect(mockStrategyLogout).toHaveBeenCalled() expect(mockClear).toHaveBeenCalledWith('logout') + expect(mockResetQueries).toHaveBeenCalledWith({ queryKey: USER_API_KEY.getUser() }) + expect(mockResetQueries).toHaveBeenCalledWith({ queryKey: MFA_API_KEY.default }) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/authentication/modules/access/useLogout/__tests__/useLogout.test.tsx` around lines 44 - 52, The test for useLogout's rejected-logout path only asserts clear('logout') but must also verify cache invalidation for USER_API_KEY and MFA_API_KEY; update the test (where mockStrategyLogout is set to reject and result.current.logout() is awaited) to also assert that mockClear was called with the USER_API_KEY and MFA_API_KEY keys (in addition to 'logout') so that useLogout still resets those caches when strategy.logout() fails.packages/authentication/modules/access/useLogin/__tests__/useLogin.test.tsx (1)
45-48: Inconsistent mock reset methods.
mockLogin.mockReset()clears both the mock implementation and call history, whilemockWriteSession.mockClear()only clears call history. SincemockWriteSessionhas a persistent.mockResolvedValue(undefined)at line 11, usingmockClearis intentional here. However, consider adding a brief comment to clarify this intentional distinction, or usemockResetconsistently and re-set the resolved value inbeforeEach.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/authentication/modules/access/useLogin/__tests__/useLogin.test.tsx` around lines 45 - 48, The afterEach currently calls mockLogin.mockReset() but mockWriteSession.mockClear(), which is intentional because mockWriteSession keeps a persistent mockResolvedValue; either add a short comment next to afterEach explaining that mockWriteSession intentionally uses mockClear to preserve its mockResolvedValue, or change to call mockReset() for mockWriteSession as well and then re-apply mockWriteSession.mockResolvedValue(undefined) inside the beforeEach setup (reference symbols: mockLogin, mockWriteSession, beforeEach, afterEach) so behavior is explicit and consistent.packages/authentication/modules/user/useCurrentUser/__tests__/useCurrentUser.test.tsx (1)
100-106: SequentialwaitForcalls can be consolidated.The three sequential
waitForcalls at lines 100-106 could potentially cause flaky tests if state transitions happen in a different order. Consider combining them or using a single condition that represents the final expected state:Suggested consolidation
- await waitFor(() => expect(result.current.isPending).toBe(false)) - await waitFor(() => expect(result.current.isLoading).toBe(false)) - await waitFor(() => - expect(resetQueriesMock).toHaveBeenCalledWith({ - queryKey: ['user', 'getUser'], - }), - ) + await waitFor(() => { + expect(result.current.isPending).toBe(false) + expect(result.current.isLoading).toBe(false) + expect(resetQueriesMock).toHaveBeenCalledWith({ + queryKey: ['user', 'getUser'], + }) + })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/authentication/modules/user/useCurrentUser/__tests__/useCurrentUser.test.tsx` around lines 100 - 106, Consolidate the three sequential waitFor assertions into a single waitFor to avoid ordering flakiness: inside one waitFor, assert that result.current.isPending is false, result.current.isLoading is false, and resetQueriesMock has been called with { queryKey: ['user', 'getUser'] } so all final expectations for the useCurrentUser test are checked atomically (references: result.current.isPending, result.current.isLoading, resetQueriesMock).packages/authentication/modules/user/useSession/__tests__/useSession.test.tsx (1)
14-41: Good test for expired session state, consider expanding coverage.The test correctly validates that user data is preserved during an expired but recoverable session. However, the
refreshmock is defined but never asserted. Consider verifying that refresh is triggered when the session is expired:Suggested assertion addition
+ const refreshMock = jest.fn() jest.spyOn(sessionClient, 'getSessionService').mockReturnValue({ getState: jest.fn().mockResolvedValue({ // ...existing mock } satisfies SessionState<Partial<User>>), - refresh: jest.fn(), + refresh: refreshMock, } as any) // ... existing test code ... await waitFor(() => expect(result.current.isRefreshing).toBe(true)) + expect(refreshMock).toHaveBeenCalled()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/authentication/modules/user/useSession/__tests__/useSession.test.tsx` around lines 14 - 41, The test sets a mocked getSessionService that includes a refresh mock but never asserts it was invoked; update the test to capture the refresh mock (e.g., store the jest.fn() passed under refresh from the getSessionService return) and add an assertion that refresh was called when the session is expired (use waitFor/expect on the captured refresh mock), keeping the existing checks on useSession (useSession, getSessionService, result.current) intact.packages/authentication/modules/access/useResetPassword/__tests__/useResetPassword.test.tsx (2)
91-118: Consider asserting the mock was not called.The test correctly expects
onSuccessnot to run when passwords don't match due to form validation. However, adding an explicit assertion thatmockResetPasswordwas never called would make the test's intent clearer and catch potential validation bypass bugs.Suggested addition
await result.current.form.handleSubmit() expect(hasOnSuccessRan).toBe(false) + expect(mockResetPassword).not.toHaveBeenCalled() })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/authentication/modules/access/useResetPassword/__tests__/useResetPassword.test.tsx` around lines 91 - 118, The test "should not submit when newPassword and confirmNewPassword are different" currently only asserts onSuccess wasn't run; also assert that the reset API mock was never invoked to ensure validation blocked submission. Update the test to add an expectation that mockResetPassword was not called (e.g., expect(mockResetPassword).not.toHaveBeenCalled()) after calling result.current.form.handleSubmit(), keeping the existing setup for useResetPassword and the onSuccess flag.
120-154: Test passes with mismatched passwords due to permissive custom schema.The custom validation schema only enforces
min(1)for both password fields without requiring them to match. SincenewPassword: '12345'differs fromconfirmNewPassword: '123456', this test inadvertently validates that mismatched passwords are accepted when using a custom schema without a.refine()check.If this is intentional to demonstrate custom schema flexibility, consider adding a comment. Otherwise, align the test values:
Suggested fix
const customDefaultValues = { newPassword: '12345', - confirmNewPassword: '123456', + confirmNewPassword: '12345', }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/authentication/modules/access/useResetPassword/__tests__/useResetPassword.test.tsx` around lines 120 - 154, The test "should allow custom defaultValues and validationSchema" currently passes because customValidationSchema only enforces min(1) and does not require newPassword and confirmNewPassword to match; update the test so the customValidationSchema enforces matching passwords (e.g., add a .refine check on customValidationSchema to compare newPassword and confirmNewPassword) and/or set customDefaultValues.confirmNewPassword to the same value as newPassword so the test reflects the intended validation; locate the customDefaultValues and customValidationSchema in this test and apply the change there (references: useResetPassword hook, customDefaultValues, customValidationSchema, test name).packages/authentication/__tests__/contracts/SessionLifecycle.contract.test.ts (1)
36-50: Consider using proper mock typing instead ofas never.The
as nevercast at line 37 (and similar ones at lines 77, 112) suggests a type mismatch between the mock return value anddecodeJWT's expected return type. Consider creating a properly typed mock return value or usingjest.mockedwith explicit type parameters to improve type safety.Example approach
// Define a partial type that satisfies the minimum contract type MockDecodedToken = { exp: number } mockedDecodeJWT.mockReturnValue({ exp: Math.floor(Date.now() / 1000) + 3600 } as MockDecodedToken)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/authentication/__tests__/contracts/SessionLifecycle.contract.test.ts` around lines 36 - 50, The tests use an unsafe cast "as never" when setting mockedDecodeJWT return values; replace that with proper mock typing by declaring a minimal decoded token type (e.g., MockDecodedToken with exp:number) and use it when calling mockedDecodeJWT.mockReturnValue, or convert the mock to a typed Jest mock via jest.mocked(decodeJWT, true) with explicit generics so mockedDecodeJWT returns the correct type; update all occurrences (mockedDecodeJWT.mockReturnValue at the spots you saw and similar casts at lines where mockedIsUserTokenValid or other mocks are typed) to remove "as never" and use the properly typed mock return instead.packages/authentication/modules/access/useSignUp/__tests__/useSignUp.test.tsx (1)
122-124: Consider stronger typing for mutation data assertion.The
as anycast at line 123 bypasses type safety. Since you're already checkingkind: 'success'at line 122, consider using a type guard or narrowing the type properly:Suggested approach
expect(result.current.mutation.data).toMatchObject({ kind: 'success' }) - expect((result.current.mutation.data as any)?.rawResponse).toStrictEqual(customRequest) + const successData = result.current.mutation.data as AuthResult & { kind: 'success' } + expect(successData.rawResponse).toStrictEqual(customRequest)Alternatively, if
AuthResultis a discriminated union, you could use:if (result.current.mutation.data?.kind === 'success') { expect(result.current.mutation.data.rawResponse).toStrictEqual(customRequest) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/authentication/modules/access/useSignUp/__tests__/useSignUp.test.tsx` around lines 122 - 124, The test currently uses an unsafe as any cast on result.current.mutation.data when asserting rawResponse; instead narrow the type by checking the discriminant (e.g., verify result.current.mutation.data?.kind === 'success') or use a type guard for AuthResult before accessing rawResponse so the assertion on result.current.mutation.data.rawResponse is type-safe (locate this in the test around the expect calls referencing result.current.mutation.data and replace the as any cast with the discriminant check/type guard).packages/authentication/modules/access/useChangePassword/__tests__/useChangePassword.test.tsx (1)
92-119: Assert the mutation never runs in the mismatch case.Right now this passes as long as
onSuccessstays false. A regression that still submits and fails later would slip through.♻️ Proposed test hardening
await result.current.form.handleSubmit() + expect(mockChangePassword).not.toHaveBeenCalled() expect(hasOnSuccessRan).toBe(false)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/authentication/modules/access/useChangePassword/__tests__/useChangePassword.test.tsx` around lines 92 - 119, The test currently only checks that onSuccess didn't run when newPassword and confirmNewPassword differ; update it to also assert that the mutation function (mockChangePassword) was never called to prevent regressions that submit but fail later. In the test in useChangePassword.test.tsx, after invoking result.current.form.handleSubmit(), add an expectation that mockChangePassword (the mocked mutation used by useChangePassword) has not been called, while keeping the existing hasOnSuccessRan assertion to ensure the form validation prevented submission.packages/authentication/modules/access/useLogin/login-success-handler.ts (1)
45-59: TODO comment noted for MFA profile hydration.The TODO at line 56 is a valid deferred task. Consider creating a tracking issue to ensure this doesn't get lost.
Would you like me to help draft an issue to track the MFA profile hydration task mentioned in the TODO?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/authentication/modules/access/useLogin/login-success-handler.ts` around lines 45 - 59, The TODO in writeMfaSession about moving MFA profile hydration into the strategy/session layer should be tracked: create a short issue outlining the work (title like "Move MFA profile hydration into auth strategy/session layer"), describe current behavior (decoded JWTs in hook), target behavior (hydrate profile during MFA completion inside the strategy or session service), list acceptance criteria (no JWT decoding in hooks, profile available post-MFA, tests), and link the issue ID in the TODO comment in writeMfaSession so it won't be lost; reference the function name writeMfaSession and the sessionService.write call when updating the comment.packages/authentication/session/storage.ts (2)
43-48: ClearingCURRENT_PROFILE_KEY_NAMEin session storage is a side effect worth documenting.The
clear()method removes the current profile cookie in addition to session tokens. This coupling between session and profile storage is intentional based on the summary, but it may be non-obvious to future maintainers. Consider adding a brief inline comment explaining why the profile is cleared alongside session tokens.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/authentication/session/storage.ts` around lines 43 - 48, The clear() method currently removes the profile cookie as well as session tokens, which is a non-obvious side effect; add a brief inline comment inside the clear() implementation (near the removeTokenAsync(CURRENT_PROFILE_KEY_NAME) call) explaining why CURRENT_PROFILE_KEY_NAME is cleared along with ACCESS_KEY_NAME, REFRESH_KEY_NAME and SESSION_TOKEN_KEY_NAME (e.g., to ensure stale profile data isn't retained after session termination or to force profile re-selection on next login), so future maintainers of clear() and removeTokenAsync understand the intentional coupling.
31-41: Consider handling explicit token removal inwrite().The current implementation only writes tokens when they are truthy. If a caller intends to clear a specific token (e.g., passing
{ accessToken: null, refreshToken: 'valid' }), theaccessTokenwon't be removed—it will retain its previous value. If partial clearing is a valid use case, consider adding explicit removal logic for falsy values, or document thatclear()should be used for full session removal.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/authentication/session/storage.ts` around lines 31 - 41, The write() method currently only writes truthy tokens, so passing null/false won't remove an existing cookie; update write(session) to explicitly remove tokens when a token field is present but falsy (e.g., accessToken === null) instead of only skipping them—use the existing setTokenAsync/REMOVE helper (or add a removeTokenAsync function) to remove ACCESS_KEY_NAME, REFRESH_KEY_NAME, and SESSION_TOKEN_KEY_NAME using cookieConfig when the corresponding session property is explicitly null/undefined-intentional removal; leave undefined properties unchanged and keep clear() for full-session wipes.packages/authentication/__tests__/contracts/AllauthStrategy.contract.test.ts (1)
127-137: Optional: Consider renaming catch parameter per SonarCloud.Static analysis suggests renaming
throwntoerror_. This is a stylistic preference—thrownis descriptive and clear in context. If the project has a convention for catch parameter naming, apply it consistently.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/authentication/__tests__/contracts/AllauthStrategy.contract.test.ts` around lines 127 - 137, Rename the catch parameter in the test's try/catch from "thrown" to the project-preferred name (e.g., "error_" or "err") to satisfy SonarCloud style rules; update the cast and subsequent references (the line casting to AuthError and assertions that use authError) so the caught value is assigned to the new identifier before casting to AuthError and running assertions in the AllauthStrategy.contract.test (the catch block that currently defines "thrown" and then const authError = thrown as AuthError).packages/utils/functions/axios/createAxiosInstance/index.ts (4)
144-150: Simplify error config type handling.The type assertion is flagged as unnecessary. Consider using a simpler approach.
Proposed fix
- const errorConfig = error.config as - | (typeof error.config & AuthRecoveryRetryRequest) - | undefined - const hasRetried = Boolean(errorConfig?.authRecoveryRetried) + const hasRetried = Boolean((error.config as AuthRecoveryRetryRequest | undefined)?.authRecoveryRetried)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/utils/functions/axios/createAxiosInstance/index.ts` around lines 144 - 150, The explicit intersecting type assertion for errorConfig is unnecessary; simplify by directly reading the flag from error.config using the specific AuthRecoveryRetryRequest shape (or optional chaining) instead of the complex union/intersection. Update the block around errorConfig/hasRetried (in createAxiosInstance) to derive hasRetried via optional chaining from error.config cast to AuthRecoveryRetryRequest | undefined (or by checking (error.config as any)?.authRecoveryRetried), leaving isAuthTokenRequired and servicesWithoutToken logic unchanged.
160-171: Simplify type assertion and remove empty object fallback.Static analysis flags the type assertion at lines 161-168 as unnecessary and the empty object at line 165 as useless since
error.config?.headersis already being spread.Proposed fix
if (latestAccessToken && latestAccessToken !== failedAccessToken) { const retryConfig = { ...error.config, [AUTH_RECOVERY_RETRY_FLAG]: true, headers: { - ...(error.config?.headers ?? {}), + ...error.config?.headers, Authorization: `${tokenType} ${latestAccessToken}`, }, - } as typeof error.config & AuthRecoveryRetryRequest + } return instance.request(retryConfig) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/utils/functions/axios/createAxiosInstance/index.ts` around lines 160 - 171, The retryConfig construction in the error handler should drop the unnecessary type assertion and the redundant empty-object fallback; replace the current block that creates retryConfig (and the "as typeof error.config & AuthRecoveryRetryRequest" cast) with a plain object literal that spreads error.config, sets [AUTH_RECOVERY_RETRY_FLAG]: true, and builds headers by spreading error.config?.headers (no "?? {}") and adding Authorization: `${tokenType} ${latestAccessToken}`, then pass that object directly to instance.request(retryConfig).
68-69: Unnecessary type assertion.Per static analysis, the type assertion on line 68 doesn't change the expression's type. The cast can be simplified.
Proposed fix
- const requestWithRetryState = request as typeof request & AuthRecoveryRetryRequest - const isRetriedRequest = Boolean(requestWithRetryState.authRecoveryRetried) + const isRetriedRequest = Boolean((request as AuthRecoveryRetryRequest).authRecoveryRetried)Or define the extended request type once and use it consistently.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/utils/functions/axios/createAxiosInstance/index.ts` around lines 68 - 69, The type assertion "const requestWithRetryState = request as typeof request & AuthRecoveryRetryRequest" is redundant; remove it and either (A) declare the function parameter as AuthRecoveryRetryRequest so you can use request.authRecoveryRetried directly, or (B) perform a single, simple cast to AuthRecoveryRetryRequest where needed (e.g., check Boolean((request as AuthRecoveryRetryRequest).authRecoveryRetried)). Update any uses of requestWithRetryState/isRetriedRequest to reference the cleaned-up approach and remove the unnecessary intermediate const.
182-197: Same simplifications apply to the second retry config.Remove the unnecessary type assertion and empty object fallback for consistency.
Proposed fix
const retryConfig = { ...error.config, [AUTH_RECOVERY_RETRY_FLAG]: true, headers: { - ...(error.config?.headers ?? {}), + ...error.config?.headers, }, - } as typeof error.config & AuthRecoveryRetryRequest + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/utils/functions/axios/createAxiosInstance/index.ts` around lines 182 - 197, The retryConfig creation currently uses an unnecessary type assertion and an explicit empty-object fallback for headers; update the retry block that creates retryConfig (the variable named retryConfig and the AUTH_RECOVERY_RETRY_FLAG usage) to remove the "as typeof error.config & AuthRecoveryRetryRequest" assertion and remove the "?? {}" fallback so headers are spread from error.config?.headers directly (i.e., headers: { ...(error.config?.headers) }), then keep the existing Authorization set/delete logic using refreshedToken and tokenType and return instance.request(retryConfig).packages/authentication/modules/auth-strategy/types.ts (2)
15-15: Consider extensibility for future strategy IDs.
AuthStrategyIdis directly derived fromALLAUTH_STRATEGY_IDconstant only. When adding new strategies, this type will need to become a union. Consider documenting this or structuring as a union type now for clarity.// Current: tied to single strategy export type AuthStrategyId = typeof ALLAUTH_STRATEGY_ID // Future-ready alternative: export type AuthStrategyId = typeof ALLAUTH_STRATEGY_ID | typeof OTHER_STRATEGY_ID🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/authentication/modules/auth-strategy/types.ts` at line 15, AuthStrategyId is currently tied to a single constant (ALLAUTH_STRATEGY_ID), which prevents easy addition of new strategies; update AuthStrategyId to be a union of strategy identifier types (e.g., include other constants like OTHER_STRATEGY_ID) or add a clear comment documenting that it should be expanded into a union when new strategy constants are added so future strategies can be represented without refactors.
134-138: Loose typing onsubmitfunction signature.The
submitmethod uses(...args: unknown[]) => Promise<TData>which loses type safety for operation-specific parameters. Consumers won't get compile-time checks for correct arguments.Consider making this generic over input parameters or documenting that concrete hook implementations should provide properly typed wrappers.
💡 Alternative with input typing
-export interface UseAuthOperationResult<TData = AuthResult> { - submit: (...args: unknown[]) => Promise<TData> +export interface UseAuthOperationResult<TData = AuthResult, TInput = unknown> { + submit: (input: TInput) => Promise<TData> error: AuthError | null isPending: boolean }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/authentication/modules/auth-strategy/types.ts` around lines 134 - 138, The UseAuthOperationResult interface currently types submit as (...args: unknown[]) => Promise<TData>, which loses argument type safety; change the interface to introduce a generic parameter for input arguments (e.g. UseAuthOperationResult<TData = AuthResult, TArgs extends unknown[] = unknown[]>) and update the submit signature to submit: (...args: TArgs) => Promise<TData>, then update any concrete implementations or factory functions (hooks that return UseAuthOperationResult) to provide the appropriate TArgs tuple so callers get compile-time checks.packages/authentication/modules/auth-strategy/allauth/strategy.ts (2)
46-52: Logout error is swallowed silently.The
logoutmethod catches errors and callsthrowAuthError, but sincethrowAuthErrorthrows, if an error occurs the function exits via exception. However, ifthrowAuthErrorsomehow doesn't throw (edge case), the method returnsundefinedimplicitly without indicating failure.This is likely fine given
throwAuthError's implementation, but consider adding an explicitthrowafterthis.throwAuthError(error)for clarity:♻️ Suggested defensive fix
async logout(): Promise<void> { try { await AllAuthApi.logout() } catch (error) { - this.throwAuthError(error) + throw this.throwAuthError(error) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/authentication/modules/auth-strategy/allauth/strategy.ts` around lines 46 - 52, The logout method catches errors from AllAuthApi.logout and delegates to this.throwAuthError(error), but to guard against the unlikely case where throwAuthError does not throw, update the catch block in logout to explicitly rethrow (e.g., throw error) after calling this.throwAuthError(error); ensure you reference the logout method, the call to AllAuthApi.logout(), and throwAuthError to locate and modify the catch block.
75-77: TODO comment noted.This documents the technical debt around using
AuthApifallback for change-password until Allauth exposes native support. The comment is clear about the reasoning.Would you like me to open a GitHub issue to track this migration task when Allauth adds native change-password support?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/authentication/modules/auth-strategy/allauth/strategy.ts` around lines 75 - 77, Create a GitHub issue to track replacing the AuthApi fallback with native Allauth change-password support (reference Allauth and AuthApi fallback currently noted in the TODO in strategy.ts) and include the current behavior, why it’s temporary, and acceptance criteria; then update the TODO in the Allauth strategy (mentioning AuthApi and the change-password flow) to reference the new issue number so future maintainers can find the migration task.packages/authentication/modules/access/useLogin/index.ts (1)
59-63: Remove unnecessary type assertion.Static analysis correctly flags that
(err as AuthError)is redundant here. The'fieldErrors' in errcheck already narrows the type sufficiently.♻️ Suggested fix
const errorWithFieldErrors = err && typeof err === 'object' && 'fieldErrors' in err - ? { response: { data: (err as AuthError).fieldErrors } } + ? { response: { data: err.fieldErrors } } : err🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/authentication/modules/access/useLogin/index.ts` around lines 59 - 63, The conditional constructing errorWithFieldErrors uses an unnecessary type assertion "(err as AuthError)"; since the "'fieldErrors' in err" guard already narrows the type, remove the cast and access fieldErrors directly (e.g., use err.fieldErrors) when building the { response: { data: ... } } object for setFormApiErrors(form, errorWithFieldErrors), leaving the rest of the logic (variables errorWithFieldErrors, err, setFormApiErrors, form) unchanged.packages/authentication/README.md (1)
501-506: Minor wording improvement for clarity.The static analysis flagged repeated use of "directly". Consider rewording for variety:
📝 Suggested revision
Consumer code should not: -- decode tokens directly -- check token expiry directly +- decode tokens manually +- check token expiry outside the session layer - branch on strategy names - call provider-specific auth services from UI code🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/authentication/README.md` around lines 501 - 506, The list of consumer prohibitions repeats the word "directly" and should be reworded for clarity and variety; update the four bullets ("decode tokens directly", "check token expiry directly", "branch on strategy names", "call provider-specific auth services from UI code") to remove the redundant "directly" (e.g., "decode tokens", "verify token expiry", "avoid branching on strategy names", "do not call provider-specific auth services from UI code") or similar concise alternatives so the intent remains clear without repetition.packages/authentication/session/server.ts (1)
68-74: Remove unnecessary type assertion.Static analysis correctly flags that
value as stringis redundant. The.filter(([, value]) => Boolean(value))already ensuresvalueis truthy (non-null/undefined string).♻️ Suggested fix using type narrowing
- tokens - .filter(([, value]) => Boolean(value)) - .forEach(([name, value]) => { - response.cookies.set(name, value as string, SESSION_COOKIE_CONFIG) - setRequestCookie(headers, name, value as string) + tokens + .filter((entry): entry is [string, string] => Boolean(entry[1])) + .forEach(([name, value]) => { + response.cookies.set(name, value, SESSION_COOKIE_CONFIG) + setRequestCookie(headers, name, value) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/authentication/session/server.ts` around lines 68 - 74, The code is using redundant type assertions "as string" inside the tokens.forEach callback; because the preceding .filter(([, value]) => Boolean(value)) already narrows out null/undefined, remove the "as string" casts from the calls to response.cookies.set and setRequestCookie in that callback (leave the filter as-is so the value is truthy and let TypeScript infer it as string). Ensure you update the callback signature if needed to reflect the tuple ([name, value]) types used by tokens so no cast is required.packages/utils/functions/fetch/baseAppFetch/__tests__/baseAppFetch.test.ts (1)
551-551: Consider usingglobalThisinstead ofglobal.Static analysis flagged this line.
globalThisis the modern cross-environment way to reference the global object and is preferred overglobal(Node-specific).♻️ Suggested fix
- const fetchMock = global.fetch as jest.Mock + const fetchMock = globalThis.fetch as jest.MockApply similarly to line 637 and other occurrences in this file.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/utils/functions/fetch/baseAppFetch/__tests__/baseAppFetch.test.ts` at line 551, Replace usage of the Node-specific global object with the cross-environment globalThis in the tests: change occurrences like "const fetchMock = global.fetch as jest.Mock" to use "globalThis.fetch" (and similar uses of global in this test file, e.g., the other mock assignments around the same test suite) so all references to the global object use globalThis instead of global.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 44a88c23-2e52-4581-817b-b450a4a81062
📒 Files selected for processing (94)
README.mdpackages/authentication/README.mdpackages/authentication/__tests__/contracts/AllauthStrategy.contract.test.tspackages/authentication/__tests__/contracts/AuthStrategyFactory.contract.test.tspackages/authentication/__tests__/contracts/SessionLifecycle.contract.test.tspackages/authentication/__tests__/contracts/SessionUtilities.contract.test.tspackages/authentication/__tests__/contracts/unauthorized-event-flow.contract.test.tspackages/authentication/client.tspackages/authentication/middleware.tspackages/authentication/modules/access/allauth/__tests__/utils.test.tspackages/authentication/modules/access/allauth/constants.tspackages/authentication/modules/access/allauth/types.tspackages/authentication/modules/access/allauth/useAllAuthGoogleLoginHandler/index.tspackages/authentication/modules/access/allauth/useAllAuthLogin/__tests__/useAllAuthLogin.test.tsxpackages/authentication/modules/access/allauth/useAllAuthLogin/index.tspackages/authentication/modules/access/allauth/useAllAuthLogout/__tests__/useAllAuthLogout.test.tsxpackages/authentication/modules/access/allauth/useAllAuthLogout/index.tspackages/authentication/modules/access/allauth/useAllAuthLogout/types.tspackages/authentication/modules/access/allauth/useAllAuthRecoverPassword/__tests__/useAllAuthRecoverPassword.test.tsxpackages/authentication/modules/access/allauth/useAllAuthRecoverPassword/index.tspackages/authentication/modules/access/allauth/useAllAuthResetPassword/__tests__/useAllAuthResetPassword.test.tsxpackages/authentication/modules/access/allauth/useAllAuthResetPassword/index.tspackages/authentication/modules/access/allauth/useAllAuthSession/index.tspackages/authentication/modules/access/allauth/useAllAuthSession/types.tspackages/authentication/modules/access/allauth/useAllAuthSignUp/__tests__/useAllAuthSignUp.test.tsxpackages/authentication/modules/access/allauth/useAllAuthSignUp/index.tspackages/authentication/modules/access/allauth/utils.tspackages/authentication/modules/access/index.tspackages/authentication/modules/access/preAuthenticateJWT/index.tspackages/authentication/modules/access/useChangePassword/__tests__/useChangePassword.test.tsxpackages/authentication/modules/access/useChangePassword/index.tspackages/authentication/modules/access/useChangePassword/types.tspackages/authentication/modules/access/useLogin/__tests__/useLogin.test.tsxpackages/authentication/modules/access/useLogin/index.tspackages/authentication/modules/access/useLogin/login-success-handler.tspackages/authentication/modules/access/useLogin/types.tspackages/authentication/modules/access/useLogout/__tests__/useLogout.test.tsxpackages/authentication/modules/access/useLogout/index.tspackages/authentication/modules/access/useRecoverPassword/__tests__/useRecoverPassword.test.tsxpackages/authentication/modules/access/useRecoverPassword/index.tspackages/authentication/modules/access/useRecoverPassword/types.tspackages/authentication/modules/access/useResetPassword/__tests__/useResetPassword.test.tsxpackages/authentication/modules/access/useResetPassword/index.tspackages/authentication/modules/access/useResetPassword/types.tspackages/authentication/modules/access/useSignUp/__tests__/useSignUp.test.tsxpackages/authentication/modules/access/useSignUp/index.tspackages/authentication/modules/access/useSignUp/types.tspackages/authentication/modules/auth-strategy/allauth/constants.tspackages/authentication/modules/auth-strategy/allauth/index.tspackages/authentication/modules/auth-strategy/allauth/session.tspackages/authentication/modules/auth-strategy/allauth/strategy.tspackages/authentication/modules/auth-strategy/allauth/utils.tspackages/authentication/modules/auth-strategy/base/base-auth-strategy.tspackages/authentication/modules/auth-strategy/constants.tspackages/authentication/modules/auth-strategy/factory.tspackages/authentication/modules/auth-strategy/preAuthenticate.tspackages/authentication/modules/auth-strategy/types.tspackages/authentication/modules/auth-strategy/utils.tspackages/authentication/modules/user/getUserSSR/__tests__/getUserSSR.test.tspackages/authentication/modules/user/getUserSSR/index.tspackages/authentication/modules/user/index.tspackages/authentication/modules/user/useCurrentUser/__tests__/fixtures/request.jsonpackages/authentication/modules/user/useCurrentUser/__tests__/useCurrentUser.test.tsxpackages/authentication/modules/user/useCurrentUser/index.tspackages/authentication/modules/user/useCurrentUser/types.tspackages/authentication/modules/user/useJWTUser/__tests__/useJWTUser.test.tsxpackages/authentication/modules/user/useJWTUser/index.tspackages/authentication/modules/user/useSession/__tests__/useSession.test.tsxpackages/authentication/modules/user/useSession/index.tspackages/authentication/modules/user/useUpdateUser/__tests__/useUpdateUser.test.tsxpackages/authentication/modules/user/useUpdateUser/index.tspackages/authentication/modules/user/useUpdateUser/types.tspackages/authentication/server.tspackages/authentication/services/auth.tspackages/authentication/session/__tests__/session-lifecycle.test.tspackages/authentication/session/client.tspackages/authentication/session/index.tspackages/authentication/session/server.tspackages/authentication/session/service.tspackages/authentication/session/storage.tspackages/authentication/types/auth.tspackages/components/modules/navigations/web/Header/AccountMenu/AccountPopover/CurrentUser/index.tsxpackages/components/modules/navigations/web/Header/AccountMenu/AccountPopover/LogoutItem/index.tsxpackages/components/modules/navigations/web/Header/AccountMenu/AccountPopover/__tests__/AccountPopover.cy.tsxpackages/components/modules/navigations/web/Header/AccountMenu/index.tsxpackages/graphql/config/environment.tspackages/utils/constants/events.tspackages/utils/functions/auth/awaitSessionRecovery/__tests__/awaitSessionRecovery.test.tspackages/utils/functions/auth/awaitSessionRecovery/index.tspackages/utils/functions/axios/createAxiosInstance/__tests__/createAxiosInstance.test.tspackages/utils/functions/axios/createAxiosInstance/index.tspackages/utils/functions/fetch/baseAppFetch/__tests__/baseAppFetch.test.tspackages/utils/functions/fetch/baseAppFetch/index.tspackages/utils/functions/fetch/baseAppFetch/types.ts
💤 Files with no reviewable changes (22)
- packages/authentication/modules/access/allauth/useAllAuthLogout/types.ts
- packages/authentication/modules/access/allauth/useAllAuthRecoverPassword/tests/useAllAuthRecoverPassword.test.tsx
- packages/authentication/modules/access/allauth/useAllAuthLogout/tests/useAllAuthLogout.test.tsx
- packages/authentication/modules/access/allauth/useAllAuthLogout/index.ts
- packages/authentication/modules/access/allauth/useAllAuthResetPassword/tests/useAllAuthResetPassword.test.tsx
- packages/authentication/modules/access/allauth/useAllAuthSession/types.ts
- packages/authentication/modules/access/allauth/useAllAuthSession/index.ts
- packages/authentication/modules/access/allauth/useAllAuthRecoverPassword/index.ts
- packages/authentication/modules/access/useResetPassword/types.ts
- packages/authentication/modules/access/useRecoverPassword/types.ts
- packages/authentication/modules/user/useJWTUser/index.ts
- packages/authentication/modules/access/allauth/useAllAuthSignUp/tests/useAllAuthSignUp.test.tsx
- packages/authentication/modules/user/useJWTUser/tests/useJWTUser.test.tsx
- packages/authentication/modules/access/allauth/useAllAuthResetPassword/index.ts
- packages/authentication/modules/access/allauth/useAllAuthLogin/index.ts
- packages/authentication/modules/user/getUserSSR/index.ts
- packages/authentication/modules/access/allauth/useAllAuthSignUp/index.ts
- packages/authentication/modules/access/allauth/constants.ts
- packages/authentication/modules/user/getUserSSR/tests/getUserSSR.test.ts
- packages/authentication/modules/access/allauth/useAllAuthLogin/tests/useAllAuthLogin.test.tsx
- packages/authentication/modules/access/useChangePassword/types.ts
- packages/authentication/modules/access/allauth/types.ts
| const errorWithFieldErrors = | ||
| err && typeof err === 'object' && 'fieldErrors' in err | ||
| ? { response: { data: (err as AuthError).fieldErrors } } | ||
| : err | ||
| setFormApiErrors(form, errorWithFieldErrors) |
There was a problem hiding this comment.
Remove unnecessary type assertion.
SonarCloud correctly flags that the as AuthError assertion at line 40 is unnecessary. The 'fieldErrors' in err check already narrows the type sufficiently for TypeScript to know err.fieldErrors exists.
Suggested fix
const errorWithFieldErrors =
err && typeof err === 'object' && 'fieldErrors' in err
- ? { response: { data: (err as AuthError).fieldErrors } }
+ ? { response: { data: err.fieldErrors } }
: err📝 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.
| const errorWithFieldErrors = | |
| err && typeof err === 'object' && 'fieldErrors' in err | |
| ? { response: { data: (err as AuthError).fieldErrors } } | |
| : err | |
| setFormApiErrors(form, errorWithFieldErrors) | |
| const errorWithFieldErrors = | |
| err && typeof err === 'object' && 'fieldErrors' in err | |
| ? { response: { data: err.fieldErrors } } | |
| : err | |
| setFormApiErrors(form, errorWithFieldErrors) |
🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis
[warning] 40-40: This assertion is unnecessary since it does not change the type of the expression.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/authentication/modules/access/useChangePassword/index.ts` around
lines 38 - 42, The code uses an unnecessary type assertion "(err as AuthError)"
when building errorWithFieldErrors; the preceding "'fieldErrors' in err" check
already narrows err, so remove the cast and directly use err.fieldErrors when
constructing { response: { data: err.fieldErrors } }; update the expression that
assigns errorWithFieldErrors (and leave the call to setFormApiErrors(form,
errorWithFieldErrors) unchanged) so TypeScript infers the correct type without
the redundant assertion.
| onSuccess: async (result, variables, context) => { | ||
| if (result.kind === 'mfa_required') { | ||
| setMfaEphemeralToken(result.ephemeralToken) | ||
| } else if (result.kind === 'success') { | ||
| await handleLoginSuccess(result) | ||
| } | ||
| // onSuccess should only run after we successfully await the handleLoginSuccess (that sets the auth tokens asynchonously) | ||
| loginOptions?.onSuccess?.(response, variables, context) | ||
| loginOptions?.onSuccess?.(result, variables, context) | ||
| }, |
There was a problem hiding this comment.
Handle redirect_required kind in login success.
The onSuccess handler branches on mfa_required and success, but AuthResult also includes redirect_required. If the strategy returns this kind, it will be silently ignored.
Consider adding explicit handling or a fallback:
💡 Suggested fix
onSuccess: async (result, variables, context) => {
if (result.kind === 'mfa_required') {
setMfaEphemeralToken(result.ephemeralToken)
} else if (result.kind === 'success') {
await handleLoginSuccess(result)
+ } else if (result.kind === 'redirect_required') {
+ // Redirect handling delegated to consumer via loginOptions.onSuccess
}
loginOptions?.onSuccess?.(result, variables, context)
},🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/authentication/modules/access/useLogin/index.ts` around lines 66 -
73, The onSuccess handler currently handles only 'mfa_required' and 'success',
so add explicit handling for the 'redirect_required' AuthResult kind inside the
onSuccess callback (alongside setMfaEphemeralToken and await handleLoginSuccess)
— for example, branch on result.kind === 'redirect_required' and call a new
helper like handleLoginRedirect(result) or perform the redirect (e.g.,
window.location.assign(result.redirectUrl)) as appropriate, then still invoke
loginOptions?.onSuccess?.(result, variables, context); ensure you reference the
existing onSuccess handler, setMfaEphemeralToken, handleLoginSuccess, and
loginOptions?.onSuccess when implementing the change.
| const isWebPlatform = !isMobilePlatform() | ||
| const API_BASE_URL = isWebPlatform | ||
| ? process.env.NEXT_PUBLIC_API_BASE_URL | ||
| : process.env.EXPO_PUBLIC_API_BASE_URL | ||
| const baseUrl = API_BASE_URL?.replace('/v1', '') | ||
|
|
||
| return profileImage.startsWith('http') || !baseUrl ? profileImage : `${baseUrl}${profileImage}` | ||
| } |
There was a problem hiding this comment.
Fragile URL manipulation for profile image resolution.
The .replace('/v1', '') on line 14 assumes a specific URL structure. This could break if:
- The API version changes (e.g.,
/v2) - The URL doesn't contain
/v1 - The URL contains multiple
/v1segments
Consider using a more robust approach, such as a dedicated base URL constant for media assets or URL parsing.
Proposed improvement
+ const MEDIA_BASE_URL = isWebPlatform
+ ? process.env.NEXT_PUBLIC_MEDIA_BASE_URL ?? process.env.NEXT_PUBLIC_API_BASE_URL?.replace(/\/v\d+$/, '')
+ : process.env.EXPO_PUBLIC_MEDIA_BASE_URL ?? process.env.EXPO_PUBLIC_API_BASE_URL?.replace(/\/v\d+$/, '')
- const baseUrl = API_BASE_URL?.replace('/v1', '')
+ const baseUrl = MEDIA_BASE_URLOr use a regex that handles version suffixes more reliably: .replace(/\/v\d+$/, '') to only strip trailing version paths.
📝 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.
| const isWebPlatform = !isMobilePlatform() | |
| const API_BASE_URL = isWebPlatform | |
| ? process.env.NEXT_PUBLIC_API_BASE_URL | |
| : process.env.EXPO_PUBLIC_API_BASE_URL | |
| const baseUrl = API_BASE_URL?.replace('/v1', '') | |
| return profileImage.startsWith('http') || !baseUrl ? profileImage : `${baseUrl}${profileImage}` | |
| } | |
| const isWebPlatform = !isMobilePlatform() | |
| const API_BASE_URL = isWebPlatform | |
| ? process.env.NEXT_PUBLIC_API_BASE_URL | |
| : process.env.EXPO_PUBLIC_API_BASE_URL | |
| const MEDIA_BASE_URL = isWebPlatform | |
| ? process.env.NEXT_PUBLIC_MEDIA_BASE_URL ?? process.env.NEXT_PUBLIC_API_BASE_URL?.replace(/\/v\d+$/, '') | |
| : process.env.EXPO_PUBLIC_MEDIA_BASE_URL ?? process.env.EXPO_PUBLIC_API_BASE_URL?.replace(/\/v\d+$/, '') | |
| const baseUrl = MEDIA_BASE_URL | |
| return profileImage.startsWith('http') || !baseUrl ? profileImage : `${baseUrl}${profileImage}` | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/authentication/modules/access/useLogin/login-success-handler.ts`
around lines 10 - 17, The current construction of API_BASE_URL and baseUrl in
isWebPlatform / API_BASE_URL / baseUrl is fragile because it blindly does
.replace('/v1','') on the environment value; change this to robustly derive the
media base URL by either (a) adding and using a dedicated environment variable
(e.g., NEXT_PUBLIC_MEDIA_BASE_URL / EXPO_PUBLIC_MEDIA_BASE_URL) and prefer that
for building profile image URLs, or (b) if you must strip version suffixes from
API_BASE_URL, use a safer approach such as parsing the URL and removing only a
trailing version segment (e.g., use a regex to strip only /v<digit+> at the end
or use the URL API to reconstruct origin + pathname without the trailing
version). Update the logic where baseUrl is computed and where profileImage is
resolved (references: isWebPlatform, API_BASE_URL, baseUrl, profileImage) so you
no longer rely on a brittle .replace('/v1','') operation.
| const mutation = useMutation({ | ||
| mutationFn: ({ email }) => ApiClass.recoverPassword({ email }), | ||
| ...options, // needs to be placed below all overridable options | ||
| onError: (err, variables, context) => { | ||
| options?.onError?.(err, variables, context) | ||
| mutationFn: (values: ForgotPasswordRequest) => strategy.recoverPassword(values), | ||
| ...options, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== packages/authentication/modules/access/useRecoverPassword/types.ts =="
fd '^types\.ts$' packages/authentication/modules/access/useRecoverPassword -x sed -n '1,220p' {}
echo
echo "== packages/authentication/modules/access/useRecoverPassword/index.ts =="
fd '^index\.ts$' packages/authentication/modules/access/useRecoverPassword -x sed -n '1,220p' {}
echo
echo "== Similar mutation-hook patterns under packages/authentication/modules/access =="
rg -n -C2 --type=ts 'UseMutationOptions|mutationFn:|\.\.\.options' packages/authentication/modules/accessRepository: silverlogic/baseapp-frontend
Length of output: 12527
Keep the hook-owned mutationFn authoritative.
The type UseRecoverPasswordOptions exposes the full UseMutationOptions type, which includes mutationFn. Combined with the spread placement after mutationFn in the mutation config, callers can override strategy.recoverPassword(...) and bypass the strategy layer plus this hook's error handling.
The codebase already uses Omit<UseMutationOptions<...>, 'mutationFn'> elsewhere (e.g., useAllAuthGoogleLogin). Apply the same pattern here—either by:
- Restricting the type in
UseRecoverPasswordOptions.optionsto excludemutationFn, or - Placing
mutationFnafter...optionsin the mutation config.
This issue also affects useSignUp, useChangePassword, and useResetPassword in the same directory.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/authentication/modules/access/useRecoverPassword/index.ts` around
lines 31 - 33, The hook currently allows callers to override the hook-owned
mutation function because UseRecoverPasswordOptions.options exposes
UseMutationOptions including mutationFn; update the options type to
Omit<UseMutationOptions<...>, 'mutationFn'> (matching useAllAuthGoogleLogin) so
callers cannot supply mutationFn, and ensure useRecoverPassword sets mutationFn
to (values: ForgotPasswordRequest) => strategy.recoverPassword(values) in the
useMutation config (or alternatively move mutationFn after ...options if you
prefer the other approach); apply the same type fix to useSignUp,
useChangePassword, and useResetPassword to keep the strategy layer and hook
error handling authoritative.
| const mutation = useMutation<AuthResult, AuthError | Error, TRegisterRequest>({ | ||
| mutationFn: (values) => strategy.signUp(values), | ||
| ...options, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether the public useSignUp options still expose a caller-overridable mutationFn.
sed -n '1,200p' packages/authentication/modules/access/useSignUp/types.ts
rg -n "UseMutationOptions|mutationFn" packages/authentication/modules/access/useSignUp -g '!**/__tests__/**'Repository: silverlogic/baseapp-frontend
Length of output: 1159
Move mutationFn after ...options to prevent callers from overriding the strategy.
The UseSignUpOptions.options property accepts UseMutationOptions, which includes mutationFn as a property. Since the mutation is configured with mutationFn first and then spreads ...options, a caller can override the strategy by passing { options: { mutationFn: customFunction } }. This defeats the purpose of enforcing the strategy layer.
💡 Hardening change
const mutation = useMutation<AuthResult, AuthError | Error, TRegisterRequest>({
- mutationFn: (values) => strategy.signUp(values),
...options,
+ mutationFn: (values) => strategy.signUp(values),
onError: (error: unknown, variables, context) => {📝 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.
| const mutation = useMutation<AuthResult, AuthError | Error, TRegisterRequest>({ | |
| mutationFn: (values) => strategy.signUp(values), | |
| ...options, | |
| const mutation = useMutation<AuthResult, AuthError | Error, TRegisterRequest>({ | |
| ...options, | |
| mutationFn: (values) => strategy.signUp(values), |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/authentication/modules/access/useSignUp/index.ts` around lines 41 -
43, The mutation currently passes mutationFn: (values) =>
strategy.signUp(values) before spreading ...options, allowing callers to
override it via UseSignUpOptions.options.mutationFn; change the order so that
...options is spread first and then set mutationFn: (values) =>
strategy.signUp(values) afterward (i.e., const mutation =
useMutation<AuthResult, AuthError | Error, TRegisterRequest>({ ...options,
mutationFn: (values) => strategy.signUp(values) }) ) to ensure the
strategy.signUp implementation cannot be overridden by callers.
| export function mapSignUpResponse(response: AllAuthSignupResponse): AuthResult { | ||
| return { | ||
| kind: 'success', | ||
| rawResponse: response, | ||
| metadata: { rawResponse: response }, | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check how mapSignUpResponse results are consumed
rg -n -C5 'mapSignUpResponse' --type=tsRepository: silverlogic/baseapp-frontend
Length of output: 3324
🏁 Script executed:
#!/bin/bash
# Check mapLoginResponse implementation and AuthResult type definition
# First, let's see the full mapLoginResponse function
rg -n -A 10 'export function mapLoginResponse' --type=ts
# Also check the AuthResult type definition
rg -n 'type AuthResult|interface AuthResult' --type=tsRepository: silverlogic/baseapp-frontend
Length of output: 1226
🏁 Script executed:
#!/bin/bash
# Read the full AuthResult type definition
cat -n packages/authentication/modules/auth-strategy/types.ts | sed -n '61,100p'
# Read the complete mapLoginResponse function
cat -n packages/authentication/modules/auth-strategy/allauth/utils.ts | sed -n '32,72p'Repository: silverlogic/baseapp-frontend
Length of output: 2980
🏁 Script executed:
#!/bin/bash
# Find the AuthSuccessResult interface/type definition
rg -n 'interface AuthSuccessResult|type AuthSuccessResult' --type=ts -A 15Repository: silverlogic/baseapp-frontend
Length of output: 1404
🏁 Script executed:
#!/bin/bash
# Check how signUp result is consumed and if there are issues with optional fields
rg -n 'await.*signUp\|\.signUp\(' --type=ts -A 5 | head -50
# Also check if there are type checks or optional chaining patterns
rg -n 'result\.user\|result\.session\|result\.profile' --type=ts | grep -v 'mapLoginResponse\|mapSignUpResponse'Repository: silverlogic/baseapp-frontend
Length of output: 54
🏁 Script executed:
#!/bin/bash
# Search more broadly for signUp usage
rg -n '\.signUp\s*\(' --type=ts -B 2 -A 8
# Also check auth-related hook/context files that might consume this
fd -e ts -e tsx 'auth' | grep -i 'hook\|context\|provider' | head -20Repository: silverlogic/baseapp-frontend
Length of output: 3698
🏁 Script executed:
#!/bin/bash
# Read the full useSignUp hook implementation
cat -n packages/authentication/modules/access/useSignUp/index.ts | head -80
# Also search for usage of the mutation result's user/session/profile
rg -n 'data\?.user\|data\?.session\|data\?.profile' --type=ts packages/authentication/modules/access/useSignUp/ -A 2Repository: silverlogic/baseapp-frontend
Length of output: 3049
🏁 Script executed:
#!/bin/bash
# Check the AllAuthSignupResponse type to see what data is available
rg -n 'type AllAuthSignupResponse|interface AllAuthSignupResponse' --type=ts -A 20
# Also search for other auth strategies to see how they handle signup
fd -type f -name '*strategy*.ts' packages/authentication/modules/auth-strategy/ | grep -v allauth | head -5Repository: silverlogic/baseapp-frontend
Length of output: 1671
🏁 Script executed:
#!/bin/bash
# Check AllAuthSignupData type definition
rg -n 'type AllAuthSignupData|interface AllAuthSignupData' --type=ts -A 20
# Check AllAuthResponse generic type
rg -n 'interface AllAuthResponse|type AllAuthResponse' --type=ts -A 10
# Look for other strategy implementations
ls -la packages/authentication/modules/auth-strategy/*/strategy.ts 2>/dev/null | head -10Repository: silverlogic/baseapp-frontend
Length of output: 2278
🏁 Script executed:
#!/bin/bash
# Check AllAuthMeta definition
rg -n 'interface AllAuthMeta|type AllAuthMeta' --type=ts -A 15
# Also check if there's comment/documentation about signup flow
rg -n 'signup\|signup' --type=ts packages/authentication/modules/auth-strategy/allauth/utils.ts -i -B 2 -A 2Repository: silverlogic/baseapp-frontend
Length of output: 1113
🏁 Script executed:
#!/bin/bash
# Check resolveUser and resolveProfile to understand data requirements
rg -n 'function resolveUser|function resolveProfile|const resolveUser|const resolveProfile' --type=ts -A 8
# Check for comments about signup behavior in the file
cat -n packages/authentication/modules/auth-strategy/allauth/utils.ts | head -100 | tail -50Repository: silverlogic/baseapp-frontend
Length of output: 5746
Consider populating user, session, and profile in mapSignUpResponse for consistency.
mapSignUpResponse returns only kind, rawResponse, and metadata, whereas mapLoginResponse extracts and populates user, session, and profile from the response metadata. Since AllAuthSignupResponse has the same structure as AllAuthLoginResponse (including meta with tokens), mapSignUpResponse should extract these fields using resolveUser() and resolveProfile() when available, or document why the signup flow intentionally omits authenticated data.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/authentication/modules/auth-strategy/allauth/utils.ts` around lines
74 - 80, mapSignUpResponse currently returns only kind/rawResponse/metadata
while mapLoginResponse extracts user, session, and profile; change
mapSignUpResponse to mirror mapLoginResponse by extracting and populating user
(using resolveUser(response.meta?.user)), session (from response.meta?.tokens or
similar), and profile (using resolveProfile(response.meta?.profile)) when
response.meta exists, falling back to undefined if not present, and still
include rawResponse and metadata; reference mapSignUpResponse, mapLoginResponse,
resolveUser, and resolveProfile when making the change.
| async function clear(reason?: AuthSessionClearedEvent['reason']): Promise<void> { | ||
| await storage.clear() | ||
|
|
||
| if (reason) { | ||
| emitCleared(reason) | ||
| } |
There was a problem hiding this comment.
Always publish a cleared signal from clear().
useSession() only invalidates its cache on AUTH_SESSION_CLEARED / AUTH_SESSION_REFRESHED. Because clear() skips AUTH_SESSION_CLEARED when reason is omitted, any plain clear() call can leave consumers showing the old authenticated state until some unrelated refetch happens.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/authentication/session/service.ts` around lines 34 - 39, The clear
function currently only calls emitCleared(reason) when a reason is provided,
which prevents AUTH_SESSION_CLEARED from being published on plain clear() calls;
update the clear implementation (function clear in session/service.ts) to always
call emitCleared, passing the provided reason if present or a sensible default
(e.g., 'manual' or 'unknown') that matches AuthSessionClearedEvent['reason'] so
consumers relying on AUTH_SESSION_CLEARED will always see the cleared signal.
| if (!currentSession.refreshToken) { | ||
| await clear('missing_refresh_token') | ||
| return strategySession.evaluate(currentSession) |
There was a problem hiding this comment.
Return the persisted state after clearing storage.
After clear('missing_refresh_token') or clear('refresh_failed'), reevaluating currentSession can still produce authenticated when the old JWT is locally valid. That makes refresh() / handleUnauthorized() disagree with the storage you just wiped.
💡 Suggested fix
if (!currentSession.refreshToken) {
await clear('missing_refresh_token')
- return strategySession.evaluate(currentSession)
+ return getState()
}
@@
} catch {
await clear('refresh_failed')
- return strategySession.evaluate(currentSession)
+ return getState()
}Also applies to: 65-67
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/authentication/session/service.ts` around lines 49 - 51, When you
call clear('missing_refresh_token') or clear('refresh_failed') you must return
the persisted/cleared session state instead of re-evaluating the old
currentSession; after calling clear(...) retrieve the session state from the
persistence layer (via the strategy/session API used in this file — e.g., call
the strategySession method that reads persisted state or a
getPersisted/getStoredSession equivalent) and return that result instead of
strategySession.evaluate(currentSession) so refresh() / handleUnauthorized() see
the cleared state; update both occurrences (the block using
clear('missing_refresh_token') and the similar block for 'refresh_failed') to
await the persisted-state getter and return it.
| throw new Error( | ||
| `Error fetching GraphQL query '${request.name}' with variables '${JSON.stringify( | ||
| variables, | ||
| )}': ${JSON.stringify(errors)}`, |
There was a problem hiding this comment.
Do not include raw GraphQL variables in this error message.
This message currently serializes the full variables payload. Auth mutations can carry passwords, reset tokens, or email addresses, so this can leak secrets/PII once the error is logged upstream.
🛡️ Proposed fix
if (errors.length > 0) {
throw new Error(
- `Error fetching GraphQL query '${request.name}' with variables '${JSON.stringify(
- variables,
- )}': ${JSON.stringify(errors)}`,
+ `Error fetching GraphQL query '${request.name}': ${JSON.stringify(errors)}`,
)
}📝 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.
| throw new Error( | |
| `Error fetching GraphQL query '${request.name}' with variables '${JSON.stringify( | |
| variables, | |
| )}': ${JSON.stringify(errors)}`, | |
| throw new Error( | |
| `Error fetching GraphQL query '${request.name}': ${JSON.stringify(errors)}`, | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/graphql/config/environment.ts` around lines 162 - 165, The error
currently includes the raw GraphQL variables (variables) when throwing an Error
for `request.name`, which can leak secrets/PII; instead, remove or redact
variable values before including them in the message (e.g., replace values with
"[REDACTED]" or only include variable keys), or call a small sanitizer like
`sanitizeVariables(variables)` and use that sanitized output in the Error
string; update the throw site that references `request.name`, `variables`, and
`errors` to use the sanitized representation (or omit variables entirely) so no
raw payload values are logged.
| if (!headers.Authorization && resolvedAccessToken && isAuthTokenRequired) { | ||
| headers.Authorization = `${tokenType} ${resolvedAccessToken}` |
There was a problem hiding this comment.
Skip session recovery when the request did not use the managed session token.
prepareFetchOptions() intentionally preserves a caller-supplied Authorization header and also supports accessTokenOverride, but shouldRetryRequest() still treats any 401 as a session failure. A bad API key / override token can therefore refresh or clear the user's session, and the retry still goes out with the same bad credential.
💡 Suggested fix
async function shouldRetryRequest({
response,
hasRetried,
isAuthTokenRequired,
isServer,
+ usesManagedSessionToken,
accessKeyName,
resolvedAccessToken,
path,
refreshAuthToken,
}: {
response: Response
hasRetried: boolean
isAuthTokenRequired: boolean
isServer: boolean
+ usesManagedSessionToken: boolean
accessKeyName: string
resolvedAccessToken: string | null | undefined
path: string
refreshAuthToken: string | null | undefined
}): Promise<boolean> {
- if (response.status !== 401 || hasRetried || !isAuthTokenRequired || isServer) {
+ if (
+ response.status !== 401 ||
+ hasRetried ||
+ !isAuthTokenRequired ||
+ isServer ||
+ !usesManagedSessionToken
+ ) {
return false
}
@@
async function executeRequest(hasRetried = false): Promise<any> {
+ const usesManagedSessionToken =
+ accessTokenOverride == null && !options.headers?.Authorization
+
const currentProfile = await resolveCurrentProfile(isServer)
@@
const retry = await shouldRetryRequest({
response,
hasRetried,
isAuthTokenRequired,
isServer,
+ usesManagedSessionToken,
accessKeyName,
resolvedAccessToken,
path,
refreshAuthToken,
})Also applies to: 114-150, 266-298
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/utils/functions/fetch/baseAppFetch/index.ts` around lines 99 - 100,
The 401-handling currently treats any 401 as a managed-session failure even when
the request used a caller-supplied Authorization or an accessTokenOverride;
update prepareFetchOptions to mark whether the Authorization header was set from
the managed session (i.e., set a boolean like usedManagedToken when you assign
headers.Authorization = `${tokenType} ${resolvedAccessToken}`) and thread that
flag into shouldRetryRequest so it only triggers session recovery/clear when
usedManagedToken is true; adjust the other similar blocks (lines ~114-150 and
~266-298) to set and propagate the same flag so retry logic ignores failures
caused by external/bad credentials or overrides.
Hercilio1
left a comment
Hercilio1
left a comment
There was a problem hiding this comment.
In general all good. Leaving one review. But also take a look into the coderabbit and sonar reviews. I believe most of them should be fixed within this PR.
| export async function httpFetch( | ||
| request: RequestParameters, | ||
| variables: Variables, | ||
| cacheConfig?: CacheConfig, |
There was a problem hiding this comment.
We don't seem to be using cacheConfig anymore. Should we keep or remove it? If keeping, maybe add a comment explaining why?
|
2 participants
Summary by CodeRabbit
New Features
useSessionhook for accessing authentication state across the appBug Fixes
Documentation
Breaking Changes
useJWTUserwithuseSessionhook