Skip to content

feat(sidebar): controlled support for Sidebar.Group#807

Merged
rohanchkrabrty merged 1 commit into
mainfrom
controlled-sidebar-group
May 14, 2026
Merged

feat(sidebar): controlled support for Sidebar.Group#807
rohanchkrabrty merged 1 commit into
mainfrom
controlled-sidebar-group

Conversation

@rohanchkrabrty
Copy link
Copy Markdown
Contributor

Summary

  • Added open, defaultOpen, and onOpenChange props to Sidebar.Group so consumers can drive a collapsible group's expanded state externally, mirroring the existing SidebarRoot API.
  • Internally replaced the key-based remount trick on the Base UI accordion with a controlled value/onValueChange, while still forcing the panel open when the parent sidebar is collapsed (preserving existing behavior).
  • Dropped the unused value prop on the group — the accordion now uses a single sentinel item value internally since each group has exactly one accordion item.
  • Documented the new props in SidebarGroupProps and added a "Controlled Group" example to the docs.
  • Added 3 tests covering defaultOpen={false}, fully controlled open/onOpenChange, and uncontrolled toggle dispatch.

@vercel
Copy link
Copy Markdown

vercel Bot commented May 14, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
apsara Ready Ready Preview, Comment May 14, 2026 7:07am

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 14, 2026

Review Change Stack

📝 Walkthrough

Walkthrough

This PR adds controlled state management to collapsible sidebar groups. The Sidebar.Group component now accepts open (controlled), defaultOpen (uncontrolled default), and onOpenChange callback props to enable external control of group expansion. The implementation updates the underlying AccordionPrimitive integration from uncontrolled to controlled mode, properly mapping accordion value changes to boolean state. Comprehensive tests validate defaultOpen behavior, controlled expansion, and callback signaling. Documentation and a new demo example show users how to manage group state externally.

Suggested reviewers

  • rsbh
  • paanSinghCoder
  • rohilsurana
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title 'feat(sidebar): controlled support for Sidebar.Group' directly describes the main change: adding controlled state support to the Sidebar.Group component.
Description check ✅ Passed The description comprehensively explains the changes including new props, internal implementation details, removed features, documentation updates, and test coverage.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 ESLint

If the error stems from missing dependencies, add them to the package.json file. For unrecoverable errors (e.g., due to private dependencies), disable the tool in the CodeRabbit configuration.

ESLint skipped: no ESLint configuration detected in root package.json. To enable, add eslint to devDependencies.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
packages/raystack/components/sidebar/sidebar-misc.tsx (1)

86-94: 💤 Low value

Consider optimizing internal state update for fully controlled mode.

When the component is fully controlled (i.e., providedOpen !== undefined), line 90 still updates internalOpen, even though this state is not used (line 84 gives precedence to providedOpen). While harmless, you could optimize this by conditionally updating internal state only when uncontrolled:

♻️ Optional optimization
 const handleOpenChange = useCallback(
   (value: unknown[]) => {
     if (isCollapsed) return;
     const nextOpen = value.length > 0;
-    setInternalOpen(nextOpen);
+    if (providedOpen === undefined) {
+      setInternalOpen(nextOpen);
+    }
     onOpenChange?.(nextOpen);
   },
-  [isCollapsed, onOpenChange]
+  [isCollapsed, providedOpen, onOpenChange]
 );
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/raystack/components/sidebar/sidebar-misc.tsx` around lines 86 - 94,
handleOpenChange currently always calls setInternalOpen even when the component
is fully controlled via providedOpen; update the handler to only update
internalOpen (via setInternalOpen) when providedOpen is undefined (i.e.,
uncontrolled) while still calling onOpenChange and respecting isCollapsed;
locate handleOpenChange and add a conditional guard around setInternalOpen that
checks providedOpen before updating internal state.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@packages/raystack/components/sidebar/sidebar-misc.tsx`:
- Around line 86-94: handleOpenChange currently always calls setInternalOpen
even when the component is fully controlled via providedOpen; update the handler
to only update internalOpen (via setInternalOpen) when providedOpen is undefined
(i.e., uncontrolled) while still calling onOpenChange and respecting
isCollapsed; locate handleOpenChange and add a conditional guard around
setInternalOpen that checks providedOpen before updating internal state.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: ba01ced7-783c-40de-923a-5d93714b22cf

📥 Commits

Reviewing files that changed from the base of the PR and between 8ccfc9a and 480d06a.

📒 Files selected for processing (6)
  • apps/www/src/components/docs/sidebar.tsx
  • apps/www/src/content/docs/components/sidebar/demo.ts
  • apps/www/src/content/docs/components/sidebar/index.mdx
  • apps/www/src/content/docs/components/sidebar/props.ts
  • packages/raystack/components/sidebar/__tests__/sidebar.test.tsx
  • packages/raystack/components/sidebar/sidebar-misc.tsx

@rohanchkrabrty rohanchkrabrty merged commit 852c8b3 into main May 14, 2026
5 checks passed
@rohanchkrabrty rohanchkrabrty deleted the controlled-sidebar-group branch May 14, 2026 10:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants