fix: Update dialog portal & banner layout improvements#112
Merged
AmintaCCCP merged 1 commit intomainfrom Apr 26, 2026
Merged
Conversation
- UpdateChecker: wrap the update dialog with ReactDOM.createPortal to render at document.body level, avoiding the fixed overlay being constrained by overflow containers in the Settings scroll area. Also use bg-black/50 syntax and z-[9999] for reliable stacking. Clicking the mask backdrop now dismisses the dialog. - UpdateNotificationBanner: fix duplicate/invalid Tailwind classes (dark:bg-brand-indigo/20/20, duplicate dark:bg-white/[0.04] etc.); add gap-3, min-w-0, flex-shrink-0, flex-wrap, and line-clamp-1 to prevent the version/date row and the "Download" button from wrapping on narrow viewports.
Contributor
📝 WalkthroughWalkthroughThe pull request refactors the update dialog to render via Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/components/UpdateChecker.tsx (3)
110-111: Nit: redundantdark:modifiers.
dark:bg-brand-indigo/20is identical to the basebg-brand-indigo/20, and similarly fordark:text-brand-violeton line 111 — thesedark:overrides are no-ops and can be dropped to keep the class list clean. Same pattern recurs at line 33 ofUpdateNotificationBanner.tsx.♻️ Proposed change
- <div className="w-12 h-12 bg-brand-indigo/20 dark:bg-brand-indigo/20 rounded-full flex items-center justify-center"> - <Package className="w-6 h-6 text-brand-violet dark:text-brand-violet" /> + <div className="w-12 h-12 bg-brand-indigo/20 rounded-full flex items-center justify-center"> + <Package className="w-6 h-6 text-brand-violet" />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/UpdateChecker.tsx` around lines 110 - 111, Remove the redundant dark-mode Tailwind classes that duplicate the base styles: in UpdateChecker.tsx remove "dark:bg-brand-indigo/20" from the div's className and remove "dark:text-brand-violet" from the Package icon's className; also apply the same cleanup in UpdateNotificationBanner.tsx where the identical "dark:" overrides are used (line with the duplicate bg/text classes). This keeps the class lists minimal while preserving styling.
2-2: Optional: prefer namedcreatePortalimport.Other portal consumers in this repo (e.g.,
RepositoryCard.tsx,MarkdownRenderer.tsx) useimport { createPortal } from 'react-dom'. Aligning here improves consistency and lets bundlers tree-shake the rest ofreact-dom.♻️ Proposed change
-import ReactDOM from 'react-dom'; +import { createPortal } from 'react-dom';And at the call site:
- ReactDOM.createPortal( + createPortal(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/UpdateChecker.tsx` at line 2, Replace the default ReactDOM import with a named createPortal import and update the portal call site: change the import line to import { createPortal } from 'react-dom' and replace any ReactDOM.createPortal(...) usage inside the UpdateChecker component (or helper functions in this file) to just createPortal(...), keeping the same props and children so behavior is unchanged; this aligns with other files like RepositoryCard.tsx and allows tree-shaking.
100-167: Consider ESC handling, scroll lock, and basic a11y for the portal dialog.Now that the modal escapes its scroll container and covers the full viewport, a few UX/a11y gaps become more noticeable:
- No
Escapekey dismissal — users typically expect this for modals.- No body scroll lock — the page underneath the backdrop can still scroll while the dialog is open.
- No
role="dialog"/aria-modal="true"/aria-labelledbyon the modal container, and no initial focus management.These are pre-existing, but the portalization makes them worth addressing now.
♻️ Sketch of an effect to handle ESC + scroll lock
const [error, setError] = useState<string | null>(null); + + React.useEffect(() => { + if (!showUpdateDialog) return; + const onKey = (e: KeyboardEvent) => { + if (e.key === 'Escape') setShowUpdateDialog(false); + }; + document.addEventListener('keydown', onKey); + const prevOverflow = document.body.style.overflow; + document.body.style.overflow = 'hidden'; + return () => { + document.removeEventListener('keydown', onKey); + document.body.style.overflow = prevOverflow; + }; + }, [showUpdateDialog]);And on the inner container:
- <div className="bg-white dark:bg-panel-dark rounded-xl shadow-xl max-w-md w-full max-h-[80vh] overflow-y-auto"> + <div + role="dialog" + aria-modal="true" + aria-labelledby="update-dialog-title" + className="bg-white dark:bg-panel-dark rounded-xl shadow-xl max-w-md w-full max-h-[80vh] overflow-y-auto" + >(and add
id="update-dialog-title"to theh3).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/UpdateChecker.tsx` around lines 100 - 167, The modal created when showUpdateDialog && updateInfo is true needs accessibility and UX improvements: add an effect (e.g., inside UpdateChecker component) that on open sets document.body.style.overflow = 'hidden' and restores it on close to lock scrolling, attaches a keydown listener to call setShowUpdateDialog(false) when Escape is pressed (cleanup on unmount), and manage initial focus by focusing the modal container or the primary button (handleDownload) when opened; also add role="dialog" aria-modal="true" and aria-labelledby referencing an id added to the h3 (e.g., id="update-dialog-title") on the inner dialog div to provide proper labelling. Ensure all listeners and body style changes are cleaned up when showUpdateDialog becomes false or the component unmounts.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/components/UpdateChecker.tsx`:
- Around line 110-111: Remove the redundant dark-mode Tailwind classes that
duplicate the base styles: in UpdateChecker.tsx remove "dark:bg-brand-indigo/20"
from the div's className and remove "dark:text-brand-violet" from the Package
icon's className; also apply the same cleanup in UpdateNotificationBanner.tsx
where the identical "dark:" overrides are used (line with the duplicate bg/text
classes). This keeps the class lists minimal while preserving styling.
- Line 2: Replace the default ReactDOM import with a named createPortal import
and update the portal call site: change the import line to import { createPortal
} from 'react-dom' and replace any ReactDOM.createPortal(...) usage inside the
UpdateChecker component (or helper functions in this file) to just
createPortal(...), keeping the same props and children so behavior is unchanged;
this aligns with other files like RepositoryCard.tsx and allows tree-shaking.
- Around line 100-167: The modal created when showUpdateDialog && updateInfo is
true needs accessibility and UX improvements: add an effect (e.g., inside
UpdateChecker component) that on open sets document.body.style.overflow =
'hidden' and restores it on close to lock scrolling, attaches a keydown listener
to call setShowUpdateDialog(false) when Escape is pressed (cleanup on unmount),
and manage initial focus by focusing the modal container or the primary button
(handleDownload) when opened; also add role="dialog" aria-modal="true" and
aria-labelledby referencing an id added to the h3 (e.g.,
id="update-dialog-title") on the inner dialog div to provide proper labelling.
Ensure all listeners and body style changes are cleaned up when showUpdateDialog
becomes false or the component unmounts.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7925cc2e-654e-47b5-a8de-3cbc88e874f4
📒 Files selected for processing (2)
src/components/UpdateChecker.tsxsrc/components/UpdateNotificationBanner.tsx
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
1 participant
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.
Summary
Two fixes based on the screenshot report:
Issue 1: Update dialog mask trapped inside overflow container
UpdateCheckeris rendered insideGeneralPanel→SettingsPanel→ scrollable content area. Thefixed inset-0overlay was constrained by the nearest overflow container, so the dark mask did not cover the full viewport.Fix: Wrap the dialog with
ReactDOM.createPortalto render atdocument.bodylevel. Also addedbg-black/50shorthand,z-[9999]for reliable z-index stacking, and click-on-mask-to-dismiss behavior.Issue 2: Banner button wrapping on narrow viewports
The "立即下载" button and close button in
UpdateNotificationBannercould wrap to a new line when the viewport was narrow. Also had invalid/duplicate Tailwind classes (dark:bg-brand-indigo/20/20with two opacity values, duplicatedark:bg-white/[0.04]).Fix: Removed invalid duplicate classes, added
gap-3,min-w-0,flex-shrink-0,flex-wrap, andline-clamp-1to the banner flex layout so buttons stay on one line and text truncates gracefully.Test plan
Summary by CodeRabbit
Style