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:
WalkthroughThe PR migrates the mobile client from the legacy SDK toward DDPSDK, standardizes REST requests and contracts on ChangesDDPSDK and REST v1 migration
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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 |
GitHub Actions
Error
GitHub Actions
Error
GitHub Actions
Error
GitHub Actions
Error
GitHub Actions
Error
GitHub Actions
Error
GitHub Actions
Error
GitHub Actions
Failure
GitHub Actions
Error
GitHub Actions
Error
GitHub Actions
Error
GitHub Actions
Failure
GitHub Actions
Error
|
Android Build Available Rocket.Chat 4.75.0.109357 Internal App Sharing: https://play.google.com/apps/test/RQQ8k09hlnQ/ahAO29uNSbexYtXkh9ju0JWAsm-kGYCdkUAO8AHULa9s326TiPYbRlXLTCmsn901C5cBgzSq_v2OEyHVCmTsxTVETK |
|
iOS Build Available Rocket.Chat 4.75.0.109358 |
# Conflicts: # app/lib/methods/sendFileMessage/sendFileMessage.ts
|
Android Build Available Rocket.Chat 4.75.0.109383 Internal App Sharing: https://play.google.com/apps/test/RQQ8k09hlnQ/ahAO29uNTLcg7dgN1Y80PNIyDebCCql2Kkhs9SoiASeBviSK36P3nUsUahK3Hw_PDQPt1L-JfuZl8ksYE0gWsFgo9j |
|
iOS Build Available Rocket.Chat 4.75.0.109384 |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
app/sagas/videoConf.ts (1)
182-196: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
videoConfResponse.successstill doesn't exist on the response type — and the failure branch was removed.Per earlier review (unresolved),
/v1/video-conference.startreturns{ data: ... }with nosuccessfield, so this condition is always falsy and the success path never runs. Previously there was at least a failure branch that resetsetCalling/hid the action sheet; that branch has now been removed entirely, so on this (always-taken) falsy path nothing happens — no state reset, no action-sheet dismissal, no error shown. The call UI can get stuck indefinitely with no feedback, since this isn't an exception path that the surroundingcatchwould handle. This directly matches the maintainer's unaddressed question about the removed logic.🐛 Suggested fix
- if (videoConfResponse.success) { + if (videoConfResponse.data) { ... + } else { + yield put(setCalling(false)); + yield call(hideActionSheetRef); + showErrorAlert(i18n.t('error-init-video-conf')); }🤖 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 `@app/sagas/videoConf.ts` around lines 182 - 196, Update the response handling in the video conference saga around videoConfResponse so it does not depend on the nonexistent success field; treat the presence of the returned data as the success condition. Restore an explicit failure path that resets setCalling, hides the action sheet via hideActionSheetRef, and shows the existing initialization error when the response lacks valid data, while preserving the direct-call and join behavior for successful responses.app/sagas/deepLinking.js (1)
162-164: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDo not treat a matching server URL as an active connection.
sdk.servercan already equalserverbefore DDP connects. In that state this skipsLOGIN.SUCCESSand opens the share-extension flow without an authenticated connection. Require authenticated/connected state, asselectServernow does, before skipping the wait.🤖 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 `@app/sagas/deepLinking.js` around lines 162 - 164, Update the deep-linking wait condition around sdk.server so a matching server URL only skips LOGIN.SUCCESS when the SDK is actually authenticated and connected, reusing the same state criteria established by selectServer; otherwise continue waiting for types.LOGIN.SUCCESS.app/lib/methods/sendFileMessage/sendFileMessage.ts (1)
59-62: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRestore session credentials for same-origin authenticated requests.
DDPSDK.sdk.getHeaders()does not supplyX-Auth-TokenorX-User-Id. The upload now sends unauthenticated requests, while these tests mock an impossible header shape and therefore mask the regression. Source session credentials fromsdk.currentLoginfor same-origin requests, and retain stripping for cross-origin URLs.
app/lib/methods/sendFileMessage/sendFileMessage.ts#L59-L62: merge same-origin session credentials into the upload headers.app/lib/methods/helpers/getAuthHeaders.test.ts#L2-L7: mocksdk.currentLoginseparately from default SDK headers.app/lib/methods/helpers/fetch.test.ts#L70-L103: model default headers and session credentials separately in same-origin and cross-origin tests.🤖 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 `@app/lib/methods/sendFileMessage/sendFileMessage.ts` around lines 59 - 62, The upload headers in sendFileMessage must merge X-Auth-Token and X-User-Id from sdk.currentLogin for same-origin requests while preserving credential stripping for cross-origin URLs. Update app/lib/methods/sendFileMessage/sendFileMessage.ts lines 59-62 accordingly; in app/lib/methods/helpers/getAuthHeaders.test.ts lines 2-7 mock currentLogin separately from default SDK headers; and in app/lib/methods/helpers/fetch.test.ts lines 70-103 model those sources separately in both same-origin and cross-origin cases.app/views/ThreadMessagesView/index.tsx (1)
347-361: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winFailure path permanently disables pagination — echoes the unresolved reviewer question.
Setting
end: truein both the newelsebranch (unsuccessfulresult.success) and the existingcatchblock means any transient failure (timeout, flaky network) permanently blocks further loads: the guardif (end || loading || !this.mounted) return;at the top ofloadwill short-circuit forever, since nothing ever resetsendback tofalse. This mirrors the still-unansweredwhy?raised bydiegolmelloon these exact lines in a previous review.Consider only resetting
loadingon failure (letting a futureonEndReachedretry) or adding an explicit retry/backoff instead of a permanentend: true.🐛 Possible fix
} else { - this.setState({ loading: false, end: true }); + // Don't permanently stop pagination on a failed request; allow retry on next onEndReached. + this.setState({ loading: false }); } } catch (e) { log(e); - this.setState({ loading: false, end: true }); + this.setState({ loading: 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 `@app/views/ThreadMessagesView/index.tsx` around lines 347 - 361, Update the failure paths in the load method so unsuccessful results and caught errors clear loading without permanently setting end to true, allowing a later onEndReached call to retry. Preserve end updates for successful pagination responses and the existing guard behavior.app/lib/methods/getSettings.ts (1)
165-179: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winIntentional
AbortSignalcancellations are logged as errors.Now that
getSettingsaccepts a cancellationsignal, an abortedfetchthrows anAbortErrorthat falls into the samecatch (e) { log(e); }as genuine failures. Iflogreports to crash/error tracking, intentional cancellations (unmount, server switch) will show up as noise alongside real failures.🛡️ Possible fix
} catch (e) { + if ((e as Error)?.name === 'AbortError') { + return; + } log(e); }🤖 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 `@app/lib/methods/getSettings.ts` around lines 165 - 179, Update the error handling in getSettings so AbortError exceptions caused by the provided cancellation signal are ignored, while genuine fetch or parsing failures continue through the existing log(e) path. Use the existing signal and catch block to distinguish intentional aborts without changing successful pagination behavior.app/ee/omnichannel/lib/index.test.ts (1)
158-183: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winStop the previous inquiry subscription before creating a replacement.
subscribeInquiry()replacesthis.inquirySubwith the new subscription, but the old stop handle is dropped unless the app dispatchesINQUIRY_UNSUBSCRIBEbetween subscribe events. This test exercises that scenario and now expects the stale subscription’sfirstStopto remain silent, leaving it orphaned indefinitely. Callthis.inquirySub?.stop()before assigning the new subscription.🤖 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 `@app/ee/omnichannel/lib/index.test.ts` around lines 158 - 183, Update the INQUIRY_SUBSCRIBE handling in subscribeInquiry to call this.inquirySub?.stop() before creating and assigning the replacement subscription. Preserve the existing behavior for the initial subscription and ensure each subsequent subscribe event stops the previous handle before mockSubscribeInquiry runs.
🤖 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 `@app/lib/methods/helpers/getAuthHeaders.test.ts`:
- Around line 13-45: Explicitly annotate all affected callbacks: use void
returns for describe, it, test, and beforeEach callbacks in
app/lib/methods/helpers/getAuthHeaders.test.ts (anchor, lines 13-45) and
app/lib/methods/helpers/isSameOrigin.test.ts (lines 3-32); use Promise<void> for
async test callbacks and void for synchronous callbacks in
app/lib/methods/helpers/fetch.test.ts (lines 70-119). In
app/sagas/selectServer.ts, annotate the state parameter for the selector
callbacks at lines 141 and 191 with the appropriate state type.
In `@app/lib/methods/helpers/isSameOrigin.test.ts`:
- Around line 21-23: Correct the test description in the “origin is undefined”
case to state that it returns false, matching the existing isSameOrigin
assertion; leave the test logic unchanged.
In `@app/lib/services/connect.ts`:
- Around line 45-47: Update the same-server early return in connect() to require
successful SDK initialization, not merely sdk.server equality. Track or reuse
the finalized initialization state, such as sdk.current?.connection.status, so
retries after loadBasicAuth() or handleTwoFactorChallenge() failures perform
disconnect and reinitialization.
In `@app/lib/services/sdk.ts`:
- Around line 291-296: Update callMethod to capture the result of
ensureInitialized() in a local variable and use that variable for the
client.callAsyncWithOptions invocation. Preserve the existing method, params,
and optional two-factor code arguments while avoiding direct access to the
potentially undefined this.current property.
---
Outside diff comments:
In `@app/ee/omnichannel/lib/index.test.ts`:
- Around line 158-183: Update the INQUIRY_SUBSCRIBE handling in subscribeInquiry
to call this.inquirySub?.stop() before creating and assigning the replacement
subscription. Preserve the existing behavior for the initial subscription and
ensure each subsequent subscribe event stops the previous handle before
mockSubscribeInquiry runs.
In `@app/lib/methods/getSettings.ts`:
- Around line 165-179: Update the error handling in getSettings so AbortError
exceptions caused by the provided cancellation signal are ignored, while genuine
fetch or parsing failures continue through the existing log(e) path. Use the
existing signal and catch block to distinguish intentional aborts without
changing successful pagination behavior.
In `@app/lib/methods/sendFileMessage/sendFileMessage.ts`:
- Around line 59-62: The upload headers in sendFileMessage must merge
X-Auth-Token and X-User-Id from sdk.currentLogin for same-origin requests while
preserving credential stripping for cross-origin URLs. Update
app/lib/methods/sendFileMessage/sendFileMessage.ts lines 59-62 accordingly; in
app/lib/methods/helpers/getAuthHeaders.test.ts lines 2-7 mock currentLogin
separately from default SDK headers; and in
app/lib/methods/helpers/fetch.test.ts lines 70-103 model those sources
separately in both same-origin and cross-origin cases.
In `@app/sagas/deepLinking.js`:
- Around line 162-164: Update the deep-linking wait condition around sdk.server
so a matching server URL only skips LOGIN.SUCCESS when the SDK is actually
authenticated and connected, reusing the same state criteria established by
selectServer; otherwise continue waiting for types.LOGIN.SUCCESS.
In `@app/sagas/videoConf.ts`:
- Around line 182-196: Update the response handling in the video conference saga
around videoConfResponse so it does not depend on the nonexistent success field;
treat the presence of the returned data as the success condition. Restore an
explicit failure path that resets setCalling, hides the action sheet via
hideActionSheetRef, and shows the existing initialization error when the
response lacks valid data, while preserving the direct-call and join behavior
for successful responses.
In `@app/views/ThreadMessagesView/index.tsx`:
- Around line 347-361: Update the failure paths in the load method so
unsuccessful results and caught errors clear loading without permanently setting
end to true, allowing a later onEndReached call to retry. Preserve end updates
for successful pagination responses and the existing guard behavior.
🪄 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 Plus
Run ID: ce0c89f2-7766-4248-bc6f-7d40c2bbf8d7
⛔ Files ignored due to path filters (4)
app/containers/message/components/__tests__/__snapshots__/Message.test.tsx.snapis excluded by!**/*.snapapp/containers/message/components/stories/__tests__/__snapshots__/leaves.test.tsx.snapis excluded by!**/*.snapapp/views/RoomView/LoadMore/__snapshots__/LoadMore.test.tsx.snapis excluded by!**/*.snappnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (39)
app/containers/Avatar/Avatar.tsxapp/containers/MessageComposer/MessageComposer.test.tsxapp/containers/MessageComposer/MessageComposer.tsxapp/definitions/rest/v1/auth.tsapp/ee/omnichannel/lib/index.test.tsapp/ee/omnichannel/lib/index.tsapp/ee/omnichannel/lib/subscriptions/inquiry.test.tsapp/ee/omnichannel/lib/subscriptions/inquiry.tsapp/lib/methods/actions.test.tsapp/lib/methods/actions.tsapp/lib/methods/getSettings.tsapp/lib/methods/helpers/fetch.test.tsapp/lib/methods/helpers/fetch.tsapp/lib/methods/helpers/getAuthHeaders.test.tsapp/lib/methods/helpers/getAuthHeaders.tsapp/lib/methods/helpers/isSameOrigin.test.tsapp/lib/methods/helpers/isSameOrigin.tsapp/lib/methods/sendFileMessage/sendFileMessage.test.tsapp/lib/methods/sendFileMessage/sendFileMessage.tsapp/lib/methods/subscriptions/room.test.tsapp/lib/methods/subscriptions/room.tsapp/lib/services/__tests__/ddpConnectionPatch.test.tsapp/lib/services/connect.test.tsapp/lib/services/connect.tsapp/lib/services/restApi.test.tsapp/lib/services/restApi.tsapp/lib/services/sdk.test.tsapp/lib/services/sdk.tsapp/sagas/deepLinking.jsapp/sagas/selectServer.tsapp/sagas/videoConf.tsapp/views/NewServerView/methods/basicAuth.tsapp/views/SearchMessagesView/index.tsxapp/views/ShareView/index.tsxapp/views/ThreadMessagesView/index.tsxapp/views/UserNotificationPreferencesView/index.tsxjest.config.jspackage.jsonpatches/@rocket.chat+api-client+0.2.56.patch
🚧 Files skipped from review as they are similar to previous changes (13)
- app/containers/Avatar/Avatar.tsx
- jest.config.js
- app/lib/methods/actions.test.ts
- app/views/SearchMessagesView/index.tsx
- app/definitions/rest/v1/auth.ts
- app/containers/MessageComposer/MessageComposer.tsx
- app/containers/MessageComposer/MessageComposer.test.tsx
- package.json
- app/ee/omnichannel/lib/subscriptions/inquiry.test.ts
- app/views/UserNotificationPreferencesView/index.tsx
- app/ee/omnichannel/lib/index.ts
- app/ee/omnichannel/lib/subscriptions/inquiry.ts
- app/lib/services/restApi.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: E2E Build Android / android-build
- GitHub Check: E2E Build iOS / ios-build
🧰 Additional context used
📓 Path-based instructions (8)
**/*.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,ts,jsx,tsx}: Use descriptive names for functions, variables, and classes that clearly convey their purpose
Write comments that explain the 'why' behind code decisions, not the 'what'
Keep functions small and focused on a single responsibility
Use const by default, let when reassignment is needed, and avoid var
Prefer async/await over .then() chains for handling asynchronous operations
Use explicit error handling with try/catch blocks for async operations
Avoid deeply nested code; refactor complex logic into helper functions
Files:
app/lib/methods/helpers/getAuthHeaders.tsapp/views/NewServerView/methods/basicAuth.tsapp/lib/methods/helpers/isSameOrigin.test.tsapp/sagas/deepLinking.jsapp/lib/methods/helpers/getAuthHeaders.test.tsapp/views/ThreadMessagesView/index.tsxapp/views/ShareView/index.tsxapp/lib/methods/helpers/fetch.test.tsapp/lib/methods/helpers/isSameOrigin.tsapp/ee/omnichannel/lib/index.test.tsapp/lib/methods/actions.tsapp/sagas/selectServer.tsapp/lib/services/__tests__/ddpConnectionPatch.test.tsapp/lib/methods/getSettings.tsapp/sagas/videoConf.tsapp/lib/methods/sendFileMessage/sendFileMessage.tsapp/lib/methods/sendFileMessage/sendFileMessage.test.tsapp/lib/services/restApi.test.tsapp/lib/methods/subscriptions/room.test.tsapp/lib/methods/helpers/fetch.tsapp/lib/methods/subscriptions/room.tsapp/lib/services/connect.test.tsapp/lib/services/sdk.test.tsapp/lib/services/sdk.tsapp/lib/services/connect.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use TypeScript for type safety; add explicit type annotations to function parameters and return types
Prefer interfaces over type aliases for defining object shapes in TypeScript
Use enums for sets of related constants rather than magic strings or numbersUse TypeScript strict mode; resolve application imports relative to the
app/base URL.
Files:
app/lib/methods/helpers/getAuthHeaders.tsapp/views/NewServerView/methods/basicAuth.tsapp/lib/methods/helpers/isSameOrigin.test.tsapp/lib/methods/helpers/getAuthHeaders.test.tsapp/views/ThreadMessagesView/index.tsxapp/views/ShareView/index.tsxapp/lib/methods/helpers/fetch.test.tsapp/lib/methods/helpers/isSameOrigin.tsapp/ee/omnichannel/lib/index.test.tsapp/lib/methods/actions.tsapp/sagas/selectServer.tsapp/lib/services/__tests__/ddpConnectionPatch.test.tsapp/lib/methods/getSettings.tsapp/sagas/videoConf.tsapp/lib/methods/sendFileMessage/sendFileMessage.tsapp/lib/methods/sendFileMessage/sendFileMessage.test.tsapp/lib/services/restApi.test.tsapp/lib/methods/subscriptions/room.test.tsapp/lib/methods/helpers/fetch.tsapp/lib/methods/subscriptions/room.tsapp/lib/services/connect.test.tsapp/lib/services/sdk.test.tsapp/lib/services/sdk.tsapp/lib/services/connect.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Follow the repository Prettier style: tabs, single quotes, 130-character width, no trailing commas, omitted arrow-function parentheses where allowed, and same-line brackets.
Files:
app/lib/methods/helpers/getAuthHeaders.tsapp/views/NewServerView/methods/basicAuth.tsapp/lib/methods/helpers/isSameOrigin.test.tsapp/sagas/deepLinking.jsapp/lib/methods/helpers/getAuthHeaders.test.tsapp/views/ThreadMessagesView/index.tsxapp/views/ShareView/index.tsxapp/lib/methods/helpers/fetch.test.tsapp/lib/methods/helpers/isSameOrigin.tsapp/ee/omnichannel/lib/index.test.tsapp/lib/methods/actions.tsapp/sagas/selectServer.tsapp/lib/services/__tests__/ddpConnectionPatch.test.tsapp/lib/methods/getSettings.tsapp/sagas/videoConf.tsapp/lib/methods/sendFileMessage/sendFileMessage.tsapp/lib/methods/sendFileMessage/sendFileMessage.test.tsapp/lib/services/restApi.test.tsapp/lib/methods/subscriptions/room.test.tsapp/lib/methods/helpers/fetch.tsapp/lib/methods/subscriptions/room.tsapp/lib/services/connect.test.tsapp/lib/services/sdk.test.tsapp/lib/services/sdk.tsapp/lib/services/connect.ts
**/*.test.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Run Jest tests with
TZ=UTCto ensure deterministic timezone-dependent test behavior.
Files:
app/lib/methods/helpers/isSameOrigin.test.tsapp/lib/methods/helpers/getAuthHeaders.test.tsapp/lib/methods/helpers/fetch.test.tsapp/ee/omnichannel/lib/index.test.tsapp/lib/services/__tests__/ddpConnectionPatch.test.tsapp/lib/methods/sendFileMessage/sendFileMessage.test.tsapp/lib/services/restApi.test.tsapp/lib/methods/subscriptions/room.test.tsapp/lib/services/connect.test.tsapp/lib/services/sdk.test.ts
app/ee/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Keep enterprise Omnichannel and livechat functionality in the enterprise layer.
Files:
app/ee/omnichannel/lib/index.test.ts
app/sagas/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use sagas for side effects such as initialization, authentication, rooms, messages, encryption, deep linking, and video conferencing.
Files:
app/sagas/selectServer.tsapp/sagas/videoConf.ts
app/sagas/videoConf.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Implement VideoConf as a server-managed Jitsi feature using Redux actions, reducers, and sagas; keep it separate from VoIP.
Files:
app/sagas/videoConf.ts
app/lib/services/{sdk,restApi,connect}.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use the SDK service for WebSocket subscriptions, the REST API service for HTTP requests via
fetch, and the connect service for server connection management.
Files:
app/lib/services/sdk.tsapp/lib/services/connect.ts
🧠 Learnings (6)
📚 Learning: 2026-04-30T17:07:51.020Z
Learnt from: diegolmello
Repo: RocketChat/Rocket.Chat.ReactNative PR: 7274
File: app/lib/services/voip/MediaCallEvents.ts:0-0
Timestamp: 2026-04-30T17:07:51.020Z
Learning: In this Rocket.Chat React Native codebase, the ESLint rule `no-void: error` is enforced. When you see a promise returned from an async call that is not awaited (a “floating promise”), do not silence it with the `void somePromise()` pattern. Instead, handle the promise explicitly by attaching `.catch(...)` (or otherwise awaiting/handling the error) so unhandled-rejection risks are addressed in a way that satisfies the existing ESLint configuration.
Applied to files:
app/lib/methods/helpers/getAuthHeaders.tsapp/views/NewServerView/methods/basicAuth.tsapp/lib/methods/helpers/isSameOrigin.test.tsapp/lib/methods/helpers/getAuthHeaders.test.tsapp/views/ThreadMessagesView/index.tsxapp/views/ShareView/index.tsxapp/lib/methods/helpers/fetch.test.tsapp/lib/methods/helpers/isSameOrigin.tsapp/ee/omnichannel/lib/index.test.tsapp/lib/methods/actions.tsapp/sagas/selectServer.tsapp/lib/services/__tests__/ddpConnectionPatch.test.tsapp/lib/methods/getSettings.tsapp/sagas/videoConf.tsapp/lib/methods/sendFileMessage/sendFileMessage.tsapp/lib/methods/sendFileMessage/sendFileMessage.test.tsapp/lib/services/restApi.test.tsapp/lib/methods/subscriptions/room.test.tsapp/lib/methods/helpers/fetch.tsapp/lib/methods/subscriptions/room.tsapp/lib/services/connect.test.tsapp/lib/services/sdk.test.tsapp/lib/services/sdk.tsapp/lib/services/connect.ts
📚 Learning: 2026-06-25T18:37:25.526Z
Learnt from: diegolmello
Repo: RocketChat/Rocket.Chat.ReactNative PR: 7434
File: app/views/ScreenLockConfigView.test.tsx:16-22
Timestamp: 2026-06-25T18:37:25.526Z
Learning: In Rocket.Chat ReactNative tests that mock selectors for `useAppSelector`, don’t require the mocked selector input to be typed as `IApplicationState` when the fixture only includes a partial Redux state slice (e.g., only `server` and `settings`). Requiring the full `IApplicationState` type in that scenario forces unsafe `as IApplicationState` casts and undermines type-safety. For these narrowly scoped selector-mock fixtures, use a less strict type (e.g., `any`) to keep the mock focused on the slice under test.
Applied to files:
app/lib/methods/helpers/isSameOrigin.test.tsapp/lib/methods/helpers/getAuthHeaders.test.tsapp/lib/methods/helpers/fetch.test.tsapp/ee/omnichannel/lib/index.test.tsapp/lib/services/__tests__/ddpConnectionPatch.test.tsapp/lib/methods/sendFileMessage/sendFileMessage.test.tsapp/lib/services/restApi.test.tsapp/lib/methods/subscriptions/room.test.tsapp/lib/services/connect.test.tsapp/lib/services/sdk.test.ts
📚 Learning: 2026-05-07T13:19:52.152Z
Learnt from: diegolmello
Repo: RocketChat/Rocket.Chat.ReactNative PR: 7304
File: app/sagas/deepLinking.js:237-243
Timestamp: 2026-05-07T13:19:52.152Z
Learning: In this codebase’s Redux-Saga usage, remember that `yield put(action)` dispatches through the Redux store synchronously, and any saga(s) that synchronously react via action listeners (and synchronous `put` chains) will run to completion before the calling saga resumes at its next `yield`. As a result, within a single saga there is no scheduler interleaving between a `yield select(...)` and a subsequent `yield take(...)` at the next `yield` point, so a check-then-take pattern like `const state = yield select(...); if (state !== TARGET) { yield take(a => a.type === TARGET); }` is safe from TOCTOU races under the synchronous `put`/take model described above.
Applied to files:
app/sagas/deepLinking.js
📚 Learning: 2026-06-24T22:58:43.390Z
Learnt from: Rohit3523
Repo: RocketChat/Rocket.Chat.ReactNative PR: 7157
File: app/views/MessagesView/index.tsx:392-392
Timestamp: 2026-06-24T22:58:43.390Z
Learning: When wrapping a React Native component (e.g., via `withSafeAreaInsets`) ensure `hoistNonReactStatics` is only required if the wrapped component actually defines static properties/methods that consumers rely on. If the component has no statics (as in `app/views/MessagesView/index.tsx`), you can omit `hoistNonReactStatics` for this case.
Applied to files:
app/views/ThreadMessagesView/index.tsxapp/views/ShareView/index.tsx
📚 Learning: 2026-06-25T18:37:44.793Z
Learnt from: diegolmello
Repo: RocketChat/Rocket.Chat.ReactNative PR: 7434
File: app/views/ScreenLockConfigView.tsx:101-141
Timestamp: 2026-06-25T18:37:44.793Z
Learning: In the Rocket.Chat React Native codebase, do not treat passing an `async` function directly to an event prop in React/React Native UI components (e.g., `onPress={async () => ...}` in TSX) as a “floating promises” CI-blocking lint issue—this repo does not enable the ESLint `no-floating-promises` rule (while `no-void` is enforced). Only raise robustness follow-ups when there are genuinely unhandled promise paths (e.g., fire-and-forget calls like `save()` that return a Promise that is neither awaited nor handled), and prefer making sure failure paths are explicitly handled/reported rather than blocking on lint-style floating-promise concerns.
Applied to files:
app/views/ThreadMessagesView/index.tsxapp/views/ShareView/index.tsx
📚 Learning: 2026-05-05T21:08:33.177Z
Learnt from: diegolmello
Repo: RocketChat/Rocket.Chat.ReactNative PR: 7298
File: patches/@rocket.chat+sdk+1.3.3-mobile.patch:79-79
Timestamp: 2026-05-05T21:08:33.177Z
Learning: In the RocketChat/Rocket.Chat.ReactNative repo, for patches under patches/*.patch (especially those touching rocket.chat/sdk), remember that the patching sandbox's node_modules reflects the pre-patch state. Grepping node_modules for symbols (e.g., userDisconnectCloseCode = 4000 in node_modules/rocket.chat/sdk/lib/drivers/ddp.ts) can yield false positives. Review patches by inspecting the diff and applying it to a fresh copy of the SDK or diffing against the SDK source, rather than relying on node_modules. Ensure the patch actually introduces/updates symbols in the SDK source and run the test suite to validate behavior after applying the patch.
Applied to files:
patches/@rocket.chat+api-client+0.2.56.patch
🪛 GitHub Check: ESLint and Test / run-eslint-and-test
app/lib/services/sdk.ts
[failure] 295-295:
Object is possibly 'undefined'.
🔇 Additional comments (29)
patches/@rocket.chat+api-client+0.2.56.patch (2)
1-30: LGTM!
7-8: 🎯 Functional CorrectnessNo change needed for the
buildFormDataargument-order swap.
buildFormData(data, formData = new FormData(), parentKey)matches the new call sites, sodelete()can pass a singleparamsargument safely.> Likely an incorrect or invalid review comment.app/lib/services/sdk.ts (4)
173-217: 🎯 Functional CorrectnessVerify 2FA header retry actually resolves the challenge for the REST-wrapped method-call path.
On retry,
post()attachesx-2fa-code/x-2fa-methodas HTTP headers (lines 207-210) for both plain REST endpoints and the/v1/method.call/...wrapper (isMethodCall). For the wrapped-method-call path, the DDP method's own arguments live inside the JSONmessagebody, and 2FA for direct DDP calls is passed as an extra method argument (seecallMethod's...(code ? [code] : [])), not a header. It's unclear whether the server's generic 2FA check also honors headers for this specific wrapped endpoint, or whether the retry needs to re-encode the code into themessagepayload instead — resending the same unchangedparams/messagewith just new headers could repeat the sametotp-required/totp-invalidfailure.Per the PR's own review thread, this is flagged as a still-open concern ("REST method-call 2FA retries may collect a code but resend without including it"), not confirmed as resolved.
60-137: LGTM! Server-switch guards inlogin(),try/catch/finallywith a bounded timeout inlogout(), and theinitialize()/ensureInitialized()/setBasicAuth()reworks all address the previously-flagged issues (stale headers across server switches, cross-server credential leakage on login, logout not clearing auth headers on failure).Also applies to: 254-285, 432-454
139-159: LGTM!get()/delete()now correctly attachthis.headers, andmethodCallWrapper()'s REST/WS routing and Date→EJSON handling match the documented/tested behavior.Also applies to: 221-241, 312-329
331-415: LGTM!subscribe()now fails fast viaensureInitialized()instead of silently returningundefined, andsubscribeNotifyUser()/subscribeRoom()implementations look consistent with the covering tests.app/lib/methods/helpers/fetch.ts (1)
17-31: LGTM! This correctly resolves the prior header-merge-order and cross-origin token-leak issues raised in earlier reviews: caller headers can override SDK defaults for same-origin requests, whileX-Auth-Token/X-User-Idare stripped for cross-origin requests even if explicitly supplied by the caller, matchingfetch.test.ts.app/lib/methods/helpers/getAuthHeaders.ts (1)
9-17: LGTM!app/lib/methods/helpers/isSameOrigin.ts (1)
1-17: LGTM! The trailing-slash normalization beforestartsWithcorrectly prevents false positives between sibling subpath deployments (e.g./workspace-avs/workspace-a-extra).app/lib/methods/sendFileMessage/sendFileMessage.test.ts (1)
68-212: LGTM! Solid coverage of the V1/V2 routing, header composition, and encrypted multipart payload behavior introduced by the SDK header centralization.app/views/ShareView/index.tsx (2)
81-92: LGTM! The migration from localselectedMessages/actionstate to a scopedmessageActionStore(limited to thekind === 'quote'flow) is internally consistent, and lifecycle cleanup (componentWillUnmount,onRemoveQuoteMessage) correctly derives from the store.Also applies to: 123-127, 242-242, 269-272, 385-387, 395-403
278-295: 🗄️ Data Integrity & IntegrationNo change needed.
sendAttachmentsdelegates tosendFileMessage, whose signature does not take auserfield, whilesendMessagestill constructs the local message author from the passed user.app/lib/methods/actions.ts (1)
131-138: 🔒 Security & PrivacyNo change needed.
fetchis imported fromapp/lib/methods/helpers/fetch, and that helper merges the SDK auth headers for same-origin requests before calling the nativefetch.app/lib/methods/sendFileMessage/sendFileMessage.ts (1)
4-4: LGTM!Also applies to: 16-25
app/lib/methods/helpers/isSameOrigin.test.ts (1)
4-19: LGTM!Also applies to: 25-31
app/lib/methods/helpers/fetch.test.ts (1)
5-5: LGTM!Also applies to: 106-119
app/views/NewServerView/methods/basicAuth.ts (1)
13-13: LGTM!app/sagas/deepLinking.js (1)
239-239: LGTM!Also applies to: 342-342
app/lib/methods/helpers/getAuthHeaders.test.ts (1)
1-45: 📐 Maintainability & Code QualityJest test suites are already run with
TZ=UTC.
package.jsondefinestestandtest-updateasTZ=UTC jest, and CI usespnpm test --runInBand, so no change is needed.app/lib/methods/getSettings.ts (1)
160-163: 🎯 Functional CorrectnessConfirm
getSettingstargets the correct server when adding a new workspace.This still relies on
reduxStore.getState().serverfor bothserverandserverVersion, the same concerndiegolmelloraised previously and left unresolved: when a new workspace is being added, does this correctly resolve to the new server being connected to, or could it read stale/previous-session state from the store?app/lib/methods/subscriptions/room.test.ts (1)
8-10: LGTM!Also applies to: 19-20, 153-172, 223-233, 235-257, 269-271, 306-347
app/lib/services/sdk.test.ts (1)
158-167: LGTM!Also applies to: 239-240, 291-325, 370-382, 602-645, 854-866, 900-928
app/lib/services/connect.test.ts (2)
1174-1208: 🎯 Functional Correctness
getWebsocketInfonow fails on any probe error, not just an explicit websocket-disabled response.These tests confirm that for any error other than one containing
'400',getWebsocketInforeturns{ success: false, message: <raw error> }(see the "falls back to the error message for non-400 failures" case). This matches a previously raised concern: a transient network/DNS blip during this probe will now cause the add-server flow to reject a valid server URL asInvalid_URL, not just a server that explicitly disables WebSockets. That thread was not confirmed resolved.
33-46: LGTM!Also applies to: 534-560, 688-719, 721-740, 1116-1172
app/lib/services/restApi.test.ts (2)
308-330: 🩺 Stability & Availability | ⚡ Quick winMissing test for
sdk.delete()failure inremovePushToken— underlying concern still unaddressed.Only the success and no-token paths are tested. A previously raised concern that
sdk.delete()failures may propagate and disrupt logout (suggesting a try/catch returningfalse) was never confirmed fixed. Add a test assertingremovePushToken()resolves tofalse(or similar) whenmockSdkDeleterejects, and verifyrestApi.tsactually guards against the rejection propagating intologout().✅ Suggested additional test
it('returns undefined and does not call delete when there is no device token', async () => { (require('../notifications').getDeviceToken as jest.Mock).mockReturnValue(undefined); expect(await removePushToken()).toBeUndefined(); expect(mockSdkDelete).not.toHaveBeenCalled(); }); + + it('returns false and does not throw when sdk.delete() rejects', async () => { + mockSdkDelete.mockRejectedValueOnce(new Error('network error')); + await expect(removePushToken()).resolves.toBe(false); + }); });
293-306: LGTM!app/lib/services/connect.ts (1)
49-113: LGTM!Also applies to: 165-182, 278-280
app/lib/methods/subscriptions/room.ts (1)
26-26: LGTM!Also applies to: 35-37, 53-58, 58-58, 76-83, 106-106, 271-400
app/lib/services/__tests__/ddpConnectionPatch.test.ts (1)
159-251: LGTM!
| describe('getAuthHeaders', () => { | ||
| beforeEach(() => { | ||
| (sdk.getHeaders as jest.Mock).mockReset(); | ||
| }); | ||
|
|
||
| it('returns full headers including the session token for same-origin URLs', () => { | ||
| (sdk.getHeaders as jest.Mock).mockReturnValue({ 'User-Agent': 'RC Mobile', 'X-Auth-Token': 'tok', 'X-User-Id': 'uid' }); | ||
| const headers = getAuthHeaders('https://open.rocket.chat/api/v1/settings'); | ||
| expect(headers['X-Auth-Token']).toBe('tok'); | ||
| expect(headers['X-User-Id']).toBe('uid'); | ||
| expect(headers['User-Agent']).toBe('RC Mobile'); | ||
| }); | ||
|
|
||
| it('strips only the session token for cross-origin URLs, keeping other headers', () => { | ||
| (sdk.getHeaders as jest.Mock).mockReturnValue({ | ||
| 'User-Agent': 'RC Mobile', | ||
| Authorization: 'Basic abc', | ||
| 'X-Auth-Token': 'tok', | ||
| 'X-User-Id': 'uid' | ||
| }); | ||
| const headers = getAuthHeaders('https://releases.rocket.chat/v2/server/supportedVersions'); | ||
| expect(headers['X-Auth-Token']).toBeUndefined(); | ||
| expect(headers['X-User-Id']).toBeUndefined(); | ||
| expect(headers['User-Agent']).toBe('RC Mobile'); | ||
| expect(headers.Authorization).toBe('Basic abc'); | ||
| }); | ||
|
|
||
| it('treats relative URLs as same-origin', () => { | ||
| (sdk.getHeaders as jest.Mock).mockReturnValue({ 'X-Auth-Token': 'tok' }); | ||
| const headers = getAuthHeaders('/api/v1/x'); | ||
| expect(headers['X-Auth-Token']).toBe('tok'); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Files:\n'
git ls-files | rg 'app/(lib/methods/helpers/(getAuthHeaders|isSameOrigin|fetch)\.test\.ts|sagas/selectServer\.ts)$|package\.json|tsconfig.*\.json' || true
printf '\nSelected file sizes:\n'
wc -l app/lib/methods/helpers/getAuthHeaders.test.ts app/lib/methods/helpers/isSameOrigin.test.ts app/lib/methods/helpers/fetch.test.ts app/sagas/selectServer.ts || true
printf '\nRelevant source:\n'
for f in app/lib/methods/helpers/getAuthHeaders.test.ts app/lib/methods/helpers/isSameOrigin.test.ts app/lib/methods/helpers/fetch.test.ts app/sagas/selectServer.ts; do
echo "--- $f ---"
cat -n "$f"
done
printf '\nSearch callback/type patterns in adjacent code:\n'
rg -n "describe\(|it\(|test\(|appSelector|function|const .*=\s*(async )?\(" app/lib/methods/helpers app/sagas/selectServer.ts --glob '*.ts' --glob '*.tsx' || true
printf '\nType config / lint rule hints:\n'
for f in $(git ls-files 'tsconfig*.json' package.json); do
if rg -n '"strict"|"noImplicitAny"|"typedef"|"require-type-annotation"|"typescript-eslint"|"eslint' "$f" > /tmp/matches.$$; then
echo "--- $f ---"
cat "$f"
fi
doneRepository: RocketChat/Rocket.Chat.ReactNative
Length of output: 50390
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'tsconfig.json:\n'
cat -n tsconfig.json
printf '\nRelevant callback annotation occurrences in surrounding code:\n'
rg -n "appSelector\(|describe\(|test\(|it\(\) as |\): void =>|=> \{|async function|function\(|function \* handleSelectServer|function\* handleSelectServer" app/sagas/selectServer.ts app/lib/methods/helpers --glob '*.ts' --glob '*.tsx' | head -n 200Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 29254
Add explicit callback annotations.
These callbacks rely on TypeScript inference while the project uses TypeScript strict mode and the repo guideline requires explicit parameter/return annotations.
app/lib/methods/helpers/getAuthHeaders.test.ts#L13-L45andapp/lib/methods/helpers/isSameOrigin.test.ts#L3-L32: annotatedescribe,it,test, andbeforeEachcallbacks withvoid.app/lib/methods/helpers/fetch.test.ts#L70-L119: annotate async test callbacks withPromise<void>and synchronous callbacks withvoid.app/sagas/selectServer.ts#L141-L141: annotate thestateparameter on the selector callback; the same selector callback at#L191should be annotated too.
[low eff
ort_and_high_reward]
📍 Affects 4 files
app/lib/methods/helpers/getAuthHeaders.test.ts#L13-L45(this comment)app/lib/methods/helpers/isSameOrigin.test.ts#L3-L32app/lib/methods/helpers/fetch.test.ts#L70-L119app/sagas/selectServer.ts#L141-L141
🤖 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 `@app/lib/methods/helpers/getAuthHeaders.test.ts` around lines 13 - 45,
Explicitly annotate all affected callbacks: use void returns for describe, it,
test, and beforeEach callbacks in app/lib/methods/helpers/getAuthHeaders.test.ts
(anchor, lines 13-45) and app/lib/methods/helpers/isSameOrigin.test.ts (lines
3-32); use Promise<void> for async test callbacks and void for synchronous
callbacks in app/lib/methods/helpers/fetch.test.ts (lines 70-119). In
app/sagas/selectServer.ts, annotate the state parameter for the selector
callbacks at lines 141 and 191 with the appropriate state type.
Source: Coding guidelines
|
Android Build Available Rocket.Chat 4.75.0.109415 Internal App Sharing: https://play.google.com/apps/test/RQQ8k09hlnQ/ahAO29uNSmMzG1Gdk7Y-HhAp5VQVKZH3b7VfrYNTpfbiIRVhF9u3o6i8c5vrBVkaoudppHFY0S5E38RvSBCGXz5HmW |
|
iOS Build Available Rocket.Chat 4.75.0.109416 |
3 participants
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
Related PRs in core repo
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes