refactor: better isRoomFederated usage
#36952
Conversation
|
Looks like this PR is ready to merge! 🎉 |
|
WalkthroughRefactors multiple React components to compute isRoomFederated once per render via a local roomIsFederated constant and replace inline calls; adjusts some memo/use-* dependencies. One handler switched from useCallback to useEffectEvent. No exported/public signatures changed. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Component
participant Fed as isRoomFederated
participant State as Local State
note over UI,Fed: federated check computed once per render (roomIsFederated)
UI->>Fed: isRoomFederated(room)
Fed-->>UI: roomIsFederated (boolean)
alt roomIsFederated == true
UI->>State: disable inputs/toggles, adjust options
else roomIsFederated == false
UI->>State: enable inputs/toggles, standard options
end
note over UI: Navigation handler change
UI->>UI: handleBack via useEffectEvent -> setState({ tab: LIST })
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ 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). (3)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. Comment |
ggazzo
added
the
stat: QA assured
dionisio-bot
bot
added
the
stat: ready to merge
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #36952 +/- ##
===========================================
+ Coverage 66.58% 66.59% +0.01%
===========================================
Files 3346 3346
Lines 114661 114666 +5
Branches 21097 21102 +5
===========================================
+ Hits 76350 76365 +15
+ Misses 35615 35605 -10
Partials 2696 2696
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
![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 (6)
apps/meteor/client/views/room/contextualBar/Info/EditRoomInfo/EditRoomInfo.tsx (1)
226-233: Avoid name shadowing: rename the retentionIgnoreThreads idThe form field name
retentionIgnoreThreads(data) and the DOM id varretentionIgnoreThreads(UI) collide. Rename the id var for clarity.Apply:
-const retentionIgnoreThreads = useId(); +const retentionIgnoreThreadsField = useId(); ... -<FieldLabel htmlFor={retentionIgnoreThreads}>{t('RetentionPolicy_DoNotPruneThreads')}</FieldLabel> +<FieldLabel htmlFor={retentionIgnoreThreadsField}>{t('RetentionPolicy_DoNotPruneThreads')}</FieldLabel> ... -<ToggleSwitch id={retentionIgnoreThreads} {...field} checked={value} /> +<ToggleSwitch id={retentionIgnoreThreadsField} {...field} checked={value} />Also applies to: 596-603
apps/meteor/client/views/room/hooks/useUserInfoActions/actions/useChangeModeratorAction.tsx (1)
90-101: Use the derived flag in the handler for consistency (and add a return)Minor polish: reuse
roomIsFederatedinhandleChangeModeratorand return the final mutation for uniform control-flow.+ const roomIsFederated = isRoomFederated(room); const handleChangeModerator = useCallback( ({ userId }: { userId: string }) => { - if (!isRoomFederated(room)) { - return toggleModerator.mutateAsync({ roomId: rid, userId: uid }); + if (!roomIsFederated) { + return toggleModerator.mutateAsync({ roomId: rid, userId: uid }); } @@ - toggleModerator.mutateAsync({ roomId: rid, userId: uid }); + return toggleModerator.mutateAsync({ roomId: rid, userId: uid }); }, - [setModal, loggedUserId, loggedUserIsModerator, loggedUserIsOwner, t, rid, uid, toggleModerator, room], + [setModal, loggedUserId, loggedUserIsModerator, loggedUserIsOwner, t, rid, uid, toggleModerator, roomIsFederated], ); - - const roomIsFederated = isRoomFederated(room);Also applies to: 139-141, 146-149
apps/meteor/client/views/room/hooks/useUserInfoActions/actions/useAddUserAction.tsx (2)
66-74: Use the derived flag inside the action for consistencyNot functional, but keeps a single source of truth for the federated check.
- if (isRoomFederated(room)) { + if (roomIsFederated) { addClickHandler.mutate({ users, handleSave: handleAddUser, }); } else { await handleAddUser({ users }); }
58-61: Avoid shadowing usernameLocal
const [username] = users;hides the outerusernamefrom props. Rename for clarity.- const [username] = users; - await inviteUser({ roomId: rid, username }); + const [targetUsername] = users; + await inviteUser({ roomId: rid, username: targetUsername });apps/meteor/client/views/room/hooks/useUserInfoActions/actions/useChangeOwnerAction.tsx (1)
134-147: Use roomIsFederated in the handler and update depsAlign the handler with the new derived flag and drop
roomfrom deps to avoid unnecessary re-creations.-const changeOwnerAction = useEffectEvent(async () => handleChangeOwner()); - -const roomIsFederated = isRoomFederated(room); +const roomIsFederated = isRoomFederated(room); +const changeOwnerAction = useEffectEvent(async () => handleChangeOwner()); @@ - if (!isRoomFederated(room)) { + if (!roomIsFederated) { return toggleOwnerMutation.mutateAsync({ roomId: rid, userId: uid }); } @@ -}, [room, loggedUserId, loggedUserIsOwner, toggleOwnerMutation, rid, uid, t, setModal]); +}, [roomIsFederated, loggedUserId, loggedUserIsOwner, toggleOwnerMutation, rid, uid, t, setModal]);Please confirm no other logic inside
handleChangeOwnerdepends on the fullroomobject; otherwise keeproomin deps.Also applies to: 89-101, 129-131
apps/meteor/client/views/admin/rooms/EditRoom.tsx (1)
274-293: Disable “React when read only” when federated to avoid a non-actionable controlCurrently it’s always checked if federated but not disabled; users can click yet it won’t uncheck, which is confusing.
- <ToggleSwitch + <ToggleSwitch id={reactWhenReadOnly} {...field} - checked={value || roomIsFederated} + checked={value || roomIsFederated} + disabled={roomIsFederated} aria-describedby={`${reactWhenReadOnly}-hint`} />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
apps/meteor/client/views/admin/rooms/EditRoom.tsx(11 hunks)apps/meteor/client/views/room/contextualBar/Info/EditRoomInfo/EditRoomInfo.tsx(1 hunks)apps/meteor/client/views/room/contextualBar/RoomMembers/RoomMembersWithData.tsx(1 hunks)apps/meteor/client/views/room/hooks/useUserInfoActions/actions/useAddUserAction.tsx(1 hunks)apps/meteor/client/views/room/hooks/useUserInfoActions/actions/useChangeModeratorAction.tsx(1 hunks)apps/meteor/client/views/room/hooks/useUserInfoActions/actions/useChangeOwnerAction.tsx(1 hunks)apps/meteor/client/views/room/hooks/useUserInfoActions/actions/useRemoveUserAction.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (6)
apps/meteor/client/views/room/contextualBar/Info/EditRoomInfo/EditRoomInfo.tsx (1)
packages/core-typings/src/IRoom.ts (1)
isRoomFederated(109-109)
apps/meteor/client/views/room/hooks/useUserInfoActions/actions/useRemoveUserAction.tsx (1)
packages/core-typings/src/IRoom.ts (1)
isRoomFederated(109-109)
apps/meteor/client/views/room/hooks/useUserInfoActions/actions/useChangeOwnerAction.tsx (1)
packages/core-typings/src/IRoom.ts (1)
isRoomFederated(109-109)
apps/meteor/client/views/room/hooks/useUserInfoActions/actions/useChangeModeratorAction.tsx (2)
packages/core-typings/src/IRoom.ts (1)
isRoomFederated(109-109)apps/meteor/app/utils/lib/i18n.ts (1)
t(6-6)
apps/meteor/client/views/room/hooks/useUserInfoActions/actions/useAddUserAction.tsx (1)
packages/core-typings/src/IRoom.ts (1)
isRoomFederated(109-109)
apps/meteor/client/views/admin/rooms/EditRoom.tsx (1)
packages/core-typings/src/IRoom.ts (1)
isRoomFederated(109-109)
⏰ 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). (7)
- GitHub Check: 🔎 Code Check / TypeScript
- GitHub Check: 🔎 Code Check / Code Lint
- GitHub Check: 🔨 Test Storybook / Test Storybook
- GitHub Check: 🔨 Test Unit / Unit Tests
- GitHub Check: 📦 Meteor Build - coverage
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (6)
apps/meteor/client/views/room/contextualBar/Info/EditRoomInfo/EditRoomInfo.tsx (1)
82-82: Compute federated flag once per render — LGTMUsing a single
isFederatedper render simplifies conditions and avoids redundant calls.apps/meteor/client/views/room/hooks/useUserInfoActions/actions/useChangeModeratorAction.tsx (1)
146-161: Memo predicate switched to roomIsFederated — LGTMReplacing inline checks with a derived
roomIsFederatedand updating deps is correct and reduces churn.apps/meteor/client/views/room/contextualBar/RoomMembers/RoomMembersWithData.tsx (1)
79-81: Switch to useEffectEvent for handleBack — LGTMStable identity without stale closures; matches adjacent event handlers.
apps/meteor/client/views/room/hooks/useUserInfoActions/actions/useAddUserAction.tsx (1)
47-53: Derived roomIsFederated for gating — LGTMThe ternary using
roomIsFederatedreads clearer and avoids re-evaluations.apps/meteor/client/views/room/hooks/useUserInfoActions/actions/useRemoveUserAction.tsx (1)
44-49: Reuse derived flag for remove permissions — LGTM
roomIsFederatedsimplifies theuserCanRemovebranch; deps already includeuserCanRemove, so memo will refresh as needed.Also applies to: 113-125
apps/meteor/client/views/admin/rooms/EditRoom.tsx (1)
138-150: Centralized federated gating across form — LGTMConsolidating to
roomIsFederatedmakes the conditions clearer and consistent.Also applies to: 199-200, 213-214, 243-244, 265-265, 286-286, 303-303, 317-318, 342-342, 359-359
https://rocketchat.atlassian.net/browse/FDR-142
Proposed changes (including videos or screenshots)
Issue(s)
Steps to test or reproduce
Further comments
Summary by CodeRabbit
Bug Fixes
Refactor