feat: configurable OTP length with 6-digit default support - #14
Merged
Conversation
|
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:
📝 WalkthroughWalkthroughThis PR introduces configurable OTP settings, replacing hardcoded 8-digit numeric OTPs with support for length (4–12, default 8) and charset (numeric or alphanumeric, default numeric) via environment variables. Configuration is threaded through initialization, routing layers, and email formatting. New utility functions format OTPs for display in plain text and HTML emails. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
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 docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
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: 8
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/auth-service/src/__tests__/recovery.test.ts`:
- Around line 153-159: Rename the failing test title to match the assertion:
change the it(...) description currently "numeric-only OTP does not match
alphanumeric-exclusive pattern" to something like "numeric-only OTP matches
alphanumeric pattern" (or "numeric-only OTP matches [A-Za-z0-9] pattern") so the
test name accurately reflects that pattern.test(otp) is expected toBe(true);
update the string in the it(...) call surrounding the test that creates pattern
and otp.
In `@packages/auth-service/src/better-auth.ts`:
- Around line 80-82: The function in better-auth.ts exposes otpLength and
otpCharset parameters which allow non-8-digit or alphanumeric OTPs; update the
implementation to enforce the auth-service policy by hardcoding or overriding
these values to otpLength = 8 and otpCharset = 'numeric' (ignore or throw on
incoming variants) wherever OTPs are generated/validated (references: otpLength,
otpCharset and the OTP generation/validation functions in better-auth.ts),
ensure the generator only produces 8 numeric digits and single-use semantics
remain, and apply the same enforcement to the other occurrences noted in this
file (the other OTP-related helper functions referenced in the review).
In `@packages/auth-service/src/index.ts`:
- Around line 138-141: The OTP configuration widens policy beyond the service
contract by allowing variable lengths and alphanumeric chars; change the otp
config so otpLength is fixed to 8 and otpCharset is fixed to 'numeric' (do not
read these from process.env), and ensure any other occurrences in this file that
set OTP policy (e.g., the otpLength and otpCharset bindings and related settings
in the same config block) are updated to use the fixed 8-digit numeric policy
and single-use handling via better-auth integration (preserve surrounding config
structure and types, only replace the env-driven values with the constant 8 and
'numeric').
In `@packages/auth-service/src/routes/account-login.ts`:
- Line 176: The current article selection uses
/^[aeiou]/i.test(opts.otpLength.toString()), which misidentifies numeric lengths
(e.g., "8" or "11"); update the logic around the article variable (where article
is computed from opts.otpLength) to detect numeric values and choose "an" when
the spoken form begins with a vowel sound (handle common numeric cases such as
numbers starting with "8" and "11" at minimum) otherwise fall back to the
vowel-letter test for non-numeric strings; refer to the article variable and
opts.otpLength/opts.otpLength.toString() when locating and replacing the check.
- Around line 196-197: The OTP input currently forces digits by hardcoding
pattern="[0-9]{...}", inputmode="numeric", and a zero-filled placeholder, which
breaks alphanumeric OTPs; update the template that renders the input
(referencing opts.otpLength and opts.otpCharset and the "otp-input" input
element) to choose attributes based on opts.otpCharset: for numeric use
pattern="[0-9]{N}", inputmode="numeric", and placeholder of
'0'.repeat(opts.otpLength); for alphanumeric use a more permissive pattern (e.g.
[A-Za-z0-9]{N} or no pattern), remove/adjust inputmode (or use "text"), and set
a neutral placeholder (e.g. repeated '•' or spaces) so valid alphanumeric codes
are not blocked client-side.
In `@packages/auth-service/src/routes/login-page.ts`:
- Around line 366-369: The article calculation using /^[aeiou]/ on
otpLength.toString() is wrong for numerals (e.g., 8, 11); change the logic that
sets article (where otpLength is used) to detect numeric cases instead of vowel
letters — e.g., convert otpLength to string and set article =
otpStr.startsWith('8') || otpStr.startsWith('11') ? 'an' : 'a' (use the
otpLength and article symbols from the diff), and replace the three occurrences
noted with this numeric-aware check.
In `@packages/auth-service/src/routes/recovery.ts`:
- Around line 269-272: The OTP input pattern currently uses [A-Za-z0-9] when
opts.otpCharset === 'alphanumeric', which permits lowercase even though
generated alphanumeric OTPs are uppercase-only; update the pattern expression to
[A-Z0-9] and ensure autocapitalize remains "characters" (and inputmode stays
'text') so the HTML validation and UX match the generated OTP alphabet; locate
the pattern string and related attributes in the OTP input markup that reference
opts.otpCharset and opts.otpLength and replace the lowercase-permitting class
with an uppercase-only character class.
- Line 248: The current vowel-regex on opts.otpLength.toString() produces
incorrect articles for numeric lengths (e.g., "a 8-digit"); change the logic to
decide the article from the numeric string's prefix instead (for example check
opts.otpLength.toString().startsWith('8') ||
opts.otpLength.toString().startsWith('11') ? 'an' : 'a') so numbers like 8 and
11 get "an"; update the const article assignment in recovery.ts (the article
computation shown in the diff) and apply the same fix to the similar occurrence
around the other instance at line 261.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9d9ed776-a4bf-40a8-8806-3c9f1e350998
📒 Files selected for processing (12)
.beads/issues.jsonl.env.examplepackages/auth-service/.env.examplepackages/auth-service/src/__tests__/consent.test.tspackages/auth-service/src/__tests__/recovery.test.tspackages/auth-service/src/better-auth.tspackages/auth-service/src/context.tspackages/auth-service/src/index.tspackages/auth-service/src/routes/account-login.tspackages/auth-service/src/routes/login-page.tspackages/auth-service/src/routes/recovery.tspackages/shared/src/crypto.ts
Kzoeps
marked this pull request as draft
March 16, 2026 08:26
Kzoeps
force-pushed
the
karma/hyper-113-switch-to-6-digit-otp
branch
from
March 17, 2026 14:45
1ecc1dc to
757bafe
Compare
Kzoeps
marked this pull request as ready for review
March 17, 2026 14:47
Contributor
Author
|
@coderabbitai please review |
|
🧠 Learnings used✅ Actions performedReview triggered.
|
Contributor
Author
@coderabbitai we are now making it configurable to 6 and 4 digits although i might remove 4 and just only bump tioll 6 but in any case its now configurable |
|
Tip For best results, initiate chat on the files or code changes.
✏️ Learnings added
⛔ Learnings removed🧠 Learnings used |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/auth-service/src/routes/recovery.ts (1)
262-262:⚠️ Potential issue | 🟡 MinorArticle grammar is incorrect for OTP lengths like 8 and 11.
The text "we sent a ${opts.otpLength}-digit code" produces "a 8-digit" instead of "an 8-digit" (and similarly "a 11-character" instead of "an 11-character"). Numbers starting with 8 or 11 require "an" because they begin with vowel sounds.
✏️ Suggested fix
+ const article = [8, 11].includes(opts.otpLength) ? 'an' : 'a' return `<!DOCTYPE html> ... - <p class="subtitle">If a backup email matches, we sent a ${opts.otpLength}-${opts.otpCharset === 'alphanumeric' ? 'character' : 'digit'} code to <strong>${escapeHtml(maskedEmail)}</strong></p> + <p class="subtitle">If a backup email matches, we sent ${article} ${opts.otpLength}-${opts.otpCharset === 'alphanumeric' ? 'character' : 'digit'} code to <strong>${escapeHtml(maskedEmail)}</strong></p>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/auth-service/src/routes/recovery.ts` at line 262, The template currently hardcodes "a" before "${opts.otpLength}-..." which yields wrong grammar for numbers like 8 and 11; modify the recovery email/html generation to compute an article variable (e.g., article = 'an' when String(opts.otpLength) starts with a vowel-sound number such as '8' or '11', otherwise 'a') and use that article in the paragraph instead of the hardcoded "a" (update the template that builds the <p class="subtitle"> containing ${opts.otpLength}-${opts.otpCharset ...} to interpolate the computed article).
🧹 Nitpick comments (3)
packages/auth-service/src/email/sender.ts (2)
324-339: Same inconsistency in welcome email plain-text body.Same observation as the sign-in email — the subject uses
formatOtpPlain(code)but the text body at line 329 uses rawcode.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/auth-service/src/email/sender.ts` around lines 324 - 339, The plain-text body built in the `text` array uses the raw `code` while the subject uses `formatOtpPlain(code)`, causing inconsistency; update the `text` assembly (the array assigned to `text`) to use `formatOtpPlain(code)` wherever the raw `code` appears so the welcome email's subject and plain-text body match (keep other elements like `pdsName` and `pdsDomain` unchanged).
275-286: Consider formatting the OTP in plain-text body for consistency with subject line.The subject line uses
formatOtpPlain(code)(e.g., "1234 5678") but the plain-text body at line 279 uses the rawcode. This creates a minor inconsistency where users see a grouped code in the subject but an ungrouped code in the text body.If the raw code is intentional for easy copy-paste from plain-text email clients, this is fine. Otherwise, consider using
formatOtpPlain(code)here as well.♻️ Optional: format plain-text body code
const text = [ `Your sign-in code for ${clientAppName}:`, '', - code, + formatOtpPlain(code), '',🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/auth-service/src/email/sender.ts` around lines 275 - 286, The plain-text email body builds the `text` variable using the raw `code`, causing an inconsistency with the subject which uses `formatOtpPlain(code)`; update the array that constructs `text` to use `formatOtpPlain(code)` instead of `code` so the OTP appears grouped consistently (keep the rest of the `text` array intact and continue to use `pdsName`/`pdsDomain` as before).packages/auth-service/src/better-auth.ts (1)
121-127: Add comment explaining theanyreturn type.Per coding guidelines,
as anycasts (andanytypes) when working with better-auth internals should include a comment explaining why. The eslint-disable is present but lacks the required explanation.✏️ Suggested comment
// eslint-disable-next-line `@typescript-eslint/no-explicit-any` +// Returns `any` because better-auth's betterAuth() return type is not exported export function createBetterAuth( emailSender: EmailSender, db: EpdsDb, otpLength: number, otpCharset: 'numeric' | 'alphanumeric' = 'numeric', ): any {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/auth-service/src/better-auth.ts` around lines 121 - 127, The exported function createBetterAuth currently returns type any and has an eslint-disable comment but no explanation; update the definition to either use a concrete return type or, if using any/ts-ignore is necessary for internal polymorphism, keep the eslint-disable but add a brief inline comment explaining why the any cast is required (e.g., unstable internal types, complex generics, or external library mismatch) and how/when it should be replaced with a concrete type; reference the createBetterAuth function and the eslint-disable-next-line `@typescript-eslint/no-explicit-any` so reviewers can find and validate the explanatory comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/auth-service/src/otp-input.ts`:
- Around line 19-26: The HTML input pattern in otp-input.ts allows lowercase
(`[A-Za-z0-9]{otpLength}`) while better-auth.ts generates uppercase OTPs via
generateRandomString(otpLength, 'A-Z', '0-9'), causing validation mismatch; fix
by either (A) restricting the pattern to uppercase `[A-Z0-9]{${otpLength}}`, add
UI hint/ styling (e.g., text-transform: uppercase or keep autocapitalize:
'characters') so users enter uppercase only, or (B) normalize submitted OTP to
uppercase in the verification path inside signInEmailOTP (call otp =
otp.toUpperCase() before checking) so lowercase input still verifies — update
the code in otp-input.ts and/or better-auth.ts accordingly.
---
Duplicate comments:
In `@packages/auth-service/src/routes/recovery.ts`:
- Line 262: The template currently hardcodes "a" before "${opts.otpLength}-..."
which yields wrong grammar for numbers like 8 and 11; modify the recovery
email/html generation to compute an article variable (e.g., article = 'an' when
String(opts.otpLength) starts with a vowel-sound number such as '8' or '11',
otherwise 'a') and use that article in the paragraph instead of the hardcoded
"a" (update the template that builds the <p class="subtitle"> containing
${opts.otpLength}-${opts.otpCharset ...} to interpolate the computed article).
---
Nitpick comments:
In `@packages/auth-service/src/better-auth.ts`:
- Around line 121-127: The exported function createBetterAuth currently returns
type any and has an eslint-disable comment but no explanation; update the
definition to either use a concrete return type or, if using any/ts-ignore is
necessary for internal polymorphism, keep the eslint-disable but add a brief
inline comment explaining why the any cast is required (e.g., unstable internal
types, complex generics, or external library mismatch) and how/when it should be
replaced with a concrete type; reference the createBetterAuth function and the
eslint-disable-next-line `@typescript-eslint/no-explicit-any` so reviewers can
find and validate the explanatory comment.
In `@packages/auth-service/src/email/sender.ts`:
- Around line 324-339: The plain-text body built in the `text` array uses the
raw `code` while the subject uses `formatOtpPlain(code)`, causing inconsistency;
update the `text` assembly (the array assigned to `text`) to use
`formatOtpPlain(code)` wherever the raw `code` appears so the welcome email's
subject and plain-text body match (keep other elements like `pdsName` and
`pdsDomain` unchanged).
- Around line 275-286: The plain-text email body builds the `text` variable
using the raw `code`, causing an inconsistency with the subject which uses
`formatOtpPlain(code)`; update the array that constructs `text` to use
`formatOtpPlain(code)` instead of `code` so the OTP appears grouped consistently
(keep the rest of the `text` array intact and continue to use
`pdsName`/`pdsDomain` as before).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8b4b27e4-f064-4e3f-b9cb-edc00a6a1d11
📒 Files selected for processing (18)
.beads/issues.jsonl.env.examplepackages/auth-service/.env.examplepackages/auth-service/src/__tests__/consent.test.tspackages/auth-service/src/__tests__/otp-input.test.tspackages/auth-service/src/__tests__/recovery.test.tspackages/auth-service/src/better-auth.tspackages/auth-service/src/context.tspackages/auth-service/src/email/sender.tspackages/auth-service/src/index.tspackages/auth-service/src/otp-input.tspackages/auth-service/src/routes/account-login.tspackages/auth-service/src/routes/login-page.tspackages/auth-service/src/routes/recovery.tspackages/shared/src/__tests__/html.test.tspackages/shared/src/crypto.tspackages/shared/src/html.tspackages/shared/src/index.ts
💤 Files with no reviewable changes (1)
- packages/auth-service/src/tests/recovery.test.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- .env.example
- packages/shared/src/crypto.ts
- packages/auth-service/src/tests/consent.test.ts
- packages/auth-service/.env.example
- packages/auth-service/src/context.ts
Kzoeps
force-pushed
the
karma/hyper-113-switch-to-6-digit-otp
branch
from
March 18, 2026 04:44
757bafe to
7b7f63a
Compare
Kzoeps
force-pushed
the
karma/hyper-113-switch-to-6-digit-otp
branch
from
March 18, 2026 04:49
7b7f63a to
11a8426
Compare
Update recovery.ts to use configurable otpLength from ctx.config (ePDS-ipm.4) Use configurable otpLength from ctx.config in login-page.ts (ePDS-ipm.3) fix: pass otpLength param to createBetterAuth and runBetterAuthMigrations (ePDS-zgl) Add generateOTP alphanumeric support to better-auth.ts (ePDS-5k2.3)
Kzoeps
force-pushed
the
karma/hyper-113-switch-to-6-digit-otp
branch
from
March 18, 2026 04:54
11a8426 to
70508d5
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/auth-service/src/routes/recovery.ts (1)
147-160:⚠️ Potential issue | 🟡 MinorNormalize separators server-side before verification.
The browser strips spaces and hyphens in
oninput, but that path is easy to bypass with disabled JS, pasted values, or scripted clients. Clean the code on the server before uppercasing it so verification does not depend on client-side behavior.🧹 Proposed fix
- const code = ((req.body.code as string) || '').trim() + const code = ((req.body.code as string) || '') + .trim() + .replace(/[\s-]/g, '')🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/auth-service/src/routes/recovery.ts` around lines 147 - 160, The server currently uses the raw code variable when calling auth.api.signInEmailOTP, relying on client-side JS to strip spaces/hyphens; instead normalize separators server-side by removing spaces and hyphens (e.g., transform the existing code variable to strip ' ' and '-' and then .toUpperCase()) before passing it into auth.api.signInEmailOTP so verification matches client behavior; update the code assignment and the value passed to signInEmailOTP in recovery.ts (the code local and the auth.api.signInEmailOTP call) accordingly.
♻️ Duplicate comments (1)
packages/auth-service/src/routes/recovery.ts (1)
249-262:⚠️ Potential issue | 🟡 MinorRestore the article check in the subtitle.
This now renders “a 8-digit code” / “a 11-digit code” for valid lengths. Please derive the article from the numeric length before composing the sentence.
✏️ Proposed fix
const maskedEmail = maskEmail(opts.email) const encodedUri = encodeURIComponent(opts.requestUri) const inputProps = buildOtpInputProps(opts.otpLength, opts.otpCharset) + const article = [8, 11].includes(opts.otpLength) ? 'an' : 'a' @@ - <p class="subtitle">If a backup email matches, we sent a ${opts.otpLength}-${opts.otpCharset === 'alphanumeric' ? 'character' : 'digit'} code to <strong>${escapeHtml(maskedEmail)}</strong></p> + <p class="subtitle">If a backup email matches, we sent ${article} ${opts.otpLength}-${opts.otpCharset === 'alphanumeric' ? 'character' : 'digit'} code to <strong>${escapeHtml(maskedEmail)}</strong></p>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/auth-service/src/routes/recovery.ts` around lines 249 - 262, The subtitle currently always uses "a" before the numeric OTP length; add a small helper (e.g., getIndefiniteArticleForNumber) and compute article = getIndefiniteArticleForNumber(opts.otpLength) before building the template, then use that article in the sentence around opts.otpLength and opts.otpCharset; implement the helper to return "an" for lengths that read with a vowel sound (common OTP lengths like 8 and 11) and "a" otherwise so the sentence around maskedEmail and escapeHtml(maskedEmail) renders correctly.
🧹 Nitpick comments (4)
packages/auth-service/src/email/sender.ts (3)
233-233: Consider formatting the OTP in the plain-text fallback body for consistency.The plain-text email body uses the raw
codewhile subject lines useformatOtpPlain(code). For longer OTP codes (8+ digits), grouping improves readability.♻️ Suggested change
- text: `Your code for ${appName} is: ${code}\n\nThis code expires in 10 minutes.\n\nIf you didn't request this, you can safely ignore this email.`, + text: `Your code for ${appName} is: ${formatOtpPlain(code)}\n\nThis code expires in 10 minutes.\n\nIf you didn't request this, you can safely ignore this email.`,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/auth-service/src/email/sender.ts` at line 233, The plain-text fallback email body uses the raw code string; update the text template in sender.ts to use the same formatted OTP as the subject by calling formatOtpPlain(code) instead of inserting code directly so long OTPs are grouped consistently (refer to the text template, the code variable and the formatOtpPlain function).
276-279: Optional: Format OTP in plain-text body for consistency with subject line.Same as the client-template path—the plain-text body uses raw
codewhile the subject usesformatOtpPlain(code).♻️ Suggested change
const text = [ `Your sign-in code for ${clientAppName}:`, '', - code, + formatOtpPlain(code), '',🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/auth-service/src/email/sender.ts` around lines 276 - 279, The plain-text email body currently inserts the raw variable code while the subject uses formatOtpPlain(code), causing inconsistency; update the plain-text body construction in the email sender (the block that builds the message array containing `Your sign-in code for ${clientAppName}:`, `''`, `code`, `''`) to use formatOtpPlain(code) instead of raw code so both subject and body display the OTP in the same formatted/plain form.
327-329: Optional: Format OTP in plain-text body for consistency with subject line.Same pattern in the welcome email plain-text body.
♻️ Suggested change
`Your verification code:`, '', - code, + formatOtpPlain(code), '',🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/auth-service/src/email/sender.ts` around lines 327 - 329, Update the plain-text bodies so the OTP appears inline with its label (e.g., "Your verification code: <code>") to match the subject line; locate the plain-text body construction in sender.ts (functions/methods that build the verification and welcome email bodies—e.g., sendVerificationEmail/sendWelcomeEmail or the variables that assemble the arrays using `code`) and replace the separate array elements (`"Your verification code:", "", code`) with a single line that concatenates the label and the code into one plain-text string; apply the same change to the welcome email plain-text body to keep formatting consistent.packages/auth-service/src/__tests__/otp-input.test.ts (1)
14-65: Add edge-length cases for the supported OTP range.These tests only exercise 6 and 8 today, so a regression at the minimum or maximum supported length would still pass. Please pin 4- and 12-character outputs as well.
Based on learnings: "OTP codes are configurable via the OTP_LENGTH environment variable (range 4–12)..."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/auth-service/src/__tests__/otp-input.test.ts` around lines 14 - 65, Add edge-length cases for the OTP range by extending the existing tests that call buildOtpInputProps to also assert behavior for otpLength 4 and 12; for both 'numeric' and 'alphanumeric' charsets verify pattern equals '[0-9]{4}'/'[A-Z0-9]{4}' and '[0-9]{12}'/'[A-Z0-9]{12}', placeholders have lengths 4 and 12 respectively, inputmode/autocapitalize remain correct, and the RegExp tests accept/ reject appropriate strings (correct length and charset, reject too short/too long and lowercase for alphanumeric). Update the tests inside the same describe('Recovery flow: OTP input props') block (reuse the existing it blocks or add new ones) referencing the buildOtpInputProps function and pattern/placeholder fields to pin minimum (4) and maximum (12) cases.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.env.example:
- Around line 111-117: Update the .env.example wording to refer to OTP_LENGTH as
the number of characters (not digits) since OTP_CHARSET can be alphanumeric;
change the comment lines that mention "number of digits" and "Must be between 4
and 12" to say "number of characters" (and keep the constraint and examples the
same) so OTP_LENGTH and OTP_CHARSET descriptions remain accurate together.
In `@packages/auth-service/src/routes/recovery.ts`:
- Around line 262-278: The OTP input lacks an accessible name and descriptive
relationship; update the HTML template in recovery.ts to provide an associated
label and aria-describedby for the input with id="code": add a visually-hidden
<label for="code"> (or include aria-label="One-time code" on the input) and add
aria-describedby="code-help" on the input, and assign id="code-help" to the
instructional paragraph that currently explains the sent code (the <p
class="subtitle">) so screen readers can read the instruction when focusing the
OTP field.
---
Outside diff comments:
In `@packages/auth-service/src/routes/recovery.ts`:
- Around line 147-160: The server currently uses the raw code variable when
calling auth.api.signInEmailOTP, relying on client-side JS to strip
spaces/hyphens; instead normalize separators server-side by removing spaces and
hyphens (e.g., transform the existing code variable to strip ' ' and '-' and
then .toUpperCase()) before passing it into auth.api.signInEmailOTP so
verification matches client behavior; update the code assignment and the value
passed to signInEmailOTP in recovery.ts (the code local and the
auth.api.signInEmailOTP call) accordingly.
---
Duplicate comments:
In `@packages/auth-service/src/routes/recovery.ts`:
- Around line 249-262: The subtitle currently always uses "a" before the numeric
OTP length; add a small helper (e.g., getIndefiniteArticleForNumber) and compute
article = getIndefiniteArticleForNumber(opts.otpLength) before building the
template, then use that article in the sentence around opts.otpLength and
opts.otpCharset; implement the helper to return "an" for lengths that read with
a vowel sound (common OTP lengths like 8 and 11) and "a" otherwise so the
sentence around maskedEmail and escapeHtml(maskedEmail) renders correctly.
---
Nitpick comments:
In `@packages/auth-service/src/__tests__/otp-input.test.ts`:
- Around line 14-65: Add edge-length cases for the OTP range by extending the
existing tests that call buildOtpInputProps to also assert behavior for
otpLength 4 and 12; for both 'numeric' and 'alphanumeric' charsets verify
pattern equals '[0-9]{4}'/'[A-Z0-9]{4}' and '[0-9]{12}'/'[A-Z0-9]{12}',
placeholders have lengths 4 and 12 respectively, inputmode/autocapitalize remain
correct, and the RegExp tests accept/ reject appropriate strings (correct length
and charset, reject too short/too long and lowercase for alphanumeric). Update
the tests inside the same describe('Recovery flow: OTP input props') block
(reuse the existing it blocks or add new ones) referencing the
buildOtpInputProps function and pattern/placeholder fields to pin minimum (4)
and maximum (12) cases.
In `@packages/auth-service/src/email/sender.ts`:
- Line 233: The plain-text fallback email body uses the raw code string; update
the text template in sender.ts to use the same formatted OTP as the subject by
calling formatOtpPlain(code) instead of inserting code directly so long OTPs are
grouped consistently (refer to the text template, the code variable and the
formatOtpPlain function).
- Around line 276-279: The plain-text email body currently inserts the raw
variable code while the subject uses formatOtpPlain(code), causing
inconsistency; update the plain-text body construction in the email sender (the
block that builds the message array containing `Your sign-in code for
${clientAppName}:`, `''`, `code`, `''`) to use formatOtpPlain(code) instead of
raw code so both subject and body display the OTP in the same formatted/plain
form.
- Around line 327-329: Update the plain-text bodies so the OTP appears inline
with its label (e.g., "Your verification code: <code>") to match the subject
line; locate the plain-text body construction in sender.ts (functions/methods
that build the verification and welcome email bodies—e.g.,
sendVerificationEmail/sendWelcomeEmail or the variables that assemble the arrays
using `code`) and replace the separate array elements (`"Your verification
code:", "", code`) with a single line that concatenates the label and the code
into one plain-text string; apply the same change to the welcome email
plain-text body to keep formatting consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9985a6c2-d48d-44e1-b7ff-b4b0cddb8243
📒 Files selected for processing (18)
.beads/issues.jsonl.env.examplepackages/auth-service/.env.examplepackages/auth-service/src/__tests__/consent.test.tspackages/auth-service/src/__tests__/otp-input.test.tspackages/auth-service/src/__tests__/recovery.test.tspackages/auth-service/src/better-auth.tspackages/auth-service/src/context.tspackages/auth-service/src/email/sender.tspackages/auth-service/src/index.tspackages/auth-service/src/otp-input.tspackages/auth-service/src/routes/account-login.tspackages/auth-service/src/routes/login-page.tspackages/auth-service/src/routes/recovery.tspackages/shared/src/__tests__/html.test.tspackages/shared/src/crypto.tspackages/shared/src/html.tspackages/shared/src/index.ts
💤 Files with no reviewable changes (1)
- packages/auth-service/src/tests/recovery.test.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/shared/src/html.ts
- packages/shared/src/tests/html.test.ts
- packages/auth-service/.env.example
- packages/shared/src/index.ts
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/auth-service/src/routes/login-page.ts (1)
321-333:⚠️ Potential issue | 🟠 MajorProvide an accessible name for the OTP input.
At Line 329, the OTP field has no
<label>/aria-label. Please bind it to the existing subtitle (id="otp-subtitle") viaaria-describedbyand add an explicit accessible name.Suggested fix
- <input type="text" id="code" name="code" required + <input type="text" id="code" name="code" required + aria-label="One-time code" + aria-describedby="otp-subtitle"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/auth-service/src/routes/login-page.ts` around lines 321 - 333, The OTP input with id "code" inside form "form-verify-otp" lacks an accessible name; add aria-describedby="otp-subtitle" to link the existing subtitle (id "otp-subtitle") and provide an explicit accessible name by either adding a visible <label for="code"> (preferred) or adding an aria-label attribute (e.g., aria-label="One-time code") on the input; ensure the label text matches the UX (e.g., "Enter one-time code") and keep the id "code" so the for/aria reference resolves correctly.
♻️ Duplicate comments (1)
packages/auth-service/src/routes/recovery.ts (1)
262-270:⚠️ Potential issue | 🟠 MajorAdd an accessible name/description for the recovery OTP field.
The OTP input at Line 269 is still unlabeled for assistive tech. Please give it an accessible name and bind the instructional subtitle via
aria-describedby.Suggested fix
- <p class="subtitle">If a backup email matches, we sent a ${opts.otpLength}-${opts.otpCharset === 'alphanumeric' ? 'character' : 'digit'} code to <strong>${escapeHtml(maskedEmail)}</strong></p> + <p id="code-help" class="subtitle">If a backup email matches, we sent a ${opts.otpLength}-${opts.otpCharset === 'alphanumeric' ? 'character' : 'digit'} code to <strong>${escapeHtml(maskedEmail)}</strong></p> @@ - <input type="text" id="code" name="code" required autofocus + <input type="text" id="code" name="code" required autofocus + aria-label="Recovery code" + aria-describedby="code-help"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/auth-service/src/routes/recovery.ts` around lines 262 - 270, The OTP input (id="code") lacks an accessible name/description; add an explicit label and/or aria attributes by giving the subtitle paragraph (class "subtitle") a unique id (e.g., "otp-instructions") and then reference it from the input via aria-describedby="otp-instructions", and also add an associated <label for="code"> (or aria-label) for the input; update the template in recovery.ts around the subtitle and the input to include the subtitle id and the input's aria-describedby (and ensure the label's for matches id="code") so screen readers will announce the instructions and field name.
🧹 Nitpick comments (1)
packages/auth-service/src/__tests__/otp-input.test.ts (1)
14-14: Consider a more general describe block name.
buildOtpInputPropsis used across login, recovery, and account-settings flows. The current name "Recovery flow: OTP input props" implies it's recovery-specific. A name like "buildOtpInputProps" would be clearer.📝 Suggested change
-describe('Recovery flow: OTP input props', () => { +describe('buildOtpInputProps', () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/auth-service/src/__tests__/otp-input.test.ts` at line 14, Rename the test suite's describe block to be generic rather than recovery-specific: change the string "Recovery flow: OTP input props" to a neutral name like "buildOtpInputProps" (or "OTP input props") so the tests reflect that buildOtpInputProps is used across login, recovery, and account-settings; update the describe invocation in otp-input.test.ts accordingly to avoid implying recovery-only scope.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/auth-service/.env.example`:
- Around line 63-65: Update the comment for the OTP_LENGTH environment variable
in packages/auth-service/.env.example: change the phrase "number of digits in
the email verification code" to "number of characters in the verification code"
(referencing the OTP_LENGTH comment block and the OTP_LENGTH variable) so it
matches the alphanumeric charset option wording and clarifies it applies to
characters, not only digits.
In `@packages/auth-service/src/better-auth.ts`:
- Around line 121-127: The eslint-disable directive is currently placed above
the function and reported as unused while the `@typescript-eslint/no-explicit-any`
violation is on the return type of createBetterAuth; move the disable so it
directly applies to the return type (or use an inline disable) — e.g., remove
the top-line "// eslint-disable-next-line `@typescript-eslint/no-explicit-any`"
and instead annotate the return type with an inline comment that disables
`@typescript-eslint/no-explicit-any` for that specific "any" (so the linter sees
the disable and the createBetterAuth return signature no longer triggers an
error).
In `@packages/auth-service/src/index.ts`:
- Around line 140-153: The OTP length parsing currently uses parseInt when
building config.otpLength which will silently accept malformed values like
"8abc"; update the parsing logic in the config initialization to use
Number(process.env.OTP_LENGTH || '8') (or equivalent strict numeric parsing) for
otpLength, then keep the existing validation that checks isNaN(config.otpLength)
and range (4–12); ensure config.otpLength remains a number and that the thrown
Error message still includes the original process.env.OTP_LENGTH for
diagnostics.
In `@packages/auth-service/src/routes/account-login.ts`:
- Around line 194-201: The OTP input in the account-login template (the <input
id="otp" name="otp"> in the account-login route template) lacks an accessible
name; add one by providing either a visible <label for="otp">OTP code</label>
before the input or an explicit aria-label/aria-labelledby attribute (e.g.,
aria-label="One-time passcode" or aria-labelledby referencing a hidden
descriptive element) so screen readers can announce the control; ensure the
label text matches the UI copy (use escapeHtml if inserting dynamic text) and
keep the input's id="otp" to preserve existing behavior and form handling.
- Around line 24-28: Move the ESLint disable so it actually suppresses the auth:
any parameter: relocate the existing "// eslint-disable-next-line
`@typescript-eslint/no-explicit-any` -- better-auth instance has no exported type"
into the function parameter list immediately before the auth: any declaration
(or place it on the line directly above that parameter) in
createAccountLoginRouter so the no-explicit-any rule is suppressed for auth: any
rather than the function declaration line; keep the rest of the signature (ctx:
AuthServiceContext): Router unchanged.
---
Outside diff comments:
In `@packages/auth-service/src/routes/login-page.ts`:
- Around line 321-333: The OTP input with id "code" inside form
"form-verify-otp" lacks an accessible name; add aria-describedby="otp-subtitle"
to link the existing subtitle (id "otp-subtitle") and provide an explicit
accessible name by either adding a visible <label for="code"> (preferred) or
adding an aria-label attribute (e.g., aria-label="One-time code") on the input;
ensure the label text matches the UX (e.g., "Enter one-time code") and keep the
id "code" so the for/aria reference resolves correctly.
---
Duplicate comments:
In `@packages/auth-service/src/routes/recovery.ts`:
- Around line 262-270: The OTP input (id="code") lacks an accessible
name/description; add an explicit label and/or aria attributes by giving the
subtitle paragraph (class "subtitle") a unique id (e.g., "otp-instructions") and
then reference it from the input via aria-describedby="otp-instructions", and
also add an associated <label for="code"> (or aria-label) for the input; update
the template in recovery.ts around the subtitle and the input to include the
subtitle id and the input's aria-describedby (and ensure the label's for matches
id="code") so screen readers will announce the instructions and field name.
---
Nitpick comments:
In `@packages/auth-service/src/__tests__/otp-input.test.ts`:
- Line 14: Rename the test suite's describe block to be generic rather than
recovery-specific: change the string "Recovery flow: OTP input props" to a
neutral name like "buildOtpInputProps" (or "OTP input props") so the tests
reflect that buildOtpInputProps is used across login, recovery, and
account-settings; update the describe invocation in otp-input.test.ts
accordingly to avoid implying recovery-only scope.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 61444110-adbf-43c8-a56d-3233281258cb
📒 Files selected for processing (19)
.beads/issues.jsonl.env.exampleAGENTS.mdpackages/auth-service/.env.examplepackages/auth-service/src/__tests__/consent.test.tspackages/auth-service/src/__tests__/otp-input.test.tspackages/auth-service/src/__tests__/recovery.test.tspackages/auth-service/src/better-auth.tspackages/auth-service/src/context.tspackages/auth-service/src/email/sender.tspackages/auth-service/src/index.tspackages/auth-service/src/otp-input.tspackages/auth-service/src/routes/account-login.tspackages/auth-service/src/routes/login-page.tspackages/auth-service/src/routes/recovery.tspackages/shared/src/__tests__/html.test.tspackages/shared/src/crypto.tspackages/shared/src/html.tspackages/shared/src/index.ts
💤 Files with no reviewable changes (1)
- packages/auth-service/src/tests/recovery.test.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/auth-service/src/context.ts
- packages/auth-service/src/tests/consent.test.ts
- packages/auth-service/src/otp-input.ts
- packages/shared/src/crypto.ts
Kzoeps
force-pushed
the
karma/hyper-113-switch-to-6-digit-otp
branch
from
March 18, 2026 05:51
70508d5 to
2f5a34e
Compare
Kzoeps
force-pushed
the
karma/hyper-113-switch-to-6-digit-otp
branch
from
March 18, 2026 05:58
2f5a34e to
365029f
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/auth-service/src/routes/account-login.ts (1)
41-43:⚠️ Potential issue | 🟡 MinorDo not swallow session lookup errors silently.
Please log this path at
debugso auth/session integration failures remain observable.🔧 Suggested fix
- } catch { - /* not logged in, continue */ + } catch (err) { + logger.debug({ err }, 'Session lookup failed; continuing as unauthenticated') }As per coding guidelines: Never swallow errors silently — log at minimum
debuglevel.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/auth-service/src/routes/account-login.ts` around lines 41 - 43, The catch block in the account-login route currently swallows session lookup errors; change the catch to capture the error (catch (err)) and emit a debug-level log including the error and context (e.g., "session lookup failed in account-login") using the route's logger (e.g., logger.debug or req.log.debug) so failures in the session lookup (the code around the session retrieval in account-login.ts) remain observable for debugging.packages/auth-service/src/better-auth.ts (1)
86-122:⚠️ Potential issue | 🟠 MajorAlways close the migration DB handle via
finally.If
betterAuth(...),getMigrations(...), orrunMigrations()throws, the handle may not close. Wrap migration logic intry/finally.🔧 Suggested fix
export async function runBetterAuthMigrations( dbLocation: string, authHostname: string, otpLength: number, otpCharset: 'numeric' | 'alphanumeric' = 'numeric', ): Promise<void> { const betterAuthDb = new Database(dbLocation) - const tempAuth = betterAuth({ - secret: process.env.AUTH_SESSION_SECRET, - database: betterAuthDb, - baseURL: `https://${authHostname}`, - basePath: '/api/auth', - plugins: [ - emailOTP({ - otpLength, - expiresIn: 600, - allowedAttempts: 5, - storeOTP: 'hashed', - ...(otpCharset === 'alphanumeric' - ? { generateOTP: () => generateRandomString(otpLength, 'A-Z', '0-9') } - : {}), - async sendVerificationOTP() {}, - }), - ], - }) - const { toBeCreated, toBeAdded, runMigrations } = await getMigrations( - tempAuth.options, - ) - if (toBeCreated.length > 0 || toBeAdded.length > 0) { - logger.info( - { - toBeCreated: toBeCreated.map((t) => t.table), - toBeAdded: toBeAdded.map((t) => t.table), - }, - 'Running better-auth migrations', - ) - await runMigrations() - logger.info('better-auth migrations complete') - } else { - logger.info('better-auth schema up to date, no migrations needed') + try { + const tempAuth = betterAuth({ + secret: process.env.AUTH_SESSION_SECRET, + database: betterAuthDb, + baseURL: `https://${authHostname}`, + basePath: '/api/auth', + plugins: [ + emailOTP({ + otpLength, + expiresIn: 600, + allowedAttempts: 5, + storeOTP: 'hashed', + ...(otpCharset === 'alphanumeric' + ? { generateOTP: () => generateRandomString(otpLength, 'A-Z', '0-9') } + : {}), + async sendVerificationOTP() {}, + }), + ], + }) + const { toBeCreated, toBeAdded, runMigrations } = await getMigrations( + tempAuth.options, + ) + if (toBeCreated.length > 0 || toBeAdded.length > 0) { + logger.info( + { + toBeCreated: toBeCreated.map((t) => t.table), + toBeAdded: toBeAdded.map((t) => t.table), + }, + 'Running better-auth migrations', + ) + await runMigrations() + logger.info('better-auth migrations complete') + } else { + logger.info('better-auth schema up to date, no migrations needed') + } + } finally { + betterAuthDb.close() } - betterAuthDb.close() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/auth-service/src/better-auth.ts` around lines 86 - 122, The migration DB handle betterAuthDb may not be closed if betterAuth, getMigrations, or runMigrations throws; wrap the migration sequence (creation of tempAuth via betterAuth, the call to getMigrations, the conditional runMigrations and related logger calls) in a try/finally and call betterAuthDb.close() inside the finally block so the DB is always closed even on errors.
♻️ Duplicate comments (1)
packages/auth-service/src/routes/account-login.ts (1)
192-200:⚠️ Potential issue | 🟠 MajorAdd an accessible name for the OTP input.
The input is currently unlabeled for assistive tech. Add a visible
<label>oraria-label/aria-labelledby.🔧 Suggested fix
<div class="field"> + <label for="otp">One-time code</label> <input type="text" id="otp" name="otp" required autofocus maxlength="${opts.otpLength}" pattern="${inputProps.pattern}"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/auth-service/src/routes/account-login.ts` around lines 192 - 200, The OTP input with id="otp" and class="otp-input" is missing an accessible name for assistive tech; add either a visible <label> associated via for="otp" (preferred) or include aria-label/aria-labelledby on the input to provide a descriptive name (e.g., "One-time code" or "OTP"); ensure the label text is visible and matches localization patterns used elsewhere, and update any tests/styles that rely on the input selector if needed.
🧹 Nitpick comments (2)
packages/auth-service/src/__tests__/otp-input.test.ts (1)
31-47: Add boundary tests for supported OTP range (4 and 12).You already verify mid-range lengths; adding explicit min/max checks will lock behavior to the supported config range and prevent regressions.
💡 Suggested test addition
+ it.each([4, 12])('supports boundary otpLength %i for both charsets', (len) => { + const numeric = buildOtpInputProps(len, 'numeric') + expect(numeric.pattern).toBe(`[0-9]{${len}}`) + expect(numeric.placeholder).toHaveLength(len) + + const alpha = buildOtpInputProps(len, 'alphanumeric') + expect(alpha.pattern).toBe(`[A-Z0-9]{${len}}`) + expect(alpha.placeholder).toHaveLength(len) + })Based on learnings: OTP codes are configurable via
OTP_LENGTHwith a supported range of 4–12.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/auth-service/src/__tests__/otp-input.test.ts` around lines 31 - 47, Add explicit boundary tests for the minimum and maximum supported OTP lengths by using buildOtpInputProps with lengths 4 and 12: for each (call buildOtpInputProps(4, ...) and buildOtpInputProps(12, ...) for both 'numeric' and 'alphanumeric'), assert the returned .pattern equals the expected pattern ('[0-9]{N}' or '[A-Z0-9]{N}'), .placeholder has length N, and verify a RegExp(`^${pattern}$`) accepts a valid string of exact length N and rejects strings that are one shorter and one longer to lock behavior to the supported OTP_LENGTH range.packages/auth-service/src/routes/login-page.ts (1)
333-334: Consider uppercasing inoninputfor alphanumeric mode UX.This avoids avoidable client-side pattern rejections when users type/paste lowercase characters.
💡 Suggested tweak
- autocapitalize="${inputProps.autocapitalize}" - oninput="this.value=this.value.replace(/[\\s-]/g,'')" + autocapitalize="${inputProps.autocapitalize}" + oninput="this.value=this.value.replace(/[\\s-]/g,'').toUpperCase()" style="letter-spacing: ${Math.max(2, Math.round(32 / opts.otpLength))}px">🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/auth-service/src/routes/login-page.ts` around lines 333 - 334, The input's oninput currently strips spaces and hyphens but doesn't uppercase pasted/typed characters for alphanumeric OTPs; update the oninput handler generation in login-page.ts to, when opts.otpMode === 'alphanumeric', also transform this.value = this.value.toUpperCase() (in addition to replace(/[\\s-]/g,'')) so lowercase letters are normalized before validation; locate the HTML input generation that uses opts.otpLength and opts.otpMode and adjust the oninput string accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/auth-service/src/better-auth.ts`:
- Around line 153-154: Remove the unnecessary and disallowed `as any` cast on
the `database` property: replace `database: betterAuthDb as any` with `database:
betterAuthDb`. Ensure the object passed into the `betterAuth()` call uses the
same uncast `betterAuthDb` instance (as is used by `runBetterAuthMigrations()`),
and update any surrounding type annotations if the compiler complains so the
`database` property matches the expected type of `betterAuth()` without using
`any`.
In `@packages/auth-service/src/routes/login-page.ts`:
- Around line 329-334: The OTP input with id="code" (class "otp-input") lacks an
accessible name; add a visible <label for="code"> (preferred) or, if visual
layout prevents it, add a descriptive aria-label/aria-labelledby for the same id
so screen readers identify the field (ensure the label text describes it as
one-time code/OTP and the for attribute matches "code"). Also verify
required/maxlength attributes remain and that any placeholder remains decorative
(not relied on as the only label).
---
Outside diff comments:
In `@packages/auth-service/src/better-auth.ts`:
- Around line 86-122: The migration DB handle betterAuthDb may not be closed if
betterAuth, getMigrations, or runMigrations throws; wrap the migration sequence
(creation of tempAuth via betterAuth, the call to getMigrations, the conditional
runMigrations and related logger calls) in a try/finally and call
betterAuthDb.close() inside the finally block so the DB is always closed even on
errors.
In `@packages/auth-service/src/routes/account-login.ts`:
- Around line 41-43: The catch block in the account-login route currently
swallows session lookup errors; change the catch to capture the error (catch
(err)) and emit a debug-level log including the error and context (e.g.,
"session lookup failed in account-login") using the route's logger (e.g.,
logger.debug or req.log.debug) so failures in the session lookup (the code
around the session retrieval in account-login.ts) remain observable for
debugging.
---
Duplicate comments:
In `@packages/auth-service/src/routes/account-login.ts`:
- Around line 192-200: The OTP input with id="otp" and class="otp-input" is
missing an accessible name for assistive tech; add either a visible <label>
associated via for="otp" (preferred) or include aria-label/aria-labelledby on
the input to provide a descriptive name (e.g., "One-time code" or "OTP"); ensure
the label text is visible and matches localization patterns used elsewhere, and
update any tests/styles that rely on the input selector if needed.
---
Nitpick comments:
In `@packages/auth-service/src/__tests__/otp-input.test.ts`:
- Around line 31-47: Add explicit boundary tests for the minimum and maximum
supported OTP lengths by using buildOtpInputProps with lengths 4 and 12: for
each (call buildOtpInputProps(4, ...) and buildOtpInputProps(12, ...) for both
'numeric' and 'alphanumeric'), assert the returned .pattern equals the expected
pattern ('[0-9]{N}' or '[A-Z0-9]{N}'), .placeholder has length N, and verify a
RegExp(`^${pattern}$`) accepts a valid string of exact length N and rejects
strings that are one shorter and one longer to lock behavior to the supported
OTP_LENGTH range.
In `@packages/auth-service/src/routes/login-page.ts`:
- Around line 333-334: The input's oninput currently strips spaces and hyphens
but doesn't uppercase pasted/typed characters for alphanumeric OTPs; update the
oninput handler generation in login-page.ts to, when opts.otpMode ===
'alphanumeric', also transform this.value = this.value.toUpperCase() (in
addition to replace(/[\\s-]/g,'')) so lowercase letters are normalized before
validation; locate the HTML input generation that uses opts.otpLength and
opts.otpMode and adjust the oninput string accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f2ff2765-516d-4aec-a21b-ce1521e391cc
📒 Files selected for processing (9)
AGENTS.mdpackages/auth-service/src/__tests__/otp-input.test.tspackages/auth-service/src/__tests__/recovery.test.tspackages/auth-service/src/better-auth.tspackages/auth-service/src/otp-input.tspackages/auth-service/src/routes/account-login.tspackages/auth-service/src/routes/login-page.tspackages/auth-service/src/routes/recovery.tspackages/shared/src/__tests__/html.test.ts
💤 Files with no reviewable changes (1)
- packages/auth-service/src/tests/recovery.test.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- AGENTS.md
- packages/auth-service/src/routes/recovery.ts
- packages/shared/src/tests/html.test.ts
- packages/auth-service/src/otp-input.ts
Kzoeps
force-pushed
the
karma/hyper-113-switch-to-6-digit-otp
branch
from
March 18, 2026 06:17
4659750 to
a8f5533
Compare
keep generate otp code as is for now fix: linting issues
Kzoeps
force-pushed
the
karma/hyper-113-switch-to-6-digit-otp
branch
from
March 18, 2026 06:19
a8f5533 to
c774f26
Compare
aspiers
reviewed
Mar 18, 2026
|
|
||
| # OTP code length — number of characters in the email verification code (default: 8) | ||
| # Must be between 4 and 12 characters. Applies to login, recovery, and account settings OTP flows. | ||
| # OTP_LENGTH=8 |
Contributor
There was a problem hiding this comment.
@Kzoeps Please can you check if you need to add these two vars to scripts/setup.sh?
aspiers
reviewed
Mar 18, 2026
Comment on lines
+146
to
+161
| if ( | ||
| isNaN(config.otpLength) || | ||
| config.otpLength < 4 || | ||
| config.otpLength > 12 | ||
| ) { | ||
| throw new Error( | ||
| `Invalid OTP_LENGTH: must be between 4 and 12, got "${process.env.OTP_LENGTH}"`, | ||
| ) | ||
| } | ||
|
|
||
| const validCharsets = ['numeric', 'alphanumeric'] | ||
| if (!validCharsets.includes(config.otpCharset)) { | ||
| throw new Error( | ||
| `Invalid OTP_CHARSET: must be 'numeric' or 'alphanumeric', got "${process.env.OTP_CHARSET}"`, | ||
| ) | ||
| } |
Contributor
There was a problem hiding this comment.
In general please always look for opportunities to keep functions small and extract logic out into smaller chunks. Keeping everything smaller yields an enormous boost in code quality / maintainability / legibility (even for agents). For example these bits could be refactored as getValidOtpConfig() or something like that.
Feel free to adopt https://github.com/aspiers/ai-config/blob/main/.agents/skills/code-refactoring-small/SKILL.md
aspiers
reviewed
Mar 18, 2026
aspiers
approved these changes
Mar 18, 2026
Contributor
|
@Kzoeps Thanks, merging with just code review and no interactive testing - taking it on trust this works for the sake of speed, and it's low risk anyway :-) Would be nice to have small follow-ups to address the minor feedback I found. |
aspiers
added a commit
that referenced
this pull request
Apr 9, 2026
The retroactive changesets added in #58 were all committed in the same commit, so @changesets/changelog-github looked up that one commit and used its PR (#58) and SHA on every generated entry in the v0.2.0 release section. That's the generator behaving correctly for its model (one changeset = one originating commit = one originating PR) but it produces misleading "this change was shipped in #58" refs when in reality each described feature landed weeks earlier in a different PR. Replace each bogus prefix with links to the real introducing PR(s). Commit SHAs are dropped entirely because a feature that spans multiple commits can't be meaningfully linked to a single SHA, and the PR link already carries the useful context. Future (non-retroactive) changesets won't hit this — they'll be added in the same PR as the feature they describe, so the generator's single-commit lookup resolves to the right PR automatically. This fix is a one-off cleanup for the bootstrap release. PR → changeset mapping: #14 → Longer sign-in codes #13, #29, #33, #36 → Choose your own handle #3, #6 → Sign in faster from third-party apps #20, #23 → Fail-fast PDS_INTERNAL_URL validation #27 → Honour PORT env var
aspiers
added a commit
that referenced
this pull request
Apr 9, 2026
The retroactive changesets added in #58 were all committed in the same commit, so @changesets/changelog-github looked up that one commit and used its PR (#58) and SHA on every generated entry in the v0.2.0 release section. That's the generator behaving correctly for its model (one changeset = one originating commit = one originating PR) but it produces misleading "this change was shipped in #58" refs when in reality each described feature landed weeks earlier in a different PR. Replace each bogus prefix with links to the real introducing PR(s). Commit SHAs are dropped entirely because a feature that spans multiple commits can't be meaningfully linked to a single SHA, and the PR link already carries the useful context. Future (non-retroactive) changesets won't hit this — they'll be added in the same PR as the feature they describe, so the generator's single-commit lookup resolves to the right PR automatically. This fix is a one-off cleanup for the bootstrap release. PR → changeset mapping: #14 → Longer sign-in codes #13, #29, #33, #36 → Choose your own handle #3, #6 → Sign in faster from third-party apps #20, #23 → Fail-fast PDS_INTERNAL_URL validation #27 → Honour PORT env var
aspiers
added a commit
that referenced
this pull request
Apr 11, 2026
…PER-301) These features have been released since v0.2.0 but were missing from docs/configuration.md. Auth Service variables: - OTP_LENGTH (4-12, default 8) and OTP_CHARSET (numeric/alphanumeric) added in v0.2.0 (#14). - EPDS_DEFAULT_HANDLE_MODE (picker/random/picker-with-random) added in v0.2.0 (#13/#29/#33/#36). Port rows now mention the v0.2.0 PORT fallback precedence (#27): - auth service: AUTH_PORT -> PORT -> 3001 - pds-core: PDS_PORT -> PORT -> 3000 So that operators migrating pre-0.2.0 Railway setups learn they can drop service-specific port overrides. Refs HYPER-295.
This was referenced Apr 11, 2026
This was referenced Apr 17, 2026
4 tasks
This was referenced Jul 15, 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
2 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.
Summary
OTP_LENGTHenv var toauth-service, validated on startup and propagated throughAuthServiceConfigto all route handlers and better-auth setupotpCharsetfield toAuthServiceConfigand alphanumeric OTP generation support inbetter-auth.tsOTP_LENGTH > 6groups by 4 or 3 if they're divisible.Summary by CodeRabbit
New Features
Documentation