Repository navigation
ios: back the device list with the cross-device sync DO (durable across updates) - #6496
Closed
lawrencecchen wants to merge 13 commits into
Closed
lawrencecchen wants to merge 13 commits into
lawrencecchen wants to merge 13 commits into
Conversation
…vives updates) Wires the already-built-but-dormant CmuxSyncStore into the iOS app so the device list is a durable, locally-cached, DO-synced collection instead of a volatile per-launch GET /api/devices read masked by an all-or-nothing fallback. Root cause of the lost-devices-on-update bug: deviceTreeDevices was registry XOR local (returns the registry list entirely if non-empty, else local paired Macs), and the registry result is in-memory and refetched each launch. Any saved Mac the registry did not currently list was masked out, and nothing durable backed the list across an update. This is the deferred Phase-2 wiring from plans/feat-do-device-list/DESIGN.md (server + package shipped; app integration was the gap): - CmuxMobileShell + cmuxFeature now depend on CmuxSyncStore; the root scene opens cmux-sync.sqlite3 next to paired-macs.sqlite3 (Application Support, survives updates) and injects it + the resolved mobileDeviceListLocalFirst flag. - New PresenceSyncTransport: a SyncTransport over its own authenticated /v1/presence/subscribe WebSocket (isolated from the live presence stream), so sync/v1 snapshot/delta frames ride alongside presence noise the client ignores. - MobileShellComposite runs a sync subscription mirroring the presence backoff loop, seeds provisional rows from the existing paired-Mac store via PairedMacMigration on first launch (so the list renders instantly pre-frame and an offline Mac is never lost), and reads the device list from DeviceSyncFacade when the flag is on. Flag OFF keeps today's /api/devices path verbatim. Cross-device: the Mac already publishes its record to the team's DO via presence heartbeat, so a fresh sign-in on another device subscribes and renders it with no setup. Gated DEBUG-on/Release-off; production list behavior is unchanged until the flag is flipped after dogfood. Tests (swift test, all green): device records survive a fresh store on the same file (the update simulation); a provisional seed renders and survives a snapshot that omits it; a snapshot from another device streams in and renders; the composite reads the sync store when the flag is on and ignores it when off. (Pre-existing MobileShellRenderGridLivenessTests fail identically on clean main.) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
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:
📝 WalkthroughWalkthroughThis PR introduces a local-first device-list sync path on iOS. It adds a new ChangesLocal-first device-list sync
Sequence Diagram(s)sequenceDiagram
participant CMUXMobileRootScene
participant MobileShellComposite
participant PresenceSyncTransport
participant SyncClient
participant CmuxSyncStore
participant DeviceTree
CMUXMobileRootScene->>CMUXMobileRootScene: openSyncStore() → SQLite db
CMUXMobileRootScene->>MobileShellComposite: init(syncStore, deviceListLocalFirst, syncTeamIDProvider, makeSyncTransport)
rect rgba(70, 130, 180, 0.5)
Note over MobileShellComposite: isSignedIn = true / resumeForegroundRefresh
MobileShellComposite->>MobileShellComposite: evaluateSyncSubscription()
MobileShellComposite->>PresenceSyncTransport: makeSyncTransport(teamID)
MobileShellComposite->>CmuxSyncStore: seedProvisional(paired-Mac data) [once]
MobileShellComposite->>SyncClient: run(transport: PresenceSyncTransport)
end
PresenceSyncTransport-->>SyncClient: inbound frame (AsyncThrowingStream)
SyncClient->>CmuxSyncStore: apply frame
SyncClient-->>MobileShellComposite: frame applied callback
MobileShellComposite->>CmuxSyncStore: reloadDeviceListFromSyncStore(teamID)
CmuxSyncStore-->>MobileShellComposite: [SyncedDeviceRecord]
MobileShellComposite->>DeviceTree: update sorted device list
Note over MobileShellComposite: loadRegistryDevices() early-returns from CmuxSyncStore when flag on
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (6 errors, 1 warning)
✅ Passed checks (16 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 05bddfeeef
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+1508
to
+1512
| let teamID = (await syncTeamIDProvider?()) ?? "" | ||
| if !teamID.isEmpty { | ||
| await reloadDeviceListFromSyncStore(teamID: teamID) | ||
| } | ||
| return |
There was a problem hiding this comment.
When mobileDeviceListLocalFirst is enabled but the DO is unavailable or the cache has not been populated yet (for example, the root scene leaves makeSyncTransport nil because no presence URL resolves), this branch still returns after the local read and never executes the existing /api/devices fallback below. That hides registry-only/cross-device Macs and leaves users with only the local paired-Mac fallback instead of today's durable registry list; fall through to deviceRegistry.listDevices() when the sync cache is empty or no sync transport is available.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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
`@Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift`:
- Around line 1687-1689: The onApplied closure triggers a full sync-store read
and sort via reloadDeviceListFromSyncStore for every applied frame, causing
inefficient repeated device collection rebuilds on the main actor during
snapshot or delta bursts. Replace the immediate reloadDeviceListFromSyncStore
call with a coalesced approach that batches reloads per run-loop tick or
debounces multiple frame applications, or alternatively modify the sync store
facade to return pre-sorted device lists once per committed batch rather than on
every individual frame application event, in accordance with the guideline that
socket-stream event paths should not rebuild large user-owned collections on
every event.
- Around line 1508-1512: The device list reloading logic has a race condition
where stale data from old async operations could be assigned after an
account/team switch. Capture both the expected user and teamID before awaiting
reloadDeviceListFromSyncStore, then verify both match the current state before
assigning any results to registryDevices. Additionally, when teamID resolution
returns empty, explicitly clear or reset registryDevices rather than leaving the
previous state visible. Apply the same guarding pattern to the related onApplied
callbacks and loadRegistryDevices continuation handlers to ensure all
local-first device reads properly validate the captured account and team
context.
In
`@Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/PresenceSyncTransport.swift`:
- Around line 88-105: The connectedTask() function can create and cache a stale
websocket if cancellation occurs during the await of tokenSource.accessToken().
To fix this, after creating the URLSessionWebSocketTask with
session.webSocketTask(with: request) and before calling task.resume() and
caching it with self.task = task, add a call to Task.checkCancellation() to
ensure the current task has not been cancelled. This will prevent a stale socket
from being stored when the stream is torn down during token retrieval.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6b2b9d72-540b-41bc-ad10-a22368ffcf97
📒 Files selected for processing (7)
Packages/Shared/CmuxSyncStore/Tests/CmuxSyncStoreTests/DeviceListSyncWiringTests.swiftPackages/iOS/CmuxMobileShell/Package.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/PresenceSyncTransport.swiftPackages/iOS/CmuxMobileShell/Tests/CmuxMobileShellTests/DeviceListLocalFirstWiringTests.swiftios/cmuxPackage/Package.swiftios/cmuxPackage/Sources/cmuxFeature/CMUXMobileRootScene.swift
Comment on lines
+88
to
+105
| private func connectedTask() async throws -> URLSessionWebSocketTask { | ||
| if let task { return task } | ||
| guard let url = PresenceClient.subscribeURL(serviceBaseURL: serviceBaseURL) else { | ||
| throw PresenceClientError.invalidServiceURL | ||
| } | ||
| guard let accessToken = await tokenSource.accessToken() else { | ||
| throw PresenceClientError.notAuthenticated | ||
| } | ||
| var request = URLRequest(url: url) | ||
| request.setValue("Bearer \(accessToken)", forHTTPHeaderField: "Authorization") | ||
| if !teamID.isEmpty { | ||
| request.setValue(teamID, forHTTPHeaderField: "X-Cmux-Team-Id") | ||
| } | ||
| let task = session.webSocketTask(with: request) | ||
| task.resume() | ||
| self.task = task | ||
| return task | ||
| } |
There was a problem hiding this comment.
Guard against cancellation before caching the websocket.
If the stream is torn down while connectedTask() is still awaiting the token, the websocket can still be created and stored after onTermination has already run. That leaves a stale socket behind and can poison the next reconnect attempt.
Proposed fix
let task = session.webSocketTask(with: request)
task.resume()
+ if Task.isCancelled {
+ task.cancel(with: .goingAway, reason: nil)
+ throw CancellationError()
+ }
self.task = task🤖 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
`@Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/PresenceSyncTransport.swift`
around lines 88 - 105, The connectedTask() function can create and cache a stale
websocket if cancellation occurs during the await of tokenSource.accessToken().
To fix this, after creating the URLSessionWebSocketTask with
session.webSocketTask(with: request) and before calling task.resume() and
caching it with self.task = task, add a call to Task.checkCancellation() to
ensure the current task has not been cancelled. This will prevent a stale socket
from being stored when the stream is torn down during token retrieval.
Contributor
Greptile SummaryWires the already-built
Confidence Score: 3/5Safe to merge with the flag off (Release default), but the sync code path has three known wiring issues that should be resolved before enabling in production. The MobileShellComposite class is @MainActor-isolated, so syncTask = Task { [weak self] in … try await client.run() } inherits that isolation — contrary to the NOT @mainactor comment. Every received frame drives SQLite writes (SyncFrameApplier.apply) and a read (reloadDeviceListFromSyncStore) on the main actor; the initial snapshot batch can contain many device records and will block the UI event loop for each transaction. Two additional issues — the fire-and-forget Task { await self.close() } in PresenceSyncTransport.onTermination and the untracked sign-out store-clearing task in MobileShellComposite — were flagged in prior review rounds. All three are gated behind mobileDeviceListLocalFirst (Release-off), so today's production behavior is unaffected, but they need to be addressed before the flag is flipped in dogfood. MobileShellComposite.swift — the sync task executor and sign-out clearing path; PresenceSyncTransport.swift — the onTermination cleanup task. Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant Scene as CMUXMobileRootScene
participant Composite as MobileShellComposite
participant Transport as PresenceSyncTransport
participant Store as CmuxSyncStore
participant Facade as DeviceSyncFacade
participant DO as cmux-presence DO
Scene->>Store: openSyncStore()
Scene->>Composite: inject(syncStore, makeSyncTransport, deviceListLocalFirst)
Note over Composite: isSignedIn -> evaluateSyncSubscription()
Composite->>Composite: startSyncSubscription()
Composite->>Store: "PairedMacMigration seed rev==0 rows"
Composite->>Transport: makeSyncTransport(teamID)
loop backoff reconnect 1s..60s
Transport->>DO: WSS /v1/presence/subscribe
DO-->>Transport: sync.snapshot / sync.delta
Transport-->>Composite: frames()
Composite->>Store: applySnapshot / applyDelta
Composite->>Facade: registryDevices(teamID, owner)
Facade-->>Composite: [RegistryDevice]
end
Note over Composite: loadRegistryDevices()
alt store non-empty
Composite->>Composite: return sync devices
else "cursor > 0 authoritative empty"
Composite->>Composite: "registryDevices = []"
else "cursor == 0"
Composite->>Composite: fall through to /api/devices
end
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant Scene as CMUXMobileRootScene
participant Composite as MobileShellComposite
participant Transport as PresenceSyncTransport
participant Store as CmuxSyncStore
participant Facade as DeviceSyncFacade
participant DO as cmux-presence DO
Scene->>Store: openSyncStore()
Scene->>Composite: inject(syncStore, makeSyncTransport, deviceListLocalFirst)
Note over Composite: isSignedIn -> evaluateSyncSubscription()
Composite->>Composite: startSyncSubscription()
Composite->>Store: "PairedMacMigration seed rev==0 rows"
Composite->>Transport: makeSyncTransport(teamID)
loop backoff reconnect 1s..60s
Transport->>DO: WSS /v1/presence/subscribe
DO-->>Transport: sync.snapshot / sync.delta
Transport-->>Composite: frames()
Composite->>Store: applySnapshot / applyDelta
Composite->>Facade: registryDevices(teamID, owner)
Facade-->>Composite: [RegistryDevice]
end
Note over Composite: loadRegistryDevices()
alt store non-empty
Composite->>Composite: return sync devices
else "cursor > 0 authoritative empty"
Composite->>Composite: "registryDevices = []"
else "cursor == 0"
Composite->>Composite: fall through to /api/devices
end
Reviews (7): Last reviewed commit: "ios: fix cmuxFeature compile (typed make..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+81
to
+84
| continuation.onTermination = { _ in | ||
| pump.cancel() | ||
| Task { await self.close() } | ||
| } |
Contributor
There was a problem hiding this comment.
Task for actor cleanup in onTermination
Task { await self.close() } is an unstructured, fire-and-forget task spawned from a @Sendable closure with no cancellation token and no structured lifetime. The cmux rule flags fire-and-forget Tasks with meaningful lifecycle, and close() is meaningful: it cancels the live URLSessionWebSocketTask and nils the stored reference on the actor. If the PresenceSyncTransport actor is deallocated before the pump task fires, self is still captured strongly by the closure, preventing deallocation until the task completes. Additionally, if multiple stream terminations overlap (e.g., rapid reconnects), each spawns an independent close task with no coordination. The canonical fix is to have the pump task itself call await self.close() after its loop exits (the pump is already Task-tracked via continuation.onTermination = { _ in pump.cancel() }) — just add await self.close() at the bottom of the pump's do block after the loop.
…h safety Autoreview found two real holes: 1. The local-first read swap gated on (flag && syncStore) but the sync subscription gates on makeSyncTransport != nil; enabling the flag without a presence URL bypassed /api/devices with nothing populating the store. Now the read swap requires makeSyncTransport too, and an empty store falls through to the registry instead of showing an empty tree. 2. The sync task pinned a team for a long-lived socket; a team switch could let an old-team frame overwrite the current list, and the seed never re-ran for the new team. reloadDeviceListFromSyncStore now drops a result whose team no longer matches the resolved team; the seed is tracked per-team; and loadRegistryDevices restarts the subscription when the resolved team changes. Tests: flag-on-without-transport falls back to registry; empty store falls back to registry; a stale team-A frame does not overwrite team-B's list. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ated tree The empty-store fallback called reloadDeviceListFromSyncStore (which assigns registryDevices = [] for an empty store) before falling through to the registry. A transient registry failure is a deliberate no-op meant to KEEP the current tree, so the pre-clear surfaced as a blank tree on a blip. Split the read into a non-mutating syncStoreDevices() the launch path uses (assign only when non-empty, else fall through without clearing); reloadDeviceListFromSyncStore still assigns unconditionally for the apply-callback path, where an empty result is authoritative (a delta tombstoned the last device). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift (1)
1630-1758: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy liftMove the device-list sync loop out of the composite.
This adds WebSocket lifecycle, migration seeding, sync applier setup, persistence reads, and list projection to a >800-line store that already owns connection, terminal, draft, presence, and pairing state. Extract this block into a small same-package coordinator that reports ordered device snapshots back to
MobileShellComposite.As per coding guidelines, production Swift files over 800 lines and files mixing state ownership, persistence, networking, and UI projection should be flagged.
🤖 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 `@Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift` around lines 1630 - 1758, Extract the device-list sync implementation out of MobileShellComposite into a separate coordinator class within the same package. Move the private properties syncTask, syncSubscriptionTeamID, and seededSyncTeams, along with the methods evaluateSyncSubscription(), startSyncSubscription(), and reloadDeviceListFromSyncStore() into a new coordinator that manages WebSocket lifecycle, migration seeding, sync applier setup, and persistence reads. Update MobileShellComposite to instantiate and delegate to this coordinator, with the coordinator reporting ordered device snapshots back to the composite via a callback or binding, allowing MobileShellComposite to remain focused on its core responsibilities without mixing state ownership, networking, and persistence concerns.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In
`@Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift`:
- Around line 1630-1758: Extract the device-list sync implementation out of
MobileShellComposite into a separate coordinator class within the same package.
Move the private properties syncTask, syncSubscriptionTeamID, and
seededSyncTeams, along with the methods evaluateSyncSubscription(),
startSyncSubscription(), and reloadDeviceListFromSyncStore() into a new
coordinator that manages WebSocket lifecycle, migration seeding, sync applier
setup, and persistence reads. Update MobileShellComposite to instantiate and
delegate to this coordinator, with the coordinator reporting ordered device
snapshots back to the composite via a callback or binding, allowing
MobileShellComposite to remain focused on its core responsibilities without
mixing state ownership, networking, and persistence concerns.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9ec691b6-5b57-4bec-b0aa-67daf90cd50a
📒 Files selected for processing (2)
Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swiftPackages/iOS/CmuxMobileShell/Tests/CmuxMobileShellTests/DeviceListLocalFirstWiringTests.swift
The subscription loop did guard let self at the top, holding a strong self across the long client.run() await, so an in-flight socket retained the whole composite past its owner's lifetime. Extract makeSyncSubscriptionClient() (resolve team + seed + build, with a weak-self apply callback) and call it via self?. so self is released before client.run(); the socket holds no strong self (transport/applier hold only the sync store, onApplied is weak). Mirrors the presence loop's per-frame weak-self pattern. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
applyDelta advances the cursor to a frame's rev with no contiguity guard, so a frame dropped from the MIDDLE of the stream silently loses a rev. The transport's bufferingNewest(256) drops the OLDEST on overflow and delivers a non-contiguous tail, which the consumer would commit past the gap. Switch to bufferingOldest: overflow drops the NEWEST frames, so the delivered prefix stays contiguous and the cursor never steps over a gap; the dropped recent frames are re-sent from the persisted cursor on reconnect/re-hello. Still end the stream on any drop so that reconnect is prompt. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
| syncTask = nil | ||
| syncSubscriptionTeamID = nil | ||
| evaluateSyncSubscription() | ||
| } |
There was a problem hiding this comment.
Team switch skips sync restart
Medium Severity
Changing the selected team only restarts the local-first device sync when loadRegistryDevices runs (for example opening or refreshing the device tree). Until then the sync WebSocket stays pinned to the old team via PresenceSyncTransport, and registryDevices can keep showing that team’s devices because onApplied reloads are rejected when syncTeamIDProvider no longer matches the captured team id.
Reviewed by Cursor Bugbot for commit 9b295aa. Configure here.
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)
Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift (1)
1636-1772: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy liftExtract the new device-sync lifecycle out of this composite.
This adds durable-store reads, paired-Mac migration seeding, WebSocket sync/backoff, and device-list mutation to an already 5,763-line production store that also owns pairing, terminal I/O, drafts, feedback, network recovery, and rendering sync. Please move this sync loop/read layer into a small same-package coordinator and leave
MobileShellCompositeresponsible for start/stop and applyingregistryDevices.As per coding guidelines, “Flag Swift production files that exceed 400 lines without a clear single responsibility, or exceed 800 lines even with mostly coherent responsibility” and “Flag files that mix UI rendering, state ownership, persistence, networking, parsing, subprocess/socket protocol, and platform bridge code in one place.”
🤖 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 `@Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift` around lines 1636 - 1772, MobileShellComposite is already 5,763 lines and the new sync lifecycle code violates single-responsibility principles by mixing persistence, networking, and state management with rendering and platform bridge code. Extract the sync loop and read layer into a new same-package coordinator class by moving the properties syncTask, syncSubscriptionTeamID, and seededSyncTeams, along with the methods evaluateSyncSubscription, startSyncSubscription, and syncStoreDevices into the new coordinator. Leave only the start/stop control and the reloadDeviceListFromSyncStore apply callback in MobileShellComposite to integrate the coordinator's results into registryDevices.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift`:
- Around line 1522-1524: Replace the optional array return type
`[RegistryDevice]?` from the `syncStoreDevices` method with a result enum that
explicitly represents the two cases: either a successful read (with a possibly
empty array) or a failed/discarded read. Create a new enum type (or use Swift's
Result type) and update the `syncStoreDevices` method to return this enum
instead of an optional collection. Then update all call sites of
`syncStoreDevices`, including the conditional check at lines 1522-1524, to
handle the new enum cases using appropriate pattern matching instead of the
current optional binding check.
---
Outside diff comments:
In
`@Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift`:
- Around line 1636-1772: MobileShellComposite is already 5,763 lines and the new
sync lifecycle code violates single-responsibility principles by mixing
persistence, networking, and state management with rendering and platform bridge
code. Extract the sync loop and read layer into a new same-package coordinator
class by moving the properties syncTask, syncSubscriptionTeamID, and
seededSyncTeams, along with the methods evaluateSyncSubscription,
startSyncSubscription, and syncStoreDevices into the new coordinator. Leave only
the start/stop control and the reloadDeviceListFromSyncStore apply callback in
MobileShellComposite to integrate the coordinator's results into
registryDevices.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: df3efb85-7ec2-48bb-9937-7aeeee80bfb4
📒 Files selected for processing (1)
Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift
Comment on lines
+1522
to
+1524
| if let devices = await syncStoreDevices(teamID: teamID), !devices.isEmpty { | ||
| registryDevices = devices | ||
| return |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file='Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift'
if command -v swiftlint >/dev/null 2>&1; then
output="$(swiftlint lint --path "$file" || true)"
if printf '%s\n' "$output" | rg -n 'discouraged_optional_collection|syncStoreDevices'; then
exit 1
fi
else
if rg -n 'syncStoreDevices\(teamID: String\) async -> \[RegistryDevice\]\?' "$file"; then
exit 1
fi
fiRepository: manaflow-ai/cmux
Length of output: 191
🏁 Script executed:
# First, check if the file exists and get its size
file='Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift'
if [ -f "$file" ]; then
lines=$(wc -l < "$file")
echo "File exists: $lines lines"
echo "---"
# Read the relevant sections around lines 1522-1524 and 1741-1770
echo "=== Context around lines 1522-1524 ==="
sed -n '1515,1535p' "$file" | cat -n
echo ""
echo "=== Context around lines 1741-1770 ==="
sed -n '1735,1780p' "$file" | cat -n
else
echo "File not found"
fiRepository: manaflow-ai/cmux
Length of output: 4115
🏁 Script executed:
# Search for the syncStoreDevices method definition
rg -n 'syncStoreDevices' 'Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift' -A 3Repository: manaflow-ai/cmux
Length of output: 678
🏁 Script executed:
# Check for discouraged_optional_collection pattern in the file
rg -n '\[RegistryDevice\]\?' 'Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift'Repository: manaflow-ai/cmux
Length of output: 144
🏁 Script executed:
# Find all calls to syncStoreDevices to ensure the diff covers all sites
rg -n 'syncStoreDevices' 'Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift'Repository: manaflow-ai/cmux
Length of output: 312
🏁 Script executed:
# Check if the file has any existing result enum patterns or similar
rg -n 'private enum.*{' 'Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift' | head -20Repository: manaflow-ai/cmux
Length of output: 154
Replace [RegistryDevice]? with a result enum to avoid SwiftLint's discouraged_optional_collection warning while preserving semantics.
The method syncStoreDevices uses nil to mean "read failed or discarded" and a non-nil array to mean "valid list (possibly empty)". SwiftLint flags optional collections, but the distinction is necessary here. Use a tiny result enum to keep both cases clear without the lint warning.
Proposed shape
+ private enum SyncStoreDeviceRead {
+ case valid([RegistryDevice])
+ case invalid
+ }
+
...
- if let devices = await syncStoreDevices(teamID: teamID), !devices.isEmpty {
+ if case .valid(let devices) = await syncStoreDevices(teamID: teamID), !devices.isEmpty {
registryDevices = devices
return
}
...
- private func syncStoreDevices(teamID: String) async -> [RegistryDevice]? {
- guard deviceListLocalFirst, let syncStore else { return nil }
+ private func syncStoreDevices(teamID: String) async -> SyncStoreDeviceRead {
+ guard deviceListLocalFirst, let syncStore else { return .invalid }
let requestingUserID = identityProvider?.currentUserID
let loaded: [RegistryDevice]
do {
loaded = try await DeviceSyncFacade(store: syncStore).registryDevices(teamID: teamID)
} catch {
mobileShellLog.debug(
"device sync facade read failed: \(String(describing: error), privacy: .public)"
)
- return nil
+ return .invalid
}
- guard isSignedIn, identityProvider?.currentUserID == requestingUserID else { return nil }
- guard (await syncTeamIDProvider?() ?? "") == teamID else { return nil }
+ guard isSignedIn, identityProvider?.currentUserID == requestingUserID else { return .invalid }
+ guard (await syncTeamIDProvider?() ?? "") == teamID else { return .invalid }
let connectedID = connectedMacDeviceID
- return loaded.sorted { lhs, rhs in
+ return .valid(loaded.sorted { lhs, rhs in
let lhsConnected = lhs.deviceId == connectedID
let rhsConnected = rhs.deviceId == connectedID
if lhsConnected != rhsConnected { return lhsConnected }
return lhs.lastSeenAt > rhs.lastSeenAt
- }
+ })
}
...
func reloadDeviceListFromSyncStore(teamID: String) async {
- if let devices = await syncStoreDevices(teamID: teamID) {
+ if case .valid(let devices) = await syncStoreDevices(teamID: teamID) {
registryDevices = devices
}
}📝 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
| if let devices = await syncStoreDevices(teamID: teamID), !devices.isEmpty { | |
| registryDevices = devices | |
| return | |
| private enum SyncStoreDeviceRead { | |
| case valid([RegistryDevice]) | |
| case invalid | |
| } | |
| // ... elsewhere in file ... | |
| if case .valid(let devices) = await syncStoreDevices(teamID: teamID), !devices.isEmpty { | |
| registryDevices = devices | |
| return | |
| } | |
| // ... function implementations ... | |
| private func syncStoreDevices(teamID: String) async -> SyncStoreDeviceRead { | |
| guard deviceListLocalFirst, let syncStore else { return .invalid } | |
| let requestingUserID = identityProvider?.currentUserID | |
| let loaded: [RegistryDevice] | |
| do { | |
| loaded = try await DeviceSyncFacade(store: syncStore).registryDevices(teamID: teamID) | |
| } catch { | |
| mobileShellLog.debug( | |
| "device sync facade read failed: \(String(describing: error), privacy: .public)" | |
| ) | |
| return .invalid | |
| } | |
| guard isSignedIn, identityProvider?.currentUserID == requestingUserID else { return .invalid } | |
| guard (await syncTeamIDProvider?() ?? "") == teamID else { return .invalid } | |
| let connectedID = connectedMacDeviceID | |
| return .valid(loaded.sorted { lhs, rhs in | |
| let lhsConnected = lhs.deviceId == connectedID | |
| let rhsConnected = rhs.deviceId == connectedID | |
| if lhsConnected != rhsConnected { return lhsConnected } | |
| return lhs.lastSeenAt > rhs.lastSeenAt | |
| }) | |
| } | |
| func reloadDeviceListFromSyncStore(teamID: String) async { | |
| if case .valid(let devices) = await syncStoreDevices(teamID: teamID) { | |
| registryDevices = devices | |
| } | |
| } |
🤖 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
`@Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift`
around lines 1522 - 1524, Replace the optional array return type
`[RegistryDevice]?` from the `syncStoreDevices` method with a result enum that
explicitly represents the two cases: either a successful read (with a possibly
empty array) or a failed/discarded read. Create a new enum type (or use Swift's
Result type) and update the `syncStoreDevices` method to return this enum
instead of an optional collection. Then update all call sites of
`syncStoreDevices`, including the conditional check at lines 1522-1524, to
handle the new enum cases using appropriate pattern matching instead of the
current optional binding check.
Source: Linters/SAST tools
The local→DO seed wrote account-derived provisional rows keyed only by team, and neither the in-flight race nor sign-out cleared them, so user A's local-only Macs could leak into a later user B on the same team/device. Two guards: - After the (non-cancellation-aware) migration await, recheck the account; if the user signed out or switched mid-seed, clear the team and abandon rather than persist/render A's rows. - On actual sign-out, clear the sync store for the teams this session seeded, so nothing account-derived survives for the next user. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…-empty handling Round 7 autoreview: - The sign-out cache wipe was a fire-and-forget Task that raced the next account's sign-in/seed. Replace it with READ-time isolation: the facade owner-scopes provisional (rev==0) rows by ownerUserId; authoritative (rev>=1) rows stay team-shared. Same model as MobilePairedMacStore persisting across sign-out and filtering by stackUserID. No clear -> no race, and consistent with the rest of the app. - The empty-store registry fallback resurrected stale registry rows even when the DO had synced an authoritatively-empty team. Gate the fallback on cursor == 0 (cache not warm yet); once synced, an empty list is authoritative and shown. Tests: provisional rows owner-scoped while authoritative rows stay shared; an authoritative empty (cursor>0) does not fall back to a stale registry device. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift (2)
1507-1524:⚠️ Potential issue | 🟠 Major | ⚡ Quick winTrack teams rendered from the cold sync-store read.
teamsToClearonly includesseededSyncTeamsandsyncSubscriptionTeamID. IfloadRegistryDevices()reads and renders a cachedteamIDbefore the subscription task reaches Line 1723, sign-out can miss that team and leave its persisted rows available to the next account. Record every team touched by local-first reads and clear that set on sign-out.Suggested shape
private var seededSyncTeams: Set<String> = [] + private var touchedSyncTeams: Set<String> = [] ... if !teamID.isEmpty { + touchedSyncTeams.insert(teamID) // Team switched while a socket was pinned to the old team: restart- let teamsToClear: Set<String> = isSignedIn + let teamsToClear: Set<String> = isSignedIn ? [] - : seededSyncTeams.union(syncSubscriptionTeamID.map { [$0] } ?? []) + : touchedSyncTeams + .union(seededSyncTeams) + .union(syncSubscriptionTeamID.map { [$0] } ?? []) ... seededSyncTeams = [] + touchedSyncTeams = []As per coding guidelines, “verify team/user switching and store clearing/revalidation are handled so a saved device list cursor/seed can’t be trusted across account/team changes.”
🤖 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 `@Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift` around lines 1507 - 1524, The `teamsToClear` set does not include teams that are loaded directly from the sync-store via the `syncStoreDevices(teamID: teamID)` call in this code block. When devices are successfully retrieved and assigned to `registryDevices`, the associated `teamID` must be tracked in a separate collection so it can be included in the sign-out clearing logic. Create a new property to track all teams touched by local-first reads (including the `teamID` parameter passed to `syncStoreDevices`), and ensure this collection is cleared along with `teamsToClear` during sign-out to prevent persisted rows from being available to the next account.Source: Coding guidelines
1636-1805: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy liftMove the device-list sync loop out of
MobileShellComposite.This adds persistence, migration, transport subscription, backoff, and cache invalidation into an already 5k+ line composite that also owns terminal I/O, pairing, drafts, presence, feedback, and workspace mutation. Extract the new device-list sync state machine into a focused helper/coordinator in the same package, with closures for
reloadDeviceListFromSyncStore/logging, so the composite only starts/stops it.As per coding guidelines, Swift production files over 800 lines and files mixing UI state, persistence, networking, socket protocol, and platform bridge responsibilities should be flagged.
🤖 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 `@Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift` around lines 1636 - 1805, Extract the device-list sync logic from MobileShellComposite into a new focused helper/coordinator class in the same package. Move the properties syncTask, syncSubscriptionTeamID, and seededSyncTeams, along with the methods evaluateSyncSubscription(), startSyncSubscription(), makeSyncSubscriptionClient(), and syncStoreDevices() into this new coordinator class. The coordinator should accept closures for reloadDeviceListFromSyncStore and logging to maintain the callback contract. Update MobileShellComposite to only start/stop the coordinator instance, keeping just the minimal interface needed to integrate with the composite's sign-in state and flag management.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift`:
- Around line 1657-1670: The fire-and-forget clear operation in the sign-out
block can race with fast account switches, causing the next sign-in to read
stale data or delete new session rows. Add a `syncCacheClearTask` property to
serialize this operation, store the cache clearing Task in it instead of using
fire-and-forget, and await this task in the `makeSyncSubscriptionClient()` and
`syncStoreDevices(teamID:)` methods before seeding or reading to ensure the
cache clear completes before any new local-first operations begin.
---
Outside diff comments:
In
`@Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift`:
- Around line 1507-1524: The `teamsToClear` set does not include teams that are
loaded directly from the sync-store via the `syncStoreDevices(teamID: teamID)`
call in this code block. When devices are successfully retrieved and assigned to
`registryDevices`, the associated `teamID` must be tracked in a separate
collection so it can be included in the sign-out clearing logic. Create a new
property to track all teams touched by local-first reads (including the `teamID`
parameter passed to `syncStoreDevices`), and ensure this collection is cleared
along with `teamsToClear` during sign-out to prevent persisted rows from being
available to the next account.
- Around line 1636-1805: Extract the device-list sync logic from
MobileShellComposite into a new focused helper/coordinator class in the same
package. Move the properties syncTask, syncSubscriptionTeamID, and
seededSyncTeams, along with the methods evaluateSyncSubscription(),
startSyncSubscription(), makeSyncSubscriptionClient(), and syncStoreDevices()
into this new coordinator class. The coordinator should accept closures for
reloadDeviceListFromSyncStore and logging to maintain the callback contract.
Update MobileShellComposite to only start/stop the coordinator instance, keeping
just the minimal interface needed to integrate with the composite's sign-in
state and flag management.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c044a4dc-fb22-4297-a833-bef3a9fa7c23
📒 Files selected for processing (2)
Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/PresenceSyncTransport.swift
The facade owner filter only applied when provisionalOwnerUserID was non-nil, so a nil owner (signed in but user id momentarily unavailable) showed ALL accounts' provisional rows — fail open. Invert it: provisional (rev==0) rows require a present, matching owner; nil owner returns no provisional rows (authoritative team-shared rows unaffected). Tests updated to seed/read with an owner and to assert nil-owner yields only the shared authoritative device. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…authoritative empty The earlier authoritative-empty fix set registryDevices=[], but deviceTreeDevices' legacy XOR fallback then synthesized the tree from the OLD pairedMacs, re-showing stale local Macs the DO no longer knows about. In local-first mode the sync store is authoritative (paired Macs are seeded into it as provisional rows), so gate that pairedMacs fallback off; loadRegistryDevices still falls back to /api/devices before the first sync, so this only bites once the store is authoritatively empty. Test: with a local paired Mac loaded and an authoritatively-empty sync store, the tree stays empty (the Mac is not resurrected). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…authoritative Round 10 autoreview: - [P1] The sync subscription Task was @mainactor, so every frame parse + apply (including shared-socket presence-tick noise) ran on the UI thread. Drop @mainactor: makeSyncSubscriptionClient (team/seed/build) and the apply callback are @mainactor and reached via await; SyncFrameApplier is its own actor; SyncClient/transport are Sendable. The frame loop now parses off-main. - [P2] Gating deviceTreeDevices' paired-Mac fallback purely on local-first hid local Macs before the store had synced (offline first launch -> empty). Gate on a new deviceSyncAuthoritative flag (set when cursor>0 / a frame commits, reset on team change + sign-out), so the fallback persists until an empty list is authoritative. Tests: before sync the tree shows the local paired Mac; once authoritatively empty it does not. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Packages/iOS/CmuxMobileShell/Tests/CmuxMobileShellTests/DeviceListLocalFirstWiringTests.swift (1)
203-216:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMake the stale-team test exercise a non-empty old-team payload
On Line 206, the composite is created without an identity provider, so
provisionalOwnerUserIDis nil and the seededrev == 0rows are filtered out before the stale-team guard is exercised. This test can pass even if stale-team overwrite protection regresses.Suggested fix
let composite = MobileShellComposite( isSignedIn: true, syncStore: store, deviceListLocalFirst: true, syncTeamIDProvider: { "team-B" }, // user is now on team-B makeSyncTransport: makeTransportFactory(), + identityProvider: FakeIdentity(userID: Self.owner), deliveredNotificationClearer: NoopDeliveredNotificationClearer() )🤖 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 `@Packages/iOS/CmuxMobileShell/Tests/CmuxMobileShellTests/DeviceListLocalFirstWiringTests.swift` around lines 203 - 216, The staleTeamFrameDoesNotOverwriteCurrentTeam test does not provide an identityProvider when creating the MobileShellComposite, which causes the seeded team-A rows to be filtered out before the stale-team overwrite protection code is even exercised. Add an identityProvider closure to the MobileShellComposite initialization so that the provisionalOwnerUserID is set and the seeded rows with rev == 0 are not filtered out, ensuring the test actually exercises the stale-team guard with a non-empty payload.Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift (1)
1522-1535:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSeparate invalid sync-store reads from authoritative empty before clearing.
syncStoreDevicesreturnsnilfor read errors and stale user/team guards, but Line 1531 still checks the cursor and can clearregistryDeviceswhencursor > 0. That turns a failed or stale cache read into an authoritative empty list. Return an explicit result and only run the cursor/clear path after a validated empty read; also re-check user/team after the cursor await.Suggested shape
+ private enum SyncStoreDeviceRead { + case valid([RegistryDevice]) + case invalid + } + @@ - if let devices = await syncStoreDevices(teamID: teamID), !devices.isEmpty { + switch await syncStoreDevices(teamID: teamID) { + case .valid(let devices) where !devices.isEmpty: registryDevices = devices return - } + case .valid: + let requestingUserID = identityProvider?.currentUserID + if let syncStore, + let cursor = try? await syncStore.cursor(teamID: teamID, collection: devicesSyncCollection), + cursor > 0 { + let currentTeamID = (await syncTeamIDProvider?()) ?? "" + guard isSignedIn, + identityProvider?.currentUserID == requestingUserID, + currentTeamID == teamID else { return } + registryDevices = [] + return + } + case .invalid: + break + } @@ - if let syncStore, - let cursor = try? await syncStore.cursor(teamID: teamID, collection: devicesSyncCollection), - cursor > 0 { - registryDevices = [] - return - }- private func syncStoreDevices(teamID: String) async -> [RegistryDevice]? { - guard deviceListLocalFirst, let syncStore else { return nil } + private func syncStoreDevices(teamID: String) async -> SyncStoreDeviceRead { + guard deviceListLocalFirst, let syncStore else { return .invalid } @@ - return nil + return .invalid } - guard isSignedIn, identityProvider?.currentUserID == requestingUserID else { return nil } - guard (await syncTeamIDProvider?() ?? "") == teamID else { return nil } + guard isSignedIn, identityProvider?.currentUserID == requestingUserID else { return .invalid } + guard (await syncTeamIDProvider?() ?? "") == teamID else { return .invalid } let connectedID = connectedMacDeviceID - return loaded.sorted { lhs, rhs in + return .valid(loaded.sorted { lhs, rhs in let lhsConnected = lhs.deviceId == connectedID let rhsConnected = rhs.deviceId == connectedID if lhsConnected != rhsConnected { return lhsConnected } return lhs.lastSeenAt > rhs.lastSeenAt - } + }) } @@ - if let devices = await syncStoreDevices(teamID: teamID) { + if case .valid(let devices) = await syncStoreDevices(teamID: teamID) { registryDevices = devices }As per coding guidelines, “When the diff replaces a fresh authoritative read with a cached value in a persistence, history, undo, or snapshot path, require explicit handling of cold (never-loaded) and stale (older-than-source) caches.”
Also applies to: 1786-1803
🤖 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 `@Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift` around lines 1522 - 1535, The syncStoreDevices function currently returns nil for both read errors and valid empty results, causing the cursor check on line 1531 to incorrectly treat failed or stale reads as authoritative empty lists. Modify syncStoreDevices to return an explicit result type that distinguishes between errors/stale results and valid empty reads. Update the logic to only proceed with the cursor check and registryDevices clearing when syncStoreDevices returns a valid empty result (not when it returns nil due to an error or stale user/team guard). Additionally, re-validate the user and team after the cursor await operation to ensure they haven't changed since the syncStoreDevices check.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In
`@Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift`:
- Around line 1522-1535: The syncStoreDevices function currently returns nil for
both read errors and valid empty results, causing the cursor check on line 1531
to incorrectly treat failed or stale reads as authoritative empty lists. Modify
syncStoreDevices to return an explicit result type that distinguishes between
errors/stale results and valid empty reads. Update the logic to only proceed
with the cursor check and registryDevices clearing when syncStoreDevices returns
a valid empty result (not when it returns nil due to an error or stale user/team
guard). Additionally, re-validate the user and team after the cursor await
operation to ensure they haven't changed since the syncStoreDevices check.
In
`@Packages/iOS/CmuxMobileShell/Tests/CmuxMobileShellTests/DeviceListLocalFirstWiringTests.swift`:
- Around line 203-216: The staleTeamFrameDoesNotOverwriteCurrentTeam test does
not provide an identityProvider when creating the MobileShellComposite, which
causes the seeded team-A rows to be filtered out before the stale-team overwrite
protection code is even exercised. Add an identityProvider closure to the
MobileShellComposite initialization so that the provisionalOwnerUserID is set
and the seeded rows with rev == 0 are not filtered out, ensuring the test
actually exercises the stale-team guard with a non-empty payload.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4e7bcfb7-b422-4d04-b97b-8774165ba8fd
📒 Files selected for processing (4)
Packages/Shared/CmuxSyncStore/Sources/CmuxSyncStore/DeviceSyncFacade.swiftPackages/Shared/CmuxSyncStore/Tests/CmuxSyncStoreTests/DeviceListSyncWiringTests.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swiftPackages/iOS/CmuxMobileShell/Tests/CmuxMobileShellTests/DeviceListLocalFirstWiringTests.swift
deviceSyncAuthoritative was a single Bool, so a synced (empty) team-A left the gate set when switching to an unsynced team-B, hiding team-B's paired-Mac fallback. Track the synced team (deviceSyncAuthoritativeTeamID) and the rendered team (deviceListTeamID); gate only when they match. Reset on sign-out; a team switch updates deviceListTeamID so the gate naturally disengages until the new team syncs. Test: team-A authoritative-empty gates; after switching to unsynced team-B the tree falls back to the local paired Mac. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…er await The cursor-read branch blanked registryDevices after an await without re-checking the session, so a stale read could clear the current device tree. Capture the requesting user, re-guard after the team-resolve await, and re-guard (user + team) after the cursor await before blanking — mirroring syncStoreDevices' own guards. A stale read now returns without mutating the new session's list. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… + lockfile
- CMUXMobileRootScene: the makeSyncTransport closure via .map was type-ambiguous
('type of expression is ambiguous'); build it with an explicit @sendable typed
closure so cmuxFeature compiles (the iOS build CI caught this; local package
builds didn't exercise cmuxFeature).
- PresenceSyncTransport: add DocC docs on the public init/send/frames (Aziz docs
policy).
- Regenerate ios/cmuxPackage/Package.resolved (originHash) for the CmuxSyncStore
dependency (SwiftPM lockfile policy).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 2c79ecf. Configure here.
| if deviceListLocalFirst, syncStore != nil, makeSyncTransport != nil, | ||
| let synced = deviceSyncAuthoritativeTeamID, synced == deviceListTeamID { | ||
| return [] | ||
| } |
There was a problem hiding this comment.
New pair hidden after empty sync
Medium Severity
With mobileDeviceListLocalFirst on, after the sync store has an authoritative empty snapshot for the team (cursor > 0), pairing a new Mac only updates pairedMacStore. PairedMacMigration does not run again, so no new provisional sync row is written. deviceTreeDevices then returns an empty list instead of synthesizing from pairedMacs, so the new Mac never appears in the device tree until the DO publishes it.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 2c79ecf. Configure here.
lawrencecchen
added
the
stale-revisit
This branch was successfully deployed
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
2 participants
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.
Problem
After updating the TestFlight build, the saved-devices list went empty. Root cause (traced in code):
deviceTreeDevices(MobileShellComposite.swift) is registry XOR local — it returns the in-memoryregistryDeviceslist entirely when non-empty, else synthesizes from local paired Macs.registryDevicesis refetched fromGET /api/deviceseach launch and is never persisted. So the moment the registry returns any non-empty-but-incomplete set, locally-saved Macs are masked out, and nothing durable backs the list across an update or surfaces it on another device.Fix: wire the already-built cross-device sync
The server side is live (the
cmux-presenceDO derives a per-teamdevicescollection from Mac heartbeats and serves it over the presence WebSocket viasync/v1), and the iOSCmuxSyncStorepackage is built + tested — it was just never imported by the app. This is the deferred "Phase 2: iOS app wiring" fromplans/feat-do-device-list/DESIGN.md.CmuxMobileShell+cmuxFeaturenow depend onCmuxSyncStore; the root scene openscmux-sync.sqlite3next topaired-macs.sqlite3(Application Support → survives updates) and injects it plus the resolvedmobileDeviceListLocalFirstflag.PresenceSyncTransport: aSyncTransportover its own authenticated/v1/presence/subscribeWebSocket, isolated from the live presence stream (online dots / route-push / auto-attach untouched).SyncClientignores the presence noise on the shared endpoint.MobileShellCompositeruns a sync subscription mirroring the presence backoff loop, seeds provisional rows from the existing paired-Mac store viaPairedMacMigrationon first launch (instant render pre-frame; offline Macs never lost; reconciliation exemptsrev == 0), and reads the list fromDeviceSyncFacadewhen the flag is on.Cross-device: the Mac already publishes itself to the team DO, so a fresh sign-in on another device subscribes and renders it with zero setup.
Safety: gated
mobileDeviceListLocalFirstDEBUG-on/Release-off; flag OFF keeps today's/api/devicespath verbatim. Production list behavior is unchanged until the flag is flipped after dogfood.Tests (all green via
swift test)devicessnapshot from another device streams in over the transport and renders locally.MobileShellRenderGridLivenessTestsfail identically on clean main; unrelated.)Dogfood
Build on
dog(mac + iOS) follows. Verify: list survives an over-the-top reinstall, and a Mac appears on a second signed-in device with no setup.🤖 Generated with Claude Code
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Note
Medium Risk
Touches session-scoped device list, multi-account privacy, and sync cursor semantics; behavior is flag-gated in Release but the wiring paths are complex and race-sensitive.
Overview
Wires the existing CmuxSyncStore into the iOS app so the device tree can load from a durable SQLite cache and sync/v1 over presence, instead of refetching
GET /api/devicesevery launch.mobileDeviceListLocalFirst(DEBUG on, Release off) keeps production on the registry path until dogfood.The composition root opens
cmux-sync.sqlite3next to paired Macs and injects store, team resolver, andPresenceSyncTransport(separate authenticated presence WebSocket).MobileShellCompositemirrors the presence loop: backoff subscription,PairedMacMigrationseed, andloadRegistryDevicesreadingDeviceSyncFacadewhen flag + transport are wired—with registry fallback when the store is empty andcursor == 0, and an authoritative empty whencursor > 0(no stale registry or paired-Mac XOR resurrection).DeviceSyncFacade.registryDevicesnow filtersrev == 0provisional rows byprovisionalOwnerUserID(nil owner fails closed);rev >= 1rows stay team-shared. Sign-out tears down the sync task without clearing the on-disk cache; reads enforce account/team guards against stale async results.Reviewed by Cursor Bugbot for commit 2c79ecf. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Backed the iOS device list with a durable, cross-device sync store so saved Macs survive updates and show on other signed-in devices. Behavior stays unchanged behind
mobileDeviceListLocalFirst(DEBUG on, Release off).New Features
CmuxSyncStoreintoCmuxMobileShell/cmuxFeature; openscmux-sync.sqlite3in Application Support.PresenceSyncTransporton a dedicated authenticated/v1/presence/subscribeforsync/v1withbufferingOldestand clean reconnects.MobileShellCompositeruns an off‑main backoff sync loop, seeds viaPairedMacMigration, and reads viaDeviceSyncFacadewhen the flag is on.Bug Fixes
/api/devices; oncecursor > 0, an empty list is authoritative.rev==0) rows are owner‑scoped and fail closed on unknown owner; authoritative rows are team‑shared; authoritative‑empty is team‑scoped; restarts on team switch; stale‑team frames cannot overwrite the current list.makeSyncTransportclosure to unblockcmuxFeaturebuild, added DocC forPresenceSyncTransport, and updatedPackage.resolved.Written for commit 2c79ecf. Summary will update on new commits.
Summary by CodeRabbit