fix: split lastOpen into sync cursor and last-seen separator - #7520
fix: split lastOpen into sync cursor and last-seen separator#7520diegolmello wants to merge 9 commits into
Conversation
…stic ls on mark unread
…utation-proof Delete updateLastOpen and its leave-room call site. Drop readMessages updateLastOpen parameter and all call sites. Add loadMessagesForRoom test pinning cursor to raw payload stamps, not device-stamped DB rows.
GitHub Actions
Inactive
WalkthroughChangesRoom synchronization now derives cursors from raw server Room synchronization and last-seen state
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant RoomView
participant loadMissedMessages
participant RocketChatSDK
participant updateMessages
participant advanceSyncCursor
RoomView->>loadMissedMessages: load missed messages
loadMissedMessages->>RocketChatSDK: fetch UPDATED and DELETED pages
RocketChatSDK-->>loadMissedMessages: return payloads and cursors
loadMissedMessages->>updateMessages: apply message changes
loadMissedMessages->>advanceSyncCursor: persist maximum UPDATED timestamp
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
app/views/RoomView/index.tsx (1)
1406-1424: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUnread separator misses the "same-as-lastSeen" case for the first rendered item.
The
!previousItembranch only checksisAfter(lastSeen), while the subsequent-item branch checksisSame(lastSeen) || isAfter(lastSeen). If the first rendered item'stsexactly equalslastSeen(e.g., the mark-as-unread anchor message happens to be the first item in the window), the separator won't show here, but would show if the same message weren't first — an inconsistent edge case.🐛 Proposed fix
if (!previousItem) { dateSeparator = item.ts; - showUnreadSeparator = lastSeen ? dayjs(item.ts).isAfter(lastSeen) : false; + showUnreadSeparator = lastSeen ? dayjs(item.ts).isSame(lastSeen) || dayjs(item.ts).isAfter(lastSeen) : false; } else {🤖 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` around lines 1406 - 1424, Update the !previousItem branch in renderItem so showUnreadSeparator treats an item timestamp equal to lastSeen the same as a timestamp after lastSeen, matching the existing subsequent-item logic while preserving the false result when lastSeen is absent.app/containers/MessageActions/index.tsx (1)
237-256: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winConvert
message.tsto a realDatebefore writingsub.ls.
TAnyMessageModelinheritsts?: string | Date, butsub.lsis typed asDate; thists as Datecan silently persist a string into WatermelonDB and break unread-separator time comparisons. Use a shared timestamp coercion helper or parse string values before assigning.🤖 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/containers/MessageActions/index.tsx` around lines 237 - 256, Update handleUnread so the message ts value is converted to a real Date before assigning it to sub.ls. Reuse the existing shared timestamp coercion helper if available, otherwise parse string timestamps explicitly, while preserving the current subscription update flow.
🧹 Nitpick comments (2)
app/lib/methods/helpers/advanceSyncCursor.test.ts (1)
47-64: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicated subscription-mock boilerplate across two test files. Both suites independently re-implement the same
database.active.get/writemocking plus asubscription.updatejest.fn that mutateslastOpenin place — this shared root cause is worth extracting into one reusable test helper.
app/lib/methods/helpers/advanceSyncCursor.test.ts#L47-L64: extract themockGet/mockWrite/subscriptionsetup into a shared helper (e.g.test/helpers/mockSubscriptionDb.ts) and import it here.app/lib/methods/loadMessagesForRoom.test.ts#L63-L83: import and use the same shared helper here instead of re-declaring the identical mock.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/lib/methods/helpers/advanceSyncCursor.test.ts` around lines 47 - 64, The subscription database mock setup is duplicated across both test suites. Extract the shared subscription state, update mutation behavior, and database active get/write mocks into a reusable helper, then import and use it in app/lib/methods/helpers/advanceSyncCursor.test.ts (lines 47-64) and app/lib/methods/loadMessagesForRoom.test.ts (lines 63-83), preserving each suite’s existing behavior.app/lib/methods/loadMissedMessages.ts (1)
99-126: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winNo safety cap on the pagination loop.
The
for (;;)loop only terminates when both cursors resolve tonull. If the server never returns a terminal cursor (bug, misconfiguration, or the0-cursor issue above), thisawaited call insideRoomView.init()would hang indefinitely, leaving the room stuck in a loading state.🛡️ Suggested guard
+ const MAX_PAGES = 200; // sanity cap to avoid an unbounded loop on a misbehaving server + let pages = 0; for (;;) { + if (++pages > MAX_PAGES) { + log(`loadMissedMessages: exceeded max pages for ${rid}`); + break; + } const { ... } = await getSyncMessagesFromCursor(...);🤖 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 99 - 126, Add a bounded iteration guard to the pagination loop in the missed-message loading flow around getSyncMessagesFromCursor, enforcing a finite maximum number of pages and exiting through the existing completion path when the cap is reached. Preserve sequential fetching, cursor updates, message processing, and sync-cursor advancement for normal terminal responses.
🤖 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/loadMessagesForRoom.test.ts`:
- Line 63: Remove the duplicate let subscription declarations, keeping a single
binding with the existing type and scope used by the test.
---
Outside diff comments:
In `@app/containers/MessageActions/index.tsx`:
- Around line 237-256: Update handleUnread so the message ts value is converted
to a real Date before assigning it to sub.ls. Reuse the existing shared
timestamp coercion helper if available, otherwise parse string timestamps
explicitly, while preserving the current subscription update flow.
In `@app/views/RoomView/index.tsx`:
- Around line 1406-1424: Update the !previousItem branch in renderItem so
showUnreadSeparator treats an item timestamp equal to lastSeen the same as a
timestamp after lastSeen, matching the existing subsequent-item logic while
preserving the false result when lastSeen is absent.
---
Nitpick comments:
In `@app/lib/methods/helpers/advanceSyncCursor.test.ts`:
- Around line 47-64: The subscription database mock setup is duplicated across
both test suites. Extract the shared subscription state, update mutation
behavior, and database active get/write mocks into a reusable helper, then
import and use it in app/lib/methods/helpers/advanceSyncCursor.test.ts (lines
47-64) and app/lib/methods/loadMessagesForRoom.test.ts (lines 63-83), preserving
each suite’s existing behavior.
In `@app/lib/methods/loadMissedMessages.ts`:
- Around line 99-126: Add a bounded iteration guard to the pagination loop in
the missed-message loading flow around getSyncMessagesFromCursor, enforcing a
finite maximum number of pages and exiting through the existing completion path
when the cap is reached. Preserve sequential fetching, cursor updates, message
processing, and sync-cursor advancement for normal terminal responses.
🪄 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: d45d5260-da25-4e5e-add2-692a029492ab
📒 Files selected for processing (20)
CLAUDE.mdCONTEXT.mdapp/containers/MessageActions/index.tsxapp/lib/methods/helpers/advanceSyncCursor.test.tsapp/lib/methods/helpers/advanceSyncCursor.tsapp/lib/methods/loadMessagesForRoom.test.tsapp/lib/methods/loadMessagesForRoom.tsapp/lib/methods/loadMissedMessages.test.tsapp/lib/methods/loadMissedMessages.tsapp/lib/methods/loadNextMessages.tsapp/lib/methods/loadSurroundingMessages.tsapp/lib/methods/readMessages.tsapp/lib/methods/subscriptions/room.test.tsapp/lib/methods/subscriptions/room.tsapp/lib/methods/updateLastOpen.tsapp/views/RoomView/constants.tsapp/views/RoomView/definitions.tsapp/views/RoomView/index.tsxapp/views/RoomView/services/getMessages.tsapp/views/RoomView/services/index.ts
💤 Files with no reviewable changes (5)
- app/views/RoomView/services/getMessages.ts
- app/lib/methods/updateLastOpen.ts
- app/views/RoomView/services/index.ts
- app/lib/methods/subscriptions/room.test.ts
- app/lib/methods/subscriptions/room.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 (3)
**/*.{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/loadNextMessages.tsapp/lib/methods/loadSurroundingMessages.tsapp/views/RoomView/definitions.tsapp/views/RoomView/constants.tsapp/containers/MessageActions/index.tsxapp/lib/methods/helpers/advanceSyncCursor.test.tsapp/lib/methods/readMessages.tsapp/lib/methods/helpers/advanceSyncCursor.tsapp/lib/methods/loadMissedMessages.test.tsapp/lib/methods/loadMessagesForRoom.tsapp/lib/methods/loadMessagesForRoom.test.tsapp/views/RoomView/index.tsxapp/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 numbers
Files:
app/lib/methods/loadNextMessages.tsapp/lib/methods/loadSurroundingMessages.tsapp/views/RoomView/definitions.tsapp/views/RoomView/constants.tsapp/containers/MessageActions/index.tsxapp/lib/methods/helpers/advanceSyncCursor.test.tsapp/lib/methods/readMessages.tsapp/lib/methods/helpers/advanceSyncCursor.tsapp/lib/methods/loadMissedMessages.test.tsapp/lib/methods/loadMessagesForRoom.tsapp/lib/methods/loadMessagesForRoom.test.tsapp/views/RoomView/index.tsxapp/lib/methods/loadMissedMessages.ts
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{js,jsx,ts,tsx}: In this React Native application, UI code should read local data from WatermelonDB; sagas synchronize that data with the server.
Use Redux and Redux-Saga for global/server state, and Zustand for feature-local stores; do not assume all state is in Redux.
Derive sync watermarks only from raw server response payloads; never use local database rows orDate.now()as the source of watermark timestamps.
Files:
app/lib/methods/loadNextMessages.tsapp/lib/methods/loadSurroundingMessages.tsapp/views/RoomView/definitions.tsapp/views/RoomView/constants.tsapp/containers/MessageActions/index.tsxapp/lib/methods/helpers/advanceSyncCursor.test.tsapp/lib/methods/readMessages.tsapp/lib/methods/helpers/advanceSyncCursor.tsapp/lib/methods/loadMissedMessages.test.tsapp/lib/methods/loadMessagesForRoom.tsapp/lib/methods/loadMessagesForRoom.test.tsapp/views/RoomView/index.tsxapp/lib/methods/loadMissedMessages.ts
🧠 Learnings (5)
📓 Common learnings
Learnt from: CR
Repo: RocketChat/Rocket.Chat.ReactNative
Timestamp: 2026-07-26T18:58:43.450Z
Learning: Before committing, run `pnpm prettier-lint` and `TZ=UTC pnpm test` for modified files.
Learnt from: CR
Repo: RocketChat/Rocket.Chat.ReactNative
Timestamp: 2026-07-26T18:58:43.450Z
Learning: Run `pnpm pod-install` before building for iOS.
Learnt from: CR
Repo: RocketChat/Rocket.Chat.ReactNative
Timestamp: 2026-07-26T18:58:43.450Z
Learning: Consult `CONTEXT.md` for repository-specific implementation guidance and `.github/README.md` for CI triggers, call graphs, and manual gates.
📚 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/loadNextMessages.tsapp/lib/methods/loadSurroundingMessages.tsapp/views/RoomView/definitions.tsapp/views/RoomView/constants.tsapp/containers/MessageActions/index.tsxapp/lib/methods/helpers/advanceSyncCursor.test.tsapp/lib/methods/readMessages.tsapp/lib/methods/helpers/advanceSyncCursor.tsapp/lib/methods/loadMissedMessages.test.tsapp/lib/methods/loadMessagesForRoom.tsapp/lib/methods/loadMessagesForRoom.test.tsapp/views/RoomView/index.tsxapp/lib/methods/loadMissedMessages.ts
📚 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/containers/MessageActions/index.tsxapp/views/RoomView/index.tsx
📚 Learning: 2026-06-25T18:37:25.526Z
Learnt from: diegolmello
Repo: RocketChat/Rocket.Chat.ReactNative PR: 7434
File: app/views/ScreenLockConfigView.test.tsx:16-22
Timestamp: 2026-06-25T18:37:25.526Z
Learning: In Rocket.Chat ReactNative tests that mock selectors for `useAppSelector`, don’t require the mocked selector input to be typed as `IApplicationState` when the fixture only includes a partial Redux state slice (e.g., only `server` and `settings`). Requiring the full `IApplicationState` type in that scenario forces unsafe `as IApplicationState` casts and undermines type-safety. For these narrowly scoped selector-mock fixtures, use a less strict type (e.g., `any`) to keep the mock focused on the slice under test.
Applied to files:
app/lib/methods/helpers/advanceSyncCursor.test.tsapp/lib/methods/loadMissedMessages.test.tsapp/lib/methods/loadMessagesForRoom.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
🔇 Additional comments (16)
app/lib/methods/helpers/advanceSyncCursor.ts (3)
9-22: LGTM!
26-29: LGTM!Also applies to: 36-46
30-35: 🎯 Functional CorrectnessNo change needed for
db.get('subscriptions')typing.
activereturnsTAppDatabase, whosegetsignature is<T extends TAppDatabaseNames>(db: T) => Collection<ObjectType<T>>; this mapssubscriptionstoTSubscriptionModel, so the assignment is already typed correctly.> Likely an incorrect or invalid review comment.CONTEXT.md (2)
93-116: LGTM!
264-264: LGTM!CLAUDE.md (1)
28-28: LGTM!app/lib/methods/loadMessagesForRoom.ts (1)
13-13: LGTM!Also applies to: 36-48, 81-84, 117-117, 128-128, 147-150
app/lib/methods/loadMessagesForRoom.test.ts (1)
265-345: LGTM!app/lib/methods/loadMissedMessages.test.ts (1)
1-199: LGTM!app/lib/methods/loadNextMessages.ts (1)
40-41: LGTM!app/lib/methods/loadSurroundingMessages.ts (1)
54-55: LGTM!app/views/RoomView/constants.ts (1)
3-13: LGTM!app/views/RoomView/definitions.ts (1)
32-65: LGTM!app/views/RoomView/index.tsx (1)
82-82: LGTM!Also applies to: 181-181, 682-696, 1124-1131
app/lib/methods/readMessages.ts (1)
8-8: 🗄️ Data Integrity & IntegrationNo caller updates needed.
The
readMessagescall sites use the 2-argument signature, andupdateLastOpenis not passed anywhere.app/lib/methods/loadMissedMessages.ts (1)
36-58: 🎯 Functional CorrectnessNo change needed: timestamp cursors are not terminal unless null.
chat.syncMessagesuses millisecond timestamps for thenextcursor, so a non-null0value would carry meaningful cursor information and would not be treated as the same terminal state asnull/undefined.> Likely an incorrect or invalid review comment.
| ...(params._updatedAt !== undefined ? { _updatedAt: params._updatedAt } : {}) | ||
| } as any); | ||
|
|
||
| let subscription: { id: string; lastOpen: Date | null; update: jest.Mock }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Duplicate let subscription declaration.
The subscription variable declaration is duplicated four times at the same location. Re-declaring a let binding in the same block scope is a compile error ("Cannot redeclare block-scoped variable").
🐛 Proposed fix
-let subscription: { id: string; lastOpen: Date | null; update: jest.Mock };
-let subscription: { id: string; lastOpen: Date | null; update: jest.Mock };
-let subscription: { id: string; lastOpen: Date | null; update: jest.Mock };
let subscription: { id: string; lastOpen: Date | null; update: jest.Mock };📝 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.
| let subscription: { id: string; lastOpen: Date | null; update: jest.Mock }; | |
| let subscription: { id: string; lastOpen: Date | null; update: jest.Mock }; |
🤖 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/loadMessagesForRoom.test.ts` at line 63, Remove the duplicate
let subscription declarations, keeping a single binding with the existing type
and scope used by the test.
|
Android Build Available Rocket.Chat 4.75.0.109417 Internal App Sharing: https://play.google.com/apps/test/RQQ8k09hlnQ/ahAO29uNRV_19mVgMINDeouhtJe_EgPtt2YDD5cIiQMisD7K46MEylW76bMU6HrnCBteq7cG4oUlavC9cbLhWPmM-k |
|
iOS Build Available Rocket.Chat 4.75.0.109418 |
1 participant
Proposed changes
subscription.lastOpenserved two conflicting meanings: the fetch cursor forchat.syncMessagesand the unread-separator position in RoomView — and it was writable by the device clock (updateLastOpen,readMessages' third argument, mark-as-unread), so a skewed device could poison the sync cursor.This PR narrows
lastOpento a single meaning — the sync cursor: the newest server-stamped message_updatedAtthis device has fetched for the room, computed from raw response payloads only (never from WatermelonDB rows, never fromDate.now()), advanced monotonically via the newadvanceSyncCursorhelper.loadMissedMessagesrewritten: null-cursor rooms delegate toloadMessagesForRoom(delegate-and-return, rethrow on failure); recursion replaced by afor(;;)drain loop that writes the cursor once per successful run when the UPDATED stream drains; DELETED pages never write; the pre-7.1.0 legacy branch is kept and also writes the cursor. ThegetMessagesrouter is deleted — RoomView importsloadMissedMessagesdirectly, fixing a live bug where the room typetwas silently dropped wheneverlastOpenwas present.loadMessagesForRoomwrites the cursor on the initial tail load only (never on backfill); jump paths (loadSurroundingMessages,loadNextMessages) are documented non-writers.ls, surfaced asRoomView.state.lastSeen(init-time snapshot captured beforereadMessages). Mark-as-unread writessub.ls = tsoptimistically; the server confirms via stream.updateLastOpendeleted,readMessagesthird argument deleted.Issue(s)
Related to #7499 — this fixes a proven field-meaning clash; whether it resolves the reported missing-messages symptom is unverified and intentionally not gated on this PR.
How to test or reproduce
TZ=UTC pnpm test app/lib/methods— new suites cover null-cursor delegation, once-per-drain writes, DELETED never writes, legacy branch, tail-load writes, and both mutation-proofs (monotonic guard, payload-only).ls; mark a message as unread → separator moves optimistically and the server confirms; kill/reopen the app offline→online → missed messages sync viachat.syncMessagesfrom the cursor.Screenshots
N/A — no visual changes (separator behavior unchanged).
Types of changes
Checklist
Further comments
Concurrency in the message-fetch path is deliberately tolerated (upsert by
_id, monotonic cursor — every step converges); no serialization added. Known adjacent issues (server-side DELETED paging defect,normalizeMessage's device-stamp fallback, rooms-list/thread-list watermarks) are documented as out of scope.Summary by CodeRabbit
New Features
Bug Fixes