fix (Breadcrumb): ref not forwarded for trigger#694
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
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 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
🤖 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
paanSinghCoder
changed the title
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 public ref type for BreadcrumbItem is incorrect because
BreadcrumbItem is declared as forwardRef<HTMLAnchorElement, ...> but when
dropdownItems is present you pass that ref into Menu.Trigger (an
HTMLButtonElement) via ref; change the component's ref type to accurately
reflect both render paths—replace forwardRef<HTMLAnchorElement, ...> with a
widened ref type such as forwardRef<HTMLAnchorElement | HTMLButtonElement,
Props> or use a generic HTMLElement/Element ref (React.Ref<HTMLAnchorElement |
HTMLButtonElement> or React.Ref<HTMLElement>) so callers get the correct element
type, and update any related prop/ref typings and exports (e.g., BreadcrumbItem,
Menu.Trigger usage, and the props interface) to avoid casting the ref with "as".
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3d86560a-642e-4661-9486-862b1bbc2279
📒 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)} | ||
| {...(props as React.ButtonHTMLAttributes<HTMLButtonElement>)} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "BreadcrumbItem signature and dropdown ref cast:"
sed -n '20,65p' packages/raystack/components/breadcrumb/breadcrumb-item.tsx
echo
echo "Menu.Trigger definition and forwarded ref target:"
rg -n -C3 'forwardRef<|Trigger' packages/raystack/components/menuRepository: raystack/apsara
Length of output: 16283
Don't cast the public anchor ref to a button ref.
Line 59 keeps BreadcrumbItem publicly typed as forwardRef<HTMLAnchorElement>, but the dropdown path now writes that ref into Menu.Trigger as HTMLButtonElement. A caller with useRef<HTMLAnchorElement>() will receive a button when dropdownItems is set, so the exported type is now lying. Widen or split the ref type so it matches the rendered element instead of hiding the mismatch with a cast.
🤖 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
- 61, The public ref type for BreadcrumbItem is incorrect because BreadcrumbItem
is declared as forwardRef<HTMLAnchorElement, ...> but when dropdownItems is
present you pass that ref into Menu.Trigger (an HTMLButtonElement) via ref;
change the component's ref type to accurately reflect both render paths—replace
forwardRef<HTMLAnchorElement, ...> with a widened ref type such as
forwardRef<HTMLAnchorElement | HTMLButtonElement, Props> or use a generic
HTMLElement/Element ref (React.Ref<HTMLAnchorElement | HTMLButtonElement> or
React.Ref<HTMLElement>) so callers get the correct element type, and update any
related prop/ref typings and exports (e.g., BreadcrumbItem, Menu.Trigger usage,
and the props interface) to avoid casting the ref with "as".
| <Menu.Trigger | ||
| ref={ref as React.Ref<HTMLButtonElement>} | ||
| className={cx(styles['breadcrumb-dropdown-trigger'], className)} | ||
| {...(props as React.ButtonHTMLAttributes<HTMLButtonElement>)} |
There was a problem hiding this comment.
Let's not pass AnchorElement props to MenuTrigger
There was a problem hiding this comment.
Done. Passing only apt props.
2 participants
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
Refactor
Style