chore: ddp client#6800
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughReplace legacy ChangesMain migration cohort
Sequence DiagramsequenceDiagram
participant Client
participant Sdk
participant DDPSDK
participant Server
Client->>Sdk: initialize(), login(), rest calls
Sdk->>Server: REST /v1/* (sdk.get / sdk.post / sdk.delete)
Sdk->>DDPSDK: probe(), forceReopen(), subscribeRoom()
DDPSDK->>Server: DDP ping/subscribe / collect events (media-signal, media-calls, video-conference)
Server->>Sdk: REST responses and DDP events
Sdk->>Client: onCollection / event callbacks / normalized errors
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
|
|
🟡 Latent breakage from the SDK migration:
Since it's a |
Follow-up review notes — DDP migrationI re-verified every open review thread against the current code. Most are either already fixed or false positives (including two bot "Criticals"). These four are the ones still worth acting on. Severity in brackets; items 1–2 have real user-facing impact, 3–4 are minor. 1. 🟡 2FA is broken for REST method calls —
|
Prerequisite for the deepLinking.js fixes: the mock now exposes `server` (the Sdk wrapper getter) instead of the removed `current.client.host` property from the old ddp-client. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
|
iOS Build Available Rocket.Chat 4.74.0.109030 |
|
Android Build Available Rocket.Chat 4.74.0.109029 Internal App Sharing: https://play.google.com/apps/test/RQQ8k09hlnQ/ahAO29uNTFOXo30wAEStyHFIiU7fa1ibAOmtnX_bRPqae4S89dq9AVtxTf2PHKBURT-6MvkF_1FZ3ak76VHdVQOZmI |
| ...(token ? { 'X-Auth-Token': token } : {}), | ||
| ...(id ? { 'X-User-Id': id } : {}) |
| ...(token ? { 'X-Auth-Token': token } : {}), | ||
| ...(id ? { 'X-User-Id': id } : {}) |
There was a problem hiding this comment.
These mock values were added because, without them, the test snapshot displayed unknown values in the header's User-Agent.
Before: "RC Mobile; ios unknown; vunknown (unknown)"
After: "RC Mobile; ios 14.0; v1.0.0 (1)"
…t.Chat.ReactNative into feat-ddp-client
…t.Chat.ReactNative into feat-ddp-client
| } finally { | ||
| try { | ||
| tempSdk?.connection.close(); | ||
| } catch (e) { | ||
| log(e); | ||
| } | ||
| } |
There was a problem hiding this comment.
Added tempSdk?.connection.close() in a finally block to ensure the connection is always cleaned up, even on errors.
Proposed changes
Migrate from legacy
@rocket.chat/sdkto@rocket.chat/ddp-client, standardize REST API paths to explicit/v1/prefixes, centralize header management via SDK, fix critical subscription lifecycle race conditions, and improve error handling across subscription and authentication flows.Issue(s)
Related: https://rocketchat.atlassian.net/browse/CORE-1579
How to test or reproduce
1. Connection lifecycle
2. Authentication
3. Real-time messages & room subscriptions
4. File operations & headers
AuthorizationandUser-AgentheadersUser-Agentare present, and the file renders correctly in the message list5. Omnichannel (requires an agent account)
6. Autocomplete
/previewin the message composer → a loading indicator appears; it clears on both success and failure (not stuck indefinitely)/nonexistent-command→ loading indicator still clears (error path)@→ user suggestion list appears; selecting a user inserts the mention and clears the list7. REST API path correctness
8. End-to-end encryption
9. Video conferencing
10. Error / edge-case handling
Screenshots
Types of changes
Checklist
Further comments
Sharing some context on why we are still using the API typings from the app codebase instead of the typings provided by the SDK and
rest-typings.In the main Rocket.Chat repository, the team is currently migrating APIs to OpenAI compatibility format. The related PR removes API typings from the
rest-typingspackage, which causes type errors when upgrading the package version in this app.To avoid blocking this DDP client migration on an upstream change that is still in progress, we are keeping the local type definitions in
app/definitions/rest/v1/for the affected endpoints. Thepatches/@rocket.chat+rest-typings+8.4.1.patchadds only the/v1/push.token(POST and DELETE) endpoint that is missing from the currently pinned package version. Once the main repo completes its API compatibility migration and publishes updated typings, the local copies can be removed and the package upgraded cleanly.Related PR: RocketChat/Rocket.Chat#40774
Summary by CodeRabbit
New Features
Bug Fixes
Improvements