feat: extend existing components with additional configuration props#3103
Open
coderrshyam wants to merge 3 commits intofuma-nama:devfrom
Open
feat: extend existing components with additional configuration props#3103coderrshyam wants to merge 3 commits intofuma-nama:devfrom
coderrshyam wants to merge 3 commits intofuma-nama:devfrom
Conversation
|
@coderrshyam is attempting to deploy a commit to the fumadocs Team on Vercel. A member of the Team first needs to authorize it. |
🦋 Changeset detectedLatest commit: 0744ed9 The changes in this PR will be included in the next version bump. This PR includes changesets to release 6 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
WalkthroughReplace className-only hooks with generic element props across DocsPage and layout components, and add per-instance prop forwarding points (navProps, smProps, lgProps, tocProps, tocPopoverProps, themeSwitch.props) across multiple layout implementations. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 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🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
There was a problem hiding this comment.
Pull request overview
This PR bumps fumadocs-ui and @fumadocs/base-ui with a minor release that extends several layout components to accept additional configuration props (extra props + className passthrough), primarily for nav containers, toggles, and DocsPage/TOC containers.
Changes:
- Added
navProps,themeSwitch.props, andsearchToggle.(smProps/lgProps)to shared layout option types to enable deeper customization. - Updated multiple layouts (
docs,notebook,flux,home) to forward the new props/class names into headers, theme toggles, and search toggles. - Updated
DocsPagePropsto use a genericpropsobject for the page container and added optional TOC container/popover props.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 41 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/radix-ui/src/layouts/shared/index.tsx | Adds shared option types for navProps, themeSwitch.props, and search toggle props. |
| packages/radix-ui/src/layouts/docs/page/index.tsx | Replaces className with props and adds TOC container/popover prop forwarding. |
| packages/radix-ui/src/layouts/notebook/page/index.tsx | Same DocsPage API updates + TOC container props forwarding for notebook layout. |
| packages/radix-ui/src/layouts/flux/page/index.tsx | Same DocsPage API update + TOC popover props forwarding for flux layout. |
| packages/radix-ui/src/layouts/docs/index.tsx | Forwards new navProps, searchToggle props, and themeSwitch.props through docs layout header/toggles. |
| packages/radix-ui/src/layouts/notebook/index.tsx | Forwards new navProps, searchToggle props, and themeSwitch.props through notebook navbar/toggles. |
| packages/radix-ui/src/layouts/flux/index.tsx | Forwards searchToggle.smProps and themeSwitch.props through flux navigation panel. |
| packages/radix-ui/src/layouts/home/client.tsx | Forwards navProps, search toggle props, and theme switch props through home header. |
| packages/base-ui/src/layouts/shared/index.tsx | Mirrors the shared option type extensions in base-ui. |
| packages/base-ui/src/layouts/docs/page/index.tsx | Mirrors DocsPage API update + TOC prop forwarding in base-ui. |
| packages/base-ui/src/layouts/notebook/page/index.tsx | Mirrors notebook DocsPage API update + TOC prop forwarding in base-ui. |
| packages/base-ui/src/layouts/flux/page/index.tsx | Mirrors flux DocsPage API update + TOC popover props forwarding in base-ui. |
| packages/base-ui/src/layouts/docs/index.tsx | Mirrors docs layout prop forwarding in base-ui. |
| packages/base-ui/src/layouts/notebook/index.tsx | Mirrors notebook navbar prop forwarding in base-ui. |
| packages/base-ui/src/layouts/flux/index.tsx | Mirrors flux prop forwarding in base-ui. |
| packages/base-ui/src/layouts/home/client.tsx | Mirrors home header prop forwarding in base-ui. |
| .changeset/gold-mice-pump.md | Declares minor releases for both packages and documents the feature. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
163
to
170
| <div | ||
| id="nd-toc" | ||
| className="sticky top-(--fd-docs-row-3) [grid-area:toc] h-[calc(var(--fd-docs-height)-var(--fd-docs-row-3))] flex flex-col w-(--fd-toc-width) pt-12 pe-4 pb-2 xl:layout:[--fd-toc-width:268px] max-xl:hidden" | ||
| className={cn( | ||
| 'sticky top-(--fd-docs-row-3) [grid-area:toc] h-[calc(var(--fd-docs-height)-var(--fd-docs-row-3))] flex flex-col w-(--fd-toc-width) pt-12 pe-4 pb-2 xl:layout:[--fd-toc-width:268px] max-xl:hidden', | ||
| tocProps?.className, | ||
| )} | ||
| {...tocProps} | ||
| > |
| * Apply props to the `#nd-page` container. | ||
| */ | ||
| className?: string; | ||
| props: ComponentProps<'article'>; |
Comment on lines
74
to
81
| <LargeSearchToggle | ||
| className="w-full rounded-full ps-2.5 max-w-[240px]" | ||
| className={cn( | ||
| 'w-full rounded-full ps-2.5 max-w-[240px]', | ||
| searchToggle.lgProps?.className, | ||
| )} | ||
| hideIfDisabled | ||
| {...searchToggle.lgProps} | ||
| /> |
Comment on lines
+161
to
+166
| className={cn( | ||
| 'p-1 h-full ms-1 rounded-xl bg-fd-muted *:rounded-lg', | ||
| themeSwitch.props?.className, | ||
| )} | ||
| mode={themeSwitch.mode} | ||
| {...themeSwitch.props} |
| mode?: 'light-dark' | 'light-dark-system'; | ||
|
|
||
| /** | ||
| * Props for the theme switch button |
| import type { I18nConfig } from 'fumadocs-core/i18n'; | ||
| import type { LinkItemType } from '@/utils/link-item'; | ||
| import Link from 'fumadocs-core/link'; | ||
| import { ButtonProps } from '@/components/ui/button'; |
Comment on lines
128
to
137
| <article | ||
| id="nd-page" | ||
| data-full={full} | ||
| className={cn( | ||
| 'flex flex-col w-full max-w-[900px] mx-auto [grid-area:main] px-4 py-6 gap-4 md:px-6 md:pt-8 xl:px-8 xl:pt-14', | ||
| full ? 'max-w-[1200px]' : 'xl:layout:[--fd-toc-width:268px]', | ||
| className, | ||
| props?.className, | ||
| )} | ||
| {...props} | ||
| > |
| * Apply props to the `#nd-page` container. | ||
| */ | ||
| className?: string; | ||
| props: ComponentProps<'article'>; |
Comment on lines
92
to
99
| <LargeSearchToggle | ||
| className="w-full rounded-full ps-2.5 max-w-[240px]" | ||
| className={cn( | ||
| 'w-full rounded-full ps-2.5 max-w-[240px]', | ||
| searchToggle.lgProps?.className, | ||
| )} | ||
| hideIfDisabled | ||
| {...searchToggle.lgProps} | ||
| /> |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/base-ui/src/layouts/notebook/index.tsx (2)
283-291:⚠️ Potential issue | 🟡 MinorPotential className override when spreading navProps on LayoutHeader.
Spreading
{...nav.navProps}after the mergedclassNamewill override the combined result ifnavPropscontains aclassName.🔧 Proposed fix
<LayoutHeader id="nd-subnav" + {...nav.navProps} className={cn( 'sticky [grid-area:header] flex flex-col top-(--fd-docs-row-1) z-10 backdrop-blur-sm transition-colors data-[transparent=false]:bg-fd-background/80 layout:[--fd-header-height:--spacing(14)]', nav.navProps?.className, showLayoutTabs && 'lg:layout:[--fd-header-height:--spacing(24)]', )} - {...nav.navProps} >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/base-ui/src/layouts/notebook/index.tsx` around lines 283 - 291, The combined className passed to LayoutHeader can be overridden because {...nav.navProps} is spread after the explicit className; to fix, ensure nav.navProps.className is merged into the computed value (using cn) or spread nav.navProps before the className prop so the final className is the merged result; update the LayoutHeader usage that builds className (reference LayoutHeader, nav.navProps, className, cn, showLayoutTabs) to either include nav.navProps.className in the cn(...) call or move {...nav.navProps} before the className prop so no className from nav.navProps can overwrite the merged classes.
332-340:⚠️ Potential issue | 🟡 MinorSame className override pattern with LargeSearchToggle.
🔧 Proposed fix
<LargeSearchToggle hideIfDisabled + {...searchToggle.lgProps} className={cn( 'w-full my-auto max-md:hidden', navMode === 'top' ? 'rounded-xl max-w-sm ps-2.5' : 'max-w-[240px]', searchToggle.lgProps?.className, )} - {...searchToggle.lgProps} />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/base-ui/src/layouts/notebook/index.tsx` around lines 332 - 340, The className from searchToggle.lgProps is being merged into cn(...) but then searchToggle.lgProps is spread after, which will override the computed className; update the JSX for LargeSearchToggle so the spreaded lgProps does not clobber the computed className — either spread {...searchToggle.lgProps} before the explicit className prop or omit className from the spread (e.g., extract const { className, ...rest } = searchToggle.lgProps and spread rest) and keep className={cn(..., className)} to ensure the merged classes are preserved.packages/radix-ui/src/layouts/docs/page/index.tsx (1)
147-156:⚠️ Potential issue | 🟠 Major
classNamecomposition is being overridden by spread order.Both containers compose
classNameviacn(...), but spreading{...props}/{...tocProps}afterward can overwrite it and drop base layout classes.💡 Proposed fix
export function DocsPage({ @@ children, props = {}, }: DocsPageProps) { + const { className: pageClassName, ...pageProps } = props; + const { className: tocClassName, ...tocContainerProps } = tocProps ?? {}; @@ <article id="nd-page" data-full={full} className={cn( @@ - props?.className, + pageClassName, )} - {...props} + {...pageProps} > @@ <div id="nd-toc" className={cn( 'sticky top-(--fd-docs-row-1) h-[calc(var(--fd-docs-height)-var(--fd-docs-row-1))] flex flex-col [grid-area:toc] w-(--fd-toc-width) pt-12 pe-4 pb-2 max-xl:hidden', - tocProps?.className, + tocClassName, )} - {...tocProps} + {...tocContainerProps} >#!/bin/bash # Read-only verification: detect className merge blocks that are followed by spread from the same prop bag. python - <<'PY' from pathlib import Path path = Path("packages/radix-ui/src/layouts/docs/page/index.tsx") lines = path.read_text().splitlines() def find_issue(prop_name): for i, line in enumerate(lines, 1): if "className={cn(" in line: block = "\n".join(lines[i-1:i+30]) if f"{prop_name}?.className" in block and f"{{...{prop_name}}}" in block: spread_line = None for j in range(i, min(i+31, len(lines)+1)): if f"{{...{prop_name}}}" in lines[j-1]: spread_line = j break print(f"[{prop_name}] className block starts at Line {i}, spread at Line {spread_line}") find_issue("props") find_issue("tocProps") PYAlso applies to: 165-170
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/radix-ui/src/layouts/docs/page/index.tsx` around lines 147 - 156, The composed className on the article and TOC containers is being clobbered by the subsequent spread of props/tocProps; update the JSX so you either (A) spread {...props} / {...tocProps} before the element's explicit className or (B) explicitly merge incoming className into your cn call (e.g. include props?.className / tocProps?.className inside the cn invocation) instead of relying on the spread order; locate the article element using className={cn(...)} and the TOC container with tocProps and adjust the spread/order so base layout classes from cn are preserved.
🧹 Nitpick comments (4)
packages/base-ui/src/layouts/shared/index.tsx (2)
5-5: Consider using a type-only import for better tree-shaking.♻️ Proposed change
-import { ButtonProps } from '@/components/ui/button'; +import type { ButtonProps } from '@/components/ui/button';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/base-ui/src/layouts/shared/index.tsx` at line 5, The import of ButtonProps should be a type-only import to aid tree-shaking and avoid pulling runtime module code: replace the current import of ButtonProps with a type import (use the TypeScript "import type { ButtonProps }" form) wherever ButtonProps is referenced in this file (e.g., in packages/base-ui/src/layouts/shared/index.tsx) and ensure no runtime usage requires the actual module so the change is safe.
46-58: Type asymmetry betweensmPropsandlgProps.Same observation as the radix-ui shared layout:
smPropsincludesButtonPropswhilelgPropsdoes not. Consider aligning these for API consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/base-ui/src/layouts/shared/index.tsx` around lines 46 - 58, The searchToggle prop types are asymmetric: smProps is typed as Omit<ComponentProps<'button'>, 'color'> & ButtonProps while lgProps is only ComponentProps<'button'>; align them by either giving lgProps the same type as smProps or removing ButtonProps from smProps so both use the same shape. Update the searchToggle definition to use a single consistent type reference (e.g., make lgProps Omit<ComponentProps<'button'>, 'color'> & ButtonProps or change smProps to ComponentProps<'button'>) and ensure consumers of searchToggle, smProps, and lgProps compile against the unified type.packages/radix-ui/src/layouts/shared/index.tsx (2)
46-58: Type asymmetry betweensmPropsandlgProps.
smPropsusesOmit<ComponentProps<'button'>, 'color'> & ButtonPropswhilelgPropsuses onlyComponentProps<'button'>. IfLargeSearchTogglealso usesButtonProps(e.g., from the same button component), consider aligning the types for consistency. Otherwise, document why the types differ.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/radix-ui/src/layouts/shared/index.tsx` around lines 46 - 58, The types for searchToggle.smProps and searchToggle.lgProps are inconsistent: smProps is typed as Omit<ComponentProps<'button'>, 'color'> & ButtonProps while lgProps is ComponentProps<'button'>; update lgProps to match smProps (ComponentProps<'button'> with the same Omit and & ButtonProps) if the LargeSearchToggle component also accepts ButtonProps, or add a concise comment above the searchToggle type explaining why lgProps intentionally differs; refer to the symbols smProps, lgProps and the LargeSearchToggle consumer to make the change or add the documentation.
5-5: Consider using a type-only import for better tree-shaking.
ButtonPropsis used only for type annotations. Usingimport typeensures it's stripped during compilation.♻️ Proposed change
-import { ButtonProps } from '@/components/ui/button'; +import type { ButtonProps } from '@/components/ui/button';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/radix-ui/src/layouts/shared/index.tsx` at line 5, The import of ButtonProps is used only for typing; change the import to a type-only import by replacing the regular import with an "import type { ButtonProps } from '@/components/ui/button';" so the compiler can erase it and improve tree-shaking; locate the import statement that currently reads "import { ButtonProps } ..." in the shared layout module and update it to the type-only form.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/base-ui/src/layouts/docs/index.tsx`:
- Around line 162-166: The className passed to ThemeToggle can be overridden by
themeSwitch.props when you spread {...themeSwitch.props} afterwards; fix by
extracting/merging the className from themeSwitch.props and then spread the
remaining props without their className so the merged className is always
applied. Specifically, reference ThemeToggle, cn, and themeSwitch.props: compute
a mergedClassName = cn('ms-auto p-0', themeSwitch.props?.className) and spread
only the rest of themeSwitch.props (excluding its className) into ThemeToggle
while passing className={mergedClassName}.
- Around line 251-256: The SearchToggle is currently given className first then
{...searchToggle.smProps}, which can overwrite the merged className; fix by
spreading searchToggle.smProps before the explicit className so the merged
className (cn('p-2', searchToggle.smProps?.className)) is applied last. Update
the JSX for SearchToggle to place {...searchToggle.smProps} before the className
prop (still using cn and hideIfDisabled) so the intended class merging works
correctly.
- Around line 201-207: The SidebarDrawer rendering of ThemeToggle uses the wrong
className override order; replicate the pattern used in docs/index.tsx by
passing className={cn('p-0', themeSwitch.props?.className)} and spreading
themeSwitch.props after that so the default 'p-0' is preserved and any
additional classes are merged (refer to ThemeToggle, SidebarDrawer,
themeSwitch.props, and cn). Ensure the explicit className prop is set on the
ThemeToggle element (not overwritten by the spread) and if a custom component is
provided via themeSwitch.component, apply the same merged className logic there
as well.
- Around line 237-244: The LayoutHeader is passing a computed className and then
spreading nav.navProps afterwards, which allows nav.navProps.className to
silently override the computed classes; fix by explicitly merging class names
instead of relying on prop order: destructure nav.navProps into ({ className:
navClassName, ...navRest }) and pass className={cn(<existing class string>,
navClassName)} to <LayoutHeader>, then spread {...navRest}; reference
LayoutHeader and nav.navProps (and the cn helper) when making the change.
In `@packages/base-ui/src/layouts/docs/page/index.tsx`:
- Around line 56-59: The public API change makes props required and removes
className; change the DocsPage props so props?: ComponentProps<'article'>
(optional) and preserve a deprecated className?: string that maps into
props.className when present; update the DocsPage component implementation to
prefer props.className but still support the legacy className prop (copy/merge
it into the final props passed to the '#nd-page' article), and mark className as
deprecated in the type/comments so callers remain compatible while you migrate
to props.
In `@packages/base-ui/src/layouts/flux/index.tsx`:
- Around line 139-144: The SearchToggle JSX spreads searchToggle.smProps after
explicitly setting className which allows an incoming smProps.className to
override the merged classes; update the prop order so that
{...searchToggle.smProps} comes first and className={cn('rounded-lg',
searchToggle.smProps?.className)} is applied last (or merge smProps.className
into cn before spreading) to ensure SearchToggle always receives the base
'rounded-lg' class while still preserving any additional classes from
searchToggle.smProps.
- Around line 150-156: The current JSX passes {...themeSwitch.props} after
setting className with cn(...), so any className inside themeSwitch.props will
overwrite the merged classes; fix by extracting and merging the incoming
className explicitly (e.g. const { className: incoming, ...rest } =
themeSwitch.props) and then call className={cn('px-1 py-0 h-full ms-1 rounded-xl
bg-fd-muted *:rounded-lg', incoming, themeSwitch.props?.className)} or simpler
className={cn('px-1 py-0 h-full ms-1 rounded-xl bg-fd-muted *:rounded-lg',
incoming)} and spread {...rest} instead of {...themeSwitch.props}; update the
element that uses themeSwitch.props (the ThemeToggle usage) to use the merged
className and spread the rest.
In `@packages/base-ui/src/layouts/flux/page/index.tsx`:
- Around line 55-58: The public API change makes props required and removes
className; revert to a non-breaking API by making props optional (change props:
ComponentProps<'article'> to props?: ComponentProps<'article'>) and retain
className as a deprecated alias by reading className from the component props
(e.g., accept className?: string on the component props or destructure it) and
merging it into the returned props object (set props.className =
[props.className, className].filter(Boolean).join(' ') when building the
article/container props). Update any references in the DocsPage/Flux page
component to use the optional props object and prefer props.className when both
are present so existing call sites that only pass className continue to work.
In `@packages/base-ui/src/layouts/home/client.tsx`:
- Around line 126-132: The mobile SearchToggle currently spreads
searchToggle.smProps after passing className, allowing smProps.className to
override the default class; fix by spreading {...searchToggle.smProps} first and
then passing className={cn('p-2', searchToggle.smProps?.className)} (or ensure
className is the last prop) so the default 'p-2' is always included and
smProps.className is merged rather than replacing it; update the SearchToggle
invocation that references searchToggle.smProps and the SearchToggle component
usage accordingly.
- Around line 74-76: The className merge is being overwritten because
{...nav.navProps} is spread after you set className; change the order or
incorporate nav.navProps.className into the merged value so the final className
includes both sources. Specifically, update the JSX around the element using
cn(...) and nav.navProps so that either the spread occurs before you set
className or you call cn(nav.navProps?.className, s.open && 'shadow-lg
rounded-b-2xl', nav.navProps && otherClasses) / include nav.navProps.className
in the cn call (referencing nav.navProps, cn, and className) to ensure
nav.navProps.className does not override the merged className.
- Around line 93-99: LargeSearchToggle's className is being set and then
potentially overwritten by spreading searchToggle.lgProps which may contain its
own className; fix by extracting className from searchToggle.lgProps, combine it
with cn(...) into a single className prop, and spread the remaining lgProps
without its className. Locate the usage referencing searchToggle.lgProps and cn
(and the LargeSearchToggle/related component) and replace the current
{...searchToggle.lgProps} spread with a destructure/omit that removes className
so the computed className prop is the definitive value.
In `@packages/base-ui/src/layouts/notebook/index.tsx`:
- Around line 368-374: The mobile SearchToggle currently spreads
{...searchToggle.smProps} after setting className so any className in smProps
will overwrite the computed cn('p-2', ...), causing the same override bug; fix
by excluding className from the spread and explicitly merging it into the final
className (e.g. extract className from searchToggle.smProps into smClassName,
spread the rest, and set className={cn('p-2', smClassName)}) so the padding
class is preserved and user-provided classes are appended.
- Around line 394-401: The current JSX passes className before spreading
themeSwitch.props so any className inside themeSwitch.props will override the
explicit className; fix by spreading themeSwitch.props first and then passing
className after the spread so the explicit className wins (i.e., update the
ThemeToggle usage: <ThemeToggle mode={themeSwitch.mode ?? 'light-dark-system'}
{...themeSwitch.props} className={themeSwitch.props?.className} />), referencing
ThemeToggle and themeSwitch.props to locate the change.
In `@packages/base-ui/src/layouts/notebook/page/index.tsx`:
- Around line 56-59: The public API changed by making props required; revert to
a non-breaking change by making props optional (props?:
ComponentProps<'article'>) and reintroducing a deprecated className?: string
prop as an alias; in the component that renders the `#nd-page` container (the
DocsPage / page component), merge className into the article props when present
(e.g., const mergedProps = {...props, ...(className ? {className} : {})}) so
existing callers that only pass className continue to work while new callers can
pass props; update types and runtime merging where ComponentProps<'article'> and
props are used.
In `@packages/radix-ui/src/layouts/docs/page/index.tsx`:
- Around line 56-60: The DocsPageProps interface currently requires props which
makes this a breaking change; update the declaration of DocsPageProps.props to
be optional (change props: ComponentProps<'article'> to props?:
ComponentProps<'article'>) and apply the same optional modifier to the second
occurrence noted around the other interface/usage (the other DocsPageProps-like
declaration at lines 107-108) so existing callers can omit the props object
without breaking.
In `@packages/radix-ui/src/layouts/flux/index.tsx`:
- Around line 150-155: SearchToggle's explicit className={cn('rounded-lg',
searchToggle.smProps?.className)} is being placed before spreading
{...searchToggle.smProps}, so any className in smProps will override the merged
classes; fix by either spreading {...searchToggle.smProps} first and then
passing className (so the explicit className wins), or better: compute the final
className using cn('rounded-lg', searchToggle.smProps?.className,
searchToggle.smProps && searchToggle.smProps.className) and pass that as the
className while omitting className from the spread (or merge smProps.className
into the cn call) to ensure 'rounded-lg' is always included; update the JSX
around SearchToggle and the smProps spread accordingly.
- Around line 160-167: ThemeToggle's className is being passed and then
themeSwitch.props is spread, which can override the computed className; fix by
extracting className from themeSwitch.props (e.g., const { className:
userClassName, ...rest } = themeSwitch.props || {}) and pass className={cn('p-1
h-full ms-1 rounded-xl bg-fd-muted *:rounded-lg', userClassName)} to ThemeToggle
while spreading {...rest} afterwards, keeping mode={themeSwitch.mode} as-is so
the computed classes are merged but not overwritten.
In `@packages/radix-ui/src/layouts/flux/page/index.tsx`:
- Around line 55-58: The change made the props field required on the component
(props: ComponentProps<'article'>) which breaks existing call sites that only
pass className; make props optional (props?: ComponentProps<'article'>) and
preserve className as a deprecated alias by mapping any provided className into
props (e.g., if className is present and props is undefined, set props = {
className }) and prefer props when both exist; update the public type and
component implementation (references: props, className, DocsPage) so consumers
continue to work while emitting a deprecation note for className.
In `@packages/radix-ui/src/layouts/home/client.tsx`:
- Around line 74-80: The prop spread {...searchToggle.lgProps} is placed after
explicit props on LargeSearchToggle and ThemeToggle which lets callers override
composed className and critical flags like hideIfDisabled/mode; fix by moving
the spread before the explicit props or by merging className and explicitly
setting hideIfDisabled/mode after the spread so explicit values win (e.g.,
ensure LargeSearchToggle uses cn('w-full ...', searchToggle.lgProps?.className)
and that hideIfDisabled is applied after spreading lgProps; apply the same
pattern for the ThemeToggle fallbacks and their mode prop).
In `@packages/radix-ui/src/layouts/notebook/page/index.tsx`:
- Around line 56-59: Change the public API to avoid a breaking change: make the
props field optional (props?: ComponentProps<'article'>) and add a deprecated
className?: string alias on the component props so existing DocsPage call sites
still work; in the component implementation (where props is spread into the
`#nd-page` container) merge the deprecated className into props.className (e.g.,
compute mergedClassName = [props?.className, className].filter(Boolean).join('
')) and spread the resulting props object, ensuring className precedence is
preserved and no runtime break occurs.
---
Outside diff comments:
In `@packages/base-ui/src/layouts/notebook/index.tsx`:
- Around line 283-291: The combined className passed to LayoutHeader can be
overridden because {...nav.navProps} is spread after the explicit className; to
fix, ensure nav.navProps.className is merged into the computed value (using cn)
or spread nav.navProps before the className prop so the final className is the
merged result; update the LayoutHeader usage that builds className (reference
LayoutHeader, nav.navProps, className, cn, showLayoutTabs) to either include
nav.navProps.className in the cn(...) call or move {...nav.navProps} before the
className prop so no className from nav.navProps can overwrite the merged
classes.
- Around line 332-340: The className from searchToggle.lgProps is being merged
into cn(...) but then searchToggle.lgProps is spread after, which will override
the computed className; update the JSX for LargeSearchToggle so the spreaded
lgProps does not clobber the computed className — either spread
{...searchToggle.lgProps} before the explicit className prop or omit className
from the spread (e.g., extract const { className, ...rest } =
searchToggle.lgProps and spread rest) and keep className={cn(..., className)} to
ensure the merged classes are preserved.
In `@packages/radix-ui/src/layouts/docs/page/index.tsx`:
- Around line 147-156: The composed className on the article and TOC containers
is being clobbered by the subsequent spread of props/tocProps; update the JSX so
you either (A) spread {...props} / {...tocProps} before the element's explicit
className or (B) explicitly merge incoming className into your cn call (e.g.
include props?.className / tocProps?.className inside the cn invocation) instead
of relying on the spread order; locate the article element using
className={cn(...)} and the TOC container with tocProps and adjust the
spread/order so base layout classes from cn are preserved.
---
Nitpick comments:
In `@packages/base-ui/src/layouts/shared/index.tsx`:
- Line 5: The import of ButtonProps should be a type-only import to aid
tree-shaking and avoid pulling runtime module code: replace the current import
of ButtonProps with a type import (use the TypeScript "import type { ButtonProps
}" form) wherever ButtonProps is referenced in this file (e.g., in
packages/base-ui/src/layouts/shared/index.tsx) and ensure no runtime usage
requires the actual module so the change is safe.
- Around line 46-58: The searchToggle prop types are asymmetric: smProps is
typed as Omit<ComponentProps<'button'>, 'color'> & ButtonProps while lgProps is
only ComponentProps<'button'>; align them by either giving lgProps the same type
as smProps or removing ButtonProps from smProps so both use the same shape.
Update the searchToggle definition to use a single consistent type reference
(e.g., make lgProps Omit<ComponentProps<'button'>, 'color'> & ButtonProps or
change smProps to ComponentProps<'button'>) and ensure consumers of
searchToggle, smProps, and lgProps compile against the unified type.
In `@packages/radix-ui/src/layouts/shared/index.tsx`:
- Around line 46-58: The types for searchToggle.smProps and searchToggle.lgProps
are inconsistent: smProps is typed as Omit<ComponentProps<'button'>, 'color'> &
ButtonProps while lgProps is ComponentProps<'button'>; update lgProps to match
smProps (ComponentProps<'button'> with the same Omit and & ButtonProps) if the
LargeSearchToggle component also accepts ButtonProps, or add a concise comment
above the searchToggle type explaining why lgProps intentionally differs; refer
to the symbols smProps, lgProps and the LargeSearchToggle consumer to make the
change or add the documentation.
- Line 5: The import of ButtonProps is used only for typing; change the import
to a type-only import by replacing the regular import with an "import type {
ButtonProps } from '@/components/ui/button';" so the compiler can erase it and
improve tree-shaking; locate the import statement that currently reads "import {
ButtonProps } ..." in the shared layout module and update it to the type-only
form.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3bba2d5a-f703-4786-b7cf-3182ec2cb7e5
📒 Files selected for processing (17)
.changeset/gold-mice-pump.mdpackages/base-ui/src/layouts/docs/index.tsxpackages/base-ui/src/layouts/docs/page/index.tsxpackages/base-ui/src/layouts/flux/index.tsxpackages/base-ui/src/layouts/flux/page/index.tsxpackages/base-ui/src/layouts/home/client.tsxpackages/base-ui/src/layouts/notebook/index.tsxpackages/base-ui/src/layouts/notebook/page/index.tsxpackages/base-ui/src/layouts/shared/index.tsxpackages/radix-ui/src/layouts/docs/index.tsxpackages/radix-ui/src/layouts/docs/page/index.tsxpackages/radix-ui/src/layouts/flux/index.tsxpackages/radix-ui/src/layouts/flux/page/index.tsxpackages/radix-ui/src/layouts/home/client.tsxpackages/radix-ui/src/layouts/notebook/index.tsxpackages/radix-ui/src/layouts/notebook/page/index.tsxpackages/radix-ui/src/layouts/shared/index.tsx
Comment on lines
+162
to
+166
| <ThemeToggle | ||
| className={cn('ms-auto p-0', themeSwitch.props?.className)} | ||
| mode={themeSwitch.mode} | ||
| {...themeSwitch.props} | ||
| /> |
There was a problem hiding this comment.
Potential className override when spreading themeSwitch.props.
Spreading {...themeSwitch.props} after setting className={cn(...)} will override the merged className if props contains a className.
🔧 Proposed fix
<ThemeToggle
+ {...themeSwitch.props}
className={cn('ms-auto p-0', themeSwitch.props?.className)}
mode={themeSwitch.mode}
- {...themeSwitch.props}
/>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/base-ui/src/layouts/docs/index.tsx` around lines 162 - 166, The
className passed to ThemeToggle can be overridden by themeSwitch.props when you
spread {...themeSwitch.props} afterwards; fix by extracting/merging the
className from themeSwitch.props and then spread the remaining props without
their className so the merged className is always applied. Specifically,
reference ThemeToggle, cn, and themeSwitch.props: compute a mergedClassName =
cn('ms-auto p-0', themeSwitch.props?.className) and spread only the rest of
themeSwitch.props (excluding its className) into ThemeToggle while passing
className={mergedClassName}.
Comment on lines
+201
to
+207
| (themeSwitch.component ?? ( | ||
| <ThemeToggle | ||
| className={cn('p-0', themeSwitch.props?.className)} | ||
| mode={themeSwitch.mode} | ||
| {...themeSwitch.props} | ||
| /> | ||
| ))} |
There was a problem hiding this comment.
Same className override pattern with ThemeToggle in SidebarDrawer.
🔧 Proposed fix
<ThemeToggle
+ {...themeSwitch.props}
className={cn('p-0', themeSwitch.props?.className)}
mode={themeSwitch.mode}
- {...themeSwitch.props}
/>📝 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
| (themeSwitch.component ?? ( | |
| <ThemeToggle | |
| className={cn('p-0', themeSwitch.props?.className)} | |
| mode={themeSwitch.mode} | |
| {...themeSwitch.props} | |
| /> | |
| ))} | |
| (themeSwitch.component ?? ( | |
| <ThemeToggle | |
| {...themeSwitch.props} | |
| className={cn('p-0', themeSwitch.props?.className)} | |
| mode={themeSwitch.mode} | |
| /> | |
| ))} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/base-ui/src/layouts/docs/index.tsx` around lines 201 - 207, The
SidebarDrawer rendering of ThemeToggle uses the wrong className override order;
replicate the pattern used in docs/index.tsx by passing className={cn('p-0',
themeSwitch.props?.className)} and spreading themeSwitch.props after that so the
default 'p-0' is preserved and any additional classes are merged (refer to
ThemeToggle, SidebarDrawer, themeSwitch.props, and cn). Ensure the explicit
className prop is set on the ThemeToggle element (not overwritten by the spread)
and if a custom component is provided via themeSwitch.component, apply the same
merged className logic there as well.
Comment on lines
237
to
244
| <LayoutHeader | ||
| id="nd-subnav" | ||
| className="[grid-area:header] sticky top-(--fd-docs-row-1) z-30 flex items-center ps-4 pe-2.5 border-b transition-colors backdrop-blur-sm h-(--fd-header-height) md:hidden max-md:layout:[--fd-header-height:--spacing(14)] data-[transparent=false]:bg-fd-background/80" | ||
| className={cn( | ||
| '[grid-area:header] sticky top-(--fd-docs-row-1) z-30 flex items-center ps-4 pe-2.5 border-b transition-colors backdrop-blur-sm h-(--fd-header-height) md:hidden max-md:layout:[--fd-header-height:--spacing(14)] data-[transparent=false]:bg-fd-background/80', | ||
| nav.navProps?.className, | ||
| )} | ||
| {...nav.navProps} | ||
| > |
There was a problem hiding this comment.
Same className override pattern with LayoutHeader.
🔧 Proposed fix
<LayoutHeader
id="nd-subnav"
+ {...nav.navProps}
className={cn(
'[grid-area:header] sticky top-(--fd-docs-row-1) z-30 flex items-center ps-4 pe-2.5 border-b transition-colors backdrop-blur-sm h-(--fd-header-height) md:hidden max-md:layout:[--fd-header-height:--spacing(14)] data-[transparent=false]:bg-fd-background/80',
nav.navProps?.className,
)}
- {...nav.navProps}
>📝 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
| <LayoutHeader | |
| id="nd-subnav" | |
| className="[grid-area:header] sticky top-(--fd-docs-row-1) z-30 flex items-center ps-4 pe-2.5 border-b transition-colors backdrop-blur-sm h-(--fd-header-height) md:hidden max-md:layout:[--fd-header-height:--spacing(14)] data-[transparent=false]:bg-fd-background/80" | |
| className={cn( | |
| '[grid-area:header] sticky top-(--fd-docs-row-1) z-30 flex items-center ps-4 pe-2.5 border-b transition-colors backdrop-blur-sm h-(--fd-header-height) md:hidden max-md:layout:[--fd-header-height:--spacing(14)] data-[transparent=false]:bg-fd-background/80', | |
| nav.navProps?.className, | |
| )} | |
| {...nav.navProps} | |
| > | |
| <LayoutHeader | |
| id="nd-subnav" | |
| {...nav.navProps} | |
| className={cn( | |
| '[grid-area:header] sticky top-(--fd-docs-row-1) z-30 flex items-center ps-4 pe-2.5 border-b transition-colors backdrop-blur-sm h-(--fd-header-height) md:hidden max-md:layout:[--fd-header-height:--spacing(14)] data-[transparent=false]:bg-fd-background/80', | |
| nav.navProps?.className, | |
| )} | |
| > |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/base-ui/src/layouts/docs/index.tsx` around lines 237 - 244, The
LayoutHeader is passing a computed className and then spreading nav.navProps
afterwards, which allows nav.navProps.className to silently override the
computed classes; fix by explicitly merging class names instead of relying on
prop order: destructure nav.navProps into ({ className: navClassName, ...navRest
}) and pass className={cn(<existing class string>, navClassName)} to
<LayoutHeader>, then spread {...navRest}; reference LayoutHeader and
nav.navProps (and the cn helper) when making the change.
Comment on lines
+251
to
256
| <SearchToggle | ||
| className={cn('p-2', searchToggle.smProps?.className)} | ||
| hideIfDisabled | ||
| {...searchToggle.smProps} | ||
| /> | ||
| ))} |
There was a problem hiding this comment.
Same className override pattern with SearchToggle.
🔧 Proposed fix
<SearchToggle
+ {...searchToggle.smProps}
className={cn('p-2', searchToggle.smProps?.className)}
hideIfDisabled
- {...searchToggle.smProps}
/>📝 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
| <SearchToggle | |
| className={cn('p-2', searchToggle.smProps?.className)} | |
| hideIfDisabled | |
| {...searchToggle.smProps} | |
| /> | |
| ))} | |
| <SearchToggle | |
| {...searchToggle.smProps} | |
| className={cn('p-2', searchToggle.smProps?.className)} | |
| hideIfDisabled | |
| /> | |
| ))} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/base-ui/src/layouts/docs/index.tsx` around lines 251 - 256, The
SearchToggle is currently given className first then {...searchToggle.smProps},
which can overwrite the merged className; fix by spreading searchToggle.smProps
before the explicit className so the merged className (cn('p-2',
searchToggle.smProps?.className)) is applied last. Update the JSX for
SearchToggle to place {...searchToggle.smProps} before the className prop (still
using cn and hideIfDisabled) so the intended class merging works correctly.
Comment on lines
+56
to
+59
| /** | ||
| * Apply class names to the `#nd-page` container. | ||
| * Apply props to the `#nd-page` container. | ||
| */ | ||
| className?: string; | ||
| props: ComponentProps<'article'>; |
There was a problem hiding this comment.
This public API change is breaking on a minor release.
This replaces the existing className customization path with a required props object, so current DocsPage call sites break even when they only set a class name. Please keep className as a deprecated alias for now and make props optional.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/base-ui/src/layouts/docs/page/index.tsx` around lines 56 - 59, The
public API change makes props required and removes className; change the
DocsPage props so props?: ComponentProps<'article'> (optional) and preserve a
deprecated className?: string that maps into props.className when present;
update the DocsPage component implementation to prefer props.className but still
support the legacy className prop (copy/merge it into the final props passed to
the '#nd-page' article), and mark className as deprecated in the type/comments
so callers remain compatible while you migrate to props.
Comment on lines
+150
to
155
| <SearchToggle | ||
| className={cn('rounded-lg', searchToggle.smProps?.className)} | ||
| hideIfDisabled | ||
| {...searchToggle.smProps} | ||
| /> | ||
| ))} |
There was a problem hiding this comment.
Potential className override when spreading smProps.
Same issue as in packages/base-ui/src/layouts/flux/index.tsx: spreading {...searchToggle.smProps} after className={cn(...)} will override the merged className if smProps contains a className property.
🔧 Proposed fix
<SearchToggle
- className={cn('rounded-lg', searchToggle.smProps?.className)}
- hideIfDisabled
{...searchToggle.smProps}
+ className={cn('rounded-lg', searchToggle.smProps?.className)}
+ hideIfDisabled
/>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/radix-ui/src/layouts/flux/index.tsx` around lines 150 - 155,
SearchToggle's explicit className={cn('rounded-lg',
searchToggle.smProps?.className)} is being placed before spreading
{...searchToggle.smProps}, so any className in smProps will override the merged
classes; fix by either spreading {...searchToggle.smProps} first and then
passing className (so the explicit className wins), or better: compute the final
className using cn('rounded-lg', searchToggle.smProps?.className,
searchToggle.smProps && searchToggle.smProps.className) and pass that as the
className while omitting className from the spread (or merge smProps.className
into the cn call) to ensure 'rounded-lg' is always included; update the JSX
around SearchToggle and the smProps spread accordingly.
Comment on lines
160
to
167
| <ThemeToggle | ||
| className="p-1 h-full ms-1 rounded-xl bg-fd-muted *:rounded-lg" | ||
| className={cn( | ||
| 'p-1 h-full ms-1 rounded-xl bg-fd-muted *:rounded-lg', | ||
| themeSwitch.props?.className, | ||
| )} | ||
| mode={themeSwitch.mode} | ||
| {...themeSwitch.props} | ||
| /> |
There was a problem hiding this comment.
Same className override issue with ThemeToggle.
🔧 Proposed fix
<ThemeToggle
- className={cn(
- 'p-1 h-full ms-1 rounded-xl bg-fd-muted *:rounded-lg',
- themeSwitch.props?.className,
- )}
- mode={themeSwitch.mode}
{...themeSwitch.props}
+ className={cn(
+ 'p-1 h-full ms-1 rounded-xl bg-fd-muted *:rounded-lg',
+ themeSwitch.props?.className,
+ )}
+ mode={themeSwitch.mode}
/>📝 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
| <ThemeToggle | |
| className="p-1 h-full ms-1 rounded-xl bg-fd-muted *:rounded-lg" | |
| className={cn( | |
| 'p-1 h-full ms-1 rounded-xl bg-fd-muted *:rounded-lg', | |
| themeSwitch.props?.className, | |
| )} | |
| mode={themeSwitch.mode} | |
| {...themeSwitch.props} | |
| /> | |
| <ThemeToggle | |
| {...themeSwitch.props} | |
| className={cn( | |
| 'p-1 h-full ms-1 rounded-xl bg-fd-muted *:rounded-lg', | |
| themeSwitch.props?.className, | |
| )} | |
| mode={themeSwitch.mode} | |
| /> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/radix-ui/src/layouts/flux/index.tsx` around lines 160 - 167,
ThemeToggle's className is being passed and then themeSwitch.props is spread,
which can override the computed className; fix by extracting className from
themeSwitch.props (e.g., const { className: userClassName, ...rest } =
themeSwitch.props || {}) and pass className={cn('p-1 h-full ms-1 rounded-xl
bg-fd-muted *:rounded-lg', userClassName)} to ThemeToggle while spreading
{...rest} afterwards, keeping mode={themeSwitch.mode} as-is so the computed
classes are merged but not overwritten.
Comment on lines
55
to
+58
| /** | ||
| * Apply class names to the `#nd-page` container. | ||
| * Apply props to the `#nd-page` container. | ||
| */ | ||
| className?: string; | ||
| props: ComponentProps<'article'>; |
There was a problem hiding this comment.
This public API change is breaking on a minor release.
This replaces the existing className customization path with a required props object, so current DocsPage call sites break even when they only set a class name. Please keep className as a deprecated alias for now and make props optional.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/radix-ui/src/layouts/flux/page/index.tsx` around lines 55 - 58, The
change made the props field required on the component (props:
ComponentProps<'article'>) which breaks existing call sites that only pass
className; make props optional (props?: ComponentProps<'article'>) and preserve
className as a deprecated alias by mapping any provided className into props
(e.g., if className is present and props is undefined, set props = { className
}) and prefer props when both exist; update the public type and component
implementation (references: props, className, DocsPage) so consumers continue to
work while emitting a deprecation note for className.
Comment on lines
74
to
+80
| <LargeSearchToggle | ||
| className="w-full rounded-full ps-2.5 max-w-[240px]" | ||
| className={cn( | ||
| 'w-full rounded-full ps-2.5 max-w-[240px]', | ||
| searchToggle.lgProps?.className, | ||
| )} | ||
| hideIfDisabled | ||
| {...searchToggle.lgProps} |
There was a problem hiding this comment.
❓ Verification inconclusive
🏁 Script executed:
cat -n packages/radix-ui/src/layouts/home/client.tsx | sed -n '70,160p'Repository: fuma-nama/fumadocs
Length of output: 1848
Prop spread ordering allows overrides of earlier explicit props.
The spread {...searchToggle.lgProps} can override the composed className and the hideIfDisabled prop if those keys exist in lgProps. Same applies to the ThemeToggle fallbacks and the mode prop at the other flagged lines.
Also applies to: 84-89, 108-113, 153-158
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/radix-ui/src/layouts/home/client.tsx` around lines 74 - 80, The prop
spread {...searchToggle.lgProps} is placed after explicit props on
LargeSearchToggle and ThemeToggle which lets callers override composed className
and critical flags like hideIfDisabled/mode; fix by moving the spread before the
explicit props or by merging className and explicitly setting
hideIfDisabled/mode after the spread so explicit values win (e.g., ensure
LargeSearchToggle uses cn('w-full ...', searchToggle.lgProps?.className) and
that hideIfDisabled is applied after spreading lgProps; apply the same pattern
for the ThemeToggle fallbacks and their mode prop).
Comment on lines
56
to
+59
| /** | ||
| * Apply class names to the `#nd-page` container. | ||
| * Apply props to the `#nd-page` container. | ||
| */ | ||
| className?: string; | ||
| props: ComponentProps<'article'>; |
There was a problem hiding this comment.
This public API change is breaking on a minor release.
This replaces the existing className customization path with a required props object, so current DocsPage call sites break even when they only set a class name. Please keep className as a deprecated alias for now and make props optional.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/radix-ui/src/layouts/notebook/page/index.tsx` around lines 56 - 59,
Change the public API to avoid a breaking change: make the props field optional
(props?: ComponentProps<'article'>) and add a deprecated className?: string
alias on the component props so existing DocsPage call sites still work; in the
component implementation (where props is spread into the `#nd-page` container)
merge the deprecated className into props.className (e.g., compute
mergedClassName = [props?.className, className].filter(Boolean).join(' ')) and
spread the resulting props object, ensuring className precedence is preserved
and no runtime break occurs.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/radix-ui/src/layouts/docs/page/index.tsx (1)
147-156:⚠️ Potential issue | 🟠 MajorPreserve internal layout classes/ids when forwarding consumer props.
{...props}and{...tocProps}currently come afterclassName/id, so caller values can override core layout styles and#nd-page/#nd-tocidentifiers.💡 Proposed fix
export function DocsPage({ @@ toc = [], children, props, }: DocsPageProps) { + const { className: pageClassName, id: _pageId, ...pageProps } = props ?? {}; + const { className: tocClassName, id: _tocId, ...tocContainerProps } = tocProps ?? {}; @@ <article + {...pageProps} id="nd-page" data-full={full} className={cn( 'flex flex-col w-full max-w-[900px] mx-auto [grid-area:main] px-4 py-6 gap-4 md:px-6 md:pt-8 xl:px-8 xl:pt-14', full ? 'max-w-[1168px]' : 'xl:layout:[--fd-toc-width:268px]', - props?.className, + pageClassName, )} - {...props} > @@ <div + {...tocContainerProps} id="nd-toc" className={cn( 'sticky top-(--fd-docs-row-1) h-[calc(var(--fd-docs-height)-var(--fd-docs-row-1))] flex flex-col [grid-area:toc] w-(--fd-toc-width) pt-12 pe-4 pb-2 max-xl:hidden', - tocProps?.className, + tocClassName, )} - {...tocProps} >Also applies to: 163-170
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/radix-ui/src/layouts/docs/page/index.tsx` around lines 147 - 156, The spreads {...props} and {...tocProps} are placed after the component's core className/id which allows callers to override core layout classes and the `#nd-page/`#nd-toc identifiers; update the article and TOC element so consumer props do not override internal layout by either moving {...props} and {...tocProps} before the hard-coded id/className or by explicitly merging className and id (e.g. use cn(props.className, '...') and prefer internal id if props.id exists) for the elements with id "nd-page" and the TOC element that uses "nd-toc" so internal styles and identifiers always take precedence.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/radix-ui/src/layouts/docs/page/index.tsx`:
- Around line 147-156: The spreads {...props} and {...tocProps} are placed after
the component's core className/id which allows callers to override core layout
classes and the `#nd-page/`#nd-toc identifiers; update the article and TOC element
so consumer props do not override internal layout by either moving {...props}
and {...tocProps} before the hard-coded id/className or by explicitly merging
className and id (e.g. use cn(props.className, '...') and prefer internal id if
props.id exists) for the elements with id "nd-page" and the TOC element that
uses "nd-toc" so internal styles and identifiers always take precedence.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: aee2507f-8cbc-4e27-8e07-2b6e67c10a90
📒 Files selected for processing (1)
packages/radix-ui/src/layouts/docs/page/index.tsx
Owner
|
hmm there's a few problems that can cause breaking changes/bugs, maybe I can rewrite the changes later |
Contributor
Author
Which type of breaking change ? We just introduced support for |
Owner
|
https://github.com/fuma-nama/fumadocs/pull/3103/changes#r2929263075 |
Contributor
Author
Fixed!! |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/base-ui/src/layouts/docs/page/index.tsx (1)
147-156:⚠️ Potential issue | 🟠 MajorSpread operator will override composed
classNameprop.At lines 155 and 169,
{...props}and{...tocProps}are spread afterclassName={cn(...)}. If these objects contain aclassNameproperty, it will override the composed defaults and break core layout styling.Extract
classNamebefore spreading:✅ Recommended fix
export function DocsPage({ ... children, props, }: DocsPageProps) { + const { className: pageClassName, ...pageProps } = props ?? {}; + const { className: tocClassName, ...tocContainerProps } = tocProps ?? {}; + return wrapper( <> ... <article id="nd-page" data-full={full} className={cn( 'flex flex-col w-full max-w-[900px] mx-auto [grid-area:main] px-4 py-6 gap-4 md:px-6 md:pt-8 xl:px-8 xl:pt-14', full ? 'max-w-[1168px]' : 'xl:layout:[--fd-toc-width:268px]', - props?.className, + pageClassName, )} - {...props} + {...pageProps} > ... </article> {tocEnabled && (tocReplace ?? ( <div id="nd-toc" className={cn( 'sticky top-(--fd-docs-row-1) h-[calc(var(--fd-docs-height)-var(--fd-docs-row-1))] flex flex-col [grid-area:toc] w-(--fd-toc-width) pt-12 pe-4 pb-2 max-xl:hidden', - tocProps?.className, + tocClassName, )} - {...tocProps} + {...tocContainerProps} >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/base-ui/src/layouts/docs/page/index.tsx` around lines 147 - 156, The spreads {...props} and {...tocProps} come after className={cn(...)} which allows incoming className to silently override composed classes; extract className from props and tocProps (e.g., const { className: propsClassName, ...restProps } = props and const { className: tocClassName, ...restTocProps } = tocProps), then pass className={cn(composedClasses, propsClassName)} on the <article id="nd-page"> (and similarly merge composed TOC classes with tocClassName for the TOC element), and spread the remaining restProps/restTocProps after—this preserves defaults while allowing consumer className to be appended.
♻️ Duplicate comments (1)
packages/base-ui/src/layouts/docs/page/index.tsx (1)
56-60:⚠️ Potential issue | 🟠 MajorPreserve deprecated
classNamealias for minor-release compatibility.The public API still drops legacy
<DocsPage className="...">usage, which is a breaking change for existing consumers (the wrapper path inpackages/base-ui/src/page.tsxforwards rest props, but this component no longer reads top-levelclassName).💡 Suggested compatibility patch
export interface DocsPageProps { + /** + * `@deprecated` Use `props.className` instead. + */ + className?: string; + /** * Apply props to the `#nd-page` container. */ props?: ComponentProps<'article'>; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/base-ui/src/layouts/docs/page/index.tsx` around lines 56 - 60, The component removed the legacy top-level className prop causing a breaking change; restore compatibility by adding an optional className?: string to the component props type (e.g., DocsPageProps or the props interface) and ensure the render of the `#nd-page` container merges the top-level className and any props.className (or props passed through) when setting the element's className (use existing class merging utility if present, otherwise concatenate safely). This preserves the deprecated <DocsPage className="..."> behavior while keeping the existing props: ComponentProps<'article'> shape.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/base-ui/src/layouts/docs/page/index.tsx`:
- Around line 147-156: The spreads {...props} and {...tocProps} come after
className={cn(...)} which allows incoming className to silently override
composed classes; extract className from props and tocProps (e.g., const {
className: propsClassName, ...restProps } = props and const { className:
tocClassName, ...restTocProps } = tocProps), then pass
className={cn(composedClasses, propsClassName)} on the <article id="nd-page">
(and similarly merge composed TOC classes with tocClassName for the TOC
element), and spread the remaining restProps/restTocProps after—this preserves
defaults while allowing consumer className to be appended.
---
Duplicate comments:
In `@packages/base-ui/src/layouts/docs/page/index.tsx`:
- Around line 56-60: The component removed the legacy top-level className prop
causing a breaking change; restore compatibility by adding an optional
className?: string to the component props type (e.g., DocsPageProps or the props
interface) and ensure the render of the `#nd-page` container merges the top-level
className and any props.className (or props passed through) when setting the
element's className (use existing class merging utility if present, otherwise
concatenate safely). This preserves the deprecated <DocsPage className="...">
behavior while keeping the existing props: ComponentProps<'article'> shape.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2d36c362-db3f-48b1-9f0f-3bb481ad1c4e
📒 Files selected for processing (1)
packages/base-ui/src/layouts/docs/page/index.tsx
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
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.
This pull request introduces a minor version update for both
fumadocs-uiand@fumadocs/base-ui, focusing on extending existing components with additional configuration props. The changes improve flexibility and customization for layout and UI components, allowing consumers to pass extra props and class names to containers and toggles. The most important updates are grouped below.Component Prop Extensions:
DocsLayout,DocsPage,Header, andDocsNavbarcomponents, enabling greater customization through newpropsandnavPropsfields. [1] [2] [3] [4] [5] [6] [7] [8] [9] [10] [11] [12] [13]DocsPage API Changes:
classNameprop with a more genericpropsprop inDocsPagePropsfor layouts (docs,flux,notebook), allowing consumers to apply any validarticleelement props to the page container. [1] [2] [3]propssupport to table of contents containers and popovers, allowing additional customization for TOC elements. [1] [2] [3]Version and Release Notes:
.changeset/gold-mice-pump.mdfile to mark these as minor changes and document the extension of existing components with additional configuration props.Type Improvements:
ComponentPropsWithoutRefwhere appropriate, ensuring proper typing for extended props. [1] [2]These changes collectively make the UI library more adaptable and easier to integrate with custom requirements.
Summary by CodeRabbit