fix(Breadcrumb)!: BREAKING CHANGE use composition and remove cloneElement#695
fix(Breadcrumb)!: BREAKING CHANGE use composition and remove cloneElement#695paanSinghCoder merged 5 commits intomainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
paanSinghCoder
changed the title
paanSinghCoder
changed the title
rohanchkrabrty
left a comment
rohanchkrabrty
left a comment
There was a problem hiding this comment.
Let's support render prop using useRender from Base UI, instead of as.
Docs - https://base-ui.com/react/utils/use-render
📝 WalkthroughWalkthroughThe pull request migrates the breadcrumb component's polymorphic customization API from an Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
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📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/raystack/components/breadcrumb/breadcrumb-item.tsx (1)
15-20:⚠️ Potential issue | 🟠 MajorThe new polymorphic contract is only implemented at runtime.
BreadcrumbItemPropsis still fixed toComponentProps<'a'>, soas={NextLink}will still reject component-specific props even though the docs now say they pass through. The same anchor-shaped ref/props are then reused for thedropdownItemsbutton branch (lines 48–50) and thecurrentspan branch (lines 73–79), which cast the ref and spread anchor props onto elements that don't match. This letsonClick,tabIndex, and other anchor-specific props leak onto the "non-clickable" current item, and allowsaria-currentto be overridden by the props spread. Theasparameter only takes effect in the default link branch (lines 88–93), not in the dropdown or current branches. This needs a polymorphic type for theaspath and narrowed prop/ref surfaces for the non-link branches.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/raystack/components/breadcrumb/breadcrumb-item.tsx` around lines 15 - 20, BreadcrumbItemProps is currently tied to ComponentProps<'a'> so the polymorphic as prop isn't typed and anchor props/refs are leaking into non-link branches (dropdownItems button and current span) — change BreadcrumbItemProps into a proper polymorphic generic (e.g., BreadcrumbItemProps<E extends ElementType = 'a'>) and derive props/ref via ComponentPropsWithRef<E> or ComponentPropsWithoutRef<E> for the link branch; create narrowed discriminated unions or separate prop subsets for the dropdown branch (use correct button HTMLAttributes/Ref<HTMLButtonElement>) and the current branch (span props/Ref<HTMLSpanElement>) so you stop spreading anchor-specific props and refs onto non-anchor elements, keep the as prop and default 'a' for the generic, and update the render branches that reference dropdownItems, current and as to use the appropriately-typed props/refs.
🤖 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/raystack/components/breadcrumb/breadcrumb-root.tsx`:
- Around line 23-36: BreadcrumbRoot currently accepts a ref prop and passes it
directly to the <nav>, which breaks ref forwarding for React 18; wrap the
component with React.forwardRef and change the signature to accept (props, ref)
so the DOM ref is forwarded to the <nav> element. Update the export to use
forwardRef(BreadcrumbRoot) (or define const BreadcrumbRoot = forwardRef(function
BreadcrumbRoot(props, ref) { ... })), keep using breadcrumbVariants,
BreadcrumbProps and ariaLabel as before, and ensure the forwarded ref is passed
to the nav element and prop types updated if needed.
---
Outside diff comments:
In `@packages/raystack/components/breadcrumb/breadcrumb-item.tsx`:
- Around line 15-20: BreadcrumbItemProps is currently tied to
ComponentProps<'a'> so the polymorphic as prop isn't typed and anchor props/refs
are leaking into non-link branches (dropdownItems button and current span) —
change BreadcrumbItemProps into a proper polymorphic generic (e.g.,
BreadcrumbItemProps<E extends ElementType = 'a'>) and derive props/ref via
ComponentPropsWithRef<E> or ComponentPropsWithoutRef<E> for the link branch;
create narrowed discriminated unions or separate prop subsets for the dropdown
branch (use correct button HTMLAttributes/Ref<HTMLButtonElement>) and the
current branch (span props/Ref<HTMLSpanElement>) so you stop spreading
anchor-specific props and refs onto non-anchor elements, keep the as prop and
default 'a' for the generic, and update the render branches that reference
dropdownItems, current and as to use the appropriately-typed props/refs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e7d72d9e-a73e-402b-9e47-05d7505683b7
📒 Files selected for processing (7)
apps/www/src/components/playground/index.tsapps/www/src/content/docs/components/breadcrumb/demo.tsapps/www/src/content/docs/components/breadcrumb/index.mdxapps/www/src/content/docs/components/breadcrumb/props.tspackages/raystack/components/breadcrumb/__tests__/breadcrumb.test.tsxpackages/raystack/components/breadcrumb/breadcrumb-item.tsxpackages/raystack/components/breadcrumb/breadcrumb-root.tsx
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/www/src/content/docs/components/breadcrumb/index.mdx`:
- Around line 95-102: The example uses the element form render={<NextLink />}
which fails TypeScript because Next.js Link requires href; update the docs and
example for Breadcrumb.Item (component Breadcrumb.Item, prop render) to use the
function form so props are forwarded at render time (render={(props) =>
<NextLink {...props} />}) or else show the element form with an explicit href
(render={<NextLink href="/" />}); change the example text and code snippet
accordingly to recommend the callback form for components with required props
and include NextLink/ Breadcrumb.Item references.
In `@packages/raystack/components/breadcrumb/breadcrumb-item.tsx`:
- Around line 42-48: The destructuring in BreadcrumbItem currently pulls only
id, title and a few aria-* fields and drops all other props (so data-*,
tabIndex, onClick, etc. never reach Menu.Trigger); change the destructuring to
capture the rest (e.g. add ...rest to const { id, title, 'aria-label':
ariaLabel, 'aria-labelledby': ariaLabelledby, 'aria-describedby':
ariaDescribedby, ...rest } = props) and spread {...rest} onto Menu.Trigger (and
the analogous element in the other code path mentioned at lines 68-75) so
data-*, tabIndex and consumer handlers are forwarded unchanged to the trigger
element.
- Around line 98-105: The span currently applies aria-current='page' and then
spreads {...props}, which allows callers to override/remove aria-current or
reattach interactive handlers; fix by filtering incoming props (the `props`
object passed to the span) to a small whitelist of neutral attributes (e.g.,
className, id, data-* and non-interactive aria-* except aria-current) or
explicitly omit interactive props like onClick, tabIndex, role, href, and then
spread the filteredProps before setting aria-current; update the JSX in
breadcrumb-item.tsx (the span with ref, cx and styles['breadcrumb-link'] /
styles['breadcrumb-link-active']) so that filteredProps are spread first and
aria-current='page' is set last to guarantee the page semantics can't be
overridden.
- Line 16: BreadcrumbItemProps currently extends useRender.ComponentProps<'a'>
(anchor ref) but the component sometimes renders Menu.Trigger (button) or a
plain <span>, causing ref type mismatches; fix by making the props/ref contract
match render variants: either (A) split BreadcrumbItem into distinct variants
with a discriminated prop (e.g. kind: 'link' | 'dropdown' | 'current') and
define BreadcrumbItemProps as a union where the 'link' variant extends
useRender.ComponentProps<'a'> (ref: HTMLAnchorElement) while 'dropdown' extends
React.ComponentPropsWithRef<'button'> (ref: HTMLButtonElement) and 'current'
omits forwarded ref or uses React.Ref<HTMLSpanElement); or (B) remove ref
forwarding from non-anchor branches by only forwarding the ref to the anchor
path in the BreadcrumbItem forwardRef implementation and use internal refs for
Menu.Trigger and the <span>, updating usages of Menu.Trigger and the current
span render path accordingly; adjust type declarations and the BreadcrumbItem
forwardRef signature to reflect the chosen approach so consumers never receive
an anchor ref for a non-anchor element.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d825f004-70d3-4e50-a27c-d3431fc0b4c9
📒 Files selected for processing (6)
apps/www/src/components/playground/breadcrumb-examples.tsxapps/www/src/content/docs/components/breadcrumb/demo.tsapps/www/src/content/docs/components/breadcrumb/index.mdxapps/www/src/content/docs/components/breadcrumb/props.tspackages/raystack/components/breadcrumb/__tests__/breadcrumb.test.tsxpackages/raystack/components/breadcrumb/breadcrumb-item.tsx
✅ Files skipped from review due to trivial changes (1)
- apps/www/src/components/playground/breadcrumb-examples.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/www/src/content/docs/components/breadcrumb/demo.ts
- apps/www/src/content/docs/components/breadcrumb/props.ts
| ### Render | ||
|
|
||
| Use the `as` prop to render the breadcrumb item as a custom component. By default, breadcrumb items are rendered as `a` tags. | ||
| Use the `render` prop to render the breadcrumb item as a custom component. Pass a JSX element (e.g. `render={<NextLink />}`) and we will replace the default `<a>` while merging props. By default, items render as `a` tags. | ||
|
|
||
| When a custom component is provided, the props are merged, with the custom component's props taking precedence over the breadcrumb item's props. | ||
| ```tsx | ||
| // Correct: set href on the item (it will be forwarded into the rendered element) | ||
| <Breadcrumb.Item href="/" render={<NextLink />}>Home</Breadcrumb.Item> | ||
| ``` |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Base UI render prop prop merging behavior
💡 Result:
Base UI's render prop uses intelligent prop merging via the mergeProps utility to combine component props with user-provided render props. Props are merged left-to-right (rightmost wins for most keys), with special handling for className (concatenated right-to-left), style (deep merged, rightmost overwrites), event handlers (merged and executed right-to-left), and ref (rightmost ref kept). This ensures custom components receive Base UI's behavioral props (like event handlers, data attributes) while preserving user styles and handlers. In render prop usage: - Element form: <Component render={} /> - mergeProps automatically combines props. - Function form: render={(props) => <Custom {...props} />} or render={(props, state) => ...} - use mergeProps manually for merging, e.g., <Custom {...mergeProps(internalProps, userProps)} />. Call event.preventBaseUIHandler to prevent Base UI's internal logic. The useRender hook implements render prop logic, merging props before spreading on the rendered element.
Citations:
- 1: https://base-ui.com/react/utils/merge-props
- 2: https://base-ui.com/react/utils/use-render
- 3: https://base-ui.com/react/handbook/composition
🌐 Web query:
Next.js Link component href required prop
💡 Result:
Yes, the href prop is required for the Next.js Link component in both App Router and Pages Router.
Citations:
- 1: https://nextjs.org/docs/app/api-reference/components/link
- 2: https://nextjs.org/docs/pages/api-reference/components/link
- 3: https://nextjs.org/docs/13/app/api-reference/components/link
🏁 Script executed:
# Check the actual file and context around the example
fd -p "breadcrumb/index.mdx" apps/www/src/content/docs/components/Repository: raystack/apsara
Length of output: 116
🏁 Script executed:
# Read the actual markdown file to see the context and full example
cat -n apps/www/src/content/docs/components/breadcrumb/index.mdx | sed -n '90,110p'Repository: raystack/apsara
Length of output: 981
🏁 Script executed:
# Check if there are type definitions or examples elsewhere for breadcrumb
fd -p "breadcrumb" apps/www/src --type f | head -20Repository: raystack/apsara
Length of output: 288
🏁 Script executed:
# Check the breadcrumb component implementation
cat apps/www/src/components/playground/breadcrumb-examples.tsxRepository: raystack/apsara
Length of output: 1127
🏁 Script executed:
# Check the demo file to see if there are live examples
cat apps/www/src/content/docs/components/breadcrumb/demo.tsRepository: raystack/apsara
Length of output: 4341
🏁 Script executed:
# Check the actual Breadcrumb.Item component implementation to understand prop forwarding
fd -p "breadcrumb" apps/www --type f -e "ts" -e "tsx" | xargs grep -l "Breadcrumb.Item\|render" | head -5Repository: raystack/apsara
Length of output: 230
🏁 Script executed:
# Find the main component source files
fd -p "breadcrumb" --type f -e "ts" -e "tsx" | grep -v "node_modules\|dist" | head -20Repository: raystack/apsara
Length of output: 585
🏁 Script executed:
# Check the Breadcrumb.Item component implementation
cat packages/raystack/components/breadcrumb/breadcrumb-item.tsxRepository: raystack/apsara
Length of output: 3017
🏁 Script executed:
# Also check the main breadcrumb file
cat packages/raystack/components/breadcrumb/breadcrumb.tsxRepository: raystack/apsara
Length of output: 391
🏁 Script executed:
# Check the types used by useRender to understand render prop expectations
rg "useRender" packages/raystack/components/breadcrumb/ -A 5Repository: raystack/apsara
Length of output: 1712
🏁 Script executed:
# Check if there are any type definitions or interfaces for the render prop
rg "ComponentProps" packages/raystack/components/breadcrumb/ -B 2 -A 2Repository: raystack/apsara
Length of output: 2677
🏁 Script executed:
# Check if there's any tsconfig or type relaxations that might allow this
fd -p "tsconfig" --type fRepository: raystack/apsara
Length of output: 233
🏁 Script executed:
# Look for actual uses of NextLink in the codebase to see how they handle it
rg "NextLink" --type tsx --type ts -A 2 -B 2 | head -50Repository: raystack/apsara
Length of output: 86
🏁 Script executed:
# Look for actual uses of NextLink in the codebase
rg "NextLink" -A 2 -B 2 | head -80Repository: raystack/apsara
Length of output: 2773
🏁 Script executed:
# Check the tsconfig to see if there's any type relaxation
cat apps/www/tsconfig.jsonRepository: raystack/apsara
Length of output: 720
🏁 Script executed:
# Check the props.ts file to see the exact type definition for render
cat apps/www/src/content/docs/components/breadcrumb/props.tsRepository: raystack/apsara
Length of output: 1689
🏁 Script executed:
# Try to understand how the render prop type is defined - check if it allows optional props
rg "render\?" packages/raystack/components/breadcrumb/ -A 1 -B 1Repository: raystack/apsara
Length of output: 41
🏁 Script executed:
# Check Next.js Link type definition to confirm href is required
rg "export.*Link" node_modules/next/link.d.ts -A 10 | head -30Repository: raystack/apsara
Length of output: 126
🏁 Script executed:
# Check if there are any tests showing how render prop is used
cat packages/raystack/components/breadcrumb/__tests__/breadcrumb.test.tsx | head -100Repository: raystack/apsara
Length of output: 2916
Use the function form of the render prop for components with required props.
The example render={<NextLink />} will fail TypeScript type-checking because Next.js Link requires the href prop. Base UI's prop merging happens at runtime, after JSX evaluation. Use the callback form instead: render={(props) => <NextLink {...props} />}. If you want to keep the element form, explicitly provide href: render={<NextLink href="/" />}.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/www/src/content/docs/components/breadcrumb/index.mdx` around lines 95 -
102, The example uses the element form render={<NextLink />} which fails
TypeScript because Next.js Link requires href; update the docs and example for
Breadcrumb.Item (component Breadcrumb.Item, prop render) to use the function
form so props are forwarded at render time (render={(props) => <NextLink
{...props} />}) or else show the element form with an explicit href
(render={<NextLink href="/" />}); change the example text and code snippet
accordingly to recommend the callback form for components with required props
and include NextLink/ Breadcrumb.Item references.
| } | ||
|
|
||
| export interface BreadcrumbItemProps extends ComponentProps<'a'> { | ||
| export interface BreadcrumbItemProps extends useRender.ComponentProps<'a'> { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n packages/raystack/components/breadcrumb/breadcrumb-item.tsxRepository: raystack/apsara
Length of output: 3836
Fix the ref type contract across all render paths in BreadcrumbItem.
BreadcrumbItemProps extends useRender.ComponentProps<'a'>, declaring an anchor-typed ref. However, the dropdown branch at line 69 casts the ref to HTMLButtonElement (used in Menu.Trigger), and the current branch at line 99 casts to HTMLSpanElement (used in <span>). Consumers can pass an anchor-oriented ref and receive a different element type at runtime.
Either split the component variants to have distinct ref types for each case, or remove ref forwarding from the non-anchor branches.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/raystack/components/breadcrumb/breadcrumb-item.tsx` at line 16,
BreadcrumbItemProps currently extends useRender.ComponentProps<'a'> (anchor ref)
but the component sometimes renders Menu.Trigger (button) or a plain <span>,
causing ref type mismatches; fix by making the props/ref contract match render
variants: either (A) split BreadcrumbItem into distinct variants with a
discriminated prop (e.g. kind: 'link' | 'dropdown' | 'current') and define
BreadcrumbItemProps as a union where the 'link' variant extends
useRender.ComponentProps<'a'> (ref: HTMLAnchorElement) while 'dropdown' extends
React.ComponentPropsWithRef<'button'> (ref: HTMLButtonElement) and 'current'
omits forwarded ref or uses React.Ref<HTMLSpanElement); or (B) remove ref
forwarding from non-anchor branches by only forwarding the ref to the anchor
path in the BreadcrumbItem forwardRef implementation and use internal refs for
Menu.Trigger and the <span>, updating usages of Menu.Trigger and the current
span render path accordingly; adjust type declarations and the BreadcrumbItem
forwardRef signature to reflect the chosen approach so consumers never receive
an anchor ref for a non-anchor element.
| const { | ||
| id, | ||
| title, | ||
| 'aria-label': ariaLabel, | ||
| 'aria-labelledby': ariaLabelledby, | ||
| 'aria-describedby': ariaDescribedby | ||
| } = props; |
There was a problem hiding this comment.
Dropdown trigger props are being dropped.
This whitelist only forwards id, title, and a few aria-* fields into Menu.Trigger, so data-*, tabIndex, and consumer handlers like onClick never reach the trigger when dropdownItems is set. That regresses the normal prop-forwarding contract and makes the dropdown path behave differently from the link path.
Also applies to: 68-75
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/raystack/components/breadcrumb/breadcrumb-item.tsx` around lines 42
- 48, The destructuring in BreadcrumbItem currently pulls only id, title and a
few aria-* fields and drops all other props (so data-*, tabIndex, onClick, etc.
never reach Menu.Trigger); change the destructuring to capture the rest (e.g.
add ...rest to const { id, title, 'aria-label': ariaLabel, 'aria-labelledby':
ariaLabelledby, 'aria-describedby': ariaDescribedby, ...rest } = props) and
spread {...rest} onto Menu.Trigger (and the analogous element in the other code
path mentioned at lines 68-75) so data-*, tabIndex and consumer handlers are
forwarded unchanged to the trigger element.
| <span | ||
| ref={ref as React.RefObject<HTMLSpanElement>} | ||
| className={cx( | ||
| styles['breadcrumb-link'], | ||
| current && styles['breadcrumb-link-active'] | ||
| ), | ||
| href, | ||
| ...props, | ||
| ...renderedElement.props, | ||
| ref | ||
| }, | ||
| label | ||
| )} | ||
| </li> | ||
| styles['breadcrumb-link-active'] | ||
| )} | ||
| aria-current='page' | ||
| {...props} |
There was a problem hiding this comment.
current can be stripped of its page semantics.
Because {...props} is spread after aria-current='page', callers can override/remove the current-page state and also reattach interactive props like onClick or tabIndex to what is supposed to be a non-link page indicator. This branch should whitelist neutral attributes and set aria-current last.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/raystack/components/breadcrumb/breadcrumb-item.tsx` around lines 98
- 105, The span currently applies aria-current='page' and then spreads
{...props}, which allows callers to override/remove aria-current or reattach
interactive handlers; fix by filtering incoming props (the `props` object passed
to the span) to a small whitelist of neutral attributes (e.g., className, id,
data-* and non-interactive aria-* except aria-current) or explicitly omit
interactive props like onClick, tabIndex, role, href, and then spread the
filteredProps before setting aria-current; update the JSX in breadcrumb-item.tsx
(the span with ref, cx and styles['breadcrumb-link'] /
styles['breadcrumb-link-active']) so that filteredProps are spread first and
aria-current='page' is set last to guarantee the page semantics can't be
overridden.
2 participants
BREAKING CHANGE
Migration
as={<NextLink href="/" />}withas={NextLink}and movehref(and other link props) toBreadcrumb.Item, e.g.<Breadcrumb.Item href="/" as={NextLink}>Home</Breadcrumb.Item>.Summary
Changes
<NextLink href="/" />} (React element).as={NextLink}(component reference). Put href and other props on Breadcrumb.Item; they are passed through to the component.<Component ref={ref} className={...} href={href} {...props}>{label}</Component>(no cloneElement). Ref is forwarded to the link or custom component (except in the dropdown path).Current page
<span aria-current="page">instead of a link so the current page is not a clickable link.Link content
<a>Home</a>for simple items. With a leading icon, the existing span structure is kept for layout.Type of Change
How Has This Been Tested?
[Describe the tests that you ran to verify your changes]
Checklist:
Screenshots (if appropriate):
[Add screenshots here]
Related Issues
[Link any related issues here using #issue-number]
Summary by CodeRabbit
New Features
renderproparia-labelprop to breadcrumb root elementBug Fixes
<span>witharia-current="page"attributeDocumentation
renderprop API