chore: remove roomToolbox prop in RoomHeader component
#37755
Conversation
|
Looks like this PR is ready to merge! 🎉 |
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughReplaces the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/meteor/client/views/room/HeaderV2/RoomHeaderE2EESetup.tsx (1)
10-10: Consider accepting slots parameter for API consistency.Unlike
Header/RoomHeaderE2EESetup.tsx(line 10), this component does not accept aslotsparameter. This creates an inconsistency between the two versions and prevents this component from preserving any existing slots that might be passed through.If this inconsistency is unintentional, apply this diff to align the API:
-const RoomHeaderE2EESetup = ({ room }: RoomHeaderProps) => { +const RoomHeaderE2EESetup = ({ room, slots = {} }: RoomHeaderProps) => {Then update the slots injection on lines 18-22 to spread existing slots:
return ( <RoomHeader room={room} slots={{ + ...slots, toolbox: { + ...slots.toolbox, content: <RoomToolboxE2EESetup />, }, }} /> );And pass slots through on line 27:
- return <RoomHeader room={room} />; + return <RoomHeader room={room} slots={slots} />;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
apps/meteor/client/views/room/Header/RoomHeader.tsx(2 hunks)apps/meteor/client/views/room/Header/RoomHeaderE2EESetup.tsx(1 hunks)apps/meteor/client/views/room/HeaderV2/RoomHeaderE2EESetup.tsx(1 hunks)
🧰 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/views/room/HeaderV2/RoomHeaderE2EESetup.tsxapps/meteor/client/views/room/Header/RoomHeader.tsxapps/meteor/client/views/room/Header/RoomHeaderE2EESetup.tsx
🧠 Learnings (1)
📚 Learning: 2025-11-27T17:56:26.050Z
Learnt from: MartinSchoeler
Repo: RocketChat/Rocket.Chat PR: 37557
File: apps/meteor/client/views/admin/ABAC/AdminABACRooms.tsx:115-116
Timestamp: 2025-11-27T17:56:26.050Z
Learning: In Rocket.Chat, the GET /v1/abac/rooms endpoint (implemented in ee/packages/abac/src/index.ts) only returns rooms where abacAttributes exists and is not an empty array (query: { abacAttributes: { $exists: true, $ne: [] } }). Therefore, in components consuming this endpoint (like AdminABACRooms.tsx), room.abacAttributes is guaranteed to be defined for all returned rooms, and optional chaining before calling array methods like .join() is sufficient without additional null coalescing.
Applied to files:
apps/meteor/client/views/room/HeaderV2/RoomHeaderE2EESetup.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: 📦 Build Packages
🔇 Additional comments (2)
apps/meteor/client/views/room/Header/RoomHeaderE2EESetup.tsx (1)
15-26: LGTM! Proper slots composition.The slots spreading logic correctly preserves existing slot content while injecting the E2EE setup toolbox. The nested spread on line 21 ensures any existing
slots.toolbox.preorslots.toolbox.posvalues are retained.apps/meteor/client/views/room/Header/RoomHeader.tsx (1)
66-74: Verify all call sites provide slots.toolbox for backward compatibility.The toolbox rendering is now gated behind
slots.toolboxexistence (line 66). Previously, the toolbox was always rendered via theroomToolboxprop. This means any existing usage of<RoomHeader room={room} />without passingslots.toolboxwill no longer render the toolbox UI.Run the following script to identify all RoomHeader call sites and verify they provide slots.toolbox:
#!/bin/bash # Find all RoomHeader usages and check if they pass slots.toolbox echo "=== Finding RoomHeader component usages ===" rg -n -C 5 '<RoomHeader\s' --type=tsx --type=ts -g '!**/Header/RoomHeader.tsx' echo -e "\n=== Checking for imports of RoomHeader ===" rg -n "import.*RoomHeader.*from.*Header/RoomHeader" --type=tsx --type=ts
dougfabris
changed the title
roomToolbox prop in RoomHeader component
…cated roomToolbox prop
ggazzo
force-pushed
the
chore/room-header
branch
from
e7fb4ad to
0177d9b
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
apps/meteor/client/views/room/Header/RoomHeader.tsx(2 hunks)apps/meteor/client/views/room/Header/RoomHeaderE2EESetup.tsx(1 hunks)apps/meteor/client/views/room/HeaderV2/RoomHeader.tsx(2 hunks)apps/meteor/client/views/room/HeaderV2/RoomHeaderE2EESetup.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/meteor/client/views/room/HeaderV2/RoomHeaderE2EESetup.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/views/room/HeaderV2/RoomHeader.tsxapps/meteor/client/views/room/Header/RoomHeader.tsxapps/meteor/client/views/room/Header/RoomHeaderE2EESetup.tsx
🧠 Learnings (1)
📚 Learning: 2025-10-28T16:53:42.761Z
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 37205
File: ee/packages/federation-matrix/src/FederationMatrix.ts:296-301
Timestamp: 2025-10-28T16:53:42.761Z
Learning: In the Rocket.Chat federation-matrix integration (ee/packages/federation-matrix/), the createRoom method from rocket.chat/federation-sdk will support a 4-argument signature (userId, roomName, visibility, displayName) in newer versions. Code using this 4-argument call is forward-compatible with planned library updates and should not be flagged as an error.
Applied to files:
apps/meteor/client/views/room/HeaderV2/RoomHeader.tsx
🧬 Code graph analysis (2)
apps/meteor/client/views/room/HeaderV2/RoomHeader.tsx (1)
apps/meteor/client/views/room/Header/RoomHeader.tsx (1)
RoomHeaderProps(19-35)
apps/meteor/client/views/room/Header/RoomHeader.tsx (1)
apps/meteor/client/views/room/HeaderV2/RoomHeader.tsx (1)
RoomHeaderProps(17-32)
⏰ 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). (2)
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (5)
apps/meteor/client/views/room/HeaderV2/RoomHeader.tsx (2)
29-29: LGTM: Type definition addition is correct.The
hidden?: booleanproperty enables conditional rendering of the toolbox, aligning with the slots-based API refactor.
34-34: LGTM: Signature correctly implements the refactor.The removal of the
roomToolboxprop and reliance onslotsaligns with the PR objective to migrate to a slots-based API.apps/meteor/client/views/room/Header/RoomHeaderE2EESetup.tsx (1)
15-26: LGTM: Slot merging correctly preserves existing values.The implementation properly spreads existing slots and toolbox properties before adding the E2EE-specific
content, ensuring no slot values are lost during the merge.apps/meteor/client/views/room/Header/RoomHeader.tsx (2)
32-32: LGTM: Type definition addition is correct.The
hidden?: booleanproperty enables conditional rendering of the toolbox, consistent with the HeaderV2 implementation.
37-37: LGTM: Signature correctly implements the refactor.The removal of the
roomToolboxprop and reliance onslotsaligns with the PR objective to migrate to a slots-based API.
There was a problem hiding this comment.
Pull request overview
This PR refactors the RoomHeader component to remove the roomToolbox prop in favor of a more flexible slots-based approach for injecting toolbox content.
Key Changes:
- Removed the
roomToolboxprop fromRoomHeadercomponents - Extended the
slots.toolboxobject to include acontentproperty for toolbox injection - Added a
hiddenflag toslots.toolboxfor conditional toolbox visibility
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
apps/meteor/client/views/room/HeaderV2/RoomHeader.tsx |
Updated to remove roomToolbox prop and use slots.toolbox.content instead; added conditional rendering based on slots.toolbox.hidden |
apps/meteor/client/views/room/HeaderV2/RoomHeaderE2EESetup.tsx |
Migrated from using roomToolbox prop to slots.toolbox.content for E2EE setup toolbox |
apps/meteor/client/views/room/Header/RoomHeader.tsx |
Updated to remove roomToolbox prop and use slots.toolbox.content instead; added conditional rendering based on slots.toolbox.hidden |
apps/meteor/client/views/room/Header/RoomHeaderE2EESetup.tsx |
Migrated from using roomToolbox prop to slots.toolbox.content for E2EE setup toolbox, with proper slots spreading |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
ggazzo
changed the title
roomToolbox prop in RoomHeader componentroomToolbox prop in RoomHeader component
📦 Docker Image Size Report📈 Changes
📊 Historical Trend---
config:
theme: "dark"
xyChart:
width: 900
height: 400
---
xychart
title "Image Size Evolution by Service (Last 30 Days + This PR)"
x-axis ["11/15 22:28", "11/16 01:28", "11/17 23:50", "11/18 22:53", "11/19 23:02", "11/21 16:49", "11/24 17:34", "11/27 22:32", "11/28 19:05", "12/01 23:01", "12/02 21:57", "12/03 21:00", "12/04 18:17", "12/05 21:56", "12/08 20:15", "12/09 19:25", "12/09 21:03 (PR)"]
y-axis "Size (GB)" 0 --> 0.5
line "account-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11]
line "authorization-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11]
line "ddp-streamer-service" [0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12]
line "omnichannel-transcript-service" [0.14, 0.14, 0.14, 0.14, 0.14, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13]
line "presence-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11]
line "queue-worker-service" [0.14, 0.14, 0.14, 0.14, 0.14, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13]
line "rocketchat" [0.36, 0.36, 0.35, 0.35, 0.35, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.35]
line "stream-hub-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11]
Statistics (last 16 days):
ℹ️ About this reportThis report compares Docker image sizes from this build against the
Updated: Tue, 09 Dec 2025 21:03:31 GMT |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #37755 +/- ##
===========================================
+ Coverage 67.71% 67.73% +0.02%
===========================================
Files 3449 3448 -1
Lines 113927 113900 -27
Branches 20915 20912 -3
===========================================
+ Hits 77143 77149 +6
+ Misses 34661 34627 -34
- Partials 2123 2124 +1
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
ggazzo
added
the
stat: QA assured
dionisio-bot
bot
added
the
stat: ready to merge
split from #37612
Proposed changes (including videos or screenshots)
Issue(s)
Steps to test or reproduce
Further comments
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.