[cristim]

Summary

The Home "Upcoming Purchase Details" dialog put initial focus on Cancel Purchase. openModal (frontend/src/modal.ts) focuses the first focusable control other than the header close-X, and Cancel Purchase comes before Close in the button row. Cancel opens a confirm, so a single keypress could not cancel anything, but the destructive action should not hold the initial focus.

showUpcomingDetails now passes initialFocus: '#close-purchase-details-btn', so focus lands on Close. The button order, the Tab trap, Escape and focus restore are unchanged.

Tests

• jest (dashboard.test.ts): after opening the dialog for a cancelable purchase, document.activeElement is Close. On main it was cancel-purchase-detail-btn.
• Playwright (tests-e2e/dialog-focus.spec.ts): the existing details-dialog test now also checks that Cancel Purchase is shown and Close is focused. Against main's dashboard.ts it fails (1 failed, 2 passed); with the fix the spec passes (3 passed).
• Locally: npm test (4715 passed, 1 skipped), npm run lint (0 errors), npm run build pass.

Closes #1653

🤖 Generated with Claude Code

[coderabbitai]

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 46 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

Learn how review limits work.

Review configuration:

Run configuration

• Source mode: Auto
• Repository: LeanerCloud/cloud-commitments-platform
• Branch: fix/issue-1653-details-focus
• File: .coderabbit.yaml
• Commit: a3992a9
• Layers: Repository YAML
• Review profile: CHILL
• Plan: Advanced
• Run ID: 1f2bd71c-a34b-4fee-8da0-17aca2c04f5a

Commits

Reviewing files that changed from the base of the PR and between b9a050c and a3992a9.

Files selected for processing: 3

• frontend/src/__tests__/dashboard.test.ts
• frontend/src/dashboard.ts
• frontend/tests-e2e/dialog-focus.spec.ts

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

Share

• X
• Mastodon
• Reddit
• LinkedIn

[cristim]

@coderabbitai review

[coderabbitai]

Action not completed

Review rate limited.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[cristim]

Cold review: claude-qa-ui-3, verdict APPROVE at a3992a9

I didn't author this PR. It's a one-line source change (frontend/src/dashboard.ts:692) plus tests.

Verified

• Root cause and fix. On main, openModal has no initialFocus, so resolveInitialFocus (modal.ts:123-136) picks the first focusable that isn't the header X. When the caller can cancel, that's #cancel-purchase-detail-btn, the destructive action (dashboard.ts:766, appended before Close at :779). The PR passes initialFocus: '#close-purchase-details-btn', resolved with el.querySelector inside the dialog, so there's no id collision with other dialogs. When the caller can't cancel, Close is still present, so focus lands on Close either way.
• Enter or Space on open can't trigger Cancel Purchase. A keyboard opener (Enter on View Details, or key auto-repeat) now lands on Close. The worst a stray key press can do is close the dialog. Cancel Purchase also opens a confirm dialog before any DELETE (cancelScheduledPurchase).
• Trap and focus return are unchanged.
  • The keydown handler still wraps Tab and Shift+Tab inside the dialog (modal.ts:161-196).
  • Escape still closes it, and closeModal restores focus to the recorded trigger.
  • The E2E keeps tabStaysInside(page, '#purchase-details-modal', 5) and the focus-restored-to-View-Details assertion after Escape.
• Tests fail on main.
  • With main's dashboard.ts, jest "initial focus lands on Close, not Cancel Purchase" fails (1 failed).
  • The rebuilt dialog-focus.spec.ts fails at toBeFocused() on #close-purchase-details-btn (1 failed, 2 passed).
  • At the head, all 3 pass.
• Local gate at the head (macOS, fresh npm ci):
  • jest: 182 suites, 4715 passed, 1 skipped;
  • lint: 0 errors;
  • build: compiled.
  • Playwright ran with CI=1 under the review and port-4173 locks.

No findings. Not merged.

[cristim]

Cold review verdict: APPROVE at a3992a9

Reviewer: claude-ui-fix-2. I did not author this PR and had no author transcript, and I treated the PR body as a claim.

Local verification (macOS, at a3992a9, under heavy-review.lock with the pw-4173 port lock)

• npm ci && npm test: 182 suites, 4715 passed, 1 skipped
• npm run lint: 0 errors (141 warnings, already on main)
• npm run build: OK
• Playwright dialog-focus.spec.ts plus a11y-focus-return.spec.ts on the production bundle: 6 passed
• The tests fail on main. With main's dashboard.ts:
  • jest initial focus lands on Close fails (1 failed);
  • after a rebuild, Playwright upcoming purchase details dialog takes focus… fails (Close is "inactive"), and the other 2 specs pass.

What I checked

• Initial focus target. initialFocus: '#close-purchase-details-btn' resolves inside the modal (resolveInitialFocus in modal.ts). Close is appended in every variant (dashboard.ts, after the conditional Cancel), so the selector always matches. If it ever didn't, the fallback is the first non-header focusable, which is today's behaviour.
• Enter or Space on open. Focus is on Close, so Enter or Space closes the dialog. The key that opened the dialog has already fired its click on "View Details" and doesn't fire again on the newly focused button. Cancel Purchase still needs a click, and then its own confirm dialog.
• Focus trap and return. The Tab trap, the Escape handler and restoring focus to the opener in openModal/closeModal are unchanged. The existing Playwright case still checks Escape and focus restore, and it passes.
• Scope. Two lines of source; the button order and styles are unchanged.

No findings.