fix: cap the re-auth banner width and move its flat CSS into the Paper theme - #2266
Merged
Conversation
…r theme (#2218) `ReAuthBannerBar` set an unconditional `w: 420`. Because the banner is centered with `left: 50%` plus a `translate(-50%, -50%)`, a viewport narrower than 420px made it overflow *both* edges equally — clipping the close button on one side and "Authorize again" on the other, which are the only two controls it has. Losing both is a dead end rather than a cosmetic clip, since dismissing is not equivalent to re-authorizing ("Authorize again" also clears the stale OAuth state). It is reachable on a narrow desktop window too, not only a phone: the element is `fixed`-positioned against the viewport. The width is now capped rather than fixed — `maw={420}` with `w="calc(100vw - 2rem)"` — so the banner shrinks with a 1rem gutter on each side and the shadow and radius still read. The same constant also carried `transform` and `zIndex` as flat CSS in component-level `styles`. Mantine exposes neither as a style prop, but that argues for the next tier in the repo's preference order — a theme variant — not for inline `styles`. Both move to a `reauth` variant in `theme/Paper.ts`, alongside the existing `code`, `contained` and `panel` variants. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LYcBw3Sftq5Yx4WpdzXttb Signed-off-by: cliffhall <cliff@futurescale.com>
There was a problem hiding this comment.
🟢 Approval recommended
The implementation resolves both reported issues and includes focused regression coverage.
Pull request overview
Fixes narrow-viewport overflow in the re-authentication banner and moves its positioning styles into the Paper theme.
Changes:
- Makes the banner responsive while retaining a 420px maximum width.
- Adds a
reauthPaper theme variant for centering and stacking. - Tests responsive width and theme variant behavior.
File summaries
| File | Description |
|---|---|
clients/web/src/theme/Paper.ts |
Adds the reauth Paper variant. |
clients/web/src/components/groups/ReAuthBanner/ReAuthBannerBar.tsx |
Applies responsive sizing and the theme variant. |
clients/web/src/components/groups/ReAuthBanner/ReAuthBannerBar.test.tsx |
Covers width constraints and theme styling. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 0
- Review effort level: Balanced
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
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
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.
Closes #2218
Both findings from the v2.5.0 milestone-merge review (#2215), filed on #2218 rather than fixed in the merge PR.
1. The fixed 420px width overflowed a narrow viewport
ReAuthBannerBarset an unconditionalw: 420. Because the banner is centred byleft: 50%plus atranslate(-50%, -50%), a viewport narrower than 420px made it overflow both edges equally — clipping the close button on one side and the action button on the other, which are the only two controls it has. Losing both is a dead end rather than a cosmetic clip: dismissing is not equivalent to re-authorizing, since "Authorize again" also clears the stale OAuth state that a plain reconnect leaves behind.It is reachable on a narrow desktop window, not only a phone — the element is
fixed-positioned against the viewport, so a browser docked to a third of a wide screen hits it.The width is now capped rather than fixed —
maw={420}withw="calc(100vw - 2rem)"— so the banner shrinks with a 1rem gutter on each side and the shadow and radius still read. Above 420px nothing changes.Before / after, at a 360px viewport
2.
transform/zIndexmoved into aPapertheme variantThe same constant carried flat CSS in component-level
styles. The file's existing comment is correct that Mantine exposes neithertransformnorzIndexas a style prop — but that argues for the next tier in the repo's preference order (props → theme variant → CSS class), not for inlinestyles. Both now live in areauthvariant inclients/web/src/theme/Paper.ts, alongside the existingcode,containedandpanelvariants, and the constant selects it withvariant: "reauth".Not actionable
The third finding on #2215 — an unterminated inline-code span in
clients/web/src/lib/authToken.ts:18— is incorrect. The line carries exactly two backticks, balanced. No change.Tests
ReAuthBannerBar.test.tsxgains two cases beside the existing one: that the width is capped rather than fixed (width: calc(100vw - 2rem),max-widthat 420px scaled), and that the centring offset arrives via the theme variant rather than inlinestyles. Nothing exercised this component below 420px before.npm run local:gatepasses.🤖 Generated with Claude Code
https://claude.ai/code/session_01LYcBw3Sftq5Yx4WpdzXttb