Skip to content

Review follow-up: upstream Nimbus scaffold findings from PR #106 (docs/src) #107

Description

@amondnet

Original review suggestions

cubic-dev-ai (full-PR pass) and gemini-code-assist raised the findings below on PR #106. Every one of them is in a scaffold-owned Nimbus starter file written by @cloudflare/create-nimbus-docs@0.7.1 / @cloudflare/nimbus-docs@0.12.0, not in content or configuration authored in this repository.

Reviewer: cubic-dev-ai, gemini-code-assist
PR: #106

File Priority Reviewer Finding Thread
docs/src/components/ui/collapsible/CollapsibleContent.astro:17 P1 cubic-dev-ai A sidebar group that renders closed on the server gets inert set from the render-time state, but the disclosure click handler only toggles data-nb-state — it never removes inert. After the user expands the group… thread
docs/src/components/ui/breadcrumbs/Breadcrumbs.astro:22 P2 cubic-dev-ai When a caller sets maxVisible below four, the fixed two-item head and tail overlap and duplicate breadcrumbs. Derive the head and tail counts from a clamped visible limit, or reject values below the four-item layout… thread
docs/src/components/ui/theme-toggle/ThemeToggle.astro:24 P2 cubic-dev-ai The theme control changes a persistent binary state, but its dynamically updated data-nb-state is not exposed to assistive technology. Add and update aria-pressed alongside the mode state so screen readers can rep… thread
docs/src/components/ui/theme-toggle/ThemeToggle.astro:16 P2 cubic-dev-ai When ThemeToggle is rendered inside a form, clicking it submits the form because the native button defaults to submit. Set its type to button so the theme control never triggers form submission. thread
docs/src/components/ui/banner/Banner.astro:46 P2 cubic-dev-ai When a banner contains an unquoted or entity-encoded javascript: URL, sanitizeBannerHtml leaves it intact and set:html renders executable markup. Replace the regex filtering with a real HTML allowlist/sanitizer,… thread
docs/src/components/ui/package-managers/package-managers.client.ts:32 P2 cubic-dev-ai When the Clipboard API rejects because of permissions or an unsupported context, this handler returns without feedback, so users cannot tell that the command was not copied. Handle an error state like `page-actions.cl… thread
docs/src/components/ui/tabs/TabsTrigger.astro:18 P2 cubic-dev-ai The exported manual TabsTrigger/TabsContent API wires none of the ARIA tab association. The trigger has no id, aria-controls, or initial aria-selected, and the panel has no id/aria-labelledby, so the tab and… thread
docs/src/components/ui/search/SearchDialog.astro:38 P2 cubic-dev-ai The search UI never announces status changes to screen readers. The empty-state <p data-search-empty> sits inside role="listbox" (an ARIA listbox only permits option/group descendants), and the dynamic status … thread
docs/src/components/ui/steps/Step.astro:21 P2 cubic-dev-ai Each renders as a plain
; the step number is drawn only by the CSS ::before counter in Steps.astro. Steps.client.ts explicitly restores role="list" on the markdown
    for Safari/screen readers,…
thread
docs/src/components/Render.astro:107 P2 cubic-dev-ai When a partial declares a components parameter, this spread overwrites the MDX component registry, so registered components in that partial become unavailable. Spread params before components, or reject `compone… thread
docs/src/components/ui/page-actions/page-actions.client.ts:39 P2 cubic-dev-ai When the Markdown request takes time, browsers that require transient user activation can reject this clipboard call because it runs after the asynchronous fetch. Prefetch the Markdown before the click, or invoke `wri… thread
docs/src/components/ui/page-actions/page-actions.client.ts:33 P2 cubic-dev-ai During docs:dev, this request goes to the configured production hostname rather than the local Markdown endpoint, so Copy page fails while the domain is unavailable. Pass a relative Markdown path to the client or re… thread
docs/src/components/ui/toc/MobileTOC.astro:42 P2 cubic-dev-ai When a heading has an empty slug, this renders a selectable entry that cannot navigate. Omit empty-slug headings from the mobile options so every option resolves to a target. thread
docs/src/components/ui/theme-toggle/theme-toggle.client.ts:30 P2 cubic-dev-ai When localStorage.setItem throws (Safari private mode, third-party/restricted storage contexts — the exact case the catch comment targets), the catch swallows the error and __nbApplyTheme() then re-reads the uncha… thread
docs/src/components/ui/search/search.client.ts:32 P2 cubic-dev-ai When a result is highlighted, pressing Enter on the dialog's Esc button selects the result instead of closing the dialog. handleKeydown is attached to the whole dialog; scope it to input so controls and focused re… thread
docs/src/components/ui/steps/Steps.astro:70 P2 cubic-dev-ai When Steps wraps a tight Markdown list containing inline markup, this selector turns each inline element into its own grid item, splitting text such as Run <code>...</code> across rows. Keep inline content in one … thread
docs/src/styles/prose.css:19 P2 cubic-dev-ai Because prose.css follows globals.css, this direct-child rule overrides component margin utilities on classed children. Aside, Card, and Banner can lose their declared top spacing; make this selector zero-sp… thread
docs/src/styles/prose.css:60 P2 cubic-dev-ai Because prose.css follows globals.css, this component-boundary rule overrides typography utilities on classed headings and paragraphs. Card and Aside titles therefore inherit the article's 0.9375rem size instead… thread
docs/src/components/ui/tabs/Tabs.astro:28 P2 cubic-dev-ai Manual tab composition renders an extra empty tablist, so the client binds the indicator and tab-list behavior to that empty list instead of the consumer's TabsList. Add an explicit automatic/manual mode and omit th… thread
docs/src/components/ui/sidebar/sidebar.client.ts:76 P2 cubic-dev-ai When a user closes a group while a filter is active, clearing the filter reopens it because resetFilter unconditionally toggles every group marked as opened by the filter. Only click the trigger when it is still ope… thread
docs/src/components/ui/dialog/dialog.client.ts:11 P2 cubic-dev-ai On every close, unlockScroll() is invoked twice: once from the MutationObserver sync() when the open attribute is removed, and once from the close event onClose. lockScroll() is only ever called once (on o… thread
docs/src/components/ui/aside/Aside.astro:27 P3 cubic-dev-ai When title is omitted, aria-label={displayTitle} defaults to the type's label (e.g. "Note") while the same string is rendered as the visible <p> title inside the aside. Screen-reader users hear the accessible na… thread
docs/src/components/ui/code/Code.astro:1 P3 cubic-dev-ai Nothing imports or registers this Code component, so it is never rendered and its Nimbus themes and transformers do not affect documentation code blocks. Register and use it in the MDX path where these behaviors are… thread
docs/src/components/ui/banner/Banner.astro:47 CRITICAL gemini-code-assist Insecure HTML sanitization using regular expressions in Banner component Problem: The sanitizeBannerHtml function uses regular expressions to sanitize HTML, which is highly prone to bypasses and can lead to Cross-Si… thread

Status

Decision: Declined (for this PR)
Reason: docs/src/components/**, docs/src/layouts/**, docs/src/styles/**, and docs/src/components/Render.astro are the upstream Nimbus template, kept byte-aligned with it so nimbus-docs outdated / nimbus-docs diff --apply can pull upstream fixes cleanly (CLAUDE.md "Docs site", and the matching eslint / Codacy exclusions). Patching them here forks the template: every later upgrade would conflict on these files, and the fixes would silently disappear on the next --apply. The right home for these findings is upstream. Note that the Banner.astro sanitizer item is also mitigated architecturally here: banner content is author-written frontmatter rendered at build time, and this site defines no banner.

Action items

  • Triage the table against the current upstream cloudflare/nimbus main; drop items already fixed there.
  • Report the remaining ones upstream (cloudflare/nimbus issues), starting with the P1 inert disclosure bug and the type="button" theme toggle.
  • After the next nimbus-docs diff --apply upgrade, re-check which items are closed and update this issue.
  • If upstream declines a finding that matters here (accessibility ones especially), decide whether to fork that one file deliberately and record it next to the eslint/Codacy exclusion comments.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    review-follow-upReview finding deferred to a follow-uptype:docsDocumentation improvements

    Type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions