fix (1-breadcrumb): docs style fixes#667
Merged
paanSinghCoder merged 6 commits intomainfrom Mar 23, 2026
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
Caution Review failedPull request was closed or merged during review 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:
📝 WalkthroughWalkthroughReworks Breadcrumb API, tests, docs, demos, and styles: adds Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant BreadcrumbItem
participant Menu
participant DOM
User->>BreadcrumbItem: click item
alt item has dropdown && not disabled
BreadcrumbItem->>Menu: open menu (map dropdownItems → Menu.Item)
Menu->>DOM: render Menu.Items (items may render as <a> via render/href)
User->>Menu: select Menu.Item → trigger action/navigation
else item is disabled or current
BreadcrumbItem->>DOM: render <span> (aria-current/aria-disabled, data attrs)
User->>BreadcrumbItem: click → no navigation, no menu
end
BreadcrumbItem->>DOM: render Separator (role="presentation", aria-hidden="true")
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
packages/raystack/components/breadcrumb/breadcrumb-root.tsx (3)
148-166:keyedhelper redundantly checksisValidElement.The
keyedfunction is only ever called with elements frombeforeItemsandafterItems, which are already validated asReactElementinstances fromparseBreadcrumbChildren. TheisValidElement(el)check is unnecessary.♻️ Optional simplification
- const keyed = (el: ReactElement, k: string) => - isValidElement(el) && el.key != null - ? el - : cloneElement(el, { key: k }); + const keyed = (el: ReactElement, k: string) => + el.key != null ? el : cloneElement(el, { key: k });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/raystack/components/breadcrumb/breadcrumb-root.tsx` around lines 148 - 166, The keyed helper unnecessarily calls isValidElement; since beforeItems/afterItems are already ReactElement instances from parseBreadcrumbChildren, remove the isValidElement check in the keyed function (breadcrumb-root.tsx) and simply return the element if el.key != null, otherwise return cloneElement(el, { key: k }); Update the keyed helper used where beforeItems/afterItems are iterated so it only checks el.key and clones when missing, keeping cloneElement usage and keys (e.g., calls in the beforeItems.forEach and afterItems.forEach loops) intact.
69-88: Consider handling unrecognized children gracefully.The
parseBreadcrumbChildrenfunction silently drops children that don't match known breadcrumb parts (Item, Separator, Ellipsis). This could lead to confusion if users pass custom elements or text nodes. Consider either logging a warning in development or documenting this behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/raystack/components/breadcrumb/breadcrumb-root.tsx` around lines 69 - 88, parseBreadcrumbChildren currently drops children that aren't recognized breadcrumb parts (BreadcrumbItem, BreadcrumbSeparator, BreadcrumbEllipsis) with no feedback; update parseBreadcrumbChildren to warn in development when an element is a valid React element but not matched by isBreadcrumbPart (and optionally when a non-empty text node or unknown node is passed) by calling console.warn or process.env.NODE_ENV check inside the flat.forEach loop; reference the existing symbols parseBreadcrumbChildren, flattenFragments(Children.toArray(children)), isValidElement, isBreadcrumbPart, BreadcrumbItem, BreadcrumbSeparator, and BreadcrumbEllipsis and emit a clear dev-only warning that includes the offending child/type so users know why their node was dropped.
171-173: Unused_propsChildrenextraction appears defensive but unnecessary.The destructuring on line 171-173 extracts
childrenfrompropsto avoid passing it twice to thenav. However,childrenwas already destructured at line 116, so it shouldn't be inprops. This suggests the typing could be tightened, but it's harmless as written.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/raystack/components/breadcrumb/breadcrumb-root.tsx` around lines 171 - 173, The extraction of children into _propsChildren from props (const { children: _propsChildren, ...restProps } = props) is redundant because children is already destructured earlier; remove that destructuring and ensure you use the existing children variable and a restProps that does not re-extract children (or tighten the prop typing to exclude children from props) so children is not handled twice in BreadcrumbRoot; update references to use restProps (or a propsWithoutChildren alias) accordingly.
🤖 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/app/examples/page.tsx`:
- Around line 61-66: The Breadcrumb.Item for the "Footwear" label has the wrong
href; update the href on the Breadcrumb.Item (the JSX element with label
"Footwear") to the correct path (e.g. change
'/products/electronics/laptops/gaming/accessories' to
'/products/electronics/laptops/gaming/accessories/footwear') so the link matches
the label.
---
Nitpick comments:
In `@packages/raystack/components/breadcrumb/breadcrumb-root.tsx`:
- Around line 148-166: The keyed helper unnecessarily calls isValidElement;
since beforeItems/afterItems are already ReactElement instances from
parseBreadcrumbChildren, remove the isValidElement check in the keyed function
(breadcrumb-root.tsx) and simply return the element if el.key != null, otherwise
return cloneElement(el, { key: k }); Update the keyed helper used where
beforeItems/afterItems are iterated so it only checks el.key and clones when
missing, keeping cloneElement usage and keys (e.g., calls in the
beforeItems.forEach and afterItems.forEach loops) intact.
- Around line 69-88: parseBreadcrumbChildren currently drops children that
aren't recognized breadcrumb parts (BreadcrumbItem, BreadcrumbSeparator,
BreadcrumbEllipsis) with no feedback; update parseBreadcrumbChildren to warn in
development when an element is a valid React element but not matched by
isBreadcrumbPart (and optionally when a non-empty text node or unknown node is
passed) by calling console.warn or process.env.NODE_ENV check inside the
flat.forEach loop; reference the existing symbols parseBreadcrumbChildren,
flattenFragments(Children.toArray(children)), isValidElement, isBreadcrumbPart,
BreadcrumbItem, BreadcrumbSeparator, and BreadcrumbEllipsis and emit a clear
dev-only warning that includes the offending child/type so users know why their
node was dropped.
- Around line 171-173: The extraction of children into _propsChildren from props
(const { children: _propsChildren, ...restProps } = props) is redundant because
children is already destructured earlier; remove that destructuring and ensure
you use the existing children variable and a restProps that does not re-extract
children (or tighten the prop typing to exclude children from props) so children
is not handled twice in BreadcrumbRoot; update references to use restProps (or a
propsWithoutChildren alias) accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 65b1ecac-7a92-4967-8461-a9e16b6675b4
📒 Files selected for processing (7)
apps/www/src/app/examples/page.tsxapps/www/src/components/demo/demo.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-root.tsx
paanSinghCoder
changed the title
paanSinghCoder
changed the title
paanSinghCoder
changed the title
rohanchkrabrty
requested changes
Mar 13, 2026
* feat: add disabled prop support for item * feat: add trailing item support from item * feat(breadcrumb): update separator accessibility attributes to use role="presentation" and aria-hidden="true" * fix: replace <a> with <span> for current item * style(breadcrumb): enhance active link styling with hover effect and default cursor * chore: Remove breadcrumb examples * refactor: merge jsx * feat (3-breadcrumb): add href support and expose data-disabled and data-current attributes (#669) * feat (breadcrumb-dropdown): add href support * feat: add data attributes * refactor: merge react 19 * refactor: pass all menuItem props
rohanchkrabrty
approved these changes
Mar 21, 2026
Contributor
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 `@packages/raystack/components/breadcrumb/breadcrumb-item.tsx`:
- Around line 92-109: The current branch returns a <span> for disabled/current
items but drops shared DOM props because ...(props) isn’t forwarded; update the
BreadcrumbItem component to forward remaining common DOM attributes (data-*,
aria-*, id, title, role, etc.) to the rendered <span
className={cx(styles['breadcrumb-link'], ...)} ref={ref} while filtering out
anchor-only props like href, target, rel; ensure the existing conditional
additions ('aria-disabled', 'data-disabled', 'aria-current', 'data-current') are
merged with the forwarded props so tests and accessibility attributes are
preserved when disabled or current.
- Around line 59-89: The dropdown branch in Breadcrumb.Item returns a Menu
directly which breaks list semantics and places the component-level className
and ref on Menu.Trigger; wrap the entire Menu in the same <li> container used by
the other branch so the item container receives className, ref and any list
semantics. Specifically, move/attach the className (styles['breadcrumb-item'] +
className) and ref to the <li> element that wraps the Menu, keep Menu.Trigger
receiving only button-specific props (exclude the item container className), and
preserve passing dropdownItems into Menu.Content/Item as currently implemented
in breadcrumb-item.tsx (identify Menu, Menu.Trigger, Menu.Content, Menu.Item and
the dropdownItems mapping).
- Around line 115-123: The cloneElement call in breadcrumb-item.tsx currently
sets className: styles['breadcrumb-link'] but then spreads
...renderedElement.props which can overwrite that className; update the
cloneElement props to merge classes using cx so the base breadcrumb-link is
preserved and any custom className from renderedElement.props or incoming props
is combined (e.g. className: cx(styles['breadcrumb-link'], props.className,
renderedElement.props?.className)); update the reference in the cloneElement
call (in the component that uses renderedElement/ref) and add a unit/test that
renders the component with as={<a className="custom" />} asserting both
styles['breadcrumb-link'] and "custom" are present to prevent regressions.
In `@packages/raystack/components/breadcrumb/breadcrumb-misc.tsx`:
- Around line 33-39: The separator currently spreads {...props} after setting
role='presentation' and aria-hidden='true' which allows callers to override
those semantics; update the Breadcrumb separator in breadcrumb-misc.tsx (the
element using ref, className, cx(styles['breadcrumb-separator'], className), and
{...props}) so the user props are applied before the fixed accessibility
attributes or remove role/aria from the public props by destructuring them out
(e.g., const { role, 'aria-hidden': ariaHidden, ...rest } = props) and then
spread rest, finally setting role='presentation' and aria-hidden='true' on the
element to ensure the separator semantics are non-overridable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4fb8ed04-2ea0-41b3-93db-03c29a3a4455
📒 Files selected for processing (8)
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.tsxpackages/raystack/components/breadcrumb/breadcrumb-misc.tsxpackages/raystack/components/breadcrumb/breadcrumb.module.css
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/www/src/content/docs/components/breadcrumb/index.mdx
- apps/www/src/content/docs/components/breadcrumb/props.ts
- apps/www/src/content/docs/components/breadcrumb/demo.ts
Comment on lines
+33
to
+39
| <span | ||
| ref={ref} | ||
| className={cx(styles['breadcrumb-separator'], className)} | ||
| role='presentation' | ||
| aria-hidden='true' | ||
| {...props} | ||
| > |
Contributor
There was a problem hiding this comment.
Keep the separator semantics non-overridable.
...props is spread after role='presentation' and aria-hidden='true', so a caller can accidentally re-expose the separator to assistive tech. Move the spread before the fixed attrs, or omit those two props from the public surface.
Suggested fix
<span
ref={ref}
+ {...props}
className={cx(styles['breadcrumb-separator'], className)}
role='presentation'
aria-hidden='true'
- {...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
| <span | |
| ref={ref} | |
| className={cx(styles['breadcrumb-separator'], className)} | |
| role='presentation' | |
| aria-hidden='true' | |
| {...props} | |
| > | |
| <span | |
| ref={ref} | |
| {...props} | |
| className={cx(styles['breadcrumb-separator'], className)} | |
| role='presentation' | |
| aria-hidden='true' | |
| > |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/raystack/components/breadcrumb/breadcrumb-misc.tsx` around lines 33
- 39, The separator currently spreads {...props} after setting
role='presentation' and aria-hidden='true' which allows callers to override
those semantics; update the Breadcrumb separator in breadcrumb-misc.tsx (the
element using ref, className, cx(styles['breadcrumb-separator'], className), and
{...props}) so the user props are applied before the fixed accessibility
attributes or remove role/aria from the public props by destructuring them out
(e.g., const { role, 'aria-hidden': ariaHidden, ...rest } = props) and then
spread rest, finally setting role='presentation' and aria-hidden='true' on the
element to ensure the separator semantics are non-overridable.
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
2 participants
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.
Breadcrumb 1st PR
Issue-600
Description
New props (root) - Reverted
-maxItems– Maximum number of breadcrumb items to show. When there are more items, the trail collapses: a fixed number at the start, ellipsis, then the rest at the end. At least 2 items are always visible (1 before and 1 after the ellipsis). Values < 2 are treated as 2.-itemsBeforeCollapse– Number of items to show before the ellipsis when collapsed. If not set, it is derived frommaxItems(e.g.maxItems={5}→ 2 before, 3 after). The count after the ellipsis is always derived.Docs updates
className, note oncurrentprop, size example with tabs).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
Documentation
Accessibility
Style
Tests