Skip to content

chore: ddp client#6800

Open
Rohit3523 wants to merge 145 commits into
developfrom
feat-ddp-client
Open

chore: ddp client#6800
Rohit3523 wants to merge 145 commits into
developfrom
feat-ddp-client

Conversation

@Rohit3523

@Rohit3523 Rohit3523 commented Nov 14, 2025

Copy link
Copy Markdown
Member

Proposed changes

Migrate from legacy @rocket.chat/sdk to @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

  • Cold start → verify the app connects to the server and all room subscriptions load
  • Background the app for 30 s → foreground it → verify the WebSocket reconnects and no messages were missed (check room badge counts match)
  • Toggle airplane mode while in a room → turn it off → verify the app reconnects and queued messages deliver without a manual refresh
  • Switch servers (Settings → Add server or switch workspace) → verify subscriptions for the old server are torn down and the new server's rooms load cleanly

2. Authentication

  • Login with username + password → full login flow completes, rooms load
  • Login with a 2FA-enabled account → 2FA challenge appears, token accepted, rooms load
  • Logout → verify the DDP session is closed, Redux state is cleared, and you land on the login screen with no leftover subscriptions (check network tab: no lingering WebSocket frames)
  • Kill and reopen the app while logged in → auto-resume from stored token, no re-login prompt

3. Real-time messages & room subscriptions

  • Send a message from a web browser → verify it appears in real-time on the device
  • Receive a message in a room you're not viewing → verify unread badge increments on the room list
  • Reply in a thread from another client → verify the thread count updates on the parent message

4. File operations & headers

  • Upload a user avatar (Profile → Edit → Avatar) → open a network inspector (e.g., Proxyman / Charles) and confirm the request includes Authorization and User-Agent headers
  • Send an image/file in a chat room → confirm auth headers and User-Agent are present, and the file renders correctly in the message list
  • Cancel an in-progress file upload → verify no zombie upload appears in the room

5. Omnichannel (requires an agent account)

  • Log in as an omnichannel agent → department queue updates are received and displayed in real-time
  • Switch to another workspace and back → queue subscriptions re-establish without a manual refresh
  • Logout then log back in → queue updates resume correctly (this was the critical regression fixed in this PR)

6. Autocomplete

  • Type /preview in the message composer → a loading indicator appears; it clears on both success and failure (not stuck indefinitely)
  • Type /nonexistent-command → loading indicator still clears (error path)
  • Type @ → user suggestion list appears; selecting a user inserts the mention and clears the list

7. REST API path correctness

  • Perform several operations (send message, mark room as read, search for a user, invite to channel) → open network inspector and verify all REST calls return 2xx, no unexpected 404s
  • Confirm channels, groups, and DMs all load their message history

8. End-to-end encryption

  • Open a room with E2E encryption enabled → send a message → verify it is shown as encrypted in transit and decrypted on both ends
  • Log out and back in → verify E2E keys are re-established and old encrypted messages still decrypt

9. Video conferencing

  • Start a Jitsi call from a room → call initiates and the join button appears for other participants
  • Join an active call via the video conference block in the message list

10. Error / edge-case handling

  • Simulate a malformed server response (e.g., temporarily point to a non-RC server) → app should not crash; it should show a connection error
  • Kill the server mid-upload → verify the app surfaces an error and does not silently swallow it

Screenshots

Types of changes

  • Bugfix (non-breaking change which fixes an issue)
  • Improvement (non-breaking change which improves a current function)
  • New feature (non-breaking change which adds functionality)
  • Documentation update (if none of the other choices apply)

Checklist

  • I have read the CONTRIBUTING doc
  • I have signed the CLA
  • Lint and unit tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works (if applicable)
  • I have added necessary documentation (if applicable)
  • Any dependent changes have been merged and published in downstream modules

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-typings package, 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. The patches/@rocket.chat+rest-typings+8.4.1.patch adds 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

    • Added v1 REST contracts for auth/login, chat send/sync, commands.list, push token DELETE, teams delete/leave, groups.open, users.presence, and related endpoints.
  • Bug Fixes

    • Fixed null-safety and stuck-loading issues in autocomplete, key distribution, thread loading, call initiation, message sync/read, avatar/header handling, and related UI flows.
  • Improvements

    • Moved many API calls to versioned /v1 routes, improved connection probing/reconnect and media subscription reliability, standardized upload/auth headers, and expanded test coverage.

