Conversation
Fetches banner config from https://jdx.dev/banner.json and renders a dismissible top-of-page announcement. Link scheme is validated to http(s): so a compromised upstream can't inject javascript: URLs. Dismissals persist per banner id in localStorage. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #600 +/- ##
=======================================
Coverage 79.03% 79.03%
=======================================
Files 48 48
Lines 7235 7235
Branches 7235 7235
=======================================
Hits 5718 5718
Misses 1140 1140
Partials 377 377 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Greptile SummaryAdds a dismissible cross-site announcement banner to the VitePress docs theme by fetching config from Confidence Score: 5/5Safe to merge; all remaining findings are P2 style suggestions. The only new finding is a missing docs/.vitepress/theme/banner.ts — localStorage exception handling (prior thread) and rel attribute Important Files Changed
Sequence DiagramsequenceDiagram
participant Browser
participant VitePress as VitePress App
participant Endpoint as jdx.dev/banner.json
participant LS as localStorage
VitePress->>Browser: enhanceApp() → initBanner()
Browser->>Endpoint: fetch(ENDPOINT)
Endpoint-->>Browser: BannerData { id, enabled, message, link }
Browser->>LS: getItem("jdx-banner-dismissed")
alt already dismissed
LS-->>Browser: b.id (match)
Browser-->>Browser: skip render
else not dismissed
Browser->>Browser: render(b) — inject .jdx-banner into body
Browser->>Browser: set --vp-layout-top-height via ResizeObserver
Note over Browser: User clicks ×
Browser->>LS: setItem("jdx-banner-dismissed", b.id)
Browser->>Browser: observer.disconnect(), el.remove()
Browser->>Browser: removeProperty(--vp-layout-top-height)
end
Reviews (8): Last reviewed commit: "docs: keep --vp-layout-top-height in syn..." | Re-trigger Greptile |
There was a problem hiding this comment.
Code Review
This pull request introduces a dynamic site banner system for the VitePress documentation, including remote data fetching, local storage-based dismissal, and responsive styling. The review feedback identifies several improvement opportunities: using sticky positioning and theme-standard z-indices for better layout integration, making the initialization logic idempotent, adding robust error handling for local storage access in restricted environments, and preventing potential race conditions during layout height calculations.
| position: relative; | ||
| z-index: 60; |
There was a problem hiding this comment.
Using position: relative with a high z-index (60) can cause the banner to overlap the navigation bar incorrectly during scrolling. Additionally, z-index: 60 is higher than the VitePress mobile menu backdrop (50), which can lead to visual glitches where the banner appears on top of the menu overlay.
Changing this to position: sticky ensures it stays at the top and integrates correctly with the --vp-layout-top-height offset used by the VitePress navbar. Using the theme's z-index variable ensures it stays below overlays like the mobile menu.
| position: relative; | |
| z-index: 60; | |
| position: sticky; | |
| top: 0; | |
| z-index: var(--vp-z-index-nav, 20); |
| export function initBanner(): void { | ||
| if (typeof window === "undefined") return; |
There was a problem hiding this comment.
| if (!b || !b.enabled) return; | ||
| if (localStorage.getItem(STORAGE_KEY) === b.id) return; |
There was a problem hiding this comment.
Accessing localStorage can throw exceptions in certain browser configurations (e.g., private mode with blocked cookies). It's safer to wrap this in a try-catch block. Additionally, ensuring the banner has an id before checking dismissal prevents potential logic errors if the API response is incomplete.
if (!b || !b.enabled || !b.id) return;
try {
if (localStorage.getItem(STORAGE_KEY) === b.id) return;
} catch {
// Fallback if localStorage is unavailable
}| btn.setAttribute("aria-label", "Dismiss"); | ||
| btn.textContent = "\u00d7"; | ||
| btn.addEventListener("click", () => { | ||
| localStorage.setItem(STORAGE_KEY, b.id); |
| requestAnimationFrame(() => { | ||
| document.documentElement.style.setProperty( | ||
| "--vp-layout-top-height", | ||
| `${el.offsetHeight}px`, | ||
| ); | ||
| }); |
There was a problem hiding this comment.
To prevent potential race conditions where the banner is dismissed before the animation frame fires, check if the element is still in the DOM before updating the layout variable.
requestAnimationFrame(() => {
if (el.isConnected) {
document.documentElement.style.setProperty(
"--vp-layout-top-height",
`${el.offsetHeight}px`,
);
}
});Banner was using position: relative which put it in document flow *and* VitePress applies --vp-layout-top-height offset, causing content to be pushed down twice. Switch to position: fixed so the banner is out of flow and --vp-layout-top-height alone handles the content offset (which is what VitePress's layout-top slot assumes). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Bump z-index to 1001 so the banner sits above custom nav overrides (e.g. hk's .VPNav at z-index: 1000 !important). - Use inherit on the dismiss button so it matches text color. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds a small footer with license, copyright, and link back to en.dev, matching the footer used on the mise docs. Rendered via VitePress's layout-bottom slot. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Center message+link in the banner, position dismiss button absolutely at the right edge so it doesn't skew the centering. - Drop rel=noreferrer on the link so en.dev gets analytics attribution for traffic from the docs. Keep rel=noopener for security. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Left/right padding was asymmetric (1rem vs 2.75rem desktop; 0.75rem vs 2.5rem mobile), which shifted the "centered" text off from true viewport center. Match the sides so justify-content: center lines up with the viewport midpoint. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
no-cache forces a conditional GET on every page load. The server sends Cache-Control: public, max-age=300, must-revalidate, so default browser caching already gives 5-min freshness, which is plenty for an announcement banner. Returning users with a dismissed banner also already short-circuit via localStorage before the fetch runs anyway. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The height CSS variable was set once in a single rAF and never updated afterward. On window resize, orientation change, or text reflow the banner height could change, leaving the VitePress nav offset stale and causing overlap or a visible gap. Observe the banner element and resync the variable whenever its size changes; disconnect the observer when the banner is dismissed. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Summary
docs/.vitepress/theme/banner.ts+banner.css\u2014 fetches banner config fromhttps://jdx.dev/banner.jsonand renders a dismissible announcement bar at the top of the docs.vitepress/theme/index.tsviaenhanceApphttp:/https:so a compromised upstream can't inject ajavascript:URLlocalStorageUsed to announce en.dev, and any future cross-site announcements.
Test plan
jdx-banner-dismissed, reload \u2014 banner returns\U0001F916 Generated with Claude Code
Note
Medium Risk
Moderate risk because it introduces client-side fetching and DOM injection in the docs site (including localStorage persistence and layout offset adjustments), which could affect layout or reliability if the remote endpoint misbehaves.
Overview
Adds a docs-site announcement banner that fetches remote config from
https://jdx.dev/banner.json, renders a fixed, dismissible top bar, and persists dismissal per-banner inlocalStorage(withhttp/httpslink validation).Updates the VitePress theme to initialize the banner via
enhanceApp()and adds a newEndevFootercomponent wired intolayout-bottom.Reviewed by Cursor Bugbot for commit d869013. Bugbot is set up for automated code reviews on this repo. Configure here.