fix: Improve settings sidebar behavior in mobile view #35520#38540
Open
Dnyanesh-29 wants to merge 3 commits intoRocketChat:developfrom
Open
fix: Improve settings sidebar behavior in mobile view #35520#38540Dnyanesh-29 wants to merge 3 commits intoRocketChat:developfrom
Dnyanesh-29 wants to merge 3 commits intoRocketChat:developfrom
Conversation
Contributor
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
|
Contributor
WalkthroughAdds an optional Changes
Sequence Diagram(s)sequenceDiagram
participant User as "User"
participant NavItem as "SidebarNavigationItem"
participant Generic as "SidebarGenericItem"
participant SidebarCtrl as "Sidebar (toggle/close)"
rect rgba(135,206,235,0.5)
User->>NavItem: click item
end
rect rgba(152,251,152,0.5)
NavItem->>Generic: forward onClick
end
rect rgba(255,182,193,0.5)
Generic->>SidebarCtrl: invoke onClick (e.g., toggle/close)
SidebarCtrl-->>User: sidebar closes
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. No actionable comments were generated in the recent review. 🎉 📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
✏️ Tip: You can disable this entire section by setting 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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
![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: 3
🤖 Fix all issues with AI agents
In `@apps/meteor/client/components/Sidebar/SidebarGenericItem.tsx`:
- Line 26: The inline click handler currently defined as (e) => {onClick?.();}
in SidebarGenericItem.tsx captures an unused event param; replace this wrapper
by passing the onClick prop directly (or using onClick && onClick) so the
element's onClick is set to onClick without the unused e, e.g. set
onClick={onClick} to simplify and remove the unused parameter.
- Around line 14-19: Remove the unused local function handleClick in the
SidebarGenericItem component: delete the handleClick declaration (the function
that calls onClick()) and rely on the existing inline onClick handler already
used in the JSX; ensure no other references to handleClick remain and that the
component still forwards the onClick prop (SidebarGenericItem, handleClick,
onClick).
In `@apps/meteor/client/views/admin/sidebar/AdminSidebar.tsx`:
- Line 29: Replace the call to sidebar.toggle in the AdminSidebarPages onClick
handler with sidebar.close to avoid accidentally re-opening the drawer and to
match the existing onClose={sidebar.close} semantics used on the header; locate
the AdminSidebarPages component invocation (symbol: AdminSidebarPages) and
change its onClick={() => {sidebar.toggle()}} to onClick={() =>
{sidebar.close()}} (or directly onClick={sidebar.close}) so the drawer is
consistently dismissed rather than toggled.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apps/meteor/client/components/Sidebar/SidebarGenericItem.tsxapps/meteor/client/components/Sidebar/SidebarItemsAssembler.tsxapps/meteor/client/components/Sidebar/SidebarNavigationItem.tsxapps/meteor/client/views/account/AccountSidebar.tsxapps/meteor/client/views/admin/sidebar/AdminSidebar.tsxapps/meteor/client/views/admin/sidebar/AdminSidebarPages.tsx
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
apps/meteor/client/components/Sidebar/SidebarItemsAssembler.tsxapps/meteor/client/views/admin/sidebar/AdminSidebarPages.tsxapps/meteor/client/components/Sidebar/SidebarNavigationItem.tsxapps/meteor/client/components/Sidebar/SidebarGenericItem.tsxapps/meteor/client/views/admin/sidebar/AdminSidebar.tsxapps/meteor/client/views/account/AccountSidebar.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: cubic · AI code reviewer
🔇 Additional comments (4)
apps/meteor/client/components/Sidebar/SidebarNavigationItem.tsx (1)
16-16: LGTM!Clean prop threading —
onClickis accepted, destructured, and forwarded toSidebarGenericItemwithout unnecessary logic.Also applies to: 26-26, 40-40
apps/meteor/client/components/Sidebar/SidebarItemsAssembler.tsx (1)
12-12: LGTM!The
onClickprop is cleanly accepted and forwarded to eachSidebarNavigationItem.Also applies to: 15-15, 32-32
apps/meteor/client/views/admin/sidebar/AdminSidebarPages.tsx (1)
9-9: LGTM!Clean prop forwarding from
AdminSidebarPagestoSidebarItemsAssembler.Also applies to: 12-12, 17-17
apps/meteor/client/views/account/AccountSidebar.tsx (1)
23-23: Thesidebar.toggle()guard is already implemented — no viewport-specific changes needed.The concern about toggling on all viewport sizes is unfounded. In
LayoutProvider.tsx(line 68-69),toggleis conditionally defined:toggle: shouldToggle ? () => setIsCollapsed((isCollapsed) => !isCollapsed) : () => undefined,Since
shouldToggleisfalseon desktop (when the 'md' breakpoint is present),toggle()becomes a no-op on desktop. The sidebar is already safe from being hidden on desktop—the guard is built into the function itself.The minor style inconsistency remains: this file passes
sidebar.toggle(reference) whileAdminSidebar.tsxwraps it as() => { sidebar.toggle() }. Both are functionally equivalent, but unifying the style would be a minor improvement.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
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.
Summary
This PR improves the mobile navigation experience within the Administration and Account sections. It ensures that the sidebar drawer automatically closes when a navigation item is selected, providing immediate visual feedback and better accessibility for mobile users.
Closes: #35520
A Note on the Revision
[!IMPORTANT] Apologies for the noise in the previous submission. I accidentally included unrelated server-side and API changes in my earlier push. I have now performed a hard reset and rebase to isolate the specific UI changes required for this fix. This PR now contains a single, clean commit with only the 6 relevant files.
Changes
Logic: Passed the sidebar layout handler via an onClick prop through the sidebar component chain.
Trigger: Updated SidebarGenericItem to execute the layout toggle upon selection.
Scope: Applied the fix to AdminSidebar and AccountSidebar for a consistent global experience.
Testing Instructions
Environment: Use a mobile device or Browser DevTools (Width < 768px).
Action: Go to Administration and click a link (e.g., Workspace).
Action: Go to Account and click a link (e.g., Profile).
Expected Result: The sidebar should slide shut immediately, revealing the selected page content without requiring a manual close.
Visuals (Fix in Action)
Screen.Recording.2026-02-07.170752.mp4
Final Files Changed Verification
apps/meteor/client/components/Sidebar/SidebarGenericItem.tsx
apps/meteor/client/components/Sidebar/SidebarItemsAssembler.tsx
apps/meteor/client/components/Sidebar/SidebarNavigationItem.tsx
apps/meteor/client/views/admin/sidebar/AdminSidebar.tsx
apps/meteor/client/views/admin/sidebar/AdminSidebarPages.tsx
apps/meteor/client/views/account/AccountSidebar.tsx
Summary by CodeRabbit