Skip to content

fix(frontend): users, API key and auth form fixes - #1133

Merged
cristim merged 16 commits into
mainfrom
fix/frontend-forms-batch-c
Oct 19, 2026
Merged

cristim merged 16 commits into
mainfrom
fix/frontend-forms-batch-c

Conversation

@cristim

@cristim cristim commented Oct 19, 2026

Copy link
Copy Markdown
Member

Summary

Seven frontend form, secret-handling and accessibility fixes in the users, API keys, auth and recommendations views.

Closes #274
Closes #717
Closes #732
Closes #733
Closes #735
Closes #736
Closes #746

Issue Fix
#274 The synthetic current-user row has Edit, Delete, selection and group toggles disabled, is skipped by select-all, and carries the session MFA state
#717 API key revoke and delete failures show as a toast when the create modal is closed
#732 API key creation is disabled while in flight; the API key and MFA recovery code views cannot be closed (Escape, Cancel, Save, Enter, closeModal) until the user acknowledges the secret
#733 Enter in the forgot-password form sends the reset instead of submitting the login handler
#735 Account and user Save buttons are disabled while their requests are in flight
#736 Group membership toggles ask for confirmation; an admin removing their own Administrators membership is warned (keyed on user id); declining reverts without an API call
#746 Users and recommendations table checkboxes get distinguishing aria-labels, including the payment option

Verification evidence (independent reviews, local build and tests at the head SHA) is posted as PR comments.

🤖 Generated with Claude Code

cristim and others added 11 commits October 19, 2026 19:11
Edit, Delete, selection and group toggles on the placeholder row hit
/api/users/current. Disable them, skip the row in select-all, and
exclude it from the MFA stat.

Closes #274

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Add aria-labels: select-all and per-user labels in the users table, and
service, type, region, account, term and payment in the recommendations
rows so each checkbox is distinguishable.

Closes #746

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Ask before adding or removing a group, and warn when an administrator
removes their own Administrators membership. Declining reverts the
checkbox without calling the API.

Closes #736

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…s closed

showError wrote to the always-present #create-apikey-error inside the
hidden create modal, so the toast fallback never ran. Use the inline
error only while that modal is open.

Closes #717

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Disable API key creation while the request is in flight so a double
click cannot mint a second key. Secret views (the API key created modal
and the MFA recovery codes in the profile modal) ignore Escape until the
user acknowledges via data-require-ack, and the profile Cancel button is
disabled meanwhile.

Closes #732

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The login submit listener stayed bound on the swapped-in reset form, so
Enter showed a login error. Swap to a reset submit listener, make the
button type=submit and disable it while the request is in flight.

Closes #733

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ight

Wrap handleAccountFormSubmit (which also serves the registration
approval onSave path) and saveUser in a try/finally that disables the
submit button, as submitOverrideForm already does.

Closes #735

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Save Changes and Enter in the profile form closed the modal through
closeModal, discarding pending MFA recovery codes. closeModal now does
nothing while data-require-ack is set, saveProfile returns early and the
profile Save button is disabled until the codes are acknowledged.

Refs #732

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…er row

The synthetic row hardcoded mfa_enabled false, so the MFA stat and the
row badge were wrong for an admin with MFA on. Use the session user value
and count the row in the stat again.

Refs #274

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Term and payment variants of one cell shared a name. Append the known
payment display label (unknown values are omitted so raw API strings
never reach the attribute).

Refs #746

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…oup set

Compare the session user id instead of the email, and recompute the
group set after the confirm dialog resolves so a concurrent toggle is
not reverted.

Refs #736

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Oct 19, 2026 •

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

  • Run on-demand review

This review includes 16 billable files and costs up to $4.00.

  • Ask an admin to make reviews automatic

Open in CodeRabbit

Reviews can continue after your included limit without a manual trigger. An admin must approve usage-based billing.

Or wait 35 minutes for your next included review.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available. Your 88 included PR review attempts over the past 7 days set your current allowance at 1 review per hour.

Learn how review limits work.

Review configuration:

⚙️ Run configuration
  • Configuration used: Repository: LeanerCloud/cloud-commitments-platform/.coderabbit.yaml
  • Review profile: CHILL
  • Plan: Essentials
  • Run ID: bea92e0f-d45a-4145-a8e9-3fc5d46a5feb
📥 Commits

Reviewing files that changed from the base of the PR and between d9b2aa7 and e8ba070.

📒 Files selected for processing: 16
  • frontend/src/__tests__/apikeys.test.ts
  • frontend/src/__tests__/auth-mfa-enroll.test.ts
  • frontend/src/__tests__/auth.test.ts
  • frontend/src/__tests__/recommendations.test.ts
  • frontend/src/__tests__/settings-accounts.test.ts
  • frontend/src/__tests__/users.test.ts
  • frontend/src/apikeys.ts
  • frontend/src/auth.ts
  • frontend/src/modal.ts
  • frontend/src/recommendations.ts
  • frontend/src/settings.ts
  • frontend/src/users/userActions.ts
  • frontend/src/users/userList.ts
  • frontend/src/users/userModals.ts
  • frontend/tests-e2e/execute-now-warning.spec.ts
  • frontend/tests-e2e/recommendations.spec.ts
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

@cristim cristim added effort/s Hours effort/xs Trivial / one-liner impact/few Limited audience impact/many Affects most users priority/p2 Backlog-worthy severity/high Significant harm type/bug Defect urgency/this-quarter Within the quarter triaged Item has been triaged labels Oct 19, 2026
cristim and others added 5 commits October 19, 2026 23:52
…ames

