feat(banner): add x for closing (with prehydration)#9009
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
PR SummaryLow Risk Overview The shared To avoid a flash of a dismissed banner before React hydrates, the root Reviewed by Cursor Bugbot for commit f2f736b. Bugbot is set up for automated code reviews on this repo. Configure here. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want fixes drafted automatically? Bugbot Autofix can create code changes for findings. A team admin can enable Autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit dc9e701. Configure here.
| localStorage.getItem(BANNER_DISMISSAL_STORAGE_KEY) === banner.text | ||
| ); | ||
| } | ||
| }, [banner]); |
There was a problem hiding this comment.
Dismissed banner flashes on remount
Medium Severity
After closing the banner in the same visit, client-side navigation that swaps page layouts remounts WithNavBar with dismissed reset to false. Dismissal is reapplied only in a useEffect after paint, and onClose does not set html[data-banner-dismissed], so the pre-hydration CSS hide does not apply and the banner can flash until the effect runs.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit dc9e701. Configure here.
There was a problem hiding this comment.
Pull request overview
Adds a dismiss (“×”) control to the shared Banner UI component and wires the Nodejs.org site banner to persist dismissal via localStorage, including a pre-hydration inline script + CSS to avoid a dismissed-banner flash on first paint.
Changes:
- Add optional dismiss button support to
@node-core/ui-componentsBanner(viaonClose) and update styling/stories. - Add site-level dismissal persistence (
localStorage) and pre-hydration hiding to prevent first-paint flash for dismissed banners. - Introduce a shared storage key constant and global CSS rule to hide the banner when marked dismissed.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/ui-components/src/Common/Banner/index.tsx | Adds optional close button support to the shared Banner component. |
| packages/ui-components/src/Common/Banner/index.module.css | Makes banner container relative and positions the close button. |
| packages/ui-components/src/Common/Banner/index.stories.tsx | Adds a Storybook story for a dismissible banner. |
| packages/ui-components/package.json | Bumps @node-core/ui-components version. |
| apps/site/util/banner.ts | Introduces a shared localStorage key constant for banner dismissal. |
| apps/site/styles/index.css | Adds a global CSS rule to hide dismissible banners when the root is marked dismissed. |
| apps/site/components/withBanner.tsx | Implements dismiss logic + persistence and passes onClose to Banner. |
| apps/site/app/[locale]/layout.tsx | Injects a pre-hydration inline script to set a root data attribute before first paint. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| export type BannerProps = { | ||
| type?: 'default' | 'warning' | 'error'; | ||
| onClose?: MouseEventHandler<HTMLButtonElement>; | ||
| } & HTMLAttributes<HTMLElement>; |
| useEffect(() => { | ||
| if (banner) { | ||
| setDismissed( | ||
| localStorage.getItem(BANNER_DISMISSAL_STORAGE_KEY) === banner.text | ||
| ); |
| const onClose = () => { | ||
| localStorage.setItem(BANNER_DISMISSAL_STORAGE_KEY, banner.text); | ||
| setDismissed(true); | ||
| }; |
📦 Build Size ComparisonSummary
Changes🔄 Modified Routes (4)
|
|
Removed |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #9009 +/- ##
==========================================
- Coverage 74.58% 74.57% -0.02%
==========================================
Files 100 100
Lines 8801 8801
Branches 324 324
==========================================
- Hits 6564 6563 -1
- Misses 2233 2234 +1
Partials 4 4 ☔ View full report in Codecov by Harness. |
|
|
||
| const banner = siteConfig.websiteBanners.index; | ||
|
|
||
| const bannerPreHydrationScript = `try{var s=document.currentScript;if(s&&localStorage.getItem(s.dataset.storageKey)===s.dataset.banner){document.documentElement.dataset.bannerDismissed='true'}}catch{}`; |
Reference PR with prehydration which removes flash from #9008 (comment)
This is not meant to be merged. It should be used as a reference by active maintainers to examine commit dc9e701