feat (sidebar): add accordion and trailingIcon support#721
Open
paanSinghCoder wants to merge 3 commits intofeat/sidebar-design-improvementsfrom
Open
feat (sidebar): add accordion and trailingIcon support#721paanSinghCoder wants to merge 3 commits intofeat/sidebar-design-improvementsfrom
paanSinghCoder wants to merge 3 commits intofeat/sidebar-design-improvementsfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughIntroduces Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 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 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 |
paanSinghCoder
changed the title
![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: 4
🧹 Nitpick comments (2)
packages/raystack/components/sidebar/sidebar-more.tsx (2)
42-44: Type assertion may accept non-SidebarItemelements.The filter only checks
isValidElement, then casts all results toReactElement<SidebarItemProps>[]. If a consumer passes other React elements (e.g.,<div>), they'll pass the filter but won't have the expected props likeas,active, orleadingIcon, potentially causing runtime issues when destructuring at line 91-99.Consider adding a more specific filter or documenting that only
Sidebar.Itemchildren are supported.♻️ Suggested approach to filter by displayName
+ import { SidebarItem } from './sidebar-item'; + const items = Children.toArray(children).filter( - isValidElement - ) as ReactElement<SidebarItemProps>[]; + (child): child is ReactElement<SidebarItemProps> => + isValidElement(child) && + (child.type as any)?.displayName === SidebarItem.displayName + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/raystack/components/sidebar/sidebar-more.tsx` around lines 42 - 44, The current creation of items using Children.toArray(children).filter(isValidElement) and casting to ReactElement<SidebarItemProps>[] can accept arbitrary valid React elements (e.g., <div>) and cause runtime errors when you later destructure expected props; change the filter to explicitly only accept Sidebar.Item elements by checking the child's type/displayName (e.g., verify (child.type as any).displayName === 'SidebarItem' or child.type === Sidebar.Item) or implement a TypeScript runtime type guard like isSidebarItemElement that narrows to ReactElement<SidebarItemProps>, and use that instead of the blanket cast on the result from Children.toArray(children) so destructuring of SidebarItemProps is safe.
136-142: Using array index as fallback key may cause issues.When
item.keyisnull, the fallback toindexcan lead to incorrect DOM reconciliation if items are reordered or removed. Consider using a more stable identifier from item props (e.g.,href,childrentext content) if keys aren't provided.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/raystack/components/sidebar/sidebar-more.tsx` around lines 136 - 142, The Menu.Item rendering uses item.key ?? index which can break reconciliation; update the key resolution in sidebar-more.tsx to use a stable identifier instead of the array index — e.g., derive a key by using item.key ?? item.href ?? String(item.children) (or item.id if available) and pass that into <Menu.Item key={...} ... />; ensure the chosen symbol (item.href / item.children / item.id) is present or coerce to string so keys are stable across reorders.
🤖 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 180-187: The Sidebar.More trigger is icon-only here (Sidebar.More
with leadingIcon={<DotsHorizontalIcon />}), so screen readers get no name;
update the usage to provide an accessible name by passing a visible label prop
or an explicit aria-label/accessibleName prop to Sidebar.More (or add a
dedicated accessible-name prop to the Sidebar.More component if you prefer
component change), and ensure the component uses that value for the collapsed
trigger aria-label (the code path around label/aria-label in Sidebar.More should
read the new prop when leadingIcon is present).
In `@apps/www/src/content/docs/components/sidebar/props.ts`:
- Around line 92-115: The Sidebar API docs are missing new group props—update
the SidebarGroupProps interface to document the same new API surface as
SidebarMoreProps: add the variant prop (e.g., variant?: string | enum) and the
Sidebar.Group-specific props accordion?: boolean and trailingIcon?: ReactNode so
the docs/API table covers the props used in tests and examples; ensure the names
match existing symbols (SidebarGroupProps, variant, accordion, trailingIcon,
Sidebar.More) and add brief JSDoc comments for each to match the style used in
SidebarMoreProps.
In `@packages/raystack/components/sidebar/sidebar-misc.tsx`:
- Around line 125-129: The Accordion is remounting because of key={isCollapsed ?
'collapsed' : 'expanded'} and uses only defaultValue (groupValue), so
expand/collapse state resets on sidebar toggle; remove the conditional key and
convert AccordionPrimitive.Root to a controlled component by adding a value
state (e.g., accordionValue) and an onValueChange handler (e.g.,
setAccordionValue) wired to AccordionPrimitive.Root's value and onValueChange
props, initializing accordionValue from groupValue if needed so panel state
persists across isCollapsed toggles.
In `@packages/raystack/components/sidebar/sidebar-root.tsx`:
- Around line 61-63: The Tooltip for collapsed sidebar items is hardcoded to
side='right' in SidebarItem (look for Tooltip.Content with side='right' in
packages/raystack/components/sidebar/sidebar-item.tsx), so update that to read
the current position from SidebarContext instead: consume position from
SidebarContext (the same context provided by SidebarContext in sidebar-root.tsx)
and pass it to Tooltip.Content (e.g., side={position} or map position to
Tooltip's side prop) so collapsed item tooltips follow the sidebar position;
ensure fallback to 'right' or an appropriate default if position is undefined.
---
Nitpick comments:
In `@packages/raystack/components/sidebar/sidebar-more.tsx`:
- Around line 42-44: The current creation of items using
Children.toArray(children).filter(isValidElement) and casting to
ReactElement<SidebarItemProps>[] can accept arbitrary valid React elements
(e.g., <div>) and cause runtime errors when you later destructure expected
props; change the filter to explicitly only accept Sidebar.Item elements by
checking the child's type/displayName (e.g., verify (child.type as
any).displayName === 'SidebarItem' or child.type === Sidebar.Item) or implement
a TypeScript runtime type guard like isSidebarItemElement that narrows to
ReactElement<SidebarItemProps>, and use that instead of the blanket cast on the
result from Children.toArray(children) so destructuring of SidebarItemProps is
safe.
- Around line 136-142: The Menu.Item rendering uses item.key ?? index which can
break reconciliation; update the key resolution in sidebar-more.tsx to use a
stable identifier instead of the array index — e.g., derive a key by using
item.key ?? item.href ?? String(item.children) (or item.id if available) and
pass that into <Menu.Item key={...} ... />; ensure the chosen symbol (item.href
/ item.children / item.id) is present or coerce to string so keys are stable
across reorders.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 56ff7da0-75fe-4315-b812-d406cb55393b
📒 Files selected for processing (12)
apps/www/src/app/examples/page.tsxapps/www/src/content/docs/components/sidebar/demo.tsapps/www/src/content/docs/components/sidebar/index.mdxapps/www/src/content/docs/components/sidebar/props.tspackages/raystack/components/sidebar/__tests__/sidebar.test.tsxpackages/raystack/components/sidebar/sidebar-item.tsxpackages/raystack/components/sidebar/sidebar-leading-visual.tsxpackages/raystack/components/sidebar/sidebar-misc.tsxpackages/raystack/components/sidebar/sidebar-more.tsxpackages/raystack/components/sidebar/sidebar-root.tsxpackages/raystack/components/sidebar/sidebar.module.csspackages/raystack/components/sidebar/sidebar.tsx
Comment on lines
+180
to
+187
| <Sidebar.More leadingIcon={<DotsHorizontalIcon />}> | ||
| <Sidebar.Item href='#' leadingIcon={<RadixBellIcon />}> | ||
| Notifications | ||
| </Sidebar.Item> | ||
| <Sidebar.Item href='#' leadingIcon={<PersonIcon />} disabled> | ||
| Billing | ||
| </Sidebar.Item> | ||
| </Sidebar.More> |
Contributor
There was a problem hiding this comment.
Give this icon-only Sidebar.More trigger an accessible name.
In packages/raystack/components/sidebar/sidebar-more.tsx (Lines 51-67), label is used as the visible trigger text and as the collapsed aria-label. With only leadingIcon here, this control is unnamed for screen readers. Pass a label, or add a dedicated accessible-name prop before showcasing icon-only usage.
Safe immediate fix
- <Sidebar.More leadingIcon={<DotsHorizontalIcon />}>
+ <Sidebar.More
+ label='More account actions'
+ leadingIcon={<DotsHorizontalIcon />}
+ >📝 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
| <Sidebar.More leadingIcon={<DotsHorizontalIcon />}> | |
| <Sidebar.Item href='#' leadingIcon={<RadixBellIcon />}> | |
| Notifications | |
| </Sidebar.Item> | |
| <Sidebar.Item href='#' leadingIcon={<PersonIcon />} disabled> | |
| Billing | |
| </Sidebar.Item> | |
| </Sidebar.More> | |
| <Sidebar.More | |
| label='More account actions' | |
| leadingIcon={<DotsHorizontalIcon />} | |
| > | |
| <Sidebar.Item href='#' leadingIcon={<RadixBellIcon />}> | |
| Notifications | |
| </Sidebar.Item> | |
| <Sidebar.Item href='#' leadingIcon={<PersonIcon />} disabled> | |
| Billing | |
| </Sidebar.Item> | |
| </Sidebar.More> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/www/src/app/examples/page.tsx` around lines 180 - 187, The Sidebar.More
trigger is icon-only here (Sidebar.More with leadingIcon={<DotsHorizontalIcon
/>}), so screen readers get no name; update the usage to provide an accessible
name by passing a visible label prop or an explicit aria-label/accessibleName
prop to Sidebar.More (or add a dedicated accessible-name prop to the
Sidebar.More component if you prefer component change), and ensure the component
uses that value for the collapsed trigger aria-label (the code path around
label/aria-label in Sidebar.More should read the new prop when leadingIcon is
present).
Comment on lines
+92
to
+115
| export interface SidebarMoreProps { | ||
| /** String for the more trigger label. */ | ||
| label?: string; | ||
|
|
||
| /** Optional ReactNode for the trigger icon. */ | ||
| leadingIcon?: ReactNode; | ||
|
|
||
| /** Sidebar items rendered inside the menu content. */ | ||
| children?: ReactNode; | ||
|
|
||
| /** Optional class names for customizing parts of the more trigger/menu. */ | ||
| classNames?: { | ||
| /** Class name for the trigger root element. */ | ||
| root?: string; | ||
| /** Class name for the leading icon container. */ | ||
| leadingIcon?: string; | ||
| /** Class name for the text element. */ | ||
| text?: string; | ||
| /** Class name for menu item root elements. */ | ||
| menuItem?: string; | ||
| /** Class name for menu content container. */ | ||
| menuContent?: string; | ||
| }; | ||
| } |
Contributor
There was a problem hiding this comment.
Document the new Sidebar.Group props in the same API table.
This update adds variant and Sidebar.More, but SidebarGroupProps still doesn't mention accordion or trailingIcon, even though both are already exercised in packages/raystack/components/sidebar/__tests__/sidebar.test.tsx (Lines 289-435) and apps/www/src/app/examples/page.tsx (Lines 114-188). The docs will ship without the main group's new API surface.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/www/src/content/docs/components/sidebar/props.ts` around lines 92 - 115,
The Sidebar API docs are missing new group props—update the SidebarGroupProps
interface to document the same new API surface as SidebarMoreProps: add the
variant prop (e.g., variant?: string | enum) and the Sidebar.Group-specific
props accordion?: boolean and trailingIcon?: ReactNode so the docs/API table
covers the props used in tests and examples; ensure the names match existing
symbols (SidebarGroupProps, variant, accordion, trailingIcon, Sidebar.More) and
add brief JSDoc comments for each to match the style used in SidebarMoreProps.
Comment on lines
+61
to
+63
| <SidebarContext | ||
| value={{ isCollapsed: !open, position, hideCollapsedItemTooltip }} | ||
| > |
Contributor
There was a problem hiding this comment.
Apply position to collapsed item tooltips as well.
position is now in SidebarContext, but packages/raystack/components/sidebar/sidebar-item.tsx (Lines 73-77) still hardcodes Tooltip.Content to side='right'. Right-positioned sidebars will keep showing collapsed item tooltips on the wrong edge.
Suggested follow-up
- const { isCollapsed, hideCollapsedItemTooltip } = useContext(SidebarContext);
+ const { isCollapsed, hideCollapsedItemTooltip, position } =
+ useContext(SidebarContext);
...
- <Tooltip.Content side='right'>{children}</Tooltip.Content>
+ <Tooltip.Content side={position === 'left' ? 'right' : 'left'}>
+ {children}
+ </Tooltip.Content>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/raystack/components/sidebar/sidebar-root.tsx` around lines 61 - 63,
The Tooltip for collapsed sidebar items is hardcoded to side='right' in
SidebarItem (look for Tooltip.Content with side='right' in
packages/raystack/components/sidebar/sidebar-item.tsx), so update that to read
the current position from SidebarContext instead: consume position from
SidebarContext (the same context provided by SidebarContext in sidebar-root.tsx)
and pass it to Tooltip.Content (e.g., side={position} or map position to
Tooltip's side prop) so collapsed item tooltips follow the sidebar position;
ensure fallback to 'right' or an appropriate default if position is undefined.
c0c7d22 to
c68a182
Compare
…mo with collapsible sections
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
1 participant
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.
Description
feat: add accordion support for the section
feat: add trailingIcon support
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
Sidebar.Morecomponent for nested, collapsible menu itemsplain,floating,inset)Documentation