Conversation
Fetches banner config from https://jdx.dev/banner.json and renders a dismissible top-of-page announcement. Dismissals persist per banner id in localStorage, so bumping the id re-shows it. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
Greptile SummaryThis PR adds a dismissible cross-site announcement banner that fetches config from Confidence Score: 4/5Safe to merge after addressing two open findings from the prior review round still visible in the diff. Good progress on the prior review — protocol injection and ResizeObserver concerns are resolved. Two P1 findings from earlier rounds (missing noreferrer, unguarded localStorage.setItem in the click handler) are still present in the code. No new P1/P0 issues identified in this pass. docs/.vitepress/theme/banner.ts — the dismiss click handler and the anchor rel attribute. Important Files Changed
Sequence DiagramsequenceDiagram
participant Browser
participant VitePress as VitePress (enhanceApp)
participant BannerTS as banner.ts
participant Endpoint as jdx.dev/banner.json
participant LocalStorage
Browser->>VitePress: Page load / hydration
VitePress->>BannerTS: initBanner()
BannerTS->>Endpoint: fetch(ENDPOINT)
Endpoint-->>BannerTS: BannerData { id, enabled, message, link }
BannerTS->>LocalStorage: getItem("jdx-banner-dismissed")
LocalStorage-->>BannerTS: stored id or null
alt not dismissed & enabled
BannerTS->>Browser: render(b) → prepend .jdx-banner to body
BannerTS->>Browser: requestAnimationFrame(syncHeight)
BannerTS->>Browser: ResizeObserver.observe(el)
Browser-->>BannerTS: User clicks ×
BannerTS->>LocalStorage: setItem("jdx-banner-dismissed", b.id)
BannerTS->>Browser: el.remove() + removeProperty(--vp-layout-top-height)
else dismissed or disabled
BannerTS-->>Browser: no-op
end
Reviews (9): Last reviewed commit: "docs: keep --vp-layout-top-height in syn..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Only allow http(s): links from the upstream JSON. Defense-in-depth against a compromised jdx.dev injecting a javascript: URL that would execute arbitrary code in the docs origin on click. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a dynamic announcement banner system for the documentation site. It includes a new CSS file for styling, a TypeScript module to fetch banner data from a remote endpoint and manage its display, and integration into the VitePress theme initialization. The banner supports a dismissal feature that persists via localStorage. The review feedback suggests adding error handling around localStorage access to prevent potential runtime errors in environments where storage is blocked or full.
| .then((r) => (r.ok ? (r.json() as Promise<BannerData>) : null)) | ||
| .then((b) => { | ||
| if (!b || !b.enabled) return; | ||
| if (localStorage.getItem(STORAGE_KEY) === b.id) return; |
There was a problem hiding this comment.
Accessing localStorage can throw an error in some environments (e.g., if cookies/storage are blocked or in certain private browsing modes). It is safer to wrap this check in a try-catch block to ensure the banner logic doesn't crash the application initialization.
try {
if (localStorage.getItem(STORAGE_KEY) === b.id) return
} catch (e) {
// ignore storage errors
}| a.href = b.link; | ||
| a.target = "_blank"; | ||
| a.rel = "noopener noreferrer"; | ||
| a.textContent = b.linkText || "Learn more"; |
There was a problem hiding this comment.
Similar to the retrieval, setting an item in localStorage can throw an error if storage is disabled or full. Wrapping this in a try-catch ensures the dismissal functionality fails gracefully without impacting the rest of the UI.
try {
localStorage.setItem(STORAGE_KEY, b.id)
} catch (e) {
// ignore storage errors
}![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
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) <[email protected]>
- Bump z-index to 1001 so the banner sits above custom nav overrides (e.g. hk's .VPNav at z-index: 1000 !important). - Use dark text on light brand backgrounds (coral/pink/gold/cyan) for readable contrast. Sites with dark brand backgrounds keep white text. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
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) <[email protected]>
- 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) <[email protected]>
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) <[email protected]>
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) <[email protected]>
The previous fallback (#3451b2, VitePress default dark blue) made black banner text unreadable if --vp-c-brand-1 ever fails to resolve. Use a fallback that matches this site's actual brand color so black text stays readable in the fallback scenario. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
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) <[email protected]>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 622e8db. Configure here.
| observer?.disconnect(); | ||
| el.remove(); | ||
| document.documentElement.style.removeProperty("--vp-layout-top-height"); | ||
| }); |
There was a problem hiding this comment.
Unhandled localStorage.setItem exception blocks banner dismissal
Low Severity
The dismiss button's click handler calls localStorage.setItem as its first statement. If this throws (e.g., QuotaExceededError on a full storage, or storage disabled in certain browsing contexts), the remaining cleanup — observer?.disconnect(), el.remove(), and removeProperty("--vp-layout-top-height") — never executes. This leaves the banner permanently visible and undismissable for the session, since every click re-throws. Wrapping the setItem in a try-catch (or moving it after el.remove()) would let dismissal still work even when persistence fails.
Reviewed by Cursor Bugbot for commit 622e8db. Configure here.
### 🐛 Bug Fixes - **(git)** skip untracked scan when HK_STASH_UNTRACKED=false by [@jdx](https://github.com/jdx) in [#861](#861) - **(run)** add post-commit and pre-rebase subcommands by [@jdx](https://github.com/jdx) in [#858](#858) ### 📚 Documentation - **(install)** recommend global hooks as primary setup path by [@jdx](https://github.com/jdx) in [#855](#855) - add cross-site announcement banner by [@jdx](https://github.com/jdx) in [#857](#857) - respect banner expires field by [@jdx](https://github.com/jdx) in [#862](#862) ### 🔍 Other Changes - vendor bats test helpers instead of git submodules by [@jdx](https://github.com/jdx) in [#859](#859) ### 📦️ Dependency Updates - bump communique to 1.0.3 by [@jdx](https://github.com/jdx) in [#863](#863) - update anthropics/claude-code-action digest to e58dfa5 by [@renovate[bot]](https://github.com/renovate[bot]) in [#864](#864) <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Low Risk** > Low risk release bookkeeping only: version numbers, generated CLI/docs, and lockfile dependency bumps with no runtime logic changes. > > **Overview** > **Release prep for `v1.44.1`.** Updates `Cargo.toml`/`Cargo.lock` (including `libc`) and all version references in generated CLI docs (`docs/cli/*`, `hk.usage.kdl`). > > Refreshes documentation examples to reference the new `v1.44.1` Pkl package URLs and adds the `1.44.1` entry to `CHANGELOG.md`. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit d7637d4. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY --> Co-authored-by: mise-en-dev <[email protected]>
1 participant
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 docsenhanceApphooklocalStorage; bumping the id re-shows itUsed 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
Adds client-side DOM injection that fetches remote banner content and persists dismissal in
localStorage, which can affect layout and introduces a new external dependency for docs pages.Overview
Introduces a cross-site, dismissible announcement banner for the docs theme:
initBanner()fetches config fromhttps://jdx.dev/banner.json, renders a fixed top bar with optional outbound link, adjusts--vp-layout-top-height, and stores per-banner dismissals inlocalStorage.Adds a new
EndevFootercomponent and wires it intoLayout.vuevia thelayout-bottomslot to display license/copyright and anen.devlink.Reviewed by Cursor Bugbot for commit 622e8db. Bugbot is set up for automated code reviews on this repo. Configure here.