refactor(client): replace useReactiveValue with useSyncExternalStore in providers#40446
Merged
tassoevan merged 2 commits intoMay 13, 2026
Conversation
Contributor
|
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 |
|
Contributor
WalkthroughThe PR replaces the ChangesReactive Store Subscription Modernization
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
🚥 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)
Git: Failed to clone repository. Please run the 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 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## refactor/auth-provider-login-token-helper #40446 +/- ##
=============================================================================
- Coverage 69.66% 69.66% -0.01%
=============================================================================
Files 3318 3317 -1
Lines 122002 122030 +28
Branches 21810 21803 -7
=============================================================================
+ Hits 84994 85013 +19
- Misses 33677 33690 +13
+ Partials 3331 3327 -4
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
ggazzo
force-pushed
the
refactor/sdk-tracker-easy-autoruns
branch
from
77c465c to
6bc7cc2
Compare
ggazzo
force-pushed
the
refactor/sdk-status-loggingin-providers
branch
from
a8afa77 to
7ad8ed8
Compare
ggazzo
added a commit
that referenced
this pull request
May 7, 2026
Meteor's socket-stream-client only accepts 'message' / 'reset' / 'disconnect' as event types — `_stream.on('connected', ...)` throws `Error: unknown event type: connected`. The throw propagated up through `connection.on(...)` to `CachedStore.performInitialization`, aborting it before `setupListener()` could open the stream subscription. The error was caught by `init().catch(console.error)` so the cache silently appeared ready while real-time updates from public-settings, subscriptions, etc. never reached the client.
Use 'reset' (fired when a new DDP session is established, effectively the post-reconnect signal) plus 'disconnect' instead. The bridge's `safeMeteorStatus()` readback covers determining the actual state for emitting `connected`/`disconnected`/`connection`.
Caught by deterministic E2E failures across PR #40445 / #40446: homepage custom-body, omnichannel department removal toggle, system-messages, account-security 2FA — all tests that change a setting via REST and expect the client UI to reflect it through the public-settings stream.
ggazzo
force-pushed
the
refactor/sdk-status-loggingin-providers
branch
from
7ad8ed8 to
7a07715
Compare
ggazzo
added a commit
that referenced
this pull request
May 8, 2026
Meteor's socket-stream-client only accepts 'message' / 'reset' / 'disconnect' as event types — `_stream.on('connected', ...)` throws `Error: unknown event type: connected`. The throw propagated up through `connection.on(...)` to `CachedStore.performInitialization`, aborting it before `setupListener()` could open the stream subscription. The error was caught by `init().catch(console.error)` so the cache silently appeared ready while real-time updates from public-settings, subscriptions, etc. never reached the client.
Use 'reset' (fired when a new DDP session is established, effectively the post-reconnect signal) plus 'disconnect' instead. The bridge's `safeMeteorStatus()` readback covers determining the actual state for emitting `connected`/`disconnected`/`connection`.
Caught by deterministic E2E failures across PR #40445 / #40446: homepage custom-body, omnichannel department removal toggle, system-messages, account-security 2FA — all tests that change a setting via REST and expect the client UI to reflect it through the public-settings stream.
ggazzo
force-pushed
the
refactor/sdk-tracker-easy-autoruns
branch
from
f8d46b0 to
8833762
Compare
ggazzo
force-pushed
the
refactor/sdk-status-loggingin-providers
branch
from
7a07715 to
b93009a
Compare
Member
Author
|
/jira ARCH-2116 |
ggazzo
added a commit
that referenced
this pull request
May 11, 2026
Meteor's socket-stream-client only accepts 'message' / 'reset' / 'disconnect' as event types — `_stream.on('connected', ...)` throws `Error: unknown event type: connected`. The throw propagated up through `connection.on(...)` to `CachedStore.performInitialization`, aborting it before `setupListener()` could open the stream subscription. The error was caught by `init().catch(console.error)` so the cache silently appeared ready while real-time updates from public-settings, subscriptions, etc. never reached the client.
Use 'reset' (fired when a new DDP session is established, effectively the post-reconnect signal) plus 'disconnect' instead. The bridge's `safeMeteorStatus()` readback covers determining the actual state for emitting `connected`/`disconnected`/`connection`.
Caught by deterministic E2E failures across PR #40445 / #40446: homepage custom-body, omnichannel department removal toggle, system-messages, account-security 2FA — all tests that change a setting via REST and expect the client UI to reflect it through the public-settings stream.
ggazzo
force-pushed
the
refactor/sdk-tracker-easy-autoruns
branch
from
b0b9796 to
a8d5dc3
Compare
…in providers
ServerProvider and AuthenticationProvider were the only callers of useReactiveValue. Both now wire their context values through useSyncExternalStore + an explicit subscribe/getSnapshot pair, dropping the Tracker.autorun that lived inside createReactiveSubscriptionFactory:
- ServerProvider listens on sdk.connection.on('connection', …) — works in both transport modes since the prior PR bridged Meteor's _stream events into that emitter. The standalone Tracker.Dependency (ddpSdkStatusDep) is gone; the SDK's own emitter is now the single source of transition events. Snapshot caching with shallow-compare keeps useSyncExternalStore identity-stable.
- AuthenticationProvider monkey-patches Accounts._setLoggingIn (Meteor's internal flip, also used in killMeteorStream.ts) to fan out logging-in transitions without entering a Tracker computation. The patch is installed on first subscribe.
useReactiveValue had no remaining consumers and is removed. createReactiveSubscriptionFactory stays — AuthorizationProvider's permission queries and UserProvider's queryPreference still rely on its Tracker bridge for reading hasPermission/hasRole/getUserPreference reactively.
ggazzo
force-pushed
the
refactor/sdk-status-loggingin-providers
branch
from
b93009a to
e02d28c
Compare
ggazzo
changed the base branch from
refactor/sdk-tracker-easy-autoruns
to
refactor/auth-provider-login-token-helper
4 tasks
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/ServerProvider.tsx`:
- Around line 136-157: The cachedStatus is computed only at module load and can
be stale until the first subscriber installs the bridge; modify
ensureStatusBridge so after setting statusBridgeStarted = true and before
returning it refreshes cachedStatus by calling computeStatus() (and if you want
to preserve existing behavior also compare with the old cachedStatus and notify
statusListeners if it changed) — update the function ensureStatusBridge to
recompute cachedStatus immediately (using computeStatus) when installing the
getDdpSdk().connection.on('connection', ...) bridge so getStatusSnapshot/readers
see the current state.
🪄 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: 452c3498-d91b-448f-b04b-e0858d03432c
📒 Files selected for processing (4)
.changeset/sdk-status-loggingin-providers.mdapps/meteor/client/hooks/useReactiveValue.tsapps/meteor/client/providers/AuthenticationProvider/AuthenticationProvider.tsxapps/meteor/client/providers/ServerProvider.tsx
💤 Files with no reviewable changes (1)
- apps/meteor/client/hooks/useReactiveValue.ts
📜 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/AuthenticationProvider/AuthenticationProvider.tsxapps/meteor/client/providers/ServerProvider.tsx
🧠 Learnings (3)
📚 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/AuthenticationProvider/AuthenticationProvider.tsxapps/meteor/client/providers/ServerProvider.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/AuthenticationProvider/AuthenticationProvider.tsxapps/meteor/client/providers/ServerProvider.tsx
📚 Learning: 2026-03-16T21:50:37.589Z
Learnt from: amitb0ra
Repo: RocketChat/Rocket.Chat PR: 39676
File: .changeset/migrate-users-register-openapi.md:3-3
Timestamp: 2026-03-16T21:50:37.589Z
Learning: For changes related to OpenAPI migrations in Rocket.Chat/OpenAPI, when removing endpoint types and validators from rocket.chat/rest-typings (e.g., UserRegisterParamsPOST, /v1/users.register) document this as a minor changeset (not breaking) per RocketChat/Rocket.Chat-Open-API#150 Rule 7. Note that the endpoint type is re-exposed via a module augmentation .d.ts in the consuming package (e.g., packages/web-ui-registration/src/users-register.d.ts). In reviews, ensure the changeset clearly states: this is a non-breaking change, the major version should not be bumped, and the changeset reflects a minor version bump. Do not treat this as a breaking change during OpenAPI migrations.
Applied to files:
.changeset/sdk-status-loggingin-providers.md
🔇 Additional comments (3)
apps/meteor/client/providers/AuthenticationProvider/AuthenticationProvider.tsx (1)
7-7: LGTM!Also applies to: 31-58, 68-68
apps/meteor/client/providers/ServerProvider.tsx (1)
15-15: LGTM!Also applies to: 164-164
.changeset/sdk-status-loggingin-providers.md (1)
1-5: LGTM!
Comment on lines
+136
to
+157
| let cachedStatus: CombinedStatus = computeStatus(); | ||
| const statusListeners = new Set<() => void>(); | ||
| let statusBridgeStarted = false; | ||
|
|
||
| const ensureStatusBridge = (): void => { | ||
| if (statusBridgeStarted) return; | ||
| statusBridgeStarted = true; | ||
| getDdpSdk().connection.on('connection', () => { | ||
| const next = computeStatus(); | ||
| if (isStatusEqual(cachedStatus, next)) return; | ||
| cachedStatus = next; | ||
| statusListeners.forEach((cb) => cb()); | ||
| }); | ||
| }; | ||
|
|
||
| const subscribeStatus = (cb: () => void): (() => void) => { | ||
| ensureStatusBridge(); | ||
| statusListeners.add(cb); | ||
| return () => { | ||
| statusListeners.delete(cb); | ||
| }; | ||
| }; |
Contributor
There was a problem hiding this comment.
Stale cachedStatus between module load and first subscribe can leave the status bar wedged.
cachedStatus is computed once at module-evaluation time (Line 136), and the SDK 'connection' listener is only installed when the first component subscribes (ensureStatusBridge, Lines 140-149). Any 'connection' transition that fires in the window between module load and the first subscribe is dropped — no listener yet, and cachedStatus is never refreshed. getStatusSnapshot then keeps handing React the stale snapshot, and because useSyncExternalStore's post-subscribe re-check also calls getStatusSnapshot (still stale), no re-render is scheduled. On a stable connection that produces no further 'connection' emits, the ConnectionStatusBar can stay stuck on the initial value (e.g. connecting). The previous Tracker.autorun path re-read on every invalidation, so this is a new regression.
Refresh the cache when the bridge is installed so the first subscriber sees current state:
🛠️ Proposed fix
const ensureStatusBridge = (): void => {
if (statusBridgeStarted) return;
statusBridgeStarted = true;
getDdpSdk().connection.on('connection', () => {
const next = computeStatus();
if (isStatusEqual(cachedStatus, next)) return;
cachedStatus = next;
statusListeners.forEach((cb) => cb());
});
+ cachedStatus = computeStatus();
};🤖 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/ServerProvider.tsx` around lines 136 - 157, The
cachedStatus is computed only at module load and can be stale until the first
subscriber installs the bridge; modify ensureStatusBridge so after setting
statusBridgeStarted = true and before returning it refreshes cachedStatus by
calling computeStatus() (and if you want to preserve existing behavior also
compare with the old cachedStatus and notify statusListeners if it changed) —
update the function ensureStatusBridge to recompute cachedStatus immediately
(using computeStatus) when installing the
getDdpSdk().connection.on('connection', ...) bridge so getStatusSnapshot/readers
see the current state.
tassoevan
approved these changes
May 13, 2026
tassoevan
merged commit addc58e
into
refactor/auth-provider-login-token-helper
25 checks passed
3 tasks
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
`useReactiveValue` had only 2 consumers — `ServerProvider` and `AuthenticationProvider` — both wired through `useReactiveValue → createReactiveSubscriptionFactory → Tracker.autorun`. Both now use `useSyncExternalStore` directly with bespoke subscribe/getSnapshot pairs, eliminating Tracker from these paths.
`createReactiveSubscriptionFactory` stays — `AuthorizationProvider` (permission queries) and `UserProvider` (`queryPreference`) still rely on its Tracker bridge for reading `hasPermission` / `hasRole` / `getUserPreference` reactively. Migrating those is a follow-up that requires changing the underlying read APIs to expose explicit subscribers.
Net Tracker imports
Before this PR: 7 client files. After: 6 (`userAndUsers.ts`, `stubMeteorStream.ts`, `Cursor.ts`, `ObserveHandle.ts`, `watch.ts`, `createReactiveSubscriptionFactory.ts`).
Test plan
Task: ARCH-2138
Summary by CodeRabbit