chore: combined QA build for #7523 + #7526 by diegolmello · Pull Request #7527 · RocketChat/Rocket.Chat.ReactNative

diegolmello · 2026-07-28T15:49:57Z

Proposed changes

Integration branch combining #7523 (server-clock sync cursor + foreground REST re-sync) and #7526 (VoIP stale-socket reconnect) on top of #7521, so both fixes can be verified on a single build. Not for review — the real review happens on the individual PRs. Merge conflicts in connect.ts, sagas/state.js, and sagas/__tests__/state.test.ts resolved here; rerere recorded, so the same resolution replays when the PRs land sequentially.

Issue(s)

How to test or reproduce

Single-device-session QA plan:

VoIP lock-screen accept (7526, physical iOS): lock device ≥30s, incoming VoIP call, accept from lock screen. Audio path should open; no timeout-remote-sdp at 10s.
Foreground re-sync (7523 + 7526 interaction): sit in a room, background past socket lifetime, another account posts, reopen from app icon. Missed messages appear (REST re-sync) and streams resume (socket reopen).
Offline room close (7523): open channel, go offline, back out, other account posts 3+ messages, come back online, re-enter. All messages present.
Notification tap / new room (7523): backgrounded + offline, other account creates channel, adds you, posts 3 messages, restore network, tap notification. Room opens with all messages.

Automated: 227 suites / 2090 tests pass on this branch (TZ=UTC pnpm test).

Screenshots

N/A — see the individual PRs for before/after recordings.

Types of changes

Bugfix (non-breaking change which fixes an issue)

Checklist

I have read the CONTRIBUTING doc
I have signed the CLA
Lint and unit tests pass locally with my changes

Further comments

Draft — exists to produce iOS/Android build artifacts for the combined QA session. Close after QA, or keep until both PRs merge.

