feat(auth): email verification via 6-digit code#2824
Closed
Conversation
Contributor
|
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:
📝 WalkthroughWalkthroughAdds a code-based email verification system: DB migration and ORM schema, repository with advisory locks, service implementing send/verify logic (cooldown, expiry, attempts), controller routes, notification template, Auth0 integration, SDK/client methods, and frontend UI + tests for six-digit code flow. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant Client as Frontend
participant Auth as AuthController
participant Service as EmailVerificationCodeService
participant Repo as EmailVerificationCodeRepository
participant DB as Database
participant Auth0 as Auth0Service
participant Notify as NotificationService
rect rgba(100,150,200,0.5)
Note over User,Notify: Send Verification Code Flow
User->>Client: Click "Send Code"
Client->>Auth: POST /v1/send-verification-code
Auth->>Service: sendCode(userInternalId)
Service->>Repo: acquireUserLock(userId)
Service->>Repo: findActiveByUserId(userId)
alt Active code within cooldown
Service-->>Auth: return existing codeSentAt
else Create new code
Service->>Repo: deleteByUserId(userId)
Service->>Repo: create(code, expiresAt)
Repo->>DB: INSERT email_verification_codes
Service->>Notify: publish email verification notification
Service-->>Auth: return new codeSentAt
end
Auth-->>Client: 200 { codeSentAt }
Client-->>User: show "Verification code sent"
end
rect rgba(150,200,100,0.5)
Note over User,Auth0: Verify Email Code Flow
User->>Client: Enter 6-digit code
Client->>Auth: POST /v1/verify-email-code
Auth->>Service: verifyCode(userInternalId, code)
Service->>Repo: acquireUserLock(userId)
Service->>Repo: findActiveByUserId(userId)
alt No active code or attempts exceeded
Service-->>Auth: error (invalid/expired/max attempts)
else Invalid code
Service->>Repo: incrementAttempts(id)
Service-->>Auth: invalid code error
else Valid code
Service->>Auth0: markEmailVerified(auth0Id)
Service->>DB: UPDATE user emailVerified=true
Service->>Repo: deleteByUserId(userId)
Service-->>Auth: { emailVerified: true }
end
Auth-->>Client: Response
Client-->>User: show result
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
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)
Comment |
❌ 21 Tests Failed:
View the top 3 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
apps/deploy-web/src/hooks/useEmailVerificationRequiredEventHandler.tsx (1)
61-61:⚠️ Potential issue | 🟡 MinorAdd
authandanalyticsServiceto theuseCallbackdependency array.Both
authandanalyticsServiceare used inside the callback (at lines 28 and 33–34) but are missing from the dependency array. While the DI container caches service instances within each container lifecycle, the container itself is recreated when settings change (API endpoint or blockchain status). Without including these services in the deps, the callback retains stale references if the services are reinitialized.🛠️ Proposed fix
- [user?.emailVerified, user?.id, requireAction, enqueueSnackbar] + [user?.emailVerified, user?.id, requireAction, enqueueSnackbar, auth, analyticsService]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/deploy-web/src/hooks/useEmailVerificationRequiredEventHandler.tsx` at line 61, The useCallback created in useEmailVerificationRequiredEventHandler references auth and analyticsService but they are missing from its dependency array; update the dependency array (currently including [user?.emailVerified, user?.id, requireAction, enqueueSnackbar]) to also include auth and analyticsService so the callback picks up reinitialized service instances; locate the useCallback in useEmailVerificationRequiredEventHandler and add auth and analyticsService to its deps to avoid stale references.apps/deploy-web/src/components/onboarding/steps/EmailVerificationContainer/EmailVerificationContainer.spec.tsx (2)
8-8: 🛠️ Refactor suggestion | 🟠 MajorUse
EmailVerificationContainer.nameinstead of a hardcoded string.As per coding guidelines, the root
describesuite should use<Subject>.nameto enable automated refactoring tools to find all references.-describe("EmailVerificationContainer", () => { +describe(EmailVerificationContainer.name, () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/deploy-web/src/components/onboarding/steps/EmailVerificationContainer/EmailVerificationContainer.spec.tsx` at line 8, Replace the hardcoded string in the root test suite with the component's name symbol: change the describe("EmailVerificationContainer", ...) to use EmailVerificationContainer.name (i.e., describe(EmailVerificationContainer.name, ...)); ensure EmailVerificationContainer is imported or in scope in EmailVerificationContainer.spec.tsx so the .name property resolves, and run tests to confirm no runtime issues.
9-9: 🛠️ Refactor suggestion | 🟠 MajorTest descriptions should use present simple, 3rd person singular, without "should".
All
it(...)descriptions in this file are prefixed with "should". Per coding guidelines, use present simple form instead.Examples:
"should render children with initial state"→"renders children with initial state""should auto-send code on mount when email is not verified"→"auto-sends code on mount when email is not verified""should handle verify code error"→"exposes verifyError on verify code failure"Also applies to: 26-26, 32-32, 40-40, 64-64, 79-79, 103-103, 118-118, 142-142
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/deploy-web/src/components/onboarding/steps/EmailVerificationContainer/EmailVerificationContainer.spec.tsx` at line 9, Update all test case descriptions in EmailVerificationContainer.spec.tsx that begin with "should" to use present simple, 3rd person singular without the word "should" (i.e., change the string passed to each it(...) call). Specifically, replace "should render children with initial state", "should auto-send code on mount when email is not verified", "should handle verify code error", and the other occurrences noted in the review with forms like "renders children with initial state", "auto-sends code on mount when email is not verified", and "exposes verifyError on verify code failure" respectively so the it(...) descriptions follow the guideline.
🧹 Nitpick comments (9)
apps/deploy-web/src/components/onboarding/steps/EmailVerificationStep/EmailVerificationStep.tsx (3)
59-66: Same concern —handleKeyDownrecreated on every keystroke due todigitsdep.Similar to
handleDigitChange, capturingdigitsand listing it as a dependency means this callback is never memoized. You can readdigits[index]via the ref or a functional pattern to stabilize it.♻️ Suggested refactor using ref
+ const digitsRef = useRef(digits); + digitsRef.current = digits; + const handleKeyDown = useCallback( (index: number, e: React.KeyboardEvent<HTMLInputElement>) => { - if (e.key === "Backspace" && !digits[index] && index > 0) { + if (e.key === "Backspace" && !digitsRef.current[index] && index > 0) { inputRefs.current[index - 1]?.focus(); } }, - [digits] + [] );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/deploy-web/src/components/onboarding/steps/EmailVerificationStep/EmailVerificationStep.tsx` around lines 59 - 66, handleKeyDown is being re-created every keystroke because it depends on the state array digits; change it to read the current value from a stable ref instead of listing digits as a dependency so the callback can be memoized. Create a digitsRef (e.g., const digitsRef = useRef(digits)) and update digitsRef.current inside a useEffect whenever digits changes, then change handleKeyDown to reference digitsRef.current[index] and remove digits from its dependency array (or give it an empty array); keep using inputRefs.current[...]?.focus() as before.
68-89:handlePastedoesn't actually need the currentdigits— it overwrites all 6 slots.The loop on lines 77–79 overwrites every index with
pasted[i] || "", so the[...digits]spread and thedigitsdependency are unnecessary. Starting from a fresh array avoids a spurious callback re-creation every keystroke.♻️ Suggested refactor
const handlePaste = useCallback( (e: React.ClipboardEvent) => { e.preventDefault(); if (isVerifying) return; const pasted = e.clipboardData.getData("text").replace(/\D/g, "").slice(0, 6); if (!pasted) return; - const newDigits = [...digits]; + const newDigits = Array(6).fill(""); for (let i = 0; i < 6; i++) { newDigits[i] = pasted[i] || ""; } setDigits(newDigits); ... }, - [digits, isVerifying, onVerifyCode] + [isVerifying, onVerifyCode] );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/deploy-web/src/components/onboarding/steps/EmailVerificationStep/EmailVerificationStep.tsx` around lines 68 - 89, The handlePaste callback unnecessarily depends on digits and creates newDigits from [...digits] even though it overwrites all 6 entries; change handlePaste (the useCallback) to build a fresh array of six entries (e.g., Array(6).fill("") then assign pasted chars) instead of spreading digits, remove digits from the dependency array, and keep isVerifying, onVerifyCode and inputRefs in the dependency list; update setDigits(newArray) and preserve the existing pasted.length logic that calls onVerifyCode(pasted) or focuses inputRefs.current[pasted.length].
32-50:digitsin the dependency array defeats memoization — consider a functional updater.
handleDigitChangecloses overdigitsand includes it in the deps array, so the callback is recreated on every keystroke. Using a functional state updater (setDigits(prev => ...)) would let you dropdigitsfrom the deps, givinguseCallbacka stable reference. This also eliminates any risk of stale-closure bugs with rapid input.♻️ Suggested refactor
const handleDigitChange = useCallback( (index: number, value: string) => { if (!/^\d*$/.test(value) || isVerifying) return; - const newDigits = [...digits]; - newDigits[index] = value.slice(-1); - setDigits(newDigits); - - if (value && index < 5) { - inputRefs.current[index + 1]?.focus(); - } - - const code = newDigits.join(""); - if (code.length === 6) { - onVerifyCode(code); - } + setDigits(prev => { + const newDigits = [...prev]; + newDigits[index] = value.slice(-1); + + if (value && index < 5) { + inputRefs.current[index + 1]?.focus(); + } + + const code = newDigits.join(""); + if (code.length === 6) { + onVerifyCode(code); + } + + return newDigits; + }); }, - [digits, isVerifying, onVerifyCode] + [isVerifying, onVerifyCode] );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/deploy-web/src/components/onboarding/steps/EmailVerificationStep/EmailVerificationStep.tsx` around lines 32 - 50, The handleDigitChange callback closes over digits and therefore is recreated each keystroke; change it to use a functional state updater to avoid referencing digits in the deps: inside handleDigitChange (and keeping checks for /^\d*$/ and isVerifying), call setDigits(prev => { const newDigits = [...prev]; newDigits[index] = value.slice(-1); if (value && index < 5) inputRefs.current[index + 1]?.focus(); const code = newDigits.join(""); if (code.length === 6) onVerifyCode(code); return newDigits; }); then update the useCallback dependency array to remove digits (keep isVerifying and onVerifyCode) so handleDigitChange remains stable and avoids stale closures.apps/deploy-web/src/components/onboarding/steps/EmailVerificationContainer/EmailVerificationContainer.tsx (1)
54-67: Cooldown interval is torn down and recreated every second.Because
cooldownSecondsis in the dependency array, each tick triggers a cleanup → re-run cycle, so thesetIntervalnever fires more than once before being cleared. It works, but is effectively a recursivesetTimeoutwearing ansetIntervalcostume, and creates unnecessary work each second.A cleaner pattern initializes the interval once and clears it when the countdown reaches zero:
♻️ Suggested refactor
useEffect(() => { if (cooldownSeconds <= 0) return; - cooldownRef.current = cooldownSeconds; const timer = setInterval(() => { setCooldownSeconds(prev => { const next = prev <= 1 ? 0 : prev - 1; cooldownRef.current = next; + if (next === 0) clearInterval(timer); return next; }); }, 1000); return () => clearInterval(timer); - }, [cooldownSeconds]); + }, [cooldownSeconds > 0]);Note: Passing
cooldownSeconds > 0(a boolean) ensures the effect only re-runs on transitions between zero and non-zero, not on every decrement. You'd still want to synccooldownRef.currentwhen a new cooldown is started (already handled insendCode).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/deploy-web/src/components/onboarding/steps/EmailVerificationContainer/EmailVerificationContainer.tsx` around lines 54 - 67, The effect recreates the interval every second because cooldownSeconds is in the dependency array; change the effect to only run when the countdown state transitions between zero and non-zero (use a boolean dependency like cooldownSeconds > 0 or similar) so the interval is created once while active, update cooldownRef.current when starting a new cooldown (sendCode already does this but ensure it's done here on start), inside the interval call setCooldownSeconds(prev => { const next = prev <= 1 ? 0 : prev - 1; if (next === 0) clearInterval(timer); cooldownRef.current = next; return next; }) and ensure the cleanup still clears the timer in the returned function; reference the existing cooldownSeconds, cooldownRef, setCooldownSeconds, and sendCode identifiers to locate where to apply this change.packages/http-sdk/src/auth/auth-http.service.ts (1)
11-13: Consider deprecatingsendVerificationEmail.The old
sendVerificationEmail(userId)method still accepts a client-supplieduserIdand targets the legacy link-based verification endpoint. If the new code-based flow is the intended replacement, marking this method as deprecated (e.g.,@deprecated) would signal to consumers to migrate.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/http-sdk/src/auth/auth-http.service.ts` around lines 11 - 13, Mark the legacy sendVerificationEmail method as deprecated by adding a `@deprecated` JSDoc comment on the sendVerificationEmail function noting it targets the legacy "/v1/send-verification-email" link-based endpoint and that consumers should migrate to the new code-based flow; update the comment to reference the replacement API/client method name (e.g., sendVerificationCode or sendVerificationCodeEmail) and any migration notes so callers know which method to use instead.apps/api/drizzle/meta/0029_snapshot.json (1)
1160-1259:email_verification_codestable looks well-designed.The schema has appropriate columns, indexes on
user_idandexpires_at, and cascading FK delete. Theexpires_atindex will support efficient cleanup of expired codes.One consideration: there's no TTL/cleanup mechanism visible in this PR for pruning expired rows. Over time, this table could accumulate stale records. You may want to add a periodic job or a cleanup step in
sendCodethat deletes old expired codes for the user.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/drizzle/meta/0029_snapshot.json` around lines 1160 - 1259, The table email_verification_codes lacks a TTL/cleanup mechanism so expired codes will accumulate; add a periodic cleanup or inline pruning in the code that creates codes (e.g., inside sendCode) to remove stale rows by executing a DELETE FROM email_verification_codes WHERE expires_at < now() (optionally scoped to user_id) before/after inserting a new code, and/or register a scheduled job/worker (cron/background task) that runs that same delete using the expires_at column and can leverage the email_verification_codes_expires_at_idx for efficiency.apps/api/src/notifications/services/notification-templates/email-verification-code-notification.ts (1)
12-14: Consider making the expiration duration configurable rather than hardcoded.The "10 minutes" text currently matches
CODE_EXPIRY_MS = 10 * 60 * 1000inEmailVerificationCodeService, but having the same value duplicated as a hardcoded string creates maintenance risk. If the TTL is ever changed, the template text could become stale. Either export the constant and reference it in the template, or pass the expiration duration as avarsfield during notification creation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/notifications/services/notification-templates/email-verification-code-notification.ts` around lines 12 - 14, The template hardcodes "10 minutes" which can drift from the TTL in EmailVerificationCodeService; to fix, make the expiry text derive from the canonical constant or passed var instead of a literal: either export CODE_EXPIRY_MS from EmailVerificationCodeService and use that exported value (converted to minutes) when building the template in email-verification-code-notification.ts, or update the Notification creation flow to include an expiry field in vars (e.g., vars.expiryMinutes) and reference that in the template string so the displayed duration always matches the service TTL.apps/api/src/auth/model-schemas/email-verification-code/email-verification-code.schema.ts (1)
23-24: Consider a composite index for active-code lookups.
findActiveByUserIdfilters by bothuser_idandexpires_at, so a composite(user_id, expires_at)index is typically a better fit than separate single-column indexes for this query path.⚙️ Suggested index adjustment
table => ({ - userIdIdx: index("email_verification_codes_user_id_idx").on(table.userId), - expiresAtIdx: index("email_verification_codes_expires_at_idx").on(table.expiresAt) + userIdExpiresAtIdx: index("email_verification_codes_user_id_expires_at_idx").on(table.userId, table.expiresAt) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/auth/model-schemas/email-verification-code/email-verification-code.schema.ts` around lines 23 - 24, Replace the two single-column indexes userIdIdx and expiresAtIdx with a composite index on (userId, expiresAt) to optimize the findActiveByUserId query; update the schema to remove userIdIdx and expiresAtIdx and add a single composite index (e.g. "email_verification_codes_user_id_expires_at_idx") defined with .on(table.userId, table.expiresAt) so lookups that filter by user_id and expires_at use the composite index.apps/api/drizzle/0029_fluffy_drax.sql (1)
17-17: Enforce one-row-per-user at the database layer.Current service flow assumes a single code row per user; making
user_idunique hardens that invariant against future regressions/manual writes.Suggested migration adjustment
-CREATE INDEX IF NOT EXISTS "email_verification_codes_user_id_idx" ON "email_verification_codes" USING btree ("user_id"); +CREATE UNIQUE INDEX IF NOT EXISTS "email_verification_codes_user_id_unique" ON "email_verification_codes" USING btree ("user_id");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/drizzle/0029_fluffy_drax.sql` at line 17, The migration currently creates a non-unique index email_verification_codes_user_id_idx on email_verification_codes(user_id); replace this with a uniqueness enforcement so the DB guarantees one row per user: either drop/omit that CREATE INDEX and add a UNIQUE constraint or create a UNIQUE INDEX on email_verification_codes(user_id) (e.g., CREATE UNIQUE INDEX IF NOT EXISTS email_verification_codes_user_id_idx ON email_verification_codes USING btree (user_id)); update any migration logic that assumes a non-unique index accordingly and ensure the migration is idempotent (use IF NOT EXISTS or add a CONSTRAINT ... UNIQUE if preferred).
🤖 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/api/src/auth/services/email-verification-code/email-verification-code.service.spec.ts`:
- Around line 205-224: The setup() test helper lacks the repo's required
signature; change the setup function to accept a single inline-typed parameter
for per-test overrides (e.g., setup({ emailVerificationCodeRepository =
mock<EmailVerificationCodeRepository>(), auth0Service = mock<Auth0Service>(),
notificationService = mock<NotificationService>(), userRepository =
mock<UserRepository>(), logger = mock<LoggerService>() } = {}) ) and use those
values when constructing the EmailVerificationCodeService instance; keep the
existing default behavior (including
emailVerificationCodeRepository.acquireUserLock.mockResolvedValue(undefined))
but allow callers to override any mock via the single-argument parameter to
conform to the spec-test convention.
In
`@apps/api/src/auth/services/email-verification-code/email-verification-code.service.ts`:
- Around line 65-89: The verifyCode flow currently calls
auth0Service.markEmailVerified while inside the WithTransaction/lock (see
verifyCode, acquireUserLock, WithTransaction), which risks divergence if the
external call succeeds but local DB updates (userRepository.updateById,
emailVerificationCodeRepository.deleteByUserId) later fail; move the Auth0
mutation out of the transaction by either (a) recording an "email-verified"
outbox/event row in the same transaction and having an independent worker
publish it after commit, or (b) register a post-commit callback that invokes
auth0Service.markEmailVerified(user.userId) only after the transaction
successfully commits; remove the direct call to auth0Service.markEmailVerified
from inside verifyCode and trigger it from the outbox/post-commit handler
instead.
In
`@apps/deploy-web/src/components/onboarding/steps/EmailVerificationContainer/EmailVerificationContainer.spec.tsx`:
- Line 181: The tests use `any` for the `user` parameter in the setup function
and for Snackbar mock props—replace these with concrete types: change the
`setup` function signature to accept a proper User type (e.g., import or define
the app's User/Account interface and use it for the `user` parameter) and type
the Snackbar mock props with the actual SnackbarProps/ToastProps interface used
by your UI library or the component under test (reference the Snackbar mock
object in the spec). Update imports to bring in the User and SnackbarProps types
and adjust any mock objects to satisfy those types (e.g., required fields and
callback signatures) so the test no longer uses `any`.
In
`@apps/deploy-web/src/components/onboarding/VerifyEmailPage/VerifyEmailPage.spec.tsx`:
- Around line 28-61: The test setup mutates globals (window.localStorage,
window.location) and returns a stale capturedHref string; update the setup
function used for VerifyEmailPage tests to accept a single inline-typed
parameter, save the original window.localStorage and window.location, replace
them with mockLocalStorage and a mock location object (exposing get/set for href
and/or an assign spy), and after render return both the mocks and a restore
function (or ensure callers call the restore) so tests can restore the original
globals; also change the returned mockLocationAssign to be a live accessor or
spy function (not the one-time capturedHref value) so redirect assertions read
the current href. Ensure you reference and update the setup function,
mockLocalStorage, capturedHref usage, and the
window.localStorage/window.location replacements around render(<VerifyEmailPage
dependencies={dependencies} />).
---
Outside diff comments:
In
`@apps/deploy-web/src/components/onboarding/steps/EmailVerificationContainer/EmailVerificationContainer.spec.tsx`:
- Line 8: Replace the hardcoded string in the root test suite with the
component's name symbol: change the describe("EmailVerificationContainer", ...)
to use EmailVerificationContainer.name (i.e.,
describe(EmailVerificationContainer.name, ...)); ensure
EmailVerificationContainer is imported or in scope in
EmailVerificationContainer.spec.tsx so the .name property resolves, and run
tests to confirm no runtime issues.
- Line 9: Update all test case descriptions in
EmailVerificationContainer.spec.tsx that begin with "should" to use present
simple, 3rd person singular without the word "should" (i.e., change the string
passed to each it(...) call). Specifically, replace "should render children with
initial state", "should auto-send code on mount when email is not verified",
"should handle verify code error", and the other occurrences noted in the review
with forms like "renders children with initial state", "auto-sends code on mount
when email is not verified", and "exposes verifyError on verify code failure"
respectively so the it(...) descriptions follow the guideline.
In `@apps/deploy-web/src/hooks/useEmailVerificationRequiredEventHandler.tsx`:
- Line 61: The useCallback created in useEmailVerificationRequiredEventHandler
references auth and analyticsService but they are missing from its dependency
array; update the dependency array (currently including [user?.emailVerified,
user?.id, requireAction, enqueueSnackbar]) to also include auth and
analyticsService so the callback picks up reinitialized service instances;
locate the useCallback in useEmailVerificationRequiredEventHandler and add auth
and analyticsService to its deps to avoid stale references.
---
Nitpick comments:
In `@apps/api/drizzle/0029_fluffy_drax.sql`:
- Line 17: The migration currently creates a non-unique index
email_verification_codes_user_id_idx on email_verification_codes(user_id);
replace this with a uniqueness enforcement so the DB guarantees one row per
user: either drop/omit that CREATE INDEX and add a UNIQUE constraint or create a
UNIQUE INDEX on email_verification_codes(user_id) (e.g., CREATE UNIQUE INDEX IF
NOT EXISTS email_verification_codes_user_id_idx ON email_verification_codes
USING btree (user_id)); update any migration logic that assumes a non-unique
index accordingly and ensure the migration is idempotent (use IF NOT EXISTS or
add a CONSTRAINT ... UNIQUE if preferred).
In `@apps/api/drizzle/meta/0029_snapshot.json`:
- Around line 1160-1259: The table email_verification_codes lacks a TTL/cleanup
mechanism so expired codes will accumulate; add a periodic cleanup or inline
pruning in the code that creates codes (e.g., inside sendCode) to remove stale
rows by executing a DELETE FROM email_verification_codes WHERE expires_at <
now() (optionally scoped to user_id) before/after inserting a new code, and/or
register a scheduled job/worker (cron/background task) that runs that same
delete using the expires_at column and can leverage the
email_verification_codes_expires_at_idx for efficiency.
In
`@apps/api/src/auth/model-schemas/email-verification-code/email-verification-code.schema.ts`:
- Around line 23-24: Replace the two single-column indexes userIdIdx and
expiresAtIdx with a composite index on (userId, expiresAt) to optimize the
findActiveByUserId query; update the schema to remove userIdIdx and expiresAtIdx
and add a single composite index (e.g.
"email_verification_codes_user_id_expires_at_idx") defined with
.on(table.userId, table.expiresAt) so lookups that filter by user_id and
expires_at use the composite index.
In
`@apps/api/src/notifications/services/notification-templates/email-verification-code-notification.ts`:
- Around line 12-14: The template hardcodes "10 minutes" which can drift from
the TTL in EmailVerificationCodeService; to fix, make the expiry text derive
from the canonical constant or passed var instead of a literal: either export
CODE_EXPIRY_MS from EmailVerificationCodeService and use that exported value
(converted to minutes) when building the template in
email-verification-code-notification.ts, or update the Notification creation
flow to include an expiry field in vars (e.g., vars.expiryMinutes) and reference
that in the template string so the displayed duration always matches the service
TTL.
In
`@apps/deploy-web/src/components/onboarding/steps/EmailVerificationContainer/EmailVerificationContainer.tsx`:
- Around line 54-67: The effect recreates the interval every second because
cooldownSeconds is in the dependency array; change the effect to only run when
the countdown state transitions between zero and non-zero (use a boolean
dependency like cooldownSeconds > 0 or similar) so the interval is created once
while active, update cooldownRef.current when starting a new cooldown (sendCode
already does this but ensure it's done here on start), inside the interval call
setCooldownSeconds(prev => { const next = prev <= 1 ? 0 : prev - 1; if (next ===
0) clearInterval(timer); cooldownRef.current = next; return next; }) and ensure
the cleanup still clears the timer in the returned function; reference the
existing cooldownSeconds, cooldownRef, setCooldownSeconds, and sendCode
identifiers to locate where to apply this change.
In
`@apps/deploy-web/src/components/onboarding/steps/EmailVerificationStep/EmailVerificationStep.tsx`:
- Around line 59-66: handleKeyDown is being re-created every keystroke because
it depends on the state array digits; change it to read the current value from a
stable ref instead of listing digits as a dependency so the callback can be
memoized. Create a digitsRef (e.g., const digitsRef = useRef(digits)) and update
digitsRef.current inside a useEffect whenever digits changes, then change
handleKeyDown to reference digitsRef.current[index] and remove digits from its
dependency array (or give it an empty array); keep using
inputRefs.current[...]?.focus() as before.
- Around line 68-89: The handlePaste callback unnecessarily depends on digits
and creates newDigits from [...digits] even though it overwrites all 6 entries;
change handlePaste (the useCallback) to build a fresh array of six entries
(e.g., Array(6).fill("") then assign pasted chars) instead of spreading digits,
remove digits from the dependency array, and keep isVerifying, onVerifyCode and
inputRefs in the dependency list; update setDigits(newArray) and preserve the
existing pasted.length logic that calls onVerifyCode(pasted) or focuses
inputRefs.current[pasted.length].
- Around line 32-50: The handleDigitChange callback closes over digits and
therefore is recreated each keystroke; change it to use a functional state
updater to avoid referencing digits in the deps: inside handleDigitChange (and
keeping checks for /^\d*$/ and isVerifying), call setDigits(prev => { const
newDigits = [...prev]; newDigits[index] = value.slice(-1); if (value && index <
5) inputRefs.current[index + 1]?.focus(); const code = newDigits.join(""); if
(code.length === 6) onVerifyCode(code); return newDigits; }); then update the
useCallback dependency array to remove digits (keep isVerifying and
onVerifyCode) so handleDigitChange remains stable and avoids stale closures.
In `@packages/http-sdk/src/auth/auth-http.service.ts`:
- Around line 11-13: Mark the legacy sendVerificationEmail method as deprecated
by adding a `@deprecated` JSDoc comment on the sendVerificationEmail function
noting it targets the legacy "/v1/send-verification-email" link-based endpoint
and that consumers should migrate to the new code-based flow; update the comment
to reference the replacement API/client method name (e.g., sendVerificationCode
or sendVerificationCodeEmail) and any migration notes so callers know which
method to use instead.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
apps/api/drizzle/0029_fluffy_drax.sqlapps/api/drizzle/meta/0029_snapshot.jsonapps/api/drizzle/meta/_journal.jsonapps/api/src/auth/controllers/auth/auth.controller.tsapps/api/src/auth/model-schemas/email-verification-code/email-verification-code.schema.tsapps/api/src/auth/model-schemas/index.tsapps/api/src/auth/repositories/email-verification-code/email-verification-code.repository.tsapps/api/src/auth/routes/index.tsapps/api/src/auth/routes/send-verification-code/send-verification-code.router.tsapps/api/src/auth/routes/verify-email-code/verify-email-code.router.tsapps/api/src/auth/services/auth0/auth0.service.tsapps/api/src/auth/services/email-verification-code/email-verification-code.service.spec.tsapps/api/src/auth/services/email-verification-code/email-verification-code.service.tsapps/api/src/notifications/services/notification-templates/email-verification-code-notification.tsapps/api/src/rest-app.tsapps/deploy-web/src/components/onboarding/VerifyEmailPage/VerifyEmailPage.spec.tsxapps/deploy-web/src/components/onboarding/VerifyEmailPage/VerifyEmailPage.tsxapps/deploy-web/src/components/onboarding/steps/EmailVerificationContainer/EmailVerificationContainer.spec.tsxapps/deploy-web/src/components/onboarding/steps/EmailVerificationContainer/EmailVerificationContainer.tsxapps/deploy-web/src/components/onboarding/steps/EmailVerificationStep/EmailVerificationStep.tsxapps/deploy-web/src/hooks/useEmailVerificationRequiredEventHandler.tsxapps/notifications/src/modules/notifications/services/email-sender/email-sender.service.tspackages/http-sdk/src/auth/auth-http.service.tspackages/http-sdk/src/auth/auth-http.types.ts
apps/api/src/auth/services/email-verification-code/email-verification-code.service.spec.ts
Outdated
Show resolved
Hide resolved
apps/api/src/auth/services/email-verification-code/email-verification-code.service.ts
Outdated
Show resolved
Hide resolved
...c/components/onboarding/steps/EmailVerificationContainer/EmailVerificationContainer.spec.tsx
Outdated
Show resolved
Hide resolved
apps/deploy-web/src/components/onboarding/VerifyEmailPage/VerifyEmailPage.spec.tsx
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
apps/deploy-web/src/components/onboarding/VerifyEmailPage/VerifyEmailPage.spec.tsx (1)
10-29:⚠️ Potential issue | 🟡 Minor
restore()must be called in afinallyblock to prevent global state leaks on assertion failures.If any
expect(...)throws,restore()on lines 14, 21, and 28 is never reached, leavingwindow.localStorageandwindow.locationpermanently replaced for all subsequent tests in the suite.🛡️ Proposed fix: wrap assertions in try/finally
it("shows redirect loading text", () => { const { restore } = setup(); - - expect(screen.queryByText("Redirecting to email verification...")).toBeInTheDocument(); - restore(); + try { + expect(screen.queryByText("Redirecting to email verification...")).toBeInTheDocument(); + } finally { + restore(); + } }); it("sets onboarding step to EMAIL_VERIFICATION in localStorage", () => { const { mockLocalStorage, restore } = setup(); - - expect(mockLocalStorage.setItem).toHaveBeenCalledWith("onboardingStep", "2"); - restore(); + try { + expect(mockLocalStorage.setItem).toHaveBeenCalledWith("onboardingStep", "2"); + } finally { + restore(); + } }); it("redirects to onboarding page", () => { const { getLocationHref, restore } = setup(); - - expect(getLocationHref()).toBe("/signup?return-to=%2F"); - restore(); + try { + expect(getLocationHref()).toBe("/signup?return-to=%2F"); + } finally { + restore(); + } });As per coding guidelines:
Use setup function instead of beforeEach in test files... avoid shared state.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/deploy-web/src/components/onboarding/VerifyEmailPage/VerifyEmailPage.spec.tsx` around lines 10 - 29, The tests call restore() after assertions which can leak global state if an assertion throws; update each test that uses setup() (the tests named "shows redirect loading text", "sets onboarding step to EMAIL_VERIFICATION in localStorage", and "redirects to onboarding page") to ensure restore() is always invoked by moving the restore() call into a finally block (call restore() in finally after the expect assertions) so window.localStorage and window.location are restored even on failures.
🤖 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/deploy-web/src/components/onboarding/steps/EmailVerificationContainer/EmailVerificationContainer.tsx`:
- Around line 48-50: The component-level boolean hasSentInitialCode should be
tracked per user ID so a new user doesn’t inherit the previous user's state; in
EmailVerificationContainer replace the single hasSentInitialCode ref with a
keyed store (e.g., Map or object) keyed by the current user id (use the user
context id) and consult/set that entry before auto-sending the initial code, and
apply the same per-user approach for any related refs used at lines ~103-108
(reset or use per-user keys for isSendingRef/cooldownRef as needed) so state is
isolated to each user session.
- Around line 81-84: In EmailVerificationContainer where elapsed and remaining
are computed (using codeSentAt, COOLDOWN_DURATION, cooldownRef.current,
setCooldownSeconds), guard against invalid/skewed timestamps by first parsing
codeSentAt with Date.parse()/new Date(...) and checking for NaN/invalid date;
clamp elapsed to a minimum of 0 (e.g., elapsed = Math.max(0, parsed ?
Math.floor((Date.now() - parsed)/1000) : 0)) and then compute remaining =
Math.min(COOLDOWN_DURATION, Math.max(0, COOLDOWN_DURATION - elapsed)) before
assigning cooldownRef.current and calling setCooldownSeconds so remaining is
always between 0 and COOLDOWN_DURATION.
In
`@apps/deploy-web/src/components/onboarding/steps/EmailVerificationStep/EmailVerificationStep.tsx`:
- Around line 126-139: The six single-character Input fields lack accessible
labels and an OTP autofill hint; update the Input elements in
EmailVerificationStep so each has an accessible name (e.g., add
aria-label={`Digit ${index + 1}`} or associate a visually-hidden label) and
include an autocomplete/autofill hint for OTP (e.g.,
autocomplete="one-time-code" or inputmode + autoComplete where supported).
Ensure these changes are applied alongside existing
refs/inputRefs.current[index], handlers (handleDigitChange, handleKeyDown) and
do not alter disabled behavior when isVerifying.
---
Duplicate comments:
In
`@apps/deploy-web/src/components/onboarding/VerifyEmailPage/VerifyEmailPage.spec.tsx`:
- Around line 10-29: The tests call restore() after assertions which can leak
global state if an assertion throws; update each test that uses setup() (the
tests named "shows redirect loading text", "sets onboarding step to
EMAIL_VERIFICATION in localStorage", and "redirects to onboarding page") to
ensure restore() is always invoked by moving the restore() call into a finally
block (call restore() in finally after the expect assertions) so
window.localStorage and window.location are restored even on failures.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
apps/api/test/functional/__snapshots__/docs.spec.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (7)
apps/api/src/auth/services/email-verification-code/email-verification-code.service.spec.tsapps/deploy-web/src/components/onboarding/VerifyEmailPage/VerifyEmailPage.spec.tsxapps/deploy-web/src/components/onboarding/steps/EmailVerificationContainer/EmailVerificationContainer.spec.tsxapps/deploy-web/src/components/onboarding/steps/EmailVerificationContainer/EmailVerificationContainer.tsxapps/deploy-web/src/components/onboarding/steps/EmailVerificationStep/EmailVerificationStep.tsxapps/deploy-web/src/hooks/useEmailVerificationRequiredEventHandler.tsxpackages/http-sdk/src/auth/auth-http.service.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/deploy-web/src/hooks/useEmailVerificationRequiredEventHandler.tsx
- apps/api/src/auth/services/email-verification-code/email-verification-code.service.spec.ts
- packages/http-sdk/src/auth/auth-http.service.ts
...eb/src/components/onboarding/steps/EmailVerificationContainer/EmailVerificationContainer.tsx
Outdated
Show resolved
Hide resolved
...eb/src/components/onboarding/steps/EmailVerificationContainer/EmailVerificationContainer.tsx
Outdated
Show resolved
Hide resolved
apps/deploy-web/src/components/onboarding/steps/EmailVerificationStep/EmailVerificationStep.tsx
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
apps/api/src/notifications/services/notification-templates/email-verification-code-notification.spec.ts (1)
4-45: Prefer a bottom-of-suitesetuphelper to reduce duplication.The arrangements on Lines 5-8, 18-21, 29-32, and 38-41 are repetitive. A shared
setupkeeps this spec consistent with the repo’s test style and easier to extend.♻️ Suggested refactor
describe(emailVerificationCodeNotification.name, () => { it("returns notification with correct structure", () => { - const user = { id: "user-123", email: "test@example.com" }; - const vars = { code: "123456" }; - - const result = emailVerificationCodeNotification(user, vars); + const { result } = setup(); expect(result.notificationId).toMatch(/^emailVerificationCode\.user-123\.\d+$/); expect(result.payload.summary).toBe("Your verification code"); expect(result.payload.description).toContain("<strong>123456</strong>"); expect(result.payload.description).toContain("expires in 10 minutes"); expect(result.user).toEqual({ id: "user-123", email: "test@example.com" }); }); it("escapes HTML characters in the code", () => { - const user = { id: "user-456", email: "test@example.com" }; - const vars = { code: '<script>alert("xss")</script>' }; - - const result = emailVerificationCodeNotification(user, vars); + const { result } = setup({ user: { id: "user-456", email: "test@example.com" }, code: '<script>alert("xss")</script>' }); expect(result.payload.description).not.toContain("<script>"); expect(result.payload.description).toContain("<script>"); expect(result.payload.description).toContain(""xss""); }); it("escapes ampersands in the code", () => { - const user = { id: "user-789", email: "test@example.com" }; - const vars = { code: "a&b" }; - - const result = emailVerificationCodeNotification(user, vars); + const { result } = setup({ user: { id: "user-789", email: "test@example.com" }, code: "a&b" }); expect(result.payload.description).toContain("a&b"); }); it("escapes single quotes in the code", () => { - const user = { id: "user-abc", email: "test@example.com" }; - const vars = { code: "it's" }; - - const result = emailVerificationCodeNotification(user, vars); + const { result } = setup({ user: { id: "user-abc", email: "test@example.com" }, code: "it's" }); expect(result.payload.description).toContain("it's"); }); + + const setup = ( + { + user = { id: "user-123", email: "test@example.com" }, + code = "123456" + }: { user?: { id: string; email: string }; code?: string } = {} + ) => { + const vars = { code }; + const result = emailVerificationCodeNotification(user, vars); + return { user, vars, result }; + }; });As per coding guidelines, "Use
setupfunction instead ofbeforeEachin test files. Thesetupfunction must be at the bottom of the rootdescribeblock, should create an object under test and return it, accept a single parameter with inline type definition, avoid shared state, and not have a specified return type."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/notifications/services/notification-templates/email-verification-code-notification.spec.ts` around lines 4 - 45, The tests duplicate user/vars/result setup across four cases; replace those repetitions by adding a bottom-of-suite setup function inside the root describe that builds the inputs and returns the object under test (call emailVerificationCodeNotification) — implement setup as a function placed at the end of the describe, accepting a single parameter with an inline type (e.g. { user: { id: string; email: string }, vars: { code: string } }) and returning the created result (do not annotate a return type), then update each it block to call setup({ user: ..., vars: ... }) and run assertions against the returned result; ensure the setup creates no shared state and references emailVerificationCodeNotification to produce the notification.
🤖 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/deploy-web/src/components/onboarding/steps/EmailVerificationStep/EmailVerificationStep.spec.tsx`:
- Line 102: The test currently uses inputs.forEach(input =>
expect(input).toBeDisabled()), which returns a value inside the forEach callback
and triggers the Biome lint rule; change this to an explicit iteration (e.g., a
for...of loop) over inputs and call expect(input).toBeDisabled() inside the loop
so the callback does not return a value—update the test in
EmailVerificationStep.spec.tsx where the inputs variable is asserted to be
disabled.
- Around line 126-150: Update the setup helper so it creates the mock handlers
internally and returns the rendered component plus those mocks: inside the setup
function (which instantiates EmailVerificationStep) create vi.fn() mocks for
onResendCode, onVerifyCode and onContinue, pass them into the
EmailVerificationStep props (along with the other inputs) and return an object
containing the render utilities and the three mock functions (and any other
useful test helpers like cooldownSeconds/isVerifying flags if desired); ensure
setup remains the bottom helper in the describe block so each test gets isolated
mock handles instead of relying on external pre-created mocks.
- Line 40: Replace all uses of screen.getByRole and screen.getAllByRole in the
EmailVerificationStep tests with screen.queryByRole and screen.queryAllByRole
(e.g., the declaration of continueButton and every other assertion that
currently calls screen.getAllByRole); update any assertions that rely on thrown
errors to instead assert null/length via the query results (e.g.,
expect(screen.queryByRole(...)).toBeNull() or
expect(screen.queryAllByRole(...)).toHaveLength(n)), keeping the same test
intent and variable names like continueButton.
---
Nitpick comments:
In
`@apps/api/src/notifications/services/notification-templates/email-verification-code-notification.spec.ts`:
- Around line 4-45: The tests duplicate user/vars/result setup across four
cases; replace those repetitions by adding a bottom-of-suite setup function
inside the root describe that builds the inputs and returns the object under
test (call emailVerificationCodeNotification) — implement setup as a function
placed at the end of the describe, accepting a single parameter with an inline
type (e.g. { user: { id: string; email: string }, vars: { code: string } }) and
returning the created result (do not annotate a return type), then update each
it block to call setup({ user: ..., vars: ... }) and run assertions against the
returned result; ensure the setup creates no shared state and references
emailVerificationCodeNotification to produce the notification.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/api/src/notifications/services/notification-templates/email-verification-code-notification.spec.tsapps/deploy-web/src/components/onboarding/steps/EmailVerificationStep/EmailVerificationStep.spec.tsx
...loy-web/src/components/onboarding/steps/EmailVerificationStep/EmailVerificationStep.spec.tsx
Outdated
Show resolved
Hide resolved
...loy-web/src/components/onboarding/steps/EmailVerificationStep/EmailVerificationStep.spec.tsx
Outdated
Show resolved
Hide resolved
...loy-web/src/components/onboarding/steps/EmailVerificationStep/EmailVerificationStep.spec.tsx
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/deploy-web/src/components/onboarding/steps/EmailVerificationContainer/EmailVerificationContainer.tsx (1)
49-50:⚠️ Potential issue | 🟡 MinorReset cooldown/sending refs when
user.idchanges.
cooldownRefandisSendingRefare component-global guards. If a different user is loaded while mounted, Line 72 can block that user with the previous user’s cooldown/sending state.🔧 Suggested fix
useEffect(() => { + // Isolate transient send/cooldown state per user session + cooldownRef.current = 0; + isSendingRef.current = false; + setCooldownSeconds(0); + setVerifyError(null); + if (!isEmailVerified && user?.id && initialCodeSentForUserRef.current !== user.id) { initialCodeSentForUserRef.current = user.id; sendCode({ silent: true }); } if (!user?.id) { initialCodeSentForUserRef.current = null; } }, [isEmailVerified, user?.id, sendCode]);Also applies to: 72-73, 104-112
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/deploy-web/src/components/onboarding/steps/EmailVerificationContainer/EmailVerificationContainer.tsx` around lines 49 - 50, The cooldownRef and isSendingRef are shared across mounts and must be reset whenever the loaded user changes; add a useEffect in EmailVerificationContainer that watches user?.id and sets isSendingRef.current = false and cooldownRef.current = 0 (and clears any related timers or intervals used by the cooldown logic) so prior user's sending/cooldown state doesn't block the new user; update any places that rely on those refs (send verification handler and cooldown timer logic) to assume they may be reset when user.id changes.
🤖 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/deploy-web/src/components/onboarding/steps/EmailVerificationStep/EmailVerificationStep.tsx`:
- Around line 35-41: The on-change handler that calls setDigits (the function
updating the OTP digit array) currently uses value.slice(-1) which discards
autofilled multi-digit values; change the handler used in EmailVerificationStep
(the digit input onChange and the similar handler around the other occurrence)
to, when value.length > 1, split the string into characters and write them into
the digits array starting at the current index (filling subsequent indices),
preserving existing digits beyond the filled range, and then update state once
with the new array; also ensure isVerifying short-circuits appropriately and
trigger the existing verification flow when all digits become non-empty.
---
Duplicate comments:
In
`@apps/deploy-web/src/components/onboarding/steps/EmailVerificationContainer/EmailVerificationContainer.tsx`:
- Around line 49-50: The cooldownRef and isSendingRef are shared across mounts
and must be reset whenever the loaded user changes; add a useEffect in
EmailVerificationContainer that watches user?.id and sets isSendingRef.current =
false and cooldownRef.current = 0 (and clears any related timers or intervals
used by the cooldown logic) so prior user's sending/cooldown state doesn't block
the new user; update any places that rely on those refs (send verification
handler and cooldown timer logic) to assume they may be reset when user.id
changes.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/deploy-web/src/components/onboarding/steps/EmailVerificationContainer/EmailVerificationContainer.tsxapps/deploy-web/src/components/onboarding/steps/EmailVerificationStep/EmailVerificationStep.spec.tsxapps/deploy-web/src/components/onboarding/steps/EmailVerificationStep/EmailVerificationStep.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/deploy-web/src/components/onboarding/steps/EmailVerificationStep/EmailVerificationStep.spec.tsx
apps/deploy-web/src/components/onboarding/steps/EmailVerificationStep/EmailVerificationStep.tsx
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
apps/deploy-web/src/components/onboarding/steps/EmailVerificationStep/EmailVerificationStep.tsx (1)
34-73: Side effects insidesetDigitsupdater function.
onVerifyCode(code)andinputRefs.current[…]?.focus()are side effects called from within thesetDigitsfunctional updater (lines 47-49, 65-66). React state updaters should be pure — in React 18 StrictMode, they can be invoked twice per update during development, which would double-fire the verify call and the focus change.A safer pattern is to move the side effects out of the updater. For example, compute the new digits, call
setDigitswith the new value, and then perform the side effects after:♻️ Suggested refactor for single-digit path
- setDigits(prev => { - const newDigits = [...prev]; - newDigits[index] = value; - - if (value && index < 5) { - inputRefs.current[index + 1]?.focus(); - } - - const code = newDigits.join(""); - if (code.length === 6) { - onVerifyCode(code); - } - - return newDigits; - }); + setDigits(prev => { + const newDigits = [...prev]; + newDigits[index] = value; + return newDigits; + }); + + if (value && index < 5) { + inputRefs.current[index + 1]?.focus(); + } + + // Check using ref to get latest combined state + // (single-char change is predictable, so we can also reconstruct here) + const updated = [...digitsRef.current]; + updated[index] = value; + const code = updated.join(""); + if (code.length === 6) { + onVerifyCode(code); + }The same applies to the multi-digit branch (lines 40-52). Consider applying a similar extraction there.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/deploy-web/src/components/onboarding/steps/EmailVerificationStep/EmailVerificationStep.tsx` around lines 34 - 73, The setDigits updater in handleDigitChange currently performs side effects (calling onVerifyCode and focusing inputRefs.current[…]) inside the functional updater; extract all side effects out of the setDigits callback: compute the newDigits array and the resulting code synchronously inside handleDigitChange (for both the multi-character branch that uses filled and the single-character branch), then call setDigits(newDigits) and afterwards run inputRefs.current[index + offset]?.focus() and, if code length === 6, call onVerifyCode(code); keep the early guard with isVerifying and preserve the regex check, and reference handleDigitChange, setDigits, inputRefs, onVerifyCode and isVerifying when making the changes.apps/api/src/auth/controllers/auth/auth.controller.spec.ts (1)
42-64: Use a child DI container insetupto improve test isolation and follow established patterns.
rootContainer.register(...)mutates shared global state, and there is no cleanup between tests. Other test files in the codebase (e.g.,auth.interceptor.spec.ts,wallet-initializer.service.spec.ts) already usecontainer.createChildContainer()to isolate test state. Apply the same pattern here for consistency.♻️ Proposed refactor
- rootContainer.register(AuthService, { + const testContainer = rootContainer.createChildContainer(); + testContainer.register(AuthService, { useValue: mock<AuthService>({ isAuthenticated: true, currentUser: user }) }); @@ - const controller = new AuthController(rootContainer.resolve(AuthService), auth0Service, userService, emailVerificationCodeService); + const controller = new AuthController( + testContainer.resolve(AuthService), + auth0Service, + userService, + emailVerificationCodeService + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/auth/controllers/auth/auth.controller.spec.ts` around lines 42 - 64, The test setup currently mutates rootContainer via rootContainer.register; change setup to create an isolated child container (use container.createChildContainer()), register the AuthService mock on that child (replace rootContainer.register(AuthService, ...) with child.register(...)), resolve AuthService from the child and construct AuthController using child.resolve(AuthService) along with the mocked auth0Service, userService, and emailVerificationCodeService, and return the child (or nothing extra) plus controller and mocks so tests no longer mutate global rootContainer and follow the established pattern used by auth.interceptor.spec.ts and wallet-initializer.service.spec.ts.
🤖 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/deploy-web/src/components/onboarding/steps/EmailVerificationStep/EmailVerificationStep.tsx`:
- Around line 75-80: The effect watching verifyError (useEffect) won't run when
the same error string is set twice, so update the flow to include a changing
dependency: either have the parent reset verifyError to null between attempts,
or add and pass an error attempt counter/timestamp prop (e.g., verifyErrorCount
or verifyErrorTimestamp) and include that in the useEffect dependency array
alongside verifyError; then in the effect call setDigits(["",...]) and focus
inputRefs.current[0] when verifyError is truthy and the counter/timestamp
changes so repeated identical errors still clear the inputs.
---
Nitpick comments:
In `@apps/api/src/auth/controllers/auth/auth.controller.spec.ts`:
- Around line 42-64: The test setup currently mutates rootContainer via
rootContainer.register; change setup to create an isolated child container (use
container.createChildContainer()), register the AuthService mock on that child
(replace rootContainer.register(AuthService, ...) with child.register(...)),
resolve AuthService from the child and construct AuthController using
child.resolve(AuthService) along with the mocked auth0Service, userService, and
emailVerificationCodeService, and return the child (or nothing extra) plus
controller and mocks so tests no longer mutate global rootContainer and follow
the established pattern used by auth.interceptor.spec.ts and
wallet-initializer.service.spec.ts.
In
`@apps/deploy-web/src/components/onboarding/steps/EmailVerificationStep/EmailVerificationStep.tsx`:
- Around line 34-73: The setDigits updater in handleDigitChange currently
performs side effects (calling onVerifyCode and focusing inputRefs.current[…])
inside the functional updater; extract all side effects out of the setDigits
callback: compute the newDigits array and the resulting code synchronously
inside handleDigitChange (for both the multi-character branch that uses filled
and the single-character branch), then call setDigits(newDigits) and afterwards
run inputRefs.current[index + offset]?.focus() and, if code length === 6, call
onVerifyCode(code); keep the early guard with isVerifying and preserve the regex
check, and reference handleDigitChange, setDigits, inputRefs, onVerifyCode and
isVerifying when making the changes.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/api/src/auth/controllers/auth/auth.controller.spec.tsapps/api/src/auth/services/auth0/auth0.service.spec.tsapps/deploy-web/src/components/onboarding/steps/EmailVerificationStep/EmailVerificationStep.spec.tsxapps/deploy-web/src/components/onboarding/steps/EmailVerificationStep/EmailVerificationStep.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/deploy-web/src/components/onboarding/steps/EmailVerificationStep/EmailVerificationStep.spec.tsx
apps/deploy-web/src/components/onboarding/steps/EmailVerificationStep/EmailVerificationStep.tsx
Outdated
Show resolved
Hide resolved
stalniy
reviewed
Feb 26, 2026
ygrishajev
reviewed
Mar 2, 2026
Contributor
ygrishajev
left a comment
ygrishajev
left a comment
There was a problem hiding this comment.
Nice one! The code-based verification is well thought out — hashed codes, timing-safe comparison, attempt limits, expiry. Good stuff. Also like that each route gets its own file, much cleaner than bundling them all together. Left some comments I'd like to align on before merging, mostly around simplifying the send/resend flow and the locking approach.
apps/api/src/auth/services/email-verification-code/email-verification-code.service.ts
Outdated
Show resolved
Hide resolved
...eb/src/components/onboarding/steps/EmailVerificationContainer/EmailVerificationContainer.tsx
Show resolved
Hide resolved
stalniy
reviewed
Mar 3, 2026
| expiresAt | ||
| }); | ||
|
|
||
| await this.notificationService.createNotification(emailVerificationCodeNotification({ id: userInternalId, email: user.email }, { code })); |
Contributor
There was a problem hiding this comment.
issue(blocking): this is external service call inside transaction with exclusive lock. If notification service is down, it may cause all db questions to be quickly used and app being stopped responding. Underneath createNotification do retries. So, it can take up to few seconds to finish this operation in case of network issues
Contributor
There was a problem hiding this comment.
instead use pg-boss to schedule this asynchronously
Contributor
Author
There was a problem hiding this comment.
Fixed — split verifyCode into two methods. The @WithTransaction() now only wraps the DB work (fetch user, fetch code FOR UPDATE, validate, increment attempts, update local emailVerified). The Auth0 markEmailVerified call happens after the transaction commits.
If Auth0 fails after the DB commits, local DB still has emailVerified: true (correct state), and the syncEmailVerified endpoint can reconcile Auth0 later.
stalniy
reviewed
Mar 3, 2026
| assert(user, 404, "User not found"); | ||
| assert(user.email, 400, "User has no email address"); | ||
|
|
||
| await this.emailVerificationCodeRepository.acquireUserLock(userInternalId); |
Contributor
There was a problem hiding this comment.
thought(non-blocking): it looks like in reality you want one user to have exactly 1 row in db for email verification code. And you achieve this by using find/create/delete + lock. I believe similar thing could be achieved with unique index on userId and relying on unique constraint
Contributor
There was a problem hiding this comment.
@CodeRabbit what does happen if application is forcefully restarted by k8s after acquiring a lock here?
Contributor
There was a problem hiding this comment.
@stalniy If the application is forcefully restarted by Kubernetes after acquiring the lock, the lock will be automatically released by PostgreSQL. Here's why:
The code uses pg_advisory_xact_lock (a transaction-scoped advisory lock). These locks have important safety properties:
-
Automatic release on connection loss: When the pod is terminated, the database connection is dropped, and PostgreSQL immediately releases all advisory locks held by that connection.
-
Transaction rollback: Since the method is decorated with
@WithTransaction(), any uncommitted changes will be rolled back when the connection is lost, maintaining data consistency. -
No persistent deadlocks: Unlike some locking mechanisms, PostgreSQL advisory locks cannot create permanent deadlocks from crashed clients—they're tied to the connection/transaction lifetime.
In practice, this means:
- If a pod crashes after acquiring the lock but before completing the operation, other pods can immediately acquire the lock once PostgreSQL detects the connection is dead (typically within seconds).
- No manual cleanup or lock release is needed.
- The worst case is that one code-send operation fails (no code is created), and the user can retry.
This is actually a robust design choice for distributed systems running in Kubernetes, where pods can be terminated at any time.
Contributor
There was a problem hiding this comment.
but we had deadlocks when similar approach was used for serializing concurrent operations in past
Contributor
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
Contributor
There was a problem hiding this comment.
Good point about transactions depending on third-party services. The current verifyCode holds a @WithTransaction() + SELECT FOR UPDATE while calling auth0Service.markEmailVerified, which means a slow/hanging Auth0 response keeps the DB connection and row lock open. Not the same conditions as the deadlocks we hit before (nested transaction + advisory lock), but same category of risk — holding DB resources while waiting on external services.
I think the real fix is simpler though: we don't need the transaction or the lock at all. The FOR UPDATE is there to serialize concurrent attempts so two requests can't both read attempts = 4 and bypass the MAX_ATTEMPTS check. But do we actually care about that? The code is 6 digits (1M possibilities), expires in 10 minutes, and sendCode is already rate-limited. An attacker gets maybe a few hundred requests in that window — 0.03% chance. If we ever need tighter brute-force protection, CF rate limiting at the edge is the right layer.
Drop the attempts logic entirely and verifyCode becomes just reads + two independent writes — no transaction, no lock, no incrementAttempts, no FOR UPDATE. No risk of holding DB resources while waiting on external services.
stalniy
reviewed
Mar 3, 2026
| } catch (error) { | ||
| if (error instanceof ResponseError) { | ||
| const body = JSON.parse(error.body); | ||
| assert(false, error.statusCode, body.message); |
Contributor
There was a problem hiding this comment.
issue(non-blocking): error response forwarding to the user as is usually is not good. Better log it and return some general message. For example, if such user exists, this code will expose this to the end client
Contributor
There was a problem hiding this comment.
By the way, why do we need such logic in console-api? We have similar one in console-web
Contributor
Author
There was a problem hiding this comment.
Fixed — 409 errors now return a generic 422 with "Unable to create account. Please try again or use a different email." to avoid leaking user existence. Other ResponseError codes (like 400 for password strength) are still forwarded since they're useful input validation feedback.
Contributor
Author
There was a problem hiding this comment.
The API handles signup because it needs to call Auth0's Management API, which requires a client secret. Keeping that credential server-side avoids exposing it in the frontend. It also centralises validation (password strength, duplicate-email handling) in one place so any future clients (CLI, mobile) get the same behavior without duplicating logic.
Contributor
Author
There was a problem hiding this comment.
And mostly this was to call verify_email: false to not send duplicate emails at account creation
Contributor
There was a problem hiding this comment.
My concern is that we have part of the auth logic in deploy-web (including its server-side), now we also have part of user signup in console-api.
This is not good because we loose track of which app is responsible for what functionality
stalniy
reviewed
Mar 3, 2026
Comment on lines
+74
to
+76
| await this.emailVerificationCodeRepository.acquireUserLock(userInternalId); | ||
|
|
||
| const record = await this.emailVerificationCodeRepository.findActiveByUserId(userInternalId); |
Contributor
There was a problem hiding this comment.
question: Why did you pick the strategy of using locks instead of searching for code with passed in userId and hashCode(code)?
Contributor
Author
There was a problem hiding this comment.
FOR UPDATE serialises concurrent verify attempts so the `attempts` counter is always read-then-incremented atomically. Without it, two concurrent wrong guesses could both read `attempts: 4`, both pass the `< 5` check, and both increment — letting an attacker get 6+ guesses on a 6-digit code.
A direct hash lookup (`WHERE userId = ? AND code = hash(?)`) would skip codes that don't match, so we'd lose the ability to increment attempts on a wrong guess. We need to first fetch the row, then check the code, then decide whether to increment or accept.
| return { emailVerified: false }; | ||
| } | ||
|
|
||
| await this.auth0Service.markEmailVerified(user.userId); |
Contributor
There was a problem hiding this comment.
issue(blocking): external service call in transaction
| assert(user, 404, "User not found"); | ||
| assert(user.email, 400, "User has no email address"); | ||
|
|
||
| await this.emailVerificationCodeRepository.acquireUserLock(userInternalId); |
Contributor
There was a problem hiding this comment.
but we had deadlocks when similar approach was used for serializing concurrent operations in past
|
|
||
| await this.auth0Service.markEmailVerified(user.userId); | ||
| await this.userRepository.updateById(userInternalId, { emailVerified: true }); | ||
| await this.emailVerificationCodeRepository.deleteByUserId(userInternalId); |
Contributor
There was a problem hiding this comment.
suggestion: instead of deleting everything you could remove only this code.
Contributor
There was a problem hiding this comment.
question: does it make sense to create a separate set of unit tests when we have integration level tests for user service in user.service.integration.ts?
| return () => clearTimeout(timer); | ||
| }, [cooldownSeconds]); | ||
|
|
||
| const sendCode = useCallback(async () => { |
Contributor
There was a problem hiding this comment.
question: is there any reason why you don't want to use useMutation from react-query here?
apps/deploy-web/src/components/onboarding/steps/EmailVerificationStep/EmailVerificationStep.tsx
Outdated
Show resolved
Hide resolved
apps/deploy-web/src/components/onboarding/VerifyEmailPage/VerifyEmailPage.tsx
Outdated
Show resolved
Hide resolved
Implements server-side email verification via 6-digit codes with: - Advisory lock to prevent race conditions on concurrent send requests - 60s cooldown enforced both client-side (cooldownRef guard) and server-side - Timing-safe code comparison with max 5 attempts - Bold code rendering in email by allowing <strong> in sanitizer
- Remove client-supplied userId from routes; use auth context instead - Add advisory lock to verifyCode to prevent TOCTOU on attempts - Guard timingSafeEqual against length mismatch (RangeError) - Fix randomInt upper bound (exclusive) to include 999999 - Enforce digits-only regex on code schema - Escape HTML in email template for defense in depth - Clear digit inputs on failed verification for better UX - Use DB record createdAt for codeSentAt instead of new Date() - Add length-mismatch test for verifyCode
- Update docs snapshot for new verification routes - Fix backend spec setup() to accept inline-typed parameter - Use .name for describe, remove "should" from test descriptions - Replace any types with concrete types in test specs - Add restore/cleanup for globals in VerifyEmailPage spec - Add auth/analyticsService to useCallback deps - Stabilize EmailVerificationStep callbacks with functional updaters - Optimize cooldown interval to avoid re-creation each tick - Mark legacy sendVerificationEmail as @deprecated
…Step Improve codecov patch coverage: - Add unit tests for emailVerificationCodeNotification (HTML escaping) - Add unit tests for EmailVerificationStep component (rendering, interactions)
- Track initial auto-send per user ID instead of global boolean ref - Clamp cooldown math against clock skew and invalid timestamps - Add aria-label and autoComplete="one-time-code" for OTP accessibility - Use queryByRole/queryAllByRole in step spec per project convention - Replace forEach with for...of loop (Biome lint compliance) - Return mock handles from setup() function
- Handle browser OTP autofill that injects multi-digit values into single input - Remove maxLength=1 to allow autofill to deliver full code - Add auth0.service tests for sendVerificationEmail and markEmailVerified - Add auth.controller tests for sendVerificationCode and verifyEmailCode - Add OTP autofill test case for EmailVerificationStep
- Auto-advance to next onboarding step after successful verification - Remove verified state UI (alert + continue button) — toast provides feedback - Fix cooldown timer by using setTimeout chain instead of setInterval - Remove isEmailVerified and onContinue from EmailVerificationStep props
… step Move onVerifyCode calls from inside setDigits updaters into a useEffect watching the digits state. This eliminates the React warning "Cannot update a component while rendering a different component".
The cooldown was computed from the server's codeSentAt timestamp, which could return the original send time rather than the resend time. This resulted in near-zero cooldown, allowing the button to be clicked again immediately. Now user-initiated resends always apply a full 60-second cooldown. Server timing is only used for the silent auto-send on mount to resume an existing cooldown.
Always apply a full 60-second cooldown after any successful send, including the silent auto-send on mount. Removes dependency on the server's codeSentAt timestamp for cooldown calculation.
Server-side sendCode now accepts a resend flag. Without it (page load), if any active code exists, the server returns early without sending a new email. With resend=true (user clicks Resend), the 60-second cooldown applies but a new code is sent when the cooldown expires. This prevents duplicate emails when the user refreshes the page while a valid code still exists (within the 10-minute expiry window).
Show verification errors via toast instead of inline Alert component. Stop clearing digit inputs on error so users can correct and retry.
…M nesting Show verifying/sending spinner inside the resend button to eliminate layout shift. Center digit input text. Fix div-inside-p in Snackbar.
…erification UX Send verification code during user registration in UserService instead of relying on frontend useEffect. Also prevent double verify-email-code calls with submittedCodeRef, auto-advance when email is already verified, and clear inputs on resend via resetKey pattern.
…registration Cover the new sendCode call in registerUser: success path, skip when already verified, and graceful error handling. Also fix integration test constructor after EmailVerificationCodeService injection.
Make EmailVerificationCodeService mock return a resolved Promise so .catch() doesn't throw. Update API docs snapshot with requestBody for send-verification-code route.
Return { emailVerified: false } instead of throwing inside
@WithTransaction, so incrementAttempts persists. Controller
asserts after the transaction commits. Also merge nested sendCode
conditions for readability.
Hash verification codes with SHA-256 before storing, fix double advance() call on successful verification, add UNIQUE(user_id) constraint, use two-int advisory lock to avoid hash collisions, replace Date.now() with randomUUID() in notification IDs, narrow response schema to z.literal(true), and add missing controller test.
…ove container/view separation - Remove advisory locks, resend flag, and deletes in favor of SELECT FOR UPDATE and rate limiting - Return 204 No Content for signup and verify-email-code endpoints - Move cooldown, notifications, and debounce from container to view component - Service throws on invalid code instead of returning boolean - Merge unique constraint removal into original migration
…9 error Split verifyCode to keep only DB work inside @WithTransaction, moving the Auth0 markEmailVerified call outside to avoid holding a transaction open on an external HTTP call. Sanitize 409 (user exists) responses to return a generic 422 message preventing user enumeration.
Separate digit input logic (paste, autofill, focus management) into a reusable VerificationCodeInput component. Also inject NextSeo through DEPENDENCIES in VerifyEmailPage.
baktun14
force-pushed
the
feat/auth0-email-code-verification
branch
from
8d7ab7a to
07be485
Compare
This was referenced Apr 9, 2026
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
3 participants
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.
Why
Ref CON-197
Replace Auth0's default email verification link with a 6-digit code flow for better UX. Users enter a code directly in the onboarding UI instead of clicking a link in a separate tab.
What
Backend
EmailVerificationCodestable with migrationEmailVerificationCodeServicewithsendCodeandverifyCodemethodspg_advisory_xact_lockto prevent race conditionsverifyCodeagainst TOCTOU on attempt countingFrontend
EmailVerificationContainermanages cooldown state via ref (prevents API spam without causing re-renders)EmailVerificationStepwith 6-digit input, paste support, auto-submit, and digit-clear on errorsendVerificationCode()andverifyEmailCode(code)no longer accept userId<strong>tag in email sanitizer for bold code displayTests
Summary by CodeRabbit
New Features
Notifications
Tests