@coderabbitai

coderabbitai Bot commented Nov 14, 2025

Copy link
Copy Markdown
Contributor

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

Walkthrough

Replace legacy @rocket.chat/sdk with a DDPSDK-backed Sdk, migrate REST calls to /v1, centralize request headers via sdk.getHeaders(), add connection liveness/reopen APIs, update subscriptions and typings, adjust uploads/avatar headers, and expand/update tests.

Changes

Main migration cohort

Layer / File(s) Summary
SDK wrapper, REST /v1 migrations, headers, and tests
app/lib/services/sdk.ts, app/lib/services/restApi.ts, app/lib/methods/helpers/*, app/lib/methods/*upload*, app/lib/services/connect.ts, app/lib/methods/subscriptions/*, app/definitions/rest/v1/*, patches/*, app/lib/services/sdk.test.ts, app/lib/services/connect.test.ts
Implements DDPSDK-backed Sdk (initialize, probe, forceReopen, disconnect, headers, rest.get/post/delete, methodCallWrapper, twoFactor handling), migrates many endpoints to /v1/*, centralizes default headers and fetch wrapper, updates upload/avatar header construction to use sdk.getHeaders(), rewrites connect lifecycle and subscription wiring to use new Sdk APIs, updates REST v1 typings and patches, and adds/updates unit tests.

Sequence Diagram

sequenceDiagram
  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
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Suggested reviewers

  • diegolmello
  • OtavioStasiak

@diegolmello

Copy link
Copy Markdown
Member

🟡 Latent breakage from the SDK migration: sdk.current?.client?.host is undefined on @rocket.chat/ddp-client — flagging as a follow-up since it's outside this PR's diff.

app/sagas/deepLinking.js isn't part of this PR's diff (its client?.host lines came from #7358, already on develop), but the migration here makes them silently break — the same gap getSettings.ts was fixed for in this PR. The new ClientStream has no host property (verified in the dist: only connection.url, which is protocol-stripped), so sdk.current?.client?.host is always undefined:

  • deepLinking.js:229 (handleOpen): const hostAlreadyConnected = sdk.current?.client?.host === host; is always false, so the "already connected → skip reconnect" fast-path from fix: Deeplink auth when host is already connected #7358 never triggers; a deeplink-with-token always runs the full ROOT_OUTSIDE → serverInitAdd → NewServer → take(SERVER.SELECT_SUCCESS) path. Safe, but the optimization is dead.
  • deepLinking.js:155 (handleShareExtension): if (sdk.current?.client?.host !== server) is always true, so it always runs yield take(types.LOGIN.SUCCESS). If selectServerRequest short-circuits in the already-connected case (the selectServer saga early-returns via sdk.server === server without emitting LOGIN.SUCCESS), that take can block indefinitely on the share splash.

Since it's a .js file, tsc didn't catch the missing property. Suggested fix at both sites: use the wrapper getter sdk.server (full server URL) — sdk.server === host and sdk.server !== server — matching the comparison the selectServer saga already uses. Worth a quick grep for any other client.host / client?.host survivors before merge.

@diegolmello

Copy link
Copy Markdown
Member

Follow-up review notes — DDP migration

I 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 — app/lib/services/sdk.ts (post() retry, ~line 197)

What's wrong: With API_Use_REST_For_DDP_Calls enabled, a 2FA-protected call hits /v1/method.call, which returns HTTP 200 with the error nested in result.message. The api-client's handleTwoFactorChallenge only fires on HTTP 400 totp errors, so it never runs for this shape — post()'s own catch handles it. But the retry return resolve(this.post(endpoint, params)) re-sends the same params with no code, and twoFactor.ts no longer sets the x-2fa-code / x-2fa-method headers. So the user's code is collected and then dropped.

ELI5: The app asks you for your 2FA code, throws it in the trash, and re-sends the same request without it. The server keeps asking "code?" — forever (or it resolves empty if you cancel).

Suggested fix: Carry the code on the retry, the way methodCall() already does with this.code. Set the headers the api-client itself uses for its 400-path retry before re-posting:

const { twoFactorCode, twoFactorMethod } = await twoFactor({
    method: details?.method,
    invalid: errorType === totpInvalid
});
// restore what twoFactor.ts used to set via RocketchatSettings.customHeaders
this.setHeaders({ 'x-2fa-code': twoFactorCode, 'x-2fa-method': twoFactorMethod });
return resolve(this.post(endpoint, params));

(Scope the headers to the retry / clear them afterward so they don't leak onto later requests, since this.headers persists.)


2. 🟡 Dead client.host guards in deep linking — app/sagas/deepLinking.js:155 & :229

What's wrong: sdk.current?.client?.host is always undefined on @rocket.chat/ddp-client — the ClientStream has no host (only connection.url). This PR migrated the same access in getSettings.ts to the Redux server, but these two .js sites were missed (tsc can't catch it in a .js file).

  • Line 229hostAlreadyConnected is always false, so the fix: Deeplink auth when host is already connected #7358 "skip reconnect" optimization never triggers. Safe, just dead.
  • Line 155sdk.current?.client?.host !== server is always true, so it always runs yield take(types.LOGIN.SUCCESS). When selectServer short-circuits the already-connected case (sdk.server === server) without emitting LOGIN.SUCCESS, the share extension hangs on the splash.

ELI5: The code asks "am I already connected to this server?" using a property that no longer exists, so the answer is always "no." Usually harmless — but in the share sheet it can wait forever for a "logged in!" signal that never arrives.

Suggested fix: Use the wrapper getter sdk.server (full URL), matching the comparison selectServer.ts:140 already uses:

// line 229
const hostAlreadyConnected = sdk.server === host;

// line 155
if (sdk.server !== server) {
    yield take(types.LOGIN.SUCCESS);
}

deepLinking.js isn't in this PR's diff (it came from #7358), so this is a follow-up. Worth grepping for any other client.host survivors before merge — at last check these were the only two.


3. 🟢 Logout doesn't stop when push-token removal fails — app/lib/methods/logout.ts:77-80

What's wrong: A { success: false } response is now detected and logged, but the flow still proceeds to wipe the server data and log out — so a "removed" workspace can keep sending push notifications.

ELI5: You tell the server "stop pinging my phone," it says "no," and the app deletes the account anyway. Notifications keep coming.

Suggested fix: May be acceptable as best-effort. If not, handle the failure instead of silently continuing — abort the removal, or retry the token delete once before proceeding. Just confirm the intended behavior.


4. 🟢 Async-executor anti-pattern — app/lib/services/sdk.ts:258 (methodCall)

What's wrong: methodCall uses new Promise(async (resolve, reject) => { … }). Style only — all logic sits inside the try, so no rejection escapes the executor. Not a correctness bug.

ELI5: A slightly awkward way to write the function. Works fine, just not idiomatic.

Suggested fix: Optional. Refactor to a plain async method with try/catch + return/throw, dropping the new Promise wrapper.

Rohit3523 and others added 3 commits June 5, 2026 19:52
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>
@github-actions

github-actions Bot commented Jun 5, 2026

Copy link
Copy Markdown

iOS Build Available

Rocket.Chat 4.74.0.109030

@github-actions

github-actions Bot commented Jun 5, 2026

Copy link
Copy Markdown

Comment on lines +89 to +90
...(token ? { 'X-Auth-Token': token } : {}),
...(id ? { 'X-User-Id': id } : {})

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Not needed

Comment on lines +15 to +16
...(token ? { 'X-Auth-Token': token } : {}),
...(id ? { 'X-User-Id': id } : {})

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

same

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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)"

Comment thread app/lib/methods/logout.ts
Comment on lines +86 to 92
} finally {
try {
tempSdk?.connection.close();
} catch (e) {
log(e);
}
}

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Added tempSdk?.connection.close() in a finally block to ensure the connection is always cleaned up, even on errors.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants