Skip to content

perf(client): stop subscribing AuthorizationProvider to Subscriptions globally #40530

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
ggazzo merged 1 commit into from
May 14, 2026
Merged

perf(client): stop subscribing AuthorizationProvider to Subscriptions globally #40530

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 30 additions & 11 deletions apps/meteor/client/providers/AuthorizationProvider.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@ type AuthorizationProviderProps = {

const noopSubscribe = (): (() => void) => () => undefined;

const subscribeToSubscriptions = (onStoreChange: () => void): (() => void) => Subscriptions.use.subscribe(onStoreChange);

@coderabbitai coderabbitai Bot May 14, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

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.


const AuthorizationProvider = ({ children }: AuthorizationProviderProps) => {
const isReady = PermissionsCachedStore.useReady();

Expand All @@ -24,14 +26,16 @@ const AuthorizationProvider = ({ children }: AuthorizationProviderProps) => {

const userId = useUserId();

// 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.
// Reactive snapshots of the three stores that change infrequently (admin-driven
// or login-time only). A re-render here propagates the new auth answer through
// context to every consumer without forcing them to re-evaluate `hasPermission`
// for unrelated traffic. Subscriptions.use is intentionally NOT observed here
// — it updates on every incoming message, member change, and unread-count flip,
// so subscribing globally would re-render every gated component on every chat
// frame. Subscription-scoped permission checks subscribe per-call below.
const usersState = useSyncExternalStore(Users.use.subscribe, () => Users.use.getState());
const permissionsState = useSyncExternalStore(Permissions.use.subscribe, () => Permissions.use.getState());
const rolesState = useSyncExternalStore(Roles.use.subscribe, () => Roles.use.getState());
const subscriptionsState = useSyncExternalStore(Subscriptions.use.subscribe, () => Subscriptions.use.getState());

const auth = useMemo(
() =>
Expand All @@ -40,27 +44,42 @@ const AuthorizationProvider = ({ children }: AuthorizationProviderProps) => {
getUserRoles: (id) => usersState.get(id)?.roles,
getPermission: (id) => permissionsState.get(id),
getRoleScope: (id) => rolesState.get(id)?.scope,
hasSubscriptionRole: (rid, roleId) => subscriptionsState.find((s) => s.rid === rid)?.roles?.includes(roleId) ?? false,
// Read Subscriptions live — reactivity for scoped checks is wired through
// the per-call subscribe returned by queryPermission/queryRole below.
hasSubscriptionRole: (rid, roleId) =>
Subscriptions.use
.getState()
.find((s) => s.rid === rid)
?.roles?.includes(roleId) ?? false,
isReady: () => true,
}),
[userId, usersState, permissionsState, rolesState, subscriptionsState],
[userId, usersState, permissionsState, rolesState],
);

const contextValue = useMemo(
(): ContextType<typeof AuthorizationContext> => ({
// Callers without `scope` never touch Subscriptions (the factory short-circuits
// at the role-scope gate). They rely on context-value identity for re-renders
// from Users/Permissions/Roles changes — which is why subscribe is noop.
// Callers with a `scope` (room id) DO touch Subscriptions, so we attach a
// per-call subscribe to that store so they re-evaluate when subscriptions
// for the relevant room flip without dragging the rest of the tree along.
queryPermission: (permission, scope, scopeRoles) => [
noopSubscribe,
scope !== undefined ? subscribeToSubscriptions : noopSubscribe,
() => auth.hasPermission(String(permission), scope ? String(scope) : undefined, scopeRoles),
],
queryAtLeastOnePermission: (permissions, scope) => [
noopSubscribe,
scope !== undefined ? subscribeToSubscriptions : noopSubscribe,
() => auth.hasAtLeastOnePermission(permissions.map(String), scope ? String(scope) : undefined),
],
queryAllPermissions: (permissions, scope) => [
noopSubscribe,
scope !== undefined ? subscribeToSubscriptions : noopSubscribe,
() => auth.hasAllPermission(permissions.map(String), scope ? String(scope) : undefined),
],
queryRole: (role, scope) => [noopSubscribe, () => !!userId && auth.hasRole(userId, String(role), scope)],
queryRole: (role, scope) => [
scope !== undefined ? subscribeToSubscriptions : noopSubscribe,
() => !!userId && auth.hasRole(userId, String(role), scope),
],
getRoles: () => Roles.state.records,
subscribeToRoles: (callback) => Roles.use.subscribe(callback),
}),
Expand Down
Loading