Restore permission change detection in useFetchChatData#1332
Open
ManojPawar2 wants to merge 1 commit into
Open
Restore permission change detection in useFetchChatData#1332ManojPawar2 wants to merge 1 commit into
ManojPawar2 wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Pull request overview
Restores the intended permission change-detection behavior in useFetchChatData by persisting the previously fetched raw permissions alongside the derived permissions map, so the hook can correctly skip rebuilding/reapplying permissions when nothing has changed.
Changes:
- Store
raw: permissionsintopermissionsRef.currentso the JSON comparison guard can short-circuit correctly. - Prevent redundant
createPermissionsMap()work andapplyPermissions()store writes when permissions are unchanged.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…eFetchChatData
`fetchAndSetPermissions` guarded its work by comparing the freshly
fetched permissions against `permissionsRef.current.raw`, but the ref
was only ever assigned `{ map }`. With `.raw` left undefined, the guard
`JSON.stringify(permissions) !== JSON.stringify(permissionsRef.current.raw)`
always evaluated to true.
As a result the permissions map was rebuilt and `applyPermissions` ran on
every call, firing all six Zustand permission setters and triggering
needless state updates and re-renders even when permissions had not
changed.
Store the raw permissions alongside the map so the comparison can
short-circuit and skip the redundant work when nothing has changed.
Closes RocketChat#1317
48ca63c to
f4d3829
Compare
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
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.
Overview
fetchAndSetPermissionsinpackages/react/src/hooks/useFetchChatData.jsis designed to fetch permissions and only rebuild and re-apply them when they have actually changed since the last fetch. That optimization was silentlybroken — the change-detection guard could never short-circuit, so permissions were rebuilt and re-applied on every call. This PR fixes the root cause with a one-line change.
Fixes #1317
Root cause
The function caches the previous result in
permissionsRefand guards its work by comparing the incoming permissions against the previously stored raw permissions:However, the ref was only ever assigned
{ map: permissionsMap }, leaving.rawpermanentlyundefined. The guard therefore reduced tosomeJSONString !== undefined, which is alwaystrue— so thechange-detection never took effect.
Impact
Because the guard never short-circuited, every invocation of
fetchAndSetPermissionswould:MapviacreatePermissionsMap, andapplyPermissions, firing all six Zustand permission setters (viewUserInfo,deleteMessage,deleteOwnMessage,forceDeleteMessage,userPin,editMessage).This resulted in redundant store writes and unnecessary re-renders even when the permissions returned by the server had not changed.
Fix
Persist the raw permissions alongside the map so the comparison has a real previous value and can correctly skip the work when nothing changed:
packages/react/src/hooks/useFetchChatData.jsAcceptance Criteria fulfillment
fetchAndSetPermissionsno longer rebuilds the permissions map on every callapplyPermissionsand the six permission setters only run when permissions actually changeHow to test
fetchAndSetPermissionsrepeatedly with unchanged permissions and confirm the six setters no longer fire on each call (no redundant updates).Video/Screenshots
N/A — no visual change. This PR removes redundant state updates; user-facing
behavior is identical when permissions actually change.