fix: derive message sync cursor from server timestamps#7506
fix: derive message sync cursor from server timestamps#7506diegolmello wants to merge 13 commits into
Conversation
…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.
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
📜 Recent review details⏰ Context from checks skipped due to timeout. (2)
WalkthroughRoom synchronization now tracks server-derived ChangesSync cursor flow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant RoomView
participant loadMissedMessages
participant syncMessages
participant Database
RoomView->>loadMissedMessages: open room and request missed messages
loadMissedMessages->>syncMessages: request UPDATED and DELETED pages
syncMessages-->>loadMissedMessages: return messages and cursors
loadMissedMessages->>Database: persist updates and removals
loadMissedMessages->>Database: advance lastOpen from _updatedAt
RoomView->>Database: persist client read state without changing lastOpen
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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.
Actionable comments posted: 2
🧹 Nitpick comments (1)
app/lib/methods/loadMissedMessages.test.ts (1)
34-40: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winType the fake server and subscription fixtures.
The
any-typed subscription, callbacks, SDK parameters, and object-shapedTFakeServerMessagealias bypass the contract this regression test is meant to protect. Use interfaces for the fixture shapes and explicit parameter/return types.As per coding guidelines, “Use TypeScript for type safety; add explicit type annotations to function parameters and return types” and “Prefer interfaces over type aliases for defining object shapes in TypeScript.”
Also applies to: 64-71, 106-117
🤖 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.test.ts` around lines 34 - 40, Replace the any-based mockSubscription, fake server, callbacks, SDK parameters, and object-shaped TFakeServerMessage alias with interfaces describing their fixture contracts. Add explicit parameter and return types across the affected helpers and fixture methods, including the update callback, while preserving the existing test behavior and SDK-compatible shapes.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/methods/helpers/advanceSyncCursor.ts`:
- Around line 33-41: Make cursor advancement atomic in the update callback
passed to subscription.update: re-read and compare s.lastOpen against latest
inside db.write, and assign the new Date only when latest is greater. Preserve
the existing early return optimization while ensuring concurrent transactions
cannot move lastOpen backward.
In `@app/lib/methods/loadMissedMessages.ts`:
- Around line 106-109: Update loadMissedMessages so advanceSyncCursor is not
called before recursive DELETED/UPDATED pagination completes. Accumulate the
updated and deleted records across the entire successful pagination chain, then
advance the shared cursor once after all recursive loadMissedMessages calls
succeed, preserving failure behavior so no cursor advancement occurs when a
later page fails.
---
Nitpick comments:
In `@app/lib/methods/loadMissedMessages.test.ts`:
- Around line 34-40: Replace the any-based mockSubscription, fake server,
callbacks, SDK parameters, and object-shaped TFakeServerMessage alias with
interfaces describing their fixture contracts. Add explicit parameter and return
types across the affected helpers and fixture methods, including the update
callback, while preserving the existing test behavior and SDK-compatible shapes.
🪄 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: 668e8fa7-ae6a-4973-bbf5-75e707783855
📒 Files selected for processing (9)
app/lib/methods/helpers/advanceSyncCursor.tsapp/lib/methods/loadMessagesForRoom.tsapp/lib/methods/loadMissedMessages.test.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.tsx
💤 Files with no reviewable changes (3)
- app/lib/methods/subscriptions/room.test.ts
- app/lib/methods/subscriptions/room.ts
- app/lib/methods/updateLastOpen.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 (4)
**/*.{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/advanceSyncCursor.tsapp/lib/methods/readMessages.tsapp/lib/methods/loadMissedMessages.test.tsapp/views/RoomView/index.tsxapp/lib/methods/loadMissedMessages.tsapp/lib/methods/loadMessagesForRoom.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/advanceSyncCursor.tsapp/lib/methods/readMessages.tsapp/lib/methods/loadMissedMessages.test.tsapp/views/RoomView/index.tsxapp/lib/methods/loadMissedMessages.tsapp/lib/methods/loadMessagesForRoom.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/advanceSyncCursor.tsapp/lib/methods/readMessages.tsapp/lib/methods/loadMissedMessages.test.tsapp/views/RoomView/index.tsxapp/lib/methods/loadMissedMessages.tsapp/lib/methods/loadMessagesForRoom.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/loadMissedMessages.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/methods/helpers/advanceSyncCursor.tsapp/lib/methods/readMessages.tsapp/lib/methods/loadMissedMessages.test.tsapp/views/RoomView/index.tsxapp/lib/methods/loadMissedMessages.tsapp/lib/methods/loadMessagesForRoom.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/loadMissedMessages.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 (1)
app/lib/methods/loadMissedMessages.test.ts (1)
97-185: 📐 Maintainability & Code QualityVerify this Jest suite runs in UTC.
Please run this timezone-sensitive suite with
TZ=UTCas required by the test-path guideline.As per coding guidelines, “
**/*.test.{ts,tsx,js,jsx}: Run Jest tests withTZ=UTCto ensure deterministic timezone-dependent test behavior.”Source: Coding guidelines
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.
|
iOS Build Available Rocket.Chat 4.75.0.109394 |
GitHub Actions
Inactive
GitHub Actions
Inactive
GitHub Actions
Error
|
Android Build Available Rocket.Chat 4.75.0.109395 Internal App Sharing: https://play.google.com/apps/test/RQQ8k09hlnQ/ahAO29uNSTa4w6eHWIILlkhCGbiivYmdiUhKLei29WzePz-8J2LE_ytSzGkD5kiXeST7P6oMClCVZQtAAQvyObM1dw |
|
iOS Build Available Rocket.Chat 4.75.0.109396 |
diegolmello
changed the title
GitHub Actions
Error
diegolmello
changed the title
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/lib/methods/loadMissedMessages.test.ts (1)
180-180: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the explicit async return type.
Per the TypeScript guidelines, annotate the Jest callback with
(): Promise<void>.Proposed change
- it('keeps a message received while offline: reading/leaving the room must not advance the cursor to the client clock', async () => { + it('keeps a message received while offline: reading/leaving the room must not advance the cursor to the client clock', async (): Promise<void> => {🤖 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.test.ts` at line 180, Update the Jest callback for the test beginning “keeps a message received while offline” to explicitly declare the async return type as Promise<void>, preserving the existing test behavior.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.
Nitpick comments:
In `@app/lib/methods/loadMissedMessages.test.ts`:
- Line 180: Update the Jest callback for the test beginning “keeps a message
received while offline” to explicitly declare the async return type as
Promise<void>, preserving the existing test behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 31667a08-33dd-4742-b934-528a16218685
📒 Files selected for processing (1)
app/lib/methods/loadMissedMessages.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 (4)
**/*.{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/loadMissedMessages.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/lib/methods/loadMissedMessages.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/methods/loadMissedMessages.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/methods/loadMissedMessages.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/methods/loadMissedMessages.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/methods/loadMissedMessages.test.ts
🔇 Additional comments (1)
app/lib/methods/loadMissedMessages.test.ts (1)
180-203: 📐 Maintainability & Code QualityRun this test with
TZ=UTC.The test appears timezone-independent because it uses
Date.UTC, but repository guidance requires Jest tests to be executed withTZ=UTC; please verify this test through the repository’s Jest command with that environment variable set.Source: Coding guidelines
GitHub Actions
Error
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.
GitHub Actions
Error
GitHub Actions
Error
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
app/lib/database/__tests__/fakeSyncServer.ts (1)
53-71: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd explicit return types to the helper functions.
serialize,createFakeSyncServer, andpaginaterely on inference for their return types.Suggested change
-const serialize = (message: IFakeServerMessage): TFakeServerMessageWire => ({ +const serialize = (message: IFakeServerMessage): TFakeServerMessageWire => ({ -export const createFakeSyncServer = (options: IFakeSyncServerOptions = {}) => { +export const createFakeSyncServer = (options: IFakeSyncServerOptions = {}): IFakeSyncServer => { -const paginate = (source: IFakeServerMessage[], next: number | null | undefined, pageSize: number) => { +const paginate = ( + source: IFakeServerMessage[], + next: number | null | undefined, + pageSize: number +): { page: TFakeServerMessageWire[]; nextCursor: number | null } => {As per coding guidelines, “add explicit type annotations to function parameters and return types.”
🤖 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 53 - 71, Add explicit return type annotations to the visible helper functions: serialize should return TFakeServerMessageWire, createFakeSyncServer should return IFakeSyncServer, and paginate should use an explicit return type matching its page and nextCursor result shape. Keep the existing implementations unchanged.Source: Coding guidelines
app/lib/database/__tests__/lokiTestDatabase.test.ts (1)
48-51: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the reset-harness test self-contained.
This test only proves reset works if the prior test happened to create a record first. Since every other integration test in this cohort relies on
resetLokiTestDatabaseactually clearing data between tests, it's worth having this specific test seed and assert independently, so it can't pass vacuously in isolation (e.g.--testNamePattern, watch-mode filtering).♻️ Proposed fix to make the test order-independent
it('resets between tests: previous records are gone', async () => { + await seedSubscription(database, { rid: 'room-2' }); const count = await database.get('messages').query().fetchCount(); expect(count).toBe(0); + const subCount = await database.get('subscriptions').query().fetchCount(); + expect(subCount).toBe(1); // sanity: seeded before this test's own beforeEach reset already ran });🤖 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.test.ts` around lines 48 - 51, Update the “resets between tests: previous records are gone” test to seed a message record before invoking or relying on the reset harness, then assert that the messages collection is empty afterward. Keep the test self-contained and order-independent while preserving the existing reset behavior used by the surrounding integration tests.app/lib/database/__tests__/lokiTestDatabase.ts (1)
74-84: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valuePin or recheck this private WatermelonDB close path before bumping the package.
This matches
@nozbe/watermelondb@0.28.1-0, but it relies on private adapter internals (database.adapter,_dispatcher._worker._bridge.driver.loki). A version/config change can silently drop the helper toPromise.resolve()and bring back the Jest hang it prevents, so keep an upgrade-related check or comment around this area.🤖 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 retain an upgrade-related check or focused comment documenting the pinned WatermelonDB private close path through database.adapter and _dispatcher._worker._bridge.driver.loki. Ensure future package or configuration changes prompt revalidation instead of silently falling back to Promise.resolve().
🤖 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 88-104: Update handleSyncMessages to explicitly branch only for
'DELETED' and 'UPDATED' types, preserving each stream’s existing pagination and
failure behavior. After those supported cases, throw an error for missing or
unsupported params.type values instead of treating them as updated requests.
- Around line 65-70: Update the paginate helper so each page includes all
records sharing the final record’s _updatedAt timestamp, even when this exceeds
pageSize. Calculate the boundary timestamp from the initial page, extend the
page through matching records at that timestamp, and emit nextCursor only when
records remain beyond that boundary; preserve serialize mapping and cursor
filtering.
---
Nitpick comments:
In `@app/lib/database/__tests__/fakeSyncServer.ts`:
- Around line 53-71: Add explicit return type annotations to the visible helper
functions: serialize should return TFakeServerMessageWire, createFakeSyncServer
should return IFakeSyncServer, and paginate should use an explicit return type
matching its page and nextCursor result shape. Keep the existing implementations
unchanged.
In `@app/lib/database/__tests__/lokiTestDatabase.test.ts`:
- Around line 48-51: Update the “resets between tests: previous records are
gone” test to seed a message record before invoking or relying on the reset
harness, then assert that the messages collection is empty afterward. Keep the
test self-contained and order-independent while preserving the existing reset
behavior used by the surrounding integration tests.
In `@app/lib/database/__tests__/lokiTestDatabase.ts`:
- Around line 74-84: Update closeLokiTestDatabase to retain an upgrade-related
check or focused comment documenting the pinned WatermelonDB private close path
through database.adapter and _dispatcher._worker._bridge.driver.loki. Ensure
future package or configuration changes prompt revalidation instead of silently
falling back to Promise.resolve().
🪄 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: dc08728d-0b0d-4e1c-a564-37b88962f40a
📒 Files selected for processing (9)
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.tsjest.config.js
📜 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/__tests__/lokiTestDatabase.test.tsjest.config.jsapp/lib/database/__tests__/fakeSyncServer.tsapp/lib/database/__tests__/loadMessagesForRoom.integration.test.tsapp/lib/database/__tests__/room.integration.test.tsapp/lib/database/__tests__/advanceSyncCursor.test.tsapp/lib/database/__tests__/reconnectCycle.integration.test.tsapp/lib/database/__tests__/lokiTestDatabase.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/lib/database/__tests__/lokiTestDatabase.test.tsapp/lib/database/__tests__/fakeSyncServer.tsapp/lib/database/__tests__/loadMessagesForRoom.integration.test.tsapp/lib/database/__tests__/room.integration.test.tsapp/lib/database/__tests__/advanceSyncCursor.test.tsapp/lib/database/__tests__/reconnectCycle.integration.test.tsapp/lib/database/__tests__/lokiTestDatabase.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__/lokiTestDatabase.test.tsjest.config.jsapp/lib/database/__tests__/fakeSyncServer.tsapp/lib/database/__tests__/loadMessagesForRoom.integration.test.tsapp/lib/database/__tests__/room.integration.test.tsapp/lib/database/__tests__/advanceSyncCursor.test.tsapp/lib/database/__tests__/reconnectCycle.integration.test.tsapp/lib/database/__tests__/lokiTestDatabase.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__/lokiTestDatabase.test.tsapp/lib/database/__tests__/fakeSyncServer.tsapp/lib/database/__tests__/loadMessagesForRoom.integration.test.tsapp/lib/database/__tests__/room.integration.test.tsapp/lib/database/__tests__/advanceSyncCursor.test.tsapp/lib/database/__tests__/reconnectCycle.integration.test.tsapp/lib/database/__tests__/lokiTestDatabase.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__/lokiTestDatabase.test.tsapp/lib/database/__tests__/loadMessagesForRoom.integration.test.tsapp/lib/database/__tests__/room.integration.test.tsapp/lib/database/__tests__/advanceSyncCursor.test.tsapp/lib/database/__tests__/reconnectCycle.integration.test.tsapp/lib/database/__tests__/loadMissedMessages.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/__tests__/lokiTestDatabase.test.tsapp/lib/database/__tests__/fakeSyncServer.tsapp/lib/database/__tests__/loadMessagesForRoom.integration.test.tsapp/lib/database/__tests__/room.integration.test.tsapp/lib/database/__tests__/advanceSyncCursor.test.tsapp/lib/database/__tests__/reconnectCycle.integration.test.tsapp/lib/database/__tests__/lokiTestDatabase.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__/lokiTestDatabase.test.tsapp/lib/database/__tests__/loadMessagesForRoom.integration.test.tsapp/lib/database/__tests__/room.integration.test.tsapp/lib/database/__tests__/advanceSyncCursor.test.tsapp/lib/database/__tests__/reconnectCycle.integration.test.tsapp/lib/database/__tests__/loadMissedMessages.integration.test.ts
🔇 Additional comments (6)
app/lib/database/__tests__/room.integration.test.ts (1)
89-179: 📐 Maintainability & Code QualityVerify this integration suite under UTC.
Run it with
TZ=UTCthrough the repository’s Jest command. As per coding guidelines, “Run Jest tests withTZ=UTCto ensure deterministic timezone-dependent test behavior.”Source: Coding guidelines
jest.config.js (1)
3-10: LGTM!app/lib/database/__tests__/advanceSyncCursor.test.ts (1)
1-170: LGTM!app/lib/database/__tests__/loadMessagesForRoom.integration.test.ts (1)
1-133: LGTM!app/lib/database/__tests__/loadMissedMessages.integration.test.ts (1)
1-210: LGTM!app/lib/database/__tests__/reconnectCycle.integration.test.ts (1)
1-178: LGTM!
| const paginate = (source: IFakeServerMessage[], next: number | null | undefined, pageSize: number) => { | ||
| const cursor = next ?? 0; | ||
| const matching = source.filter(message => message._updatedAt > cursor).sort((a, b) => a._updatedAt - b._updatedAt); | ||
| const page = matching.slice(0, pageSize); | ||
| const nextCursor = matching.length > page.length ? page[page.length - 1]._updatedAt : null; | ||
| return { page: page.map(serialize), nextCursor }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Do not split identical timestamps across pages.
With pageSize = 1, two records sharing _updatedAt return one record and set nextCursor to that timestamp; the next > query permanently excludes the other. Extend the page through its boundary timestamp before emitting the cursor.
🤖 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 65 - 70, Update
the paginate helper so each page includes all records sharing the final record’s
_updatedAt timestamp, even when this exceeds pageSize. Calculate the boundary
timestamp from the initial page, extend the page through matching records at
that timestamp, and emit nextCursor only when records remain beyond that
boundary; preserve serialize mapping and cursor filtering.
| handleSyncMessages(params) { | ||
| if (params.type === 'DELETED') { | ||
| deletedPageRequests += 1; | ||
| if (server.failDeletedPageAtRequest && deletedPageRequests === server.failDeletedPageAtRequest) { | ||
| throw new Error('DELETED page request failed'); | ||
| } | ||
| const { page, nextCursor } = paginate(server.deleted, params.next, deletedPageSize); | ||
| return { result: { deleted: page, cursor: { next: nextCursor, previous: null } } }; | ||
| } | ||
|
|
||
| updatedPageRequests += 1; | ||
| if (server.failUpdatedPageAtRequest && updatedPageRequests === server.failUpdatedPageAtRequest) { | ||
| throw new Error('UPDATED page request failed'); | ||
| } | ||
| const { page, nextCursor } = paginate(server.updated, params.next, updatedPageSize); | ||
| return { result: { updated: page, cursor: { next: nextCursor, previous: null } } }; | ||
| }, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reject unsupported sync types.
Any missing or invalid type currently behaves as UPDATED, so integration tests can pass when callers omit or misspell the cursor stream. Validate 'UPDATED' and 'DELETED' explicitly, then throw for other values.
🤖 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 88 - 104, Update
handleSyncMessages to explicitly branch only for 'DELETED' and 'UPDATED' types,
preserving each stream’s existing pagination and failure behavior. After those
supported cases, throw an error for missing or unsupported params.type values
instead of treating them as updated requests.
…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
Waiting
GitHub Actions
Waiting
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.
1 participant
Proposed changes
Messages that arrived while a room wasn't actively synced could permanently disappear on mobile.
subscription.lastOpendoubles as thechat.syncMessagescursor, but it was written from the client clock and advanced unconditionally — on every room read and again when leaving the room. That asserts "synced up to now" without the client having verified it, so the cursor could jump past messages the client never received. Once the cursor points past a message, the server answers with a successful empty response, the cursor re-advances (client clock again), and that message is skipped forever. Web/desktop were unaffected — only the mobile cursor pointed past the messages.The cursor overran real sync coverage two independent ways:
This PR makes the cursor server-derived, so it can only ever point at timestamps the client has actually persisted:
advanceSyncCursorhelper: after a batch of messages is persisted,lastOpenadvances to the max_updatedAtof the batch — only forward, never on an empty fetch, and never from older-history (scroll-up) windows, where an edited old message's recent_updatedAtcould jump the cursor past unsynced newer messages.loadMissedMessagesadvances the cursor after every successful sync;loadMessagesForRoombootstraps it on the first/newest load.readMessagesno longer writeslastOpen(it only marks the room read now), and the client-clock reseed on room unsubscribe (updateLastOpen) is removed.Two adjacent sync bugs in
loadMissedMessagesare also fixed:updatedslot, persisting deleted messages as updates.Issue(s)
How to test or reproduce
Offline coverage gap (no clock change needed):
Clock skew:
Before this fix the new messages never appear (and never recover, even after restarts). After the fix they load normally. Regression coverage lives in
app/lib/methods/loadMissedMessages.test.ts, which exercises both the offline coverage gap and the skewed-clock scenario against the >=7.1 cursor API semantics.Screenshots
N/A (sync behavior; no UI changes)
Types of changes
Checklist
Further comments
Reusing
lastOpen(instead of adding a dedicated cursor field) was a deliberate choice: enumerating all readers/writers showed it is exclusively the sync cursor — the unread separator usesroom.ls— so a new field plus migration would only duplicate it. Messages received live over the stream while a room is open don't advance the cursor; the next open re-syncs them once (idempotent upsert), which is safe and keeps the invariant simple: the cursor only ever points at server timestamps the client has actually persisted.Summary by CodeRabbit
New Features
Bug Fixes
Tests