Conversation
📝 WalkthroughWalkthroughThe login flow was refactored: the server action now accepts explicit Changes
Sequence DiagramsequenceDiagram
participant User as User
participant UI as SignIn Component
participant Action as loginAction
participant API as ServerAuthApi
participant Modal as Modal System
User->>UI: submit(login, password)
activate UI
UI->>Action: loginAction(login, password)
activate Action
Action->>API: signIn(login, password)
activate API
API-->>Action: success or ApiErrorPayload
deactivate API
alt success
Action-->>UI: token & user
UI->>UI: analytics, update user context, close modal, redirect
else error
Action-->>UI: ApiErrorPayload
UI->>UI: set submitErrors, clear password
alt account inactive
UI->>Modal: open accountInactive
else other errors
UI->>UI: display field/submit errors
end
end
deactivate Action
deactivate UI
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
front_end/src/app/(main)/accounts/actions.ts (2)
41-55:⚠️ Potential issue | 🟠 MajorNon-
ApiErrorexceptions result in silent failure — no error shown and no success.When the caught error is not an
ApiError, the function returns{ errors: undefined }. On the caller side (signin.tsx),state?.errorsis falsy so the error branch is skipped, andstate?.useris also undefined so the success branch is skipped. The user sees the form stop loading with zero feedback.Return a generic error message for the non-
ApiErrorpath:Proposed fix
} catch (err: unknown) { return { errors: ApiError.isApiError(err) ? (err.data as ApiErrorPayload) - : undefined, + : { detail: "Something went wrong. Please try again." }, }; }
57-73:⚠️ Potential issue | 🟠 MajorAdd
revalidatePathafter successful login inloginAction.
loginAction(lines 57-73) sets auth tokens but does not callrevalidatePath("/"), whilesignUpAction(line 131) does. Without cache revalidation after setting tokens, Next.js may continue serving cached server-rendered pages with stale auth state (e.g., showing the "Log In" button instead of the user profile) until the cache naturally expires.Proposed fix
const authManager = await getAuthCookieManager(); authManager.setAuthTokens(response.tokens); + revalidatePath("/"); // Set user's language preference as the active locale
🤖 Fix all issues with AI agents
In `@front_end/src/components/auth/signin.tsx`:
- Around line 54-56: The branch handling login errors (the if (state?.errors)
block inside the submit handler in signin.tsx) contradicts the PR goal to
preserve the password: remove the call to resetField("password") so
setSubmitErrors(state.errors as ErrorResponse) does not clear the password;
alternatively, if clearing password was intended, update the PR description to
reflect that behavior—locate the error branch around setSubmitErrors and remove
the resetField("password") call to keep the password value on failed attempts.
- Around line 54-55: Remove the unnecessary cast to ErrorResponse when assigning
state.errors: instead of forcing setSubmitErrors(state.errors as ErrorResponse)
allow submitErrors to hold the true union type (FieldErrorsFrom<...> |
ApiErrorPayload) or narrow at use-site; update the submitErrors state type to
the union (or change the set call to perform a runtime check on state.errors
like the existing checks at lines 59–61) so you no longer mask structural
differences between form validation errors and API payloads—refer to
state.errors, setSubmitErrors, submitErrors, ErrorResponse, FormErrorMessage,
and extractError when making this change.
🧹 Nitpick comments (4)
front_end/src/app/(main)/accounts/actions.ts (1)
35-39:LoginActionState.errorstype includesFieldErrorsFrom<typeof signInSchema>which is now dead.Since
loginActionno longer performs schema validation and never returnsFieldErrorsFrom, that branch of the union (and transitively thesignInSchemaimport usage in this type, plus theFieldErrorsFromhelper) is unreachable fromloginAction. Consider narrowing the type to justApiErrorPayloadif no other consumers rely on the old shape.front_end/src/components/auth/signin.tsx (3)
58-69: Fragile inactive-account detection — consider a dedicated field or error code.The check casts to
{ user_state?: string }and string-compares"inactive". This is tightly coupled to a specific API response shape. If the backend ever renames the field or changes the value, this silently breaks.Consider having
loginActionreturn a structured discriminant (e.g.,isInactive: boolean) instead of pushing API-response introspection into the UI component.
63-68:setCurrentModal(null)immediately beforesetCurrentModal({...})is likely unnecessary.React 18+ batches state updates within the same synchronous block, so the intermediate
nullvalue will never be rendered. If the goal is to force a modal remount, akeyprop on the modal component would be more explicit and reliable.
49-82:setSubmitErrorsmissing fromuseCallbackdependency array.Line 80 lists
[setCurrentModal, setUser, handlePostLoginAction, resetField]but omitssetSubmitErrorsandloginAction. React'suseStatesetters are identity-stable so this won't cause a bug today, but thereact-hooks/exhaustive-depslint rule will flag it.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@front_end/src/components/auth/signin.tsx`:
- Line 38: Replace the incorrect ErrorResponse typing on submitErrors with the
actual union returned by loginAction: change the useState generic for
submitErrors to match LoginActionState['errors'] (e.g., use the conditional type
typeof loginAction extends Promise<LoginActionState> ?
NonNullable<LoginActionState>['errors'] : never or explicitly
FieldErrorsFrom<typeof signInSchema> | ApiErrorPayload) so that
submitErrors/setSubmitErrors and checks like state.errors.user_state align with
the runtime shape; update the declaration that currently references submitErrors
and setSubmitErrors accordingly (symbols: submitErrors, setSubmitErrors,
loginAction, LoginActionState, signInSchema, FieldErrorsFrom, ApiErrorPayload).
🧹 Nitpick comments (2)
front_end/src/components/auth/signin.tsx (2)
49-81: Well-structuredonSubmitwith clear error/success branching.The refactor from
useActionState/useTransitionto auseCallback+useServerActionpattern is clean and easier to follow. Error handling, inactive-account detection, and success-path logic are all clearly separated.Two minor observations:
Lines 64–65 — double
setCurrentModalcall.setCurrentModal(null)immediately followed bysetCurrentModal({type: "accountInactive", …})will be batched by React 18+, so the intermediatenullvalue is never rendered. If the intent is to force-close the current modal before opening the new one (e.g., triggering anonCloseside-effect), verify that this batching doesn't skip that side-effect. If it's just "replace the current modal," thenullcall is redundant.Line 80 —
setSubmitErrorsomitted from the dependency array. Safe at runtime (React guaranteesuseStatesetters are stable), butreact-hooks/exhaustive-depswill likely warn. Consider adding it for lint compliance, or suppressing with a comment if already configured.
58-69: Inactive-account narrowing uses a type assertion that can be avoided.After the
typeofandinchecks on lines 59–60, TypeScript still doesn't narrowstate.errorsenough, so line 61 usesas { user_state?: string }. This is safe given the preceding guards, but you can avoid the assertion with a small helper or by indexing directly:Suggested simplification
- const isInactive = - typeof state.errors === "object" && - "user_state" in state.errors && - (state.errors as { user_state?: string }).user_state === "inactive"; + const isInactive = + typeof state.errors === "object" && + state.errors !== null && + "user_state" in state.errors && + (state.errors as Record<string, unknown>).user_state === "inactive";Or extract a type-guard:
function hasUserState(obj: unknown): obj is { user_state: string } { return typeof obj === "object" && obj !== null && "user_state" in obj && typeof (obj as Record<string, unknown>).user_state === "string"; }
🚀 Preview EnvironmentYour preview environment is ready!
Details
ℹ️ Preview Environment InfoIsolation:
Limitations:
Cleanup:
|
1 participant
useActionStatewithuseServerActionSummary by CodeRabbit
New Features
Bug Fixes