perf(client): stop subscribing AuthorizationProvider to Subscriptions globally#40530
Conversation
… globally The previous PR (#40493) observed Users + Permissions + Roles + Subscriptions via useSyncExternalStore at the provider level. Any change to any of those stores re-renders the provider, which re-renders every consumer of usePermission / useAtLeastOnePermission / useAllPermissions / useRole. Subscriptions updates on every incoming message, every unread- count flip, every member change — i.e. constantly during normal chat usage — so all 232 permission-gated components in the tree were churning React passes on every chat frame. Of those 232 consumers, 179 (77%) call the hooks without a `scope` arg. The factory short-circuits at the role-scope gate before touching Subscriptions, so those callers never depend on Subscriptions reactivity. Only the 53 consumers that pass a room id need Subscriptions to fire re-renders, and they care about subscription changes for that specific room anyway. Split the reactivity: - Provider keeps a global subscribe on Users + Permissions + Roles (admin- driven, infrequent — re-render fan-out is acceptable). - Subscriptions reads go LIVE via Subscriptions.use.getState() inside the factory's hasSubscriptionRole accessor. - queryPermission / queryAtLeastOnePermission / queryAllPermissions / queryRole return a per-call subscribe: noop when no scope is passed, Subscriptions.use.subscribe when scope is set. Net: 77% of permission-gated components stop re-rendering on every chat frame. The remaining 23% still react to subscription changes for the room they're gating on — same as before — but only to that store, and without dragging the rest of the tree through React reconciliation.
|
/jira ARCH-2116 |
|
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 |
|
Walkthrough
ChangesScope-scoped subscription reactivity
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![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
🧹 Nitpick comments (1)
apps/meteor/client/providers/AuthorizationProvider.tsx (1)
29-35: ⚡ Quick winRemove the new implementation comments.
These blocks restate PR rationale in the source. Please keep that context in the PR/commit history instead.
As per coding guidelines,
Avoid code comments in the implementation.Also applies to: 47-48, 61-66
🤖 Prompt for 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. In `@apps/meteor/client/providers/AuthorizationProvider.tsx` around lines 29 - 35, Remove the new explanatory implementation comments added in AuthorizationProvider.tsx (the multi-line block describing "Reactive snapshots..." and the other blocks around Subscriptions.use / hasPermission at the other noted locations), deleting those PR-rationale comment blocks so only code remains; ensure you leave no leftover commented text in the AuthorizationProvider component and run formatting/linting to keep the file syntactically clean.
🤖 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/providers/AuthorizationProvider.tsx`:
- Line 15: subscribeToSubscriptions currently calls Subscriptions.use.subscribe
without a room scope so every scoped query listener is notified on any change;
modify subscribeToSubscriptions to accept a room id (e.g., scope or roomId) and
when Subscriptions.use.subscribe invokes the callback, compare the new snapshot
for that room to the previous snapshot and only call onStoreChange if that
room's subscription data actually changed. Update any callers (the scoped query*
hooks) to pass the room id into subscribeToSubscriptions and ensure the
comparison logic uses the same keying used by Subscriptions (e.g.,
subscriptionMap[roomId] or similar) so only the relevant room's updates trigger
the listener.
---
Nitpick comments:
In `@apps/meteor/client/providers/AuthorizationProvider.tsx`:
- Around line 29-35: Remove the new explanatory implementation comments added in
AuthorizationProvider.tsx (the multi-line block describing "Reactive
snapshots..." and the other blocks around Subscriptions.use / hasPermission at
the other noted locations), deleting those PR-rationale comment blocks so only
code remains; ensure you leave no leftover commented text in the
AuthorizationProvider component and run formatting/linting to keep the file
syntactically clean.
🪄 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: d0c3dffb-84da-4cfc-864c-fd2a97410cde
📒 Files selected for processing (1)
apps/meteor/client/providers/AuthorizationProvider.tsx
📜 Review details
🧰 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/providers/AuthorizationProvider.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/providers/AuthorizationProvider.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/providers/AuthorizationProvider.tsx
🔇 Additional comments (1)
apps/meteor/client/providers/AuthorizationProvider.tsx (1)
49-56: LGTM!
|
|
||
| const noopSubscribe = (): (() => void) => () => undefined; | ||
|
|
||
| const subscribeToSubscriptions = (onStoreChange: () => void): (() => void) => Subscriptions.use.subscribe(onStoreChange); |
There was a problem hiding this comment.
Scoped queries are still subscribing to the whole Subscriptions store.
subscribeToSubscriptions never receives the room id, so every scoped query* installs the same global Subscriptions.use.subscribe listener. That means scoped authorization hooks still invalidate on unrelated room updates, which falls short of the room-scoped reactivity this refactor is targeting. Thread scope into the subscribe helper and only notify when that room’s subscription snapshot changes.
Also applies to: 67-82
🤖 Prompt for 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.
In `@apps/meteor/client/providers/AuthorizationProvider.tsx` at line 15,
subscribeToSubscriptions currently calls Subscriptions.use.subscribe without a
room scope so every scoped query listener is notified on any change; modify
subscribeToSubscriptions to accept a room id (e.g., scope or roomId) and when
Subscriptions.use.subscribe invokes the callback, compare the new snapshot for
that room to the previous snapshot and only call onStoreChange if that room's
subscription data actually changed. Update any callers (the scoped query* hooks)
to pass the room id into subscribeToSubscriptions and ensure the comparison
logic uses the same keying used by Subscriptions (e.g., subscriptionMap[roomId]
or similar) so only the relevant room's updates trigger the listener.
ggazzo
merged commit b962604
into
refactor/authorization-functions-factory
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## refactor/authorization-functions-factory #40530 +/- ##
=========================================================================
Coverage 69.61% 69.62%
=========================================================================
Files 3322 3322
Lines 122612 122606 -6
Branches 21851 21867 +16
=========================================================================
+ Hits 85361 85366 +5
+ Misses 33915 33907 -8
+ Partials 3336 3333 -3
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
1 participant
Summary
The previous PR (#40493) observed `Users` + `Permissions` + `Roles` + `Subscriptions` via `useSyncExternalStore` at the provider level. Any change to any of those stores re-renders the provider, which re-renders every consumer of `usePermission` / `useAtLeastOnePermission` / `useAllPermissions` / `useRole`. `Subscriptions` updates on every incoming message, every unread-count flip, every member change — i.e. constantly during normal chat usage — so all 232 permission-gated components in the tree were churning React passes on every chat frame.
Of those 232 consumers, 179 (77%) call the hooks without a `scope` arg. The factory short-circuits at the role-scope gate before touching `Subscriptions`, so those callers never depend on `Subscriptions` reactivity. Only the 53 consumers that pass a room id need `Subscriptions` to fire re-renders, and they care about subscription changes for that specific room anyway.
Strategy
Impact
Net: 77% of permission-gated components stop re-rendering on every chat frame. The remaining 23% still react to subscription changes for the room they're gating on — same as before — but only to that store, and without dragging the rest of the tree through React reconciliation.
Test plan
Task: ARCH-2147
Summary by CodeRabbit
Release Notes