fix (breadcrumb): ref not forwarded for trigger#694
fix (breadcrumb): ref not forwarded for trigger#694paanSinghCoder wants to merge 2 commits intomainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughBreadcrumbItem's dropdown trigger now forwards a ref (cast to HTMLButtonElement) and forwards id, title, and ARIA attributes to the Menu.Trigger. Change is internal, public API and dropdown rendering remain unchanged. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~7 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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
🤖 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-item.tsx`:
- Around line 58-61: The component declares the forwarded ref as
HTMLAnchorElement but sometimes forwards it to Menu.Trigger (an
HTMLButtonElement), causing a type mismatch; update the ref type on the
component (the forwarded ref symbol `ref`) to widen it to either
`HTMLAnchorElement | HTMLButtonElement` or `HTMLElement` so callers' refs remain
correct, and adjust the prop/forwardRef signature where `ref` is declared (the
forwardRef in breadcrumb-item.tsx) to use the chosen union/HTMLElement type and
update any local casts so the `Menu.Trigger` usage no longer force-casts `ref`.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: adad8e04-c918-4548-baf2-9986ee0f80b1
📒 Files selected for processing (1)
packages/raystack/components/breadcrumb/breadcrumb-item.tsx
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)
55-82:⚠️ Potential issue | 🔴 CriticalDropdown path must wrap in
<li>element to maintain valid list semantics.The non-dropdown path wraps content in
<li className={...}>but the dropdown path returns<Menu>directly. Since the parentBreadcrumbRootrenders<ol>and expects<li>children, the dropdown path violates semantic HTML. This produces invalid DOM structure (<ol><Menu>...</Menu></ol>) and breaks accessibility expectations for breadcrumb navigation.🤖 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 55 - 82, The dropdown branch in breadcrumb-item.tsx returns <Menu> directly which breaks the parent <ol> semantics; wrap the existing Menu JSX inside the same <li className={cx(styles['breadcrumb-item'], className)}> wrapper used by the non-dropdown path so the component always renders an <li> child for BreadcrumbRoot. Keep the same ref usage on Menu.Trigger, preserve id/title/aria props and the dropdownItems mapping to Menu.Item, and ensure the className uses styles['breadcrumb-item'] (or the same variable used in the non-dropdown branch) so styling and semantics remain consistent.
🤖 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-item.tsx`:
- Around line 58-66: The dropdown branch only forwards specific props to
Menu.Trigger (id, title, aria-*) causing data-*, other ARIA attributes, and
event handlers to be dropped when dropdownItems is present; update the
Menu.Trigger call in breadcrumb-item.tsx (the dropdown path where Menu.Trigger
is rendered) to spread the remaining props (e.g., include ...props or a
filteredProps object) so that Menu.Trigger receives the same external attributes
and event handlers as the non-dropdown path (which uses ...props), ensuring
consistent prop forwarding for dropdownItems and non-dropdown variants.
---
Outside diff comments:
In `@packages/raystack/components/breadcrumb/breadcrumb-item.tsx`:
- Around line 55-82: The dropdown branch in breadcrumb-item.tsx returns <Menu>
directly which breaks the parent <ol> semantics; wrap the existing Menu JSX
inside the same <li className={cx(styles['breadcrumb-item'], className)}>
wrapper used by the non-dropdown path so the component always renders an <li>
child for BreadcrumbRoot. Keep the same ref usage on Menu.Trigger, preserve
id/title/aria props and the dropdownItems mapping to Menu.Item, and ensure the
className uses styles['breadcrumb-item'] (or the same variable used in the
non-dropdown branch) so styling and semantics remain consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 08e69747-c14b-4753-b829-bd9a6e0332e2
📒 Files selected for processing (1)
packages/raystack/components/breadcrumb/breadcrumb-item.tsx
| <Menu.Trigger | ||
| ref={ref as React.Ref<HTMLButtonElement>} | ||
| className={cx(styles['breadcrumb-dropdown-trigger'], className)} | ||
| id={props.id} | ||
| title={props.title} | ||
| aria-label={props['aria-label']} | ||
| aria-labelledby={props['aria-labelledby']} | ||
| aria-describedby={props['aria-describedby']} | ||
| > |
There was a problem hiding this comment.
Inconsistent prop forwarding between dropdown and non-dropdown paths.
The non-dropdown path spreads all props via ...props (line 94), but the dropdown path only forwards a subset (id, title, and three ARIA attributes). This means data-* attributes, other ARIA attributes, and event handlers passed by callers will be silently ignored when dropdownItems is provided.
Consider spreading the remaining props to Menu.Trigger for consistency, or document the limited prop support for the dropdown variant.
Suggested fix to forward remaining props
<Menu.Trigger
ref={ref as React.Ref<HTMLButtonElement>}
className={cx(styles['breadcrumb-dropdown-trigger'], className)}
- id={props.id}
- title={props.title}
- aria-label={props['aria-label']}
- aria-labelledby={props['aria-labelledby']}
- aria-describedby={props['aria-describedby']}
+ {...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.
| <Menu.Trigger | |
| ref={ref as React.Ref<HTMLButtonElement>} | |
| className={cx(styles['breadcrumb-dropdown-trigger'], className)} | |
| id={props.id} | |
| title={props.title} | |
| aria-label={props['aria-label']} | |
| aria-labelledby={props['aria-labelledby']} | |
| aria-describedby={props['aria-describedby']} | |
| > | |
| <Menu.Trigger | |
| ref={ref as React.Ref<HTMLButtonElement>} | |
| className={cx(styles['breadcrumb-dropdown-trigger'], className)} | |
| {...props} | |
| > |
🤖 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 58
- 66, The dropdown branch only forwards specific props to Menu.Trigger (id,
title, aria-*) causing data-*, other ARIA attributes, and event handlers to be
dropped when dropdownItems is present; update the Menu.Trigger call in
breadcrumb-item.tsx (the dropdown path where Menu.Trigger is rendered) to spread
the remaining props (e.g., include ...props or a filteredProps object) so that
Menu.Trigger receives the same external attributes and event handlers as the
non-dropdown path (which uses ...props), ensuring consistent prop forwarding for
dropdownItems and non-dropdown variants.
1 participant
Description
fix: Forward ref when BreadcrumbItem uses dropdown
fix: Props silently ignored in dropdown path
When
BreadcrumbItemis used withdropdownItems, the forwarded ref and other props was never attached and was effectively dropped. The ref and other props are now passed through toMenu.Trigger, so callers can attach a ref to the same interactive element in both link and dropdown variants.Type of Change
How Has This Been Tested?
Manual
Checklist:
Screenshots (if appropriate):
[Add screenshots here]
Related Issues
[Link any related issues here using #issue-number]
Summary by CodeRabbit
Release Notes