fix(web): align MFA enrollment banner with Pencil design#143
Conversation
The MFA banner had no grid-area assignment in the CSS Grid layout, causing it to be auto-placed into the 180px sidebar column. This made text wrap word-by-word and the banner overlap the footer. Adds a dedicated `banner` grid row, adds shield-alert icon, and aligns all spacing/typography to the Pencil design specification. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughAdds a dedicated banner row to the app shell grid layout and restyled the MFA enrollment prompt component to display as a banner with icon, updated text formatting, and adjusted z-index positioning. Includes planning documentation for the MFA banner-footer overlap fix. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The banner's z-index: 2 created a stacking context that intercepted pointer events on the user menu dropdown, causing the logout button click to fail in e2e tests. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
apps/web/src/components/mfa/MfaEnrollmentPrompt.tsx (1)
100-100: Hardcodedstrokecolor — prefercurrentColor+ CSS variable.
stroke="#F59E0B"duplicates the amber warning color that the rest of the MFA prompt already references viavar(--color-warning)(e.g.,.mfa-prompt-text strong). If the design token changes, the icon colour won't update.🎨 Proposed refactor
In
MfaEnrollmentPrompt.tsx:- stroke="#F59E0B" + stroke="currentColor"In
App.css.mfa-prompt-icon:.mfa-prompt-icon { flex-shrink: 0; + color: var(--color-warning); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/mfa/MfaEnrollmentPrompt.tsx` at line 100, The SVG in MfaEnrollmentPrompt.tsx currently hardcodes stroke="#F59E0B"; change the SVG to use stroke="currentColor" (or remove the explicit stroke so it inherits) and update the component's styling by applying the .mfa-prompt-icon class that sets color: var(--color-warning) so the icon color follows the design token; locate the SVG in MfaEnrollmentPrompt.tsx and the CSS rule to add or update .mfa-prompt-icon { color: var(--color-warning); } so the icon inherits the warning color.apps/web/src/App.css (1)
2118-2123:font-weight: 500bypasses the design token system.All other font-weight values in this file use
var(--font-weight-*)variables. If--font-weight-mediumdoesn't exist yet, this is a good opportunity to introduce the token so future theme changes propagate automatically.🎨 Proposed refactor
Add the token to your CSS variables/design tokens file:
--font-weight-medium: 500;Then in
App.css:-.mfa-prompt-btn-setup { - font-weight: 500; +.mfa-prompt-btn-setup { + font-weight: var(--font-weight-medium);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/App.css` around lines 2118 - 2123, Replace the hardcoded font-weight in the .mfa-prompt-btn-setup rule with a design token: add a new CSS variable --font-weight-medium (value 500) to your design tokens/variables file, then change font-weight: 500 to font-weight: var(--font-weight-medium) inside the .mfa-prompt-btn-setup rule so it follows the existing token system.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/web/src/components/mfa/MfaEnrollmentPrompt.tsx`:
- Line 92: The MFA prompt currently uses role="banner" on the div with className
"mfa-prompt" inside the MfaEnrollmentPrompt component which creates a duplicate
landmark; change that role to role="status" and add aria-live="polite" (and keep
any existing accessible labeling like aria-label or visually-hidden text) so the
prompt becomes a non-critical, politely announced status region instead of a
second banner.
---
Nitpick comments:
In `@apps/web/src/App.css`:
- Around line 2118-2123: Replace the hardcoded font-weight in the
.mfa-prompt-btn-setup rule with a design token: add a new CSS variable
--font-weight-medium (value 500) to your design tokens/variables file, then
change font-weight: 500 to font-weight: var(--font-weight-medium) inside the
.mfa-prompt-btn-setup rule so it follows the existing token system.
In `@apps/web/src/components/mfa/MfaEnrollmentPrompt.tsx`:
- Line 100: The SVG in MfaEnrollmentPrompt.tsx currently hardcodes
stroke="#F59E0B"; change the SVG to use stroke="currentColor" (or remove the
explicit stroke so it inherits) and update the component's styling by applying
the .mfa-prompt-icon class that sets color: var(--color-warning) so the icon
color follows the design token; locate the SVG in MfaEnrollmentPrompt.tsx and
the CSS rule to add or update .mfa-prompt-icon { color: var(--color-warning); }
so the icon inherits the warning color.
…rompt The app header already carries the implicit banner ARIA landmark. Use role="status" with aria-live="polite" instead for the MFA prompt, which is semantically correct for a non-critical notification. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Verify and close two active debug sessions after confirming all issues are fixed on main with commit/PR cross-references. corekit-auth-uat: 4 issues resolved (ISSUE-001 tab crash, ISSUE-003 JWKS persistence, ISSUE-004 SecurityTab wiring, ISSUE-002 documented). Added post-session follow-up fixes (#205, #210, #213) and E2E coverage mapping for 19 previously-skipped UAT test cases. mfa-banner-footer-overlap: grid-area fix (#143) and z-index follow-up verified on main with file/line references. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: b1ae6e4f2098
Verify and close two active debug sessions after confirming all issues are fixed on main with commit/PR cross-references. corekit-auth-uat: 4 issues resolved (ISSUE-001 tab crash, ISSUE-003 JWKS persistence, ISSUE-004 SecurityTab wiring, ISSUE-002 documented). Added post-session follow-up fixes (#205, #210, #213) and E2E coverage mapping for 19 previously-skipped UAT test cases. mfa-banner-footer-overlap: grid-area fix (#143) and z-index follow-up verified on main with file/line references. Entire-Checkpoint: b1ae6e4f2098 Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
Summary
grid-areaassignment, causing it to be auto-placed into the 180px sidebar column — resulting in word-by-word text wrapping and footer overlapbannergrid row between header and sidebar/main in the CSS Grid template (desktop + mobile)[!]vs//), fixed font size (11px), button padding (6px 16px), font weight (500), and text layout (flex with 4px gap)Test plan
[!] secure your account, and description--setup-mfabutton has correct padding and navigates to Settings > Security[x]dismiss works and banner does not reappear in the same session🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
Bug Fixes
Style