refactor: login, user profile, dashboard#129
Merged
gaboesquivel merged 16 commits intomainfrom Mar 10, 2026
Merged
Conversation
- Add @faker-js/faker and generate funny random name for new users on magic link signup - Include user name in session (auth plugin, api-key-auth) and session/user response - Pass initialError to LoginForm in LoginActions for consistent error display
- Collocate login-form.tsx in apps/next/app/auth/login/ per route rules - Remove packages/react/src/components entirely (LoginForm, spec) - Update login-actions to import LoginForm from ./login-form - Add cursor rule and docs: @repo/react hooks/helpers only, no UI - Update package-conventions, frontend-stack, packages.mdx - Update packages/react README with hooks-only constraint
- Add generateLoginCode() in jwt.ts (100000–999999)
- Email template: loginCode prop, code display + sign-in button
- Request route: 6-digit code, APP_NAME env, subject {code} - verification code
- Verify route: 6-digit format validation (400 if invalid)
- Login form: code-entry view after email sent, useMagicLinkVerify hook
- Remove defaultEmail from LoginForm/LoginActions
- E2E: manual code entry flow, enterLoginCodeAndSubmit helper
- Docs: update magic link flow (dual path: code entry + link click)
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughSwitches magic-link auth from token URLs to 6‑digit codes with verificationId and IP-based attempt tracking; adds unique username generation and username column; introduces profile PATCH API and profile UI; moves security tabs to path segments; removes LoginForm from the React package in favor of hooks; updates OpenAPI, DB schemas, tests, and client hooks. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Frontend as Next.js Frontend
participant Backend as Fastify Backend
participant Email as Email Service
participant Database as Database
rect rgba(200,150,255,0.5)
Note over User,Email: Magic-link auth (6-digit code / verificationId)
end
User->>Frontend: Submit email
Frontend->>Backend: POST /auth/magiclink/request { email }
Backend->>Database: SELECT user by email
alt user exists
Database-->>Backend: user row
else new user
Backend->>Backend: generateFunnyUsername()
Backend->>Database: INSERT user with username
Database-->>Backend: created user
end
Backend->>Backend: generateLoginCode() -> code
Backend->>Database: insert verification record (hash + verificationId, expires)
Backend->>Email: send email { loginCode: code, magicLink(with verificationId) }
Email-->>User: email with code + link
User->>Frontend: Enter code (or click link)
Frontend->>Backend: POST /auth/magiclink/verify { token: code, verificationId?, email? }
Backend->>Database: lookup verification by id or email and validate hash/expiry
alt valid & not expired
Backend->>Backend: create session, issue tokens
Backend-->>Frontend: { token, refreshToken }
else invalid/expired or rate-limited
Backend-->>Frontend: error (400/401/404/429)
end
sequenceDiagram
participant User
participant Frontend as Next.js Frontend
participant Backend as Fastify Backend
participant Database as Database
rect rgba(150,200,255,0.5)
Note over Frontend,Database: Profile update flow (username uniqueness)
end
User->>Frontend: Edit name/username and Save
Frontend->>Backend: PATCH /account/profile { name?, username? }
Backend->>Database: if username provided -> SELECT users WHERE username=...
alt username taken
Database-->>Backend: found
Backend-->>Frontend: 409 USERNAME_TAKEN
else username free or omitted
Backend->>Database: UPDATE users SET ...
Database-->>Backend: updated user
Backend-->>Frontend: 200 { user }
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 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: 14
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
apps/next/e2e/auth-helpers.ts (1)
123-131:⚠️ Potential issue | 🟡 Minor
loginAsTestUserstill bypasses the code-entry flow.After waiting for the new "Check your email" step, this helper still pulls the backend token/code and hits the callback route directly. That means the e2e happy path never exercises
enterLoginCodeAndSubmit, so regressions in the new verification UI can stay green.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/next/e2e/auth-helpers.ts` around lines 123 - 131, loginAsTestUser currently skips the UI code-entry flow by extracting the backend token and calling verifyMagicLink directly; change it to exercise the real verification UI by keeping sendMagicLink and waiting for the "Check your email" heading, then use extractToken to get the code and call enterLoginCodeAndSubmit(page, token) (instead of verifyMagicLink) so the e2e path types the code into the UI and submits it; ensure you still assert response.status() === 200, wait for the successMessage visibility, keep short delays if needed, and remove the direct call to verifyMagicLink to fully exercise the enterLoginCodeAndSubmit flow.apps/fastify/src/routes/auth/session/user.ts (1)
99-105:⚠️ Potential issue | 🟠 MajorThe session object is not refreshed after profile updates, causing stale data.
The
/account/profile/updateroute updates the database and returns the fresh values to the caller, but it does not refreshrequest.session.user. Since/auth/session/userservesnameandusernamefrom the session object (which is only populated during authentication), users will see stale profile values until they sign in again.Either refresh
request.session.userafter the database update, or fetchname/usernamefrom the database in the/auth/session/userendpoint.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/fastify/src/routes/auth/session/user.ts` around lines 99 - 105, The /auth/session/user route is returning name/username from request.session.user which can be stale after /account/profile/update; either refresh the session after updating the DB or read the latest profile from the DB in this endpoint. Fix option A: in the /account/profile/update handler, after performing the DB update, assign the updated name and username to request.session.user (e.g., request.session.user.name = updated.name; request.session.user.username = updated.username) and persist the session (call the session store save/commit method your app uses). Fix option B: modify the /auth/session/user handler to ignore request.session.user.name/username and instead query the user record by request.session.user.id to return the freshest name and username. Use the identifiers request.session.user, /account/profile/update handler, and /auth/session/user handler to locate the code.apps/fastify/src/routes/auth/magiclink/request.ts (1)
72-89:⚠️ Potential issue | 🟡 MinorPer-email brute-force protection is missing for the 6-digit code verification.
The 6-digit code is protected by single-use enforcement (deletion after verification) and expiration, which mitigates risk. However, with only 1,000,000 possibilities and a global rate limiter (not per-email), an attacker could still distribute brute-force attempts across the validity window. Consider adding explicit per-email/IP attempt tracking in
/auth/magiclink/verifywith progressive backoff or temporary lockout after N failures.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/fastify/src/routes/auth/magiclink/request.ts` around lines 72 - 89, The magic-link flow currently creates a 6-digit code (generateLoginCode, hashToken) but lacks per-email brute-force protection; add per-identifier attempt tracking (either fields on the verification row inserted via db.insert(verification) such as failedAttempts, firstFailureAt, lockedUntil or a lightweight attempts table keyed by identifier/IP) and enforce checks in the /auth/magiclink/verify handler: on each failed verification increment failedAttempts and set lockedUntil with progressive backoff after configurable thresholds, reject attempts while locked, and reset failedAttempts/lockedUntil on successful verification or when lock expires; ensure tokenPlain handling and single-use deletion still occur after a successful verify.
🟡 Minor comments (7)
apps/next/package.json-43-43 (1)
43-43:⚠️ Potential issue | 🟡 MinorSentry's built-in scrubbing handles auth/session headers by default; verify edge case scrubbing in staging.
Line 43 adds
@sentry/nextjs, and this PR touches auth/profile flows. Sentry's built-in PII scrubbing automatically strips cookies, authorization headers, and session tokens per error-handling.mdx, so no custombeforeSendhook is required by default. However, verify that sensitive identifiers or domain-specific auth context are not captured in breadcrumbs or console logs in staging before merge.Minor:
instrumentation.tsimports.jsfiles, but source files are.ts—verify this is handled correctly at build time or update imports to reference.tsextensions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/next/package.json` at line 43, Package addition of "@sentry/nextjs" is fine because Sentry's default PII scrubbing covers auth/session headers; verify in staging that no sensitive identifiers are captured in breadcrumbs or console logs and remove any custom beforeSend hooks if they only duplicate this behavior. Also inspect instrumentation.ts where imports reference ".js"—either update those import paths to ".ts" (or remove the explicit extension) or confirm the build/bundler (Next.js/TS config) resolves .ts -> .js correctly so the instrumentation imports work at runtime. Ensure tests or manual checks in staging validate auth/profile flows and that Sentry events do not contain cookies, Authorization headers, session tokens, or domain-specific identifiers.apps/next/lib/auth/auth-utils.ts-10-10 (1)
10-10:⚠️ Potential issue | 🟡 MinorRequire
user.idonce a session user is present.
getUserInfo()already returnsnullfor unauthenticated or invalid responses. Makingidoptional turns a backend contract regression into partial authenticated state, which is exactly the kind of profile/dashboard breakage this refactor wants to avoid.Suggested change
- id: z.string().optional(), + id: z.string(), @@ - id?: string + id: stringAs per coding guidelines, "ensure strict type narrowing when handling user/profile data."
Also applies to: 45-45
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/next/lib/auth/auth-utils.ts` at line 10, The schema currently declares id as optional ("id: z.string().optional()") which allows a partially authenticated state; change the Zod schema(s) so that id is required (use z.string() without .optional()) wherever present (the occurrence around the id field and the other instance mentioned) so getUserInfo()/session user handling always yields a defined id for authenticated users and enables strict type narrowing in downstream code.apps/fastify/src/routes/auth/magiclink/request.test.ts-90-90 (1)
90-90:⚠️ Potential issue | 🟡 MinorAssert that the subject code and link code are the same value.
These assertions only verify that each value looks like six digits. A rendering bug could put one code in the subject and a different code in the URL and this suite would still pass. Capture the subject prefix and compare it to
extractToken()in the same test.Suggested change
- expect(sentEmail?.subject).toMatch(/^\d{6} - .* verification code$/) + const subjectCode = sentEmail?.subject.match(/^(\d{6}) - .* verification code$/)?.[1] + expect(subjectCode).toBeTruthy() const magicLink = fastify.fakeEmail?.extractMagicLink(sentEmail) expect(magicLink).toBeTruthy() expect(magicLink).toContain('token=') + expect(fastify.fakeEmail?.extractToken()).toBe(subjectCode)Also applies to: 132-132
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/fastify/src/routes/auth/magiclink/request.test.ts` at line 90, Capture the six-digit prefix from sentEmail.subject (use the existing regex match) and assert it equals the token returned by extractToken(sentEmail.body or sentEmail.html) instead of only matching digit patterns; update both occurrences that currently only check /^\d{6} - .* verification code$/ (the one referencing sentEmail and the other at the second location) so they extract the numeric prefix (e.g., via match()[1]) and compare strict equality to extractToken(...) in those tests.packages/react/README.md-7-7 (1)
7-7:⚠️ Potential issue | 🟡 MinorUpdate the Exports section to document all public hooks.
The Exports section (lines 9–17) lists only 7 items, but
packages/react/src/index.tsexports 24+ hooks. Missing documentation includesuseMagicLinkVerify(magic link verification),useProfileUpdate(profile mutations), and many authentication/profile hooks (useOAuthLogin,usePasskeyAuth,useTotpSetup,useUser, etc.). Consumers cannot discover available functionality from the README.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/README.md` at line 7, The Exports section in packages/react README is incomplete; update it to list and briefly describe all public hooks exported by packages/react/src/index.ts (notably add useMagicLinkVerify, useProfileUpdate, useOAuthLogin, usePasskeyAuth, useTotpSetup, useUser and the other auth/profile hooks) so consumers can discover functionality; scan the export list in src/index.ts, add each exported hook name with a short one-line description next to it in the README Exports section, and ensure naming and spelling match the identifiers in src/index.ts exactly.apps/docu/content/docs/architecture/authentication.mdx-425-429 (1)
425-429:⚠️ Potential issue | 🟡 MinorUse
callbackUrlconsistently in the diagram.This switches to
callbackURL, but the rest of the page and the surrounding auth examples usecallbackUrl. The casing mismatch makes the callback flow harder to follow.As per coding guidelines, "Align documentation with code changes across apps/docs related to authentication flow (magic link vs code)."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/docu/content/docs/architecture/authentication.mdx` around lines 425 - 429, The diagram uses callbackURL in the GET request but the docs and examples use callbackUrl; update the diagram to use callbackUrl consistently (e.g., change "GET /auth/callback/magiclink?token={code}&callbackURL=..." to use callbackUrl) and verify other occurrences in the same diagram lines (CallbackPage and redirect to callbackURL) are changed to callbackUrl so the flow (CallbackPage, FastifyAPI) matches the rest of the authentication docs.apps/next/app/(dashboard)/settings/(profile)/profile-section.tsx-143-147 (1)
143-147:⚠️ Potential issue | 🟡 MinorDon't advertise avatar upload before wiring it up.
This copy says the avatar can be clicked to upload a file, but nothing in this section is actually interactive. Users can't complete the action the UI promises here.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/next/app/`(dashboard)/settings/(profile)/profile-section.tsx around lines 143 - 147, The UI text under the "Avatar" heading currently promises a clickable upload but no upload UI exists; either remove the misleading copy or implement a real upload flow. To fix, in profile-section.tsx update the paragraph node that reads "Click on the avatar to upload a custom one from your files." (the <p className="text-muted-foreground text-sm"> element) to neutral copy (e.g., "Add an avatar to personalize your profile.") OR implement an interactive avatar upload by adding an <input type="file" accept="image/*" /> tied to a new handler (e.g., handleAvatarUpload) and state for preview; wire the input to an Avatar/preview element, call your upload API (e.g., uploadAvatar) on change, handle validation/progress/errors, and ensure accessible labels for the clickable avatar area so the UI no longer misleads users.apps/next/app/auth/login/login-form.tsx-33-40 (1)
33-40:⚠️ Potential issue | 🟡 MinorRestore email state when navigating back from code entry.
The email is cleared when transitioning to the code-entry step (line 74), but
handleBackToEmail(line 166) does not restore it. Users must re-enter their email if they go back, which is avoidable friction.Consider preserving the email value across the two-step flow:
- Remove
setEmail('')from line 74- Optionally clear it only on explicit success or hard reset scenarios
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/next/app/auth/login/login-form.tsx` around lines 33 - 40, The form clears the email when moving to the code-entry step which forces users to re-type it; remove the call that does setEmail('') when switching to the "enter code" view (the code that clears email during the transition) and ensure handleBackToEmail does not clear or overwrite the existing email state; instead only clear email on explicit success (e.g., after onVerifySuccess resolves) or a hard reset action so the component-level email state is preserved across the two-step flow (refer to the email state variable, the transition code that currently calls setEmail(''), and the handleBackToEmail handler).
🧹 Nitpick comments (14)
apps/next/app/(dashboard)/settings/security/page.tsx (1)
3-4: Add an explicit return type to the exported page.This page relies on inference today. Since
redirect()never returns, annotating the component asnevermakes the control flow explicit and keeps it aligned with the repo’s TS export rule.♻️ Suggested change
-export default function SecurityPage() { +export default function SecurityPage(): never { redirect('/settings/security/passkeys') }As per coding guidelines, "Apply explicit return types on exported functions/APIs".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/next/app/`(dashboard)/settings/security/page.tsx around lines 3 - 4, The exported function SecurityPage currently relies on type inference; explicitly annotate its return type as never since it always calls redirect() which doesn't return—update the function signature export default function SecurityPage(): never { ... } and keep the existing redirect('/settings/security/passkeys') call so the control flow and repo TS export rule are satisfied.apps/next/app/(dashboard)/page-title.tsx (2)
15-19: Add an explicit return type to the exported component.
PageTitleis part of the module’s public surface, so its return type should be declared instead of inferred.✏️ Proposed fix
+import type { JSX } from 'react' import { usePathname } from 'next/navigation' @@ -export function PageTitle() { +export function PageTitle(): JSX.Element | null {As per coding guidelines, "Apply explicit return types on exported functions/APIs."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/next/app/`(dashboard)/page-title.tsx around lines 15 - 19, The exported function PageTitle currently has an inferred return type; update its signature to declare an explicit return type (e.g., JSX.Element | null) so the public API is typed explicitly. Locate the PageTitle function (which uses usePathname and pageTitles) and change its declaration to include the explicit return type, ensuring any imports for JSX types are present if needed.
5-13: Colocate route titles with their routes.Adding more pathname entries here keeps growing a shared registry that can drift from the App Router structure. Prefer defining the title next to each page/layout and reading that route-local source instead of extending a central map.
As per coding guidelines, "Route collocation: place route-specific UI logic under the route path (e.g., auth/login under apps/next)."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/next/app/`(dashboard)/page-title.tsx around lines 5 - 13, The centralized pageTitles map in page-title.tsx (pageTitles) should be removed and each route should define its own title adjacent to its page/layout; update each route folder (e.g., app/, app/markets, app/settings, app/settings/security, etc.) to export a route-local title (use Next.js metadata export or `export const title = '...'` in the page/layout file) and change any consumers of pageTitles to import the route-local title or read metadata from the current route; delete the entries for '/', '/markets', '/settings', '/settings/security', '/settings/security/passkeys', '/settings/security/totp', and '/settings/security/apikeys' from pageTitles and refactor code that referenced pageTitles to resolve titles from the new per-route exports (or Next metadata) instead.apps/next/app/(dashboard)/settings/security/totp/page.tsx (1)
3-4: Add an explicit return type to the page export.This keeps the route component aligned with the repo’s strict TypeScript convention.
♻️ Suggested cleanup
+import type { ReactElement } from 'react' import { TotpCard } from '../totp-card' -export default function TotpPage() { +export default function TotpPage(): ReactElement { return <TotpCard /> }As per coding guidelines, "Apply explicit return types on exported functions/APIs; use CamelCase for variables; avoid enums."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/next/app/`(dashboard)/settings/security/totp/page.tsx around lines 3 - 4, The exported TotpPage function lacks an explicit return type; update the TotpPage signature to include a proper React return type (e.g., JSX.Element or React.ReactNode / React.ReactElement) to satisfy the repo's strict TypeScript convention, keeping the implementation returning <TotpCard /> unchanged and ensuring any necessary React types are imported where TotpPage is defined.apps/next/app/(dashboard)/settings/security/passkeys/page.tsx (1)
3-4: Add an explicit return type to the page export.This keeps the route component aligned with the repo’s strict TypeScript convention.
♻️ Suggested cleanup
+import type { ReactElement } from 'react' import { PasskeysCard } from '../passkeys-card' -export default function PasskeysPage() { +export default function PasskeysPage(): ReactElement { return <PasskeysCard /> }As per coding guidelines, "Apply explicit return types on exported functions/APIs; use CamelCase for variables; avoid enums."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/next/app/`(dashboard)/settings/security/passkeys/page.tsx around lines 3 - 4, The exported page component PasskeysPage lacks an explicit return type; update its signature to include an explicit return type (e.g., change export default function PasskeysPage() to export default function PasskeysPage(): React.JSX.Element) so the route component follows the repo's strict TypeScript convention and keeps the API explicitly typed.apps/next/app/(dashboard)/settings/security/layout.tsx (1)
1-8: Type the exported layout explicitly.This keeps the shared route wrapper aligned with the repo’s strict TypeScript convention and makes the prop types a bit cleaner.
♻️ Suggested cleanup
+import type { ReactElement, ReactNode } from 'react' import { SecurityTabs } from './security-tabs' -export default function SecurityLayout({ children }: { children: React.ReactNode }) { +export default function SecurityLayout({ children }: { children: ReactNode }): ReactElement { return ( <div className="mx-auto max-w-2xl space-y-6"> <SecurityTabs /> <div className="mt-6">{children}</div> </div> ) }As per coding guidelines, "Apply explicit return types on exported functions/APIs; use CamelCase for variables; avoid enums."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/next/app/`(dashboard)/settings/security/layout.tsx around lines 1 - 8, The exported SecurityLayout function is missing an explicit return type; update the SecurityLayout signature to include explicit prop and return types (e.g., type the props as { children: React.ReactNode } and add a return type like JSX.Element or React.FC<{children: React.ReactNode}>) so the exported layout follows the project's strict TypeScript convention and keeps the prop types explicit.apps/fastify/src/lib/env.ts (1)
73-73: TightenAPP_NAMEvalidation before it reaches auth emails.
APP_NAMEis now user-facing. As written, whitespace or the dev placeholder can make it to production and yield generic verification emails. Prefer trimming and rejecting the placeholder in production so misconfiguration fails at boot.Suggested change
+const appNameSchema = isProduction + ? z + .string() + .trim() + .min(1) + .refine(val => val !== 'Your App', 'APP_NAME must be configured in production') + : z.string().trim().min(1).default('Your App') + export const env = createEnv({ server: { @@ - APP_NAME: z.string().default('Your App'), + APP_NAME: appNameSchema,As per coding guidelines, "Use types and interfaces; prefer type-first validation (Zod-first)."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/fastify/src/lib/env.ts` at line 73, The APP_NAME zod schema currently allows whitespace and the dev placeholder; update the APP_NAME entry in env.ts (the APP_NAME z.string() definition) to trim input, enforce a non-empty minimum, and add a refinement that rejects the placeholder value (e.g., "Your App") when running in production (NODE_ENV === 'production'), so misconfiguration fails at boot rather than leaking into emails. Use zod transform/trim and .min(1) plus .refine(...) referencing process.env.NODE_ENV to enforce the production check on the APP_NAME schema.apps/fastify/src/routes/auth/oauth/facebook/exchange.ts (1)
169-180: Only generate a username when a user insert is actually needed.
generateFunnyUsername(db)runs before you know whether the insert will happen, so every returning Facebook login pays for an extra username lookup even thoughonConflictDoNothing({ target: users.email })turns the insert into a no-op. Check for the user first, or generate the username only after you've determined a create is needed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/fastify/src/routes/auth/oauth/facebook/exchange.ts` around lines 169 - 180, Currently generateFunnyUsername(db) is called unconditionally before the insert, causing unnecessary lookups even when onConflictDoNothing prevents creation; change the flow to first check for an existing user with db.select().from(users).where(eq(users.email, email)) and only call generateFunnyUsername(db) and perform db.insert(users)...values({...}).onConflictDoNothing({ target: users.email }) when no user is found (or alternatively perform the insert and, if it returns zero affected rows, then select the existing user), ensuring generateFunnyUsername is invoked only when a new user record will actually be created.packages/react/src/hooks/use-profile-update.ts (1)
13-16: Reuse the generated request type for this public hook.The inline
{ name?: string; username?: string | null }creates a second API contract here. If the profile endpoint schema changes, this hook can drift silently. Prefer importing the generated body type from@repo/coreand reusing it inmutationFn.As per coding guidelines, "Use types and interfaces; prefer type-first validation (Zod-first)." and "Always export/import with named exports where possible; maintain clear public API."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/src/hooks/use-profile-update.ts` around lines 13 - 16, The inline inline request type in mutationFn should be replaced by the generated request/body type exported from `@repo/core` to avoid drifting API contracts; add a named import (e.g., import { ProfileUpdateRequest } from '@repo/core') and change the mutationFn signature to use that imported type (mutationFn: (body: ProfileUpdateRequest) => client.account.profile({ body, throwOnError: true })), ensuring you use the named export from `@repo/core` and preserve existing nullability/optional semantics from the generated type.apps/fastify/src/routes/auth/magiclink/verify.ts (1)
33-45: Push the 6-digit constraint intoVerifySchema.Right now Fastify accepts any string at the TypeBox/schema layer and only rejects it inside the handler, so the autogenerated contract and runtime validation can drift.
Suggested change
const VerifySchema = Type.Object({ - token: Type.String(), + token: Type.String({ pattern: '^\\d{6}$' }), }) @@ - if (!/^\d{6}$/.test(token)) - return reply.code(400).send({ - code: 'INVALID_TOKEN', - message: 'Token must be a 6-digit code', - })As per coding guidelines, "Verify Fastify functional patterns and TypeBox schema usage".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/fastify/src/routes/auth/magiclink/verify.ts` around lines 33 - 45, The route currently validates the 6-digit token only in the handler, so update the request schema (VerifySchema) to enforce the token pattern at the TypeBox/schema level (e.g., set token to a string with a 6-digit regexp/pattern) so Fastify runtime/contract validation rejects invalid tokens before the handler runs; then ensure the route still references VerifySchema (where token is defined) and remove the in-handler /^\d{6}$/ check or keep it only as a defensive assertion if desired.apps/next/app/auth/login/login-code-view.tsx (1)
19-28: Add an explicit return type toLoginCodeView.The exported component currently relies on inference.
As per coding guidelines, "Apply explicit return types on exported functions/APIs".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/next/app/auth/login/login-code-view.tsx` around lines 19 - 28, The exported component LoginCodeView currently lacks an explicit return type; update its function signature to include a JSX return type (for example add ": JSX.Element" or "React.ReactElement") so it conforms to the project guideline "Apply explicit return types on exported functions/APIs"; modify the declaration "export function LoginCodeView(...: LoginCodeViewProps)" to "export function LoginCodeView(...: LoginCodeViewProps): JSX.Element" (or React.ReactElement) and ensure any necessary type imports (e.g., React) are present.packages/email/emails/magic-link-login.tsx (1)
19-24: Add an explicit return type toMagicLinkLoginEmail.The exported component currently relies on inference.
As per coding guidelines, "Apply explicit return types on exported functions/APIs".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/email/emails/magic-link-login.tsx` around lines 19 - 24, The exported function MagicLinkLoginEmail currently lacks an explicit return type; update its signature to include a JSX/React element return type (e.g., : JSX.Element or : React.ReactElement) so the declaration becomes MagicLinkLoginEmail(props: Props): JSX.Element (or React.ReactElement) while keeping the same parameters (magicLink, loginCode, expirationMinutes, fullName) and exported name; ensure any necessary React types are imported if not already.apps/next/app/auth/login/login-actions.tsx (1)
149-149: Add an explicit return type toLoginActions.The exported component currently relies on inference.
As per coding guidelines, "Apply explicit return types on exported functions/APIs".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/next/app/auth/login/login-actions.tsx` at line 149, The exported function LoginActions currently has no explicit return type; update its signature to declare the return type explicitly (e.g., : JSX.Element or React.ReactElement) so the exported component adheres to the project's typing guidelines; locate the LoginActions function declaration and add the appropriate React return type to its signature.apps/next/app/auth/login/login-form.tsx (1)
72-75: Keep the submitted email for back/resend flows.The component clears
handleBackToEmail()doesn't restore it. Going back to request a new code forces the user to re-enter an address the form already knows.Also applies to: 166-170
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/next/app/auth/login/login-form.tsx` around lines 72 - 75, The code clears the submitted email when switching to the code-entry step (calls setEmail('') in the block that sets setShowCodeEntry(true)), which prevents handleBackToEmail() from restoring it; remove the setEmail('') (and any related clearing in the other block at lines ~166-170) so the submitted email is preserved, and if necessary add/restore a dedicated submittedEmail state (or pass the existing email state through) so handleBackToEmail() can repopulate the email input without forcing the user to retype it. Ensure setCode('') and setEmailValidationError(null) remain as needed but do not wipe the submitted email.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.gitleaks.toml:
- Line 18: The allowlist entry '''\.env\.local\.example$''' is too broad
(matches that filename anywhere); replace it with a regex that includes the
repo-relative directory prefix and anchors the start and end so it only matches
the single intended example file (i.e., include the directory path before
\.env\.local\.example and use ^...$), updating the pattern in the .gitleaks.toml
allowlist entry accordingly.
In `@apps/fastify/.gitignore`:
- Around line 55-57: The current broad ignore rule "profile*" hides any file or
directory whose basename starts with "profile" (e.g., src/lib/profile.ts);
replace it with a narrow pattern matching clinic artifacts (e.g.,
profile-*.json, profile-*.txt or specific profiler suffixes you produce) and
ensure the route directory is fully re-included by using the explicit negation
for its contents (e.g., keep "!src/routes/account/profile/" and also
"!src/routes/account/profile/**") so only actual clinic output is ignored and
source files like src/routes/account/profile/profile.schema.ts remain tracked.
In `@apps/fastify/src/lib/auth-web3.ts`:
- Around line 84-93: generateFunnyUsername is called before starting the
transaction causing a race where two concurrent signups can pick the same
username; move the call to generateFunnyUsername inside the transactional
callback (so it runs using tx / within db.transaction before tx.insert(users))
or implement retry-on-unique-constraint-failure around tx.insert(users); update
all call sites that currently call generateFunnyUsername(db) outside
transactions (places using db.transaction, tx.insert(users), and direct
db.insert(users) including flows that use .onConflictDoNothing) to either
generate the username inside the transaction or handle uniqueness errors and
retry.
In `@apps/fastify/src/lib/username.ts`:
- Around line 25-35: The final collision fallback can produce a >48-char
username and skips uniqueness checking; update the collision path in the
username generation logic so the appended fallback suffix keeps the full
username <=48 and is re-checked against the DB (i.e., compute a trimmed base or
a suffix length based on remaining characters, build a newCandidate from
candidate/base + fallbackSuffix that is guaranteed <=48, then query the users
table again using the same db.select(...).from(users).where(eq(users.username,
newCandidate)) loop and retry (generate new suffix) until a unique username is
found or return the unique newCandidate); reference the variables/identifiers
candidate, existing2, users, db.select and eq to locate and change the code.
In `@apps/fastify/src/routes/account/profile/update.ts`:
- Around line 76-88: The current availability check is racy; after the
optimistic check you must handle a unique-username conflict on the actual
update. Wrap the db.update(users).set(updates).where(eq(users.id,
userId)).returning() call in a try/catch and, in the catch, detect the DB
unique-constraint error (e.g. Postgres SQLSTATE '23505' or the constraint name
like 'users_username_key') and return reply.code(409).send({ code:
'USERNAME_TAKEN', message: 'Username is already in use' }) instead of letting it
bubble as a 500; keep the existing pre-check to short-circuit common cases but
convert constraint violations at the update into the documented 409 response.
- Around line 58-65: The early-return condition incorrectly counts updatedAt so
single-field updates are skipped; in the route handler change the check that
currently uses Object.keys(updates).length to instead verify that neither name
nor username was provided (e.g., check name === undefined && username ===
undefined) before returning early, so that when either name or username is
present the code proceeds to perform the database UPDATE using the updates
object (which includes updatedAt).
In `@apps/fastify/src/routes/auth/oauth/google/verify-id-token.ts`:
- Around line 105-111: generateFunnyUsername(db) is being called inside the
transaction but uses the outer db client, causing a race between uniqueness
checks and the tx.insert(users). Either change generateFunnyUsername to accept a
transaction-compatible client (signature like generateFunnyUsername(client:
typeof db | typeof tx) and call it with tx) so all reads use the same
transactional handle, or move the username generation out of db.transaction()
and pass the already-generated username into the transaction before calling
tx.insert({ id: userId, email, emailVerified: true, name, username }). Ensure
references to generateFunnyUsername, db, tx and the tx.insert(users) call are
updated accordingly.
In `@apps/fastify/src/routes/auth/oauth/twitter/exchange.ts`:
- Around line 221-227: generateFunnyUsername is being called with the outer db
client inside db.transaction, causing its availability checks to run outside the
transaction and risk races; update the call sites in exchange.ts (where
generateFunnyUsername(db) is used before tx.insert(users)) and in
verify-id-token.ts to pass the transaction client (tx) instead, or refactor the
generateFunnyUsername function signature to accept a generic DB
client/transaction and use that inside its availability queries so all
reads/writes occur within the same transaction boundary.
In `@apps/next/app/`(dashboard)/settings/(profile)/profile-section.tsx:
- Around line 35-45: buildSavePayload currently includes an empty string for
name which violates the PATCH schema; update the logic in buildSavePayload so
that state.name === '' is treated as “no change” (omit name from payload) — only
set payload.name when state.name is non-empty and differs from userName (use the
existing userName computed from user?.name). Ensure payload.name is never set to
'' (omit or set undefined) while preserving the existing username handling.
- Line 227: The current assignment for userId in profile-section.tsx prefixes
the id with "user_", causing copied IDs to not match backend responses; remove
the synthetic prefix and set userId to the raw id (e.g., use userForForm.id
directly, ensuring you coerce to string or null as needed) so the copied value
matches the API's id format and downstream workflows.
In `@apps/next/app/`(dashboard)/settings/security/security-tabs.tsx:
- Around line 3-7: The current implementation uses Tabs and TabsList with plain
Link elements (plus role="tab") which only mimic styling and lose the tabs
library keyboard/focus semantics; replace the plain Link children with the tabs
primitive by using the TabsTrigger component with the asChild prop to wrap each
Link (e.g., use TabsTrigger asChild around Link for each tab item) so the
underlying link is rendered but the Tabs library still manages keyboard
navigation/focus/aria behavior; alternatively, if you intentionally want no tab
semantics, replace the Tabs/TabsList usage with a plain nav component instead.
In `@apps/next/app/`(dashboard)/settings/security/totp-card.tsx:
- Line 29: The component initializes setupRequested to true, causing automatic
TOTP setup calls that create a new secret and invalidate any in-progress
enrollment; change this to start false (revert initialization of setupRequested)
so setup is user-triggered, or modify the backend reuse behavior by having the
setup endpoint (./apps/fastify/src/routes/account/link/totp/setup.ts) return an
existing unfinished setup within its 10-minute window; update the component
state references (setupRequested, setSetupRequested) to only trigger setup when
the user clicks the start/setup action so QR codes and manual keys remain valid
during enrollment.
In `@apps/next/app/auth/login/login-actions.tsx`:
- Around line 223-227: The onVerifySuccess handler should not redirect before
ensuring tokens are persisted: wrap the call to updateAuthTokens inside a
try/catch in the onVerifySuccess flow (the async handler passed to the
component) so any rejection is caught, log or surface the error and set the
component error state (e.g., reuse initialError or call the existing error
handler) and only call router.push('/') after updateAuthTokens resolves
successfully; reference the onVerifySuccess callback and updateAuthTokens
function and ensure failures do not cause an unhandled rejection or silent
navigation.
In `@packages/react/src/hooks/use-magic-link-verify.ts`:
- Around line 23-27: The magiclink verification call in the useMutation
(client.auth.magiclink.verify inside use-magic-link-verify.ts) is missing
throwOnError: true; update the mutationFn call to pass throwOnError: true to
client.auth.magiclink.verify so HTTP errors are thrown and React Query can
surface them (i.e., call client.auth.magiclink.verify({ body: variables,
throwOnError: true })).
---
Outside diff comments:
In `@apps/fastify/src/routes/auth/magiclink/request.ts`:
- Around line 72-89: The magic-link flow currently creates a 6-digit code
(generateLoginCode, hashToken) but lacks per-email brute-force protection; add
per-identifier attempt tracking (either fields on the verification row inserted
via db.insert(verification) such as failedAttempts, firstFailureAt, lockedUntil
or a lightweight attempts table keyed by identifier/IP) and enforce checks in
the /auth/magiclink/verify handler: on each failed verification increment
failedAttempts and set lockedUntil with progressive backoff after configurable
thresholds, reject attempts while locked, and reset failedAttempts/lockedUntil
on successful verification or when lock expires; ensure tokenPlain handling and
single-use deletion still occur after a successful verify.
In `@apps/fastify/src/routes/auth/session/user.ts`:
- Around line 99-105: The /auth/session/user route is returning name/username
from request.session.user which can be stale after /account/profile/update;
either refresh the session after updating the DB or read the latest profile from
the DB in this endpoint. Fix option A: in the /account/profile/update handler,
after performing the DB update, assign the updated name and username to
request.session.user (e.g., request.session.user.name = updated.name;
request.session.user.username = updated.username) and persist the session (call
the session store save/commit method your app uses). Fix option B: modify the
/auth/session/user handler to ignore request.session.user.name/username and
instead query the user record by request.session.user.id to return the freshest
name and username. Use the identifiers request.session.user,
/account/profile/update handler, and /auth/session/user handler to locate the
code.
In `@apps/next/e2e/auth-helpers.ts`:
- Around line 123-131: loginAsTestUser currently skips the UI code-entry flow by
extracting the backend token and calling verifyMagicLink directly; change it to
exercise the real verification UI by keeping sendMagicLink and waiting for the
"Check your email" heading, then use extractToken to get the code and call
enterLoginCodeAndSubmit(page, token) (instead of verifyMagicLink) so the e2e
path types the code into the UI and submits it; ensure you still assert
response.status() === 200, wait for the successMessage visibility, keep short
delays if needed, and remove the direct call to verifyMagicLink to fully
exercise the enterLoginCodeAndSubmit flow.
---
Minor comments:
In `@apps/docu/content/docs/architecture/authentication.mdx`:
- Around line 425-429: The diagram uses callbackURL in the GET request but the
docs and examples use callbackUrl; update the diagram to use callbackUrl
consistently (e.g., change "GET
/auth/callback/magiclink?token={code}&callbackURL=..." to use callbackUrl) and
verify other occurrences in the same diagram lines (CallbackPage and redirect to
callbackURL) are changed to callbackUrl so the flow (CallbackPage, FastifyAPI)
matches the rest of the authentication docs.
In `@apps/fastify/src/routes/auth/magiclink/request.test.ts`:
- Line 90: Capture the six-digit prefix from sentEmail.subject (use the existing
regex match) and assert it equals the token returned by
extractToken(sentEmail.body or sentEmail.html) instead of only matching digit
patterns; update both occurrences that currently only check /^\d{6} - .*
verification code$/ (the one referencing sentEmail and the other at the second
location) so they extract the numeric prefix (e.g., via match()[1]) and compare
strict equality to extractToken(...) in those tests.
In `@apps/next/app/`(dashboard)/settings/(profile)/profile-section.tsx:
- Around line 143-147: The UI text under the "Avatar" heading currently promises
a clickable upload but no upload UI exists; either remove the misleading copy or
implement a real upload flow. To fix, in profile-section.tsx update the
paragraph node that reads "Click on the avatar to upload a custom one from your
files." (the <p className="text-muted-foreground text-sm"> element) to neutral
copy (e.g., "Add an avatar to personalize your profile.") OR implement an
interactive avatar upload by adding an <input type="file" accept="image/*" />
tied to a new handler (e.g., handleAvatarUpload) and state for preview; wire the
input to an Avatar/preview element, call your upload API (e.g., uploadAvatar) on
change, handle validation/progress/errors, and ensure accessible labels for the
clickable avatar area so the UI no longer misleads users.
In `@apps/next/app/auth/login/login-form.tsx`:
- Around line 33-40: The form clears the email when moving to the code-entry
step which forces users to re-type it; remove the call that does setEmail('')
when switching to the "enter code" view (the code that clears email during the
transition) and ensure handleBackToEmail does not clear or overwrite the
existing email state; instead only clear email on explicit success (e.g., after
onVerifySuccess resolves) or a hard reset action so the component-level email
state is preserved across the two-step flow (refer to the email state variable,
the transition code that currently calls setEmail(''), and the handleBackToEmail
handler).
In `@apps/next/lib/auth/auth-utils.ts`:
- Line 10: The schema currently declares id as optional ("id:
z.string().optional()") which allows a partially authenticated state; change the
Zod schema(s) so that id is required (use z.string() without .optional())
wherever present (the occurrence around the id field and the other instance
mentioned) so getUserInfo()/session user handling always yields a defined id for
authenticated users and enables strict type narrowing in downstream code.
In `@apps/next/package.json`:
- Line 43: Package addition of "@sentry/nextjs" is fine because Sentry's default
PII scrubbing covers auth/session headers; verify in staging that no sensitive
identifiers are captured in breadcrumbs or console logs and remove any custom
beforeSend hooks if they only duplicate this behavior. Also inspect
instrumentation.ts where imports reference ".js"—either update those import
paths to ".ts" (or remove the explicit extension) or confirm the build/bundler
(Next.js/TS config) resolves .ts -> .js correctly so the instrumentation imports
work at runtime. Ensure tests or manual checks in staging validate auth/profile
flows and that Sentry events do not contain cookies, Authorization headers,
session tokens, or domain-specific identifiers.
In `@packages/react/README.md`:
- Line 7: The Exports section in packages/react README is incomplete; update it
to list and briefly describe all public hooks exported by
packages/react/src/index.ts (notably add useMagicLinkVerify, useProfileUpdate,
useOAuthLogin, usePasskeyAuth, useTotpSetup, useUser and the other auth/profile
hooks) so consumers can discover functionality; scan the export list in
src/index.ts, add each exported hook name with a short one-line description next
to it in the README Exports section, and ensure naming and spelling match the
identifiers in src/index.ts exactly.
---
Nitpick comments:
In `@apps/fastify/src/lib/env.ts`:
- Line 73: The APP_NAME zod schema currently allows whitespace and the dev
placeholder; update the APP_NAME entry in env.ts (the APP_NAME z.string()
definition) to trim input, enforce a non-empty minimum, and add a refinement
that rejects the placeholder value (e.g., "Your App") when running in production
(NODE_ENV === 'production'), so misconfiguration fails at boot rather than
leaking into emails. Use zod transform/trim and .min(1) plus .refine(...)
referencing process.env.NODE_ENV to enforce the production check on the APP_NAME
schema.
In `@apps/fastify/src/routes/auth/magiclink/verify.ts`:
- Around line 33-45: The route currently validates the 6-digit token only in the
handler, so update the request schema (VerifySchema) to enforce the token
pattern at the TypeBox/schema level (e.g., set token to a string with a 6-digit
regexp/pattern) so Fastify runtime/contract validation rejects invalid tokens
before the handler runs; then ensure the route still references VerifySchema
(where token is defined) and remove the in-handler /^\d{6}$/ check or keep it
only as a defensive assertion if desired.
In `@apps/fastify/src/routes/auth/oauth/facebook/exchange.ts`:
- Around line 169-180: Currently generateFunnyUsername(db) is called
unconditionally before the insert, causing unnecessary lookups even when
onConflictDoNothing prevents creation; change the flow to first check for an
existing user with db.select().from(users).where(eq(users.email, email)) and
only call generateFunnyUsername(db) and perform
db.insert(users)...values({...}).onConflictDoNothing({ target: users.email })
when no user is found (or alternatively perform the insert and, if it returns
zero affected rows, then select the existing user), ensuring
generateFunnyUsername is invoked only when a new user record will actually be
created.
In `@apps/next/app/`(dashboard)/page-title.tsx:
- Around line 15-19: The exported function PageTitle currently has an inferred
return type; update its signature to declare an explicit return type (e.g.,
JSX.Element | null) so the public API is typed explicitly. Locate the PageTitle
function (which uses usePathname and pageTitles) and change its declaration to
include the explicit return type, ensuring any imports for JSX types are present
if needed.
- Around line 5-13: The centralized pageTitles map in page-title.tsx
(pageTitles) should be removed and each route should define its own title
adjacent to its page/layout; update each route folder (e.g., app/, app/markets,
app/settings, app/settings/security, etc.) to export a route-local title (use
Next.js metadata export or `export const title = '...'` in the page/layout file)
and change any consumers of pageTitles to import the route-local title or read
metadata from the current route; delete the entries for '/', '/markets',
'/settings', '/settings/security', '/settings/security/passkeys',
'/settings/security/totp', and '/settings/security/apikeys' from pageTitles and
refactor code that referenced pageTitles to resolve titles from the new
per-route exports (or Next metadata) instead.
In `@apps/next/app/`(dashboard)/settings/security/layout.tsx:
- Around line 1-8: The exported SecurityLayout function is missing an explicit
return type; update the SecurityLayout signature to include explicit prop and
return types (e.g., type the props as { children: React.ReactNode } and add a
return type like JSX.Element or React.FC<{children: React.ReactNode}>) so the
exported layout follows the project's strict TypeScript convention and keeps the
prop types explicit.
In `@apps/next/app/`(dashboard)/settings/security/page.tsx:
- Around line 3-4: The exported function SecurityPage currently relies on type
inference; explicitly annotate its return type as never since it always calls
redirect() which doesn't return—update the function signature export default
function SecurityPage(): never { ... } and keep the existing
redirect('/settings/security/passkeys') call so the control flow and repo TS
export rule are satisfied.
In `@apps/next/app/`(dashboard)/settings/security/passkeys/page.tsx:
- Around line 3-4: The exported page component PasskeysPage lacks an explicit
return type; update its signature to include an explicit return type (e.g.,
change export default function PasskeysPage() to export default function
PasskeysPage(): React.JSX.Element) so the route component follows the repo's
strict TypeScript convention and keeps the API explicitly typed.
In `@apps/next/app/`(dashboard)/settings/security/totp/page.tsx:
- Around line 3-4: The exported TotpPage function lacks an explicit return type;
update the TotpPage signature to include a proper React return type (e.g.,
JSX.Element or React.ReactNode / React.ReactElement) to satisfy the repo's
strict TypeScript convention, keeping the implementation returning <TotpCard />
unchanged and ensuring any necessary React types are imported where TotpPage is
defined.
In `@apps/next/app/auth/login/login-actions.tsx`:
- Line 149: The exported function LoginActions currently has no explicit return
type; update its signature to declare the return type explicitly (e.g., :
JSX.Element or React.ReactElement) so the exported component adheres to the
project's typing guidelines; locate the LoginActions function declaration and
add the appropriate React return type to its signature.
In `@apps/next/app/auth/login/login-code-view.tsx`:
- Around line 19-28: The exported component LoginCodeView currently lacks an
explicit return type; update its function signature to include a JSX return type
(for example add ": JSX.Element" or "React.ReactElement") so it conforms to the
project guideline "Apply explicit return types on exported functions/APIs";
modify the declaration "export function LoginCodeView(...: LoginCodeViewProps)"
to "export function LoginCodeView(...: LoginCodeViewProps): JSX.Element" (or
React.ReactElement) and ensure any necessary type imports (e.g., React) are
present.
In `@apps/next/app/auth/login/login-form.tsx`:
- Around line 72-75: The code clears the submitted email when switching to the
code-entry step (calls setEmail('') in the block that sets
setShowCodeEntry(true)), which prevents handleBackToEmail() from restoring it;
remove the setEmail('') (and any related clearing in the other block at lines
~166-170) so the submitted email is preserved, and if necessary add/restore a
dedicated submittedEmail state (or pass the existing email state through) so
handleBackToEmail() can repopulate the email input without forcing the user to
retype it. Ensure setCode('') and setEmailValidationError(null) remain as needed
but do not wipe the submitted email.
In `@packages/email/emails/magic-link-login.tsx`:
- Around line 19-24: The exported function MagicLinkLoginEmail currently lacks
an explicit return type; update its signature to include a JSX/React element
return type (e.g., : JSX.Element or : React.ReactElement) so the declaration
becomes MagicLinkLoginEmail(props: Props): JSX.Element (or React.ReactElement)
while keeping the same parameters (magicLink, loginCode, expirationMinutes,
fullName) and exported name; ensure any necessary React types are imported if
not already.
In `@packages/react/src/hooks/use-profile-update.ts`:
- Around line 13-16: The inline inline request type in mutationFn should be
replaced by the generated request/body type exported from `@repo/core` to avoid
drifting API contracts; add a named import (e.g., import { ProfileUpdateRequest
} from '@repo/core') and change the mutationFn signature to use that imported
type (mutationFn: (body: ProfileUpdateRequest) => client.account.profile({ body,
throwOnError: true })), ensuring you use the named export from `@repo/core` and
preserve existing nullability/optional semantics from the generated type.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 55c8fc9c-0a11-4f74-82fb-2e4e1457c253
⛔ Files ignored due to path filters (11)
apps/fastify/.env-sampleis excluded by!**/.env*apps/fastify/src/db/migrations/0012_aberrant_johnny_blaze.sqlis excluded by!apps/fastify/src/db/migrations/**apps/fastify/src/db/migrations/meta/0012_snapshot.jsonis excluded by!apps/fastify/src/db/migrations/**apps/fastify/src/db/migrations/meta/_journal.jsonis excluded by!apps/fastify/src/db/migrations/**apps/next/.env.local.exampleis excluded by!**/.env*packages/core/src/api-client.gen.tsis excluded by!**/*.gen.tspackages/core/src/api-wrapper.gen.tsis excluded by!**/*.gen.tspackages/core/src/gen/index.tsis excluded by!**/gen/**,!**/gen/**packages/core/src/gen/sdk.gen.tsis excluded by!**/gen/**,!**/gen/**,!**/*.gen.tspackages/core/src/gen/types.gen.tsis excluded by!**/gen/**,!**/gen/**,!**/*.gen.tspnpm-lock.yamlis excluded by!**/pnpm-lock.yaml,!**/pnpm-lock.yaml
📒 Files selected for processing (57)
.cursor/rules/frontend/stack.mdc.gitleaks.tomlapps/docu/content/docs/architecture/authentication.mdxapps/docu/content/docs/architecture/frontend-stack.mdxapps/docu/content/docs/development/package-conventions.mdxapps/docu/content/docs/development/packages.mdxapps/fastify/.gitignoreapps/fastify/openapi/openapi.jsonapps/fastify/package.jsonapps/fastify/src/db/schema/tables/users.tsapps/fastify/src/lib/api-key-auth.tsapps/fastify/src/lib/auth-web3.tsapps/fastify/src/lib/env.tsapps/fastify/src/lib/jwt.tsapps/fastify/src/lib/username.tsapps/fastify/src/plugins/auth.tsapps/fastify/src/routes/account/profile/update.tsapps/fastify/src/routes/auth/magiclink/request.test.tsapps/fastify/src/routes/auth/magiclink/request.tsapps/fastify/src/routes/auth/magiclink/verify.test.tsapps/fastify/src/routes/auth/magiclink/verify.tsapps/fastify/src/routes/auth/oauth/facebook/exchange.tsapps/fastify/src/routes/auth/oauth/github/exchange.tsapps/fastify/src/routes/auth/oauth/google/verify-id-token.tsapps/fastify/src/routes/auth/oauth/twitter/exchange.tsapps/fastify/src/routes/auth/session/user.tsapps/next/app/(dashboard)/_dashboard-shell.tsxapps/next/app/(dashboard)/page-title.tsxapps/next/app/(dashboard)/settings/(profile)/profile-section.tsxapps/next/app/(dashboard)/settings/security/apikeys/page.tsxapps/next/app/(dashboard)/settings/security/layout.tsxapps/next/app/(dashboard)/settings/security/page.tsxapps/next/app/(dashboard)/settings/security/passkeys/page.tsxapps/next/app/(dashboard)/settings/security/security-section.tsxapps/next/app/(dashboard)/settings/security/security-tabs.tsxapps/next/app/(dashboard)/settings/security/totp-card.tsxapps/next/app/(dashboard)/settings/security/totp/page.tsxapps/next/app/auth/login/login-actions.tsxapps/next/app/auth/login/login-code-view.tsxapps/next/app/auth/login/login-form.tsxapps/next/app/auth/login/page.tsxapps/next/e2e/auth-helpers.tsapps/next/e2e/magic-link-auth.spec.tsapps/next/e2e/passkey-auth.spec.tsapps/next/e2e/security/api-keys.spec.tsapps/next/e2e/security/authenticator.spec.tsapps/next/e2e/security/passkeys.spec.tsapps/next/lib/auth/auth-utils.tsapps/next/next.config.mjsapps/next/package.jsonpackages/email/emails/magic-link-login.tsxpackages/react/README.mdpackages/react/src/components/login-form.spec.tsxpackages/react/src/hooks/use-magic-link-verify.tspackages/react/src/hooks/use-profile-update.tspackages/react/src/index.tspackages/ui/src/components/scroll-area.tsx
💤 Files with no reviewable changes (3)
- apps/docu/content/docs/development/packages.mdx
- apps/next/app/(dashboard)/settings/security/security-section.tsx
- packages/react/src/components/login-form.spec.tsx
apps/next/app/(dashboard)/settings/(profile)/profile-section.tsx
Outdated
Show resolved
Hide resolved
- gitleaks: narrow .env.local.example allowlist to apps/next/ - fastify gitignore: replace profile* with clinic-specific patterns, add route negation - username: move generateFunnyUsername inside tx, fix >48 char fallback, add Db|Tx type - profile update: fix early return (name/username check), handle unique constraint 409 - magic-link verify: add throwOnError; brute-force protection (auth_attempts, 429) - profile-section: buildSavePayload omit empty name, raw userId (no user_ prefix) - login-actions: wrap updateAuthTokens in try/catch before redirect - security-tabs: use TabsTrigger asChild for tab semantics - totp-card: setupRequested false (user-triggered) - login-form: preserve email when switching to code entry - avatar copy: neutral text (no upload promise) - session user: fetch name/username from DB - e2e: enterLoginCodeAndSubmit instead of verifyMagicLink - docs: callbackUrl in diagram; README exports; verify token schema - auth-utils: id required; use-profile-update AccountProfileUpdateData - explicit return types; APP_NAME env validation; Facebook username opt
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
apps/next/app/(dashboard)/settings/security/security-tabs.tsx (1)
20-21: Consider reducing style duplication with TabsTrigger.The
triggerStylesconstant duplicates several classes already present in theTabsTriggercomponent (e.g.,data-[state=active]variants, sizing, transitions). When usingasChild, both the internal TabsTrigger styles and these styles are applied.Consider keeping only the overrides specific to this component's layout needs (like
min-w-0and grid-related sizing) and relying on TabsTrigger's base styles for the rest.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/next/app/`(dashboard)/settings/security/security-tabs.tsx around lines 20 - 21, The triggerStyles string duplicates base styles already provided by the TabsTrigger component causing redundant/conflicting classes when used with asChild; trim triggerStyles to only the layout-specific overrides (e.g., keep min-w-0, grid/width/height constraints and any component-specific gap/padding adjustments) and remove duplicated variants and transitions (like data-[state=active] variants, sizing, transition-[color,box-shadow], and svg utility rules) so TabsTrigger's base styles handle those; update the usage that passes triggerStyles into TabsTrigger to rely on TabsTrigger defaults and only supply the minimal layout classes needed for this settings/security-tabs component.apps/next/app/(dashboard)/settings/security/layout.tsx (1)
9-12: Redundant margin class.The
mt-6on line 11 is redundant since the parent'sspace-y-6already appliesmargin-top: 1.5remto non-first children.Suggested fix
return ( <div className="mx-auto max-w-2xl space-y-6"> <SecurityTabs /> - <div className="mt-6">{children}</div> + <div>{children}</div> </div> )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/next/app/`(dashboard)/settings/security/layout.tsx around lines 9 - 12, The inner div uses className="mt-6" which duplicates spacing already provided by the parent wrapper's space-y-6; remove the redundant mt-6 from the div that renders {children} in apps/next/app/(dashboard)/settings/security/layout.tsx (the element directly after <SecurityTabs />) so spacing is solely controlled by the parent's space-y-6.apps/fastify/src/routes/auth/session/user.ts (1)
92-93: Consider selecting only the needed fields fromuserstable.The query fetches all columns but only
nameandusernameare used. A selective query would reduce data transfer.♻️ Suggested optimization
- ;[userRow] = await db.select().from(users).where(eq(users.id, userId)) + ;[userRow] = await db + .select({ name: users.name, username: users.username }) + .from(users) + .where(eq(users.id, userId))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/fastify/src/routes/auth/session/user.ts` around lines 92 - 93, The query currently selects all columns with db.select().from(users).where(eq(users.id, userId)) and assigns the result to userRow but only uses userRow.name and userRow.username; change the select to request only the needed columns (e.g., select(users.name, users.username) or equivalent projection supported by your query builder) so the statement in the code that assigns ;[userRow] = await db.select()... fetches only name and username, reducing data transfer and memory usage.apps/next/app/(dashboard)/settings/(profile)/profile-section.tsx (1)
225-227: Consider renamingnameDirtytoisDirtyorformDirty.The variable name
nameDirtyis misleading since it tracks changes to bothnameandusernamefields, not just the name.♻️ Suggested rename
- const nameDirty = + const isDirty = (state.name ?? '') !== (userForForm.name ?? '') || (state.username ?? '') !== (userForForm.username ?? '')And update references at lines 102, 231, 232, and 290.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/next/app/`(dashboard)/settings/(profile)/profile-section.tsx around lines 225 - 227, The variable nameDirty is misleading because it tracks both state.name and state.username changes versus userForForm.name and userForForm.username; rename nameDirty to a clearer identifier like isDirty or formDirty across the file (update the declaration that computes (state.name ?? '') !== (userForForm.name ?? '') || (state.username ?? '') !== (userForForm.username ?? '') and all uses of nameDirty), ensuring references in the component (places where nameDirty is read/used) are updated to the new name to keep behavior identical.apps/fastify/src/routes/auth/magiclink/verify.ts (1)
46-50: Redundant validation—consider removing or keeping for explicit error message.The TypeBox schema on line 16 already validates the
^\d{6}$pattern. This runtime check is redundant but does provide a more specific error code (INVALID_TOKEN) vs the generic validation error from TypeBox.If intentional for UX, this is fine. Otherwise, you could remove it and let the schema handle validation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/fastify/src/routes/auth/magiclink/verify.ts` around lines 46 - 50, Remove the redundant runtime token regex check: delete the if (!/^\d{6}$/.test(token)) { ... } block in verify.ts so the TypeBox schema (line 16) handles token validation and Fastify's validation errors are used instead; if you want to keep the explicit UX error, alternatively keep the block but add a comment referencing the TypeBox schema and ensure the error code 'INVALID_TOKEN' is covered in tests and any consumers expecting that code.apps/fastify/src/routes/auth/magiclink/request.ts (1)
57-73: User creation transaction is sound, but username collisions at the database level lack explicit handling.The
users.usernamecolumn has aUNIQUEconstraint. WhilegenerateFunnyUsername()retries collision avoidance, an edge case exists where exhausting retries could still produce a duplicate username. In this scenario, the transaction will fail when the database constraint is violated, throwing a generic "Failed to create user" error.Consider wrapping the transaction in a try-catch to explicitly handle
UNIQUEconstraint violations, distinguishing them from other transaction failures for better error reporting and potential retry logic.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/fastify/src/routes/auth/magiclink/request.ts` around lines 57 - 73, The user creation transaction around db.transaction (which inserts into users and relies on generateFunnyUsername for users.username) should be wrapped in a try-catch that inspects DB constraint errors (e.g., unique-violation on users.username) so username collisions are handled explicitly; catch the error from the transaction, detect the UNIQUE constraint violation for the users.username column (DB-specific error code/message), and either retry generating a username (calling generateFunnyUsername again) or throw a clear, distinct error that indicates a username uniqueness conflict, while leaving other transaction errors to propagate as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/fastify/src/routes/auth/magiclink/verify.ts`:
- Around line 54-65: The IP-based rate limit in verify.ts uses request.ip
directly (used when querying authAttempts for type 'magic_link' and checking
lockedUntil) which can be spoofed behind proxies; update this to use the same
trusted-client extraction as the global rate-limit plugin (either enable
Fastify's trustProxy or parse request.headers['x-forwarded-for'] with the same
key-generation logic) and replace request.ip with that trusted client IP before
querying authAttempts and comparing lockedUntil so local rate checks cannot be
bypassed by forged headers.
In `@apps/fastify/src/routes/auth/oauth/facebook/exchange.ts`:
- Around line 169-183: When provisioning a new OAuth user in exchange.ts, the
current flow uses generateFunnyUsername() once then inserts with
.onConflictDoNothing({ target: users.email }) which doesn't handle unique
constraint collisions on users.username; change this to a bounded retry loop
that: calls generateFunnyUsername() each attempt, attempts
db.insert(users).values(...), and on success selects the user; if the insert
fails due to a username unique-constraint (inspect the DB error code/constraint
name) regenerate and retry up to N times, and if .onConflictDoNothing({ target:
users.email }) returns no row because the email already exists select the
existing user and proceed; apply the same retry pattern to the Google/Twitter
username insert paths as well so transient username collisions are retried
instead of failing the sign-in.
---
Nitpick comments:
In `@apps/fastify/src/routes/auth/magiclink/request.ts`:
- Around line 57-73: The user creation transaction around db.transaction (which
inserts into users and relies on generateFunnyUsername for users.username)
should be wrapped in a try-catch that inspects DB constraint errors (e.g.,
unique-violation on users.username) so username collisions are handled
explicitly; catch the error from the transaction, detect the UNIQUE constraint
violation for the users.username column (DB-specific error code/message), and
either retry generating a username (calling generateFunnyUsername again) or
throw a clear, distinct error that indicates a username uniqueness conflict,
while leaving other transaction errors to propagate as before.
In `@apps/fastify/src/routes/auth/magiclink/verify.ts`:
- Around line 46-50: Remove the redundant runtime token regex check: delete the
if (!/^\d{6}$/.test(token)) { ... } block in verify.ts so the TypeBox schema
(line 16) handles token validation and Fastify's validation errors are used
instead; if you want to keep the explicit UX error, alternatively keep the block
but add a comment referencing the TypeBox schema and ensure the error code
'INVALID_TOKEN' is covered in tests and any consumers expecting that code.
In `@apps/fastify/src/routes/auth/session/user.ts`:
- Around line 92-93: The query currently selects all columns with
db.select().from(users).where(eq(users.id, userId)) and assigns the result to
userRow but only uses userRow.name and userRow.username; change the select to
request only the needed columns (e.g., select(users.name, users.username) or
equivalent projection supported by your query builder) so the statement in the
code that assigns ;[userRow] = await db.select()... fetches only name and
username, reducing data transfer and memory usage.
In `@apps/next/app/`(dashboard)/settings/(profile)/profile-section.tsx:
- Around line 225-227: The variable nameDirty is misleading because it tracks
both state.name and state.username changes versus userForForm.name and
userForForm.username; rename nameDirty to a clearer identifier like isDirty or
formDirty across the file (update the declaration that computes (state.name ??
'') !== (userForForm.name ?? '') || (state.username ?? '') !==
(userForForm.username ?? '') and all uses of nameDirty), ensuring references in
the component (places where nameDirty is read/used) are updated to the new name
to keep behavior identical.
In `@apps/next/app/`(dashboard)/settings/security/layout.tsx:
- Around line 9-12: The inner div uses className="mt-6" which duplicates spacing
already provided by the parent wrapper's space-y-6; remove the redundant mt-6
from the div that renders {children} in
apps/next/app/(dashboard)/settings/security/layout.tsx (the element directly
after <SecurityTabs />) so spacing is solely controlled by the parent's
space-y-6.
In `@apps/next/app/`(dashboard)/settings/security/security-tabs.tsx:
- Around line 20-21: The triggerStyles string duplicates base styles already
provided by the TabsTrigger component causing redundant/conflicting classes when
used with asChild; trim triggerStyles to only the layout-specific overrides
(e.g., keep min-w-0, grid/width/height constraints and any component-specific
gap/padding adjustments) and remove duplicated variants and transitions (like
data-[state=active] variants, sizing, transition-[color,box-shadow], and svg
utility rules) so TabsTrigger's base styles handle those; update the usage that
passes triggerStyles into TabsTrigger to rely on TabsTrigger defaults and only
supply the minimal layout classes needed for this settings/security-tabs
component.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 58ace144-4427-487e-bbe6-e99ef972cb88
⛔ Files ignored due to path filters (4)
apps/fastify/src/db/migrations/0013_familiar_master_chief.sqlis excluded by!apps/fastify/src/db/migrations/**apps/fastify/src/db/migrations/meta/0013_snapshot.jsonis excluded by!apps/fastify/src/db/migrations/**apps/fastify/src/db/migrations/meta/_journal.jsonis excluded by!apps/fastify/src/db/migrations/**packages/core/src/gen/types.gen.tsis excluded by!**/gen/**,!**/gen/**,!**/*.gen.ts
📒 Files selected for processing (35)
.gitleaks.tomlapps/docu/content/docs/architecture/authentication.mdxapps/fastify/.gitignoreapps/fastify/openapi/openapi.jsonapps/fastify/src/db/schema/index.tsapps/fastify/src/db/schema/tables/auth-attempts.tsapps/fastify/src/lib/auth-web3.tsapps/fastify/src/lib/env.tsapps/fastify/src/lib/username.tsapps/fastify/src/routes/account/profile/update.tsapps/fastify/src/routes/auth/magiclink/request.test.tsapps/fastify/src/routes/auth/magiclink/request.tsapps/fastify/src/routes/auth/magiclink/verify.tsapps/fastify/src/routes/auth/oauth/facebook/exchange.tsapps/fastify/src/routes/auth/oauth/google/verify-id-token.tsapps/fastify/src/routes/auth/oauth/twitter/exchange.tsapps/fastify/src/routes/auth/session/user.tsapps/fastify/test/utils/db.tsapps/next/app/(dashboard)/page-title.tsxapps/next/app/(dashboard)/settings/(profile)/profile-section.tsxapps/next/app/(dashboard)/settings/security/layout.tsxapps/next/app/(dashboard)/settings/security/page.tsxapps/next/app/(dashboard)/settings/security/passkeys/page.tsxapps/next/app/(dashboard)/settings/security/security-tabs.tsxapps/next/app/(dashboard)/settings/security/totp-card.tsxapps/next/app/(dashboard)/settings/security/totp/page.tsxapps/next/app/auth/login/login-actions.tsxapps/next/app/auth/login/login-code-view.tsxapps/next/app/auth/login/login-form.tsxapps/next/e2e/auth-helpers.tsapps/next/lib/auth/auth-utils.tspackages/email/emails/magic-link-login.tsxpackages/react/README.mdpackages/react/src/hooks/use-magic-link-verify.tspackages/react/src/hooks/use-profile-update.ts
🚧 Files skipped from review as they are similar to previous changes (9)
- apps/next/app/(dashboard)/settings/security/passkeys/page.tsx
- packages/react/src/hooks/use-magic-link-verify.ts
- apps/next/app/auth/login/login-code-view.tsx
- apps/next/app/(dashboard)/settings/security/totp-card.tsx
- apps/fastify/src/lib/username.ts
- apps/next/lib/auth/auth-utils.ts
- apps/fastify/.gitignore
- apps/fastify/src/lib/auth-web3.ts
- apps/next/app/(dashboard)/settings/security/totp/page.tsx
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
apps/fastify/src/routes/auth/oauth/twitter/exchange.ts (2)
1-324: File exceeds the 300-line limit (324 lines).Consider extracting the helper functions
runTwitterExchangeTxandfetchTwitterOAuthDatato a shared OAuth utilities module (e.g.,lib/oauth-twitter.ts). This would reduce the route file size and enable reuse if other Twitter-related routes are added. As per coding guidelines: "File size limit is 300 lines (exempt: test files and UI components)."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/fastify/src/routes/auth/oauth/twitter/exchange.ts` around lines 1 - 324, The file is over the 300-line limit; extract the large helper functions into a new module to shrink the route file. Move runTwitterExchangeTx and fetchTwitterOAuthData (including their dependent types TwitterTokenResponse, TwitterUser, TwitterAccountData) into a new lib/oauth-twitter.ts, export them, and import them in this route; ensure you also export any helper types used by callers and update imports for getDb, encryptAccountTokens, env, generateJti, hashToken, generateFunnyUsername, isUniqueViolation, and related symbols so oauthExchangeRoute continues to call runTwitterExchangeTx and fetchTwitterOAuthData with the same signatures and behavior.
269-279: Consider renamingcodeto avoid shadowing the request body parameter.The variable
codeat line 269 shadows the destructuredcodefromconst { code, state } = request.bodyat line 216. While technically safe due to different scopes, this can cause confusion during maintenance.♻️ Suggested rename for clarity
- const code = + const errorCode = err instanceof Error && err.message === 'USER_FETCH_FAILED' ? 'FETCH_USER_FAILED' : 'UPSTREAM_SERVICE_ERROR' - return reply.code(code === 'FETCH_USER_FAILED' ? 400 : 502).send({ - code, + return reply.code(errorCode === 'FETCH_USER_FAILED' ? 400 : 502).send({ + code: errorCode, message: - code === 'FETCH_USER_FAILED' + errorCode === 'FETCH_USER_FAILED' ? 'Invalid Twitter user response' : 'Failed to exchange code for token or fetch user', })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/fastify/src/routes/auth/oauth/twitter/exchange.ts` around lines 269 - 279, The local variable named "code" inside the error-handling block shadows the destructured "code" from const { code, state } = request.body; rename the local variable (e.g., errorCode or responseCode) and update all usages in the conditional and reply call (the ternary that chooses HTTP status and the message/code fields) to use the new name; ensure the rename covers the assignment (err instanceof Error ... ? 'FETCH_USER_FAILED' : 'UPSTREAM_SERVICE_ERROR') and the two places where it is read for status and message so no shadowing remains.apps/fastify/src/routes/auth/oauth/google/verify-id-token.ts (1)
162-172: Consider usingfindOrCreateUserByEmailfor consistency.The inline retry loop duplicates the pattern already implemented in
findOrCreateUserByEmail(used by Facebook OAuth). While functionally correct, using the shared helper would reduce code duplication and ensure consistent behavior across OAuth providers.That said, the Google flow has additional complexity (account linking within the transaction), which may justify the custom implementation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/fastify/src/routes/auth/oauth/google/verify-id-token.ts` around lines 162 - 172, The retry loop around runGoogleVerifyIdTokenTx duplicates the retry/find-or-create pattern used by findOrCreateUserByEmail; replace the custom maxRetries loop by reusing findOrCreateUserByEmail (or move the account-linking logic inside a shared transactional helper) so Google OAuth reuses the same retry/unique-violation handling; specifically, either call findOrCreateUserByEmail from verify-id-token flow and then perform the Google-specific account linking in a transaction, or refactor runGoogleVerifyIdTokenTx to delegate user creation to findOrCreateUserByEmail to eliminate the duplicated retry logic and ensure consistent behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/fastify/src/lib/request.ts`:
- Around line 8-15: getTrustedClientIp currently treats an empty
'x-forwarded-for' value as valid because it uses the nullish coalescing operator
(??), which doesn't catch empty strings; this can return '' and create
inconsistent rate-limit keys. Update getTrustedClientIp to treat empty strings
as missing by trimming and checking for a non-empty value (e.g., use a truthy
check or explicit length test) when extracting the first IP from
request.headers['x-forwarded-for'], and if empty fall back to request.ip and
then 'unknown'; ensure you reference and adjust the logic inside
getTrustedClientIp where 'forwarded', 'ips', and the return expression are
handled.
In `@apps/fastify/src/routes/auth/magiclink/request.ts`:
- Around line 105-106: The code is placing the 6-digit verifier into the URL via
magicLinkUrl.searchParams.set('token', code) which makes the secret leakable;
change the link to never include the short numeric code. Instead populate the
URL with a non-secret identifier (e.g., verificationId or a high-entropy
one-time linkToken) and keep the 6-digit code delivered only in the email body;
update the magic link generation logic around magicLinkUrl, callbackUrl and code
so the URL carries only the non-secret id/token and the server verifies the
submitted code out-of-band when the user follows the link.
- Line 57: The current flow selects the user with "let [user] = await
db.select().from(users).where(eq(users.email, email))" then attempts to insert
and on unique-violation it retries the insert loop; to handle races where
another request inserted the same email first, catch the unique-violation branch
in the insert logic (the code handling insert retry around the users.insert
call) and before retrying the insert re-query the user by email (same
db.select().from(users).where(eq(users.email, email)) pattern) and if found
treat it as the winner (use that user and break retry loop), otherwise proceed
to retry for username collisions—apply the same change for the other retry block
in the 61-80 region so email races converge to the existing account rather than
repeatedly attempting the same insert.
In `@apps/fastify/src/routes/auth/magiclink/verify.ts`:
- Around line 52-55: The handler currently reads authAttempts with a
select-then-update/insert which can race — add a UNIQUE constraint on (key,type)
for the authAttempts table via a new Drizzle migration (do not edit existing
migrations directly) and replace the select-then-update logic in the verify
handler with a single atomic upsert/transaction that increments the attempts
column in SQL (use an INSERT ... ON CONFLICT (...) DO UPDATE SET attempts =
authAttempts.attempts + 1 or the equivalent Drizzle upsert API) so concurrent
requests cannot create duplicate rows or lose increments; update the logic in
the verify.ts code that references authAttempts (the select then conditional
insert/update block) to use that upsert and then check the attempts count
atomically to enforce the 5-attempt lock.
- Around line 17-18: The handler currently authenticates solely on the 6-digit
code, which allows collisions across concurrent verifications; update the verify
flow in verify.ts to require a second client-known identifier (email or
per-request verificationId) in the request (add to the input schema), then hash
the code as before but change the DB lookup (e.g., the function currently doing
findVerificationByCode / getVerificationRow) to query by both hashedCode AND the
second identifier (email or verificationId). Also update any helper signatures
(verifyMagicLink / findVerificationByCode) to accept that second parameter and
use it in the WHERE clause so only the intended verification row matches.
---
Nitpick comments:
In `@apps/fastify/src/routes/auth/oauth/google/verify-id-token.ts`:
- Around line 162-172: The retry loop around runGoogleVerifyIdTokenTx duplicates
the retry/find-or-create pattern used by findOrCreateUserByEmail; replace the
custom maxRetries loop by reusing findOrCreateUserByEmail (or move the
account-linking logic inside a shared transactional helper) so Google OAuth
reuses the same retry/unique-violation handling; specifically, either call
findOrCreateUserByEmail from verify-id-token flow and then perform the
Google-specific account linking in a transaction, or refactor
runGoogleVerifyIdTokenTx to delegate user creation to findOrCreateUserByEmail to
eliminate the duplicated retry logic and ensure consistent behavior.
In `@apps/fastify/src/routes/auth/oauth/twitter/exchange.ts`:
- Around line 1-324: The file is over the 300-line limit; extract the large
helper functions into a new module to shrink the route file. Move
runTwitterExchangeTx and fetchTwitterOAuthData (including their dependent types
TwitterTokenResponse, TwitterUser, TwitterAccountData) into a new
lib/oauth-twitter.ts, export them, and import them in this route; ensure you
also export any helper types used by callers and update imports for getDb,
encryptAccountTokens, env, generateJti, hashToken, generateFunnyUsername,
isUniqueViolation, and related symbols so oauthExchangeRoute continues to call
runTwitterExchangeTx and fetchTwitterOAuthData with the same signatures and
behavior.
- Around line 269-279: The local variable named "code" inside the error-handling
block shadows the destructured "code" from const { code, state } = request.body;
rename the local variable (e.g., errorCode or responseCode) and update all
usages in the conditional and reply call (the ternary that chooses HTTP status
and the message/code fields) to use the new name; ensure the rename covers the
assignment (err instanceof Error ... ? 'FETCH_USER_FAILED' :
'UPSTREAM_SERVICE_ERROR') and the two places where it is read for status and
message so no shadowing remains.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 783b3e91-811f-4f41-9741-0285dc85122d
📒 Files selected for processing (13)
apps/fastify/src/lib/db-errors.tsapps/fastify/src/lib/oauth-user.tsapps/fastify/src/lib/request.tsapps/fastify/src/plugins/rate-limit.tsapps/fastify/src/routes/auth/magiclink/request.tsapps/fastify/src/routes/auth/magiclink/verify.tsapps/fastify/src/routes/auth/oauth/facebook/exchange.tsapps/fastify/src/routes/auth/oauth/google/verify-id-token.tsapps/fastify/src/routes/auth/oauth/twitter/exchange.tsapps/fastify/src/routes/auth/session/user.tsapps/next/app/(dashboard)/settings/(profile)/profile-section.tsxapps/next/app/(dashboard)/settings/security/layout.tsxapps/next/app/(dashboard)/settings/security/security-tabs.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/next/app/(dashboard)/settings/security/security-tabs.tsx
- apps/next/app/(dashboard)/settings/security/layout.tsx
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/next/app/terms/page.tsx (1)
6-89: Consider extracting the shared policy-page shell.This page and
apps/next/app/privacy/page.tsxduplicate the same layout, back link, metadata row, section wrapper, and contact block almost verbatim. Pulling that into a small shared component or content model would make future legal-copy updates much less likely to drift.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/next/app/terms/page.tsx` around lines 6 - 89, The Terms page duplicates layout/structure found in privacy/page.tsx; extract the shared shell into a reusable component (e.g., PolicyPageShell or PolicyShell) that encapsulates the back Link + ChevronLeft, title prop, last-updated prop, outer wrappers (min-h-svh p-4 md:p-6, mx-auto max-w-3xl space-y-6), the section container (space-y-6 text-sm) and the contact block; implement PolicyPageShell to accept props (title, updatedAt, children) and preserve existing className values and accessibility attributes, then replace the content in apps/next/app/terms/page.tsx and apps/next/app/privacy/page.tsx to import and render PolicyPageShell with the page-specific sections as children.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/next/app/privacy/page.tsx`:
- Line 16: Update the placeholder legal contact and stale date strings: replace
"legal@example.com" with the project's real privacy/data-rights email and update
the "Last updated: March 2025" text to the current release date (e.g., "Last
updated: March 2026") in the privacy page component (the JSX node containing the
text "Last updated: March 2025") and make the identical updates in the mirrored
Terms page where "legal@example.com" and the same "Last updated" block appear
(lines referenced around 81-87).
---
Nitpick comments:
In `@apps/next/app/terms/page.tsx`:
- Around line 6-89: The Terms page duplicates layout/structure found in
privacy/page.tsx; extract the shared shell into a reusable component (e.g.,
PolicyPageShell or PolicyShell) that encapsulates the back Link + ChevronLeft,
title prop, last-updated prop, outer wrappers (min-h-svh p-4 md:p-6, mx-auto
max-w-3xl space-y-6), the section container (space-y-6 text-sm) and the contact
block; implement PolicyPageShell to accept props (title, updatedAt, children)
and preserve existing className values and accessibility attributes, then
replace the content in apps/next/app/terms/page.tsx and
apps/next/app/privacy/page.tsx to import and render PolicyPageShell with the
page-specific sections as children.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5da7b1f4-2583-4ac3-b4a2-aba60df1a101
📒 Files selected for processing (3)
apps/next/app/privacy/page.tsxapps/next/app/terms/page.tsxapps/next/proxy.ts
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/next/e2e/auth-helpers.ts (1)
53-74:⚠️ Potential issue | 🟡 MinorFix
extractToken()to read token independently ofverificationId.The
/test/magic-link/lastendpoint returns bothtokenandverificationIdas optional fields (each can be null independently).extractMagicLinkData()currently requires both to be present and returns null otherwise. SinceloginAsTestUser()only needs the token forenterLoginCodeAndSubmit(), it will fail unnecessarily when the endpoint returns a token but noverificationId(e.g., when the DB has no@test.airecord withtokenPlain, but the token is extracted fromfakeEmail).Keep
extractMagicLinkData()forverifyMagicLink()which legitimately requires both fields, but relaxextractToken()to extract the token field independently or via a separate path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/next/e2e/auth-helpers.ts` around lines 53 - 74, The current extractMagicLinkData enforces both token and verificationId, causing failures when only token exists; keep extractMagicLinkData as-is for verifyMagicLink, but change or add extractToken to call the same `${apiUrl}/test/magic-link/last` endpoint (reusing the retry/delay logic) and return the token independently of verificationId (i.e., succeed when data.token exists even if data.verificationId is null), ensuring loginAsTestUser()/enterLoginCodeAndSubmit() only requires the token while leaving verifyMagicLink() to continue using extractMagicLinkData() when both fields are needed.
♻️ Duplicate comments (1)
apps/fastify/src/routes/auth/magiclink/request.ts (1)
41-48:⚠️ Potential issue | 🟡 MinorEmail race still possible on final retry attempt.
The unique-violation handler only re-queries by email when
attempt < maxRetries - 1. On the last attempt, if another request created the user concurrently, this will throw instead of returning the existing user.🧩 Proposed fix
} catch (err) { - if (isUniqueViolation(err) && attempt < maxRetries - 1) { + if (isUniqueViolation(err)) { const [existing] = await db.select().from(users).where(eq(users.email, email)) if (existing) return existing + if (attempt >= maxRetries - 1) throw err continue } throw err }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/fastify/src/routes/auth/magiclink/request.ts` around lines 41 - 48, The unique-violation handler currently only re-queries for an existing user when attempt < maxRetries - 1, so on the last retry a concurrent creation will surface as an exception; change the catch block that checks isUniqueViolation(err) to always re-query the users table by email (using db.select().from(users).where(eq(users.email, email))) and return the existing record if found (regardless of attempt), otherwise continue or rethrow as appropriate so the final retry returns the concurrently-created user instead of throwing; references: isUniqueViolation, db.select().from(users).where(eq(users.email, email)), attempt, maxRetries.
🧹 Nitpick comments (4)
apps/fastify/src/routes/auth/oauth/google/verify-id-token.ts (2)
23-79: Well-structured transactional helper.The refactored design correctly addresses the previous race condition concern by calling
findOrCreateUserByEmailoutside the transaction boundary, ensuring username generation uses consistent DB reads.Minor observation: Lines 50-55 define
accessToken,refreshToken,idToken, and expiry fields inaccountDatathat are never used in the insert (lines 64-70).♻️ Suggested cleanup to remove unused fields
const accountData = { id: existingAccount?.id ?? randomUUID(), userId: linkedUserId, accountId, providerId: 'google' as const, - accessToken: null as string | null, - refreshToken: null as string | null, - idToken: null as string | null, - accessTokenExpiresAt: null as Date | null, - refreshTokenExpiresAt: null as Date | null, scope: 'openid email profile', }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/fastify/src/routes/auth/oauth/google/verify-id-token.ts` around lines 23 - 79, The accountData object in runGoogleVerifyIdTokenTx declares unused fields (accessToken, refreshToken, idToken, accessTokenExpiresAt, refreshTokenExpiresAt) that are never written to the DB; remove these unused fields from accountData (or from its construction) and ensure the insert branch that creates a new account only supplies the actual persisted properties (id, userId, accountId, providerId, scope), referencing accountData.id/userId/accountId/providerId/scope and leaving out the unused token/expiry properties to keep the shape minimal and avoid dead fields.
39-70: Add a unique constraint on(providerId, accountId)to prevent duplicate account records.The SELECT-then-INSERT pattern (lines 39-70) is vulnerable to a race condition where concurrent requests could both find no existing account and insert duplicates. This is a pre-existing pattern also present in the Facebook OAuth flow. The transaction alone doesn't prevent this since SELECT doesn't acquire row locks.
To resolve this, add a composite unique constraint on
(providerId, accountId)to the account table and useonConflictDoUpdateto safely handle concurrent inserts.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/fastify/src/routes/auth/oauth/google/verify-id-token.ts` around lines 39 - 70, Add a composite unique constraint on (providerId, accountId) in the account table schema and change the insert logic in verify-id-token.ts to use an upsert instead of a plain insert to avoid race conditions: keep the initial lookup (tx.select... from account where ...) if you want to prefer an existing row, but replace the tx.insert(account).values({...}) branch with a tx.insert(...).values(...).onConflictDoUpdate(...) that targets the (account.providerId, account.accountId) constraint and updates relevant fields (userId, updatedAt, and any tokens/scopes) so concurrent inserts merge safely; ensure you still set id to a generated UUID when inserting and preserve existingAccount.id when updating (use the conflict update to set userId = EXCLUDED.userId or the incoming value as appropriate).apps/fastify/src/routes/reference/template.ts (1)
192-212: No user feedback on verification failure.When the API returns an error (invalid code, expired, rate-limited), the user sees no feedback—the form just does nothing. Consider displaying the error message from the response.
♻️ Proposed enhancement
try { const res = await fetch(apiUrl + '/auth/magiclink/verify', { method: 'POST', headers: { 'Content-Type': 'application/json' }, body: JSON.stringify({ verificationId: verificationIdFromUrl, token: code }), }); if (res.ok) { const data = await res.json(); localStorage.setItem('scalar-token', data.token); history.replaceState({}, '', '/reference'); updateScalarAuth(scalarApiReference, data.token); banner.remove(); if (window.updateLoginButton) window.updateLoginButton(); + } else { + const err = await res.json().catch(() => ({ message: 'Verification failed' })); + const errSpan = banner.querySelector('.error-msg') || (() => { + const s = document.createElement('span'); + s.className = 'error-msg'; + s.style.cssText = 'color:`#f87171`;margin-left:8px;'; + banner.querySelector('form')?.appendChild(s); + return s; + })(); + errSpan.textContent = err.message || 'Invalid code'; } } catch (err) { console.error(err); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/fastify/src/routes/reference/template.ts` around lines 192 - 212, The submit handler on banner.querySelector('form') currently ignores non-OK responses and exceptions so users receive no feedback; update the submit listener (the async callback attached to banner.querySelector('form') that uses apiUrl and verificationIdFromUrl) to handle res.ok === false and caught errors by reading the response body (await res.json() or text()) when res is not ok, extracting a readable error message, and rendering that message into the DOM (e.g., an error element inside banner near the input with aria-live="polite") so users see invalid/expired/rate-limit messages; also ensure caught exceptions display a generic error message in the same error element and that the element is cleared/removed on successful verification when updateScalarAuth runs.apps/fastify/src/lib/oauth-twitter.ts (1)
33-38: Consider keeping session issuance behind the shared helper.This helper now duplicates the session-row creation contract that
apps/fastify/src/lib/session.ts:15-49already owns. Reusing a single issuer will reduce drift if token payloads, expiry math, or session fields change again.Also applies to: 100-112
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/fastify/src/lib/oauth-twitter.ts` around lines 33 - 38, runTwitterExchangeTx duplicates the session-row creation logic that the shared session issuer helper (the function in session.ts that creates session rows and returns { userId, sessionId, refreshJti}) already implements; remove the duplicated session creation in runTwitterExchangeTx and the other duplicated block (around the 100-112 area) and instead import and call that shared session issuer function, passing the same inputs (db, user/account identifiers and any token/expiry data) and return its {userId, sessionId, refreshJti} so session issuance remains centralized and consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/fastify/src/lib/oauth-twitter.ts`:
- Around line 136-161: Change the generic throws for token/user fetch failures
to propagate upstream HTTP status and body so the caller can distinguish 4xx vs
5xx: when tokenRes.ok or userRes.ok is false, read the response body (text or
json) and throw a specific Error (or custom Error subclass) that includes a
stage identifier ("token_exchange" or "user_fetch"), the response status
(tokenRes.status / userRes.status) and the response body; similarly include the
upstream body when tokenData.error exists or access_token is missing. Update
references to TOKEN_EXCHANGE_FAILED and USER_FETCH_FAILED in the token exchange
block (tokenRes, tokenData, accessToken) and the user fetch block (userRes,
userData, twUser) to throw the enriched error instead of the current sentinels
so the route can map provider 4xx/5xx distinctly.
- Around line 164-171: The account row is inconsistent because accountData.scope
is hard-coded to include "offline.access" while accountData.refreshToken may be
null; update the accountData construction (TwitterAccountData / accountData) to
derive scope from the actual token response/request (e.g., use tokenData.scope
or the granted/requested scope variable) and only include "offline.access" when
the tokenData indicates it was granted or when refreshToken is non-null,
ensuring scope reflects actual granted permissions rather than a constant.
In `@apps/fastify/src/routes/auth/link/verify.test.ts`:
- Around line 43-46: Tighten the assertion in the test for
/auth/magiclink/verify: replace the broad expect([400, 401,
404]).toContain(response.statusCode) with an explicit
expect(response.statusCode).toBe(401), parse the response body
(JSON.parse(response.body)) and assert the error code equals 'INVALID_TOKEN';
update the test around the payload { email: 'nonexistent@example.com', token:
'000000' } in verify.test.ts and keep the rest of the test flow unchanged.
In `@apps/fastify/src/routes/auth/magiclink/verify.ts`:
- Around line 63-74: The IP-based rate limiter is bypassing Fastify's trustProxy
because getTrustedClientIp reads X-Forwarded-For directly; update
getTrustedClientIp to return request.ip (which respects Fastify's trustProxy
setting) and ensure verify.ts (where getTrustedClientIp is used) relies on that
value for authAttempts keying; keep the function name getTrustedClientIp and do
not parse headers manually so server.ts's trustProxy configuration is honored.
In `@apps/fastify/src/routes/test/magic-link.ts`:
- Around line 35-43: The query returns the latest verification for any `@test.ai`
address regardless of type; restrict it to magic-link rows by adding a condition
on verification.type (e.g., require verification.type === 'magic_link') in the
same where clause used for the query chain (the select/from/where/orderBy/limit
sequence that references verification, verification.identifier,
verification.tokenPlain and verification.createdAt) so the route only selects
rows inserted with type: 'magic_link' before falling back to
request.server.fakeEmail extraction.
In `@apps/fastify/test/utils/auth-helper.ts`:
- Line 56: The helper currently sends payload: { email, token } but then reads
the magic code from the global last-outbox message which can belong to a
different inbox; update the helper to explicitly bind the extracted code to the
requested inbox by locating the most recent email whose recipient matches the
provided email (not simply the last outbox entry), extract the magic link/code
from that message, and include that extracted code in the payload used for
verification (replace the stale use of token or last-outbox code). Target the
helper that builds the request payload (the place where payload: { email, token
} is set and where the outbox is scanned) and change the outbox lookup to filter
by recipient email before extracting the code.
In `@apps/next/app/auth/callback/magiclink/route.ts`:
- Around line 57-63: The current guard only checks presence of verificationId
and token before calling client.auth.magiclink.verify; add server-side
validation to ensure token matches a 6-digit numeric format (e.g., /^\d{6}$/)
and verificationId meets expected pattern/length before calling
magiclink.verify, and if validation fails redirect back to the verification step
(e.g., `/auth/callback/magiclink?message=INVALID_CODE` or similar) rather than
`/auth/login`; update the route handler in route.ts where verificationId, token,
and client.auth.magiclink.verify are used to enforce this check and return the
local error redirect without invoking the API.
- Around line 13-15: The current callbackURL extraction accepts
protocol-relative values like "//evil.example" because it only checks
startsWith('/'); update the validation around callbackURL (the
searchParams.get('callbackURL') logic and before new URL(callbackURL,
request.url) is used) to either: 1) only accept strings that start with a single
leading slash and not two (i.e., /^\/[^/].*/ or equivalent) so protocol-relative
values are rejected, or 2) parse the provided callbackURL with new
URL(callbackURL, request.url) and verify that url.origin === new
URL(request.url).origin before using it; reject or fallback to '/' if the check
fails. Ensure this change is applied wherever callbackURL is computed/used (the
callbackURL variable and the code that calls new URL(callbackURL, request.url)).
In `@apps/next/app/auth/callback/page.tsx`:
- Around line 28-32: The current redirect when params.token exists forwards only
token and callbackURL to /auth/callback/magiclink but the magiclink GET handler
now expects verificationId and treats token-only requests as INVALID_TOKEN;
preserve the legacy token flow by adding a legacy indicator and keeping the
original token parameter in the redirect so the server can detect and handle old
token links—update the redirect call in page.tsx (the block that reads
params.token and calls redirect) to include an extra query flag like
legacyToken=1 (or the agreed legacy query key) alongside token and callbackURL
so /auth/callback/magiclink (verificationId/INVALID_TOKEN logic) can route
legacy token flows correctly.
In `@apps/next/app/auth/login/login-form.tsx`:
- Around line 171-182: The component currently spreads caller-supplied props
last, letting a provided onSubmit override internal handlers; update both
LoginCodeView and LoginEmailView so {...props} is spread before the component's
explicit handlers (e.g. onSubmit={handleCodeSubmit},
onSubmit={handleEmailSubmit}), ensuring the internal functions
(handleCodeSubmit, handleEmailSubmit) and other explicit props take precedence
over any incoming onSubmit from LoginFormProps when showCodeEntry toggles.
In `@apps/next/lib/env.ts`:
- Around line 33-34: The NEXT_PUBLIC_LEGAL_EMAIL currently has a default
placeholder which should not be used in production; update the env schema for
NEXT_PUBLIC_LEGAL_EMAIL so it is required (no default) when running in
production and only use the 'legal@example.com' default for local/dev.
Concretely, modify the NEXT_PUBLIC_LEGAL_EMAIL validator in env.ts (the
z.string().email() entry) to conditionally apply .default('legal@example.com')
only when NODE_ENV !== 'production' (or validate/process env with a branch that
throws when missing in production) so production fails closed while local dev
keeps the convenience default.
---
Outside diff comments:
In `@apps/next/e2e/auth-helpers.ts`:
- Around line 53-74: The current extractMagicLinkData enforces both token and
verificationId, causing failures when only token exists; keep
extractMagicLinkData as-is for verifyMagicLink, but change or add extractToken
to call the same `${apiUrl}/test/magic-link/last` endpoint (reusing the
retry/delay logic) and return the token independently of verificationId (i.e.,
succeed when data.token exists even if data.verificationId is null), ensuring
loginAsTestUser()/enterLoginCodeAndSubmit() only requires the token while
leaving verifyMagicLink() to continue using extractMagicLinkData() when both
fields are needed.
---
Duplicate comments:
In `@apps/fastify/src/routes/auth/magiclink/request.ts`:
- Around line 41-48: The unique-violation handler currently only re-queries for
an existing user when attempt < maxRetries - 1, so on the last retry a
concurrent creation will surface as an exception; change the catch block that
checks isUniqueViolation(err) to always re-query the users table by email (using
db.select().from(users).where(eq(users.email, email))) and return the existing
record if found (regardless of attempt), otherwise continue or rethrow as
appropriate so the final retry returns the concurrently-created user instead of
throwing; references: isUniqueViolation,
db.select().from(users).where(eq(users.email, email)), attempt, maxRetries.
---
Nitpick comments:
In `@apps/fastify/src/lib/oauth-twitter.ts`:
- Around line 33-38: runTwitterExchangeTx duplicates the session-row creation
logic that the shared session issuer helper (the function in session.ts that
creates session rows and returns { userId, sessionId, refreshJti}) already
implements; remove the duplicated session creation in runTwitterExchangeTx and
the other duplicated block (around the 100-112 area) and instead import and call
that shared session issuer function, passing the same inputs (db, user/account
identifiers and any token/expiry data) and return its {userId, sessionId,
refreshJti} so session issuance remains centralized and consistent.
In `@apps/fastify/src/routes/auth/oauth/google/verify-id-token.ts`:
- Around line 23-79: The accountData object in runGoogleVerifyIdTokenTx declares
unused fields (accessToken, refreshToken, idToken, accessTokenExpiresAt,
refreshTokenExpiresAt) that are never written to the DB; remove these unused
fields from accountData (or from its construction) and ensure the insert branch
that creates a new account only supplies the actual persisted properties (id,
userId, accountId, providerId, scope), referencing
accountData.id/userId/accountId/providerId/scope and leaving out the unused
token/expiry properties to keep the shape minimal and avoid dead fields.
- Around line 39-70: Add a composite unique constraint on (providerId,
accountId) in the account table schema and change the insert logic in
verify-id-token.ts to use an upsert instead of a plain insert to avoid race
conditions: keep the initial lookup (tx.select... from account where ...) if you
want to prefer an existing row, but replace the tx.insert(account).values({...})
branch with a tx.insert(...).values(...).onConflictDoUpdate(...) that targets
the (account.providerId, account.accountId) constraint and updates relevant
fields (userId, updatedAt, and any tokens/scopes) so concurrent inserts merge
safely; ensure you still set id to a generated UUID when inserting and preserve
existingAccount.id when updating (use the conflict update to set userId =
EXCLUDED.userId or the incoming value as appropriate).
In `@apps/fastify/src/routes/reference/template.ts`:
- Around line 192-212: The submit handler on banner.querySelector('form')
currently ignores non-OK responses and exceptions so users receive no feedback;
update the submit listener (the async callback attached to
banner.querySelector('form') that uses apiUrl and verificationIdFromUrl) to
handle res.ok === false and caught errors by reading the response body (await
res.json() or text()) when res is not ok, extracting a readable error message,
and rendering that message into the DOM (e.g., an error element inside banner
near the input with aria-live="polite") so users see invalid/expired/rate-limit
messages; also ensure caught exceptions display a generic error message in the
same error element and that the element is cleared/removed on successful
verification when updateScalarAuth runs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 838364a9-c8ed-4aa8-8406-1aea072b75d1
⛔ Files ignored due to path filters (4)
apps/fastify/src/db/migrations/0014_slippery_puff_adder.sqlis excluded by!apps/fastify/src/db/migrations/**apps/fastify/src/db/migrations/meta/0014_snapshot.jsonis excluded by!apps/fastify/src/db/migrations/**apps/fastify/src/db/migrations/meta/_journal.jsonis excluded by!apps/fastify/src/db/migrations/**packages/core/src/gen/types.gen.tsis excluded by!**/gen/**,!**/gen/**,!**/*.gen.ts
📒 Files selected for processing (31)
apps/fastify/eslint.config.mjsapps/fastify/openapi/openapi.jsonapps/fastify/src/db/schema/tables/auth-attempts.tsapps/fastify/src/lib/oauth-twitter.tsapps/fastify/src/lib/request.tsapps/fastify/src/routes/account/link/email/request.test.tsapps/fastify/src/routes/account/link/wallet/unlink.test.tsapps/fastify/src/routes/account/link/wallet/verify.test.tsapps/fastify/src/routes/auth/link/verify.test.tsapps/fastify/src/routes/auth/magiclink/request.test.tsapps/fastify/src/routes/auth/magiclink/request.tsapps/fastify/src/routes/auth/magiclink/verify.test.tsapps/fastify/src/routes/auth/magiclink/verify.tsapps/fastify/src/routes/auth/oauth/google/verify-id-token.tsapps/fastify/src/routes/auth/oauth/twitter/exchange.tsapps/fastify/src/routes/auth/session/logout.test.tsapps/fastify/src/routes/auth/session/refresh.test.tsapps/fastify/src/routes/reference.tsapps/fastify/src/routes/reference/template.tsapps/fastify/src/routes/test/magic-link.test.tsapps/fastify/src/routes/test/magic-link.tsapps/fastify/test/utils/auth-helper.tsapps/fastify/test/utils/fake-email.tsapps/next/app/auth/callback/magiclink/route.tsapps/next/app/auth/callback/page.tsxapps/next/app/auth/login/login-form.tsxapps/next/app/privacy/page.tsxapps/next/app/terms/page.tsxapps/next/components/policy-page-shell.tsxapps/next/e2e/auth-helpers.tsapps/next/lib/env.ts
✅ Files skipped from review due to trivial changes (1)
- apps/fastify/src/routes/account/link/email/request.test.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- apps/fastify/src/routes/auth/magiclink/request.test.ts
- apps/fastify/src/db/schema/tables/auth-attempts.ts
- apps/fastify/src/lib/request.ts
- apps/next/app/privacy/page.tsx
- apps/fastify/src/routes/auth/magiclink/verify.test.ts
… and UX - oauth-twitter: propagate upstream status/body in OAuthUpstreamError, derive scope from token - oauth-twitter: use createSessionAndIssueTokens instead of duplicating session logic - verify tests: assert 401 + INVALID_TOKEN for invalid token cases - getTrustedClientIp: use request.ip to respect Fastify trustProxy - magic-link test route: filter by verification.type === magic_link - auth-helper: extract token from email matching requested recipient - magiclink route: validate token format, verificationId, callbackURL; handle protocol-relative - callback page: add legacyToken=1 for token-only redirect - login-form: spread props before explicit handlers so internal handlers win - env: require NEXT_PUBLIC_LEGAL_EMAIL in production - e2e auth-helpers: extractToken polls /test/magic-link/last for token-only - magiclink request: always re-query on unique violation - google verify-id-token: remove unused accountData fields, use upsert - account schema: add unique(providerId, accountId) - reference template: show error message on verify failure
This was referenced Mar 10, 2026
Merged
Merged
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
Improvements