The row checkbox aria-label became "Select <service> <type> <region> ..."
in #746, so the exact name "Select recommendation" no longer matches.
Locate it with /^Select / as recommendations.spec.ts already does; the
locators are scoped to a single row.

Refs #746

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…x names

Build the label from non-empty parts, show resource_type (engine) and the
same payment text as the table cell, so engine-only variants stay distinct.

Refs #746

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Refs #736

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Refs #733

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Only known payment display labels reach the aria-label; unknown raw API
strings are omitted.

Refs #746

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@cristim

cristim commented Oct 19, 2026

Copy link
Copy Markdown
Member Author

Merge gate at e8ba0709dfeb4d92f707e0730598fd0fe419f323

Independent review 1 (cold brief, non-author): APPROVE 57f2752b678444be63f1e511b95acf755f9712c4 (pass 1 APPROVE at 3eeb9067, pass 2 on the delta)

Pass 1 verified #732 secret views cannot be dismissed before acknowledgement by any path (Escape via closeModal, Cancel and Save disabled, implicit Enter blocked, no backdrop handler on either modal, flag cleared only by the acknowledgement buttons, both enroll and regenerate flows), the synchronous createInFlight mint guard, #733 listener swap, #735 try/finally re-enable, #274, #717 and #746. Mutations killed: closeModal ack guard, createInFlight early return and reset, forgot-password listener, self-demotion detection, both Save re-enables. Survivor: login removeEventListener removal.

Pass 2: 4dd0aeb2 label built from non-empty parts with engine and formatPayment (through escapeHtml); 60bb028f post-dialog re-validation and select-all disable; 57f2752b closes the surviving mutant. Mutations killed: engine branch dropped; latestError re-check disabled; select-all disabled removed; removeEventListener deleted. Targeted jest 721/721.

Pass 3 at e8ba0709dfeb4d92f707e0730598fd0fe419f323: APPROVE. The unknown payment is omitted from the label (one source line; escapeHtml still applied). recommendations.test.ts and xss-recommendations-payment.test.ts pass (419/419). Mutation: restoring formatPayment(rec.payment) fails the XSS test.

Independent review 2 (cold brief, non-author): APPROVE 57f2752b678444be63f1e511b95acf755f9712c4 (pass 1 REQUEST_CHANGES at 3eeb9067)

Pass 1 findings and resolution, each re-checked by the reviewer with a mutation probe that fails a test:

  1. Row checkbox name omitted engine (RDS rows differing only by engine collided): fixed in 4dd0aeb2.
  2. Unmapped payment dropped from the name, trailing space: fixed in 4dd0aeb2 (formatPayment, empty parts filtered).
  3. Post-dialog groupsToSave not re-validated (ci: run the Go and Docker heavy jobs on ubuntu-24.04-arm #1405 view-only plus write combination): fixed in 60bb028f.
  4. Select-all users enabled with only the synthetic row: fixed in 60bb028f.
    5-6. Pre-existing stale allUsers on rapid in-flight toggles; fix(frontend): account, user and registration forms can be double-submitted #735 registration form note: not introduced here.

Pass 2 low finding: the reviewer reported that removing the login removeEventListener left the auth tests green. Orchestrator check against an export of this exact SHA: auth.ts has two such calls. Removing the forgot-password one (auth.ts:836, the #733 swap) fails "Enter in the reset form never runs the login handler (issue #733)" (1 failed, 55 passed). Removing the MFA-step one (auth.ts:773, not part of #733) leaves 56 passing, which is the probe the reviewer most likely ran. The #733 guard holds; finding closed as not reproducible for the #733 line.

Reviewer confirmed: its probe had removed :773; re-run deleting only :836 fails the #733 test. Finding withdrawn.

Pass 3 at e8ba0709dfeb4d92f707e0730598fd0fe419f323: APPROVE. The full local jest run at 57f2752b caught xss-recommendations-payment.test.ts failing: the formatPayment fallback put a raw unknown payment into the aria-label. e8ba0709 goes back to known display labels only, keeping the engine part and the clean join. recommendations.test.ts and xss-recommendations-payment.test.ts pass (419 tests). Nit: two rows that differ only by an unmapped payment share a name (accepted trade-off).

Local verification (macOS, exact head):

  • npm ci, npm run build, npm run lint (0 errors) and npx jest (108 suites, 3423 passed, 1 skipped) all pass.
  • The full jest run at 57f2752b is what caught the xss-recommendations-payment.test.ts regression fixed in e8ba0709.
  • The worktree also holds five staged files from someone else's parked work. None are in this PR's commits; they are recorded in the fleet's needs-user file.

CI: all checks are green at this SHA, including Frontend E2E with the updated checkbox locators.

Threads: none open. CodeRabbit is waived as a merge gate.

@cristim

cristim commented Oct 19, 2026

Copy link
Copy Markdown
Member Author

Combined check before merge: a9e54751 + #1132 (c565be0f) + this PR (e8ba0709) merges cleanly. On that tree (adf2aa7d), npx tsc --noEmit, npm run build and npx jest pass (108 suites, 3452 passed, 1 skipped). Main has since gained #1134 (backend only) and the #1132 squash, which has the same frontend content. GitHub reports this PR MERGEABLE/CLEAN against current main.

@cristim
cristim merged commit ba38120 into main Oct 19, 2026
27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment