fix(sync): derive the message sync cursor from the server clock - #7523
fix(sync): derive the message sync cursor from the server clock#7523diegolmello wants to merge 27 commits into
Conversation
`subscription.lastOpen` did double duty as both the `chat.syncMessages` fetch cursor and the unread-separator anchor, and every writer stamped it from the DEVICE clock. The server compares that cursor against message `_updatedAt`, which is a SERVER clock value. Any forward device skew — or simply closing a room, which stamped `lastOpen = new Date()` regardless of what had been fetched — pushed the cursor past messages that existed on the server but had never reached the device. Those messages were then permanently invisible: the server would never report anything "newer" than the cursor again. `lastOpen` now means one thing only: the max `_updatedAt` of the server response actually received for that room. - add `writeSyncWatermark`, which reads the max `_updatedAt` off the RAW payload. It must run before `normalizeMessage`, which invents a device-clock `_updatedAt` for rows lacking one. It is deliberately not monotonic, so an already-poisoned future cursor can heal. - write the watermark only where the payload proves coverage: from `updated` once the `chat.syncMessages` UPDATED cursor drains, and from the first batch of an initial tail load. The jump paths never write it — a ts-ordered forward walk never sees the `_updatedAt` of pre-anchor messages. - delete every device-clock writer: `updateLastOpen`, the `unsubscribe()` call that closed a room by advancing the cursor, and the `readMessages` `updateLastOpen` flag. Closing a room offline no longer loses messages. - treat a cursor in the future as absent so a skewed room falls back to a full tail load instead of syncing from nothing. - `loadMissedMessages` no longer accepts a `lastOpen` argument, removing the last path by which a caller could inject a device clock. - move the unread separator to its own RoomView state field `lastSeen`, fed from `room.ls`, so it no longer reads a fetch watermark. The DB column keeps its `lastOpen` name; no migration.
Backgrounding can outlive the DDP socket without emitting `connected` or `close`, so `RoomSubscription.handleConnection` never fires and messages sent during the window never land. `chat.syncMessages` rides REST, so calling `loadMissedMessages` from the foreground saga delivers the intent of the reverted socket-probing change with no connection-layer risk: no probing, no forced reopen, nothing racing the SDK reconnect timer. `checkAndReopen()` is deleted — it no-ops when connected, which is exactly the state the foreground gate requires, so it could never heal anything. Also resets `RoomSubscription.isAlive` in `subscribe`, which was only ever set in the constructor; a reused instance would immediately unsubscribe itself.
Deriving the cursor from the server clock stops new poisoning, but it cannot undo it. Rows already in users' databases carry a device-clock `lastOpen` written by the deleted `updateLastOpen` — closing a room while offline stamped `new Date()` regardless of what had actually been fetched. Such a cursor sits ahead of the server's newest `_updatedAt` for the room, so every `chat.syncMessages` drains an empty UPDATED page, there is no `_updatedAt` to take as a watermark, and the gap is permanent. The future-skew clamp does not catch it: the value was device-now when written and is now in the past. There is deliberately no migration and no backfill, so those cursors heal themselves on the next open instead. When the UPDATED cursor drains with an empty payload, compare the subscription's server-supplied `lastMessage._id` against the local messages table. If the server says a newest message exists and it was never delivered here, the cursor is provably lying: run a full tail load, which re-anchors the watermark from a real server response. The check costs one `getMessageById` on a sync that had nothing to do anyway, never runs mid-pagination or on a non-empty payload, skips a room with no `lastMessage`, and recovers via `loadMessagesForRoom` so it cannot recurse. Also document the timestamp trust boundary in CONTEXT.md: a `_updatedAt` from a server response is server truth and the only legitimate cursor source, while the same field on a WatermelonDB row is device-tainted.
`roomsRequest` rode only `LOGIN.SUCCESS`. If the socket died silently while backgrounded, every `stream-notify-user` update was lost and this delta fetch is the only thing that heals the rooms list.
…d the retry
A notification tap for a room the user was just added to routes through
canOpenRoom, which returns a bare { rid } and creates no subscription row.
RoomView therefore falls into findAndObserveRoom, which throws and installs
observeSubscriptions. When the row later arrives from the rooms sync,
observeSubscriptions only swapped it into state, so init() never re-ran: the
room got no ls-based unread separator and sent no read receipt.
Re-run init() on the transition from "no row" to "row present", guarded by a
one-shot flag plus an in-flight flag so a concurrent or repeated emission
cannot re-enter it.
The init() failure path also re-armed a 300ms timer with no cap, hammering a
room that had no row yet. It is now capped at 5 attempts with exponential
backoff, the previous handle is cleared before re-arming, and the counter
resets once init() succeeds.
The nine inline `act(async () => {})` calls tripped require-await and
no-await-in-loop, failing the eslint CI gate. A single helper that awaits a
microtask expresses the same flush honestly instead of suppressing the rules.
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 change separates server-derived ChangesSynchronization and room state
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 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: 4
🧹 Nitpick comments (9)
app/views/RoomView/index.test.tsx (1)
109-143: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd explicit return types to the test helpers.
Annotate
buildRow,renderRoomView,flush, andretryDelaysreturn types rather than relying on inference. As per coding guidelines, TypeScript functions must have explicit parameter and return annotations.Also applies to: 205-206
🤖 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.test.tsx` around lines 109 - 143, Annotate the test helpers buildRow, renderRoomView, flush, and retryDelays with explicit parameter and return types. Preserve their current behavior and infer each return type from the existing returned row object, render result, act-based promise, and retry-delay value respectively.Source: Coding guidelines
app/views/RoomView/index.tsx (1)
1160-1160: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDeclare the helper return type.
Use
setLastSeen = (lastSeen: Date | null): void => .... As per coding guidelines, TypeScript functions must have explicit parameter and return annotations.🤖 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 1160, Update the setLastSeen helper to explicitly declare a void return type while preserving its existing Date | null parameter and setState behavior.Source: Coding guidelines
app/lib/methods/loadMessagesForRoom.ts (1)
78-82: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDrop the
as IMessagecast by typing the snapshot to what it actually is.The snapshot objects only carry
_updatedAt, andwriteSyncWatermarkonly needs{ _updatedAt?: string | Date }[]. Casting toIMessagemisrepresents the shape and would silently pass a partial row anywhereIMessageis expected.♻️ Proposed typing tightening
-}): Promise<{ messages: IMessage[]; firstBatch: IMessage[]; lastBatchWasFull: boolean }> { +}): Promise<{ messages: IMessage[]; firstBatch: { _updatedAt?: string | Date }[]; lastBatchWasFull: boolean }> { @@ - let firstBatch: IMessage[] = []; + let firstBatch: { _updatedAt?: string | Date }[] = []; @@ - firstBatch = batch.map(message => ({ _updatedAt: message._updatedAt } as IMessage)); + firstBatch = batch.map(message => ({ _updatedAt: message._updatedAt }));🤖 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.ts` around lines 78 - 82, Update the firstBatch snapshot in the loadMessagesForRoom flow to use a type representing only the optional _updatedAt string-or-Date field, matching writeSyncWatermark’s input. Remove the as IMessage cast while preserving the existing batch mapping and timestamp capture behavior.Source: Coding guidelines
app/lib/methods/loadMissedMessages.test.ts (2)
101-109: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueHoist the duplicated
flushandCURSORhelpers to module scope.Both nested describes redefine identical
flushandCURSOR; a single module-level definition keeps the suites in sync.Also applies to: 176-180
🤖 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 101 - 109, Move the duplicated CURSOR constant and flush helper from the nested “sync watermark” describes to module scope in loadMissedMessages.test.ts. Remove the nested definitions and reuse the shared module-level symbols in both suites, preserving their current values and behavior.
122-147: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPagination tests depend on the recursive call being fire-and-forget.
await flush()(onesetImmediatetick) happens to drain the un-awaited recursiveloadMissedMessageschain today, but it is not tied to the number of pending pages — a third page or an extraawaitinside the recursion would make these flaky rather than failing loudly. Consider looping the flush until the expectation holds (or exposing the recursion promise) so the tests don't encode a microtask-count assumption.Also applies to: 234-255
🤖 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 122 - 147, Make the pagination tests around loadMissedMessages wait for completion without relying on a single flush tick draining the fire-and-forget recursive chain. Update the assertions in this test and the referenced similar case to repeatedly flush or otherwise await completion until the final watermark write is observed, while preserving the check that no write occurs before the final page.app/lib/methods/loadMissedMessages.ts (1)
25-43: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
cursoris silently ignored once either continuation token is present.With
cursor && !updatedNext && !deletedNext, a call carryingcursorplus onlydeletedNextfetches nothing on the UPDATED side, soupdatedis[]andupdatedNextisundefined— which downstream (Line 150) reads as "UPDATED fully drained". That is correct for the recursive continuation calls (which never passcursor), but the signature allows a combination that quietly mis-signals drainage. Consider making the two modes explicit (a discriminated{ cursor }vs{ updatedNext, deletedNext }argument) so the invalid mix can't be constructed.🤖 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 25 - 43, Make the loadMissedMessages argument shape explicit so initial calls accept only cursor, while continuation calls accept updatedNext/deletedNext without cursor. Update the function’s signature and branching around syncMessages to use this discriminated mode, preventing cursor from being combined with either continuation token and preserving recursive continuation behavior.app/sagas/__tests__/state.test.ts (2)
127-130: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert behavior, not source text.
Reading
state.jsand grepping forcheckAndReopencouples the test to source formatting: it fails if the identifier ever appears in a comment, and passes if socket-reopen logic returns under another name. Mock../../lib/services/connectand assert the function is never invoked during the foreground flow instead.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/sagas/__tests__/state.test.ts` around lines 127 - 130, Replace the source-text assertion in the “no longer reaches into the connection layer” test with a behavioral test: mock ../../lib/services/connect, execute the foreground flow through the relevant state saga behavior, and assert the mocked connection/reopen function is never invoked. Remove the readFileSync/source-grep approach while preserving coverage that the foreground flow does not reopen the socket.
46-50: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated fixed-count microtask flush in both saga tests. Both files hand-roll the same
for (let i = 0; i < 60; i += 1) await Promise.resolve()helper. The iteration count encodes today's await depth in the foreground saga, so adding oneawaitanywhere in the resync chain makes these suites fail for reasons unrelated to the assertion. Replace with a single shared poll-until-condition helper (e.g. retry the expectation across ticks with a bounded deadline).
app/sagas/__tests__/state.test.ts#L46-L50: remove the local helper and import the shared one.app/sagas/__tests__/state.foregroundResync.test.ts#L71-L76: remove the local helper (and its comment) and import the same shared one.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/sagas/__tests__/state.test.ts` around lines 46 - 50, Replace the duplicated fixed-count flush helpers with one shared bounded poll-until-condition test utility. In app/sagas/__tests__/state.test.ts:46-50, remove flushSagaMicrotasks and import the shared helper; in app/sagas/__tests__/state.foregroundResync.test.ts:71-76, remove the local helper and its comment and import the same helper, updating call sites to poll the relevant expectations across ticks.app/sagas/state.js (1)
46-49: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winForeground presence and rooms delta now wait on a network sync with no timeout.
resyncSubscribedRoomblocks onloadMissedMessages+readMessages; on a stalled REST request (common right at resume on a flaky link)roomsRequest()andsetUserPresenceOnline()are deferred for the whole socket/HTTP timeout.takeLatestonly cancels on the next FOREGROUND action. Consider forking the room resync so the rooms delta and presence update aren't gated on it.♻️ Proposed reordering
- yield resyncSubscribedRoom(); + yield fork(resyncSubscribedRoom); // A silently dead socket dropped every `stream-notify-user` update; this delta fetch is // the only thing that heals the rooms list. yield put(roomsRequest());Note this changes
state.test.tstiming expectations, since the resync would no longer be complete when the saga returns.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/sagas/state.js` around lines 46 - 49, Update the foreground handling saga around resyncSubscribedRoom so the room resync runs without blocking the subsequent roomsRequest() and setUserPresenceOnline() updates; fork or otherwise start the resync asynchronously while preserving cancellation behavior. Adjust state.test.ts timing expectations to reflect that the saga can return before resyncSubscribedRoom completes.
🤖 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 148-155: Restrict the healLyingCursor call in the
loadMissedMessages pagination flow so it runs only when the entire run is
drained, not merely when the UPDATED cursor is exhausted. Preserve
writeSyncWatermark behavior, but add the overall continuation state (including
deletedNext) to the condition around healLyingCursor, preventing repeated
healing on deleted-only pages.
- Around line 140-146: Handle the recursive loadMissedMessages call in the
deletedNext/updatedNext continuation with an explicit .catch(...) handler, so
rejections from subsequent load or SDK requests are not left unhandled. Preserve
the existing arguments and recursion behavior, and use the repository’s
established error-reporting approach inside the handler.
In `@app/views/RoomView/index.tsx`:
- Around line 719-731: Guard the catch block in RoomView’s init flow with
this.mounted before calling setState or scheduling retries, so rejected
in-flight loads after componentWillUnmount are ignored. Ensure retryInitTimeout
is only created while mounted, and add a test covering unmounting before a
deferred getMessages rejection settles.
- Around line 668-671: The init() guard in RoomView’s initialization flow
currently drops calls while initializing; queue a reinitialization request
instead. Track that another initialization is needed when this.initializing is
already true, then rerun initialization after the active attempt completes so
the adopted subscribed room can apply lastSeen and read messages.
---
Nitpick comments:
In `@app/lib/methods/loadMessagesForRoom.ts`:
- Around line 78-82: Update the firstBatch snapshot in the loadMessagesForRoom
flow to use a type representing only the optional _updatedAt string-or-Date
field, matching writeSyncWatermark’s input. Remove the as IMessage cast while
preserving the existing batch mapping and timestamp capture behavior.
In `@app/lib/methods/loadMissedMessages.test.ts`:
- Around line 101-109: Move the duplicated CURSOR constant and flush helper from
the nested “sync watermark” describes to module scope in
loadMissedMessages.test.ts. Remove the nested definitions and reuse the shared
module-level symbols in both suites, preserving their current values and
behavior.
- Around line 122-147: Make the pagination tests around loadMissedMessages wait
for completion without relying on a single flush tick draining the
fire-and-forget recursive chain. Update the assertions in this test and the
referenced similar case to repeatedly flush or otherwise await completion until
the final watermark write is observed, while preserving the check that no write
occurs before the final page.
In `@app/lib/methods/loadMissedMessages.ts`:
- Around line 25-43: Make the loadMissedMessages argument shape explicit so
initial calls accept only cursor, while continuation calls accept
updatedNext/deletedNext without cursor. Update the function’s signature and
branching around syncMessages to use this discriminated mode, preventing cursor
from being combined with either continuation token and preserving recursive
continuation behavior.
In `@app/sagas/__tests__/state.test.ts`:
- Around line 127-130: Replace the source-text assertion in the “no longer
reaches into the connection layer” test with a behavioral test: mock
../../lib/services/connect, execute the foreground flow through the relevant
state saga behavior, and assert the mocked connection/reopen function is never
invoked. Remove the readFileSync/source-grep approach while preserving coverage
that the foreground flow does not reopen the socket.
- Around line 46-50: Replace the duplicated fixed-count flush helpers with one
shared bounded poll-until-condition test utility. In
app/sagas/__tests__/state.test.ts:46-50, remove flushSagaMicrotasks and import
the shared helper; in app/sagas/__tests__/state.foregroundResync.test.ts:71-76,
remove the local helper and its comment and import the same helper, updating
call sites to poll the relevant expectations across ticks.
In `@app/sagas/state.js`:
- Around line 46-49: Update the foreground handling saga around
resyncSubscribedRoom so the room resync runs without blocking the subsequent
roomsRequest() and setUserPresenceOnline() updates; fork or otherwise start the
resync asynchronously while preserving cancellation behavior. Adjust
state.test.ts timing expectations to reflect that the saga can return before
resyncSubscribedRoom completes.
In `@app/views/RoomView/index.test.tsx`:
- Around line 109-143: Annotate the test helpers buildRow, renderRoomView,
flush, and retryDelays with explicit parameter and return types. Preserve their
current behavior and infer each return type from the existing returned row
object, render result, act-based promise, and retry-delay value respectively.
In `@app/views/RoomView/index.tsx`:
- Line 1160: Update the setLastSeen helper to explicitly declare a void return
type while preserving its existing Date | null parameter and setState 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: d4c42329-684f-48cf-8e1d-96ab8da5dfa9
📒 Files selected for processing (24)
CONTEXT.mdapp/containers/MessageActions/index.tsxapp/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/subscriptions/roomCloseCursor.test.tsapp/lib/methods/updateLastOpen.tsapp/lib/methods/writeSyncWatermark.test.tsapp/lib/methods/writeSyncWatermark.tsapp/lib/services/connect.tsapp/sagas/__tests__/state.foregroundResync.test.tsapp/sagas/__tests__/state.test.tsapp/sagas/state.jsapp/views/RoomView/constants.tsapp/views/RoomView/definitions.tsapp/views/RoomView/index.test.tsxapp/views/RoomView/index.tsxapp/views/RoomView/services/getMessages.ts
💤 Files with no reviewable changes (2)
- app/lib/methods/updateLastOpen.ts
- app/lib/services/connect.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/methods/writeSyncWatermark.tsapp/views/RoomView/constants.tsapp/lib/methods/writeSyncWatermark.test.tsapp/lib/methods/loadNextMessages.tsapp/views/RoomView/definitions.tsapp/lib/methods/loadSurroundingMessages.tsapp/lib/methods/subscriptions/roomCloseCursor.test.tsapp/containers/MessageActions/index.tsxapp/sagas/state.jsapp/sagas/__tests__/state.foregroundResync.test.tsapp/views/RoomView/services/getMessages.tsapp/lib/methods/subscriptions/room.test.tsapp/lib/methods/loadMessagesForRoom.test.tsapp/lib/methods/readMessages.tsapp/lib/methods/subscriptions/room.tsapp/views/RoomView/index.test.tsxapp/lib/methods/loadMissedMessages.test.tsapp/sagas/__tests__/state.test.tsapp/lib/methods/loadMessagesForRoom.tsapp/lib/methods/loadMissedMessages.tsapp/views/RoomView/index.tsx
**/*.{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/writeSyncWatermark.tsapp/views/RoomView/constants.tsapp/lib/methods/writeSyncWatermark.test.tsapp/lib/methods/loadNextMessages.tsapp/views/RoomView/definitions.tsapp/lib/methods/loadSurroundingMessages.tsapp/lib/methods/subscriptions/roomCloseCursor.test.tsapp/containers/MessageActions/index.tsxapp/sagas/__tests__/state.foregroundResync.test.tsapp/views/RoomView/services/getMessages.tsapp/lib/methods/subscriptions/room.test.tsapp/lib/methods/loadMessagesForRoom.test.tsapp/lib/methods/readMessages.tsapp/lib/methods/subscriptions/room.tsapp/views/RoomView/index.test.tsxapp/lib/methods/loadMissedMessages.test.tsapp/sagas/__tests__/state.test.tsapp/lib/methods/loadMessagesForRoom.tsapp/lib/methods/loadMissedMessages.tsapp/views/RoomView/index.tsx
**/*.{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/writeSyncWatermark.tsapp/views/RoomView/constants.tsapp/lib/methods/writeSyncWatermark.test.tsapp/lib/methods/loadNextMessages.tsapp/views/RoomView/definitions.tsapp/lib/methods/loadSurroundingMessages.tsapp/lib/methods/subscriptions/roomCloseCursor.test.tsapp/containers/MessageActions/index.tsxapp/sagas/state.jsapp/sagas/__tests__/state.foregroundResync.test.tsapp/views/RoomView/services/getMessages.tsapp/lib/methods/subscriptions/room.test.tsapp/lib/methods/loadMessagesForRoom.test.tsapp/lib/methods/readMessages.tsapp/lib/methods/subscriptions/room.tsapp/views/RoomView/index.test.tsxapp/lib/methods/loadMissedMessages.test.tsapp/sagas/__tests__/state.test.tsapp/lib/methods/loadMessagesForRoom.tsapp/lib/methods/loadMissedMessages.tsapp/views/RoomView/index.tsx
**/*.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/writeSyncWatermark.test.tsapp/lib/methods/subscriptions/roomCloseCursor.test.tsapp/sagas/__tests__/state.foregroundResync.test.tsapp/lib/methods/subscriptions/room.test.tsapp/lib/methods/loadMessagesForRoom.test.tsapp/views/RoomView/index.test.tsxapp/lib/methods/loadMissedMessages.test.tsapp/sagas/__tests__/state.test.ts
app/sagas/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use sagas for side effects such as initialization, authentication, rooms, messages, encryption, deep linking, and video conferencing.
Files:
app/sagas/__tests__/state.foregroundResync.test.tsapp/sagas/__tests__/state.test.ts
🧠 Learnings (5)
📚 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/writeSyncWatermark.tsapp/views/RoomView/constants.tsapp/lib/methods/writeSyncWatermark.test.tsapp/lib/methods/loadNextMessages.tsapp/views/RoomView/definitions.tsapp/lib/methods/loadSurroundingMessages.tsapp/lib/methods/subscriptions/roomCloseCursor.test.tsapp/containers/MessageActions/index.tsxapp/sagas/__tests__/state.foregroundResync.test.tsapp/views/RoomView/services/getMessages.tsapp/lib/methods/subscriptions/room.test.tsapp/lib/methods/loadMessagesForRoom.test.tsapp/lib/methods/readMessages.tsapp/lib/methods/subscriptions/room.tsapp/views/RoomView/index.test.tsxapp/lib/methods/loadMissedMessages.test.tsapp/sagas/__tests__/state.test.tsapp/lib/methods/loadMessagesForRoom.tsapp/lib/methods/loadMissedMessages.tsapp/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/writeSyncWatermark.test.tsapp/lib/methods/subscriptions/roomCloseCursor.test.tsapp/sagas/__tests__/state.foregroundResync.test.tsapp/lib/methods/subscriptions/room.test.tsapp/lib/methods/loadMessagesForRoom.test.tsapp/views/RoomView/index.test.tsxapp/lib/methods/loadMissedMessages.test.tsapp/sagas/__tests__/state.test.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.test.tsxapp/views/RoomView/index.tsx
📚 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-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.test.tsxapp/views/RoomView/index.tsx
🔇 Additional comments (27)
app/containers/MessageActions/index.tsx (1)
252-252: LGTM!app/views/RoomView/constants.ts (1)
3-13: LGTM!app/views/RoomView/definitions.ts (1)
32-65: LGTM!app/views/RoomView/index.test.tsx (1)
1-1: 📐 Maintainability & Code QualityRun this suite with
TZ=UTC.Ensure the Jest invocation for this new timezone-sensitive test file sets
TZ=UTC, including in CI. As per coding guidelines, Jest tests must run withTZ=UTCfor deterministic timezone-dependent behavior.Source: Coding guidelines
app/views/RoomView/index.tsx (1)
120-185: LGTM!Also applies to: 1436-1448
app/views/RoomView/services/getMessages.ts (1)
5-15: LGTM!CONTEXT.md (1)
107-122: LGTM!Also applies to: 261-261
app/lib/methods/loadMessagesForRoom.test.ts (1)
8-8: LGTM!Also applies to: 35-35, 237-287
app/lib/methods/loadSurroundingMessages.ts (1)
53-55: LGTM!app/lib/methods/writeSyncWatermark.test.ts (1)
1-88: LGTM!app/lib/methods/writeSyncWatermark.ts (1)
1-47: LGTM!app/lib/methods/subscriptions/room.test.ts (1)
1-102: LGTM!app/lib/methods/subscriptions/room.ts (2)
65-82: LGTM! Removing theupdateLastOpenwrite on close correctly stops the device clock from poisoning the sync cursor while offline, matching theroomCloseCursor.test.tsregression tests.
46-63: 🩺 Stability & AvailabilityNo change needed here.
RoomViewcallsRoomClass.subscribe()only through its instance lifecycle, and the resubscribe path wheresubscribe()is invoked while the same instance already holdssub.promisesis not exercised by the current code path.> Likely an incorrect or invalid review comment.app/lib/methods/subscriptions/roomCloseCursor.test.ts (1)
1-128: LGTM!app/lib/methods/loadMessagesForRoom.ts (2)
36-45: LGTM!
149-154: LGTM!app/lib/methods/loadMissedMessages.test.ts (2)
46-99: LGTM!
149-173: LGTM!app/lib/methods/loadMissedMessages.ts (2)
15-15: LGTM!Also applies to: 61-90
95-113: LGTM!app/lib/methods/loadNextMessages.ts (1)
39-40: LGTM!app/sagas/__tests__/state.foregroundResync.test.ts (2)
1-53: LGTM!
95-121: LGTM!app/sagas/__tests__/state.test.ts (1)
79-125: LGTM!app/sagas/state.js (1)
19-32: LGTM!app/lib/methods/readMessages.ts (1)
8-8: 🗄️ Data Integrity & IntegrationNo action needed.
readMessagesis only called with two arguments, andupdateLastOpenis no longer present in the codebase.
GitHub Actions
Error
|
Android Build Available Rocket.Chat 4.75.0.109423 Internal App Sharing: https://play.google.com/apps/test/RQQ8k09hlnQ/ahAO29uNRNSw-fj_V4lKiwqYQX48gQI0nm26aeC6bjcpPMPz_ZWWtT0tUzP_O53MpBM0IGo2WJ-BJAkGswhZi8QivV |
|
iOS Build Available Rocket.Chat 4.75.0.109424 |
An edited older message can postdate batch 1's newest _updatedAt, so a first-batch-only snapshot could leave the cursor below it and miss later edits. Snapshot the raw _updatedAt of all batches before updateMessages mutates rows, and collapse updateLastOpen's max loop to Math.max with the invalid-date filter kept.
|
I tested this PR following the ticket video instructions and using a newly created channel and I could not reproduce the issue! Passing test cases:
Issue still present:
Potential known issue:
|
GitHub Actions
Error
GitHub Actions
Error
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/views/RoomView/index.tsx (1)
704-709: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winRetry scheduling is unmount-unsafe and unbounded.
On
init()failure this always armsretryInitTimeoutat a fixed 300ms with no check onthis.mountedand no retry cap/backoff. If the component unmounts while a rejectedinit()is settling (or on persistent failure, e.g. offline), this can callsetStateon an unmounted instance and retry indefinitely. A prior review round already flagged this exact "don't schedule retries after unmount" concern without an evidenced fix, and the corresponding unmount/backoff test coverage was removed fromindex.test.tsxin this PR (per the AI summary), so there's currently no test guarding this path either.🔒 Proposed fix
} catch (e) { this.setState({ loading: false }); - this.retryInitTimeout = setTimeout(() => { - this.init(); - }, 300); + if (this.mounted) { + this.retryInitTimeout = setTimeout(() => { + if (this.mounted) { + this.init(); + } + }, 300); + } }🤖 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 704 - 709, Update the init() failure handling and retry scheduling to check this.mounted before calling setState or creating a retry timer, and ensure the timer callback rechecks this.mounted before invoking init(). Replace the unbounded fixed 300ms retry loop with the component’s existing retry limit/backoff mechanism, and clear any pending retry timeout during unmount.
🤖 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/views/RoomView/index.tsx`:
- Around line 681-694: Update the readMessages/setLastSeen flow in readMessages
so its “seen up to” boundary comes from the server-validated sync cursor or
freshly fetched messages returned by RoomServices.getMessages, not new Date().
Rename newLastOpen to reflect that boundary and keep the existing joined-room
alert/unread handling unchanged.
- Line 696: Update the catch handler for readMessages in RoomView to call the
shared log(e) helper instead of console.log(e), matching the error-reporting
pattern used by the other catch blocks in the file.
---
Outside diff comments:
In `@app/views/RoomView/index.tsx`:
- Around line 704-709: Update the init() failure handling and retry scheduling
to check this.mounted before calling setState or creating a retry timer, and
ensure the timer callback rechecks this.mounted before invoking init(). Replace
the unbounded fixed 300ms retry loop with the component’s existing retry
limit/backoff mechanism, and clear any pending retry timeout during unmount.
🪄 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: 52795e2b-f292-4bbe-aaf1-8ea140e6d553
📒 Files selected for processing (7)
CONTEXT.mdapp/lib/methods/loadMessagesForRoom.test.tsapp/lib/methods/loadMessagesForRoom.tsapp/lib/methods/loadMissedMessages.test.tsapp/lib/methods/loadMissedMessages.tsapp/views/RoomView/index.test.tsxapp/views/RoomView/index.tsx
💤 Files with no reviewable changes (2)
- app/lib/methods/loadMissedMessages.test.ts
- app/lib/methods/loadMissedMessages.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- CONTEXT.md
📜 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/views/RoomView/index.test.tsxapp/lib/methods/loadMessagesForRoom.test.tsapp/lib/methods/loadMessagesForRoom.tsapp/views/RoomView/index.tsx
**/*.{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/index.test.tsxapp/lib/methods/loadMessagesForRoom.test.tsapp/lib/methods/loadMessagesForRoom.tsapp/views/RoomView/index.tsx
**/*.{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/index.test.tsxapp/lib/methods/loadMessagesForRoom.test.tsapp/lib/methods/loadMessagesForRoom.tsapp/views/RoomView/index.tsx
**/*.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/index.test.tsxapp/lib/methods/loadMessagesForRoom.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/index.test.tsxapp/lib/methods/loadMessagesForRoom.test.tsapp/lib/methods/loadMessagesForRoom.tsapp/views/RoomView/index.tsx
📚 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.test.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/views/RoomView/index.test.tsxapp/lib/methods/loadMessagesForRoom.test.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/views/RoomView/index.test.tsxapp/views/RoomView/index.tsx
🔇 Additional comments (5)
app/lib/methods/loadMessagesForRoom.test.ts (1)
254-268: LGTM!app/lib/methods/loadMessagesForRoom.ts (1)
36-49: LGTM!Also applies to: 78-82, 118-118, 129-129, 149-154
app/views/RoomView/index.tsx (2)
121-121: LGTM!Also applies to: 180-180, 1125-1132, 1408-1420
265-306: 🎯 Functional CorrectnessNo change needed.
stateAttrsUpdatealready includeslastSeen, solastSeenchanges are covered byshouldComponentUpdate.app/views/RoomView/index.test.tsx (1)
138-150: LGTM!
readMessages wrote `ls` from the device clock while POSTing subscriptions.read, so a skewed clock drew the unread separator early or late until the next subscriptions.get corrected it. The server already stamps `ls` and the subscription stream delivers it via createOrUpdateSubscription, so drop the optimistic write and the now unused `ls` parameter.
GitHub Actions
Inactive
GitHub Actions
Inactive
diegolmello
left a comment
diegolmello
left a comment
There was a problem hiding this comment.
Two-axis review of this PR against its base (purring-seahorse): Standards (repo + project conventions, plus a Fowler smell baseline) and Spec (SUP-1078 + the PR description). Findings are inline. Worst on each axis: the test-only export class RoomView, and loadMissedMessages still silently no-op'ing when the subscription row / room type can't be resolved — which is the exact failure mode SUP-1078 reports.
One finding has no diff line to attach to: app/views/RoomView/docs/ARCHITECTURE.md keeps an Invariants section, and the new "init dispatches on cursor presence" predicate isn't recorded there even though index.tsx init behaviour changed.
|
iOS Build Available Rocket.Chat 4.75.0.109444 |
getMessages is the sole gate between loadMessagesForRoom and loadMissedMessages, so the no-cursor fallback and the future-cursor guard duplicated its job. Cursor writes are server-derived now, so a skewed device clock can no longer poison lastOpen.
Both loaders now accumulate the raw server `_updatedAt` of every batch or page they walk and write the cursor once, at the end, instead of trusting a single batch to carry the newest stamp. `loadMessagesForRoom` snapshots inside `fetchBatch` for every batch, not only the first: the tail load can recurse over several pages of history when hidden system messages consume the visible page, and the fetch order within that load is not part of the contract. Taking the max across all of them is future proof — if the first batch ever became the oldest one, a first-batch-only snapshot would silently lower the cursor. `loadMissedMessages` threads the accumulator through its pagination. Its pages descend from the newest, so writing from the last page alone landed the cursor on the oldest page walked. The write still fires only once the UPDATED cursor has drained, and only on a call that actually fetched an UPDATED page — a DELETED-only continuation must not write again. An abort mid-pagination writes no cursor: the run is re-fetched. `updateLastOpen` is untouched and stays deliberately non-monotonic, so an older server value can still heal a poisoned cursor. The write gate in `loadMessagesForRoom` is untouched too: older pages (`latest`) and gap fills (`loaderItem`) never reach the newest history regardless of fetch order, so letting them write could only lower the cursor.
- catch rejections from the un-awaited pagination continuation - skip null _updatedAt payload rows so a null-only payload can't write an epoch-0 cursor - annotate the resume-sync fixture helper and assert the persisted cursor is sent to chat.syncMessages - re-point the null-lastOpen resume test: the history fallback was removed, RoomView's getMessages owns the initial load
GitHub Actions
Inactive
2 participants
Proposed changes
subscription.lastOpendid double duty: it was both the fetch cursor handed tochat.syncMessagesand the anchor for the unread separator. It was also written from the device clock, while the server compares the cursor against each message's_updatedAt(server clock). Any skew, or any write not tied to a real fetch, silently moved the cursor past messages that were never delivered — and they were never fetched again.The direction matters, and only one side is safe:
The two roles are now split, and the cursor only ever moves to a timestamp the server produced:
updateLastOpennow takes a raw server payload instead of defaulting tonew Date(). It writes the max_updatedAtin that payload, read beforenormalizeMessageruns (that path invents anew Date()fallback for rows that lack one). It is deliberately non-monotonic, so an already-poisoned future cursor heals downward.readMessagesno longer writeslastOpenat all (it ownsls), andRoomSubscription.unsubscribeno longer stamps the cursor on room close — that write was what advanced the cursor past messages posted while offline.MessageActions) still writeslsfrom the tapped message'sts— it rewinds the unread separator and is now the only device-clock writer oflsleft. It never touches the cursor.loadMissedMessagesresolves the cursor from the subscription row itself, treats a future-dated cursor as absent so it self-heals, falls back toloadMessagesForRoomwhen there is no cursor instead of no-op'ing, and advances the cursor only once the UPDATED page cursor has drained.loadMessagesForRoomsnapshots the first batch's server_updatedAtvalues and hands them toupdateLastOpen; paging older history and gap fills still never move the cursor.Separator behaviour is unchanged —
RoomViewstate renameslastOpen→lastSeenand feeds the separator fromls, so the two meanings cannot collapse into one column again.CONTEXT.mdgains a Timestamp Trust Boundary section naming Last Open, Last Seen, Server Timestamp and Device Timestamp.Issue(s)
https://rocketchat.atlassian.net/browse/SUP-1078
How to test or reproduce
Offline room close.
Post at least three:
subscriptions/rooms.tsback-fillslastMessageinto the messages table for rooms you are not in, which self-heals any single-message repro.Screenshots
Recorded on an Android 36 emulator against
mobile.qa.rocket.chat, before/after on the same sequence.Offline room close — before: the cursor jumps to the device clock at close time (11:26:14, past all three messages), and A-M1/A-M2 never arrive. After: the cursor equals the last message's server
_updatedAtto the millisecond (1785164328836=14:58:48.836Z) and all three are present.Types of changes
Checklist
Further comments
Stacked on #7521 (base
purring-seahorse) — that PR rolls back the #7298 socket probing, so it needs to land first.221 suites / 2027 tests pass,
tscclean, 0 lint errors.