chore: Replace some SidebarV1 components in favor of SidebarV2 - #41498
Conversation
|
Looks like this PR is ready to merge! 🎉 |
|
WalkthroughSidebar components and footer layouts were migrated from the original Fuselage primitives to Sidebar V2 equivalents, with updated prop forwarding, item props, and footer spacing/content structure. ChangesSidebar V2 migration
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #41498 +/- ##
===========================================
- Coverage 68.61% 68.57% -0.04%
===========================================
Files 4134 4134
Lines 160802 160840 +38
Branches 29273 29252 -21
===========================================
- Hits 110338 110304 -34
- Misses 45356 45430 +74
+ Partials 5108 5106 -2
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
adcfcbf to
c0ae698
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@apps/meteor/client/components/Sidebar/SidebarGenericItem.tsx`:
- Around line 5-10: Update SidebarGenericItemProps to include the forwarded
anchor attributes accepted by SidebarV2Item, preferably by deriving them with
ComponentPropsWithoutRef<typeof SidebarV2Item> while preserving the existing
href, active, children, and externalUrl props. Ensure SidebarNavigationItem can
pass aria-current and other standard anchor attributes without JSX type errors.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d22ec425-1c65-4db3-91bf-42001f1c4a57
📒 Files selected for processing (4)
apps/meteor/client/components/Sidebar/Sidebar.tsxapps/meteor/client/components/Sidebar/SidebarGenericItem.tsxapps/meteor/client/sidebar/footer/SidebarFooterDefault.tsxapps/meteor/client/sidebar/footer/SidebarFooterWatermark.tsx
📜 Review details
⏰ Context from checks skipped due to timeout. (3)
- GitHub Check: cubic · AI code reviewer
- GitHub Check: Hacktron Security Check
- GitHub Check: 🔨 Test UI (EE) / MongoDB 8.0 coverage (4/5)
🧰 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/sidebar/footer/SidebarFooterDefault.tsxapps/meteor/client/components/Sidebar/Sidebar.tsxapps/meteor/client/sidebar/footer/SidebarFooterWatermark.tsxapps/meteor/client/components/Sidebar/SidebarGenericItem.tsx
🧠 Learnings (2)
📚 Learning: 2026-03-27T14:52:56.865Z
Learnt from: dougfabris
Repo: RocketChat/Rocket.Chat PR: 39892
File: apps/meteor/client/views/room/contextualBar/Threads/Thread.tsx:150-155
Timestamp: 2026-03-27T14:52:56.865Z
Learning: In Rocket.Chat, there are two different `ModalBackdrop` components with different prop APIs. During review, confirm the import source: (1) `rocket.chat/fuselage` `ModalBackdrop` uses `ModalBackdropProps` based on `BoxProps` (so it supports `onClick` and other Box/DOM props) and does not have an `onDismiss` prop; (2) `rocket.chat/ui-client` `ModalBackdrop` uses a narrower props interface like `{ children?: ReactNode; onDismiss?: () => void }` and handles Escape keypress and outside mouse-up, and it does not forward arbitrary DOM props such as `onClick`. Flag mismatched props (e.g., `onDismiss` passed to the fuselage component or `onClick` passed to the ui-client component) and ensure the usage matches the correct component being imported.
Applied to files:
apps/meteor/client/sidebar/footer/SidebarFooterDefault.tsxapps/meteor/client/components/Sidebar/Sidebar.tsxapps/meteor/client/sidebar/footer/SidebarFooterWatermark.tsxapps/meteor/client/components/Sidebar/SidebarGenericItem.tsx
📚 Learning: 2026-05-06T12:21:44.083Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 40256
File: apps/meteor/client/components/CreateDiscussion/CreateDiscussion.tsx:121-149
Timestamp: 2026-05-06T12:21:44.083Z
Learning: Field wrappers in rocket.chat/fuselage-forms (Field, FieldLabel, FieldRow, FieldError, FieldHint) auto-create htmlFor/id associations, aria-describedby, and role="alert" for errors. Do not manually set htmlFor, id, aria-describedby, or role attributes when using these wrappers. This automatic wiring does not apply to plain rocket.chat/fuselage components, which require explicit ID wiring per the accessibility docs. In code reviews, prefer using fuselage-forms wrappers for form fields and verify there is no unnecessary manual ID/aria wiring in files that use these wrappers. If a component uses plain fuselage components, ensure proper id wiring as per docs.
Applied to files:
apps/meteor/client/sidebar/footer/SidebarFooterDefault.tsxapps/meteor/client/components/Sidebar/Sidebar.tsxapps/meteor/client/sidebar/footer/SidebarFooterWatermark.tsxapps/meteor/client/components/Sidebar/SidebarGenericItem.tsx
🔇 Additional comments (4)
apps/meteor/client/components/Sidebar/Sidebar.tsx (1)
1-6: 🎯 Functional CorrectnessVerify that
SidebarV2preserves the removed wrapper contract.
apps/meteor/client/sidebar/SidebarRegion.tsx:92-108still renders<Sidebar />without props. The old wrapper suppliedrole='navigation',display='flex',flexDirection='column', andheight='100%'; this change removes all four. Confirm thatSidebarV2provides equivalent landmark and full-height column behavior before merging.apps/meteor/client/components/Sidebar/SidebarGenericItem.tsx (1)
1-4: LGTM!apps/meteor/client/sidebar/footer/SidebarFooterDefault.tsx (1)
2-2: LGTM!Also applies to: 26-28
apps/meteor/client/sidebar/footer/SidebarFooterWatermark.tsx (1)
1-1: LGTM!Also applies to: 29-38
Proposed changes (including videos or screenshots)
Issue(s)
Steps to test or reproduce
Further comments
CORE-2455
Summary by CodeRabbit
Style
Bug Fixes