chore(@rocket.chat/ddp-client): introduce experimental Meteor independent DDP client#40301
Merged
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 |
🦋 Changeset detectedLatest commit: 95ecdba 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 |
Contributor
WalkthroughThis PR introduces an experimental SDK-over-DDP transport feature that routes Meteor DDP traffic through ChangesSDK-over-DDP Transport Migration
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes This PR introduces a substantial and intricate architectural migration affecting authentication, subscription, streaming, and DDP protocol layers across client and server. The changes span multiple new SDK modules, numerous client overrides, and complex interactions between Meteor's legacy transport, DDPSDK, and login/presence flows. Each logical section introduces dense logic with specific retry semantics, error handling, and state management that requires careful reasoning to ensure compatibility with both SDK-enabled and fallback Meteor-backed modes. 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. Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
ggazzo
changed the title
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## fix/ddp-dispatcher-non-method-frames #40301 +/- ##
========================================================================
- Coverage 69.96% 69.58% -0.39%
========================================================================
Files 3309 3309
Lines 120994 121346 +352
Branches 21805 21730 -75
========================================================================
- Hits 84659 84439 -220
- Misses 33026 33582 +556
- Partials 3309 3325 +16
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
worktree-sdk-over-ddp-client
branch
3 times, most recently
from
e0d347f to
de8a406
Compare
ggazzo
changed the title
ggazzo
force-pushed
the
worktree-sdk-over-ddp-client
branch
6 times, most recently
from
4dfa46d to
56c5e1e
Compare
Member
Author
|
@copilot resolve the merge conflicts in this pull request |
Contributor
ggazzo
force-pushed
the
worktree-sdk-over-ddp-client
branch
3 times, most recently
from
d24843c to
c2d763d
Compare
|
|
ggazzo
changed the base branch from
develop
to
fix/ddp-dispatcher-non-method-frames
DDPDispatcher's pushItem path runs for every payload that goes through dispatch — including connect, sub, unsub, and ping/pong. When the head of the queue is a wait block (e.g. a login), pushItem deliberately returns without sending so the next method waits. Applied to a non- method payload, that means the connect frame ws.onopen emits gets dropped on the floor: the socket sits open, the DDP handshake never happens, and any caller waiting on `connected` hangs forever. Visible failure mode: token-resume on page reload dispatched the wait login while the socket was still connecting; when ws.onopen later fired the connect frame, it was queued behind that login and never sent. Status stuck at "connecting". Fix: short-circuit dispatch for any payload whose msg is not 'method' — emit it straight away. Wait/queue serialization is a property of method calls, not protocol frames.
Previously both rejected with `Error('Connection in progress')` when
called against an already-connecting/connected state. That's hostile to
the only callers we have:
- The retry timer scheduled by `ws.onclose` fires `void this.reconnect()`
with no `.catch`. If an external `connect()` won the race, the timer
rejected and the unhandled rejection surfaced as a pageError on every
reconnect cycle.
- Bootstrap paths in SDK consumers (e.g. apps/meteor/client/lib/sdk/ddpSdk.ts)
already wrap in `.catch(() => {})` purely to silence the same noise.
Make both methods resolve with the current status when the connection is
already in flight or established, and have them clear the pending retry
timer on entry. The retry timer's callback also re-checks the status
before calling `reconnect()` so a connection re-established in the
meantime doesn't trigger a redundant reset cycle.
Also short-circuit `ws.onclose` when the closed socket isn't `this.ws`
anymore — a stale close from a replaced socket otherwise flipped
`status` back to `disconnected`, bumped `retryCount`, and scheduled a
fresh timer while a newer socket was healthy.
Tests cover all four shapes:
- reconnect/connect when already connected → resolves, doesn't throw
- retry timer firing after an external connect won the race → no
unhandled rejection, status stays connected
- stale ws.onclose after socket replacement → status and retryCount
untouched, no extra timer scheduled
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Companion to the 'idempotent connect/reconnect' fix in 7742877. retryCount was incremented on every disconnect but never zeroed on successful (re)connect. With the default budget of 1, a single force-logout cycle that chains back-to-back ws.close events (server broadcasts force_logout → SDK reconnects → app calls Meteor.logout() on top → server's logout handler closes the WS again) drained the budget. The next disconnect's onclose handler bailed at `retryCount >= retryOptions.retryCount`, leaving the SDK permanently disconnected. Method frames already in the dispatcher queue stayed queued forever — observed in EE microservices CI as e2ee-passphrase-management.spec.ts:87 (and :76) where the final loginByUserState's login frame never reached the server because the SDK socket never reconnected. Stack trace pointing at the offending second logout: Accounts.logout → standardLogout (saml.ts) → logout (UserProvider) → useResetE2EPasswordMutation.onSuccess The reset E2E password mutation calls `Meteor.logout()` on success by design (to force re-login with the new keys). In monolith CE that went via Meteor's own connection (separate retry policy); in EE microservices it now hits the SDK socket via stubMeteorStream and exposes the budget exhaustion. Verified locally with BASE_URL=http://localhost:4000 IS_EE=true: - e2ee-passphrase-management.spec.ts:76 ✓ - e2ee-passphrase-management.spec.ts:87 ✓ - e2ee-key-reset.spec.ts ✓ Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ggazzo
force-pushed
the
fix/ddp-dispatcher-non-method-frames
branch
from
6084971 to
881312e
Compare
ggazzo
force-pushed
the
worktree-sdk-over-ddp-client
branch
9 times, most recently
from
c8b8f18 to
a2f7db0
Compare
Adds the `Use_RC_SDK` admin setting (General → Use Rocket.Chat SDK) and
matching client-side opt-in helper. The migration ships dormant — legacy
Meteor DDP transport remains the default. Three sources resolve the flag,
URL > localStorage > meta tag:
- `?sdk_transport=on|off` URL parameter (per-tab)
- `rc-config-sdk_transport` localStorage entry (per-user)
- `<meta name="rc-sdk-transport-enabled">` injected from the
`Use_RC_SDK` setting (workspace-wide opt-in / kill-switch)
The flag is read once at module init and used to dispatch the SDK code
paths back to their Meteor equivalents when off:
- The 5 SDK-transport overrides (`ddpOverREST`, `ddpSdkCollectionBridge`,
`subscribeViaSDK`, `stubMeteorStream`, `killMeteorStream`) wrap their
bodies in `if (isSdkTransportEnabled())` — `Meteor.connection._send`,
`_subscribe`, and `_stream` stay un-wrapped when off.
- `apps/meteor/app/utils/client/lib/SDKClient.ts` keeps both
`createNewMeteorStream` and `createNewDdpSdkStream`. With the flag off
streams use Meteor's subscribe + the original
`Meteor.connection._stream.on('message')` bridge; with the flag on the
SDK stream is used. `publish` similarly dispatches between
`Meteor.call` and `getDdpSdk().client.callAsync`.
- `apps/meteor/client/providers/ServerProvider.tsx` only subscribes to
DDPSDK connection events, combines DDPSDK status into `getStatus`,
and calls `ensureConnectedAndAuthenticated` from `reconnect()` /
`getDdpSdk().connection.close()` from `disconnect()` when the flag is
on. With the flag off, `getStatus` returns `Meteor.status()` directly.
- The boot-time side-effect block in `ddpSdk.ts` (loginWithToken wrap,
userIdStore subscriber, reset-on-reconnect listener) is gated as well
— keeping the SDK socket "anonymous" when off so it doesn't create
duplicate Presence sessions.
E2e default stays off, matching CI's dormant baseline; the SDK-transport
path is reachable via URL/localStorage for manual verification or a
dedicated future job.
ggazzo
force-pushed
the
worktree-sdk-over-ddp-client
branch
from
a2f7db0 to
6e52a48
Compare
ggazzo
changed the title
coderabbitai
Bot
added
the
type: feature
![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: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
ee/apps/ddp-streamer/src/DDPStreamer.ts (1)
53-79:⚠️ Potential issue | 🟠 Major | ⚡ Quick winApply the terminate fallback to the sessionId force-logout path too.
Line [58] uses
ws.close()without the 5s guard, while the uid path (Line [74]-Line [79]) has a hard-stop fallback. This leaves one forced-logout path weaker and potentially hanging.Suggested patch
this.onEvent('user.forceLogout', (uid: string, sessionId?: string) => { + const closeWithGuard = (ws: WebSocket): void => { + ws.close(); + const guard = setTimeout(() => { + if (ws.readyState !== WebSocket.CLOSED) { + ws.terminate(); + } + }, 5000); + ws.once('close', () => clearTimeout(guard)); + }; + this.wss?.clients.forEach((ws) => { const client = clientMap.get(ws); if (sessionId) { if (client?.connection.id === sessionId) { - ws.close(); + closeWithGuard(ws); } return; } if (client?.userId === uid) { - ws.close(); - const guard = setTimeout(() => { - if (ws.readyState !== ws.CLOSED) { - ws.terminate(); - } - }, 5000); - ws.once('close', () => clearTimeout(guard)); + closeWithGuard(ws); } }); });🤖 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 53 - 79, The sessionId branch inside the onEvent('user.forceLogout') handler currently calls ws.close() but lacks the 5s terminate fallback used in the uid branch; update the sessionId path (the block that checks if client?.connection.id === sessionId) to perform ws.close(), create the same guard timer that calls ws.terminate() after 5000ms if ws.readyState !== ws.CLOSED, and clear that timer on ws.once('close', ...) so the session-specific forced-logout uses the same graceful-close-with-terminate-fallback logic as the uid path.apps/meteor/app/utils/client/lib/SDKClient.ts (1)
26-47:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard
nullbefore dereferencing the DDP payload.
typeof null === 'object', so these checks still letnullreachmsg.msg/msg.fields.eventName. A malformed frame would throw inside the stream bridge instead of being ignored.Suggested fix
const isChangedCollectionPayload = ( msg: any, ): msg is { msg: 'changed'; collection: string; fields: { eventName: string; args: unknown[] } } => { - if (typeof msg !== 'object' && (msg !== null || msg !== undefined)) { + if (msg == null || typeof msg !== 'object') { return false; } if (msg.msg !== 'changed') { return false; } if (typeof msg.collection !== 'string') { return false; } - if (typeof msg.fields !== 'object' && (msg.fields !== null || msg.fields !== undefined)) { + if (msg.fields == null || typeof msg.fields !== 'object') { return false; } if (typeof msg.fields.eventName !== 'string') { return false; }🤖 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 26 - 47, The type-guard is letting null through because typeof null === 'object'; update isChangedCollectionPayload so it first explicitly rejects null/undefined before dereferencing: in the function (identify by name isChangedCollectionPayload and the parameter msg) replace the incorrect checks with explicit null/undefined guards such as if (msg === null || msg === undefined || typeof msg !== 'object') return false; and similarly for fields use if (msg.fields === null || msg.fields === undefined || typeof msg.fields !== 'object') return false; ensuring those checks occur before accessing msg.msg or msg.fields.eventName.
🧹 Nitpick comments (1)
ee/apps/ddp-streamer/src/DDPStreamer.ts (1)
63-72: ⚡ Quick winRemove the large inline implementation comment.
Please avoid embedding this rationale block directly in runtime code; keep intent in naming or external docs instead.
As per coding guidelines: "Avoid code comments in the implementation".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ee/apps/ddp-streamer/src/DDPStreamer.ts` around lines 63 - 72, Remove the large multi-line rationale block embedded in DDPStreamer.ts and replace it with a concise one-line comment or a self-documenting identifier; specifically, in the method handling socket shutdown (the code touching the ws object and the graceful close + terminate fallback logic), delete the long inline explanation and either rename the function/variable to convey intent (e.g., gracefulCloseWithFallback) or add a short comment like "Graceful close with terminate fallback for unresponsive sockets." Move the full rationale and behavior details to external docs (README or design doc) and reference that doc in a brief in-code note if needed.
🤖 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 code currently calls
ensureConnectedAndAuthenticated().catch(() => undefined).then(...), so the .then
branch runs even after a reject and recreates the unwanted anonymous
subscription; fix by only executing the subscription logic when
ensureConnectedAndAuthenticated resolves successfully — move the .catch to the
end (call ensureConnectedAndAuthenticated().then(() => { /* subscribe:
getDdpSdk(), sdk.client.subscribe(...), subscription.ready(),
sdk.client.onCollection(...) */ }).catch(() => undefined)) or explicitly gate
the .then body on a successful result; update references to getDdpSdk(),
subscription, offCollection, meta.ready and the ready/error event emissions
accordingly so they run only on successful authentication/connection.
In `@apps/meteor/client/lib/sdk/ddpSdk.ts`:
- Around line 118-149: The auth-wrapper around sdk.account.loginWithToken is
swallowing authentication errors (returning undefined) so inflightLogin never
rejects and ensureConnectedAndAuthenticated's cleanup is skipped; update that
wrapper (the function wrapping sdk.account.loginWithToken) to let auth errors
propagate by rethrowing the error after any local cleanup (e.g. after calling
Accounts._unstoreLoginToken() and setting Meteor.connection.setUserId(null))
instead of returning undefined, so inflightLogin rejects and
ensureConnectedAndAuthenticated can run its stale-token recovery.
- Around line 30-47: startConnect currently swallows connect() failures and
waitForConnected only waits for the 'connected' event, which can deadlock
ensureConnectedAndAuthenticated; update startConnect (the function calling
sdk.connection.connect()) to re-throw the caught error (do not swallow in the
catch) so the promise rejects on failure, and change the call site in
ensureConnectedAndAuthenticated from using void startConnect(sdk) to await
startConnect(sdk) so connection errors surface immediately instead of waiting
forever in waitForConnected.
In `@apps/meteor/client/lib/sdk/meteorBackedSdk.ts`:
- Around line 169-173: The close() implementation currently calls
Meteor.disconnect() while connect() is a no-op, which breaks shared Meteor for
callers that expect to reconnect; either make both proxy methods no-ops or
implement connect() to restore the Meteor connection and restart the status
bridge. Update the proxy's connect() and close() (referencing connect, close,
stopBridge, and Meteor.disconnect) so they are symmetric: either remove
Meteor.disconnect() and stopBridge?() from close() so both methods are inert, or
have connect() call Meteor.reconnect() (or the correct Meteor connection
restore) and reinitialize the status bridge (reset stopBridge) so a subsequent
connect() after close() returns the client to an operational state.
In `@apps/meteor/client/lib/sdk/sdkTransportEnabled.ts`:
- Around line 19-24: The current logic only short-circuits on URL 'on' and
treats a stored localStorage='off' as if absent, so change the precedence to:
first read URL param (if 'on' return true; if 'off' return false), then read
localStorage for KEY and return true if 'on' or false if 'off' (i.e. honor
explicit 'off' before falling back), and only after those checks consult the
meta tag (META_NAME). Update the code that uses fromUrl,
window.localStorage.getItem(`rc-config-${KEY}`), and
meta?.getAttribute('content') to implement that exact URL > localStorage > meta
precedence.
In `@apps/meteor/client/meteor/overrides/ddpOverREST.ts`:
- Around line 73-80: The endpoint selection currently uses getUserId() and
wasResumeLogin to choose between 'method.call' and 'method.callAnon'; change it
so that any non-resume login goes through 'method.callAnon' regardless of
getUserId(): update the logic that sets endpoint (the variable named endpoint)
to check if methodName === 'login' && !wasResumeLogin and force
'method.callAnon', otherwise keep the existing branch between 'method.call' and
'method.callAnon' based on getUserId() and wasResumeLogin.
In `@apps/meteor/client/meteor/overrides/stubMeteorStream.ts`:
- Around line 89-90: The Tracker.Dependency type and usage are inverted: extend
the inline type for TrackerDependency to include depend(): void (not just
changed()), update status() to call statusListeners.depend() so callers register
a reactive dependency, and ensure wherever you signal updates (the code that
mutates status) calls statusListeners.changed() to invalidate dependents; apply
the same fix to the other occurrence around the 124–130 block (use the same
TrackerDependency, call depend() in readers like status(), and call changed()
when the status value is updated).
In `@apps/meteor/client/meteor/overrides/subscribeViaSDK.ts`:
- Around line 17-21: The rejection path currently only calls onError but must
also propagate SDK subscription failures via onStop(err) to match Meteor
semantics; update the subscribeViaSDK promise rejection handler (the code using
SubscribeCallbacks and its onError/onStop callbacks) to call onStop(err) with
the error when a subscription fails (in addition to or instead of onError as
appropriate), ensuring the error is passed through to callers expecting
server-initiated failure via onStop.
In `@apps/meteor/client/startup/startup.ts`:
- Around line 80-98: syncOnce currently sets lastSyncedUid before
runUserDataSync completes which causes in-flight attempts to be deduped and lost
on failure; change syncOnce to track in-flight vs completed separately (e.g.
introduce an inFlightUid or a Set inFlightUids) and only set lastSyncedUid after
runUserDataSync resolves successfully, or alternatively record a pendingRetry
flag and in the runUserDataSync finally block check if userIdStore.getState()
=== uid and replay the sync if needed; update references in syncOnce,
lastSyncedUid, runUserDataSync error/finally handling and any callers
(onLoggedIn, userIdStore.subscribe, boot-time call) so retries are not
swallowed.
In `@apps/meteor/tests/e2e/page-objects/fragments/home-content.ts`:
- Around line 139-148: The current wait uses messageListItems.count() and
lastUserMessage which can track the wrong tail item if other messages arrive;
change the final wait to target the specific message you sent by capturing the
message text (or the exact item index created: messageListItems.nth(before)) and
poll/assert that that specific element has left the pending state and contains
the sent text. Concretely, after btnSend.click() store the sent text (or compute
the newItem = this.messageListItems.nth(before)), then replace the count-based
poll and lastUserMessage assertion with a poll/expect against newItem (or the
element located by the sent text) toBeVisible/
not.toHaveClass(/rcx-message--pending/) and toContainText(sentText).
---
Outside diff comments:
In `@apps/meteor/app/utils/client/lib/SDKClient.ts`:
- Around line 26-47: The type-guard is letting null through because typeof null
=== 'object'; update isChangedCollectionPayload so it first explicitly rejects
null/undefined before dereferencing: in the function (identify by name
isChangedCollectionPayload and the parameter msg) replace the incorrect checks
with explicit null/undefined guards such as if (msg === null || msg ===
undefined || typeof msg !== 'object') return false; and similarly for fields use
if (msg.fields === null || msg.fields === undefined || typeof msg.fields !==
'object') return false; ensuring those checks occur before accessing msg.msg or
msg.fields.eventName.
In `@ee/apps/ddp-streamer/src/DDPStreamer.ts`:
- Around line 53-79: The sessionId branch inside the onEvent('user.forceLogout')
handler currently calls ws.close() but lacks the 5s terminate fallback used in
the uid branch; update the sessionId path (the block that checks if
client?.connection.id === sessionId) to perform ws.close(), create the same
guard timer that calls ws.terminate() after 5000ms if ws.readyState !==
ws.CLOSED, and clear that timer on ws.once('close', ...) so the session-specific
forced-logout uses the same graceful-close-with-terminate-fallback logic as the
uid path.
---
Nitpick comments:
In `@ee/apps/ddp-streamer/src/DDPStreamer.ts`:
- Around line 63-72: Remove the large multi-line rationale block embedded in
DDPStreamer.ts and replace it with a concise one-line comment or a
self-documenting identifier; specifically, in the method handling socket
shutdown (the code touching the ws object and the graceful close + terminate
fallback logic), delete the long inline explanation and either rename the
function/variable to convey intent (e.g., gracefulCloseWithFallback) or add a
short comment like "Graceful close with terminate fallback for unresponsive
sockets." Move the full rationale and behavior details to external docs (README
or design doc) and reference that doc in a brief in-code note if needed.
🪄 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: 3177b8d5-b387-4f39-9f40-7f05a41971cb
⛔ 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
🧰 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/views/root/hooks/loggedIn/useForceLogout.tsapps/meteor/client/meteor/overrides/ddpSdkCollectionBridge.tsapps/meteor/client/meteor/overrides/subscribeViaSDK.tsapps/meteor/app/ui-master/server/index.tsapps/meteor/client/lib/sdk/streamerAdapter.tsapps/meteor/client/lib/sdk/sdkTransportEnabled.tsapps/meteor/app/notifications/client/lib/Presence.tsapps/meteor/server/settings/general.tsapps/meteor/client/meteor/overrides/index.tsapps/meteor/client/meteor/overrides/stubMeteorStream.tsapps/meteor/client/lib/presence.tsapps/meteor/client/providers/ServerProvider.tsxee/apps/ddp-streamer/src/DDPStreamer.tsapps/meteor/client/lib/sdk/meteorBackedSdk.tsapps/meteor/client/startup/startup.tsapps/meteor/client/lib/cachedStores/CachedStore.tsapps/meteor/client/lib/sdk/ddpSdk.tsapps/meteor/client/meteor/overrides/ddpOverREST.tsapps/meteor/client/meteor/overrides/killMeteorStream.tsapps/meteor/tests/e2e/page-objects/fragments/home-content.tsapps/meteor/app/utils/client/lib/SDKClient.tsapps/meteor/client/lib/loggedIn.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 (7)
📚 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/views/root/hooks/loggedIn/useForceLogout.tsapps/meteor/client/meteor/overrides/ddpSdkCollectionBridge.tsapps/meteor/client/meteor/overrides/subscribeViaSDK.tsapps/meteor/client/lib/sdk/streamerAdapter.tsapps/meteor/client/lib/sdk/sdkTransportEnabled.tsapps/meteor/client/meteor/overrides/index.tsapps/meteor/client/meteor/overrides/stubMeteorStream.tsapps/meteor/client/lib/presence.tsapps/meteor/client/lib/sdk/meteorBackedSdk.tsapps/meteor/client/startup/startup.tsapps/meteor/client/lib/cachedStores/CachedStore.tsapps/meteor/client/lib/sdk/ddpSdk.tsapps/meteor/client/meteor/overrides/ddpOverREST.tsapps/meteor/client/meteor/overrides/killMeteorStream.tsapps/meteor/client/lib/loggedIn.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/views/root/hooks/loggedIn/useForceLogout.tsapps/meteor/client/meteor/overrides/ddpSdkCollectionBridge.tsapps/meteor/client/meteor/overrides/subscribeViaSDK.tsapps/meteor/app/ui-master/server/index.tsapps/meteor/client/lib/sdk/streamerAdapter.tsapps/meteor/client/lib/sdk/sdkTransportEnabled.tsapps/meteor/app/notifications/client/lib/Presence.tsapps/meteor/server/settings/general.tsapps/meteor/client/meteor/overrides/index.tsapps/meteor/client/meteor/overrides/stubMeteorStream.tsapps/meteor/client/lib/presence.tsee/apps/ddp-streamer/src/DDPStreamer.tsapps/meteor/client/lib/sdk/meteorBackedSdk.tsapps/meteor/client/startup/startup.tsapps/meteor/client/lib/cachedStores/CachedStore.tsapps/meteor/client/lib/sdk/ddpSdk.tsapps/meteor/client/meteor/overrides/ddpOverREST.tsapps/meteor/client/meteor/overrides/killMeteorStream.tsapps/meteor/tests/e2e/page-objects/fragments/home-content.tsapps/meteor/app/utils/client/lib/SDKClient.tsapps/meteor/client/lib/loggedIn.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/views/root/hooks/loggedIn/useForceLogout.tsapps/meteor/client/meteor/overrides/ddpSdkCollectionBridge.tsapps/meteor/client/meteor/overrides/subscribeViaSDK.tsapps/meteor/app/ui-master/server/index.tsapps/meteor/client/lib/sdk/streamerAdapter.tsapps/meteor/client/lib/sdk/sdkTransportEnabled.tsapps/meteor/app/notifications/client/lib/Presence.tsapps/meteor/server/settings/general.tsapps/meteor/client/meteor/overrides/index.tsapps/meteor/client/meteor/overrides/stubMeteorStream.tsapps/meteor/client/lib/presence.tsee/apps/ddp-streamer/src/DDPStreamer.tsapps/meteor/client/lib/sdk/meteorBackedSdk.tsapps/meteor/client/startup/startup.tsapps/meteor/client/lib/cachedStores/CachedStore.tsapps/meteor/client/lib/sdk/ddpSdk.tsapps/meteor/client/meteor/overrides/ddpOverREST.tsapps/meteor/client/meteor/overrides/killMeteorStream.tsapps/meteor/tests/e2e/page-objects/fragments/home-content.tsapps/meteor/app/utils/client/lib/SDKClient.tsapps/meteor/client/lib/loggedIn.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: 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
📚 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
🔇 Additional comments (3)
apps/meteor/package.json (1)
105-105: Dependency addition looks correct for the flagged transport rollout.The workspace dependency placement under
dependenciesmatches the runtime use case described in this PR.packages/i18n/src/locales/en.i18n.json (1)
5619-5621: Looks good — clear and consistent localization keys for the new setting.The label, description, and alert copy are coherent and match the existing i18n key style.
apps/meteor/client/providers/ServerProvider.tsx (1)
107-110: Please verifyconnection.on('connection')is a real DDPSDK event.The rest of this PR subscribes to
connected/disconnected. Ifconnectionis not emitted here,ddpSdkStatusDep.changed()never runs anduseReactiveValue(getStatus)stays stale after transport transitions.
Comment on lines
+178
to
+204
| 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); | ||
| }); | ||
| }); |
Contributor
There was a problem hiding this comment.
Don't create the SDK subscription after auth/connect failure.
This chain still runs the .then(...) branch after ensureConnectedAndAuthenticated() rejects, which recreates the anonymous nosub path this helper is trying to avoid.
Suggested fix
- void ensureConnectedAndAuthenticated()
- .catch(() => undefined)
- .then(() => {
+ void ensureConnectedAndAuthenticated()
+ .then(() => {
if (stopped) return;
const sdk = getDdpSdk();
subscription = sdk.client.subscribe(`stream-${streamName}`, key, { useCollection: false, args });
…
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);
});
- });
+ })
+ .catch((err) => {
+ if (stopped) return;
+ ee.emit('ready', [err]);
+ ee.emit('error', 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 178 - 204, The
code currently calls ensureConnectedAndAuthenticated().catch(() =>
undefined).then(...), so the .then branch runs even after a reject and recreates
the unwanted anonymous subscription; fix by only executing the subscription
logic when ensureConnectedAndAuthenticated resolves successfully — move the
.catch to the end (call ensureConnectedAndAuthenticated().then(() => { /*
subscribe: getDdpSdk(), sdk.client.subscribe(...), subscription.ready(),
sdk.client.onCollection(...) */ }).catch(() => undefined)) or explicitly gate
the .then body on a successful result; update references to getDdpSdk(),
subscription, offCollection, meta.ready and the ready/error event emissions
accordingly so they run only on successful authentication/connection.
Comment on lines
+30
to
+47
| const startConnect = (sdk: DDPSDK): Promise<unknown> => { | ||
| if (connectPromise) return connectPromise; | ||
| connectPromise = sdk.connection.connect().catch((err) => { | ||
| console.warn('[ddpSdk] connect failed', err); | ||
| // Allow a retry on the next call. | ||
| connectPromise = undefined; | ||
| }); | ||
| return connectPromise; | ||
| }; | ||
|
|
||
| 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(); | ||
| }); | ||
| }); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
wc -l apps/meteor/client/lib/sdk/ddpSdk.tsRepository: RocketChat/Rocket.Chat
Length of output: 107
🏁 Script executed:
cat -n apps/meteor/client/lib/sdk/ddpSdk.tsRepository: RocketChat/Rocket.Chat
Length of output: 13826
A failed connect() can deadlock every auth caller.
startConnect() swallows the rejection in its catch handler without re-throwing, and waitForConnected() only resolves when the 'connected' event fires. If sdk.connection.connect() fails, the promise settles successfully but the connection never emits 'connected', leaving ensureConnectedAndAuthenticated() hanging indefinitely at line 90. Any caller awaiting this function (stream bootstrap, auth flows) stalls until timeout.
The fix requires both: re-throw the error in startConnect() and change void startConnect(sdk) to await startConnect(sdk) at line 88 so the rejection surfaces and prevents waiting on an event that will never fire.
Suggested fix
const startConnect = (sdk: DDPSDK): Promise<unknown> => {
if (connectPromise) return connectPromise;
connectPromise = sdk.connection.connect().catch((err) => {
console.warn('[ddpSdk] connect failed', err);
// Allow a retry on the next call.
connectPromise = undefined;
+ throw err;
});
return connectPromise;
};
…
if (
sdk.connection.status === 'idle' ||
sdk.connection.status === 'closed' ||
sdk.connection.status === 'disconnected' ||
sdk.connection.status === 'failed'
) {
- void startConnect(sdk);
+ await startConnect(sdk);
}
await waitForConnected(sdk);🤖 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 30 - 47, startConnect
currently swallows connect() failures and waitForConnected only waits for the
'connected' event, which can deadlock ensureConnectedAndAuthenticated; update
startConnect (the function calling sdk.connection.connect()) to re-throw the
caught error (do not swallow in the catch) so the promise rejects on failure,
and change the call site in ensureConnectedAndAuthenticated from using void
startConnect(sdk) to await startConnect(sdk) so connection errors surface
immediately instead of waiting forever in waitForConnected.
Comment on lines
+118
to
+149
| inflightLogin = (async () => { | ||
| try { | ||
| await sdk.account.loginWithToken(token); | ||
| } finally { | ||
| inflightLogin = undefined; | ||
| } | ||
| })(); | ||
|
|
||
| try { | ||
| await inflightLogin; | ||
| } catch (error) { | ||
| if (isAuthError(error) && readStoredLoginToken() === token) { | ||
| // Server rejected the stored token. Without this branch the stored | ||
| // token stays in localStorage forever and the router keeps the user | ||
| // wedged on /home with no main UI and no login form: ddpOverREST | ||
| // routes Meteor's resume login through DDPSDK / REST (not Meteor's | ||
| // own WS), and on rejection the resume invoker errors but the | ||
| // account state isn't cleared automatically. The token-stable | ||
| // guard (readStoredLoginToken() === token) avoids kicking the user | ||
| // out when localStorage was updated mid-flight by a parallel flow | ||
| // (fresh registration, Meteor's own resume) — the 401 is then on a | ||
| // stale token a newer credential already replaced. Drop the local | ||
| // credentials manually instead of calling Meteor.logout(): the | ||
| // latter dispatches a `logout` method which itself races against | ||
| // parallel re-auth flows in CI's parallel-shard environment and | ||
| // kicked otherwise-healthy tests out. | ||
| Accounts._unstoreLoginToken(); | ||
| (Meteor.connection as unknown as { setUserId: (uid: string | null) => void }).setUserId(null); | ||
| return; | ||
| } | ||
| console.warn('[ddpSdk] loginWithToken failed', error); | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
wc -l apps/meteor/client/lib/sdk/ddpSdk.tsRepository: RocketChat/Rocket.Chat
Length of output: 107
🏁 Script executed:
cat -n apps/meteor/client/lib/sdk/ddpSdk.ts | sed -n '100,270p'Repository: RocketChat/Rocket.Chat
Length of output: 8945
The wrapper swallows auth failures, preventing cleanup in ensureConnectedAndAuthenticated().
The wrapper at lines 250–264 intercepts all sdk.account.loginWithToken() calls globally. When an auth error occurs (line 256), it returns undefined (line 260) instead of throwing. This causes the await inflightLogin at line 127 to resolve successfully, bypassing the catch block that cleans up stale credentials at lines 144–146. The stale-token recovery logic is thus skipped for auth failures.
🤖 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 118 - 149, The
auth-wrapper around sdk.account.loginWithToken is swallowing authentication
errors (returning undefined) so inflightLogin never rejects and
ensureConnectedAndAuthenticated's cleanup is skipped; update that wrapper (the
function wrapping sdk.account.loginWithToken) to let auth errors propagate by
rethrowing the error after any local cleanup (e.g. after calling
Accounts._unstoreLoginToken() and setting Meteor.connection.setUserId(null))
instead of returning undefined, so inflightLogin rejects and
ensureConnectedAndAuthenticated can run its stale-token recovery.
Comment on lines
+169
to
+173
| connect: () => Promise.resolve(), | ||
| close: () => { | ||
| if (typeof Meteor.disconnect === 'function') Meteor.disconnect(); | ||
| stopBridge?.(); | ||
| }, |
Contributor
There was a problem hiding this comment.
Don't disconnect Meteor here unless the proxy can reconnect it.
close() tears down the shared Meteor connection, but connect() is a no-op. Any caller that closes and later reconnects this fallback DDPSDK will leave the whole client offline when Use_RC_SDK is disabled. Either keep both lifecycle methods as no-ops in the proxy, or have connect() restore the Meteor connection and reset the status bridge before reuse.
🤖 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 169 - 173, The
close() implementation currently calls Meteor.disconnect() while connect() is a
no-op, which breaks shared Meteor for callers that expect to reconnect; either
make both proxy methods no-ops or implement connect() to restore the Meteor
connection and restart the status bridge. Update the proxy's connect() and
close() (referencing connect, close, stopBridge, and Meteor.disconnect) so they
are symmetric: either remove Meteor.disconnect() and stopBridge?() from close()
so both methods are inert, or have connect() call Meteor.reconnect() (or the
correct Meteor connection restore) and reinitialize the status bridge (reset
stopBridge) so a subsequent connect() after close() returns the client to an
operational state.
Comment on lines
+19
to
+24
| const fromUrl = new URLSearchParams(window.location.search).get(KEY); | ||
| if (fromUrl === 'on') return true; | ||
| if (fromUrl === 'off') return false; | ||
| if (window.localStorage.getItem(`rc-config-${KEY}`) === 'on') return true; | ||
| const meta = window.document?.querySelector(`meta[name="${META_NAME}"]`); | ||
| return meta?.getAttribute('content') === 'on'; |
Contributor
There was a problem hiding this comment.
Honor persisted localStorage=off before falling back to the meta tag.
Right now only 'on' short-circuits. If the workspace-level meta enables the SDK globally, a user who explicitly stored 'off' still gets forced onto the SDK path, which breaks the documented URL > localStorage > meta precedence.
Suggested fix
const fromUrl = new URLSearchParams(window.location.search).get(KEY);
if (fromUrl === 'on') return true;
if (fromUrl === 'off') return false;
- if (window.localStorage.getItem(`rc-config-${KEY}`) === 'on') return true;
+ const fromLocalStorage = window.localStorage.getItem(`rc-config-${KEY}`);
+ if (fromLocalStorage === 'on') return true;
+ if (fromLocalStorage === 'off') return false;
const meta = window.document?.querySelector(`meta[name="${META_NAME}"]`);
return meta?.getAttribute('content') === 'on';📝 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.
Suggested change
| const fromUrl = new URLSearchParams(window.location.search).get(KEY); | |
| if (fromUrl === 'on') return true; | |
| if (fromUrl === 'off') return false; | |
| if (window.localStorage.getItem(`rc-config-${KEY}`) === 'on') return true; | |
| const meta = window.document?.querySelector(`meta[name="${META_NAME}"]`); | |
| return meta?.getAttribute('content') === 'on'; | |
| const fromUrl = new URLSearchParams(window.location.search).get(KEY); | |
| if (fromUrl === 'on') return true; | |
| if (fromUrl === 'off') return false; | |
| const fromLocalStorage = window.localStorage.getItem(`rc-config-${KEY}`); | |
| if (fromLocalStorage === 'on') return true; | |
| if (fromLocalStorage === 'off') return false; | |
| const meta = window.document?.querySelector(`meta[name="${META_NAME}"]`); | |
| return meta?.getAttribute('content') === 'on'; |
🤖 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/sdkTransportEnabled.ts` around lines 19 - 24, The
current logic only short-circuits on URL 'on' and treats a stored
localStorage='off' as if absent, so change the precedence to: first read URL
param (if 'on' return true; if 'off' return false), then read localStorage for
KEY and return true if 'on' or false if 'off' (i.e. honor explicit 'off' before
falling back), and only after those checks consult the meta tag (META_NAME).
Update the code that uses fromUrl,
window.localStorage.getItem(`rc-config-${KEY}`), and
meta?.getAttribute('content') to implement that exact URL > localStorage > meta
precedence.
Comment on lines
+73
to
80
| // Login itself is the call that establishes auth — running it through | ||
| // `method.call` would force the REST middleware to validate the very | ||
| // token we're trying to use, and the server would 401 with "You must | ||
| // be logged in" before even invoking the login method. The 401 then | ||
| // short-circuits the resume callback, leaving the stale token in | ||
| // localStorage and the user wedged on /home with no main UI. | ||
| const endpoint = !getUserId() || wasResumeLogin ? 'method.callAnon' : 'method.call'; | ||
|
|
Contributor
There was a problem hiding this comment.
Route every non-resume login through method.callAnon.
Line 79 still sends login to method.call whenever getUserId() is truthy. That reintroduces the stale-session/account-switch failure described above: REST auth validates the old token before the login handler runs, so password/SAML/OAuth login can 401 again instead of clearing the session. login should be anonymous regardless of the current client state.
Suggested fix
- const endpoint = !getUserId() || wasResumeLogin ? 'method.callAnon' : 'method.call';
+ const endpoint = message.method === 'login' || !getUserId() || wasResumeLogin ? 'method.callAnon' : 'method.call';🤖 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 73 - 80, The
endpoint selection currently uses getUserId() and wasResumeLogin to choose
between 'method.call' and 'method.callAnon'; change it so that any non-resume
login goes through 'method.callAnon' regardless of getUserId(): update the logic
that sets endpoint (the variable named endpoint) to check if methodName ===
'login' && !wasResumeLogin and force 'method.callAnon', otherwise keep the
existing branch between 'method.call' and 'method.callAnon' based on getUserId()
and wasResumeLogin.
Comment on lines
+89
to
+90
| const TrackerDependency = (Tracker as unknown as { Dependency?: new () => { changed(): void } }).Dependency; | ||
| const statusListeners = TrackerDependency ? new TrackerDependency() : undefined; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n apps/meteor/client/meteor/overrides/stubMeteorStream.ts | head -140Repository: RocketChat/Rocket.Chat
Length of output: 5808
🏁 Script executed:
rg "Tracker.Dependency|depend\(\)|\.changed\(" apps/meteor/client/meteor/overrides/stubMeteorStream.ts -B 2 -A 2Repository: RocketChat/Rocket.Chat
Length of output: 48
🌐 Web query:
Meteor Tracker.Dependency API documentation depend changed
💡 Result:
Meteor Tracker.Dependency API documentation: the relevant API surface is Tracker.Dependency#depend([fromComputation]) and Tracker.Dependency#changed, and the docs consistently describe them the same way (no evidence found of a documentation “depend changed” breaking change in the API itself). 1) Tracker.Dependency#depend([fromComputation]) - Declares that the current reactive computation (or the computation passed via fromComputation) depends on the Dependency. The computation will be invalidated the next time the dependency changes. [1][2] - If there is no current computation and depend is called with no arguments, it does nothing and returns false. [1] - Returns true when the computation is a new dependent rather than an existing one. [1] 2) Tracker.Dependency#changed - Invalidate all dependent computations immediately and remove them as dependents. [1][2] 3) Docs example (getter calls depend; setter calls changed) - The official Tracker docs show a pattern where a getter calls weatherDep.depend and the setter calls weatherDep.changed to trigger reactivity. [2][1] So if your question is “what changed in the docs for depend?”, the current official documentation for Tracker.Dependency continues to define depend as “declare dependency” and changed as “invalidate dependents”, with the same getter/setter example pattern. [2][1] If instead you meant “Deps.Dependency” (older name) vs “Tracker.Dependency” (newer name), tell me your Meteor version and what exact page/wording you saw that looked different, and I can trace that specific doc change.
Citations:
Fix Tracker.Dependency usage—status() should call depend(), not changed().
The type definition at line 89 is incomplete (missing depend(): void), and the implementation is backwards. The status() method should register reactivity with depend() so that computations reading the status are invalidated when it changes. Currently, every call to status() invalidates all dependents immediately while reactive callers never register a dependency.
Suggested fix
- const TrackerDependency = (Tracker as unknown as { Dependency?: new () => { changed(): void } }).Dependency;
+ const TrackerDependency = (Tracker as unknown as { Dependency?: new () => { depend(): void; changed(): void } }).Dependency;
const statusListeners = TrackerDependency ? new TrackerDependency() : undefined;
…
status() {
- statusListeners?.changed?.();
+ statusListeners?.depend?.();
return this.currentStatus;
},
statusChanged() {
statusListeners?.changed?.();Also applies to: 124–130
🤖 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/stubMeteorStream.ts` around lines 89 -
90, The Tracker.Dependency type and usage are inverted: extend the inline type
for TrackerDependency to include depend(): void (not just changed()), update
status() to call statusListeners.depend() so callers register a reactive
dependency, and ensure wherever you signal updates (the code that mutates
status) calls statusListeners.changed() to invalidate dependents; apply the same
fix to the other occurrence around the 124–130 block (use the same
TrackerDependency, call depend() in readers like status(), and call changed()
when the status value is updated).
Comment on lines
+17
to
+21
| type SubscribeCallbacks = { | ||
| onReady?: () => void; | ||
| onError?: (err: Error) => void; | ||
| onStop?: (err?: Error) => void; | ||
| }; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
python - <<'PY'
from pathlib import Path
import re
pattern = re.compile(
r'Meteor(?:\.connection)?\.subscribe\s*\((?:(?!\);).)*?\{(?:(?!\}).)*?\bonStop\s*:',
re.S,
)
for path in Path('.').rglob('*'):
if path.suffix not in {'.ts', '.tsx', '.js'}:
continue
try:
text = path.read_text()
except Exception:
continue
for match in pattern.finditer(text):
line = text.count('\n', 0, match.start()) + 1
print(f'{path}:{line}')
PYRepository: RocketChat/Rocket.Chat
Length of output: 48
🏁 Script executed:
cat -n apps/meteor/client/meteor/overrides/subscribeViaSDK.tsRepository: RocketChat/Rocket.Chat
Length of output: 2574
🏁 Script executed:
rg "onStop" --type ts --type tsx --type js -B 2 -A 2 | head -100Repository: RocketChat/Rocket.Chat
Length of output: 94
🏁 Script executed:
rg "onStop" -B 2 -A 2 | head -150Repository: RocketChat/Rocket.Chat
Length of output: 9486
Propagate failed subscriptions through onStop(err) to preserve Meteor compatibility.
The rejection path only calls onError, but Meteor reserves onStop(err) for server-initiated subscription failures. The adapter's promise rejection should invoke onStop with the error so callers relying on this path receive SDK-side failures.
Suggested fix
subscription
.ready()
.then(() => callbacks.onReady?.())
- .catch((err: Error) => callbacks.onError?.(err));
+ .catch((err: Error) => {
+ callbacks.onError?.(err);
+ callbacks.onStop?.(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 rejection path currently only calls onError but must also propagate SDK
subscription failures via onStop(err) to match Meteor semantics; update the
subscribeViaSDK promise rejection handler (the code using SubscribeCallbacks and
its onError/onStop callbacks) to call onStop(err) with the error when a
subscription fails (in addition to or instead of onError as appropriate),
ensuring the error is passed through to callers expecting server-initiated
failure via onStop.
Comment on lines
+80
to
+98
| let lastSyncedUid: string | undefined; | ||
| const syncOnce = (uid: string | undefined): void => { | ||
| // Reset on logout transitions so a subsequent re-login (same uid or different) | ||
| // runs a fresh sync. Force-logout via the SDK loginWithToken wrap clears | ||
| // creds via Accounts._unstoreLoginToken() + setUserId(null), which does NOT | ||
| // fire Accounts.onLogout — so without this branch, lastSyncedUid stays set, | ||
| // the next login is deduped, runUserDataSync is skipped, and | ||
| // useUserDataSyncReady stays false (page wedged on PageLoading). | ||
| if (!uid) { | ||
| lastSyncedUid = undefined; | ||
| return; | ||
| } | ||
| if (uid === lastSyncedUid) return; | ||
| lastSyncedUid = uid; | ||
| void runUserDataSync(uid).catch((err) => { | ||
| console.warn('[startup] runUserDataSync failed; clearing dedup to allow a retry', err); | ||
| if (lastSyncedUid === uid) lastSyncedUid = undefined; | ||
| }); | ||
| }; |
Contributor
There was a problem hiding this comment.
lastSyncedUid can swallow the only retry for a login.
syncOnce() marks the uid as handled before runUserDataSync() settles, and this file can feed it from three places (onLoggedIn, userIdStore.subscribe, and the eager boot-time call). If the first attempt is still in flight, the later signals get deduped; then a failure at Lines 94-97 only clears the guard and never replays the consumed event. That still leaves SDK-mode re-logins vulnerable to getting stuck on the failed first sync.
Please track “in flight” separately from “successfully synced”, or queue a pending retry and replay it in finally when userIdStore.getState() === uid.
🤖 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 80 - 98, syncOnce
currently sets lastSyncedUid before runUserDataSync completes which causes
in-flight attempts to be deduped and lost on failure; change syncOnce to track
in-flight vs completed separately (e.g. introduce an inFlightUid or a Set
inFlightUids) and only set lastSyncedUid after runUserDataSync resolves
successfully, or alternatively record a pendingRetry flag and in the
runUserDataSync finally block check if userIdStore.getState() === uid and replay
the sync if needed; update references in syncOnce, lastSyncedUid,
runUserDataSync error/finally handling and any callers (onLoggedIn,
userIdStore.subscribe, boot-time call) so retries are not swallowed.
Comment on lines
+139
to
+148
| const before = await this.messageListItems.count(); | ||
|
|
||
| await this.composer.btnSend.click(); | ||
|
|
||
| // Use `>=` rather than `==` because some flows (e.g. just-created | ||
| // encrypted channels) drop additional list items in alongside the | ||
| // user's send (other in-flight messages, decryption-status items), | ||
| // so an exact count is racy. | ||
| await expect.poll(() => this.messageListItems.count(), { timeout: 10_000 }).toBeGreaterThanOrEqual(before + 1); | ||
| await expect(this.lastUserMessage).not.toHaveClass(/rcx-message--pending/); |
Contributor
There was a problem hiding this comment.
Wait on the submitted message, not just the new tail item.
messageListItems.count() plus lastUserMessage can succeed on an unrelated row when other messages land at the same time. In that case this helper returns even though the message you just sent is still pending or failed. Anchor the final assertion to the submitted message text, or to the exact row created after before, instead of the current list tail.
Possible adjustment
const before = await this.messageListItems.count();
await this.composer.btnSend.click();
await expect.poll(() => this.messageListItems.count(), { timeout: 10_000 }).toBeGreaterThanOrEqual(before + 1);
- await expect(this.lastUserMessage).not.toHaveClass(/rcx-message--pending/);
+ await expect(this.getMessageByText(text).last()).not.toHaveClass(/rcx-message--pending/);🤖 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/tests/e2e/page-objects/fragments/home-content.ts` around lines
139 - 148, The current wait uses messageListItems.count() and lastUserMessage
which can track the wrong tail item if other messages arrive; change the final
wait to target the specific message you sent by capturing the message text (or
the exact item index created: messageListItems.nth(before)) and poll/assert that
that specific element has left the pending state and contains the sent text.
Concretely, after btnSend.click() store the sent text (or compute the newItem =
this.messageListItems.nth(before)), then replace the count-based poll and
lastUserMessage assertion with a poll/expect against newItem (or the element
located by the sent text) toBeVisible/ not.toHaveClass(/rcx-message--pending/)
and toContainText(sentText).
19 tasks
d-gubert
changed the title
d-gubert
changed the title
…llers Previous fix collapsed `connecting` and `connected` into the same `Promise.resolve(true)` short-circuit. That silenced the unhandled rejection from the retry timer but, for the `connecting` half, replaced one lie with another: a second caller that arrived mid-handshake observed `true` even when the same handshake later received a `failed` payload (handshake's owning promise rejected, but the second caller saw success). Track the in-flight handshake on `this.connectPromise` and return it from both `connect()` and `reconnect()` while `status === 'connecting'`. The `connected` branch is unchanged. The promise is published before any status emit so a synchronous re-entrant call still observes a valid promise to share. Tests: a concurrent `connect()` call gets the same instance and surfaces the eventual `failed`; same for a concurrent `reconnect()` reaching success.
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
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.
Note
This is an intermediate step towards completely removing the use of the meteor client (auth, oauth, ddp, tracker, etc.), all this code must be removed at once after we have migrated everything to the SDK, so even if not everything is perfect or makes complete sense, it should only last a short time. That's why it's under a flag, if the original behavior is affected we must revert everything and adjust before reinserting
Summary
Migrates the frontend DDP transport from Meteor's WebSocket to our own
@rocket.chat/ddp-clientSDK, gated behind a runtime flag for stagedrollout. The migration ships dormant by default — legacy Meteor DDP
stays the active transport — and can be opted into per-tab, per-user,
or per-workspace. Meteor Accounts stays as the auth anchor.
Runtime flag —
Use_RC_SDKThree sources resolve the flag in order; default
offif none match:?sdk_transport=on|off— per-tab override.rc-config-sdk_transportin localStorage — persisted per-user.<meta name="rc-sdk-transport-enabled">injected from the newUse_RC_SDKadmin setting (General → Use Rocket.Chat SDK) — globalopt-in / kill-switch.
apps/meteor/client/lib/sdk/sdkTransportEnabled.tsis the singlesource of truth; the rest of the codebase reads it once at module init
and dispatches accordingly.
What the flag gates
When the flag is off,
getDdpSdk()returns a Meteor-backedpass-through proxy (
apps/meteor/client/lib/sdk/meteorBackedSdk.ts)with the same
DDPSDKshape —client.subscribe↦Meteor.connection.subscribe,client.callAsync↦Meteor.callAsync,account.uid↦Meteor.userId(),connection.statusderived fromMeteor.status()via Tracker autorun.Consumers (
ServerProvider,presence.ts,SDKClient, the overridemodules) keep calling
getDdpSdk()unconditionally and behave likedevelop without any per-call-site flag checks. No second WebSocket is
opened, no auth lifecycle runs, no Presence session is duplicated
server-side.
When the flag is on,
getDdpSdk()returns the realDDPSDKinstance and the five transport overrides install:
ddpSdkCollectionBridge— re-feeds DDPSDK collection frames intoMeteor's
_streamHandlers.onMessage.subscribeViaSDK— routesMeteor.connection.subscribethroughsdk.client.subscribe.stubMeteorStream— replacesMeteor.connection._streamwith astub that forwards outbound frames through the SDK socket.
killMeteorStream— drains revival/quiescence bookkeeping andoutstanding-method blocks on logout.
ddpOverREST— wrapsMeteor.connection._sendso non-bypassedmethods route via REST. Always on (matches develop's behavior),
not gated by the flag — the resume-only
loginhandler inddp-streamer-service403s every other login shape, and routing viaREST is the only way the form login flow works in microservices.
Other always-on changes
These are server-side or package-internal fixes that apply regardless
of the flag:
ee/apps/ddp-streamer/src/DDPStreamer.ts—ws.close()(graceful)on
user.forceLogoutinstead ofws.terminate(), with a 5s fallbackto terminate. Lets the
notify-user/<uid>/force_logoutstreammessage reach the client before the socket goes down.
packages/ddp-client/src/Connection.ts— idempotentconnect()/reconnect(),retryCountreset on successful connection,stale
ws.oncloseignored when a newer ws has taken over, retrytimer re-checks status before firing.
packages/ddp-client/src/DDPDispatcher.ts— non-method framesbypass the wait queue so
connect/sub/pingaren't queued behinda wait
loginblock.Boot-time flag-gated bits
ddpSdk.tsside-effect block (loginWithToken auth-error wrap,userIdStore subscriber,
ensureConnectedAndAuthenticatedboot path).app/notifications/client/lib/Presence.ts— only registers theuser-presence streamer on the SDK socket when the flag is on. With
flag off, only
Meteor.connection's streamer runs.loggedIn.ts—whenLoggedIn/onLoggedInuse bothAccounts.onLoginand auserIdStorebridge when flag on (REST-routed login can't fill
Meteor.users); with flag off they matchdevelop's
Accounts.onLogin-only behavior.useForceLogout.ts— only clears local credentials manually whenflag on; with flag off the legacy WS-reconnect-then-relogin chain
handles cleanup.
startup.ts—runUserDataSyncawaits SDK auth beforesynchronizeUserDataonly when flag on.Test plan
account-manage-devices, admin-room, account-profile all pass on
monolith.
(bit-for-bit develop behavior).
should be open after login — Meteor's stays present as the
Accounts anchor, DDPSDK carries every method/subscription.
login still work end-to-end. Session resume on reload goes
straight into the app.
in near-realtime.
disable the DDPSDK socket via DevTools and verify the offline
banner appears.
no SDK socket opens, no proxy WebSocket activity.
Task: ARCH-2127
Summary by CodeRabbit
New Features
Bug Fixes
Tests