Add dismissible top announcement banner - #48
Merged
Conversation
Introduces a new `AnnouncementBanner` component rendered above the app shell with a message, optional external action link, and dismiss button. Dismissal is persisted per announcement ID in `localStorage`, so re-showing later only requires changing the ID. `AppShell` was updated to a column layout and now mounts the banner above the existing sidebar/canvas structure.
✅ Deploy Preview for overup-app ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughAdds a configurable, dismissible announcement banner with localStorage persistence and accessible controls, then renders it above the main application shell content. ChangesAnnouncement banner
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant AppShell
participant AnnouncementBanner
participant localStorage
participant User
AppShell->>AnnouncementBanner: Render banner
AnnouncementBanner->>localStorage: Read dismissal flag
localStorage-->>AnnouncementBanner: Return stored state
User->>AnnouncementBanner: Click dismiss
AnnouncementBanner->>localStorage: Persist dismissal flag
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![amazon-q-developer[bot]](https://avatars.githubusercontent.com/in/1220912?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary
This PR introduces a dismissible announcement banner component with localStorage persistence. The implementation is well-structured with good accessibility practices, but there are 2 critical issues that must be fixed before merging:
Critical Issues
- Logic Error (Line 35): The banner is initialized as dismissed, preventing it from appearing on first load for new users
- Security Vulnerability (Lines 72-82): Missing URL validation creates an XSS risk if user-controlled data is passed to
actionHref
Overall Assessment
The component design is solid with proper ARIA attributes, responsive styling, and error handling for localStorage. Once the initialization logic and URL validation are fixed, this will be ready to merge.
You can now have the agent implement changes and create commits directly on your pull request's source branch. Simply comment with /q followed by your request in natural language to ask the agent to make changes.
| actionHref = 'https://github.com/BotCoder254/overup', | ||
| }: AnnouncementBannerProps) { | ||
| const storageKey = `${STORAGE_PREFIX}${id}`; | ||
| const [dismissed, setDismissed] = useState(true); |
There was a problem hiding this comment.
🛑 Logic Error: Initialize dismissed state to false instead of true. The current implementation causes the banner to never appear on first render because the useEffect runs after the initial render, so users with no prior dismissal never see the announcement. The useEffect will correctly set it to dismissed if needed after checking localStorage.
Suggested change
| const [dismissed, setDismissed] = useState(true); | |
| const [dismissed, setDismissed] = useState(false); |
Comment on lines
+72
to
+82
| {actionLabel && actionHref && ( | ||
| <a | ||
| href={actionHref} | ||
| target="_blank" | ||
| rel="noopener noreferrer" | ||
| className="hidden shrink-0 items-center gap-1 rounded bg-white px-3 py-1.5 text-xs font-semibold text-primary transition-colors hover:bg-white/90 focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-white focus-visible:ring-offset-2 focus-visible:ring-offset-primary sm:inline-flex" | ||
| > | ||
| {actionLabel} | ||
| <ArrowUpRight size={14} aria-hidden="true" /> | ||
| </a> | ||
| )} |
There was a problem hiding this comment.
🛑 Security Vulnerability: Validate the actionHref URL before rendering the anchor tag to prevent XSS attacks through javascript: protocol URIs. An attacker could inject malicious JavaScript if this prop accepts user-controlled input.1
Suggested change
| {actionLabel && actionHref && ( | |
| <a | |
| href={actionHref} | |
| target="_blank" | |
| rel="noopener noreferrer" | |
| className="hidden shrink-0 items-center gap-1 rounded bg-white px-3 py-1.5 text-xs font-semibold text-primary transition-colors hover:bg-white/90 focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-white focus-visible:ring-offset-2 focus-visible:ring-offset-primary sm:inline-flex" | |
| > | |
| {actionLabel} | |
| <ArrowUpRight size={14} aria-hidden="true" /> | |
| </a> | |
| )} | |
| {actionLabel && actionHref && (() => { | |
| try { | |
| const url = new URL(actionHref, window.location.origin); | |
| if (!['http:', 'https:'].includes(url.protocol)) return null; | |
| } catch { | |
| return null; | |
| } | |
| return ( | |
| <a | |
| href={actionHref} | |
| target="_blank" | |
| rel="noopener noreferrer" | |
| className="hidden shrink-0 items-center gap-1 rounded bg-white px-3 py-1.5 text-xs font-semibold text-primary transition-colors hover:bg-white/90 focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-white focus-visible:ring-offset-2 focus-visible:ring-offset-primary sm:inline-flex" | |
| > | |
| {actionLabel} | |
| <ArrowUpRight size={14} aria-hidden="true" /> | |
| </a> | |
| ); | |
| })()} |
Footnotes
-
CWE-79: Improper Neutralization of Input During Web Page Generation ('Cross-site Scripting') - https://cwe.mitre.org/data/definitions/79.html ↩
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/layout/AnnouncementBanner.tsx`:
- Around line 58-94: Update every rounded class in the AnnouncementBanner JSX,
including the icon container, action link, and dismiss button, from rounded to
rounded-md so all border radii follow the 6px guideline.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 107f8d1d-3140-40ba-8fc7-6782f7cebb86
📒 Files selected for processing (2)
src/components/layout/AnnouncementBanner.tsxsrc/components/layout/AppShell.tsx
Comment on lines
+58
to
+94
| return ( | ||
| <div role="status" aria-live="polite" className="shrink-0 bg-primary text-white"> | ||
| <div className="mx-auto flex items-center gap-2.5 px-3 py-2 sm:gap-3 sm:px-5"> | ||
| <span | ||
| aria-hidden="true" | ||
| className="hidden shrink-0 rounded bg-white/15 p-1.5 sm:inline-flex" | ||
| > | ||
| <Megaphone size={16} /> | ||
| </span> | ||
|
|
||
| <p className="min-w-0 flex-1 text-[13px] leading-snug line-clamp-2 sm:line-clamp-none sm:text-sm"> | ||
| {message} | ||
| </p> | ||
|
|
||
| {actionLabel && actionHref && ( | ||
| <a | ||
| href={actionHref} | ||
| target="_blank" | ||
| rel="noopener noreferrer" | ||
| className="hidden shrink-0 items-center gap-1 rounded bg-white px-3 py-1.5 text-xs font-semibold text-primary transition-colors hover:bg-white/90 focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-white focus-visible:ring-offset-2 focus-visible:ring-offset-primary sm:inline-flex" | ||
| > | ||
| {actionLabel} | ||
| <ArrowUpRight size={14} aria-hidden="true" /> | ||
| </a> | ||
| )} | ||
|
|
||
| <button | ||
| type="button" | ||
| onClick={close} | ||
| aria-label="Dismiss announcement" | ||
| className="shrink-0 rounded p-1.5 text-white/80 transition-colors hover:bg-white/15 hover:text-white focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-white focus-visible:ring-offset-2 focus-visible:ring-offset-primary" | ||
| > | ||
| <X size={18} aria-hidden="true" /> | ||
| </button> | ||
| </div> | ||
| </div> | ||
| ); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Ensure border radius adheres to the 6px guideline.
The rounded class in Tailwind CSS typically applies a 4px border radius. As per coding guidelines, the React frontend should "keep border radius at 6px". Please update the rounded classes to rounded-md (which defaults to 6px) or rounded-[6px].
🎨 Proposed fixes for border radius
- className="hidden shrink-0 rounded bg-white/15 p-1.5 sm:inline-flex"
+ className="hidden shrink-0 rounded-md bg-white/15 p-1.5 sm:inline-flex"- className="hidden shrink-0 items-center gap-1 rounded bg-white px-3 py-1.5 text-xs font-semibold text-primary transition-colors hover:bg-white/90 focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-white focus-visible:ring-offset-2 focus-visible:ring-offset-primary sm:inline-flex"
+ className="hidden shrink-0 items-center gap-1 rounded-md bg-white px-3 py-1.5 text-xs font-semibold text-primary transition-colors hover:bg-white/90 focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-white focus-visible:ring-offset-2 focus-visible:ring-offset-primary sm:inline-flex"- className="shrink-0 rounded p-1.5 text-white/80 transition-colors hover:bg-white/15 hover:text-white focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-white focus-visible:ring-offset-2 focus-visible:ring-offset-primary"
+ className="shrink-0 rounded-md p-1.5 text-white/80 transition-colors hover:bg-white/15 hover:text-white focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-white focus-visible:ring-offset-2 focus-visible:ring-offset-primary"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| return ( | |
| <div role="status" aria-live="polite" className="shrink-0 bg-primary text-white"> | |
| <div className="mx-auto flex items-center gap-2.5 px-3 py-2 sm:gap-3 sm:px-5"> | |
| <span | |
| aria-hidden="true" | |
| className="hidden shrink-0 rounded bg-white/15 p-1.5 sm:inline-flex" | |
| > | |
| <Megaphone size={16} /> | |
| </span> | |
| <p className="min-w-0 flex-1 text-[13px] leading-snug line-clamp-2 sm:line-clamp-none sm:text-sm"> | |
| {message} | |
| </p> | |
| {actionLabel && actionHref && ( | |
| <a | |
| href={actionHref} | |
| target="_blank" | |
| rel="noopener noreferrer" | |
| className="hidden shrink-0 items-center gap-1 rounded bg-white px-3 py-1.5 text-xs font-semibold text-primary transition-colors hover:bg-white/90 focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-white focus-visible:ring-offset-2 focus-visible:ring-offset-primary sm:inline-flex" | |
| > | |
| {actionLabel} | |
| <ArrowUpRight size={14} aria-hidden="true" /> | |
| </a> | |
| )} | |
| <button | |
| type="button" | |
| onClick={close} | |
| aria-label="Dismiss announcement" | |
| className="shrink-0 rounded p-1.5 text-white/80 transition-colors hover:bg-white/15 hover:text-white focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-white focus-visible:ring-offset-2 focus-visible:ring-offset-primary" | |
| > | |
| <X size={18} aria-hidden="true" /> | |
| </button> | |
| </div> | |
| </div> | |
| ); | |
| return ( | |
| <div role="status" aria-live="polite" className="shrink-0 bg-primary text-white"> | |
| <div className="mx-auto flex items-center gap-2.5 px-3 py-2 sm:gap-3 sm:px-5"> | |
| <span | |
| aria-hidden="true" | |
| className="hidden shrink-0 rounded-md bg-white/15 p-1.5 sm:inline-flex" | |
| > | |
| <Megaphone size={16} /> | |
| </span> | |
| <p className="min-w-0 flex-1 text-[13px] leading-snug line-clamp-2 sm:line-clamp-none sm:text-sm"> | |
| {message} | |
| </p> | |
| {actionLabel && actionHref && ( | |
| <a | |
| href={actionHref} | |
| target="_blank" | |
| rel="noopener noreferrer" | |
| className="hidden shrink-0 items-center gap-1 rounded-md bg-white px-3 py-1.5 text-xs font-semibold text-primary transition-colors hover:bg-white/90 focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-white focus-visible:ring-offset-2 focus-visible:ring-offset-primary sm:inline-flex" | |
| > | |
| {actionLabel} | |
| <ArrowUpRight size={14} aria-hidden="true" /> | |
| </a> | |
| )} | |
| <button | |
| type="button" | |
| onClick={close} | |
| aria-label="Dismiss announcement" | |
| className="shrink-0 rounded-md p-1.5 text-white/80 transition-colors hover:bg-white/15 hover:text-white focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-white focus-visible:ring-offset-2 focus-visible:ring-offset-primary" | |
| > | |
| <X size={18} aria-hidden="true" /> | |
| </button> | |
| </div> | |
| </div> | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/layout/AnnouncementBanner.tsx` around lines 58 - 94, Update
every rounded class in the AnnouncementBanner JSX, including the icon container,
action link, and dismiss button, from rounded to rounded-md so all border radii
follow the 6px guideline.
Source: Coding guidelines
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.
Introduces a new
AnnouncementBannercomponent rendered above the app shell with a message, optional external action link, and dismiss button. Dismissal is persisted per announcement ID inlocalStorage, so re-showing later only requires changing the ID.AppShellwas updated to a column layout and now mounts the banner above the existing sidebar/canvas structure.Summary by CodeRabbit