`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.

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.

…n row A room with lastOpen resumes via the missed-messages loader; a room without one pulls history directly, ending the double fetch when a push-notification open starts before the subscription row arrives. Also renames the read-receipt local newLastOpen to readReceiptTime so it no longer masquerades as a cursor.

Rapid foreground/background cycles were dispatching roomsRequest() on every FOREGROUND, hammering the rooms-delta endpoint. Add a 60-second module-level throttle so cycles inside the window collapse to a single delta fetch, while a foreground after the window still heals the rooms list over REST. The 60-second window balances spam protection against missed-rooms risk: the call exists to recover from a silently dead socket, so a window much longer would re-open real missed-rooms windows, while the spam scenario is seconds to a minute.

…essages-notification # Conflicts: # app/lib/methods/subscriptions/room.test.ts

Two foreground tests in state.test.ts ran within the same real-time 60s window, so the throttle in state.js suppressed the second dispatch and CI went red. Mock Date.now to jump 2 minutes per read so each test's foreground is always eligible.

…P socket The SDK send() waits on a 'disconnected' event that nothing ever emitted, so zombie sockets caused in-flight sends to hang forever. Add reopenNow() to force a single shared reconnect and emit 'disconnected' to reject those sends, plus a bounded probe() for gray-zone liveness checks. Restore ddpSocket.test.ts with coverage for probe, reopenNow, subscription preservation, concurrent reconnect deduplication, and the send() listener-leak fix. Add @rocket.chat/sdk and tiny-events to Jest's transform-ignore exceptions so the SDK's TypeScript source is transformed.

After a long suspension the DDP socket can be a zombie: readyState=1 and connected=true while sends hang and no pong arrives. The native accept path previously replayed/answered immediately, so the WebRTC setup timed out at the remote-sdp stage. Add a single guarded accept helper that every accept path funnels through: - classify the socket by lastPing age and force reopenNow() when stale; - wait for login readiness and for the media-signal/media-calls subscriptions to be acked on the current socket; - replay REST state signals and answer only if the call is not already bound; - on timeout/failure terminate the native call, reset the native accepted id, and queue a best-effort hangup. Expose the socket via sdk.current.ddp and add DDPDriver passthroughs plus a waitForNotifyUserMediaSubs readiness helper in the SDK patch. Also guard checkVoipPermission so it does not reset the media session while a call is active or being accepted.

Extract the age/ping classification into classifySocketHealth in waitForLoginReady.ts and make the foreground-saga helper getSocketStaleness delegate to it. The accept gate imports from the lightweight helper file to avoid pulling connect.ts (and its heavy deps) into the VoIP unit tests.

On foreground after long suspension the DDP socket can be zombie while redux still reads connected=true. Classify socket freshness via lastPing and pingInterval: reopen immediately when stale, probe in the gray zone, and keep the existing checkAndReopen path for healthy/fresh sockets. Adds an in-flight probe guard so rapid AppState flaps do not stack probes.

Aborted gates now return early without running the failure ladder (terminate/endCall). activeGates cleanup only deletes its own controller so newer gates survive. Live-signal 'accepted' notifications from the stream listener funnel through acceptNativeCallWithReadiness instead of calling answerCall directly. SDK waitForNotifyUserMediaSubs now polls up to the timeout for media-signal/media-calls subscriptions to appear after a forced reopen. Also type the DDP shape in acceptNativeCall and remove "as any" from the AbortSignal fallback.

…nto tmp-merge

…3-7526-on-7521 # Conflicts: # app/lib/services/connect.ts # app/sagas/__tests__/state.test.ts # app/sagas/state.js

coderabbitai · 2026-07-28T15:50:05Z

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 2204ff40-855c-405d-86f6-f1270203c1c3

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

🔍 Trigger review

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

github-actions · 2026-07-28T16:45:56Z

Android Build Available

Rocket.Chat 4.75.0.109436

Internal App Sharing: https://play.google.com/apps/test/RQQ8k09hlnQ/ahAO29uNRDIwYV0NxqYfBR1P1fJGW51OYkOmZv7MnP-IzLHYQ-FZZ9-zVoDe1sxbs0HPp4Rxk4AuzMXw1azKnv0hRM

github-actions · 2026-07-28T17:12:07Z

iOS Build Available

Rocket.Chat 4.75.0.109437

diegolmello

Structural quality pass on the combined diff (harsh-review request). Verdict: the bugs are real, the Last Open vs Last Seen split is the right reframing, and test coverage is strong — but the implementation layers too many ad-hoc state machines, module-level mutable state, and patched-SDK behavior. Land the conceptual model; refactor the machinery before this becomes the baseline. Inline comments ordered by severity.

Highest-leverage restructurings if we want to go further:

Model the sync cursor as a value object (resolve() / isLying() / heal()) — kills the future-check, normalizeCursor, and most branching in loadMissedMessages.
Eliminate module-level Maps/flags (lastTailLoadAttemptByRoom, isProbingSocket, lastRoomsRequestAt, activeGates) in favor of explicit context or an existing store.
Make acceptNativeCallWithReadiness a pure readiness checker returning { ready, reason }; let MediaSessionInstance own answer/terminate/retry.

diegolmello · 2026-07-28T17:16:04Z

-			lastOpenTimestamp = lastUpdate?.getTime();
-		}
-		const result = await getSyncMessagesFromCursor(roomId, lastOpenTimestamp, updatedNext, deletedNext);
+		const result = await getSyncMessagesFromCursor(roomId, cursor?.getTime(), updatedNext, deletedNext);


Major — this function is now a monolith. It resolves the cursor, heals future cursors, falls back to full history, branches on server version, paginates, advances the cursor, and runs the lying-cursor detector with cooldown. That's four loader jobs in one flow.

Split into resolveCursor(subscription) / syncFromCursor(cursor) / healCursorIfLying(roomId, cursor, serverResponse) as separate units. Better: model the cursor as a small value object with resolve() / isLying() / heal() — most of this branching disappears.

diegolmello · 2026-07-28T17:16:04Z

 const count = 50;
+const TAIL_LOAD_COOLDOWN_MS = 5 * 60 * 1000;
+
+const lastTailLoadAttemptByRoom = new Map<string, number>();


Major — module-level mutable state. lastTailLoadAttemptByRoom is hidden state that makes the loader impure and tests order-dependent. Same pattern appears in sagas/state.js (isProbingSocket, lastRoomsRequestAt) and acceptNativeCall.ts (activeGates). Pass a context object through, or put cooldown state in an existing store.

diegolmello · 2026-07-28T17:16:05Z

 	}

-	const [updatedMessages, deletedMessages] = await Promise.all(promises);
+	const [updatedMessages, deletedMessages] = await Promise.all([updatedPromise, deletedPromise]);


Minor — Promise.all([updatedPromise, deletedPromise]) with let-declared possibly-undefined slots. Works, but the undefined-slot semantics are subtle. Build a promises array (as before) or make the UPDATED/DELETED calls explicit and parallel without the lets.

diegolmello · 2026-07-28T17:16:05Z

 	}

 	const allMessages: IMessage[] = [];
+	const serverUpdatedAt: { _updatedAt?: string | Date }[] = [];


Major — history loader coupled to cursor persistence. The loader now returns a parallel serverUpdatedAt array and calls updateLastOpen only because updateMessages mutates _updatedAt (the comment admits this). History loading shouldn't know about sync-cursor persistence.

Cleaner: make updateMessages non-mutating for _updatedAt, or snapshot server timestamps at the caller that owns the cursor decision and keep this loader agnostic.

diegolmello · 2026-07-28T17:16:05Z

+export async function updateLastOpen(rid: string, payload: { _updatedAt?: string | Date }[]): Promise<void> {
 	try {
-		const db = database.active;
+		const timestamps = payload.map(m => new Date(m._updatedAt as string | Date).getTime()).filter(t => !Number.isNaN(t));


Minor — cast hides undefined. m._updatedAt as string | Date papers over the optional field; new Date(undefined) yields NaN which is then silently filtered. Handle it explicitly: const t = m._updatedAt; if (t == null) return NaN; — or tighten the payload type so the invariant is real.

diegolmello · 2026-07-28T17:16:05Z

+   * If the subscriptions are not yet present (e.g. immediately after reopenNow),
+   * it polls the socket subscription map until they appear or the timeout expires.
+   */
+  waitForNotifyUserMediaSubs = (timeoutMs = 8000): Promise<boolean> => {


Major — too much behavioral logic in a patch file. This now adds reopenNow, probe, waitForNotifyUserMediaSubs (a 100ms subscription-map poll), connection-identity guards, and exposes ddp as any. 400+ lines of lifecycle behavior in a node_modules patch is high-maintenance debt and a regression risk on every SDK bump.

Preferred: upstream the reopenNow/probe hooks to the SDK, or wrap the SDK in an app-owned adapter that owns reconnect/probe orchestration.

diegolmello · 2026-07-28T17:16:05Z

+	mediaSession.endCall(callId);
+}
+
+export async function acceptNativeCallWithReadiness(callId: string, mediaSession: NativeCallMediaSession): Promise<void> {


Major — this gate mixes too many concerns. One module does socket classification, reopen/probe orchestration, login readiness, media-subscription polling, abort handling, concurrent-call gating (activeGates), and call teardown. handleFailure does three unrelated things, and the timeouts (8000/2000/etc.) are hardcoded inline.

Split into layers: prepareSocketForCall(ddp) / awaitCallReadiness(ddp, signal) / abortOrRun(callId, fn). Make the gate a pure readiness checker returning { ready, reason } and move terminate/end/reset back to MediaSessionInstance — failure paths become testable in isolation. Name the timeout constants.

diegolmello · 2026-07-28T17:16:05Z

+
+const activeGates = new Map<string, AbortController>();
+
+function onAbort(signal: AbortSignal | undefined, callback: () => void): void {


Minor — duplicated helper. This onAbort is byte-identical to the one in waitForLoginReady.ts. Extract to a shared abort-utils.ts.

diegolmello · 2026-07-28T17:16:05Z


+/** Classify DDP socket freshness from last pong. Returns 'fresh' when the SDK
+ *  lacks the new probe/reopen hooks so callers fall back to checkAndReopen. */
+export function getSocketStaleness(ddp: any): 'stale' | 'gray' | 'fresh' {


Minor — thin synonym layer. getSocketStaleness just maps classifySocketHealth's 'healthy'/'probe'/'reopen' onto 'fresh'/'gray'/'stale'. Two functions, two vocabularies, one concept. Make classifySocketHealth return the caller-facing categories directly and delete this wrapper.

diegolmello · 2026-07-28T17:16:05Z

 	private subSubscription?: Subscription;
 	private queryUnreads?: Subscription;
 	private retryInitTimeout?: ReturnType<typeof setTimeout>;
+	private initRetries = 0;


Major — RoomView keeps accumulating ad-hoc state. An already oversized class gains initRetries, initializing, didAdoptSubscriptionRow, plus exponential-backoff retry, and the notification-tap subscription adoption is bolted onto observeSubscriptions (with more inline magic numbers: retry counts, delays).

Extract a RoomInitialization hook/service that owns rid→subscription adoption, retry policy, and loader dispatch. RoomView should consume a ready subscription and call init(), not orchestrate the adoption race.

diegolmello

Deep review — 14 analysis angles

Posting here per request, though the PR body notes review belongs on the individual PRs: findings on patches/, acceptNativeCall.ts, MediaSessionInstance.ts, MediaCallEvents.ts, connect.ts, waitForLoginReady.ts and state.js's socket ladder are #7526; findings on the cursor/loader/readMessages/RoomView-init files are #7523.

Blocking

#7526 — the SDK patch has 7 defects, 5 of which break reconnect, keepalive, or a pending send for the remainder of the session. All five trace to two root causes: once + off(self) against tiny-events' splice(-1, 1) fallback (verified by running the vendored copy), and reopenPromise resolving on timeout rather than rejecting. Separately the patch has grown 58 -> 310 lines and now carries app domain logic in node_modules, with no test able to catch a bad re-derivation on the next SDK bump. Recommend reworking it rather than adding hunks.

Also on #7526: the readiness gate terminates the call it was added to protect on iOS cold start (two independent causes), answers the call twice on the warm path (the second accept() tears down a call that succeeded), and can take ~20s before doing either.

#7523 — two user-visible regressions on top of the sync bugs. Mark-unread is broken (the write moved from lastOpen to ls, which readMessages stomps), and every foreground now marks the open room fully read — clearing unread the user never saw, locally and server-side.

Structural

The original bug is still open on the most common path. Redux meteor.connected only clears on a 'close' event a half-open socket never fires, so it latches true over a socket the SDK already considers dead. Welding the heal to APP_STATE.FOREGROUND means a socket dying while foregrounded (wifi->cellular, captive portal, VPN flap) is never detected. Reconciling that one latch would let the ladder, the REST re-sync and the throttled roomsRequest all be deleted.
Both cursor self-heals should be one migration. The comment says "There is no migration for those rows", but migration 29 already uses unsafeExecuteSql for exactly this. As written, normalizeCursor misses the common poisoned case and never converges when it does fire.
The socket-health contract has three unlinked definitions and its remediation ladder is copy-pasted in two files that have already drifted within this PR.

48 inline comments below with the specific mechanism and failure path for each.

diegolmello · 2026-07-28T17:18:24Z

+      }
+
+      if (this.reopenPromise) {
+        return this.reopenPromise.then(() => resolve(this.connection)).catch(reject)


reopenPromise resolving on timeout kills the SDK's reconnect retry loop.

reopenNow's 10s timer calls resolve() regardless of connection state (createConnection().catch(() => {}) already swallowed the failure). open() now returns this.reopenPromise.then(() => resolve(this.connection)), so when the device is offline open() resolves successfully with a dead/CONNECTING socket after 10s.

reopen() is try { await this.open() } catch { this.reopen() } — it only re-arms in the catch. That catch never runs, nothing is scheduled, and there is no live connection left to fire onClose → reopen(). The client sits offline with no retry until the next foreground.

Same path makes DDPDriver.connect() neither resolve nor reject: its this.ddp.open().catch(reject) no longer rejects, and it is still awaiting a once('connected') that will never come.

Reject on timeout (or resolve with a boolean the caller checks).

diegolmello · 2026-07-28T17:18:24Z

+    }
+
+    this.reopenPromise = new Promise<void>(resolve => {
+      this.openTimeout && clearTimeout(this.openTimeout as any)


clearTimeout without delete permanently disables automatic reconnect.

clearTimeout(this.openTimeout); // handle stays truthy

reopen() guards with if (this.openTimeout) return, and the cleared timer's callback is the only other code that deletes the field — it can never run now. checkAndReopen does delete this.openTimeout; this path doesn't.

Trigger is exactly the scenario this PR targets: socket dies in background → onClose schedules reopen() (sets openTimeout) → foreground classifies stale → reopenNow() cancels the timer and leaves the handle. From then on every onClose → reopen() early-returns. The app loses all automatic reconnection for the session; a socket dropping while foregrounded stays down indefinitely.

Compounds with the foreground-welded healing below: together this makes the foreground-drop case worse than before the PR.

diegolmello · 2026-07-28T17:18:24Z

+    this.reopenPromise = new Promise<void>(resolve => {
+      this.openTimeout && clearTimeout(this.openTimeout as any)
+      this.lastPing = 0
+      this.emit('disconnected')


reopenNow never emits 'close', so redux stays latched and the new socket is never authenticated — the readiness gate becomes a no-op.

createConnection() detaches the old socket's onclose before closing it, so onClose never runs and emit('close') never fires. connect.ts's closeListener is the only thing dispatching disconnectAction(), so redux meteor.connected stays true across a brand-new DDP session that has no login.

Then onOpen → driver 'connected' → connect.ts:121 reads meteor.connected === true → early-returns → loginRequest({ resume }) is never dispatched.

Downstream, acceptNativeCall.ts:104's waitForLoginReady(8000) reads isAuthenticated && meteor.connected off that stale flag and returns ready immediately. waitForNotifyUserMediaSubs then re-subscribes on the unauthenticated socket; Socket.subscribe swallows the server's nosub in its own .catch, and resubscribe's .then(() => true) reports success.

Net: the gate says ready, the signalling stream is dead, and the call answers into nothing. This is the gate that #7526 exists to add.

Secondary: emit('disconnected') passes no argument, so send()'s once('disconnected', reject) rejects with undefined, and the SDK's own handlers (ddp.ts:305, ddp.ts:395) dereference err.message → TypeError, replacing the real error.

diegolmello · 2026-07-28T17:18:24Z

+      const cleanup = () => {
+        if (settled) return
+        settled = true
+        this.off('open', cleanup)


this.off('open', cleanup) inside a once callback deletes an unrelated listener.

tiny-events' once wraps the listener and calls self.off(type, __once) before invoking it. So by the time cleanup runs, neither cleanup nor a wrapper with .listener === cleanup is in the array — index stays -1 and off falls through to splice(-1, 1), removing the last 'open' listener.

Reproduced directly against the vendored node_modules/tiny-events: with _listeners['open'] = [driverEcho, __once(cleanup)], emitting 'open' leaves the array empty — driverEcho is gone.

That echo is DDPDriver.connect's permanent this.ddp.on('open', () => this.emit('connected')) (ddp.ts:503). After the first reopenNow(), connect.ts:120's connectedListener never runs again for the session: no connectSuccess(), no loginRequest({ resume }). Only a full sdk.initialize + connect() re-registers it — checkAndReopen() goes straight to ddp.open() and does not.

If a Socket.send open-waiter (ddp.ts:255) happens to be last instead, that send hangs forever.

Fix: drop the off call (once already removed it), or use on + explicit removal.

diegolmello · 2026-07-28T17:18:24Z

+
+      this.createConnection().catch(() => {})
+
+      const timeout = setTimeout(() => cleanup(), 10000)


lastPing = 0 is never restored on a failed reopen, so every later health check reads stale.

reopenNow sets lastPing = 0 at line 125 and only a successful onOpen restores it — but the 10s timeout resolves rather than rejects. So after a failed reopen, classifySocketHealth computes age = Date.now() - 0 and returns 'reopen' on every subsequent read.

Every foreground transition and every call accept then starts another full 10s reopenNow, with no backoff and no way to distinguish "just reopened, first pong pending" from "dead for ten minutes".

Combined with the serialized gate in acceptNativeCall.ts, an offline device pays reopenNow 10s + waitForNotifyUserMediaSubs 8s = 18s before handleFailure finally terminates the CallKit call.

diegolmello · 2026-07-28T17:18:26Z


-		await this.applyRestStateSignals();
+		const { nativeAcceptedCallId } = useCallStore.getState();
+		if (nativeAcceptedCallId) {


init() used to unconditionally await this.applyRestStateSignals(). Now, when nativeAcceptedCallId is set, the only path to it is through the readiness gate — and any gate failure both skips the REST replay and terminates the native call.

Warm relaunch with a lock-screen-accepted call: checkVoipPermission (login.js:270) runs from selectServer's DB-read phase, before sdk.current.login() sets DDPDriver.userId. waitForNotifyUserMediaSubs returns Promise.resolve(false) on !this.userId → mediaSubsReady === false → handleFailure → call terminated, and applyRestStateSignals() — which in base recovered the ongoing call from mediaCallsStateSignals — never runs.

Same whenever the media subs simply don't land within 8s on a slow reconnect.

diegolmello · 2026-07-28T17:18:26Z

 	if (data.host && isVoipIncomingHostCurrentWorkspace(data.host, adapters.getActiveServerUrl)) {
-		mediaSessionInstance.applyRestStateSignals().catch(error => {
-			mediaCallLogger.error(`${TAG} applyRestStateSignals failed:`, error);
+		mediaSessionInstance.acceptNativeCallWithReadiness(data.callId!).catch(error => {


data.callId! — callId is only ever checked in non-blocking if (callId) guards upstream, so undefined can reach activeGates.set() / terminateNativeCall().

diegolmello · 2026-07-28T17:18:26Z

@@ -0,0 +1,129 @@
+jest.mock('../../lib/methods/helpers/localAuthentication', () => ({


Lines 1-42 are byte-identical to state.foregroundResync.test.ts:1-42 (confirmed by diff), and state.test.ts:1-40 repeats 7 of the same 8 jest.mock blocks — three copies of one saga's mock graph.

The only reason to split was resetting module-level lastRoomsRequestAt, which this file already solves in-place with jest.resetModules() + require('../state') at line 63. Merge with the sibling, or share a setup module.

state.js gained four imports in this PR; each had to be mocked three times. The next one fails all three suites until pasted a third time — and a mock updated in only two lets one suite assert against a stale collaborator.

diegolmello · 2026-07-28T17:18:26Z

+	return store;
+}
+
+function makeDdp(overrides: Record<string, any> = {}) {


makeDdp is written three times — here, waitForLoginReady.socketHealth.test.ts:21-28, and acceptNativeCall.test.ts:60-69 — with no shared factory and no type constraining them. They already disagree: only the socketHealth one has config: { ping }, only the VoIP one has waitForNotifyUserMediaSubs.

That matters because a fake missing a field makes the production capability sniff (typeof ddp.probe !== 'function') take the legacy branch — so the test passes while asserting the fallback path instead of the feature under test. Silently green coverage for the exact reconnect logic this PR exists to add.

__mocks__/@rocket.chat/sdk.js already exists and is the natural home.

diegolmello · 2026-07-28T17:18:26Z

+			params: ['uid/media-calls'],
+			unsubscribe: jest.fn()
+		};
+		jest.spyOn(driver, 'subscribe').mockResolvedValue({});


This asserts only the 4-argument call shape of driver.subscribe, which is exactly what hides the bug flagged on the patch: DDPDriver.subscribe is (topic, eventname, ...args), so sub.id never reaches Socket.subscribe's id parameter and every "resubscribe" mints a duplicate.

The test locks in the broken behaviour. Assert on the resulting ddp.subscriptions keys instead.

diegolmello

Review of the combined QA build. Two blockers (cold-start appInit skip; accept-before-init terminates call) and eight worth-fixing items inline below.

Smaller nits, not inlined:

loadMissedMessages.ts:16 — lastTailLoadAttemptByRoom Map never cleared (bounded, tiny entries, but no cleanup on logout/room close).
loadMissedMessages.ts:62 — pagination re-reads the subscription row every recursion page; passing the cursor (or an explicit lastOpen param, as the old API had) avoids N+1 reads.
loadMissedMessages.ts:96-110 — normalizeCursor does getSubscriptionByRoomId + getMessageById before the cooldown check; moving the cooldown guard first skips both reads on suppressed runs.
loadMessagesForRoom.ts:22 — subscription row read twice (once for resolveHideSystemMessages, again inside updateLastOpen).
acceptNativeCall.ts:25 — onAbort helper duplicated verbatim from waitForLoginReady.ts:25; extract a shared helper.
SDK patch waitForNotifyUserMediaSubs — poll runs to the 8s deadline even if the socket dies mid-wait; consider clearing the interval on close/login-failure.
MediaSessionInstance.ts — tryAnswerIfNativeAcceptedNotification(signal, useGate) boolean leaks path knowledge; two named methods would read better.
state.js — module-level isProbingSocket/lastRoomsRequestAt could be declarative saga effects (throttle/takeLatest).
connect.ts:448 — getSocketStaleness(ddp: any); test file VoipCallLifecycle.integration.test.tsx:161,166 — 4-line comment (rule max 3) and another any.

diegolmello · 2026-07-28T17:31:48Z

+					mediaCallLogger.error(`${TAG} acceptNativeCallWithReadiness (initial) failed:`, error);
 				});
 				mediaCallLogger.log(`${TAG} Same workspace as VoIP host; skipped deepLinkingOpen`);
 				return true;


Blocker. Returning true here makes Root skip appInit immediately after launching the async readiness gate. If the gate then fails, handleFailure only ends the call — nothing re-dispatches appInit/deep-linking, so the app is left past the startup gate in an uninitialized state. The failure path needs to run normal startup when the gate doesn't succeed.

diegolmello · 2026-07-28T17:31:48Z

+			return;
+		}
+
+		if (!loginReady || !mediaSubsReady || !mediaSession.isInitialized()) {


Blocker. The gate waits for socket/login/media-subs but checks mediaSession.isInitialized() exactly once and fails hard. On cold start (or the microtask after LOGIN.SUCCESS) a native accept event can arrive before MediaSessionInstance.init() completes — the call the user just accepted gets terminated. Wait for init (or defer the accept) instead of treating not-yet-initialized as a terminal failure.

diegolmello · 2026-07-28T17:31:48Z

 		await updateMessages({ rid: args.rid, update: updated, remove: deleted });

 		if (deletedNext || updatedNext) {
 			loadMissedMessages({


This recursive pagination call is fire-and-forget — no await, no .catch. The exported promise resolves after page 1 while later pages are still in flight, so the foreground saga (state.js:35-36) calls readMessages immediately after and can mark messages read before they've even been persisted. A rejection in a later page also becomes an unhandled promise rejection. await the recursion (or return it so callers can).

diegolmello · 2026-07-28T17:31:48Z

+
+	// A cursor in the future was written from a skewed device clock; the server would report
+	// nothing newer than it, permanently hiding messages. Treat it as absent so it self-heals.
+	const cursor = persistedCursor && persistedCursor.getTime() > Date.now() ? undefined : persistedCursor;


lastOpen is now server-stamped (updateLastOpen writes the raw payload _updatedAt), so a device clock running behind the server makes a perfectly legitimate cursor look future-dated. This guard then discards it and falls back to a full tail load on every sync for that room. The check can't distinguish a poisoned cursor (device clock ahead) from a normal server-ahead cursor — needs a tolerance window or a different poison detector.

diegolmello · 2026-07-28T17:31:48Z

+			checkAndReopen();
+		}
+
+		yield resyncSubscribedRoom();


This serializes the rooms-list refresh behind the full open-room sync (loadMissedMessages + readMessages) on the foreground hot path — new in this PR (base only called checkAndReopen()). The roomsRequest delta is independent of the open-room sync; dispatching it in parallel avoids delaying the rooms list by however long chat.syncMessages + read receipts take.

diegolmello · 2026-07-28T17:31:48Z

+					this.observeRoom(data[0]);
+					// The room was opened before its subscription row existed (notification tap): init() ran
+					// against a bare { rid }, so re-run it now to get the unread separator and read receipt.
+					this.setState({ room: data[0], joined: true }, () => {


Adoption re-runs init() but never clears a pending backoff retry scheduled by a previous failed init() (this.retryInitTimeout, set around :734). If the row materializes while a retry is pending, the stale retry fires after adoption-init succeeded → redundant second getMessages + readMessages. clearTimeout(this.retryInitTimeout) here before re-running init.

diegolmello · 2026-07-28T17:31:48Z

+			return;
+		}
+
+		const { call } = useCallStore.getState();


No abort is wired to a user hangup: if the user hangs up from the native UI while the gate is waiting, nothing cancels it (only a second gate for the same callId aborts the first). The gate runs to completion on a dead call — answerCall's missing-call branch terminates safely, so this isn't a re-answer bug, but wiring hangup → controller.abort() avoids doing answer work on a call that's already over.

diegolmello · 2026-07-28T17:31:48Z

 		}

-		await this.applyRestStateSignals();
+		const { nativeAcceptedCallId } = useCallStore.getState();


When nativeAcceptedCallId is set, init bypasses applyRestStateSignals() entirely and REST signals are only replayed after the gate succeeds. A readiness stall longer than the 8s timeout drops a call the old code would have answered from the REST payload. Matches the wait-until-ready design, but consider replaying REST state signals as a fallback on gate timeout before giving up on the call.

diegolmello · 2026-07-28T17:31:48Z

+	};
+
+	try {
+		const ddp = sdk.current?.ddp as VoipReadyDdp | undefined;


If the patched SDK surface is missing (reopenNow/probe/lastPing — e.g. patch not applied at install), the only path is handleFailure: every incoming VoIP accept is terminated. patch-package makes this unlikely, but a degraded fallback (checkAndReopen/ungated answer) beats a hard fail for a capability probe.

diegolmello · 2026-07-28T17:31:48Z


+/** Classify DDP socket freshness from last pong. Returns 'fresh' when the SDK
+ *  lacks the new probe/reopen hooks so callers fall back to checkAndReopen. */
+export function getSocketStaleness(ddp: any): 'stale' | 'gray' | 'fresh' {


Socket-health policy now lives in four places: this wrapper, the duplicated capability guard in acceptNativeCall.ts:85, the probe→reopen sequence in state.js:56, and the SDK patch. Two parallel vocabularies (fresh/gray/stale vs healthy/probe/reopen) and two copies of the reopenNow/probe/lastPing guard must be kept in sync. Worth consolidating classification + capability detection into one connect-layer helper both the saga and the VoIP gate use.

diegolmello added 23 commits July 27, 2026 00:08

fix(state): request the rooms delta on foreground

89392d7

`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.

