refactor(docs): Ship docs redesign#831
Conversation
Co-locate one-off components in app/ and keep only true design system components in components/. Merge CustomDocsLayout into layout.tsx to eliminate unnecessary abstraction. **Component locations:** - app/components/NavigationBar - used only in root layout - app/(docs)/components/Sidebar - used only in docs layout - app/(docs)/components/Navigation - used only in docs layout - app/(docs)/[[...slug]]/components/DocsPageLayout - page structure - app/(docs)/[[...slug]]/components/PageActions - page-specific actions **Design system (components/):** - AsideLink, Button, Collapsible, ScrollArea - custom docs components - DownloadButton, DatabaseTable, ResourceCard, ResourceGrid - MDX components **Changes:** - Merged CustomDocsLayout into app/(docs)/layout.tsx - Removed abbreviations (nav → Navigation, toc → TableOfContents) - All components in ComponentName/ComponentName.tsx + index.ts structure - Deleted empty ai/, docs/, ui/ folders - Updated all import paths
- Remove non-null assertion in TableOfContents.tsx - Replace SVGProps<any> with SVGProps<SVGSVGElement> - Add role="img" and aria-label to all SVG icons - Add aria-hidden="true" to decorative SVG - Fix useEffect dependencies in Sidebar.tsx - Replace array index key with unique identifier in Breadcrumb - Remove dangerouslySetInnerHTML from ResourceCard - Fix PopoverProps imports to use Collapsible types - Fix ResourceCard import path in ResourceGrid - Add type annotation for item parameter in TOCItemType.find()
📝 WalkthroughWalkthroughA comprehensive redesign of the documentation application replaces the previous layout system with a Fumadocs-based architecture. Changes introduce new layout components (DocsPageLayout, PageClient), a sidebar navigation system, a top-level navigation bar, multiple UI primitives (Button, Collapsible, ScrollArea), context providers for navigation state, and expanded dependency support including Radix UI components, framer-motion, and class-variance-authority. Documentation content is reorganized and styling is overhauled with a new OKLCh-based color system. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/docs/src/app/(docs)/[[...slug]]/components/PageActions/PageActions.tsx (1)
108-164: Missingrole="img"attribute on Scira AI SVG.Other SVG icons in this file have
role="img"for accessibility consistency, but the Scira AI SVG is missing this attribute.🔧 Suggested fix
icon: ( <svg width="910" height="934" viewBox="0 0 910 934" fill="none" + role="img" xmlns="http://www.w3.org/2000/svg" >
🤖 Fix all issues with AI agents
In `@apps/docs/src/app/`(docs)/components/Navigation/Navigation.tsx:
- Around line 39-56: The transparent state is only initialized from
transparentMode and can become stale when transparentMode changes to/from
"always" or "none"; add a new effect that watches transparentMode and sets
setTransparent(true) when transparentMode === "always" and setTransparent(false)
when transparentMode === "none" (keep the existing effect for "top" behavior
using the scroll listener). Replace the magic number 10 in the scroll listener
with a module-level constant (e.g., SCROLL_TRANSPARENT_THRESHOLD) and use that
constant in the listener and initial check. Finally, split out the Title
component into its own file (export default Title) and keep NavProvider in
Navigation.tsx to follow the one-component-per-file guideline.
In `@apps/docs/src/app/`(docs)/components/Sidebar/Sidebar.tsx:
- Around line 103-105: The fallback skeleton in the Sidebar component uses
Math.random() to set dynamic widths, causing SSR/client hydration mismatches;
replace the non-deterministic expression in the style width (the inline style
where width: `${Math.random() * (70 - 30) + 30}%`) with a deterministic
calculation that uses a stable value such as the item index or a predefined
array of widths (for example use index to compute 30 + ((index * 17) % 40) or
pick from [30,45,60,...]) so the server and client render identical widths
during hydration.
In
`@apps/docs/src/app/components/NavigationBar/components/NavigationMobile/NavigationMobile.tsx`:
- Around line 35-37: The error thrown in useNavbarMobile currently references
"NavbarMobileProvider" but the actual provider component is exported as
NavbarProvider; update the error message inside the useNavbarMobile hook to
reference the correct provider name "NavbarProvider" (or match whatever exported
provider symbol is used) so the thrown Error accurately guides developers when
the hook is used outside the provider.
In `@apps/docs/src/app/global.css`:
- Around line 127-130: The current CSS radius variables (--radius-sm,
--radius-md, --radius-lg, --radius-xl) subtract fixed pixel values from a small
base (--radius), which can yield zero/negative radii when --radius is small;
update the definitions to avoid subtraction by either increasing the base
--radius value to a safe minimum (e.g., match shadcn/ui default) or replace the
calc-subtractions with relative multipliers (e.g., --radius-sm:
calc(var(--radius) * 0.75); --radius-md: calc(var(--radius) * 0.9); keep
--radius-lg and --radius-xl as var(--radius) and calc(var(--radius) * 1.25)
respectively) so rounding/clamping issues are prevented while preserving
proportional sizing.
In `@apps/docs/src/components/AsideLink/AsideLink.tsx`:
- Line 10: The component declares and destructures a unused prop startWith in
AsideLink; replace the exact-match check (pathname === href) with the shared
isActive utility so startWith is used for nested matching: import isActive from
'apps/docs/src/lib/is-active.ts' (or the project's alias), then update the
active test inside AsideLink to use isActive(pathname, href, startWith) (or pass
startWith as the option expected by that utility); if you prefer not to support
nested matching, remove startWith from the prop/type and destructuring and keep
the exact-match check. Ensure you update the AsideLink prop type and the
destructuring accordingly and remove any unused import/variable.
In `@apps/docs/src/components/ResourceCard/ResourceCard.tsx`:
- Line 27: The ArrowUpRight icon in ResourceCard is purely decorative and needs
an accessibility attribute; update the JSX element for ArrowUpRight to include
aria-hidden="true" so assistive tech ignores the SVG (e.g., modify the
ArrowUpRight element in ResourceCard/ResourceCard.tsx to add aria-hidden="true"
alongside its existing className and props).
🧹 Nitpick comments (25)
apps/docs/src/components/Button/Button.tsx (2)
35-39: RedundantclassNameproperty in interface.
ButtonHTMLAttributes<HTMLButtonElement>already includesclassName?: string, so explicitly declaring it again inButtonPropsis unnecessary.🧹 Suggested simplification
export interface ButtonProps extends ButtonHTMLAttributes<HTMLButtonElement>, - VariantProps<typeof buttonVariants> { - className?: string; -} + VariantProps<typeof buttonVariants> {}
41-48: Consider adding ref forwarding for reusability.If this Button component will be used where refs are needed (e.g., for focus management, animations, or integration with third-party libraries), consider using
forwardRef.🔧 Optional: Add ref forwarding
-import type { ButtonHTMLAttributes } from "react"; +import { forwardRef, type ButtonHTMLAttributes } from "react"; -export function Button({ className, variant, size, ...props }: ButtonProps) { - return ( - <button - className={cn(buttonVariants({ variant, size, className }))} - {...props} - /> - ); -} +export const Button = forwardRef<HTMLButtonElement, ButtonProps>( + ({ className, variant, size, ...props }, ref) => { + return ( + <button + ref={ref} + className={cn(buttonVariants({ variant, size, className }))} + {...props} + /> + ); + }, +); +Button.displayName = "Button";apps/docs/src/components/Button/index.ts (1)
1-1: Consider exporting theButtoncomponent as well.The barrel file only exports
buttonVariants, but theButtoncomponent itself is not re-exported. For consistency and to make the component usable by other parts of the app, consider exporting both.♻️ Suggested export
-export { buttonVariants } from "./Button"; +export { Button, buttonVariants } from "./Button";apps/docs/src/app/global.css (1)
56-56: Inconsistent color format: HSL used instead of OKLCh.All other color variables use OKLCh format, but
--backgrounduses HSL. For consistency with the documented "OKLCh Color System", consider usingoklch(0 0 0)for pure black.Suggested fix
- --background: hsl(0 0% 0%); + --background: oklch(0 0 0);apps/docs/src/app/(docs)/components/Sidebar/components/SidebarContent/SidebarContent.tsx (2)
19-116:PlayIconandGetStartedIconare duplicates.Both icons render identical SVG paths. Consider reusing a single icon component.
♻️ Proposed fix
-const PlayIcon = () => ( - <svg - xmlns="http://www.w3.org/2000/svg" - width="1.2em" - height="1.2em" - viewBox="0 0 24 24" - role="img" - aria-label="Play" - > - <path - fill="currentColor" - d="M12 2C6.48 2 2 6.48 2 12s4.48 10 10 10s10-4.48 10-10S17.52 2 12 2m-1 14H9V8h2zm1 0V8l5 4z" - /> - </svg> -); +const PlayIcon = ({ label = "Play" }: { label?: string }) => ( + <svg + xmlns="http://www.w3.org/2000/svg" + width="1.2em" + height="1.2em" + viewBox="0 0 24 24" + role="img" + aria-label={label} + > + <path + fill="currentColor" + d="M12 2C6.48 2 2 6.48 2 12s4.48 10 10 10s10-4.48 10-10S17.52 2 12 2m-1 14H9V8h2zm1 0V8l5 4z" + /> + </svg> +); -// Section header icons -const GetStartedIcon = () => ( - <svg - xmlns="http://www.w3.org/2000/svg" - width="1.4em" - height="1.4em" - viewBox="0 0 24 24" - role="img" - aria-label="Get Started" - > - <path - fill="currentColor" - d="M12 2C6.48 2 2 6.48 2 12s4.48 10 10 10s10-4.48 10-10S17.52 2 12 2m-1 14H9V8h2zm1 0V8l5 4z" - /> - </svg> -); +const GetStartedIcon = () => <PlayIcon label="Get Started" />;
1-200: Consider extracting icons to a shared module or usinglucide-react.This file defines 8 inline icon components alongside the data structure. Per coding guidelines, prefer one component per file. Since
lucide-reactis already a dependency (imported forLucideIcontype), consider:
- Using equivalent lucide icons where available (e.g.,
Play,Book,Download,BarChart3,Code)- Extracting custom icons to a co-located
icons.tsxif lucide equivalents don't match the designThis is a low-priority refactor since the icons are simple SVG wrappers tightly coupled to this data structure.
apps/docs/COMPONENTS.md (1)
173-202: Add a language identifier to the fenced code block.The fenced code block is missing a language identifier. Adding one improves syntax highlighting and satisfies markdown linting rules.
📝 Proposed fix
-``` +```text apps/docs/src/components/ ├── ui/ # shadcn/ui and custom UI componentsapps/docs/src/components/ScrollArea/ScrollArea.tsx (1)
13-17: ScrollArea always includes a vertical scrollbar.The
ScrollAreacomponent unconditionally renders a verticalScrollBar(line 16). This may limit flexibility for use cases where only horizontal scrolling is needed or where the scrollbar should be explicitly controlled by the consumer.Consider making the scrollbar inclusion configurable or documenting this behavior.
🔧 Proposed fix for configurable scrollbar
+type ScrollAreaProps = React.ComponentPropsWithoutRef<typeof ScrollAreaPrimitive.Root> & { + showVerticalScrollbar?: boolean; + showHorizontalScrollbar?: boolean; +}; const ScrollArea = React.forwardRef< React.ComponentRef<typeof ScrollAreaPrimitive.Root>, - React.ComponentPropsWithoutRef<typeof ScrollAreaPrimitive.Root> ->(({ className, children, ...props }, ref) => ( + ScrollAreaProps +>(({ className, children, showVerticalScrollbar = true, showHorizontalScrollbar = false, ...props }, ref) => ( <ScrollAreaPrimitive.Root ref={ref} className={cn("overflow-hidden", className)} {...props} > {children} <ScrollAreaPrimitive.Corner /> - <ScrollBar orientation="vertical" /> + {showVerticalScrollbar && <ScrollBar orientation="vertical" />} + {showHorizontalScrollbar && <ScrollBar orientation="horizontal" />} </ScrollAreaPrimitive.Root> ));apps/docs/src/app/(docs)/components/Navigation/Navigation.tsx (2)
44-49: Extract the scroll threshold into a named constant.This improves readability and keeps future tweaks localized. As per coding guidelines, please avoid magic numbers.
♻️ Suggested refactor
+const TRANSPARENT_SCROLL_THRESHOLD = 10; + const listener = () => { if (document.documentElement.hasAttribute("data-anchor-scrolling")) { return; } - setTransparent(window.scrollY < 10); + setTransparent(window.scrollY < TRANSPARENT_SCROLL_THRESHOLD); };
35-89: SplitNavProviderandTitleinto separate files.This file defines multiple React components, which conflicts with the one-component-per-file guideline. Please consider extracting
Title(and optionally the hook) into their own files. As per coding guidelines.apps/docs/src/mdx-components.tsx (1)
3-6: Prefer concrete component imports over barrels.To avoid barrel file abuse and potential circular dependencies, consider importing directly from component files. As per coding guidelines.
♻️ Suggested refactor
-import { DatabaseTable } from "@/components/DatabaseTable"; -import { DownloadButton } from "@/components/DownloadButton"; -import { ResourceCard } from "@/components/ResourceCard"; -import { ResourceGrid } from "@/components/ResourceGrid"; +import { DatabaseTable } from "@/components/DatabaseTable/DatabaseTable"; +import { DownloadButton } from "@/components/DownloadButton/DownloadButton"; +import { ResourceCard } from "@/components/ResourceCard/ResourceCard"; +import { ResourceGrid } from "@/components/ResourceGrid/ResourceGrid";apps/docs/src/app/layout.tsx (1)
4-5: Consider re-exportingNavbarProviderfrom the NavigationBar barrel.The import on line 5 reaches into the internal structure of NavigationBar (
/components/NavigationMobile). This couples the layout to the internal organization of NavigationBar.For a cleaner public API, re-export
NavbarProviderfrom@/app/components/NavigationBar/index.ts:// In NavigationBar/index.ts export { default as NavigationBar } from "./NavigationBar"; export { NavbarProvider } from "./components/NavigationMobile";Then simplify the import here:
-import { NavigationBar } from "@/app/components/NavigationBar"; -import { NavbarProvider } from "@/app/components/NavigationBar/components/NavigationMobile"; +import { NavigationBar, NavbarProvider } from "@/app/components/NavigationBar";apps/docs/src/components/ResourceGrid/ResourceGrid.tsx (1)
17-18: Consider usinghrefalone as the key if hrefs are unique.The current key
${resource.href}-${index}still includes the array index. Ifhrefvalues are guaranteed unique (which they should be for distinct resources), the index suffix is unnecessary and could mask issues where duplicate hrefs exist.If duplicate hrefs are possible, keeping the composite key is a valid safeguard.
-<ResourceCard key={`${resource.href}-${index}`} {...resource} /> +<ResourceCard key={resource.href} {...resource} />apps/docs/src/components/ResourceCard/ResourceCard.tsx (1)
29-33: Consider making the entire card clickable for better UX.The card has hover states (
group-hover) suggesting interactivity across the entire surface, but only the title is actually linked. This may confuse users who expect the whole card to be clickable.💡 Option: Wrap entire card content in the anchor
<div className={cn( - "relative flex justify-between rounded-none flex-col group space-y-1 border transition-colors hover:bg-muted/80", + "relative flex justify-between rounded-none flex-col group space-y-1 border transition-colors", className, )} > - <div> - <ArrowUpRight aria-hidden="true" className="absolute top-3 right-3 h-4 w-4 group-hover:opacity-100 opacity-80 text-muted-foreground transition-colors group-hover:text-foreground no-underline underline-offset-0" /> - <div className="p-4 py-0 flex items-start justify-between"> - <a href={href} target="_blank" rel="noopener noreferrer"> - <h3 className="font-semibold text-md tracking-tight no-underline"> - {title} - </h3> - </a> - </div> - <p className="p-4 py-0 text-sm md:decoration-none text-muted-foreground"> - {description} - </p> - </div> + <a href={href} target="_blank" rel="noopener noreferrer" className="hover:bg-muted/80 transition-colors"> + <div> + <ArrowUpRight aria-hidden="true" className="absolute top-3 right-3 h-4 w-4 group-hover:opacity-100 opacity-80 text-muted-foreground transition-colors group-hover:text-foreground" /> + <div className="p-4 py-0 flex items-start justify-between"> + <h3 className="font-semibold text-md tracking-tight no-underline"> + {title} + </h3> + </div> + <p className="p-4 py-0 text-sm text-muted-foreground"> + {description} + </p> + </div> + </a>apps/docs/src/app/(docs)/[[...slug]]/page.tsx (1)
19-21: Consider adding a comment explaining the empty header div.The empty
<div className="w-10 h-4"></div>as thetableOfContent.headeris unclear. A brief comment would help future maintainers understand its purpose (presumably a spacer).apps/docs/src/app/components/NavigationBar/NavigationBar.tsx (2)
11-42: Consider extracting helper components to separate files.This file contains multiple components (
SupersetLogo,SidebarTrigger,NavigationBar,NavLink). Per coding guidelines, prefer one component per file. Consider extracting at minimumSupersetLogoandNavLinkto co-located files within this component's folder.📁 Suggested structure
NavigationBar/ ├── NavigationBar.tsx # Main component + SidebarTrigger (tightly coupled) ├── NavLink.tsx # Reusable link component ├── SupersetLogo.tsx # SVG logo component ├── index.ts # Barrel export └── components/ └── NavigationMobile/ # Already exists
110-111: Prefercn()utility for className composition.String concatenation is used here, but the
cn()utility is available and used elsewhere in the codebase for consistent class merging.🔧 Proposed fix
+import { cn } from "@/lib/cn"; ... - className={`${baseClasses} ${className || ""}`} + className={cn(baseClasses, className)}Apply similarly on line 121.
apps/docs/src/app/(docs)/components/Sidebar/Sidebar.tsx (1)
21-30:useEffectdependency oncurrentOpenmay cause unnecessary re-runs.Including
currentOpenin the dependency array means the effect re-runs whenever any section is toggled, not just on pathname changes. Consider using a ref or functional update pattern to avoid the dependency.♻️ Proposed fix using functional update
useEffect(() => { // Ensure the current section is open when pathname changes const defaultValue = contents.findIndex((item) => item.list.some((listItem) => listItem.href === pathname), ); const currentSection = defaultValue === -1 ? 0 : defaultValue; - if (!currentOpen.includes(currentSection)) { - setCurrentOpen((prev) => [...prev, currentSection]); - } - }, [pathname, currentOpen]); + setCurrentOpen((prev) => + prev.includes(currentSection) ? prev : [...prev, currentSection] + ); + }, [pathname]);This removes
currentOpenfrom the dependency array by moving the check inside the functional update.apps/docs/src/components/DatabaseTable/DatabaseTable.tsx (1)
73-74: Addaria-hidden="true"to decorative icons.The icons within the labeled badge spans (Key, Link, Check, X) are decorative since the adjacent text already conveys the meaning. Adding
aria-hidden="true"prevents screen readers from announcing redundant icon content.♻️ Suggested fix
- <Key className="h-3 w-3" /> + <Key className="h-3 w-3" aria-hidden="true" />Apply similarly to
Link,Check, andXicons.Also applies to: 82-83, 100-101, 108-109
apps/docs/src/app/components/NavigationBar/components/NavigationMobile/NavigationMobile.tsx (2)
49-49: Consider extracting magic number to a named constant.The
56pxvalue for the navigation bar height appears to be a layout constant. Extracting it to a named constant improves maintainability and documents its purpose.♻️ Suggested refactor
+const NAV_HEIGHT_PX = 56; + export const NavigationMobile = () => { // ... return ( <div className={cn( - "fixed top-[56px] inset-x-0 transform-gpu z-[100] bg-background grid grid-rows-[0fr] duration-300 transition-all navbar:hidden", + `fixed top-[${NAV_HEIGHT_PX}px] inset-x-0 transform-gpu z-[100] bg-background grid grid-rows-[0fr] duration-300 transition-all navbar:hidden`,Alternatively, consider using a CSS variable like
--fd-nav-heightfor consistency with other layout components in this PR.
66-76: Verify uniqueness ofitem.titleused as React key.If
section.listcontains items with duplicate titles, React's reconciliation may behave unexpectedly. Consider usingitem.hrefas the key for link items since URLs should be unique.♻️ Suggested fix
{section.list.map((item) => { if (item.separator || item.group) { return ( <div - key={item.title} + key={`group-${item.title}`} className="text-xs text-muted-foreground py-1" > {item.title} </div> ); } return ( <Link - key={item.title} + key={item.href} href={item.href}apps/docs/src/app/(docs)/[[...slug]]/components/DocsPageLayout/DocsPageLayout.tsx (1)
190-193: Consider internationalization for "Edit on GitHub" text.The text "Edit on GitHub" is hardcoded while other UI text in this codebase uses
I18nLabel(as seen on line 152). For consistency with the internationalization approach, consider using the i18n pattern here as well.♻️ Suggested approach
+import { useI18n } from "fumadocs-ui/contexts/i18n"; function EditOnGitHub({ // ... }: EditOnGitHubOptions) { + const { text } = useI18n(); // ... return ( <a ...> <Edit className="size-3.5" /> - Edit on GitHub + {text.editOnGitHub ?? "Edit on GitHub"} </a> ); }This would require adding the
editOnGitHubkey to the i18n configuration.apps/docs/src/app/(docs)/[[...slug]]/components/DocsPageLayout/components/PageClient/components/TableOfContents/TableOfContents.tsx (3)
169-173: Consider adding a guard for malformed URLs.The selector assumes
item.urlstarts with#. If it doesn't, the query selector could fail or match unintended elements.💡 Optional defensive check
for (let i = 0; i < items.length; i++) { + const urlHash = items[i].url.startsWith("#") ? items[i].url.slice(1) : items[i].url; const element: HTMLElement | null = container.querySelector( - `a[href="#${items[i].url.slice(1)}"]`, + `a[href="#${urlHash}"]`, ); if (!element) continue;
245-253: Consider extracting offset constants for clarity.The magic numbers in
getItemOffsetandgetLineOffsetcould be extracted to named constants for better maintainability.💡 Suggested refactor
+const TOC_ITEM_OFFSET = { + DEFAULT: 14, + DEPTH_3: 26, + DEPTH_4_PLUS: 36, +} as const; + +const TOC_LINE_OFFSET = { + DEFAULT: 0, + DEPTH_3_PLUS: 10, +} as const; + function getItemOffset(depth: number): number { - if (depth <= 2) return 14; - if (depth === 3) return 26; - return 36; + if (depth <= 2) return TOC_ITEM_OFFSET.DEFAULT; + if (depth === 3) return TOC_ITEM_OFFSET.DEPTH_3; + return TOC_ITEM_OFFSET.DEPTH_4_PLUS; } function getLineOffset(depth: number): number { - return depth >= 3 ? 10 : 0; + return depth >= 3 ? TOC_LINE_OFFSET.DEPTH_3_PLUS : TOC_LINE_OFFSET.DEFAULT; }
315-315: Clarify the fallback pattern forTocProvider.The
Context.Provider || Contextfallback is unusual sinceContext.Providershould always exist in React. If this is defensive coding for an edge case, consider adding a comment explaining the rationale.
| const [transparent, setTransparent] = useState(transparentMode !== "none"); | ||
|
|
||
| useEffect(() => { | ||
| if (transparentMode !== "top") return; | ||
|
|
||
| const listener = () => { | ||
| if (document.documentElement.hasAttribute("data-anchor-scrolling")) { | ||
| return; | ||
| } | ||
| setTransparent(window.scrollY < 10); | ||
| }; | ||
|
|
||
| listener(); | ||
| window.addEventListener("scroll", listener, { passive: true }); | ||
| return () => { | ||
| window.removeEventListener("scroll", listener); | ||
| }; | ||
| }, [transparentMode]); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "Navigation.tsx" -type fRepository: superset-sh/superset
Length of output: 127
🏁 Script executed:
cat -n ./apps/docs/src/app/(docs)/components/Navigation/Navigation.tsxRepository: superset-sh/superset
Length of output: 221
🏁 Script executed:
cat -n "./apps/docs/src/app/(docs)/components/Navigation/Navigation.tsx"Repository: superset-sh/superset
Length of output: 2551
Sync transparent state when transparentMode changes to/from "always" or "none" modes.
The transparent state initializes from transparentMode but won't update if the prop changes after mount. When transparentMode is "always" or "none", the existing effect early-returns and leaves the state stale. Add a separate effect to sync state for non-"top" modes.
Extract the scroll threshold magic number to a named constant:
Line 48 uses hardcoded 10 for the scroll threshold; extract this to a module-level constant.
Split into separate files:
The file defines both NavProvider and Title components. Move Title to its own file per the one-component-per-file guideline.
🤖 Prompt for AI Agents
In `@apps/docs/src/app/`(docs)/components/Navigation/Navigation.tsx around lines
39 - 56, The transparent state is only initialized from transparentMode and can
become stale when transparentMode changes to/from "always" or "none"; add a new
effect that watches transparentMode and sets setTransparent(true) when
transparentMode === "always" and setTransparent(false) when transparentMode ===
"none" (keep the existing effect for "top" behavior using the scroll listener).
Replace the magic number 10 in the scroll listener with a module-level constant
(e.g., SCROLL_TRANSPARENT_THRESHOLD) and use that constant in the listener and
initial check. Finally, split out the Title component into its own file (export
default Title) and keep NavProvider in Navigation.tsx to follow the
one-component-per-file guideline.
| style={{ | ||
| width: `${Math.random() * (70 - 30) + 30}%`, | ||
| }} |
There was a problem hiding this comment.
Hydration mismatch: Math.random() produces different values on server and client.
Using Math.random() in the fallback skeleton will produce different widths during SSR vs client hydration, causing a React hydration mismatch warning. Use deterministic widths instead.
🐛 Proposed fix using deterministic widths
- <div
- className="h-3 bg-muted rounded-md"
- style={{
- width: `${Math.random() * (70 - 30) + 30}%`,
- }}
- aria-hidden="true"
- />
+ <div
+ className="h-3 bg-muted rounded-md w-1/2"
+ aria-hidden="true"
+ />Alternatively, use the item index to create varied but deterministic widths:
width: `${30 + ((index * 17) % 40)}%`🤖 Prompt for AI Agents
In `@apps/docs/src/app/`(docs)/components/Sidebar/Sidebar.tsx around lines 103 -
105, The fallback skeleton in the Sidebar component uses Math.random() to set
dynamic widths, causing SSR/client hydration mismatches; replace the
non-deterministic expression in the style width (the inline style where width:
`${Math.random() * (70 - 30) + 30}%`) with a deterministic calculation that uses
a stable value such as the item index or a predefined array of widths (for
example use index to compute 30 + ((index * 17) % 40) or pick from
[30,45,60,...]) so the server and client render identical widths during
hydration.
| throw new Error( | ||
| "useNavbarMobile must be used within a NavbarMobileProvider", | ||
| ); |
There was a problem hiding this comment.
Fix inconsistent provider name in error message.
The error message references "NavbarMobileProvider" but the actual exported component is named NavbarProvider. This inconsistency could confuse developers debugging context issues.
🐛 Suggested fix
if (!context) {
throw new Error(
- "useNavbarMobile must be used within a NavbarMobileProvider",
+ "useNavbarMobile must be used within a NavbarProvider",
);
}📝 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.
| throw new Error( | |
| "useNavbarMobile must be used within a NavbarMobileProvider", | |
| ); | |
| throw new Error( | |
| "useNavbarMobile must be used within a NavbarProvider", | |
| ); |
🤖 Prompt for AI Agents
In
`@apps/docs/src/app/components/NavigationBar/components/NavigationMobile/NavigationMobile.tsx`
around lines 35 - 37, The error thrown in useNavbarMobile currently references
"NavbarMobileProvider" but the actual provider component is exported as
NavbarProvider; update the error message inside the useNavbarMobile hook to
reference the correct provider name "NavbarProvider" (or match whatever exported
provider symbol is used) so the thrown Error accurately guides developers when
the hook is used outside the provider.
| --radius-sm: calc(var(--radius) - 4px); | ||
| --radius-md: calc(var(--radius) - 2px); | ||
| --radius-lg: var(--radius); | ||
| --radius-xl: calc(var(--radius) + 4px); |
There was a problem hiding this comment.
Radius calculations produce negative/zero values.
With --radius: 0.2rem (≈3.2px), the subtraction yields:
--radius-sm: 3.2px − 4px = −0.8px (clamped to 0)--radius-md: 3.2px − 2px = 1.2px
This pattern expects a larger base radius (e.g., 0.5rem from shadcn/ui). Either increase --radius or adjust the calculations.
Option 1: Increase base radius
- --radius: 0.2rem;
+ --radius: 0.5rem;Option 2: Use multipliers instead of subtraction
- --radius-sm: calc(var(--radius) - 4px);
- --radius-md: calc(var(--radius) - 2px);
- --radius-lg: var(--radius);
- --radius-xl: calc(var(--radius) + 4px);
+ --radius-sm: calc(var(--radius) * 0.5);
+ --radius-md: calc(var(--radius) * 0.75);
+ --radius-lg: var(--radius);
+ --radius-xl: calc(var(--radius) * 2);📝 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.
| --radius-sm: calc(var(--radius) - 4px); | |
| --radius-md: calc(var(--radius) - 2px); | |
| --radius-lg: var(--radius); | |
| --radius-xl: calc(var(--radius) + 4px); | |
| --radius-sm: calc(var(--radius) * 0.5); | |
| --radius-md: calc(var(--radius) * 0.75); | |
| --radius-lg: var(--radius); | |
| --radius-xl: calc(var(--radius) * 2); |
🤖 Prompt for AI Agents
In `@apps/docs/src/app/global.css` around lines 127 - 130, The current CSS radius
variables (--radius-sm, --radius-md, --radius-lg, --radius-xl) subtract fixed
pixel values from a small base (--radius), which can yield zero/negative radii
when --radius is small; update the definitions to avoid subtraction by either
increasing the base --radius value to a safe minimum (e.g., match shadcn/ui
default) or replace the calc-subtractions with relative multipliers (e.g.,
--radius-sm: calc(var(--radius) * 0.75); --radius-md: calc(var(--radius) * 0.9);
keep --radius-lg and --radius-xl as var(--radius) and calc(var(--radius) * 1.25)
respectively) so rounding/clamping issues are prevented while preserving
proportional sizing.
| type Props = { | ||
| href: string; | ||
| children: React.ReactNode; | ||
| startWith: string; |
There was a problem hiding this comment.
Unused startWith prop.
The startWith prop is declared in the type (line 10) and destructured (line 19) but never used in the component logic. Additionally, the isActive check on line 26 only performs exact matching (pathname === href), while the relevant code snippet shows an existing isActive utility in apps/docs/src/lib/is-active.ts that supports nested path matching.
Consider either removing the unused prop or implementing the intended behavior using the existing utility.
🔧 Option 1: Remove unused prop
type Props = {
href: string;
children: React.ReactNode;
- startWith: string;
title?: string | null;
className?: string;
activeClassName?: string;
} & React.AnchorHTMLAttributes<HTMLAnchorElement>;
export const AsideLink = ({
href,
children,
- startWith,
title,
className,
activeClassName,
...props
}: Props) => {🔧 Option 2: Use the existing isActive utility
"use client";
import Link from "next/link";
import { usePathname } from "next/navigation";
import { cn } from "@/lib/cn";
+import { isActive } from "@/lib/is-active";
type Props = {
href: string;
children: React.ReactNode;
- startWith: string;
+ nested?: boolean;
title?: string | null;
className?: string;
activeClassName?: string;
} & React.AnchorHTMLAttributes<HTMLAnchorElement>;
export const AsideLink = ({
href,
children,
- startWith,
+ nested = false,
title,
className,
activeClassName,
...props
}: Props) => {
const pathname = usePathname();
- const isActive = pathname === href;
+ const active = isActive(href, pathname, nested);Also applies to: 19-19, 26-26
🤖 Prompt for AI Agents
In `@apps/docs/src/components/AsideLink/AsideLink.tsx` at line 10, The component
declares and destructures a unused prop startWith in AsideLink; replace the
exact-match check (pathname === href) with the shared isActive utility so
startWith is used for nested matching: import isActive from
'apps/docs/src/lib/is-active.ts' (or the project's alias), then update the
active test inside AsideLink to use isActive(pathname, href, startWith) (or pass
startWith as the option expected by that utility); if you prefer not to support
nested matching, remove startWith from the prop/type and destructuring and keep
the exact-match check. Ensure you update the AsideLink prop type and the
destructuring accordingly and remove any unused import/variable.
| )} | ||
| > | ||
| <div> | ||
| <ArrowUpRight className="absolute top-3 right-3 h-4 w-4 group-hover:opacity-100 opacity-80 text-muted-foreground transition-colors group-hover:text-foreground no-underline underline-offset-0" /> |
There was a problem hiding this comment.
Add aria-hidden="true" to decorative icon.
The ArrowUpRight icon is decorative (the link text provides semantic meaning). Per the PR objectives' accessibility fixes, decorative SVGs should have aria-hidden="true".
🔧 Proposed fix
- <ArrowUpRight className="absolute top-3 right-3 h-4 w-4 group-hover:opacity-100 opacity-80 text-muted-foreground transition-colors group-hover:text-foreground no-underline underline-offset-0" />
+ <ArrowUpRight aria-hidden="true" className="absolute top-3 right-3 h-4 w-4 group-hover:opacity-100 opacity-80 text-muted-foreground transition-colors group-hover:text-foreground no-underline underline-offset-0" />📝 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.
| <ArrowUpRight className="absolute top-3 right-3 h-4 w-4 group-hover:opacity-100 opacity-80 text-muted-foreground transition-colors group-hover:text-foreground no-underline underline-offset-0" /> | |
| <ArrowUpRight aria-hidden="true" className="absolute top-3 right-3 h-4 w-4 group-hover:opacity-100 opacity-80 text-muted-foreground transition-colors group-hover:text-foreground no-underline underline-offset-0" /> |
🤖 Prompt for AI Agents
In `@apps/docs/src/components/ResourceCard/ResourceCard.tsx` at line 27, The
ArrowUpRight icon in ResourceCard is purely decorative and needs an
accessibility attribute; update the JSX element for ArrowUpRight to include
aria-hidden="true" so assistive tech ignores the SVG (e.g., modify the
ArrowUpRight element in ResourceCard/ResourceCard.tsx to add aria-hidden="true"
alongside its existing className and props).
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@apps/docs/src/app/`(docs)/[[...slug]]/components/DocsPageLayout/components/PageClient/components/TableOfContents/TableOfContents.tsx:
- Around line 363-371: The Text component instance and ChevronRight icon in
TableOfContents.tsx are decorative and need aria-hidden set to prevent screen
readers from announcing them; update the JSX where Text (the empty <Text
className="size-4 shrink-0" />) and ChevronRight (the <ChevronRight ... />) are
rendered to include aria-hidden="true" (ensure the ChevronRight receives the
prop and the Text element likewise), keeping existing className and logic
intact.
🧹 Nitpick comments (4)
apps/docs/src/app/(docs)/[[...slug]]/components/DocsPageLayout/components/PageClient/components/TableOfContents/TableOfContents.tsx (4)
86-124: Extract magic numbers to named constants.The scroll threshold (
1) and gradient size (40px) are repeated. Per coding guidelines, extract these to named constants at module top for clarity and maintainability.♻️ Suggested refactor
Add constants at the top of the file (after imports):
const SCROLL_THRESHOLD_PX = 1; const GRADIENT_FADE_SIZE_PX = 40;Then update the usage:
const checkScroll = () => { const { scrollTop, scrollHeight, clientHeight } = viewport; const newState = { - isAtTop: scrollTop <= 1, - isAtBottom: scrollTop + clientHeight >= scrollHeight - 1, + isAtTop: scrollTop <= SCROLL_THRESHOLD_PX, + isAtBottom: scrollTop + clientHeight >= scrollHeight - SCROLL_THRESHOLD_PX, }; // ... }; const maskImage = useMemo(() => { const { isAtTop, isAtBottom } = scrollState; if (isAtTop && isAtBottom) return "none"; if (isAtTop) - return "linear-gradient(to bottom, black calc(100% - 40px), transparent)"; + return `linear-gradient(to bottom, black calc(100% - ${GRADIENT_FADE_SIZE_PX}px), transparent)`; if (isAtBottom) - return "linear-gradient(to top, black calc(100% - 40px), transparent)"; - return "linear-gradient(to bottom, transparent, black 40px, black calc(100% - 40px), transparent)"; + return `linear-gradient(to top, black calc(100% - ${GRADIENT_FADE_SIZE_PX}px), transparent)`; + return `linear-gradient(to bottom, transparent, black ${GRADIENT_FADE_SIZE_PX}px, black calc(100% - ${GRADIENT_FADE_SIZE_PX}px), transparent)`; }, [scrollState]);
245-253: Extract layout constants for depth-based offsets.Per coding guidelines, these magic numbers should be extracted to named constants at module top.
♻️ Suggested refactor
+// TOC item indentation offsets by heading depth +const ITEM_OFFSET_DEPTH_2 = 14; +const ITEM_OFFSET_DEPTH_3 = 26; +const ITEM_OFFSET_DEPTH_4_PLUS = 36; +const LINE_OFFSET_DEPTH_3_PLUS = 10; function getItemOffset(depth: number): number { - if (depth <= 2) return 14; - if (depth === 3) return 26; - return 36; + if (depth <= 2) return ITEM_OFFSET_DEPTH_2; + if (depth === 3) return ITEM_OFFSET_DEPTH_3; + return ITEM_OFFSET_DEPTH_4_PLUS; } function getLineOffset(depth: number): number { - return depth >= 3 ? 10 : 0; + return depth >= 3 ? LINE_OFFSET_DEPTH_3_PLUS : 0; }
315-315: Unnecessary fallback pattern.
Context.Provideris always truthy for a context created withcreateContext, so the|| Contextfallback never executes. Additionally, usingContextdirectly as a component would not work. Simplify this line.♻️ Suggested fix
-const TocProvider = Context.Provider || Context; +const TocProvider = Context.Provider;
1-396: Consider splitting into separate component files.Per coding guidelines, each
.tsxfile should contain one component. This file contains 8 components. While these components form a cohesive TOC module, consider organizing them into separate files within aTableOfContents/directory:
Toc.tsx- Main containerTocItemsEmpty.tsx- Empty stateTOCScrollArea.tsx- Scroll wrapperTOCItems.tsxandTOCItem.tsx- Item renderingTocPopover/- Popover family (TocPopover.tsx,TocPopoverTrigger.tsx,TocPopoverContent.tsx)This would improve maintainability and align with the project's component organization patterns.
| <Text className="size-4 shrink-0" /> | ||
| {text.toc} | ||
| <ChevronRight | ||
| className={cn( | ||
| "size-4 shrink-0 text-fd-muted-foreground transition-all", | ||
| !current && "opacity-0", | ||
| open ? "rotate-90" : "-ms-1.5", | ||
| )} | ||
| /> |
There was a problem hiding this comment.
Add aria-hidden to decorative icons.
The <Text> and <ChevronRight> icons are decorative (accompanying the "Table of Contents" text label). Per the PR objectives and accessibility best practices, add aria-hidden="true" to prevent screen readers from announcing them.
♿ Suggested fix
-<Text className="size-4 shrink-0" />
+<Text className="size-4 shrink-0" aria-hidden="true" />
{text.toc}
<ChevronRight
className={cn(
"size-4 shrink-0 text-fd-muted-foreground transition-all",
!current && "opacity-0",
open ? "rotate-90" : "-ms-1.5",
)}
+ aria-hidden="true"
/>📝 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.
| <Text className="size-4 shrink-0" /> | |
| {text.toc} | |
| <ChevronRight | |
| className={cn( | |
| "size-4 shrink-0 text-fd-muted-foreground transition-all", | |
| !current && "opacity-0", | |
| open ? "rotate-90" : "-ms-1.5", | |
| )} | |
| /> | |
| <Text className="size-4 shrink-0" aria-hidden="true" /> | |
| {text.toc} | |
| <ChevronRight | |
| className={cn( | |
| "size-4 shrink-0 text-fd-muted-foreground transition-all", | |
| !current && "opacity-0", | |
| open ? "rotate-90" : "-ms-1.5", | |
| )} | |
| aria-hidden="true" | |
| /> |
🤖 Prompt for AI Agents
In
`@apps/docs/src/app/`(docs)/[[...slug]]/components/DocsPageLayout/components/PageClient/components/TableOfContents/TableOfContents.tsx
around lines 363 - 371, The Text component instance and ChevronRight icon in
TableOfContents.tsx are decorative and need aria-hidden set to prevent screen
readers from announcing them; update the JSX where Text (the empty <Text
className="size-4 shrink-0" />) and ChevronRight (the <ChevronRight ... />) are
rendered to include aria-hidden="true" (ensure the ChevronRight receives the
prop and the Text element likewise), keeping existing className and logic
intact.
…TS.md - Move is-active.ts to PageClient/utils (only used there) - Delete apps/docs/COMPONENTS.md (no longer needed) - Update import path in PageClient.tsx
saddlepaddle
changed the title
1 participant
Summary
Restructured docs components following AGENTS.md conventions and resolved all lint/type errors.
Component Restructuring
Code Quality Fixes
Accessibility (8 SVG icons)
role="img"andaria-labelto all icon componentsaria-hidden="true"to decorative SVGsType Safety
SVGProps<any>withSVGProps<SVGSVGElement>Best Practices
dangerouslySetInnerHTMLfrom ResourceCardTesting
Summary by CodeRabbit
New Features
Documentation
Style
✏️ Tip: You can customize this high-level summary in your review settings.