fix(ux/auth): name only the missing password criteria, not all four - #470
Conversation
QA review on the reset-password / admin-setup / profile-change / user-create
flows noted that the bundled "must contain at least one uppercase letter,
one lowercase letter, one number, and one special character" message
appears even when only one rule actually fails. The length rule already
returns its own targeted message; complexity was the only diverger.
Add describePasswordValidationError() in auth.ts that returns the
single specific message when one rule fails, or names only the rules
that actually failed when several fail ("Password must contain one
uppercase letter and one special character"). Replace the bundled
string at all four call sites — both modals in auth.ts, the profile
change-password flow, and the user-create modal.
Closes #458.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds module-level SPECIAL_CHAR_RE and exported describePasswordValidationError(password); updates profile modal to show a live requirements checklist and inline password errors; replaces duplicated complexity checks in reset-password, admin-setup, and create-user flows with the helper; updates tests to assert inline errors and live indicator behavior. ChangesPassword validation consolidation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review Generated by Claude Code |
🧩 Analysis chain🏁 Script executed: Length of output: 8440
🔴 Potential regex inconsistency —
|
CodeRabbit on #470: updatePasswordRequirements (the live ○/✓ indicator) had its own inline /[!@#$%^&*()_+\-=\[\]{};':"\\|,.<>\/?]/ regex while describePasswordValidationError used SPECIAL_CHAR_RE — if either ever edited the character set without the other, the live indicator and the submit-time validator would silently disagree (indicator says "all green" while validator still rejects, or vice versa). Hoist SPECIAL_CHAR_RE to module-level (before updatePasswordRequirements so both can read it) and replace the inline literal. Also swap the "missing.pop() as string" assertion for the conventional non-null "!" since the length-guard above proves it's defined.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
frontend/src/auth.ts (1)
168-191: ⚡ Quick winUse the same special-character regex for live and submit-time checks.
describePasswordValidationErrorusesSPECIAL_CHAR_RE(Line 186), butupdatePasswordRequirementsstill has a separate inline regex (Line 157). Reusing one constant avoids future drift between inline indicators and submit-time errors.♻️ Proposed refactor
function updatePasswordRequirements(password: string, prefix = 'req-'): void { const requirements = { length: password.length >= 12, uppercase: /[A-Z]/.test(password), lowercase: /[a-z]/.test(password), number: /[0-9]/.test(password), - special: /[!@#$%^&*()_+\-=\[\]{};':"\\|,.<>\/?]/.test(password) + special: SPECIAL_CHAR_RE.test(password) };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/auth.ts` around lines 168 - 191, The password-special-character regex is duplicated: describePasswordValidationError uses SPECIAL_CHAR_RE but updatePasswordRequirements uses an inline regex; this can drift. Modify updatePasswordRequirements to import/use the existing SPECIAL_CHAR_RE constant instead of its inline /[!@#$%^&*()_+\-=\[\]{};':"\\|,.<>\/?]/ regex so both live UI indicators and submit-time validation (functions updatePasswordRequirements and describePasswordValidationError) use the same canonical regex constant.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@frontend/src/auth.ts`:
- Around line 168-191: The password-special-character regex is duplicated:
describePasswordValidationError uses SPECIAL_CHAR_RE but
updatePasswordRequirements uses an inline regex; this can drift. Modify
updatePasswordRequirements to import/use the existing SPECIAL_CHAR_RE constant
instead of its inline /[!@#$%^&*()_+\-=\[\]{};':"\\|,.<>\/?]/ regex so both live
UI indicators and submit-time validation (functions updatePasswordRequirements
and describePasswordValidationError) use the same canonical regex constant.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f9bc663d-d25d-45a4-a7d5-6840ed6d496d
📒 Files selected for processing (2)
frontend/src/auth.tsfrontend/src/users/userModals.ts
|
@coderabbitai review Generated by Claude Code |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 9297
Previous findings — resolved ✅
No new issues found. The diff is clean: the constant placement, guard logic, message composition, and import in One remaining non-code item 🟡 The manual test plan checkboxes in the PR description are still unchecked. Worth ticking them off for the four flows (reset-password, admin-setup, profile-change, user-create) before merging — just to confirm the error message surfaces correctly end-to-end. [approve] |
|
@coderabbitai full review The previous review flagged that Generated by Claude Code |
🧩 Analysis chain🏁 Script executed: Length of output: 35709
✅ Previously flagged items — confirmed fixed
🔍 Additional findings from the full file
Profile modal lacks live password indicators — pre-existing, out of scope 🟡
SummaryNo new issues introduced. The SPECIAL_CHAR_RE unification is solid, the message helper is correct, and the four validation sites are consistently handled. The PR is ready to merge. ✅ Actions performedFull review triggered. |
The Profile change-password modal was the only password-entry flow without live `req-*` criterion indicators and the only one that surfaced password-validation failures via `alert(...)` instead of an inline `<div class="error-message">`. PR #470 unified the failure TEXT across all four flows; this commit closes the remaining two UX gaps so the surface matches too. - Replicate the five `password-requirements` rows from the reset / admin-setup modals under `#profile-new-password`, using a flow-specific `profile-req-` ID prefix so they cannot collide with the reset (`req-`) or setup (`setup-req-`) indicators if both modals are ever mounted concurrently. `updatePasswordRequirements` already takes a prefix parameter — no helper change needed. - Wire `input` events on the new-password field to the live updater. Listener is registered once on initial modal creation (the modal DOM is reused across opens), and each `openProfileModal` call resets the indicators to their empty-input state so a previous session's check-marks don't bleed in. - Add `<div id="profile-password-error" class="error-message hidden">` next to the password fields and route both new-password validation paths (length/complexity via `describePasswordValidationError`, plus the confirm-mismatch path) through it instead of `alert(...)`. The error clears on every submit attempt and on modal reopen. Other `saveProfile` paths (missing current password, success toast, API error) keep their `alert(...)` surface — out of scope here. Also add `autocomplete="new-password"` to the two profile password inputs so password managers don't auto-fill the user's stored credential into "new password" fields (parity with reset / setup). Tests: four new cases in the profile-modal suite covering live indicator rendering, criterion class toggling on input, indicator + error reset on reopen, and inline-error rendering for the length/complexity failure path. The existing `save profile validates new password confirmation` test was updated to assert the inline div surface and that `alert` is NOT called.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
frontend/src/__tests__/auth.test.ts (1)
542-568: ⚡ Quick winRe-query
profile-password-errorafter reopening the modal.This test keeps a pre-reopen
errorDivreference and asserts it after reopen. If the modal is re-rendered, that reference may be stale and not represent the live DOM.Proposed test hardening
- const errorDiv = document.getElementById('profile-password-error') as HTMLElement; - errorDiv.textContent = 'old error'; - errorDiv.classList.remove('hidden'); + const errorDiv = document.getElementById('profile-password-error') as HTMLElement; + errorDiv.textContent = 'old error'; + errorDiv.classList.remove('hidden'); @@ - expect(errorDiv.textContent).toBe(''); - expect(errorDiv.classList.contains('hidden')).toBe(true); + const reopenedErrorDiv = document.getElementById('profile-password-error') as HTMLElement; + expect(reopenedErrorDiv.textContent).toBe(''); + expect(reopenedErrorDiv.classList.contains('hidden')).toBe(true);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/__tests__/auth.test.ts` around lines 542 - 568, The test "reopening profile modal resets stale indicators and error" holds a stale reference in errorDiv to '`#profile-password-error`' before the modal is closed; after the modal is re-opened you must re-query the DOM for that element and use the fresh reference in assertions. Update the test to call document.getElementById('profile-password-error') (or querySelector) again after userEmail.click() and awaiting, then assert on that fresh element's textContent and classList; keep the existing checks for 'profile-req-length' and 'profile-req-uppercase' as-is.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@frontend/src/__tests__/auth.test.ts`:
- Around line 542-568: The test "reopening profile modal resets stale indicators
and error" holds a stale reference in errorDiv to '`#profile-password-error`'
before the modal is closed; after the modal is re-opened you must re-query the
DOM for that element and use the fresh reference in assertions. Update the test
to call document.getElementById('profile-password-error') (or querySelector)
again after userEmail.click() and awaiting, then assert on that fresh element's
textContent and classList; keep the existing checks for 'profile-req-length' and
'profile-req-uppercase' as-is.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fe086905-0016-476d-8245-6105b2de90cd
📒 Files selected for processing (2)
frontend/src/__tests__/auth.test.tsfrontend/src/auth.ts
CodeRabbit on #470: the "reopening profile modal resets stale indicators and error" test caches errorDiv before close-and-reopen and then asserts on it. The close-and-reopen path re-renders the modal, so the cached reference is detached from the live DOM and the assertions would silently pass even if the new modal regressed. Re-query #profile-password-error after the reopen, matching the pattern already used for #profile-req-length / #profile-req-uppercase two lines above.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Summary
QA review on the reset-password / admin-setup / profile-change / user-create flows found that when a new password fails multiple complexity rules at once, the form surfaces a single bundled message listing every rule ("uppercase, lowercase, number, and special") even when only one is missing. The length rule already returns its own targeted message; complexity was the only diverger.
Fix
frontend/src/auth.ts:describePasswordValidationError(password)that returns a single specific message ("Password must contain one uppercase letter") when one rule fails, or names only the rules that actually failed when several fail ("Password must contain one uppercase letter and one special character"). Length keeps its own message.handleResetPasswordSubmit(reset-password modal)handleAdminSetupSubmit(admin bootstrap modal)frontend/src/users/userModals.ts::saveUser(admin-creating-a-user)Result: 2 files, +38 / −51.
Closes #458.
Test plan
go build ./...(no Go change, sanity)Out of scope
validateCharacterRequirements(internal/auth/service_password.go) still emits a bundled message; the live indicator on the modal usually short-circuits before the request goes out, so the backend message is rarely user-visible. If we want full parity, that's a follow-up — the live indicators already mirror the new copy.Additional fixes (2026-05-20)
Review of PR #470 surfaced two pre-existing UX gaps in the Profile change-password modal that the original PR did not touch (it only changed text). Added in commit
adc33db:password-requirementsblock withreq-*rows that turn green as each rule is satisfied; the Profile modal had none. Replicated the same block under#profile-new-passwordwith aprofile-req-ID prefix (so it cannot collide with the reset / setup indicators if both modals are mounted concurrently), and wired theinputevent toupdatePasswordRequirements(value, 'profile-req-'). The existing helper already accepts a prefix parameter — no helper change needed.saveProfileusesalert()for password-validation failures while reset / setup use inline<div class="error-message">. Now that PR fix(ux/auth): name only the missing password criteria, not all four #470 unified the failure TEXT, the surface was the only inconsistency left. Added<div id="profile-password-error" class="error-message hidden">next to the password fields and routed both new-password validation paths (length/complexity + confirm-mismatch) through it. OthersaveProfilepaths (missing current password, success toast, API error) keep their existingalert(...)surface — out of scope here.Tests: four new cases in
frontend/src/__tests__/auth.test.tscovering live indicator rendering, criterion class toggling on input, indicator + error reset on reopen, and inline-error rendering for the length/complexity failure path. Existingsave profile validates new password confirmationcase updated to assert the inline div and thatalertis NOT called.Generated by Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests