Repository navigation
fix(mobile): resync worktree list + idempotent notification replay on reconnect (#8498 #8129) - #8690
Merged
nwparker merged 2 commits intoJul 14, 2026
Conversation
The mobile host screen seeded its worktree list from the home-written cache and only refreshed via a focus-gated 3s poll — so after a background/sleep reconnect (or a partial home fetch) it showed a stale/incomplete Projects list with no way to force a refresh. - Refetch worktrees + repo metadata on the transition INTO 'connected' (not every poll tick), via a small extracted useWorktreeResync hook. - Add pull-to-refresh (RefreshControl) to the worktree SectionList. - Write the confirmed snapshot back through setCachedWorktrees from the host screen so a reconnect refetch or remount can't serve a poisoned cache. Verified: mobile tsc clean; oxlint/oxfmt clean; worktree-cache write-through test + full mobile suite green. Co-authored-by: Brandon Bennett <branben@users.noreply.github.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📥 CommitsReviewing files that changed from the base of the PR and between 38c74ac866a11014c661410660821e100cc7afdf and 73cce8d. 📒 Files selected for processing (8)
🚧 Files skipped from review as they are similar to previous changes (7)
📝 WalkthroughWalkthroughThe mobile worktree screen now centralizes reconnect and pull-to-refresh resynchronization, writes successful worktree responses through to the host cache, and adds cache tests. Desktop mobile notifications now receive monotonic replay sequences through a bounded buffer and a new missed-notifications RPC. Mobile subscriptions persist per-host watermarks, fetch missed events after reconnects, deduplicate replayed events, and add coverage for these behaviors. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
mobile/src/notifications/mobile-notifications.ts (1)
282-356: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winReconnect catch-up can be permanently skipped if the watermark hasn't loaded yet.
fetchMissed()is only invoked whenreconnectReadyCount > 1 && watermarkLoaded(Lines 354-356).watermarkLoadedflips totrueonly inside theloadLastSeenSeq(hostId).then(...)callback (Lines 324-327), which resolves after an AsyncStorage round-trip. If the first reconnect (reconnectReadyCount === 2) fires before that resolves, catch-up for that reconnect is dropped — not deferred, not retried.This isn't just a delayed catch-up: once any subsequent live event (which can carry a higher, globally-monotonic
notificationSequnrelated to the skipped gap) advanceslastDeliveredSeqpast the missed range, a laterfetchMissed()call will requestlastSeenSeqat that newer, higher value. Per the desktop's own "cuts by seq > lastSeenSeq" contract (as stated in the comment at Line 284), the skipped gap's notifications will never be returned again — this is permanent loss of exactly the missed-notification scenario this PR exists to fix.Queue the catch-up instead of dropping it when the watermark isn't loaded yet:
🐛 Proposed fix: queue catch-up until the watermark loads
let watermarkLoaded = false + let catchupPending = false void loadLastSeenSeq(hostId).then((seq) => { lastDeliveredSeq = Math.max(lastDeliveredSeq, seq) watermarkLoaded = true + if (catchupPending) { + catchupPending = false + void fetchMissed() + } })- if (reconnectReadyCount > 1 && watermarkLoaded) { - void fetchMissed() - } + if (reconnectReadyCount > 1) { + if (watermarkLoaded) { + void fetchMissed() + } else { + catchupPending = true + } + }No test currently exercises a reconnect racing the initial
loadLastSeenSeqresolution — worth adding once fixed.
🧹 Nitpick comments (1)
mobile/src/transport/use-worktree-resync.ts (1)
36-39: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winFetch worktrees and repository metadata concurrently.
Since
fetchWorktreesandfetchRepoMetadataare independent network requests, awaiting them sequentially increases the total time the user sees the refresh spinner. You can run them concurrently usingPromise.allto reduce the total refresh latency.⚡ Proposed performance improvement
try { - await fetchWorktrees({ allowDuringModal: true }) - await fetchRepoMetadata() + await Promise.all([ + fetchWorktrees({ allowDuringModal: true }), + fetchRepoMetadata() + ]) } finally {
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 626bca44-7036-4aaf-a60c-99583603c1d9
📥 Commits
Reviewing files that changed from the base of the PR and between f01bfd9 and 38c74ac866a11014c661410660821e100cc7afdf.
📒 Files selected for processing (10)
mobile/app/h/[hostId]/index.tsxmobile/src/cache/worktree-cache.test.tsmobile/src/notifications/mobile-notifications.test.tsmobile/src/notifications/mobile-notifications.tsmobile/src/notifications/notification-reconnect-catchup.tsmobile/src/transport/use-worktree-resync.tssrc/main/runtime/mobile-notification-replay.test.tssrc/main/runtime/mobile-notification-replay.tssrc/main/runtime/orca-runtime.tssrc/main/runtime/rpc/methods/notifications.ts
Comment on lines
+288
to
+317
| async function fetchMissed(): Promise<void> { | ||
| if (disposed) { | ||
| return | ||
| } | ||
| const missed = await client | ||
| .sendRequest('notifications.getMissedSince', { lastSeenSeq: lastDeliveredSeq }) | ||
| .then((response) => { | ||
| if (!response.ok) { | ||
| return [] | ||
| } | ||
| const result = response.result as { notifications?: unknown[] } | undefined | ||
| return Array.isArray(result?.notifications) ? result.notifications : [] | ||
| }) | ||
| .catch(() => []) | ||
| for (const raw of missed) { | ||
| const event = raw as NotificationEvent | DismissNotificationEvent | ||
| const key = seenKeyForEvent(event) | ||
| if (key && seenReplay.has(key)) { | ||
| continue | ||
| } | ||
| if (key) { | ||
| seenReplay.add(key) | ||
| } | ||
| if (event.type === 'notification') { | ||
| await deliverLive('notification', event) | ||
| } else if (event.type === 'dismiss') { | ||
| await deliverLive('dismiss', event) | ||
| } | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
fetchMissed() doesn't recheck disposed after the await — can deliver notifications post-teardown.
The only disposed guard in fetchMissed() is at entry (Line 289), before await client.sendRequest(...). If the subscription is disposed while that RPC is in flight, the function still iterates and calls deliverLive (Lines 311-315) — scheduling/dismissing local notifications for a subscription whose cleanup already ran. The live-stream path (Lines 365-372) correctly rechecks disposed right before dispatching; fetchMissed should do the same.
🐛 Proposed fix: recheck `disposed` inside the replay loop
for (const raw of missed) {
+ if (disposed) {
+ return
+ }
const event = raw as NotificationEvent | DismissNotificationEvent📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| async function fetchMissed(): Promise<void> { | |
| if (disposed) { | |
| return | |
| } | |
| const missed = await client | |
| .sendRequest('notifications.getMissedSince', { lastSeenSeq: lastDeliveredSeq }) | |
| .then((response) => { | |
| if (!response.ok) { | |
| return [] | |
| } | |
| const result = response.result as { notifications?: unknown[] } | undefined | |
| return Array.isArray(result?.notifications) ? result.notifications : [] | |
| }) | |
| .catch(() => []) | |
| for (const raw of missed) { | |
| const event = raw as NotificationEvent | DismissNotificationEvent | |
| const key = seenKeyForEvent(event) | |
| if (key && seenReplay.has(key)) { | |
| continue | |
| } | |
| if (key) { | |
| seenReplay.add(key) | |
| } | |
| if (event.type === 'notification') { | |
| await deliverLive('notification', event) | |
| } else if (event.type === 'dismiss') { | |
| await deliverLive('dismiss', event) | |
| } | |
| } | |
| } | |
| async function fetchMissed(): Promise<void> { | |
| if (disposed) { | |
| return | |
| } | |
| const missed = await client | |
| .sendRequest('notifications.getMissedSince', { lastSeenSeq: lastDeliveredSeq }) | |
| .then((response) => { | |
| if (!response.ok) { | |
| return [] | |
| } | |
| const result = response.result as { notifications?: unknown[] } | undefined | |
| return Array.isArray(result?.notifications) ? result.notifications : [] | |
| }) | |
| .catch(() => []) | |
| for (const raw of missed) { | |
| if (disposed) { | |
| return | |
| } | |
| const event = raw as NotificationEvent | DismissNotificationEvent | |
| const key = seenKeyForEvent(event) | |
| if (key && seenReplay.has(key)) { | |
| continue | |
| } | |
| if (key) { | |
| seenReplay.add(key) | |
| } | |
| if (event.type === 'notification') { | |
| await deliverLive('notification', event) | |
| } else if (event.type === 'dismiss') { | |
| await deliverLive('dismiss', event) | |
| } | |
| } | |
| } |
…8129) Mobile "push" notifications are local notifications fired over the live WebSocket; dispatchMobileNotification was a synchronous fan-out with no queue, so any completion dispatched while the phone socket was reaped (background/sleep) was lost with no replay on reconnect. Desktop: - MobileNotificationReplayBuffer records every dispatched notification with a monotonic seq (cap 256, oldest-first evict). dispatchMobileNotification tags live listeners with notificationSeq; notifications.getMissedSince(seq) returns exactly seq > watermark — idempotent by construction. Mobile: - Persist lastSeenSeq per host; on a reconnect `ready` fetch missed events from that watermark and dedup by id#seq (in-memory guard) so an event that arrives on both the live stream and a replay is pushed exactly once. Field-mismatch fix (bug in the original PR, missed by its CodeRabbit rebuttal): the replay path returned `seq` while the live fan-out and the client both use `notificationSeq`, so a replay-only delivery never advanced or persisted the client watermark (latent duplicate-push across restarts, the #5070 class). The replay buffer now exposes `notificationSeq` end-to-end. Proven by a failing repro first; covered by regression tests on both sides. Reconnect catch-up helpers extracted to notification-reconnect-catchup.ts to keep mobile-notifications.ts under its max-lines budget. Verified: desktop tsc + mobile tsc clean; oxlint/oxfmt clean; max-lines ratchet OK; replay buffer 8/8, mobile notifications 12/12. Co-authored-by: Brandon Bennett <branben@users.noreply.github.com>
nwparker
force-pushed
the
nwparker/mobile-reconnect-is-socket-only-missed-notificat
branch
from
July 14, 2026 07:06
38c74ac to
73cce8d
Compare
This was referenced Jul 14, 2026
6 tasks
nwparker
added a commit
that referenced
this pull request
Jul 28, 2026
…persist a watermark past them (#10816) * fix(mobile): keep the reconnect watermark alive across the app's own teardown The catch-up added in #8690 could never run. app/index.tsx unsubscribes the notification stream on every non-'connected' state and builds a fresh subscription on reconnect, so the closure holding the ready-counter, the delivered watermark and the seen-set is destroyed exactly when a reconnect needs them. Every reconnect looked like a cold open, `reconnectReadyCount` was always 1, and notifications dispatched while the socket was down were never fetched. Move that state to a per-host module-scope session so it survives the teardown. Refs #8591 Co-authored-by: Orca <help@stably.ai> * fix(mobile): tag the notification watermark with a counter epoch so a desktop restart can't kill catch-up The desktop's notification `seq` is a per-process in-memory counter that starts at 0 on every launch. The mobile client's watermark is persisted in AsyncStorage and monotonic. After a desktop restart the two index different counters, so a client holding seq 57 meets a fresh counter at 2, `57 >= 2` cuts everything, and reconnect catch-up dies silently until the new process out-dispatches the old watermark — 57 notifications later. Users see nothing and get no error (#8591). Stamp every dispatched notification with an epoch identifying the counter lifetime, ride it on the `ready` frame and the getMissedSince response, and persist it beside the watermark. A watermark whose epoch doesn't match the live counter is void: the client resets to 0 and the desktop returns its retained buffer instead of nothing. The epoch param is optional on the wire in both directions, so a client or daemon that predates it degrades to today's seq-only cut rather than erroring. Also extracts the OS-permission helpers to notification-permissions.ts (re- exported, so no importer changes) to keep mobile-notifications.ts under its max-lines budget. Mutation-tested: 3 mutations applied to the epoch logic, 3 killed — including the storage-seed race guard, whose first mutant survived until the deferred-read test was added. * fix(mobile): make the notification watermark atomic and counter-scoped Round-1 review found four ways the epoch fix could still lose notifications. All four are addressed here. 1. Seen-set survived an epoch change. Seen-keys are seq-derived, and terminal bells carry no notificationId (they key on `seq:N` alone). After a restart the fresh counter re-issues low seqs, so a replayed post-restart bell was dropped as a duplicate of a bell from the previous counter. The dedup window belongs to one counter lifetime, so it is cleared on epoch change. 2. Legacy watermarks were trusted. Pre-upgrade installs stored a bare seq with no epoch. Adopting the first observed epoch as "nothing changed" left that unprovenanced seq cutting a counter it was never measured against — #8591 through the upgrade path. An epoch-less seq no longer survives adoption. 3. seq and epoch were separate storage keys. A process death between the two writes left epoch-B beside seq-57-from-A: a pair that looks internally valid on the next launch and is therefore trusted. They are now one JSON value, which cannot tear, with a read-only migration from the legacy key. 4. Sessions were never retired. They live at module scope so they survive the subscription teardown a reconnect performs, so host removal is the only thing that can drop them. Removal now retires the session and its watermark. Mutation-tested: 3 mutations, 3 killed. The first version of the bell test passed with the fix removed — it exercised the live path, which only adds to the seen-set; only the replay path consults it. Rewritten against the replay path, it fails with `expected 1 to be 2`: the literal lost notification. Mobile notifications + transport: 355 passed. Desktop replay: 11/11. * fix(mobile): catch up on the first connection after a cold open Catch-up hung off 'has this process connected before', which is false on the first ready of a fresh launch — exactly the post-upgrade / post-eviction case that loses everything between the stored watermark and the next live seq. Wait for the persisted read, then catch up whenever this device has delivered for the host before; a first-ever pairing still gets no replay. Co-authored-by: Orca <help@stably.ai> * fix(mobile): serialize live delivery behind the watermark seed, and key catch-up on the record Co-authored-by: Orca <help@stably.ai> * test(mobile): pin the two catch-up mechanisms mutation testing found unguarded Mutating each mechanism of the #8591 fix in turn showed two survived with the suite still green: the seed's epoch-provenance check, and the host session outliving the subscription teardown. Both are load-bearing, so pin them. - seen-set survives teardown: the desktop's retained buffer replays a notification already delivered live, and only the session-scoped seen-set stops a duplicate banner. - a seed resolving after a live epoch was adopted must not reinstate the dead watermark. Not reachable through subscribeToDesktopNotifications today ('ready' awaits the seed first), so it asserts on the exported pair and says so. Co-authored-by: Orca <help@stably.ai> * fix(mobile): serialize notification delivery per host so the watermark can't outrun what was shown Addresses two MAJOR findings from review of this branch. MAJOR #1 — the watermark could be persisted past a notification the user never saw. `deliverLive` advanced `lastDeliveredSeq` before awaiting the local show, and replay + live delivery ran concurrently, so a live seq 11 handled while catch-up was still showing seq 6 persisted 11. A process death before 7..10 were shown lost them permanently: the next launch asks the desktop for seq > 11. This predates the branch — `origin/main` advances the watermark at the same point — so it is a residual this fix closes, not a regression the branch introduced. It is fixed here because the branch is what makes the watermark load-bearing. Three changes: - the advance moves AFTER the show/dismiss await, so the watermark means "everything up to here reached the user" rather than "was dispatched" - a per-host `deliveryTail` promise chain (`enqueueHostDelivery`) serializes deliveries, so a monotonic advance is also an in-order one - the catch-up batch is ONE queue entry, not one per event. Awaiting per event returns to the event loop between replays and let a live event slot in between seq 6 and 7 — which is exactly the interleave being fixed. The RPC stays outside the queue: `sendRequest` waits up to 30s and holding the chain for that would stall live delivery on a slow link. MAJOR #2 — every delivery awaits the persisted read, so an AsyncStorage read that never settled disabled the host's notifications for the whole app lifetime, with no error and nothing to see. The seed is now bounded at 3s; a late seed still applies when it lands. Proceeding unseeded is strictly better: the watermark stays 0, so catch-up over-fetches and the seen-set de-duplicates. Serializing removed an overlap the duplicate-suppression relied on: `showLocalNotification` deduped two same-id events by observing the first still pending when the second arrived. With deliveries serialized the first completes first, so the second saw no pending state and scheduled a second banner for the same notification. The claim moves to enqueue time, where the overlap is still observable. Dismisses are deliberately not claimed — a dismiss for a shown id is what retires it. Evidence — each mechanism disabled individually against the unchanged suite: - batch-as-one-entry -> reverted to per-item enqueue: ordering test fails - watermark advance -> moved back before the await: ordering test fails - seed timeout -> removed: wedged-read test fails - live-path claim -> removed: concurrent-dedup test fails - replay-path claim -> removed: cross-path dedup test fails Each kills exactly one test, so no mechanism is unguarded and none is redundant. `mobile-notifications.test.ts`'s local `flushAsync` drained 10 microtask ticks. Deliveries are now several awaits deeper, so a fixed tick count under-drains; it yields to the macrotask queue instead. Verified with real timers that the behavior it asserts is unchanged — only the drain depth was wrong. Full mobile suite: 344 files, 2499 passed, 2 skipped. tsc clean, oxlint clean. --------- Co-authored-by: Orca <help@stably.ai>
enrick-dev
pushed a commit
to enrick-dev/orca
that referenced
this pull request
Oct 15, 2026
…persist a watermark past them (stablyai#10816) * fix(mobile): keep the reconnect watermark alive across the app's own teardown The catch-up added in stablyai#8690 could never run. app/index.tsx unsubscribes the notification stream on every non-'connected' state and builds a fresh subscription on reconnect, so the closure holding the ready-counter, the delivered watermark and the seen-set is destroyed exactly when a reconnect needs them. Every reconnect looked like a cold open, `reconnectReadyCount` was always 1, and notifications dispatched while the socket was down were never fetched. Move that state to a per-host module-scope session so it survives the teardown. Refs stablyai#8591 Co-authored-by: Orca <help@stably.ai> * fix(mobile): tag the notification watermark with a counter epoch so a desktop restart can't kill catch-up The desktop's notification `seq` is a per-process in-memory counter that starts at 0 on every launch. The mobile client's watermark is persisted in AsyncStorage and monotonic. After a desktop restart the two index different counters, so a client holding seq 57 meets a fresh counter at 2, `57 >= 2` cuts everything, and reconnect catch-up dies silently until the new process out-dispatches the old watermark — 57 notifications later. Users see nothing and get no error (stablyai#8591). Stamp every dispatched notification with an epoch identifying the counter lifetime, ride it on the `ready` frame and the getMissedSince response, and persist it beside the watermark. A watermark whose epoch doesn't match the live counter is void: the client resets to 0 and the desktop returns its retained buffer instead of nothing. The epoch param is optional on the wire in both directions, so a client or daemon that predates it degrades to today's seq-only cut rather than erroring. Also extracts the OS-permission helpers to notification-permissions.ts (re- exported, so no importer changes) to keep mobile-notifications.ts under its max-lines budget. Mutation-tested: 3 mutations applied to the epoch logic, 3 killed — including the storage-seed race guard, whose first mutant survived until the deferred-read test was added. * fix(mobile): make the notification watermark atomic and counter-scoped Round-1 review found four ways the epoch fix could still lose notifications. All four are addressed here. 1. Seen-set survived an epoch change. Seen-keys are seq-derived, and terminal bells carry no notificationId (they key on `seq:N` alone). After a restart the fresh counter re-issues low seqs, so a replayed post-restart bell was dropped as a duplicate of a bell from the previous counter. The dedup window belongs to one counter lifetime, so it is cleared on epoch change. 2. Legacy watermarks were trusted. Pre-upgrade installs stored a bare seq with no epoch. Adopting the first observed epoch as "nothing changed" left that unprovenanced seq cutting a counter it was never measured against — stablyai#8591 through the upgrade path. An epoch-less seq no longer survives adoption. 3. seq and epoch were separate storage keys. A process death between the two writes left epoch-B beside seq-57-from-A: a pair that looks internally valid on the next launch and is therefore trusted. They are now one JSON value, which cannot tear, with a read-only migration from the legacy key. 4. Sessions were never retired. They live at module scope so they survive the subscription teardown a reconnect performs, so host removal is the only thing that can drop them. Removal now retires the session and its watermark. Mutation-tested: 3 mutations, 3 killed. The first version of the bell test passed with the fix removed — it exercised the live path, which only adds to the seen-set; only the replay path consults it. Rewritten against the replay path, it fails with `expected 1 to be 2`: the literal lost notification. Mobile notifications + transport: 355 passed. Desktop replay: 11/11. * fix(mobile): catch up on the first connection after a cold open Catch-up hung off 'has this process connected before', which is false on the first ready of a fresh launch — exactly the post-upgrade / post-eviction case that loses everything between the stored watermark and the next live seq. Wait for the persisted read, then catch up whenever this device has delivered for the host before; a first-ever pairing still gets no replay. Co-authored-by: Orca <help@stably.ai> * fix(mobile): serialize live delivery behind the watermark seed, and key catch-up on the record Co-authored-by: Orca <help@stably.ai> * test(mobile): pin the two catch-up mechanisms mutation testing found unguarded Mutating each mechanism of the stablyai#8591 fix in turn showed two survived with the suite still green: the seed's epoch-provenance check, and the host session outliving the subscription teardown. Both are load-bearing, so pin them. - seen-set survives teardown: the desktop's retained buffer replays a notification already delivered live, and only the session-scoped seen-set stops a duplicate banner. - a seed resolving after a live epoch was adopted must not reinstate the dead watermark. Not reachable through subscribeToDesktopNotifications today ('ready' awaits the seed first), so it asserts on the exported pair and says so. Co-authored-by: Orca <help@stably.ai> * fix(mobile): serialize notification delivery per host so the watermark can't outrun what was shown Addresses two MAJOR findings from review of this branch. MAJOR stablyai#1 — the watermark could be persisted past a notification the user never saw. `deliverLive` advanced `lastDeliveredSeq` before awaiting the local show, and replay + live delivery ran concurrently, so a live seq 11 handled while catch-up was still showing seq 6 persisted 11. A process death before 7..10 were shown lost them permanently: the next launch asks the desktop for seq > 11. This predates the branch — `origin/main` advances the watermark at the same point — so it is a residual this fix closes, not a regression the branch introduced. It is fixed here because the branch is what makes the watermark load-bearing. Three changes: - the advance moves AFTER the show/dismiss await, so the watermark means "everything up to here reached the user" rather than "was dispatched" - a per-host `deliveryTail` promise chain (`enqueueHostDelivery`) serializes deliveries, so a monotonic advance is also an in-order one - the catch-up batch is ONE queue entry, not one per event. Awaiting per event returns to the event loop between replays and let a live event slot in between seq 6 and 7 — which is exactly the interleave being fixed. The RPC stays outside the queue: `sendRequest` waits up to 30s and holding the chain for that would stall live delivery on a slow link. MAJOR stablyai#2 — every delivery awaits the persisted read, so an AsyncStorage read that never settled disabled the host's notifications for the whole app lifetime, with no error and nothing to see. The seed is now bounded at 3s; a late seed still applies when it lands. Proceeding unseeded is strictly better: the watermark stays 0, so catch-up over-fetches and the seen-set de-duplicates. Serializing removed an overlap the duplicate-suppression relied on: `showLocalNotification` deduped two same-id events by observing the first still pending when the second arrived. With deliveries serialized the first completes first, so the second saw no pending state and scheduled a second banner for the same notification. The claim moves to enqueue time, where the overlap is still observable. Dismisses are deliberately not claimed — a dismiss for a shown id is what retires it. Evidence — each mechanism disabled individually against the unchanged suite: - batch-as-one-entry -> reverted to per-item enqueue: ordering test fails - watermark advance -> moved back before the await: ordering test fails - seed timeout -> removed: wedged-read test fails - live-path claim -> removed: concurrent-dedup test fails - replay-path claim -> removed: cross-path dedup test fails Each kills exactly one test, so no mechanism is unguarded and none is redundant. `mobile-notifications.test.ts`'s local `flushAsync` drained 10 microtask ticks. Deliveries are now several awaits deeper, so a fixed tick count under-drains; it yields to the macrotask queue instead. Verified with real timers that the behavior it asserts is unchanged — only the drain depth was wrong. Full mobile suite: 344 files, 2499 passed, 2 skipped. tsc clean, oxlint clean. --------- Co-authored-by: Orca <help@stably.ai>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Take-over of @branben's mobile-reconnect work from #8605 / #8591. Ships the two fixes that are correct, on-target, and clear of the in-flight relay PR (#8536), corrected so mobile CI actually passes:
fix(mobile): resync worktree list on reconnect + pull-to-refresh (#8498)fix(mobile): replay missed notifications idempotently on reconnect (#8129, partial)Deliberately NOT included (see Scope below): #6784 (raw connection-error surfacing) and #4500 (ws-transport pong-liveness) — both touch connection-health / connection-lifecycle code owned by the relay PR #8536. Kept out to avoid stepping on that work.
Two isolated commits for clean review/rollback.
Closes #8498.What was wrong with the original PR (#8605)
The original bundle was verified against source and found to have defects the author's own checks missed (they ran the root typecheck, which does not cover
mobile/— mobile is typechecked separately bymobile.yml):use-host-connection-state.tsimported./transport/types/./transport/rpc-client(→ nonexistenttransport/transport/…), anduse-last-connection-error.tsimporteduseCtxfromclient-contextwhere it is not exported (runtime crash on every host/session/tasks screen). Confirmed by runningcd mobile && npx tsc --noEmit.notificationSeq, but the desktop replay path returnedseq, and the mobile client only readsnotificationSeq. So a replay-only delivery never advanced or persisted the client watermark → latent duplicate-push across restarts (the [Bug]: Mobile app shows duplicate notifications for every agent completion #5070 class). The original test hid it by mocking the wrong field shape; the CodeRabbit comment that flagged this was wrongly dismissed. Proven with a failing repro before fixing.mobile-notifications.tsover the 300-line oxlintmax-lineslimit (348 counted) — another mobile-only gate the author's desktop ratchet didn't catch.Fixes in this PR
#8498 — worktree resync
connected(not every 3s poll tick), via a small extracteduseWorktreeResynchook.RefreshControl) on the worktreeSectionList.setCachedWorktreesfrom the host screen, so a reconnect refetch or remount can't serve a poisoned/partial cache.#8129 — notification replay (partial; catches up on reconnect)
MobileNotificationReplayBuffer(monotonic seq, cap 256, oldest-first evict) +notifications.getMissedSince(seq)— returns exactlyseq > watermark, idempotent by construction, auth-gated identically tonotifications.subscribe.lastSeenSeqper host; on a reconnectreadyit fetches missed events from that watermark and dedups byid#seq.notificationSeqend-to-end, so a replay-only delivery advances + persists the watermark. Regression-tested on both sides.notification-reconnect-catchup.tsto keepmobile-notifications.tsunder its max-lines budget (no suppression added).Evidence
mobile-notification-replay.test.ts8/8;tsc -p config/tsconfig.node.json0 errors.mobile-notifications.test.ts12/12 +worktree-cache.test.ts2/2;cd mobile && npx tsc --noEmitclean;oxlint+oxfmt --check .clean; full mobile suite 1471 pass.check-max-lines-ratchetOK (no new bypasses/suppressions).ELI5
Your phone shows a stale project list and drops "agent done" pings when it wakes from sleep, because on reconnect it just re-opens the socket and asks for nothing it missed. This PR makes it catch up: on reconnect it re-fetches the worktree list (and you can pull-to-refresh), and it asks the desktop "what notifications did I miss since #N?" and shows exactly those — never the same one twice.
Scope — why #6784 and #4500 are held back
The relay PR #8536 rewrites
mobile/app/index.tsx,client-context.tsx,types.ts, all four screens, and adds a whole connection-supervisor layer. #6784 (connection-health error UX) and #4500 (ws heartbeat / connection lifecycle) live squarely in that surface, so changing them now would conflict with and complicate the relay landing. They're left for relay-aware follow-ups. (#6784's original raw-error approach was also misaligned with the issue, which asks for a Tailscale recommendation; #4500's confirmed repro is renderer-side and already addressed by #8630/#8673.)Trade-offs / user-regressions
readyafter a full app relaunch skips the fetch to avoid a burst of stale notifications. Missed-while-killed events are not replayed on a cold start.Co-authored-by: Brandon Bennett (@branben) — original architecture from #8605.
Made with Orca 🐋