test(RoomView): share one awaiting act flush helper

ba000ed

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.

fix(RoomView): lazy state reads in init to heal notification-tap race

6524e83

refactor(sync): align cursor terminology and drop dead code

9ec8dcf

fix(sync): throttle self-heal tail load per room

59b0d8b

Merge remote-tracking branch 'origin/purring-seahorse' into missing-m…

2a5e376

…essages-notification # Conflicts: # app/lib/methods/subscriptions/room.test.ts

fix(sdk): serialize forced reopen against concurrent open, harden probe

bb8220f

test(voip): align call-lifecycle integration tests with gated accept

79d151d

Merge remote-tracking branch 'origin/missing-messages-notification' i…

378af93

…nto tmp-merge

Merge remote-tracking branch 'origin/dry-badger' into integration/752…

0071edb

…3-7526-on-7521 # Conflicts: # app/lib/services/connect.ts # app/sagas/__tests__/state.test.ts # app/sagas/state.js

diegolmello temporarily deployed to approve_e2e_testing July 28, 2026 15:50 — with GitHub Actions Inactive

diegolmello temporarily deployed to ios_build July 28, 2026 15:53 — with GitHub Actions Inactive

diegolmello temporarily deployed to android_build July 28, 2026 15:53 — with GitHub Actions Inactive

diegolmello requested a deployment to upload_android July 28, 2026 16:44 — with GitHub Actions Waiting

diegolmello commented Jul 28, 2026

View reviewed changes

diegolmello closed this Jul 31, 2026

diegolmello deleted the integration/7523-7526-on-7521 branch July 31, 2026 18:28


		const activeGates = new Map<string, AbortController>();

		function onAbort(signal: AbortSignal \| undefined, callback: () => void): void {

		@@ -0,0 +1,129 @@
		jest.mock('../../lib/methods/helpers/localAuthentication', () => ({

Uh oh!

Conversation

diegolmello commented Jul 28, 2026

Proposed changes

Issue(s)

How to test or reproduce

Screenshots

Types of changes

Checklist

Further comments

Uh oh!

coderabbitai Bot commented Jul 28, 2026

Review skipped

Uh oh!

github-actions Bot commented Jul 28, 2026

Uh oh!

github-actions Bot commented Jul 28, 2026

Uh oh!

diegolmello left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

diegolmello left a comment

Choose a reason for hiding this comment

Deep review — 14 analysis angles

Blocking

Structural

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

diegolmello left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!