chore(@rocket.chat/ddp-client): introduce experimental Meteor independent DDP client (re-add)#40430
Conversation
…on status snapshot Two regressions reintroduced by reapplying #40301: - `ddpOverREST` rebuilt every REST-side error with `error: 'unknown'`, dropping the original DDP error code. The Rocket.Chat REST middleware encodes the original Meteor error inside `body.message` as a DDP `result` frame for `method.call` / `method.callAnon`, so forward that frame untouched when present. Only fall back to a synthetic frame for auth-middleware errors (e.g. 401) where `body.message` is a plain string. Without this, `withAsyncTOTP` no longer recognised `totp-required` and the 2FA modal never opened for any Meteor method protected by `twoFactorRequired`. - `ServerProvider.getStatus` dropped the `{ ...Meteor.status() }` spread on the legacy path. `Meteor.status()` returns the same internal object on every call (mutated in place), so `useReactiveValue` / `useSyncExternalStore` compared snapshots by identity and stopped re-rendering. `ConnectionStatusBar` froze on connect/drop until a hard reload. Restore the spread.
|
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 |
🦋 Changeset detectedLatest commit: 57e1e2d The changes in this PR will be included in the next version bump. This PR includes changesets to release 42 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
WalkthroughThis PR introduces an experimental SDK-over-DDP transport that routes Meteor DDP traffic through the ChangesSDK-over-DDP Transport
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes The PR spans 20+ files with heterogeneous logic across multiple interconnected layers: SDK singleton management, Meteor interception/stubbing, authentication coordination, and conditional branching throughout. While individual files follow consistent patterns, the cumulative complexity of ensuring correct transport routing across presence, login, streaming, and collection updates demands careful verification of state synchronization and error handling paths. Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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
added
the
type: feature
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
🧹 Nitpick comments (9)
apps/meteor/client/meteor/overrides/ddpOverREST.ts (2)
9-37: 💤 Low valueMinor:
shouldBypasscould reuse the newisResumeLoginhelper.Line 24 inlines the same
method === 'login' && params?.[0]?.resumecheck that line 9 just extracted. Consolidating keeps both call sites in sync if the resume-detection rule changes.♻️ Proposed fix
-const shouldBypass = ({ msg, method, params }: Meteor.IDDPMessage): boolean => { - if (msg !== 'method') { +const shouldBypass = (message: Meteor.IDDPMessage): boolean => { + const { msg, method } = message; + if (msg !== 'method') { return true; } - // In microservices CI, ddp-streamer-service registers `login`, `logout`, + // In microservices CI, ddp-streamer-service registers `login`, `logout`, // `setUserStatus`, and `UserPresence:*` as native methods (configureServer.ts // in ee/apps/ddp-streamer); every other method delegates to the Meteor // service via callMethodWithToken (extra hop). Bypassing these to Meteor's // own WS routes them straight to ddp-streamer for the fast path; routing // them through REST would wedge them on the slow rocketchat-main path // instead, blowing past the 5s `expect(...).toBeVisible()` timeouts the // post-relogin tests rely on. - if (method === 'login' && params?.[0]?.resume) { + if (isResumeLogin(message)) { return true; }🤖 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/meteor/overrides/ddpOverREST.ts` around lines 9 - 37, The inline resume-login check in shouldBypass duplicates the helper isResumeLogin; replace the condition if (method === 'login' && params?.[0]?.resume) with a call to isResumeLogin({ method, params }) so shouldBypass reuses the centralized logic (remove the duplicated expression and use the isResumeLogin helper instead).
45-57: 💤 Low valueConsider containing
_streamHandlers.onMessageasync rejections.
_streamHandlers.onMessagereturns a Promise (the message handler is implemented as an async generator, as called out inddpSdkCollectionBridge.tslines 73-81). If a stale frame ever lands here without an active invoker, an unhandled rejection would bubble out of this scope. Mirroring the bridge's wrap pattern keeps behavior consistent across both call sites.♻️ Proposed fix
- this._streamHandlers.onMessage(resultMessage); + const result = this._streamHandlers.onMessage(resultMessage) as unknown; + if (result && typeof (result as Promise<unknown>).then === 'function') { + (result as Promise<unknown>).catch((err) => { + console.warn('[ddpOverREST] processResult drop (async)', err); + }); + }🤖 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/meteor/overrides/ddpOverREST.ts` around lines 45 - 57, The processResult function should guard against unhandled promise rejections from this._streamHandlers.onMessage: instead of calling it directly, invoke it through the same async-wrap pattern used by the bridge (i.e., wrap the returned Promise and attach a .catch handler so rejections are handled and routed to the stream handler's error/cleanup path), keeping the early-return check on this._methodInvokers[message.id] and otherwise forwarding the message with this._livedata_data and then calling this._streamHandlers.onMessage via the wrapped/catch-handled invocation to prevent unhandled rejections.apps/meteor/client/lib/sdk/meteorBackedSdk.ts (2)
60-70: 💤 Low valueAvoid mutating Meteor's
SubscriptionHandleviaObject.assign.
Object.assign(sub, …)overwrites Meteor's reactiveready()(boolean reactive) with one that returnsPromise.resolve()on the same object. While the cast hides this from typing, anything that still treats the returned handle as a Meteor handle (e.g. shared subscription pools, Tracker code re-readingready()) sees broken semantics. Prefer returning a new wrapper object that holdssubas the underlying handle.♻️ Proposed fix
- const sub = (Meteor.connection.subscribe as (name: string, ...args: unknown[]) => Meteor.SubscriptionHandle)(name, ...args); - // Approximate DDPSDK's Subscription shape with Meteor's handle. The - // codebase only reads `stop`/`ready`/`isReady`/`id` from it. - return Object.assign(sub, { - id: '', - isReady: false, - ready: () => Promise.resolve(), - onChange: () => undefined, - }) as unknown as ReturnType<DDPSDK['client']['subscribe']>; + const sub = (Meteor.connection.subscribe as (name: string, ...args: unknown[]) => Meteor.SubscriptionHandle)(name, ...args); + return { + id: '', + isReady: false, + ready: () => Promise.resolve(), + onChange: () => undefined, + stop: () => sub.stop(), + } as unknown as ReturnType<DDPSDK['client']['subscribe']>;🤖 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/lib/sdk/meteorBackedSdk.ts` around lines 60 - 70, In the subscribe function, avoid mutating Meteor.connection.subscribe's SubscriptionHandle (do not use Object.assign(sub, ...)); instead return a new wrapper object that holds the original sub and delegates stop, ready (returning sub.ready()), isReady (reflecting sub.ready or sub.isReady), and id to the underlying handle while adding the onChange method and matching the DDPSDK['client']['subscribe'] return shape; update the wrapper to call sub.stop(), forward sub.ready()/sub.isReady access, and expose id from sub without overwriting the original SubscriptionHandle.
81-98: ⚡ Quick winFix
onCollectionlistener accumulation on Meteor.connection._stream.Each
onCollection(id, callback)invocation permanently binds a handler to the underlying stream'seventCallbacks.messagearray with no removal mechanism. Repeated calls (during navigation, room switches) cause accumulating handlers that match every DDP frame N times. While Meteor's_streamindeed lacks anoffmethod in this implementation, the handler leak is still preventable: store a reference to the unsubscribe callback returned bystream.on()(if available) or maintain a cleanup registry to filter handlers instead of binding direct callbacks.🤖 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/lib/sdk/meteorBackedSdk.ts` around lines 81 - 98, The onCollection function currently attaches a permanent handler to Meteor.connection._stream via stream.on('message', handler) causing listener accumulation; fix it by making each handler removable: when calling stream.on in onCollection, capture and return the unsubscribe function if stream.on provides one (use that to remove the handler), and if stream.on does not return an unsubscribe, wrap handler with an active flag registered in a local cleanup registry (or closure) so the returned function flips the flag to ignore future invocations; update references to onCollection, handler, stream.on and noopUnsubscribe so callers get a real unsubscribe that prevents the leak.apps/meteor/client/lib/sdk/ddpSdk.ts (2)
152-162: 💤 Low value
isAuthErrorreferenced before its declaration.It's a
constarrow function (not a hoistedfunctiondeclaration) declared at L152 but referenced insideensureConnectedAndAuthenticatedat L129. Runtime is fine — the reference resolves only when the exported function is later invoked, by which point the module has finished evaluating — but if anything ever invokesensureConnectedAndAuthenticatedsynchronously during module init (e.g., a top-levelawaitimport side effect), this would TDZ. MoveisAuthErrorabove its first use to keep the file readable and TDZ-safe.🤖 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/lib/sdk/ddpSdk.ts` around lines 152 - 162, Move the isAuthError declaration so it appears before its first use in ensureConnectedAndAuthenticated (or convert it to a hoisted function declaration) to avoid a temporal-dead-zone risk; locate the const isAuthError = (...) arrow function and place it above the ensureConnectedAndAuthenticated function (or replace it with function isAuthError(...) { ... }) so the identifier is available at module-evaluation time.
217-217: ⚡ Quick winInconsistent flag check — re-evaluates
isSdkTransportEnabled()instead of using the module-cached value.L10 already captures
const sdkTransportEnabled = isSdkTransportEnabled();, and every other site in this file (and inSDKClient.ts/ServerProvider.tsx) reads the cached constant. CallingisSdkTransportEnabled()again here means a different result is theoretically possible (e.g., if the implementation reads from a mutable source), and the dead code at L52 vs L217 is jarring on a quick read.🔧
-if (typeof window !== 'undefined' && isSdkTransportEnabled()) { +if (typeof window !== 'undefined' && sdkTransportEnabled) {🤖 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/lib/sdk/ddpSdk.ts` at line 217, The condition re-calls isSdkTransportEnabled() instead of using the previously cached constant sdkTransportEnabled; change the if-check to use sdkTransportEnabled so the module-level flag is used consistently (e.g., replace isSdkTransportEnabled() with sdkTransportEnabled in the window-check branch around the code using window), ensuring all references in this module match the module-cached value and avoid re-evaluation.apps/meteor/app/utils/client/lib/SDKClient.ts (3)
149-250: ⚡ Quick winSignificant duplication with
createNewMeteorStream.The
onChange,ready,onError,onStop,isReady, andunsubListblocks increateNewDdpSdkStream(lines 206–248) are nearly identical to the equivalents increateNewMeteorStream(lines 93–146). Only thesubscribe/onCollectionplumbing is transport-specific. Consider extracting a sharedmakeStreamMapValue(meta, ee, { stop })factory that produces the common surface, leaving each transport responsible only for triggeringee.emit('ready' | 'error' | 'stop'). That removes ~40 lines and prevents the two paths from drifting (e.g., the legacyonChangestill emits an extra blank line that the SDK variant dropped — see line 97 vs line 208).🤖 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/utils/client/lib/SDKClient.ts` around lines 149 - 250, The two stream creators duplicate the subscriber surface (onChange, ready, onError, onStop, isReady, unsubList) between createNewDdpSdkStream and createNewMeteorStream; extract that common behavior into a small factory (e.g., makeStreamMapValue(meta, ee, handlers)) that returns the StreamMapValue shape and accepts a transport-specific stop callback, then have createNewDdpSdkStream and createNewMeteorStream only perform their transport-specific subscribe/onCollection wiring and call ee.emit('ready'|'error'|'stop') as needed; update both functions to use makeStreamMapValue so the shared logic lives in one place and remove the duplicated blocks.
198-203: 💤 Low valuePer-subscription
onCollectionlisteners scale O(N²) with active subscriptions.Every
createNewDdpSdkStreamcall registers its own listener onstream-${streamName}, then filters byeventName !== key. With N active subscriptions on the same collection (e.g.,notify-user,notify-room), every incoming frame walks all N listeners just to dispatch one. ThestreamProxyalready keys bystream-${streamName}/${key}— you can register a singleonCollectionperstreamName(memoized in aMap<string, () => void>) and let the proxy's keyed emit do the routing, mirroring how the legacyMeteor.connection._streambridge at lines 270–276 works.🤖 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/utils/client/lib/SDKClient.ts` around lines 198 - 203, The current createNewDdpSdkStream registers a new sdk.client.onCollection listener for each subscription which causes O(N²) work; fix it by memoizing a single onCollection listener per streamName (use a Map<string, () => void> e.g., streamListeners) so the first createNewDdpSdkStream call for a given streamName registers sdk.client.onCollection(`stream-${streamName}`, ...) and stores its off/unsubscribe function, and subsequent calls reuse that listener instead of adding more; inside that single listener remove the per-listener filtering and simply forward events via streamProxy.emit(`stream-${streamName}/${data.fields?.eventName}`, data.fields?.args) so the proxy handles keyed routing, and ensure you call the stored off function when the last subscription for that streamName is removed.
359-367: 💤 Low value
publisherrors are silently swallowed on the SDK path.
void getDdpSdk().client.callAsync(...)discards both the resolution and rejection. If the server returns an error for astream-${name}write (auth, validation, rate-limit), the caller has no way to react and there's no telemetry — it just silently drops on the floor. The legacyMeteor.call(...)without a callback has the same ergonomics, so this matches existing behavior, but on the SDK path it's worth at least logging:🔧 Proposed fix
const publish = sdkTransportEnabled ? (name: string, args: unknown[]) => { - void getDdpSdk().client.callAsync(`stream-${name}`, ...args); + void getDdpSdk() + .client.callAsync(`stream-${name}`, ...args) + .catch((err) => console.warn('[sdk.publish] failed', name, err)); }🤖 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/utils/client/lib/SDKClient.ts` around lines 359 - 367, The SDK publish branch is dropping promise rejections from getDdpSdk().client.callAsync; update the sdkTransportEnabled branch in the publish definition so callAsync is not called with a leading void but instead attach a .catch handler that logs the error (including the stream name and relevant args/context) so failures aren’t silently swallowed; if there’s a project logging utility available in this file prefer that (e.g., processLogger or similar), otherwise use console.error, and keep the Meteor.call path unchanged.
🤖 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/app/utils/client/lib/SDKClient.ts`:
- Around line 178-204: The current flow calls
ensureConnectedAndAuthenticated().catch(() => undefined).then(...) which can
hang if ensureConnectedAndAuthenticated() never settles; update the logic in the
subscribe block to race ensureConnectedAndAuthenticated() with a timeout and
handle rejection so ready() always resolves or rejects: wrap the call to
ensureConnectedAndAuthenticated() (used before calling getDdpSdk(), creating
subscription and setting up offCollection) in a Promise.race with a short
timeout, and on timeout or rejection emit ee.emit('ready', [err]) and
ee.emit('error', err) (and return early so no subscription is created);
alternatively, if getDdpSdk().connection.status === 'connected' &&
getDdpSdk().account.uid is true, bypass the wait and subscribe immediately —
adjust the code around ensureConnectedAndAuthenticated(), getDdpSdk(),
subscription, and the ready() handlers accordingly.
In `@apps/meteor/client/lib/cachedStores/CachedStore.ts`:
- Around line 53-56: Delete the multi-line implementation comment in
CachedStore.ts (the block explaining the bump from 18 → 19 and the EJSON/JSON
rationale) so the file contains no inline implementation commentary; instead,
move that explanation into the PR description or the changeset/changelog for
this change (reference: the CachedStore.ts comment block near the version bump
and any nearby comment tokens mentioning "18 → 19" and "EJSON/JSON" so you
remove the correct text).
In `@apps/meteor/client/lib/loggedIn.ts`:
- Around line 74-81: Both Accounts.onLogin(handler) and
subscribeToLogin(handler) can register the same handler when sdkTransportEnabled
is true, causing onLoggedIn to fire twice; change the subscription logic so only
one source is registered: create accountsSubscription =
Accounts.onLogin(handler) only when sdkTransportEnabled is false, and otherwise
use stopUserIdSubscription = subscribeToLogin(handler) (i.e., make the
Accounts.onLogin registration conditional on !sdkTransportEnabled). Keep the
isLoggedIn() immediate check and ensure variable names accountsSubscription and
stopUserIdSubscription remain unchanged.
In `@apps/meteor/client/lib/sdk/ddpSdk.ts`:
- Around line 73-124: The race occurs because inflightLogin is checked before
the 500ms busy-wait, allowing two callers to both pass the check and each start
loginWithToken; modify ensureConnectedAndAuthenticated to re-check inflightLogin
immediately after the busy-wait (and return/await it if set) before assigning a
new inflightLogin, or alternatively assign inflightLogin to a promise
placeholder before the busy-wait so only the first caller creates the actual
login promise; reference ensureConnectedAndAuthenticated, inflightLogin,
sdk.account.loginWithToken, waitForConnected and startConnect when applying the
fix.
- Around line 40-48: waitForConnected currently only listens for the 'connected'
event and can hang if the DDP handshake fails; update waitForConnected (function
waitForConnected) to also check sdk.connection.status for 'failed' up-front and
add a 'connection' event listener that rejects the Promise when status ===
'failed' (or any terminal failure), ensuring you clean up both listeners (the
'connected' and 'connection' handlers) when resolving or rejecting so no
handlers leak.
In `@apps/meteor/client/lib/sdk/meteorBackedSdk.ts`:
- Around line 190-200: The getter user in meteorBackedSdk currently casts
Meteor.user() (which returns {_id, ...}) to {id, username?, token?,
tokenExpires?}, causing account.user?.id to be undefined; fix by mapping the
shape explicitly in the user getter: read Meteor.user() into a local u and
return an object that sets id: u._id (and copies username/token/tokenExpires if
present), or alternatively update the advertised type to include _id and adjust
callers; locate the getter named "user" and change the return to construct the
correct id field instead of an unsafe cast.
In `@apps/meteor/client/lib/sdk/sdkTransportEnabled.ts`:
- Around line 9-11: Update the JSDoc in sdkTransportEnabled.ts to reference the
actual admin setting name used by the server: replace the incorrect
`SDK_DDP_Transport_Enabled` with `Use_RC_SDK` (the setting watched in the server
index). Ensure the doc line describing the `<meta
name="rc-sdk-transport-enabled" ...>` injection mentions `Use_RC_SDK` so future
maintainers can locate the correct admin setting.
In `@apps/meteor/client/meteor/overrides/stubMeteorStream.ts`:
- Around line 38-44: The status() getter currently calls the dependency's
changed() which incorrectly invalidates dependents on every read; change it to
call the dependency's depend() (e.g., this.statusListeners?.depend()) so reads
register a dependency, and leave changed() only in statusChanged() to notify
updates; apply the same swap for the other occurrences mentioned (around the
blocks that reference statusListeners.changed()/depend() at the other
locations).
- Around line 29-47: The stub type MeteorIDDPStream and its implementation are
missing the internal boolean flag _connected, causing Meteor's runtime checks
(e.g., MethodInvoker) to treat the stream as disconnected; add a _connected:
boolean property to the MeteorIDDPStream type and ensure the stub instance sets
and updates _connected = true on successful connect/reconnect and _connected =
false on disconnect/_lostConnection so runtime checks see the correct connection
state (update functions/methods send, reconnect, disconnect, _lostConnection to
toggle _connected accordingly).
In `@apps/meteor/client/meteor/overrides/subscribeViaSDK.ts`:
- Around line 17-21: The subscription failure handling in subscribeViaSDK (and
its SubscribeCallbacks type) currently only invokes onError(err), which breaks
Meteor's expected onStop(err) contract; update the failure path so it invokes
both callbacks: call onStop(err) first (to preserve Meteor semantics) and then
onError(err) if provided, ensuring you reference the SubscribeCallbacks.onStop
and SubscribeCallbacks.onError handlers used in subscribeViaSDK to locate and
modify the error branch.
In `@apps/meteor/client/providers/ServerProvider.tsx`:
- Around line 117-132: sdkStatusToMeteor currently maps only sdkStatus and
ignores Meteor's own status, which violates the "worst-case of both transports"
intent; update sdkStatusToMeteor to consider both sdkStatus and the Meteor
`status` (and still include `retryCount`/`retryTime`) by computing a severity
ranking for statuses from both sources and returning the more severe result
(e.g., map statuses to numeric severity, compare sdkStatus vs. meteor.status,
then return the status/connected value corresponding to the worse severity),
keeping the existing shape returned (status, connected, ...retry) and
referencing sdkStatusToMeteor, sdkStatus, and the meteor variable in the
implementation.
In `@apps/meteor/client/startup/startup.ts`:
- Around line 43-65: runUserDataSync can apply stale session data after
awaiting; after any await points (notably after
ensureConnectedAndAuthenticated() and synchronizeUserData(uid)) re-check that
userIdStore.getState() === uid (and that user is still present) and return early
if it changed; only then call sdk.call('userSetUtcOffset', ...) and
emitStatusChange(user.status) to avoid leaking updates into a different or
logged-out session.
In `@apps/meteor/client/views/root/hooks/loggedIn/useForceLogout.ts`:
- Around line 20-25: The early return in useForceLogout (the branch checking
sdkTransportEnabled) skips local credential cleanup and leaves
Meteor.loginToken/userId set; remove the early return and ensure the legacy
branch still performs the same local cleanup steps (clear
Meteor._localStorage/Meteor.loginToken and Meteor.userId, and call the same
local logout/cleanup functions used in the SDK path) when handling the
force_logout path in useForceLogout so credentials are cleared immediately even
if sdkTransportEnabled is false.
In `@ee/apps/ddp-streamer/src/DDPStreamer.ts`:
- Around line 63-72: The large inline rationale in DDPStreamer (around the
socket shutdown logic that uses ws.close() with a fallback to ws.terminate())
should be removed from implementation code; replace the block comment with a
concise one-line note like "Graceful close with terminate fallback" or delete
the comment entirely, and move the detailed explanation into the PR/changelog.
Locate the socket shutdown code in the DDPStreamer class (the section
referencing ws.close() and ws.terminate()) and update the comment accordingly
while leaving the existing behavior (close then terminate fallback) unchanged.
---
Nitpick comments:
In `@apps/meteor/app/utils/client/lib/SDKClient.ts`:
- Around line 149-250: The two stream creators duplicate the subscriber surface
(onChange, ready, onError, onStop, isReady, unsubList) between
createNewDdpSdkStream and createNewMeteorStream; extract that common behavior
into a small factory (e.g., makeStreamMapValue(meta, ee, handlers)) that returns
the StreamMapValue shape and accepts a transport-specific stop callback, then
have createNewDdpSdkStream and createNewMeteorStream only perform their
transport-specific subscribe/onCollection wiring and call
ee.emit('ready'|'error'|'stop') as needed; update both functions to use
makeStreamMapValue so the shared logic lives in one place and remove the
duplicated blocks.
- Around line 198-203: The current createNewDdpSdkStream registers a new
sdk.client.onCollection listener for each subscription which causes O(N²) work;
fix it by memoizing a single onCollection listener per streamName (use a
Map<string, () => void> e.g., streamListeners) so the first
createNewDdpSdkStream call for a given streamName registers
sdk.client.onCollection(`stream-${streamName}`, ...) and stores its
off/unsubscribe function, and subsequent calls reuse that listener instead of
adding more; inside that single listener remove the per-listener filtering and
simply forward events via
streamProxy.emit(`stream-${streamName}/${data.fields?.eventName}`,
data.fields?.args) so the proxy handles keyed routing, and ensure you call the
stored off function when the last subscription for that streamName is removed.
- Around line 359-367: The SDK publish branch is dropping promise rejections
from getDdpSdk().client.callAsync; update the sdkTransportEnabled branch in the
publish definition so callAsync is not called with a leading void but instead
attach a .catch handler that logs the error (including the stream name and
relevant args/context) so failures aren’t silently swallowed; if there’s a
project logging utility available in this file prefer that (e.g., processLogger
or similar), otherwise use console.error, and keep the Meteor.call path
unchanged.
In `@apps/meteor/client/lib/sdk/ddpSdk.ts`:
- Around line 152-162: Move the isAuthError declaration so it appears before its
first use in ensureConnectedAndAuthenticated (or convert it to a hoisted
function declaration) to avoid a temporal-dead-zone risk; locate the const
isAuthError = (...) arrow function and place it above the
ensureConnectedAndAuthenticated function (or replace it with function
isAuthError(...) { ... }) so the identifier is available at module-evaluation
time.
- Line 217: The condition re-calls isSdkTransportEnabled() instead of using the
previously cached constant sdkTransportEnabled; change the if-check to use
sdkTransportEnabled so the module-level flag is used consistently (e.g., replace
isSdkTransportEnabled() with sdkTransportEnabled in the window-check branch
around the code using window), ensuring all references in this module match the
module-cached value and avoid re-evaluation.
In `@apps/meteor/client/lib/sdk/meteorBackedSdk.ts`:
- Around line 60-70: In the subscribe function, avoid mutating
Meteor.connection.subscribe's SubscriptionHandle (do not use Object.assign(sub,
...)); instead return a new wrapper object that holds the original sub and
delegates stop, ready (returning sub.ready()), isReady (reflecting sub.ready or
sub.isReady), and id to the underlying handle while adding the onChange method
and matching the DDPSDK['client']['subscribe'] return shape; update the wrapper
to call sub.stop(), forward sub.ready()/sub.isReady access, and expose id from
sub without overwriting the original SubscriptionHandle.
- Around line 81-98: The onCollection function currently attaches a permanent
handler to Meteor.connection._stream via stream.on('message', handler) causing
listener accumulation; fix it by making each handler removable: when calling
stream.on in onCollection, capture and return the unsubscribe function if
stream.on provides one (use that to remove the handler), and if stream.on does
not return an unsubscribe, wrap handler with an active flag registered in a
local cleanup registry (or closure) so the returned function flips the flag to
ignore future invocations; update references to onCollection, handler, stream.on
and noopUnsubscribe so callers get a real unsubscribe that prevents the leak.
In `@apps/meteor/client/meteor/overrides/ddpOverREST.ts`:
- Around line 9-37: The inline resume-login check in shouldBypass duplicates the
helper isResumeLogin; replace the condition if (method === 'login' &&
params?.[0]?.resume) with a call to isResumeLogin({ method, params }) so
shouldBypass reuses the centralized logic (remove the duplicated expression and
use the isResumeLogin helper instead).
- Around line 45-57: The processResult function should guard against unhandled
promise rejections from this._streamHandlers.onMessage: instead of calling it
directly, invoke it through the same async-wrap pattern used by the bridge
(i.e., wrap the returned Promise and attach a .catch handler so rejections are
handled and routed to the stream handler's error/cleanup path), keeping the
early-return check on this._methodInvokers[message.id] and otherwise forwarding
the message with this._livedata_data and then calling
this._streamHandlers.onMessage via the wrapped/catch-handled invocation to
prevent unhandled rejections.
🪄 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: b877ad5c-ecad-492a-a3fc-5e1c32c79d0f
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (26)
.changeset/use-rc-sdk-transport-setting.mdapps/meteor/app/notifications/client/lib/Presence.tsapps/meteor/app/ui-master/server/index.tsapps/meteor/app/utils/client/lib/SDKClient.tsapps/meteor/client/lib/cachedStores/CachedStore.tsapps/meteor/client/lib/loggedIn.tsapps/meteor/client/lib/presence.tsapps/meteor/client/lib/sdk/ddpSdk.tsapps/meteor/client/lib/sdk/meteorBackedSdk.tsapps/meteor/client/lib/sdk/sdkTransportEnabled.tsapps/meteor/client/lib/sdk/streamerAdapter.tsapps/meteor/client/main.tsapps/meteor/client/meteor/overrides/ddpOverREST.tsapps/meteor/client/meteor/overrides/ddpSdkCollectionBridge.tsapps/meteor/client/meteor/overrides/index.tsapps/meteor/client/meteor/overrides/killMeteorStream.tsapps/meteor/client/meteor/overrides/stubMeteorStream.tsapps/meteor/client/meteor/overrides/subscribeViaSDK.tsapps/meteor/client/providers/ServerProvider.tsxapps/meteor/client/startup/startup.tsapps/meteor/client/views/root/hooks/loggedIn/useForceLogout.tsapps/meteor/package.jsonapps/meteor/server/settings/general.tsapps/meteor/tests/e2e/page-objects/fragments/home-content.tsee/apps/ddp-streamer/src/DDPStreamer.tspackages/i18n/src/locales/en.i18n.json
📜 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 (3)
**/*.{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/main.tsapps/meteor/client/lib/sdk/sdkTransportEnabled.tsapps/meteor/client/lib/cachedStores/CachedStore.tsapps/meteor/server/settings/general.tsapps/meteor/client/meteor/overrides/index.tsapps/meteor/app/ui-master/server/index.tsapps/meteor/client/meteor/overrides/ddpSdkCollectionBridge.tsapps/meteor/client/lib/sdk/ddpSdk.tsee/apps/ddp-streamer/src/DDPStreamer.tsapps/meteor/client/lib/sdk/streamerAdapter.tsapps/meteor/client/meteor/overrides/subscribeViaSDK.tsapps/meteor/app/utils/client/lib/SDKClient.tsapps/meteor/client/meteor/overrides/killMeteorStream.tsapps/meteor/tests/e2e/page-objects/fragments/home-content.tsapps/meteor/client/lib/presence.tsapps/meteor/client/lib/sdk/meteorBackedSdk.tsapps/meteor/client/meteor/overrides/ddpOverREST.tsapps/meteor/app/notifications/client/lib/Presence.tsapps/meteor/client/providers/ServerProvider.tsxapps/meteor/client/startup/startup.tsapps/meteor/client/lib/loggedIn.tsapps/meteor/client/meteor/overrides/stubMeteorStream.tsapps/meteor/client/views/root/hooks/loggedIn/useForceLogout.ts
apps/meteor/tests/e2e/page-objects/**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
Utilize existing page objects pattern from
apps/meteor/tests/e2e/page-objects/
Files:
apps/meteor/tests/e2e/page-objects/fragments/home-content.ts
apps/meteor/tests/e2e/**/*.{ts,spec.ts}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
apps/meteor/tests/e2e/**/*.{ts,spec.ts}: Store commonly used locators in variables/constants for reuse
Follow Page Object Model pattern consistently in Playwright tests
Files:
apps/meteor/tests/e2e/page-objects/fragments/home-content.ts
🧠 Learnings (8)
📚 Learning: 2026-02-10T16:32:42.586Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 38528
File: apps/meteor/client/startup/roles.ts:14-14
Timestamp: 2026-02-10T16:32:42.586Z
Learning: In Rocket.Chat's Meteor client code, DDP streams use EJSON and Date fields arrive as Date objects; do not manually construct new Date() in stream handlers (for example, in sdk.stream()). Only REST API responses return plain JSON where dates are strings, so implement explicit conversion there if needed. Apply this guidance to all TypeScript files under apps/meteor/client to ensure consistent date handling in DDP streams and REST responses.
Applied to files:
apps/meteor/client/main.tsapps/meteor/client/lib/sdk/sdkTransportEnabled.tsapps/meteor/client/lib/cachedStores/CachedStore.tsapps/meteor/client/meteor/overrides/index.tsapps/meteor/client/meteor/overrides/ddpSdkCollectionBridge.tsapps/meteor/client/lib/sdk/ddpSdk.tsapps/meteor/client/lib/sdk/streamerAdapter.tsapps/meteor/client/meteor/overrides/subscribeViaSDK.tsapps/meteor/client/meteor/overrides/killMeteorStream.tsapps/meteor/client/lib/presence.tsapps/meteor/client/lib/sdk/meteorBackedSdk.tsapps/meteor/client/meteor/overrides/ddpOverREST.tsapps/meteor/client/startup/startup.tsapps/meteor/client/lib/loggedIn.tsapps/meteor/client/meteor/overrides/stubMeteorStream.tsapps/meteor/client/views/root/hooks/loggedIn/useForceLogout.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 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/client/main.tsapps/meteor/client/lib/sdk/sdkTransportEnabled.tsapps/meteor/client/lib/cachedStores/CachedStore.tsapps/meteor/server/settings/general.tsapps/meteor/client/meteor/overrides/index.tsapps/meteor/app/ui-master/server/index.tsapps/meteor/client/meteor/overrides/ddpSdkCollectionBridge.tsapps/meteor/client/lib/sdk/ddpSdk.tsee/apps/ddp-streamer/src/DDPStreamer.tsapps/meteor/client/lib/sdk/streamerAdapter.tsapps/meteor/client/meteor/overrides/subscribeViaSDK.tsapps/meteor/app/utils/client/lib/SDKClient.tsapps/meteor/client/meteor/overrides/killMeteorStream.tsapps/meteor/tests/e2e/page-objects/fragments/home-content.tsapps/meteor/client/lib/presence.tsapps/meteor/client/lib/sdk/meteorBackedSdk.tsapps/meteor/client/meteor/overrides/ddpOverREST.tsapps/meteor/app/notifications/client/lib/Presence.tsapps/meteor/client/startup/startup.tsapps/meteor/client/lib/loggedIn.tsapps/meteor/client/meteor/overrides/stubMeteorStream.tsapps/meteor/client/views/root/hooks/loggedIn/useForceLogout.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/client/main.tsapps/meteor/client/lib/sdk/sdkTransportEnabled.tsapps/meteor/client/lib/cachedStores/CachedStore.tsapps/meteor/server/settings/general.tsapps/meteor/client/meteor/overrides/index.tsapps/meteor/app/ui-master/server/index.tsapps/meteor/client/meteor/overrides/ddpSdkCollectionBridge.tsapps/meteor/client/lib/sdk/ddpSdk.tsee/apps/ddp-streamer/src/DDPStreamer.tsapps/meteor/client/lib/sdk/streamerAdapter.tsapps/meteor/client/meteor/overrides/subscribeViaSDK.tsapps/meteor/app/utils/client/lib/SDKClient.tsapps/meteor/client/meteor/overrides/killMeteorStream.tsapps/meteor/tests/e2e/page-objects/fragments/home-content.tsapps/meteor/client/lib/presence.tsapps/meteor/client/lib/sdk/meteorBackedSdk.tsapps/meteor/client/meteor/overrides/ddpOverREST.tsapps/meteor/app/notifications/client/lib/Presence.tsapps/meteor/client/startup/startup.tsapps/meteor/client/lib/loggedIn.tsapps/meteor/client/meteor/overrides/stubMeteorStream.tsapps/meteor/client/views/root/hooks/loggedIn/useForceLogout.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/client/main.tsapps/meteor/client/lib/sdk/sdkTransportEnabled.tsapps/meteor/client/lib/cachedStores/CachedStore.tsapps/meteor/server/settings/general.tsapps/meteor/client/meteor/overrides/index.tsapps/meteor/app/ui-master/server/index.tsapps/meteor/client/meteor/overrides/ddpSdkCollectionBridge.tsapps/meteor/client/lib/sdk/ddpSdk.tsee/apps/ddp-streamer/src/DDPStreamer.tsapps/meteor/client/lib/sdk/streamerAdapter.tsapps/meteor/client/meteor/overrides/subscribeViaSDK.tsapps/meteor/app/utils/client/lib/SDKClient.tsapps/meteor/client/meteor/overrides/killMeteorStream.tsapps/meteor/tests/e2e/page-objects/fragments/home-content.tsapps/meteor/client/lib/presence.tsapps/meteor/client/lib/sdk/meteorBackedSdk.tsapps/meteor/client/meteor/overrides/ddpOverREST.tsapps/meteor/app/notifications/client/lib/Presence.tsapps/meteor/client/providers/ServerProvider.tsxapps/meteor/client/startup/startup.tsapps/meteor/client/lib/loggedIn.tsapps/meteor/client/meteor/overrides/stubMeteorStream.tsapps/meteor/client/views/root/hooks/loggedIn/useForceLogout.ts
📚 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/use-rc-sdk-transport-setting.md
📚 Learning: 2025-12-16T17:29:40.430Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 37834
File: apps/meteor/tests/e2e/page-objects/fragments/admin-flextab-emoji.ts:12-22
Timestamp: 2025-12-16T17:29:40.430Z
Learning: In all page-object files under apps/meteor/tests/e2e/page-objects/, import expect from ../../utils/test (Playwright's async expect) instead of from Jest. Jest's expect is synchronous and incompatible with web-first assertions like toBeVisible, which can cause TypeScript errors.
Applied to files:
apps/meteor/tests/e2e/page-objects/fragments/home-content.ts
📚 Learning: 2026-02-24T19:39:42.247Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 38493
File: apps/meteor/tests/e2e/page-objects/fragments/message.ts:7-7
Timestamp: 2026-02-24T19:39:42.247Z
Learning: In RocketChat e2e tests, avoid using data-qa attributes to locate elements. Prefer semantic locators such as getByRole, getByLabel, getByText, getByTitle and ARIA-based selectors. Apply this rule to all TypeScript files under apps/meteor/tests/e2e to improve test reliability, accessibility, and maintainability.
Applied to files:
apps/meteor/tests/e2e/page-objects/fragments/home-content.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/ServerProvider.tsx
🔇 Additional comments (18)
apps/meteor/package.json (1)
105-105: Dependency addition is correctly scoped.Adding
@rocket.chat/ddp-clientas a runtime workspace dependency here matches the new transport path usage.apps/meteor/server/settings/general.ts (1)
99-104:Use_RC_SDKsetting registration looks good.Defaulting to
falsewith explicit i18n description/alert keeps the feature safely gated while exposing clear admin controls.apps/meteor/app/ui-master/server/index.ts (1)
31-33: Meta-tag toggle wiring is correct.This watcher cleanly maps
Use_RC_SDKtorc-sdk-transport-enabledwith deterministicon/offvalues for client detection.packages/i18n/src/locales/en.i18n.json (1)
5622-5624: Clear and actionable experimental feature copy.The new label, description, and alert text are consistent and set correct expectations for admins enabling the SDK transport path.
.changeset/use-rc-sdk-transport-setting.md (1)
1-5: LGTM!Changeset accurately enumerates the new admin setting, meta-tag, URL parameter, and localStorage precedence and matches the bump scope.
apps/meteor/tests/e2e/page-objects/fragments/home-content.ts (1)
123-149: LGTM!Transport-agnostic flow looks correct: snapshotting
before, polling forcount >= before + 1, and assertinglastUserMessageno longer hasrcx-message--pendingis a reasonable DOM-only completion signal that doesn't depend on REST vs DDP transport. The>=(instead of==) is well-justified for encrypted-channel decryption-status items.apps/meteor/client/lib/sdk/sdkTransportEnabled.ts (1)
16-28: LGTM!SSR guard, defensive try/catch, and the URL > localStorage > meta resolution order match the documented contract. Defaulting to
falsekeeps the migration dormant.apps/meteor/client/meteor/overrides/ddpSdkCollectionBridge.ts (1)
65-99: LGTM!Bridge surface and frame filtering look correct: SDK-internal id detection prevents
No callback invoker for method ...throws on Meteor's_process_updated/_process_result, and the dual sync/async error containment around_streamHandlers.onMessagekeeps the queue draining when an individual frame hits a dead invoker. Module-load-only installation matches the rest of the boot pipeline.apps/meteor/client/meteor/overrides/ddpOverREST.ts (1)
102-142: LGTM — preserves Meteor error codes forwithAsyncTOTP.Forwarding the original DDP-encoded
resultframe frombody.message(whenmsg === 'result'andidmatches) is the correct fix for the TOTP regression — codes liketotp-requiredsurvive untouched. The id match prevents accidental cross-talk, and the synthetic-frame fallback still handles auth-middleware 401s. Good defensive sequencing.apps/meteor/client/main.ts (1)
1-4: LGTM!The side-effect import is placed after
./meteor/overrides(which installs_sendinterception, the SDK collection bridge, etc.) and./meteor/startup, which is the right ordering — the DDP SDK singleton is materialised once the override surface is wired.apps/meteor/client/meteor/overrides/index.ts (1)
2-5: LGTM!Side-effect import order is reasonable: REST routing wired first, then the SDK collection bridge, subscription routing, stream stub, and revival reset. Each new module self-gates on
isSdkTransportEnabled(), so the legacy path remains untouched.apps/meteor/app/utils/client/lib/SDKClient.ts (1)
264-277: LGTM — legacy bridge correctly gated.Skipping the
Meteor.connection._stream.on('message', ...)bridge when SDK transport is enabled is the right call: with the flag on, frames arrive on the DDPSDK socket viaonCollection(line 198) and bridging the Meteor stream too would double-firestreamProxy. The flag-off path preserves the legacy behavior exactly.apps/meteor/client/providers/ServerProvider.tsx (3)
140-144: LGTM — restoring the spread fixes theuseSyncExternalStoreidentity check.
Meteor.status()mutates and returns the same internal object on every call, so without the spreaduseReactiveValue'suseSyncExternalStoresnapshot identity never changes andConnectionStatusBarstops re-rendering on connect/drop — exactly as the inline comment describes. The fix matches the PR's stated regression.
81-100: 💤 Low value
reconnectdoesn't wait for Meteor's reconnect before re-authing the SDK — verified as safe.
Meteor.reconnect()is fire-and-forget; the call toensureConnectedAndAuthenticated()runs immediately after. This creates a race where the SDK's login can finish before Meteor's session re-establishes, breaking ordering guarantees if downstream code relies on Meteor state being ready first.Verification via codebase search confirms this is safe:
onLoggedInconsumers (incomingMessages.ts,roles.ts,startup.ts) use SDK methods (sdk.stream,sdk.rest.get) and SDK-sourced state (userIdStore) rather than reading Meteor state directly. The codebase already acknowledges the ordering risk in comments (e.g.,ddpSdk.tslines 267–273 explicitly explain why separateensureConnectedAndAuthenticatedcalls are avoided in certain paths to prevent duplicate Accounts.onLogin fires).
107-110:'connection'event covers every status transition.DDPSDK's
Connection.emitStatus()emits a'connection'event of typeConnectionStatusfor every state change ('idle' | 'connecting' | 'connected' | 'failed' | 'closed' | 'disconnected' | 'reconnecting'), so a single listener here is enough to invalidateddpSdkStatusDepfor all transitions handled bysdkStatusToMeteor.apps/meteor/client/lib/sdk/ddpSdk.ts (3)
252-264: LGTM — auth-error swallowing prevents unhandled-rejection storms on auto-relogin.DDPSDK auto-fires
loginWithTokenon everyconnectedevent withvoid, so without this wrap, an auth-shaped rejection would bubble towindow.onunhandledrejection. Returningundefinedonly onisAuthErrorkeeps non-auth failures (e.g., network) flowing to the caller. The recovery path through Meteor'sDDP.onReconnect→stubMeteorStreamis the right place for actual recovery, as the comment explains.
198-208: 💤 Low value
teardownAuthenticatedConnectioncloses the connection without resettinginstance, andinflightLoginis not cleared during teardown.After logout, the DDP socket is closed but
instancestill points to the same SDK object. The next call togetDdpSdk()returns that closed instance (L51 short-circuits oninstanceset), andensureConnectedAndAuthenticatedrelies on the status check at L82 callingstartConnectto revive it. However:
sdk.connection.connect()is called on a previously-close()d instance without creating a new SDK instance. The public@rocket.chat/ddp-clientdocumentation recommends creating a new SDK instance viaDDPSDK.create()orDDPSDK.createAndConnect()after closing, not reconnecting the same instance — whether the connection is idempotent after close is an undocumented implementation detail.inflightLoginis not cleared during teardown (only in the finally block of L118–124). If a login request is in flight when logout occurs, it continues executing and can re-populateaccount.uidon the post-teardown instance even thoughaccount.userwas cleared.
14-18: ⚡ Quick winThe URL transformation in
computeDdpUrlcorrectly handles relative ROOT_URLs with subpaths.
DDPSDK.create()expectsws://orwss://URLs with host and optional path (as shown in DDPSDK tests callingDDPSDK.create('ws://localhost:1234')and the documented example'wss://open.rocket.chat/websocket'). ConnectionImpl strips the protocol and appends/websocketinternally, so passingwss://example.com/chatcorrectly resolves towss://example.com/chat/websocketfor subpath-mounted installs. The current regex transformation is appropriate for the documented DDPSDK API.
| void ensureConnectedAndAuthenticated() | ||
| .catch(() => undefined) | ||
| .then(() => { | ||
| if (stopped) return; | ||
| const sdk = getDdpSdk(); | ||
| subscription = sdk.client.subscribe(`stream-${streamName}`, key, { useCollection: false, args }); | ||
|
|
||
| subscription | ||
| .ready() | ||
| .then(() => { | ||
| if (stopped) return; | ||
| meta.ready = true; | ||
| ee.emit('ready', [undefined, { msg: 'ready', subs: [subscription!.id] }]); | ||
| }) | ||
| .catch((err) => { | ||
| if (stopped) return; | ||
| ee.emit('ready', [err]); | ||
| ee.emit('error', err); | ||
| }); | ||
|
|
||
| offCollection = sdk.client.onCollection(`stream-${streamName}`, (data: any) => { | ||
| if (data?.msg !== 'changed') return; | ||
| if (data.collection !== `stream-${streamName}`) return; | ||
| if (data.fields?.eventName !== key) return; | ||
| streamProxy.emit(`stream-${streamName}/${key}` as keyof EventMap, data.fields.args); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
ready() can hang indefinitely if ensureConnectedAndAuthenticated() never settles.
The auth gate is wrapped in .catch(() => undefined).then(...), so a rejected auth still subscribes. But a pending auth (e.g., DDP socket never reaches connected because there's no timeout in waitForConnected) means the .then callback never runs, no subscription is created, and any caller awaiting stream.ready() waits forever — including useReactiveValue consumers that gate UI on subscription readiness.
Consider either:
- propagating a connect/auth timeout into
ee.emit('ready', [err])soready()rejects, or - not deferring the subscribe at all when
sdk.connection.status === 'connected' && sdk.account.uid(most post-login subscription calls), and only deferring on the explicitly-anonymous path.
🤖 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/utils/client/lib/SDKClient.ts` around lines 178 - 204, The
current flow calls ensureConnectedAndAuthenticated().catch(() =>
undefined).then(...) which can hang if ensureConnectedAndAuthenticated() never
settles; update the logic in the subscribe block to race
ensureConnectedAndAuthenticated() with a timeout and handle rejection so ready()
always resolves or rejects: wrap the call to ensureConnectedAndAuthenticated()
(used before calling getDdpSdk(), creating subscription and setting up
offCollection) in a Promise.race with a short timeout, and on timeout or
rejection emit ee.emit('ready', [err]) and ee.emit('error', err) (and return
early so no subscription is created); alternatively, if
getDdpSdk().connection.status === 'connected' && getDdpSdk().account.uid is
true, bypass the wait and subscribe immediately — adjust the code around
ensureConnectedAndAuthenticated(), getDdpSdk(), subscription, and the ready()
handlers accordingly.
| // Bumped from 18 → 19 to invalidate caches populated before the DDPSDK | ||
| // wire encoding was switched from JSON to EJSON. Entries written by the | ||
| // JSON window stored dates as ISO strings instead of Date instances, so | ||
| // fields like subscription.ls would fail `.getTime()` when read back. |
There was a problem hiding this comment.
Remove inline implementation commentary in this block.
The new multi-line rationale comment should be moved to PR/changeset docs, keeping implementation comment-free per repo rule.
Proposed change
- // Bumped from 18 → 19 to invalidate caches populated before the DDPSDK
- // wire encoding was switched from JSON to EJSON. Entries written by the
- // JSON window stored dates as ISO strings instead of Date instances, so
- // fields like subscription.ls would fail `.getTime()` when read back.
private readonly version = 19;As per coding guidelines, **/*.{ts,tsx,js}: Avoid code comments in implementation.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Bumped from 18 → 19 to invalidate caches populated before the DDPSDK | |
| // wire encoding was switched from JSON to EJSON. Entries written by the | |
| // JSON window stored dates as ISO strings instead of Date instances, so | |
| // fields like subscription.ls would fail `.getTime()` when read back. | |
| private readonly version = 19; |
🤖 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/lib/cachedStores/CachedStore.ts` around lines 53 - 56,
Delete the multi-line implementation comment in CachedStore.ts (the block
explaining the bump from 18 → 19 and the EJSON/JSON rationale) so the file
contains no inline implementation commentary; instead, move that explanation
into the PR description or the changeset/changelog for this change (reference:
the CachedStore.ts comment block near the version bump and any nearby comment
tokens mentioning "18 → 19" and "EJSON/JSON" so you remove the correct text).
| // With the SDK transport on, login can land via REST (ddpOverREST) without | ||
| // filling Meteor.users — Accounts.onLogin's autorun would never fire. | ||
| // Bridge off userIdStore as belt-and-braces. With the flag off, the legacy | ||
| // DDP path populates Meteor.users and Accounts.onLogin fires reliably; the | ||
| // extra userIdStore subscription would just double-fire callbacks. | ||
| const accountsSubscription = Accounts.onLogin(handler); | ||
| const stopUserIdSubscription = sdkTransportEnabled ? subscribeToLogin(handler) : undefined; | ||
| if (isLoggedIn()) handler(); |
There was a problem hiding this comment.
onLoggedIn now fires twice for one successful login.
With SDK transport enabled, Line 79 and Line 80 subscribe to two different sources for the same login transition. This PR already had to add syncOnce in apps/meteor/client/startup/startup.ts to mask that duplication there; other onLoggedIn callers will still double-run their side effects.
🤖 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/lib/loggedIn.ts` around lines 74 - 81, Both
Accounts.onLogin(handler) and subscribeToLogin(handler) can register the same
handler when sdkTransportEnabled is true, causing onLoggedIn to fire twice;
change the subscription logic so only one source is registered: create
accountsSubscription = Accounts.onLogin(handler) only when sdkTransportEnabled
is false, and otherwise use stopUserIdSubscription = subscribeToLogin(handler)
(i.e., make the Accounts.onLogin registration conditional on
!sdkTransportEnabled). Keep the isLoggedIn() immediate check and ensure variable
names accountsSubscription and stopUserIdSubscription remain unchanged.
| const waitForConnected = (sdk: DDPSDK): Promise<void> => { | ||
| if (sdk.connection.status === 'connected') return Promise.resolve(); | ||
| return new Promise<void>((resolve) => { | ||
| const stop = sdk.connection.on('connected', () => { | ||
| stop(); | ||
| resolve(); | ||
| }); | ||
| }); | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's read the specific file and lines mentioned
head -60 apps/meteor/client/lib/sdk/ddpSdk.ts | tail -25Repository: RocketChat/Rocket.Chat
Length of output: 852
🏁 Script executed:
# Let's see the full context around those lines
sed -n '30,60p' apps/meteor/client/lib/sdk/ddpSdk.tsRepository: RocketChat/Rocket.Chat
Length of output: 1129
🏁 Script executed:
# Search for the Connection class definition and its event API
rg "class Connection|\.on\(" apps/meteor/client/lib/sdk/ddpSdk.ts -A 3Repository: RocketChat/Rocket.Chat
Length of output: 151
🏁 Script executed:
# Check what status values are possible and if 'failed' is a real status
rg "failed|connected" apps/meteor/client/lib/sdk/ddpSdk.ts -B 2 -A 2Repository: RocketChat/Rocket.Chat
Length of output: 1795
🏁 Script executed:
# Look for all uses of waitForConnected to understand impact
rg "waitForConnected" apps/meteor/client/lib/sdk/ddpSdk.tsRepository: RocketChat/Rocket.Chat
Length of output: 155
🏁 Script executed:
# Search for Connection implementation or types in the codebase
fd -e ts -e tsx | xargs rg "connection.*status|emitStatus" | head -20Repository: RocketChat/Rocket.Chat
Length of output: 1894
🏁 Script executed:
# Read the Connection class to understand the event API
sed -n '1,100p' packages/ddp-client/src/Connection.ts | head -80Repository: RocketChat/Rocket.Chat
Length of output: 1543
🏁 Script executed:
# Find emitStatus and surrounding context to understand how events work
rg -B 5 -A 5 "private emitStatus" packages/ddp-client/src/Connection.tsRepository: RocketChat/Rocket.Chat
Length of output: 280
🏁 Script executed:
# Check the event listener signature - what does .on() return
rg -B 3 -A 3 "\.on\(" packages/ddp-client/src/Connection.ts | head -30Repository: RocketChat/Rocket.Chat
Length of output: 204
🏁 Script executed:
# Find all places waitForConnected is called
rg "waitForConnected" apps/meteor/client/lib/sdk/ddpSdk.ts -B 5 -A 10Repository: RocketChat/Rocket.Chat
Length of output: 730
🏁 Script executed:
# Check if there's a distinction between 'connected' and 'connection' events
rg "'connected'|'connection'" packages/ddp-client/src/Connection.ts | head -20Repository: RocketChat/Rocket.Chat
Length of output: 696
🏁 Script executed:
# Check the complete context where waitForConnected is called
rg -B 10 -A 15 "await waitForConnected" apps/meteor/client/lib/sdk/ddpSdk.tsRepository: RocketChat/Rocket.Chat
Length of output: 864
🏁 Script executed:
# Verify the Emitter types to understand event signatures
rg "extends Emitter" packages/ddp-client/src/Connection.ts -A 10Repository: RocketChat/Rocket.Chat
Length of output: 451
🏁 Script executed:
# Check if there's error handling around the waitForConnected call
rg "ensureConnectedAndAuthenticated|waitForConnected" apps/meteor/client/lib/sdk/ddpSdk.ts -B 2 -A 8Repository: RocketChat/Rocket.Chat
Length of output: 3149
🏁 Script executed:
# Look for any tests or documentation about Connection status transitions
rg "failed|'connection'" packages/ddp-client/src/Connection.ts -B 2 -A 2 | head -50Repository: RocketChat/Rocket.Chat
Length of output: 1250
🏁 Script executed:
# Check what happens after 'failed' status - is there auto-recovery
rg -A 20 "this.status = 'failed'" packages/ddp-client/src/Connection.tsRepository: RocketChat/Rocket.Chat
Length of output: 819
🏁 Script executed:
# Verify the flow after 'failed' and 'disconnected' events
rg "failed|disconnect" packages/ddp-client/src/Connection.ts | grep -E "(emit|status)" | head -30Repository: RocketChat/Rocket.Chat
Length of output: 257
🏁 Script executed:
# Check if startConnect is called in any retry or recovery logic
rg "startConnect" apps/meteor/client/lib/sdk/ddpSdk.ts -B 5 -A 10 | head -60Repository: RocketChat/Rocket.Chat
Length of output: 1522
waitForConnected hangs indefinitely when DDP connection fails — add rejection handler for terminal failure status.
The function only listens for the 'connected' event. If the WebSocket handshake fails (server sends 'failed' message or connection drops during initialization), the Connection emits a 'connection' event with status 'failed' and then 'disconnected', but waitForConnected never resolves or rejects. This leaves ensureConnectedAndAuthenticated() and all its callers (userIdStore.subscribe at L282, login flows) indefinitely suspended.
The suggested fix correctly listens to the 'connection' event and rejects when status becomes 'failed':
🔧 Suggested fix — handle terminal failure
const waitForConnected = (sdk: DDPSDK): Promise<void> => {
if (sdk.connection.status === 'connected') return Promise.resolve();
- return new Promise<void>((resolve) => {
- const stop = sdk.connection.on('connected', () => {
- stop();
- resolve();
- });
- });
+ return new Promise<void>((resolve, reject) => {
+ const offConnected = sdk.connection.on('connected', () => {
+ offConnected();
+ offFailed();
+ resolve();
+ });
+ const offFailed = sdk.connection.on('connection', (status) => {
+ if (status === 'failed') {
+ offConnected();
+ offFailed();
+ reject(new Error('DDPSDK connection failed'));
+ }
+ });
+ });
};🤖 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/lib/sdk/ddpSdk.ts` around lines 40 - 48, waitForConnected
currently only listens for the 'connected' event and can hang if the DDP
handshake fails; update waitForConnected (function waitForConnected) to also
check sdk.connection.status for 'failed' up-front and add a 'connection' event
listener that rejects the Promise when status === 'failed' (or any terminal
failure), ensuring you clean up both listeners (the 'connected' and 'connection'
handlers) when resolving or rejecting so no handlers leak.
| export const ensureConnectedAndAuthenticated = async (): Promise<void> => { | ||
| const sdk = getDdpSdk(); | ||
|
|
||
| // IMPORTANT: must wait for the DDP `connected` handshake before issuing | ||
| // any wait-method (login uses wait:true). DDPDispatcher serializes wait | ||
| // blocks at the queue head, so a login dispatched while connecting | ||
| // queues ahead of the connect frame ws.onopen later emits — the connect | ||
| // frame ends up wedged in a non-wait block behind the wait block and | ||
| // never flushes, leaving the socket open but DDP-unhandshaked. | ||
| if ( | ||
| sdk.connection.status === 'idle' || | ||
| sdk.connection.status === 'closed' || | ||
| sdk.connection.status === 'disconnected' || | ||
| sdk.connection.status === 'failed' | ||
| ) { | ||
| void startConnect(sdk); | ||
| } | ||
| await waitForConnected(sdk); | ||
|
|
||
| const token = readStoredLoginToken(); | ||
| if (!token || sdk.account.uid) { | ||
| return; | ||
| } | ||
|
|
||
| if (inflightLogin) { | ||
| await inflightLogin; | ||
| return; | ||
| } | ||
|
|
||
| // Give Meteor's own login flow (resume routed through stubMeteorStream | ||
| // + adoptAccountFromMeteorLoginResult) time to populate sdk.account | ||
| // before we issue our own loginWithToken. If adopt fires first, we can | ||
| // short-circuit and avoid sending a second login frame on the SDK | ||
| // socket — which would otherwise create a duplicate Presence | ||
| // connection (processConnectionStatus prefers ONLINE over AWAY in the | ||
| // aggregate, breaking the auto-away flow). 500ms covers a single | ||
| // server roundtrip in CI; if the stub-routed login hasn't completed by | ||
| // then, fall back to issuing our own loginWithToken below. | ||
| for (let i = 0; i < 20 && !sdk.account.uid; i++) { | ||
| await new Promise<void>((resolve) => setTimeout(resolve, 25)); | ||
| } | ||
| if (sdk.account.uid) { | ||
| return; | ||
| } | ||
|
|
||
| inflightLogin = (async () => { | ||
| try { | ||
| await sdk.account.loginWithToken(token); | ||
| } finally { | ||
| inflightLogin = undefined; | ||
| } | ||
| })(); |
There was a problem hiding this comment.
Race in inflightLogin gating — two concurrent callers can each fire a loginWithToken.
The intent (per the comments at L102–110) is that only one login goes out at a time, but the gate at L97 is before the 500 ms busy-wait at L111–113. Two concurrent callers interleave like this:
- Call A awaits
waitForConnected(L90) → yields. - Call B awaits
waitForConnected→ yields. - A resumes;
inflightLoginisundefined; falls through L97. - A enters the busy-wait loop and
awaitssetTimeout(L112) → yields. - B resumes;
inflightLoginstillundefined(A hasn't reached L118); falls through L97. - B also enters the busy-wait, eventually reaches L118 — both A and B set their own
inflightLoginand dispatch a login frame.
That's exactly the duplicate-Presence-session scenario the surrounding comments warn about (processConnectionStatus ends up with two entries, breaking auto-away). Re-check inflightLogin after the busy-wait, or move the inflightLogin = ... assignment up before the first await.
🔧 Suggested fix
for (let i = 0; i < 20 && !sdk.account.uid; i++) {
await new Promise<void>((resolve) => setTimeout(resolve, 25));
}
if (sdk.account.uid) {
return;
}
+ if (inflightLogin) {
+ await inflightLogin;
+ return;
+ }
inflightLogin = (async () => {🤖 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/lib/sdk/ddpSdk.ts` around lines 73 - 124, The race occurs
because inflightLogin is checked before the 500ms busy-wait, allowing two
callers to both pass the check and each start loginWithToken; modify
ensureConnectedAndAuthenticated to re-check inflightLogin immediately after the
busy-wait (and return/await it if set) before assigning a new inflightLogin, or
alternatively assign inflightLogin to a promise placeholder before the busy-wait
so only the first caller creates the actual login promise; reference
ensureConnectedAndAuthenticated, inflightLogin, sdk.account.loginWithToken,
waitForConnected and startConnect when applying the fix.
| type SubscribeCallbacks = { | ||
| onReady?: () => void; | ||
| onError?: (err: Error) => void; | ||
| onStop?: (err?: Error) => void; | ||
| }; |
There was a problem hiding this comment.
❓ Verification inconclusive
Meteor.subscribe callbacks onStop error nosub official docs
Meteor.subscribe callbacks: use onStop (not onStop “error” vs “nosub” separately). 1) What onStop does (officially) - Meteor.subscribe accepts an optional callbacks object that may include onStop and onReady.[1][2] - If the subscription fails or is terminated by the server, onStop is called with a Meteor.Error argument.[1][2] - If the subscription is stopped by calling stop on the subscription handle or inside the publication, onStop is called with no arguments.[1][2] 2) About “nosub” and the onStop error - The internal DDP “nosub” message corresponds to “subscription not found” / subscription termination conditions.[3] - In Meteor’s implementation and tests for subscribe errors, when a “nosub” comes back and the subscription is terminated, the client-side onStop receives the error (a Meteor.Error) as the optional first argument.[3] 3) Common “onStop error: nosub / callback error” gotchas - Don’t expect onStop to fire from an onError handler; onError was replaced by the more general onStop callback.[3] - If you stop a subscription in response to cleanup code (or if your publication calls this.stop / this.error), expect onStop to be called either with an error (server error/termination) or with no argument (client stop / in-publication stop).[1][2] If you paste the exact runtime error and the code around your Meteor.subscribe (callbacks object and where you call stop), I can pinpoint whether you’re hitting the expected “nosub -> onStop(error)” path versus a separate exception thrown inside your onStop callback.
Citations:
- 1: https://v2-docs.meteor.com/api/pubsub
- 2: https://docs.meteor.com/api/meteor.md
- 3: meteor/meteor@085e02d
Script executed:
fd -t f "subscribeViaSDK.ts" --exec wc -l {}Repository: RocketChat/Rocket.Chat
Repository: RocketChat/Rocket.Chat
Exit code: 0
stdout:
65 ./apps/meteor/client/meteor/overrides/subscribeViaSDK.ts
Script executed:
cat -n ./apps/meteor/client/meteor/overrides/subscribeViaSDK.tsRepository: RocketChat/Rocket.Chat
Repository: RocketChat/Rocket.Chat
Exit code: 0
stdout:
1 import { Meteor } from 'meteor/meteor';
2
3 import { getDdpSdk } from '../../lib/sdk/ddpSdk';
4 import { isSdkTransportEnabled } from '../../lib/sdk/sdkTransportEnabled';
5
6 /**
7 * Route Meteor.connection.subscribe through DDPSDK so the few direct
8 * publications that Accounts and Meteor core still open
9 * (loginServiceConfiguration, meteor_autoupdate_clientVersions, ...)
10 * ride our single WebSocket instead of Meteor's.
11 *
12 * The collection frames produced by those subscriptions are re-fed into
13 * Meteor.connection._streamHandlers by ddpSdkCollectionBridge, so
14 * Meteor's collection dispatch keeps working unchanged.
15 */
16
17 type SubscribeCallbacks = {
18 onReady?: () => void;
19 onError?: (err: Error) => void;
20 onStop?: (err?: Error) => void;
21 };
22
23 const extractCallbacks = (args: unknown[]): { params: unknown[]; callbacks: SubscribeCallbacks } => {
24 if (args.length === 0) return { params: [], callbacks: {} };
25
26 const last = args[args.length - 1];
27
28 if (typeof last === 'function') {
29 return { params: args.slice(0, -1), callbacks: { onReady: last as () => void } };
30 }
31
32 if (
33 last !== null &&
34 typeof last === 'object' &&
35 (typeof (last as any).onReady === 'function' ||
36 typeof (last as any).onError === 'function' ||
37 typeof (last as any).onStop === 'function')
38 ) {
39 return { params: args.slice(0, -1), callbacks: last as SubscribeCallbacks };
40 }
41
42 return { params: args, callbacks: {} };
43 };
44
45 type MeteorSubscriptionHandle = Meteor.SubscriptionHandle;
46
47 if (isSdkTransportEnabled()) {
48 (Meteor.connection as any).subscribe = ((name: string, ...rest: unknown[]): MeteorSubscriptionHandle => {
49 const { params, callbacks } = extractCallbacks(rest);
50 const subscription = getDdpSdk().client.subscribe(name, ...params);
51
52 subscription
53 .ready()
54 .then(() => callbacks.onReady?.())
55 .catch((err: Error) => callbacks.onError?.(err));
56
57 return {
58 stop: () => {
59 subscription.stop();
60 callbacks.onStop?.();
61 },
62 ready: () => subscription.isReady,
63 } as MeteorSubscriptionHandle;
64 }) as Meteor.IMeteorConnection['subscribe'];
65 }
Correct error handling to match Meteor's onStop(error) contract for failed subscriptions.
This override sends subscription failures only to onError (line 55), but Meteor callers typically observe subscription failures through onStop(error). Existing code implementing only onStop will silently miss errors when the SDK transport is enabled.
On subscription failure, call both onStop(err) and onError(err) to maintain backward compatibility with standard Meteor behavior while supporting SDK-aware callbacks.
Required fix
subscription
.ready()
.then(() => callbacks.onReady?.())
- .catch((err: Error) => callbacks.onError?.(err));
+ .catch((err: Error) => {
+ callbacks.onStop?.(err);
+ callbacks.onError?.(err);
+ });🤖 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/meteor/overrides/subscribeViaSDK.ts` around lines 17 - 21,
The subscription failure handling in subscribeViaSDK (and its SubscribeCallbacks
type) currently only invokes onError(err), which breaks Meteor's expected
onStop(err) contract; update the failure path so it invokes both callbacks: call
onStop(err) first (to preserve Meteor semantics) and then onError(err) if
provided, ensuring you reference the SubscribeCallbacks.onStop and
SubscribeCallbacks.onError handlers used in subscribeViaSDK to locate and modify
the error branch.
| switch (sdkStatus) { | ||
| case 'connected': | ||
| return { status: 'connected', connected: true, ...retry }; | ||
| case 'connecting': | ||
| return { status: 'connecting', connected: false, ...retry }; | ||
| case 'reconnecting': | ||
| return { status: 'connecting', connected: false, ...retry }; | ||
| case 'failed': | ||
| return { status: 'failed', connected: false, ...retry }; | ||
| case 'closed': | ||
| case 'disconnected': | ||
| return { status: 'waiting', connected: false, ...retry }; | ||
| case 'idle': | ||
| default: | ||
| return { status: 'offline', connected: false, ...retry }; | ||
| } |
There was a problem hiding this comment.
sdkStatusToMeteor discards Meteor's own status when it's worse than the SDK's.
The PR comment at lines 102–104 says the goal is "the worst-case of both transports", but the implementation only ever maps sdkStatus and uses meteor solely for retryCount/retryTime. If the SDK is 'connected' but Meteor is 'waiting'/'failed' (e.g., the SDK socket recovered first), the bar will show 'connected' and hide the underlying Meteor outage — the opposite of worst-case.
If "worst-case" is the intent, fold Meteor's status in:
🔧 Suggested combine
const getStatus = sdkTransportEnabled
? () => {
ddpSdkStatusDep!.depend();
- return sdkStatusToMeteor(getDdpSdk().connection.status, Meteor.status());
+ const meteor = Meteor.status();
+ const sdk = sdkStatusToMeteor(getDdpSdk().connection.status, meteor);
+ return sdk.connected && meteor.connected ? sdk : !sdk.connected ? sdk : meteor;
}If the intent is actually "SDK-first, Meteor only for retry metadata", clarify the comment.
🤖 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 117 - 132,
sdkStatusToMeteor currently maps only sdkStatus and ignores Meteor's own status,
which violates the "worst-case of both transports" intent; update
sdkStatusToMeteor to consider both sdkStatus and the Meteor `status` (and still
include `retryCount`/`retryTime`) by computing a severity ranking for statuses
from both sources and returning the more severe result (e.g., map statuses to
numeric severity, compare sdkStatus vs. meteor.status, then return the
status/connected value corresponding to the worse severity), keeping the
existing shape returned (status, connected, ...retry) and referencing
sdkStatusToMeteor, sdkStatus, and the meteor variable in the implementation.
| const runUserDataSync = async (uid: string) => { | ||
| // synchronizeUserData opens a `stream-notify-user/${uid}/userData` sub | ||
| // over DDPSDK. The server rejects that sub with "not-allowed" until | ||
| // DDPSDK has completed loginWithToken on its own socket. Both | ||
| // runUserDataSync and ensureConnectedAndAuthenticated are subscribers | ||
| // of userIdStore, so without sequencing the sub races the auth and | ||
| // hits the rejection on every re-login. Await the SDK auth here so | ||
| // the sub fires authenticated. | ||
| try { | ||
| await ensureConnectedAndAuthenticated(); | ||
| } catch { | ||
| // non-fatal: sdk.stream queues until DDPSDK eventually auths | ||
| } | ||
| const user = await synchronizeUserData(uid); | ||
| if (!user) return; | ||
|
|
||
| const utcOffset = -new Date().getTimezoneOffset() / 60; | ||
| if (user.utcOffset !== utcOffset) { | ||
| sdk.call('userSetUtcOffset', utcOffset); | ||
| } | ||
|
|
||
| emitStatusChange(user.status); | ||
| }; |
There was a problem hiding this comment.
Guard runUserDataSync against logout and user switches.
runUserDataSync(uid) awaits twice and then still calls userSetUtcOffset / emitStatusChange without re-checking that userIdStore.getState() still matches uid. A logout or fast re-login in the middle of those awaits can apply stale session data to the next session.
Suggested stale-request guard
const runUserDataSync = async (uid: string) => {
@@
try {
await ensureConnectedAndAuthenticated();
} catch {
// non-fatal: sdk.stream queues until DDPSDK eventually auths
}
+ if (userIdStore.getState() !== uid) {
+ return;
+ }
const user = await synchronizeUserData(uid);
- if (!user) return;
+ if (!user || userIdStore.getState() !== uid) return;
const utcOffset = -new Date().getTimezoneOffset() / 60;
if (user.utcOffset !== utcOffset) {
sdk.call('userSetUtcOffset', utcOffset);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const runUserDataSync = async (uid: string) => { | |
| // synchronizeUserData opens a `stream-notify-user/${uid}/userData` sub | |
| // over DDPSDK. The server rejects that sub with "not-allowed" until | |
| // DDPSDK has completed loginWithToken on its own socket. Both | |
| // runUserDataSync and ensureConnectedAndAuthenticated are subscribers | |
| // of userIdStore, so without sequencing the sub races the auth and | |
| // hits the rejection on every re-login. Await the SDK auth here so | |
| // the sub fires authenticated. | |
| try { | |
| await ensureConnectedAndAuthenticated(); | |
| } catch { | |
| // non-fatal: sdk.stream queues until DDPSDK eventually auths | |
| } | |
| const user = await synchronizeUserData(uid); | |
| if (!user) return; | |
| const utcOffset = -new Date().getTimezoneOffset() / 60; | |
| if (user.utcOffset !== utcOffset) { | |
| sdk.call('userSetUtcOffset', utcOffset); | |
| } | |
| emitStatusChange(user.status); | |
| }; | |
| const runUserDataSync = async (uid: string) => { | |
| // synchronizeUserData opens a `stream-notify-user/${uid}/userData` sub | |
| // over DDPSDK. The server rejects that sub with "not-allowed" until | |
| // DDPSDK has completed loginWithToken on its own socket. Both | |
| // runUserDataSync and ensureConnectedAndAuthenticated are subscribers | |
| // of userIdStore, so without sequencing the sub races the auth and | |
| // hits the rejection on every re-login. Await the SDK auth here so | |
| // the sub fires authenticated. | |
| try { | |
| await ensureConnectedAndAuthenticated(); | |
| } catch { | |
| // non-fatal: sdk.stream queues until DDPSDK eventually auths | |
| } | |
| if (userIdStore.getState() !== uid) { | |
| return; | |
| } | |
| const user = await synchronizeUserData(uid); | |
| if (!user || userIdStore.getState() !== uid) return; | |
| const utcOffset = -new Date().getTimezoneOffset() / 60; | |
| if (user.utcOffset !== utcOffset) { | |
| sdk.call('userSetUtcOffset', utcOffset); | |
| } | |
| emitStatusChange(user.status); | |
| }; |
🤖 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/startup/startup.ts` around lines 43 - 65, runUserDataSync
can apply stale session data after awaiting; after any await points (notably
after ensureConnectedAndAuthenticated() and synchronizeUserData(uid)) re-check
that userIdStore.getState() === uid (and that user is still present) and return
early if it changed; only then call sdk.call('userSetUtcOffset', ...) and
emitStatusChange(user.status) to avoid leaking updates into a different or
logged-out session.
| if (!sdkTransportEnabled) { | ||
| // Flag off: develop's exact behaviour — only set the session flag. | ||
| // The legacy WS-reconnect → loginWithToken-fails → accounts-base | ||
| // chain handles the actual local cleanup. | ||
| return; | ||
| } |
There was a problem hiding this comment.
Do not skip local credential cleanup on the legacy branch
Returning early on Line 20 means Meteor.loginToken/userId can remain populated after force_logout until a later reconnect path runs. That creates stale-auth races (notably around startup sync/reset logic) and can leave the client in an inconsistent logged-in state.
Proposed fix
- if (!sdkTransportEnabled) {
- // Flag off: develop's exact behaviour — only set the session flag.
- // The legacy WS-reconnect → loginWithToken-fails → accounts-base
- // chain handles the actual local cleanup.
- return;
- }
+ // Always clear local auth artifacts immediately on force logout.
+ // This avoids stale session races while reconnect/logout chains settle.🤖 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/views/root/hooks/loggedIn/useForceLogout.ts` around lines
20 - 25, The early return in useForceLogout (the branch checking
sdkTransportEnabled) skips local credential cleanup and leaves
Meteor.loginToken/userId set; remove the early return and ensure the legacy
branch still performs the same local cleanup steps (clear
Meteor._localStorage/Meteor.loginToken and Meteor.userId, and call the same
local logout/cleanup functions used in the SDK path) when handling the
force_logout path in useForceLogout so credentials are cleared immediately even
if sdkTransportEnabled is false.
| // Graceful close: lets the WS lib flush queued frames (including | ||
| // the `notify-user/<uid>/force_logout` stream message that the | ||
| // monolith listener at apps/meteor/server/modules/listeners/listeners.module.ts:49 | ||
| // just enqueued) before the socket goes down. Previously this was | ||
| // `ws.terminate()`, which sends a TCP RST immediately and drops | ||
| // the queued frames — clients depending on the stream message | ||
| // (useForceLogout hook → Accounts._unstoreLoginToken + setUserId(null)) | ||
| // then never see the cleanup, leaving stale credentials in | ||
| // localStorage. Falls back to terminate() after a short grace | ||
| // period for unresponsive sockets. |
There was a problem hiding this comment.
Move this rationale out of implementation code.
Please remove the long inline explanation and keep the behavior self-evident in code; rationale can live in PR/changeset docs.
Proposed change
- // Graceful close: lets the WS lib flush queued frames (including
- // the `notify-user/<uid>/force_logout` stream message that the
- // monolith listener at apps/meteor/server/modules/listeners/listeners.module.ts:49
- // just enqueued) before the socket goes down. Previously this was
- // `ws.terminate()`, which sends a TCP RST immediately and drops
- // the queued frames — clients depending on the stream message
- // (useForceLogout hook → Accounts._unstoreLoginToken + setUserId(null))
- // then never see the cleanup, leaving stale credentials in
- // localStorage. Falls back to terminate() after a short grace
- // period for unresponsive sockets.
ws.close();As per coding guidelines, **/*.{ts,tsx,js}: Avoid code comments in implementation.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Graceful close: lets the WS lib flush queued frames (including | |
| // the `notify-user/<uid>/force_logout` stream message that the | |
| // monolith listener at apps/meteor/server/modules/listeners/listeners.module.ts:49 | |
| // just enqueued) before the socket goes down. Previously this was | |
| // `ws.terminate()`, which sends a TCP RST immediately and drops | |
| // the queued frames — clients depending on the stream message | |
| // (useForceLogout hook → Accounts._unstoreLoginToken + setUserId(null)) | |
| // then never see the cleanup, leaving stale credentials in | |
| // localStorage. Falls back to terminate() after a short grace | |
| // period for unresponsive sockets. | |
| ws.close(); |
🤖 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 `@ee/apps/ddp-streamer/src/DDPStreamer.ts` around lines 63 - 72, The large
inline rationale in DDPStreamer (around the socket shutdown logic that uses
ws.close() with a fallback to ws.terminate()) should be removed from
implementation code; replace the block comment with a concise one-line note like
"Graceful close with terminate fallback" or delete the comment entirely, and
move the detailed explanation into the PR/changelog. Locate the socket shutdown
code in the DDPStreamer class (the section referencing ws.close() and
ws.terminate()) and update the comment accordingly while leaving the existing
behavior (close then terminate fallback) unchanged.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #40430 +/- ##
===========================================
- Coverage 69.99% 69.57% -0.42%
===========================================
Files 3309 3317 +8
Lines 120956 121867 +911
Branches 21683 21855 +172
===========================================
+ Hits 84666 84794 +128
- Misses 32990 33744 +754
- Partials 3300 3329 +29
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
fix/sdk-v2-totp-and-status-regressions
branch
from
ed8edd9 to
57e1e2d
Compare
1 participant
Summary
Re-adds the experimental SDK-over-DDP transport originally landed in #40301, with two regression fixes found while validating the legacy (flag-off) path.
Use_RC_SDKadmin setting and the@rocket.chat/ddp-client-backed transport from chore(@rocket.chat/ddp-client): introduce experimental Meteor independent DDP client #40301. Default remains off — the migration ships dormant.resultframe frommethod.call/method.callAnonREST responses back into Meteor's_streamHandlersinstead of synthesising anerror: 'unknown'envelope. Without this,withAsyncTOTPno longer recognisedtotp-requiredand the 2FA modal never opened for any Meteor method protected bytwoFactorRequired.{ ...Meteor.status() }spread inServerProvider.getStatuson the legacy path.Meteor.status()reuses the same internal object (mutated in place), so dropping the spread madeuseSyncExternalStoresee an unchanged snapshot andConnectionStatusBarfroze on connect/drop.Test plan
Use_RC_SDKoff (default), trigger a Meteor method protected bytwoFactorRequired(e.g.saveUserProfilefor a user with TOTP enabled) and confirm the TOTP modal opens and the call resolves once a valid code is submitted.Use_RC_SDKoff, drop the network and confirmConnectionStatusBarflips to "waiting"/"connecting" and back to "connected" without a hard reload.Use_RC_SDKon (URL?sdk_transport=onor admin setting), smoke test login, message send, presence, and force-logout.Task: ARCH-2127
Summary by CodeRabbit
New Features
Chores