fix: lib-led DDP stream restoration on Zombie Connection#7513
fix: lib-led DDP stream restoration on Zombie Connection#7513diegolmello wants to merge 20 commits into
Conversation
A failed resume-login could strand a connected socket that is not
authenticated with zero subscriptions (a Zombie Connection). Recovery
now lives in the SDK's own resubscribe path plus two surgical app-side
gates, with no separate restoration owner.
- SDK patch: forceReopen no longer wipes the subscription registry, so
the next login replays every sub via subscribeAll(); add a synchronous
(name, params) idempotency guard so replay and app re-issue dedup, and
normalize eager-send entries with onEvent.
- connect.ts: the connected listener early-returns only when already
authenticated, so a Zombie Connection re-runs resume-login.
- state.js: on foreground inside ROOT_INSIDE with a token but no auth,
dispatch loginRequest({ resume }).
- room.ts: drop the teardown of prior subscriptions in handleLogin; the
fresh DDP session already dropped them and the SDK dedups the re-sub.
GitHub Actions
Inactive
|
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 updates DDP reconnection and subscription replay, adds zombie-connection resume handling, changes room subscription cleanup, introduces server-derived sync cursor advancement, adds LokiJS integration coverage, and replaces room initialization retry timeouts with a capped backoff scheduler. ChangesConnection and subscription recovery
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
Suggested labels: 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
Inactive
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
patches/@rocket.chat+sdk+1.3.3-mobile.patch (2)
198-217: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueCallback silently dropped if
existing.onEventis falsy.if (existing) { if (callback && existing.onEvent) existing.onEvent(callback) return Promise.resolve(existing) }If
existing.onEventis ever missing, the caller'scallbackis silently discarded — no error, no log, and the caller believes it subscribed successfully. Currently unreachable because every write path tothis.subscriptionsnow setsonEvent(line 191, line 238), but it's worth a defensive log if this branch is ever hit, since a silently-dropped message callback is hard to diagnose.🤖 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 `@patches/`@rocket.chat+sdk+1.3.3-mobile.patch around lines 198 - 217, Update the existing-subscription branch in subscribe so a provided callback is not silently discarded when existing.onEvent is unavailable. Preserve the current callback attachment when existing.onEvent exists, and add defensive diagnostic logging for the missing-handler case before returning the resolved existing subscription.
152-182: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNo test coverage for the new
checkAndReopen/probebucket logic.This three-bucket decision tree (stale/fresh/gray+probe) is new, non-trivial branching logic with hardcoded timing thresholds (
ping*2,2000,probe's 2s timeout), yetddpSocket.test.tsonly covers subscription replay/dedup, notcheckAndReopen/probe. A regression here (e.g., wrong bucket boundary) would silently degrade reconnect reliability.Want me to draft unit tests for the three buckets (stale, fresh, gray-probe-pass, gray-probe-fail)?
🤖 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 `@patches/`@rocket.chat+sdk+1.3.3-mobile.patch around lines 152 - 182, Add unit coverage in ddpSocket.test.ts for checkAndReopen and probe, covering disconnected or stale connections forcing reopen, fresh traffic returning without probing, gray-window probe success preserving the connection, and probe failure forcing reopen. Assert the boundary behavior for ping*2 and 2000ms thresholds, and mock timing, probe responses, and forceReopen so each bucket’s action is verified.
🤖 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.
Nitpick comments:
In `@patches/`@rocket.chat+sdk+1.3.3-mobile.patch:
- Around line 198-217: Update the existing-subscription branch in subscribe so a
provided callback is not silently discarded when existing.onEvent is
unavailable. Preserve the current callback attachment when existing.onEvent
exists, and add defensive diagnostic logging for the missing-handler case before
returning the resolved existing subscription.
- Around line 152-182: Add unit coverage in ddpSocket.test.ts for checkAndReopen
and probe, covering disconnected or stale connections forcing reopen, fresh
traffic returning without probing, gray-window probe success preserving the
connection, and probe failure forcing reopen. Assert the boundary behavior for
ping*2 and 2000ms thresholds, and mock timing, probe responses, and forceReopen
so each bucket’s action is verified.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a1b88ac6-08ef-4a28-84f8-c265f30a6d78
📒 Files selected for processing (9)
CONTEXT.mdapp/lib/methods/subscriptions/room.test.tsapp/lib/methods/subscriptions/room.tsapp/lib/services/connect.test.tsapp/lib/services/connect.tsapp/lib/services/ddpSocket.test.tsapp/sagas/__tests__/state.test.tsapp/sagas/state.jspatches/@rocket.chat+sdk+1.3.3-mobile.patch
📜 Review details
⏰ Context from checks skipped due to timeout. (3)
- GitHub Check: E2E Build iOS / ios-build
- GitHub Check: E2E Build Android / android-build
- GitHub Check: ESLint and Test / run-eslint-and-test
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{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/sagas/__tests__/state.test.tsapp/lib/methods/subscriptions/room.tsapp/lib/services/connect.test.tsapp/lib/methods/subscriptions/room.test.tsapp/sagas/state.jsapp/lib/services/ddpSocket.test.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/sagas/__tests__/state.test.tsapp/lib/methods/subscriptions/room.tsapp/lib/services/connect.test.tsapp/lib/methods/subscriptions/room.test.tsapp/lib/services/ddpSocket.test.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/sagas/__tests__/state.test.tsapp/lib/methods/subscriptions/room.tsapp/lib/services/connect.test.tsapp/lib/methods/subscriptions/room.test.tsapp/sagas/state.jsapp/lib/services/ddpSocket.test.tsapp/lib/services/connect.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/__tests__/state.test.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/sagas/__tests__/state.test.tsapp/lib/services/connect.test.tsapp/lib/methods/subscriptions/room.test.tsapp/lib/services/ddpSocket.test.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/connect.ts
🧠 Learnings (4)
📚 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/sagas/__tests__/state.test.tsapp/lib/methods/subscriptions/room.tsapp/lib/services/connect.test.tsapp/lib/methods/subscriptions/room.test.tsapp/lib/services/ddpSocket.test.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/sagas/__tests__/state.test.tsapp/lib/services/connect.test.tsapp/lib/methods/subscriptions/room.test.tsapp/lib/services/ddpSocket.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/state.js
📚 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+sdk+1.3.3-mobile.patch
🔇 Additional comments (15)
patches/@rocket.chat+sdk+1.3.3-mobile.patch (7)
2-11: LGTM!
19-41: LGTM!
46-58: LGTM!
96-150:forceReopendesign looks sound.Registry preservation, the
_reopenInFlightguard, and the synchronousdisconnected/closeemission before teardown are all consistent with the documented Zombie-Connection fix and match theddpSocket.test.tsassertions.
184-192: LGTM!
219-263: LGTM!
264-272: LGTM!app/lib/services/ddpSocket.test.ts (1)
109-181: Solid coverage for registry preservation, replay, and dedup.Tests correctly exercise
forceReopenregistry preservation,subscribeAllreplay-with-original-id, andsubscribe()dedup with callback fan-out, matching the patch's intended contract.app/lib/services/connect.test.ts (2)
605-647: LGTM on test scenarios themselves (aside from the duplicate declaration flagged separately).
611-613: 🎯 Functional CorrectnessNo change needed — there’s no duplicate
ResumeActiondeclaration.The file contains a single
type ResumeActiondeclaration inside the “connect — connected-guard” scope, so there’s no TS2300 duplicate-identifier issue here.> Likely an incorrect or invalid review comment.app/sagas/__tests__/state.test.ts (1)
33-73: LGTM!CONTEXT.md (1)
183-189: LGTM!app/lib/methods/subscriptions/room.ts (1)
96-117: Teardown removal is justified by the SDK-level dedup contract.Skipping
unsubscribe()on the priorthis.promisesinhandleLoginrelies entirely on the SDK's registry preservation +(name, params)dedup (verified inddpSocket.test.ts) to prevent server-side subscription leaks across repeated reopen cycles. Sincesdk.subscribeRoom(this.rid)reuses the same(name, [rid])key on every call, the SDK collapses repeats into the single preserved registry entry, so this is consistent with the cross-layer design.app/lib/methods/subscriptions/room.test.ts (1)
196-236: LGTM!app/lib/services/connect.ts (1)
119-131: 🩺 Stability & AvailabilityNo change needed here. The login saga runs
takeLatest(types.LOGIN.REQUEST, handleLoginRequest), so concurrentLOGIN.REQUESTactions coalesce to the latest request.> Likely an incorrect or invalid review comment.
GitHub Actions
Inactive
GitHub Actions
Error
|
iOS Build Available Rocket.Chat 4.75.0.109406 |
|
Android Build Available Rocket.Chat 4.75.0.109405 Internal App Sharing: https://play.google.com/apps/test/RQQ8k09hlnQ/ahAO29uNTLfTP4VJnh5jNzrKABoRttfkaZD04qxn0lnCXOZz03w_OQ50jwrYdGEIuaKCMGW4r5BkNOKLPg2-2fSJZ0 |
…ent clock skew subscription.lastOpen doubles as the chat.syncMessages cursor but was written from the client clock and advanced unconditionally after every room open. A client clock ahead of the server orphans messages that arrived while the app was backgrounded: the server answers a future cursor with a successful empty response, the app re-advances the cursor, and the window is skipped forever. The cursor now advances only from the max _updatedAt of persisted batches (advanceSyncCursor), never on an empty fetch and never from older-history windows. readMessages no longer writes lastOpen and updateLastOpen (client-clock reseed on unsubscribe) is removed. Also fixes two sync bugs in loadMissedMessages: DELETED pagination results landing in the updated slot when only deletedNext paginated, and the un-awaited recursive pagination call.
Advancing lastOpen per page was unsafe across the independent UPDATED/DELETED streams: a newer timestamp from one stream's page could move the shared cursor past records the other stream still had pending, skipping them permanently if a later page failed. Pages are still persisted as they arrive, but the cursor now advances exactly once after the whole chain completes. Also re-checks lastOpen inside the database write so concurrent syncs can't move the cursor backward. Addresses CodeRabbit review on #7506.
Stands up the real WatermelonDB JS core (writer lock, db.write serialization, queries) over an in-memory LokiJSAdapter and the production appSchema, mocking only sdk. Ships a shared fake >=7.1 chat.syncMessages server (paginated UPDATED/DELETED cursors, failure injection) and seed/reset helpers for the sync-cursor integration suites on PR #7506.
Add advanceSyncCursor.test.ts (9 tests) asserting the real persisted subscription.lastOpen on the LokiJS in-memory WatermelonDB harness, with only log mocked and the real getSubscriptionByRoomId running against the live DB. Covers max-of-batch, forward-only (older + equal), empty batch, all-missing _updatedAt, missing-skipped-without-poisoning, no-subscription clean return, and error swallow+log. The re-read-inside-write guard is exercised deterministically against the real writer lock: a gated concurrent db.write holds the lock while advanceSyncCursor reads the stale cursor and queues behind it, then commits a higher timestamp so the in-flight write must skip its stale value rather than regress. Mutating the guard to an unconditional set fails this test. Also fix 3 pre-existing tsc errors in the harness (optional Relation access and SubscriptionType cast) so the project tsc gate passes.
Cover the !args.latest gate in loadMessagesForRoom: newest load advances the sync cursor to the window max; a scroll-up window (latest set) never advances it; and the edited-old-message defense keeps the cursor pinned when scroll-up surfaces an old message carrying a recent _updatedAt. Real persisted messages + real subscription.lastOpen on the LokiJS harness; only sdk/auxStore/ encryption/log mocked. Gate proven load-bearing by mutation.
The LokiJS integration harness left the in-memory driver open, so a full
`jest --runInBand` (the CI command) hung after a green run ("Jest did not
exit one second after the test run has completed"). Add closeLokiTestDatabase
— reaching the in-process driver the same way the adapter's own testClone does
(_dispatcher._worker._bridge.driver) — and call it from afterAll in every suite
that builds a database. The real CI command now exits on its own; no
--forceExit needed.
Also exclude the harness helpers (lokiTestDatabase.ts, fakeSyncServer.ts) from
jest collection via testPathIgnorePatterns, matching the existing
testHelpers.tsx entry. jest-expo's testMatch globs every __tests__/**/*, so
these non-test helpers failed the full suite with "Your test suite must
contain at least one test".
The re-read-guard test deliberately queues a db.write behind a running writer. In non-production builds WatermelonDB's WorkQueue.enqueue arms a ~1.5s setTimeout to warn about a possibly stuck queue whenever a write is enqueued behind an in-flight one. That live timer outlives Jest's 1s post-run window and prints the benign "Jest did not exit one second after the test run has completed" warning on the advanceSyncCursor suite. Wrap the contended section in withWriterQueueDiagnosticCleared, which captures and clears that timer, so the event loop drains and the suite exits cleanly without --forceExit. No assertion or behavior changed.
babel-jest hoists jest.mock() to the top of the module regardless of source position (the factories only close over mock-prefixed vars), so the real imports never needed to sit below the mocks behind // eslint-disable-next-line import/first. Move them into the normal top import block and place the jest.mock() calls after, removing the disable comments and the import/order 'empty line within import group' nit. No behavior change; mocking still applies identically.
…B suites loadMissedMessages.test.ts is entirely superseded — its 5 cases now have mutation-proven real-DB equivalents in loadMissedMessages.integration.test.ts (paginated deletions, no-advance-on-failure) and reconnectCycle.integration.test.ts (offline coverage-gap, clock skew, backgrounded delivery). Delete the file. subscriptions/room.test.ts keeps its subscribe/handleLogin/handleClose/DDP-recovery/ isAlive coverage but drops the lone updateMessage-concurrency case: the mock cannot take the WMDB writer lock and only models a hand-rolled "pending changes" string, so room.integration.test.ts supersedes it with the real prepareUpdate diagnostic.
GitHub Actions
Inactive
GitHub Actions
Inactive
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
app/lib/database/__tests__/advanceSyncCursor.test.ts (1)
17-49: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the repeated Loki-integration-test scaffolding into
lokiTestDatabase.ts. All four new integration test files re-declare the identicalmockActiveDatabase+jest.mock('../../database', ...)getter-factory, and three of them additionally duplicate the sameT0..T5time constants andpersistedLastOpenhelper verbatim. One shared root cause: no common test-utility module for this boilerplate, even thoughlokiTestDatabase.tsalready hosts the sibling shared helpers (createLokiTestDatabase,seedSubscription, etc.).
app/lib/database/__tests__/advanceSyncCursor.test.ts#L17-L49: move thejest.mock('../../database', ...)factory pattern,T0..T5constants, andpersistedLastOpenhelper intolokiTestDatabase.tsand import them here.app/lib/database/__tests__/loadMessagesForRoom.integration.test.ts#L14-L74: replace the duplicated factory/constants/persistedLastOpenwith the shared import.app/lib/database/__tests__/loadMissedMessages.integration.test.ts#L20-L75: replace the duplicated factory/constants/persistedLastOpenwith the shared import.app/lib/database/__tests__/room.integration.test.ts#L22-L62: replace the duplicatedjest.mock('../../database', ...)factory with the shared import (itsT1/T2constants are a smaller, differently-scoped variant of the same pattern and can be left local or folded in at the author's discretion).🤖 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/database/__tests__/advanceSyncCursor.test.ts` around lines 17 - 49, Extract the repeated Loki test scaffolding into lokiTestDatabase.ts. In app/lib/database/__tests__/advanceSyncCursor.test.ts#L17-L49, move the mockActiveDatabase database mock, T0–T5 constants, and persistedLastOpen helper, then import them; apply the same replacement in app/lib/database/__tests__/loadMessagesForRoom.integration.test.ts#L14-L74 and app/lib/database/__tests__/loadMissedMessages.integration.test.ts#L20-L75. In app/lib/database/__tests__/room.integration.test.ts#L22-L62, replace only the duplicated database mock with the shared utility, leaving its local T1/T2 constants or folding them into the shared constants as appropriate.app/lib/database/__tests__/lokiTestDatabase.ts (1)
74-84: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueGuard
closeLokiTestDatabaseagainst adapter layout changes.The helper currently relies on private WatermelonDB internals (
_dispatcher._worker._bridge.driver.loki) that are not exposed by the LokiJS adapter API, so an upgrade can silently break cleanup. Keep this code behind a targeted compatibility fallback or a documented internal-api boundary if this cleanup path must remain.🤖 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/database/__tests__/lokiTestDatabase.ts` around lines 74 - 84, Update closeLokiTestDatabase to isolate the private WatermelonDB adapter traversal behind a targeted, documented compatibility fallback, and preserve the no-op behavior when Loki’s close method is unavailable. Avoid depending directly on the internal _dispatcher/_worker/_bridge/driver.loki layout without explicitly marking or containing that internal-API boundary.
🤖 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/loadMissedMessages.ts`:
- Around line 88-125: Replace full message accumulation in syncPages with only
the _updatedAt values needed by advanceSyncCursor, and update
advanceSyncCursor’s parameter type to accept this lightweight shape. Preserve
timestamp collection for both updated and deleted records and pass the
lightweight accumulator through loadMissedMessages without retaining complete
payloads.
---
Nitpick comments:
In `@app/lib/database/__tests__/advanceSyncCursor.test.ts`:
- Around line 17-49: Extract the repeated Loki test scaffolding into
lokiTestDatabase.ts. In
app/lib/database/__tests__/advanceSyncCursor.test.ts#L17-L49, move the
mockActiveDatabase database mock, T0–T5 constants, and persistedLastOpen helper,
then import them; apply the same replacement in
app/lib/database/__tests__/loadMessagesForRoom.integration.test.ts#L14-L74 and
app/lib/database/__tests__/loadMissedMessages.integration.test.ts#L20-L75. In
app/lib/database/__tests__/room.integration.test.ts#L22-L62, replace only the
duplicated database mock with the shared utility, leaving its local T1/T2
constants or folding them into the shared constants as appropriate.
In `@app/lib/database/__tests__/lokiTestDatabase.ts`:
- Around line 74-84: Update closeLokiTestDatabase to isolate the private
WatermelonDB adapter traversal behind a targeted, documented compatibility
fallback, and preserve the no-op behavior when Loki’s close method is
unavailable. Avoid depending directly on the internal
_dispatcher/_worker/_bridge/driver.loki layout without explicitly marking or
containing that internal-API boundary.
🪄 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: edbf7a4b-8187-4df5-8e7a-ca15f2f660ce
📒 Files selected for processing (17)
app/lib/database/__tests__/advanceSyncCursor.test.tsapp/lib/database/__tests__/fakeSyncServer.tsapp/lib/database/__tests__/loadMessagesForRoom.integration.test.tsapp/lib/database/__tests__/loadMissedMessages.integration.test.tsapp/lib/database/__tests__/lokiTestDatabase.test.tsapp/lib/database/__tests__/lokiTestDatabase.tsapp/lib/database/__tests__/reconnectCycle.integration.test.tsapp/lib/database/__tests__/room.integration.test.tsapp/lib/methods/helpers/advanceSyncCursor.tsapp/lib/methods/loadMessagesForRoom.tsapp/lib/methods/loadMissedMessages.tsapp/lib/methods/readMessages.tsapp/lib/methods/subscriptions/room.test.tsapp/lib/methods/subscriptions/room.tsapp/lib/methods/updateLastOpen.tsapp/views/RoomView/index.tsxjest.config.js
💤 Files with no reviewable changes (3)
- app/lib/methods/updateLastOpen.ts
- app/lib/methods/subscriptions/room.ts
- app/lib/methods/subscriptions/room.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: ESLint and Test / run-eslint-and-test
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{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/database/__tests__/advanceSyncCursor.test.tsapp/lib/database/__tests__/lokiTestDatabase.test.tsapp/views/RoomView/index.tsxapp/lib/methods/helpers/advanceSyncCursor.tsapp/lib/methods/readMessages.tsapp/lib/methods/loadMessagesForRoom.tsapp/lib/database/__tests__/fakeSyncServer.tsapp/lib/database/__tests__/room.integration.test.tsapp/lib/database/__tests__/lokiTestDatabase.tsapp/lib/database/__tests__/loadMessagesForRoom.integration.test.tsapp/lib/methods/loadMissedMessages.tsapp/lib/database/__tests__/reconnectCycle.integration.test.tsapp/lib/database/__tests__/loadMissedMessages.integration.test.tsjest.config.js
**/*.{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/database/__tests__/advanceSyncCursor.test.tsapp/lib/database/__tests__/lokiTestDatabase.test.tsapp/views/RoomView/index.tsxapp/lib/methods/helpers/advanceSyncCursor.tsapp/lib/methods/readMessages.tsapp/lib/methods/loadMessagesForRoom.tsapp/lib/database/__tests__/fakeSyncServer.tsapp/lib/database/__tests__/room.integration.test.tsapp/lib/database/__tests__/lokiTestDatabase.tsapp/lib/database/__tests__/loadMessagesForRoom.integration.test.tsapp/lib/methods/loadMissedMessages.tsapp/lib/database/__tests__/reconnectCycle.integration.test.tsapp/lib/database/__tests__/loadMissedMessages.integration.test.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/database/__tests__/advanceSyncCursor.test.tsapp/lib/database/__tests__/lokiTestDatabase.test.tsapp/views/RoomView/index.tsxapp/lib/methods/helpers/advanceSyncCursor.tsapp/lib/methods/readMessages.tsapp/lib/methods/loadMessagesForRoom.tsapp/lib/database/__tests__/fakeSyncServer.tsapp/lib/database/__tests__/room.integration.test.tsapp/lib/database/__tests__/lokiTestDatabase.tsapp/lib/database/__tests__/loadMessagesForRoom.integration.test.tsapp/lib/methods/loadMissedMessages.tsapp/lib/database/__tests__/reconnectCycle.integration.test.tsapp/lib/database/__tests__/loadMissedMessages.integration.test.tsjest.config.js
app/lib/database/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use WatermelonDB for the offline-first SQLite database; keep models under
model/and schema definitions underschema/.
Files:
app/lib/database/__tests__/advanceSyncCursor.test.tsapp/lib/database/__tests__/lokiTestDatabase.test.tsapp/lib/database/__tests__/fakeSyncServer.tsapp/lib/database/__tests__/room.integration.test.tsapp/lib/database/__tests__/lokiTestDatabase.tsapp/lib/database/__tests__/loadMessagesForRoom.integration.test.tsapp/lib/database/__tests__/reconnectCycle.integration.test.tsapp/lib/database/__tests__/loadMissedMessages.integration.test.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/database/__tests__/advanceSyncCursor.test.tsapp/lib/database/__tests__/lokiTestDatabase.test.tsapp/lib/database/__tests__/room.integration.test.tsapp/lib/database/__tests__/loadMessagesForRoom.integration.test.tsapp/lib/database/__tests__/reconnectCycle.integration.test.tsapp/lib/database/__tests__/loadMissedMessages.integration.test.ts
🧠 Learnings (4)
📚 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/database/__tests__/advanceSyncCursor.test.tsapp/lib/database/__tests__/lokiTestDatabase.test.tsapp/views/RoomView/index.tsxapp/lib/methods/helpers/advanceSyncCursor.tsapp/lib/methods/readMessages.tsapp/lib/methods/loadMessagesForRoom.tsapp/lib/database/__tests__/fakeSyncServer.tsapp/lib/database/__tests__/room.integration.test.tsapp/lib/database/__tests__/lokiTestDatabase.tsapp/lib/database/__tests__/loadMessagesForRoom.integration.test.tsapp/lib/methods/loadMissedMessages.tsapp/lib/database/__tests__/reconnectCycle.integration.test.tsapp/lib/database/__tests__/loadMissedMessages.integration.test.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/database/__tests__/advanceSyncCursor.test.tsapp/lib/database/__tests__/lokiTestDatabase.test.tsapp/lib/database/__tests__/room.integration.test.tsapp/lib/database/__tests__/loadMessagesForRoom.integration.test.tsapp/lib/database/__tests__/reconnectCycle.integration.test.tsapp/lib/database/__tests__/loadMissedMessages.integration.test.ts
📚 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/RoomView/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/RoomView/index.tsx
🔇 Additional comments (14)
app/views/RoomView/index.tsx (1)
681-695: LGTM!app/lib/methods/readMessages.ts (1)
8-8: LGTM!Also applies to: 47-48
app/lib/methods/helpers/advanceSyncCursor.ts (1)
1-51: LGTM!app/lib/methods/loadMessagesForRoom.ts (1)
8-8: LGTM!Also applies to: 142-146
app/lib/methods/loadMissedMessages.ts (1)
1-3: LGTM!Also applies to: 23-37
app/lib/database/__tests__/reconnectCycle.integration.test.ts (1)
1-178: LGTM!jest.config.js (1)
3-10: LGTM!app/lib/database/__tests__/advanceSyncCursor.test.ts (1)
63-169: LGTM!app/lib/database/__tests__/loadMessagesForRoom.integration.test.ts (1)
88-132: LGTM!app/lib/database/__tests__/loadMissedMessages.integration.test.ts (1)
92-209: LGTM!app/lib/database/__tests__/lokiTestDatabase.test.ts (1)
1-70: LGTM!app/lib/database/__tests__/fakeSyncServer.ts (1)
58-117: LGTM!app/lib/database/__tests__/room.integration.test.ts (2)
139-153: 🎯 Functional Correctness | ⚡ Quick winVerify
database.batch()call shape matches this test's variadic assumption.
batchSpy.mock.calls[0]having length 3 only holds ifRoomSubscription.updateMessagecallsdatabase.batch(record1, record2, record3)(spread). WatermelonDB'sbatch()also accepts a single array argument (database.batch([...])), which would make this call have length 1 instead and fail the assertion. Sinceroom.tsisn't in this review batch, please confirm the implementation's actual call shape.
106-137: LGTM!Also applies to: 155-178
GitHub Actions
Error
|
Android Build Available Rocket.Chat 4.75.0.109407 Internal App Sharing: https://play.google.com/apps/test/RQQ8k09hlnQ/ahAO29uNQqol2SsVOluLO5EpoO1wg9CTGmTd49pYxM4x0U8gjgvswHkEAJdXDBwUAtg3Q1C24osS9-lEqej6E-3LWj |
|
iOS Build Available Rocket.Chat 4.75.0.109408 |
A subscription whose lastOpen is unset — typically a room reached by tapping a push notification — resolved an undefined chat.syncMessages cursor, so loadMissedMessages issued no requests and resolved as success. Nothing was ever backfilled. getLastUpdate now resolves the cursor from the first candidate that is a real point in time: lastOpen, then the newest server-stamped message this device holds, then ls, then ts. ls is written by every device, so it can sit past messages this one never received; the newest persisted message reconstructs what advanceSyncCursor would have stored. Rows stamped from the device clock — synthetic load-more sentinels and unsent TEMP/ERROR sends — are excluded, since taking one as the cursor would ask the server for changes newer than a clock that may run ahead. loadMessagesForRoom keeps the synthetic load-more row out of the array handed to advanceSyncCursor for the same reason: normalizeMessage stamps its missing _updatedAt with new Date(). Covered by LokiJS integration suites asserting the recovered message row, not just the cursor value.
GitHub Actions
Error
GitHub Actions
Error
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
app/lib/database/__tests__/cursorClockPoisoning.integration.test.ts (1)
49-52: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor: prefer an
interfaceover atypealias for this object shape.As per coding guidelines,
**/*.{ts,tsx}should "Prefer interfaces over type aliases for defining object shapes in TypeScript."TSdkGetRouterdescribes an object shape and could be aninterfaceinstead.🤖 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/database/__tests__/cursorClockPoisoning.integration.test.ts` around lines 49 - 52, Change TSdkGetRouter from a type alias to an interface while preserving its mockImplementation method signature and parameter types.Source: Coding guidelines
app/lib/database/services/Message.ts (1)
32-45: 🚀 Performance & Scalability | 🔵 TrivialVerify a composite index backs this
rid+_updated_at DESC+takequery.This query now runs on the reconnect/resume hot path (via
loadMissedMessages→getLastUpdate) for potentially many rooms. WatermelonDB supportsQ.sortBy/Q.takeon the SQLite adapter, but doesn't auto-create composite indexes — those need to be declared in the schema. Without an index covering(rid, _updated_at), SQLite will scan and sort all of a room's messages before limiting toNEWEST_MESSAGE_SCAN.Please confirm the
messagesschema (not included in this review) has an appropriate index.🤖 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/database/services/Message.ts` around lines 32 - 45, Ensure the messages schema declares a composite index covering rid and _updated_at, with ordering appropriate for the getNewestMessageUpdatedAt query’s Q.where, Q.sortBy descending, and Q.take usage. Update the schema definition rather than changing getNewestMessageUpdatedAt, and preserve the existing query behavior.
🤖 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/loadMissedMessages.ts`:
- Around line 45-53: Update getLastUpdate so it validates sub.lastOpen first and
returns it immediately when it is a valid positive-date cursor; only call
getNewestMessageUpdatedAt(rid) when lastOpen is absent or invalid, then preserve
the existing fallback evaluation using sub.ls and sub.ts.
---
Nitpick comments:
In `@app/lib/database/__tests__/cursorClockPoisoning.integration.test.ts`:
- Around line 49-52: Change TSdkGetRouter from a type alias to an interface
while preserving its mockImplementation method signature and parameter types.
In `@app/lib/database/services/Message.ts`:
- Around line 32-45: Ensure the messages schema declares a composite index
covering rid and _updated_at, with ordering appropriate for the
getNewestMessageUpdatedAt query’s Q.where, Q.sortBy descending, and Q.take
usage. Update the schema definition rather than changing
getNewestMessageUpdatedAt, and preserve the existing query 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: 4fbde26b-4968-4032-8f36-00977c084495
📒 Files selected for processing (8)
app/lib/database/__tests__/advanceSyncCursor.test.tsapp/lib/database/__tests__/cursorClockPoisoning.integration.test.tsapp/lib/database/__tests__/lokiTestDatabase.tsapp/lib/database/__tests__/nullCursorRecurrence.integration.test.tsapp/lib/database/services/Message.tsapp/lib/methods/helpers/advanceSyncCursor.tsapp/lib/methods/loadMessagesForRoom.tsapp/lib/methods/loadMissedMessages.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- app/lib/database/tests/advanceSyncCursor.test.ts
- app/lib/database/tests/lokiTestDatabase.ts
- app/lib/methods/helpers/advanceSyncCursor.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: ESLint and Test / run-eslint-and-test
- GitHub Check: format
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{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/database/services/Message.tsapp/lib/database/__tests__/cursorClockPoisoning.integration.test.tsapp/lib/methods/loadMessagesForRoom.tsapp/lib/database/__tests__/nullCursorRecurrence.integration.test.tsapp/lib/methods/loadMissedMessages.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/database/services/Message.tsapp/lib/database/__tests__/cursorClockPoisoning.integration.test.tsapp/lib/methods/loadMessagesForRoom.tsapp/lib/database/__tests__/nullCursorRecurrence.integration.test.tsapp/lib/methods/loadMissedMessages.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/database/services/Message.tsapp/lib/database/__tests__/cursorClockPoisoning.integration.test.tsapp/lib/methods/loadMessagesForRoom.tsapp/lib/database/__tests__/nullCursorRecurrence.integration.test.tsapp/lib/methods/loadMissedMessages.ts
app/lib/database/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use WatermelonDB for the offline-first SQLite database; keep models under
model/and schema definitions underschema/.
Files:
app/lib/database/services/Message.tsapp/lib/database/__tests__/cursorClockPoisoning.integration.test.tsapp/lib/database/__tests__/nullCursorRecurrence.integration.test.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/database/__tests__/cursorClockPoisoning.integration.test.tsapp/lib/database/__tests__/nullCursorRecurrence.integration.test.ts
🧠 Learnings (2)
📚 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/database/services/Message.tsapp/lib/database/__tests__/cursorClockPoisoning.integration.test.tsapp/lib/methods/loadMessagesForRoom.tsapp/lib/database/__tests__/nullCursorRecurrence.integration.test.tsapp/lib/methods/loadMissedMessages.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/database/__tests__/cursorClockPoisoning.integration.test.tsapp/lib/database/__tests__/nullCursorRecurrence.integration.test.ts
🔇 Additional comments (3)
app/lib/methods/loadMessagesForRoom.ts (1)
8-8: LGTM!Also applies to: 132-150
app/lib/database/__tests__/cursorClockPoisoning.integration.test.ts (1)
1-48: LGTM!Also applies to: 53-190
app/lib/database/__tests__/nullCursorRecurrence.integration.test.ts (1)
1-195: LGTM!
| const getLastUpdate = async (rid: string): Promise<Date | null> => { | ||
| const sub = await getSubscriptionByRoomId(rid); | ||
| if (!sub) { | ||
| return null; | ||
| } | ||
| return sub.lastOpen; | ||
| const candidates = [sub.lastOpen, await getNewestMessageUpdatedAt(rid), sub.ls, sub.ts]; | ||
| const cursor = candidates.find(candidate => candidate && new Date(candidate).getTime() > 0); | ||
| return cursor ? new Date(cursor) : null; | ||
| }; |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick win
Unconditional getNewestMessageUpdatedAt query even when lastOpen is already valid.
The candidates array literal evaluates await getNewestMessageUpdatedAt(rid) unconditionally before .find() runs, so this extra WatermelonDB scan/sort executes even when sub.lastOpen is already a valid cursor — the common case for any room that's been opened before. Since this runs per-room during the resume/resubscribe flow, it's an avoidable DB round-trip on a hot path.
⚡ Proposed fix: short-circuit before the async fallback
const getLastUpdate = async (rid: string): Promise<Date | null> => {
const sub = await getSubscriptionByRoomId(rid);
if (!sub) {
return null;
}
- const candidates = [sub.lastOpen, await getNewestMessageUpdatedAt(rid), sub.ls, sub.ts];
- const cursor = candidates.find(candidate => candidate && new Date(candidate).getTime() > 0);
- return cursor ? new Date(cursor) : null;
+ const isValidCursor = (candidate?: Date | string | number | null) => !!candidate && new Date(candidate).getTime() > 0;
+ if (isValidCursor(sub.lastOpen)) {
+ return new Date(sub.lastOpen as Date);
+ }
+ const cursor = [await getNewestMessageUpdatedAt(rid), sub.ls, sub.ts].find(isValidCursor);
+ return cursor ? new Date(cursor) : null;
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const getLastUpdate = async (rid: string): Promise<Date | null> => { | |
| const sub = await getSubscriptionByRoomId(rid); | |
| if (!sub) { | |
| return null; | |
| } | |
| return sub.lastOpen; | |
| const candidates = [sub.lastOpen, await getNewestMessageUpdatedAt(rid), sub.ls, sub.ts]; | |
| const cursor = candidates.find(candidate => candidate && new Date(candidate).getTime() > 0); | |
| return cursor ? new Date(cursor) : null; | |
| }; | |
| const getLastUpdate = async (rid: string): Promise<Date | null> => { | |
| const sub = await getSubscriptionByRoomId(rid); | |
| if (!sub) { | |
| return null; | |
| } | |
| const isValidCursor = (candidate?: Date | string | number | null) => | |
| !!candidate && new Date(candidate).getTime() > 0; | |
| if (isValidCursor(sub.lastOpen)) { | |
| return new Date(sub.lastOpen as Date); | |
| } | |
| const cursor = [await getNewestMessageUpdatedAt(rid), sub.ls, sub.ts].find(isValidCursor); | |
| return cursor ? new Date(cursor) : null; | |
| }; |
🤖 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/loadMissedMessages.ts` around lines 45 - 53, Update
getLastUpdate so it validates sub.lastOpen first and returns it immediately when
it is a valid positive-date cursor; only call getNewestMessageUpdatedAt(rid)
when lastOpen is absent or invalid, then preserve the existing fallback
evaluation using sub.ls and sub.ts.
The subscription idempotency key used a literal NUL byte as its separator, which put a raw 0x00 into patches/@rocket.chat+sdk+1.3.3-mobile.patch. Git classified the file as binary, so the ~130 SDK lines that are the substance of this change - the dedup guard, the _pendingSubscriptions claim, the subscribeAll replay - rendered as "Bin 7546 -> 12177 bytes" and could not be read in any diff view. Write the separator as a unicode escape instead. The runtime key is the same character, so subscription identity is unchanged. Also mark *.patch as text with an explicit diff attribute, so a future control byte in a patch cannot hide its contents again.
GitHub Actions
Error
GitHub Actions
Error
An unbounded awaited page walk in `syncPages` met an uncapped 300ms init retry in
RoomView, which together could hold ~13-70 requests/second indefinitely.
- bound the `chat.syncMessages` chain with MAX_PAGES, mirroring
loadMessagesForRoom's MAX_BATCHES; the next sync resumes from the persisted cursor
- thread a running max `_updatedAt` through `syncPages` instead of accumulating
every page's full message payload for the whole chain
- back the RoomView init retry off exponentially (300ms doubling) and give up after
five attempts; the budget is restored once an init succeeds
- feed only `updated` to `advanceSyncCursor` and retype `deleted` as
`{ _id, _deletedAt }[]`, matching what the server projection actually carries, so
the cursor's independence from deletion times is structural, not incidental
The page bound made the persisted cursor wrong whenever truncation hit the DELETED
stream: only the UPDATED max was carried out, so a truncated DELETED walk either
livelocked (deletions with no updates left the cursor unmoved and every later sync
re-walked the same ten pages) or skipped deletions between the abandoned stop-point
and a further-along updated max.
- `syncPages` now returns `{ highestUpdatedAt, deletedStop }` and the cursor resolves
to the lower of the two live stop-points; both streams filter exclusively, so that
costs a redundant refetch, never a record
- the fake sync server models deleted records the way the server projects them —
`{ _id, _deletedAt }`, paginated and sorted on `_deletedAt`
- the RoomView init retry keeps the exponential backoff but drops the attempt cap and
settles at the 10s floor: nothing else re-enters init() for an authenticated room, so
giving up left the room blank with no retry and no error affordance
- `syncPages` takes an options object, `advanceSyncCursor`'s parameter says
`serverLatest`, and the stale @ts-ignore reason states the real mismatch
A room that keeps failing now retries forever at the 10s floor, so the failure left no record at all. Record the first failure since the last successful init and stay silent for the rest of the loop. Also: `advanceSyncCursor`'s contract note admits a `_deletedAt` stop-point, the exhausted-stream check reads `deletedStop === null`, and the fake sync server states that its DELETED cursor is the intended contract rather than the shipped server's.
GitHub Actions
Waiting
GitHub Actions
Waiting
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
app/lib/database/__tests__/fakeSyncServer.ts (1)
91-96: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd explicit signatures to the local helpers.
paginate,handleSyncMessages, andinstallOnrely on inferred/contextual return and parameter types. Declare them explicitly to satisfy the TypeScript guideline.As per coding guidelines, “add explicit type annotations to function parameters and return types.”
Also applies to: 114-130, 132-139
🤖 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/database/__tests__/fakeSyncServer.ts` around lines 91 - 96, Update the local helpers paginate, handleSyncMessages, and installOn with explicit type annotations for every parameter and their return types, replacing reliance on inference or contextual typing. Preserve their existing behavior and use the surrounding types and return shapes already established in fakeSyncServer.Source: Coding guidelines
🤖 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/database/__tests__/fakeSyncServer.ts`:
- Around line 12-15: Update the DELETED behavior in the fake sync server and its
associated documentation so it does not present `_deletedAt`-based cursors as
the supported production contract. Either reproduce mountCursorQuery’s deployed
ordering after projection or explicitly mark the fake as intentionally
divergent, and ensure tests avoid asserting unsupported DELETED page ordering or
cursor semantics.
In `@app/views/RoomView/index.tsx`:
- Line 383: Update the RoomView initialization retry flow around init() and the
cleanup containing this.initRetry.cancel() so async success and catch paths
first verify this.mounted (or an equivalent lifecycle generation token) before
updating state or scheduling retries. Ensure an in-flight init rejection after
unmount cannot retain RoomView or trigger further network work, while preserving
normal mounted retry behavior.
---
Nitpick comments:
In `@app/lib/database/__tests__/fakeSyncServer.ts`:
- Around line 91-96: Update the local helpers paginate, handleSyncMessages, and
installOn with explicit type annotations for every parameter and their return
types, replacing reliance on inference or contextual typing. Preserve their
existing behavior and use the surrounding types and return shapes already
established in fakeSyncServer.
🪄 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: 60fb9d5e-96c8-4584-9510-21b835a2af55
📒 Files selected for processing (11)
app/lib/database/__tests__/advanceSyncCursor.test.tsapp/lib/database/__tests__/fakeSyncServer.tsapp/lib/database/__tests__/loadMissedMessages.integration.test.tsapp/lib/database/__tests__/lokiTestDatabase.test.tsapp/lib/database/__tests__/nullCursorRecurrence.integration.test.tsapp/lib/methods/helpers/advanceSyncCursor.tsapp/lib/methods/loadMessagesForRoom.tsapp/lib/methods/loadMissedMessages.tsapp/views/RoomView/index.tsxapp/views/RoomView/services/initRetryScheduler.test.tsapp/views/RoomView/services/initRetryScheduler.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- app/lib/methods/loadMessagesForRoom.ts
- app/lib/database/tests/lokiTestDatabase.test.ts
- app/lib/database/tests/advanceSyncCursor.test.ts
- app/lib/methods/loadMissedMessages.ts
- app/lib/database/tests/nullCursorRecurrence.integration.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: ESLint and Test / run-eslint-and-test
- GitHub Check: format
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{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/views/RoomView/services/initRetryScheduler.tsapp/lib/methods/helpers/advanceSyncCursor.tsapp/views/RoomView/services/initRetryScheduler.test.tsapp/views/RoomView/index.tsxapp/lib/database/__tests__/fakeSyncServer.tsapp/lib/database/__tests__/loadMissedMessages.integration.test.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/views/RoomView/services/initRetryScheduler.tsapp/lib/methods/helpers/advanceSyncCursor.tsapp/views/RoomView/services/initRetryScheduler.test.tsapp/views/RoomView/index.tsxapp/lib/database/__tests__/fakeSyncServer.tsapp/lib/database/__tests__/loadMissedMessages.integration.test.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/views/RoomView/services/initRetryScheduler.tsapp/lib/methods/helpers/advanceSyncCursor.tsapp/views/RoomView/services/initRetryScheduler.test.tsapp/views/RoomView/index.tsxapp/lib/database/__tests__/fakeSyncServer.tsapp/lib/database/__tests__/loadMissedMessages.integration.test.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/views/RoomView/services/initRetryScheduler.test.tsapp/lib/database/__tests__/loadMissedMessages.integration.test.ts
app/lib/database/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use WatermelonDB for the offline-first SQLite database; keep models under
model/and schema definitions underschema/.
Files:
app/lib/database/__tests__/fakeSyncServer.tsapp/lib/database/__tests__/loadMissedMessages.integration.test.ts
🧠 Learnings (4)
📚 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/views/RoomView/services/initRetryScheduler.tsapp/lib/methods/helpers/advanceSyncCursor.tsapp/views/RoomView/services/initRetryScheduler.test.tsapp/views/RoomView/index.tsxapp/lib/database/__tests__/fakeSyncServer.tsapp/lib/database/__tests__/loadMissedMessages.integration.test.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/views/RoomView/services/initRetryScheduler.test.tsapp/lib/database/__tests__/loadMissedMessages.integration.test.ts
📚 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/RoomView/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/RoomView/index.tsx
🔇 Additional comments (9)
app/views/RoomView/index.tsx (2)
118-118: LGTM!Also applies to: 138-138, 680-680
694-694: 🗄️ Data Integrity & IntegrationVerify the dropped
readMessagesargument against its new API.This changes the call from
(room.rid, readAt, true)to(room.rid, readAt). Confirm the new default preserves the intended read/cursor behavior and that all other callers use the updated contract.app/views/RoomView/services/initRetryScheduler.test.ts (2)
1-94: LGTM!
1-94: 📐 Maintainability & Code QualityRun this Jest file with
TZ=UTC.This file matches
**/*.test.ts; execute it under UTC as required by the repository guidelines.Source: Coding guidelines
app/views/RoomView/services/initRetryScheduler.ts (1)
1-36: LGTM!app/lib/methods/helpers/advanceSyncCursor.ts (1)
6-18: LGTM!Also applies to: 20-53
app/lib/database/__tests__/loadMissedMessages.integration.test.ts (2)
97-234: 📐 Maintainability & Code QualityRun this integration suite with
TZ=UTC.The repository test guideline requires deterministic timezone execution for
*.test.tsfiles.As per coding guidelines, “Run Jest tests with
TZ=UTCto ensure deterministic timezone-dependent test behavior.”Source: Coding guidelines
196-198: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRemove the second
map.Line 197 destructures the params object emitted by the first
mapas[, params], causing a runtime “not iterable” failure before the cursor assertion.Proposed fix
const deletedCursors = mockedSdkGet.mock.calls .map(([, params]) => params as unknown as { type?: string; next?: number }) - .map(([, params]) => params as unknown as { type?: string; next?: number }) .filter(params => params.type === 'DELETED') .map(params => params.next);> Likely an incorrect or invalid review comment.app/lib/database/__tests__/fakeSyncServer.ts (1)
37-40: 🎯 Functional CorrectnessManual verification needed for the duplicate exported type aliases.
The repository contents could not be inspected in this environment to determine whether
TFakeServerMessageWireorTFakeServerDeletedWireare declared multiple times inapp/lib/database/__tests__/fakeSyncServer.ts.
| * This models the DELETED cursor as intended, not as shipped: the server's | ||
| * `mountCursorQuery` sorts unconditionally by `_updatedAt`, which the DELETED projection | ||
| * strips, so its real `next` is not guaranteed to be the page's max `_deletedAt`. Do not | ||
| * read this fake as ground truth for that stream's ordering. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Files:"
fd -a 'fakeSyncServer\.ts$|test.*Sync|.*Sync.*test.*|.*sync.*' app | sed 's#^\./##' | head -100
echo
echo "fakeSyncServer outline:"
if [ -f app/lib/database/__tests__/fakeSyncServer.ts ]; then
wc -l app/lib/database/__tests__/fakeSyncServer.ts
ast-grep outline app/lib/database/__tests__/fakeSyncServer.ts || true
echo
echo "fakeSyncServer relevant lines 1-180:"
cat -n app/lib/database/__tests__/fakeSyncServer.ts | sed -n '1,180p'
fi
echo
echo "Search for mountCursorQuery / DELETED / _deletedAt / _updatedAt in tests/helpers:"
rg -n "mountCursorQuery|DELETED|_deletedAt|_updatedAt|nextCursor|cursor" app/lib/database/__tests__ app -g '*.{ts,tsx,js,jsx}' | head -250Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 2071
Use the supported DELETED cursor contract in the fake.
The fake emits DELETED next cursors from _deletedAt, but the deployed mountCursorQuery sorts DELETED entries by _updatedAt after the projection strips that field. Make this fake either mirror the production ordering behavior or mark it intentionally divergent so tests don’t rely on unsupported page ordering/cursor semantics.
🤖 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/database/__tests__/fakeSyncServer.ts` around lines 12 - 15, Update
the DELETED behavior in the fake sync server and its associated documentation so
it does not present `_deletedAt`-based cursors as the supported production
contract. Either reproduce mountCursorQuery’s deployed ordering after projection
or explicitly mark the fake as intentionally divergent, and ensure tests avoid
asserting unsupported DELETED page ordering or cursor semantics.
| if (this.retryInitTimeout) { | ||
| clearTimeout(this.retryInitTimeout); | ||
| } | ||
| this.initRetry.cancel(); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Prevent retries from surviving unmount.
cancel() only removes a timer that has not fired. If an in-flight init() rejects after unmount, this catch still updates state and schedules another retry, retaining the dead RoomView and continuing network work. Guard the post-await success path and catch with this.mounted or a lifecycle generation token.
Suggested guard
const member = await this.getRoomMember();
+if (!this.mounted) {
+ return;
+}
this.setState({ canAutoTranslate, member, loading: false });
this.initRetry.reset();
} catch (e) {
+if (!this.mounted) {
+ return;
+}
this.setState({ loading: false });Also applies to: 702-709
🤖 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/RoomView/index.tsx` at line 383, Update the RoomView initialization
retry flow around init() and the cleanup containing this.initRetry.cancel() so
async success and catch paths first verify this.mounted (or an equivalent
lifecycle generation token) before updating state or scheduling retries. Ensure
an in-flight init rejection after unmount cannot retain RoomView or trigger
further network work, while preserving normal mounted retry behavior.
1 participant
Proposed changes
A failed resume-login could leave the app with a live DDP socket that is connected but not authenticated and holding zero subscriptions — a Zombie Connection. In that state streams never come back until the app is restarted: no new messages, no room-list updates.
Recovery now lives in the SDK's own resubscribe path plus two small app-side gates, with no separate restoration owner:
forceReopenno longer wipes the subscription registry, so the next login replays every subscription viasubscribeAll(). A synchronous(name, params)idempotency guard dedups a replayed subscription against one the app re-issues, and eager-send entries are normalized withonEvent.connectedstream listener early-returns only when the session is already authenticated, so a Zombie Connection re-runs resume-login instead of stranding.handleLogin; the fresh DDP session already dropped them server-side and the SDK dedups the re-subscribe.The reported symptom is two separate defects
Verifying the original report on an Android emulator showed that the user-visible "message is missing until I clear the cache" symptom is caused by two independent bugs, and this PR fixes only the first:
advanceSyncCursorchange in fix: derive message sync cursor from server timestamps #7506.With this PR alone the connection recovers but the missed message stays missing. With #7506 underneath it, the reported repro stops reproducing — the missed message arrives shortly after foreground, exactly once, in correct chronological order. This PR is therefore stacked on #7506 and the two are meant to land together.
Note for installs that already hit the bug
#7506 prevents new gaps; it does not repair gaps that already exist locally. Rooms that were gapped before upgrading split in two:
lastOpenis unset, simply opening the room refetches the missing messages and the room heals.lastOpenwas already stamped past the missing window, nothing in the app will re-request it — neither re-opening the room nor restarting the app heals it. Those users keep the hole until local storage is cleared.No migration is included to repair the second case.
Issue(s)
How to test or reproduce
Screenshots
N/A — no UI changes.
Types of changes
Checklist
Further comments
The fix is deliberately subtractive: rather than adding a restoration owner that races the SDK, it lets the SDK own resubscribe and removes the app-side code that was fighting it. The one addition is the idempotency guard that makes replay safe against a concurrent app re-subscribe.
This PR is stacked on #7506, so its base is that branch rather than
develop; review and merge it after #7506.Summary by CodeRabbit