refactor(client): factor out hasPermission / hasRole into pure factory#40493
Merged
Conversation
Member
Author
|
/jira ARCH-2116 |
|
Contributor
|
Looks like this PR is ready to merge! 🎉 |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📜 Recent review details⏰ 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). (5)
WalkthroughAuthorization logic was extracted into a dependency-injected factory, bound to live zustand store accessors for non-React callers, client modules were reduced to thin re-exports of the live binding, and the React provider was updated to use store snapshots with the new factory. ChangesAuthorization Architecture Refactor
🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
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}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
apps/meteor/app/authorization/client/liveAuthorizationFunctions.ts (1)
7-11: ⚡ Quick winDrop the explanatory block comment.
This module-level prose describes intent rather than encoding it. The rationale fits better in the PR description or commit message; readers can infer the live-binding behavior from
liveDepsand the factory call.♻️ Proposed cleanup
-// Bind the pure factory to live zustand store accessors. Each accessor reads -// fresh state on every call, so non-React callers (services, lib code, startup -// scripts) keep their previous "always reflects the current store" contract -// without going through Meteor's Tracker. React consumers should use the -// AuthorizationContext instead, which injects React-reactive snapshots. const liveDeps: AuthorizationDeps = {As per coding guidelines: "Avoid code comments in the implementation".
🤖 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/app/authorization/client/liveAuthorizationFunctions.ts` around lines 7 - 11, Remove the module-level explanatory block comment at the top of liveAuthorizationFunctions.ts; the rationale belongs in the PR/commit message, not code. Keep the implementation as-is (the live binding is already expressed by liveDeps and the factory invocation), so delete the multi-line prose and leave only the code that defines liveDeps and the factory call (and keep short, factual comments only if needed near liveDeps or AuthorizationContext).apps/meteor/client/providers/AuthorizationProvider.tsx (1)
27-30: ⚡ Quick winRemove the inline rationale block inside the component body.
The behavior is already clear from the four
useSyncExternalStorecalls below and the memoizedauth/contextValue. Implementation-level prose belongs in the PR description, not in the render body.♻️ Proposed cleanup
- // Reactive snapshots of every store the authorization helpers read. The - // provider re-renders whenever any of them change, and the new context - // value (memoized below) flows down so consumers re-render through React - // instead of through Tracker. const usersState = useSyncExternalStore(Users.use.subscribe, () => Users.use.getState());As per coding guidelines: "Avoid code comments in the implementation".
🤖 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 27 - 30, Remove the inline rationale comment block inside the AuthorizationProvider component body; keep the four useSyncExternalStore calls and the memoized auth/contextValue logic intact, but delete the explanatory prose that sits in the render body (i.e., the comment describing reactive snapshots and why the provider re-renders) so implementation-level commentary is removed from the component and can live in the PR description instead.
🤖 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.
Nitpick comments:
In `@apps/meteor/app/authorization/client/liveAuthorizationFunctions.ts`:
- Around line 7-11: Remove the module-level explanatory block comment at the top
of liveAuthorizationFunctions.ts; the rationale belongs in the PR/commit
message, not code. Keep the implementation as-is (the live binding is already
expressed by liveDeps and the factory invocation), so delete the multi-line
prose and leave only the code that defines liveDeps and the factory call (and
keep short, factual comments only if needed near liveDeps or
AuthorizationContext).
In `@apps/meteor/client/providers/AuthorizationProvider.tsx`:
- Around line 27-30: Remove the inline rationale comment block inside the
AuthorizationProvider component body; keep the four useSyncExternalStore calls
and the memoized auth/contextValue logic intact, but delete the explanatory
prose that sits in the render body (i.e., the comment describing reactive
snapshots and why the provider re-renders) so implementation-level commentary is
removed from the component and can live in the PR description instead.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c6ad4f82-ea37-4479-8691-b14a940356c1
📒 Files selected for processing (5)
apps/meteor/app/authorization/client/hasPermission.tsapps/meteor/app/authorization/client/hasRole.tsapps/meteor/app/authorization/client/liveAuthorizationFunctions.tsapps/meteor/app/authorization/lib/createAuthorizationFunctions.tsapps/meteor/client/providers/AuthorizationProvider.tsx
📜 Review details
⏰ 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)
- GitHub Check: 📦 Build Packages
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🧰 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/app/authorization/client/hasRole.tsapps/meteor/app/authorization/client/liveAuthorizationFunctions.tsapps/meteor/app/authorization/client/hasPermission.tsapps/meteor/client/providers/AuthorizationProvider.tsxapps/meteor/app/authorization/lib/createAuthorizationFunctions.ts
🧠 Learnings (4)
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In the Rocket.Chat repository, do not reference Biome lint rules in code review feedback. Biome is not used even if biome.json exists; only reference Biome rules if there is explicit, project-wide usage documented. For TypeScript files, review lint implications without Biome guidance unless the project enables Biome rules.
Applied to files:
apps/meteor/app/authorization/client/hasRole.tsapps/meteor/app/authorization/client/liveAuthorizationFunctions.tsapps/meteor/app/authorization/client/hasPermission.tsapps/meteor/app/authorization/lib/createAuthorizationFunctions.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In this repository (RocketChat/Rocket.Chat), Biome lint rules are not used even if a biome.json exists. When reviewing TypeScript files (e.g., packages/ui-voip/src/providers/useMediaSession.ts), ensure lint suggestions do not reference Biome-specific rules. Rely on general ESLint/TypeScript lint rules and project conventions instead.
Applied to files:
apps/meteor/app/authorization/client/hasRole.tsapps/meteor/app/authorization/client/liveAuthorizationFunctions.tsapps/meteor/app/authorization/client/hasPermission.tsapps/meteor/app/authorization/lib/createAuthorizationFunctions.ts
📚 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/app/authorization/client/hasRole.tsapps/meteor/app/authorization/client/liveAuthorizationFunctions.tsapps/meteor/app/authorization/client/hasPermission.tsapps/meteor/client/providers/AuthorizationProvider.tsxapps/meteor/app/authorization/lib/createAuthorizationFunctions.ts
📚 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
🔇 Additional comments (5)
apps/meteor/app/authorization/lib/createAuthorizationFunctions.ts (1)
1-101: LGTM!apps/meteor/client/providers/AuthorizationProvider.tsx (2)
31-47: LGTM!
51-67: ⚡ Quick winNo action needed; the scope handling is correct and intentional.
The review comment flagged an apparent inconsistency:
queryRolepassesscopedirectly toauth.hasRole()without coercing it toString(), whilequeryPermission,queryAtLeastOnePermission, andqueryAllPermissionsall coerce viascope ? String(scope) : undefined.However, the type definitions show this difference is justified:
queryRole'sscopeparameter is typed asIRoom['_id'], which resolves tostring(orBranded<string>in the federated variant). The other three methods acceptscope?: string | ObjectId. SincequeryRoleis semantically scoped to rooms and room IDs are always strings, no coercion is needed. The inconsistency in coercion reflects intentional type-level narrowing, not a bug.apps/meteor/app/authorization/client/hasRole.ts (1)
1-3: LGTM!apps/meteor/app/authorization/client/hasPermission.ts (1)
1-3: LGTM!
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #40493 +/- ##
========================================
Coverage 69.61% 69.61%
========================================
Files 3324 3322 -2
Lines 122651 122612 -39
Branches 21864 21851 -13
========================================
- Hits 85381 85361 -20
+ Misses 33939 33915 -24
- Partials 3331 3336 +5
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
9 tasks
The client-side hasPermission, hasAllPermission, hasAtLeastOnePermission, hasRole, and userHasAllPermission helpers used to reach into Users, Permissions, Roles, Subscriptions, PermissionsCachedStore and userIdStore through Meteor's Tracker bridge (meteor/watch.ts). The reactivity ran through Tracker.autorun inside AuthorizationProvider's createReactiveSubscriptionFactory wrapper. Split the helpers into two layers: - apps/meteor/app/authorization/lib/createAuthorizationFunctions.ts Pure factory. Takes accessors (getCurrentUserId, getUserRoles, getPermission, getRoleScope, hasSubscriptionRole, isReady) and returns hasRole / hasAllPermission / hasAtLeastOnePermission / hasPermission / userHasAllPermission. No store, no Meteor, no Tracker — trivially testable in isolation. - apps/meteor/app/authorization/client/liveAuthorizationFunctions.ts Binds the factory to live zustand store accessors via getState(). Each exported function reads fresh state on every call, so non-React callers (services, lib code, startup scripts) keep their previous "always-reflects-the-current-store" behaviour without going through Tracker. apps/meteor/app/authorization/client/hasPermission.ts and hasRole.ts now just re-export the singleton's methods, so the 24 existing client callers do not change. apps/meteor/client/providers/AuthorizationProvider.tsx no longer uses createReactiveSubscriptionFactory. It observes the four backing stores via useSyncExternalStore and feeds the snapshots into createAuthorizationFunctions inside useMemo; the context value's getSnapshot calls the resulting functions. The subscribe slot is a no-op because each store transition re-renders the provider, which rebuilds the context value, which is what React consumers ultimately observe. No behaviour change for callers. The remaining meteor/watch.ts consumer is settings.ts, migrated separately.
ggazzo
force-pushed
the
refactor/authorization-functions-factory
branch
from
a07381f to
3f257ad
Compare
… 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.
5 tasks
PR #40530 narrowed Subscriptions reactivity. Users.use was still observed in full: every Users record update — presence, status, last login, avatar etag — re-rendered the provider and re-evaluated `hasPermission` for every gated component in the tree. None of those fields affect any auth answer. The authorization factory only ever reads `Users.use.getState().get(id)?.roles`, and in practice that `id` is always the current user (the few callers of `userHasAllPermission` with an arbitrary userId don't exist in the codebase today). Introduce an `AuthorizableUser = Pick<IUser, '_id' | 'roles'>` type for documentation and switch the provider's Users subscription to a selector that returns just the current user's roles array. Zustand preserves the `roles` array reference across updates that don't touch that field, so `useSyncExternalStore`'s Object.is comparison keeps the provider still through presence churn — re-rendering only when the current user's role assignments actually change. For the (untaken) arbitrary-userId path, the factory still falls back to a live store read.
gabriellsh
approved these changes
May 14, 2026
ggazzo
added
the
stat: QA assured
dionisio-bot
Bot
added
the
stat: ready to merge
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
2 participants
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.
Summary
The client-side `hasPermission` / `hasAllPermission` / `hasAtLeastOnePermission` / `hasRole` / `userHasAllPermission` helpers used to reach into `Users`, `Permissions`, `Roles`, `Subscriptions`, `PermissionsCachedStore` and `userIdStore` through Meteor's Tracker bridge (`meteor/watch.ts`). The reactivity ran through `Tracker.autorun` inside `AuthorizationProvider`'s `createReactiveSubscriptionFactory` wrapper.
Split the helpers into two layers:
`apps/meteor/app/authorization/client/hasPermission.ts` and `hasRole.ts` now just re-export the singleton's methods, so the 24 existing client callers do not change.
`apps/meteor/client/providers/AuthorizationProvider.tsx` no longer uses `createReactiveSubscriptionFactory`. It observes the four backing stores via `useSyncExternalStore` and feeds the snapshots into `createAuthorizationFunctions` inside `useMemo`; the context value's `getSnapshot` calls the resulting functions. The `subscribe` slot is a no-op because each store transition re-renders the provider, which rebuilds the context value, which is what React consumers ultimately observe.
No behaviour change for callers. The remaining `meteor/watch.ts` consumer (`client/lib/settings/settings.ts`) is migrated separately.
Test plan
Task: ARCH-2142
Summary by CodeRabbit