refactor: move data fetching from RoomEdit to RoomEditWithData#38688
refactor: move data fetching from RoomEdit to RoomEditWithData#38688sezallagwal wants to merge 2 commits intoRocketChat:developfrom
Conversation
|
|
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 |
|
|
|
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)
🧰 Additional context used📓 Path-based instructions (1)**/*.{ts,tsx,js}📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
Files:
🧠 Learnings (4)📚 Learning: 2025-10-28T16:53:42.761ZApplied to files:
📚 Learning: 2025-11-27T17:56:26.050ZApplied to files:
📚 Learning: 2025-11-04T16:49:19.107ZApplied to files:
📚 Learning: 2025-09-25T09:59:26.461ZApplied to files:
🧬 Code graph analysis (1)apps/meteor/client/views/omnichannel/directory/chats/ChatInfo/RoomEdit/RoomEditWithData.tsx (3)
⏰ 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)
🔇 Additional comments (3)
WalkthroughRoomEdit was converted into a presentational component: it no longer fetches SLA policies, priorities, or custom-fields metadata. RoomEditWithData now performs permission checks and data fetching, then passes Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(200,200,255,0.5)
actor User
end
rect rgba(200,255,200,0.5)
participant UI as RoomEditWithData
participant Comp as RoomEdit
end
rect rgba(255,200,200,0.5)
participant API as DataSources
participant Perm as Permissions
end
User->>UI: open Room Edit
UI->>Perm: check permissions (view/edit custom fields)
Perm-->>UI: canViewCustomFields (true/false)
alt canViewCustomFields == true
UI->>API: fetch customFieldsMetadata
API-->>UI: customFieldsMetadata
end
UI->>API: fetch slaPolicies, priorities
API-->>UI: slaPolicies, priorities
UI->>Comp: render with props (slaPolicies, priorities, customFieldsMetadata?, canViewCustomFields)
Comp-->>User: rendered RoomEdit UI
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@apps/meteor/client/views/omnichannel/directory/chats/ChatInfo/RoomEdit/RoomEditWithData.tsx`:
- Line 5: Replace the non-reactive hasAtLeastOnePermission usage with the
reactive hook useAtLeastOnePermission from `@rocket.chat/ui-contexts` in the
RoomEditWithData component: update the import (remove hasAtLeastOnePermission
and import useAtLeastOnePermission) and change the place where
canViewCustomFields is computed to call useAtLeastOnePermission(...) instead of
hasAtLeastOnePermission(...), ensuring the component re-renders when permissions
change.
🧹 Nitpick comments (2)
apps/meteor/client/views/omnichannel/directory/chats/ChatInfo/RoomEdit/RoomEdit.tsx (2)
53-53: Long function signature — consider destructuring on multiple lines for readability.The parameter list is getting wide. This is a minor nit; consider breaking it across lines.
130-136: Truthiness checks on imported components are always true.
SlaPoliciesSelect &&(Line 130) andPrioritiesSelect &&(Line 134) are always truthy since these are statically imported components. These guards are redundant. That said, this appears to be pre-existing code not introduced in this PR, so it can be addressed separately.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/meteor/client/views/omnichannel/directory/chats/ChatInfo/RoomEdit/RoomEdit.tsxapps/meteor/client/views/omnichannel/directory/chats/ChatInfo/RoomEdit/RoomEditWithData.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/omnichannel/directory/chats/ChatInfo/RoomEdit/RoomEdit.tsxapps/meteor/client/views/omnichannel/directory/chats/ChatInfo/RoomEdit/RoomEditWithData.tsx
🧠 Learnings (3)
📚 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/omnichannel/directory/chats/ChatInfo/RoomEdit/RoomEdit.tsxapps/meteor/client/views/omnichannel/directory/chats/ChatInfo/RoomEdit/RoomEditWithData.tsx
📚 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/omnichannel/directory/chats/ChatInfo/RoomEdit/RoomEdit.tsxapps/meteor/client/views/omnichannel/directory/chats/ChatInfo/RoomEdit/RoomEditWithData.tsx
📚 Learning: 2025-11-04T16:49:19.107Z
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 37377
File: apps/meteor/ee/server/hooks/federation/index.ts:86-88
Timestamp: 2025-11-04T16:49:19.107Z
Learning: In Rocket.Chat's federation system (apps/meteor/ee/server/hooks/federation/), permission checks follow two distinct patterns: (1) User-initiated federation actions (creating rooms, adding users to federated rooms, joining from invites) should throw MeteorError to inform users they lack 'access-federation' permission. (2) Remote server-initiated federation events should silently skip/ignore when users lack permission. The beforeAddUserToRoom hook only executes for local user-initiated actions, so throwing an error there is correct. Remote federation events are handled separately by the federation Matrix package with silent skipping logic.
Applied to files:
apps/meteor/client/views/omnichannel/directory/chats/ChatInfo/RoomEdit/RoomEditWithData.tsx
🧬 Code graph analysis (2)
apps/meteor/client/views/omnichannel/directory/chats/ChatInfo/RoomEdit/RoomEdit.tsx (2)
apps/meteor/client/views/omnichannel/additionalForms/SlaPoliciesSelect.tsx (1)
SlaPoliciesSelect(15-32)apps/meteor/client/views/omnichannel/additionalForms/PrioritiesSelect.tsx (1)
PrioritiesSelect(20-69)
apps/meteor/client/views/omnichannel/directory/chats/ChatInfo/RoomEdit/RoomEditWithData.tsx (4)
apps/meteor/server/services/authorization/service.ts (1)
hasAtLeastOnePermission(73-78)packages/ddp-client/src/livechat/LivechatClientImpl.ts (1)
visitor(151-159)apps/meteor/client/views/omnichannel/directory/hooks/useSlaPolicies.tsx (1)
useSlaPolicies(7-21)apps/meteor/client/views/omnichannel/directory/hooks/useCustomFieldsMetadata.tsx (1)
useCustomFieldsMetadata(12-19)
⏰ 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 (3)
apps/meteor/client/views/omnichannel/directory/chats/ChatInfo/RoomEdit/RoomEdit.tsx (1)
22-32: Clean prop typing usingComponentProps.The derived types keep RoomEdit's prop contract in sync with the child components automatically. This is a good pattern.
apps/meteor/client/views/omnichannel/directory/chats/ChatInfo/RoomEdit/RoomEditWithData.tsx (2)
29-34: Data-fetching hooks look correctly lifted.The hooks are properly invoked at the top level and their results are passed down as props. The conditional
enabled: canViewCustomFieldsonuseCustomFieldsMetadatacorrectly avoids unnecessary fetching.
36-38: Loading guard correctly aggregates all query states.When a query is disabled (e.g.,
useCustomFieldsMetadatawithenabled: false), TanStack Query v5 reportsisLoadingasfalse(sinceisLoading = isPending && isFetching), so this won't block rendering unnecessarily.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
2 participants
Description
Moves data-fetching hooks (useSlaPolicies, useCustomFieldsMetadata, useOmnichannelPriorities) and the
canViewCustomFieldspermission check from RoomEdit (child) to RoomEditWithData (parent), resolving the TODO atRoomEdit.tsx:120-121.This ensures RoomEdit is a purely presentational component that receives all data as props — matching the established pattern used by all sibling
*WithDatacomponents (AgentEditWithData, UnitEditWithData, TagEditWithData, EditTriggerWithData, EditCustomFieldsWithData, SlaEditWithData, PriorityEditFormWithData).Changes
slaPolicies,customFieldsMetadata,priorities,canViewCustomFieldsas props.Summary by CodeRabbit
Refactor
Bug Fixes
UI