Preserve iOS workspace groups during reconnect - #9388
Merged
Merged
Conversation
📝 WalkthroughWalkthroughThe workspace-list response now records whether ChangesWorkspace group refresh
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant MobileCoreRPCClient
participant MobileSyncWorkspaceListResponse
participant MobileShellComposite
participant WorkspaceGroupState
MobileCoreRPCClient->>MobileSyncWorkspaceListResponse: decode workspace-list response
MobileSyncWorkspaceListResponse-->>MobileCoreRPCClient: groups and groupsFieldWasPresent
MobileCoreRPCClient->>MobileShellComposite: applyRemoteWorkspaceList
MobileShellComposite->>WorkspaceGroupState: preserve, update, or clear group metadata
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 25✅ Passed checks (25 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 11894-11895: Remove the empty-workspace veto around existingGroups
so an authoritative response with groups: [] can clear stale group headers;
preserve any guard only for incomplete reconnects, using groupsAreAuthoritative
to distinguish them. Add coverage for a connected authoritative snapshot
containing empty workspaces and groups.
🪄 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 Plus
Run ID: 472781e4-7a7d-4bc9-85d8-d196f3ce7191
📒 Files selected for processing (4)
Packages/iOS/CmuxMobileRPC/Sources/CmuxMobileRPC/MobileSyncWorkspaceListResponse.swiftPackages/iOS/CmuxMobileRPC/Tests/CmuxMobileRPCTests/MobileCoreRPCClientTests.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swiftPackages/iOS/CmuxMobileShell/Tests/CmuxMobileShellTests/MobileShellCompositePreviewTests.swift
Resolve the workspace-list response test conflict and keep authoritative connected empty snapshots able to clear stale iOS group headers.
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
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)
10812-10825: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winRemove timer-based probe coordination.
Lines 10812-10825 add probe-and-repair work inside a task that an outer timer cancels. Both request paths already pass
timeoutNanosecondstoclient.sendRequest.Keep timeout ownership in
MobileCoreRPCClient.sendRequest. Move the switch directly intoprobeEventSubscriptionLivenessand remove the outer task and timer. This avoids a second cancellation owner in the socket liveness path.As per coding guidelines, “Do not introduce or materially expand … timers … for synchronization … or delayed coordination.”
🤖 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 10812 - 10825, Remove the outer timer-backed task around the subscription probe in MobileShellComposite and move the active/missing/unsupported switch directly into probeEventSubscriptionLiveness. Invoke requestTerminalEventSubscriptionProbe and requestTerminalEventSubscription there, preserving their timeoutNanoseconds arguments and existing repair behavior, while leaving cancellation and timeout ownership to MobileCoreRPCClient.sendRequest.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 10812-10825: Remove the outer timer-backed task around the
subscription probe in MobileShellComposite and move the
active/missing/unsupported switch directly into probeEventSubscriptionLiveness.
Invoke requestTerminalEventSubscriptionProbe and
requestTerminalEventSubscription there, preserving their timeoutNanoseconds
arguments and existing repair behavior, while leaving cancellation and timeout
ownership to MobileCoreRPCClient.sendRequest.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 2bde3d63-71eb-4c01-bc90-7a9d2324f920
📒 Files selected for processing (4)
Packages/iOS/CmuxMobileRPC/Sources/CmuxMobileRPC/MobileSyncWorkspaceListResponse.swiftPackages/iOS/CmuxMobileRPC/Tests/CmuxMobileRPCTests/MobileCoreRPCClientTests.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swiftPackages/iOS/CmuxMobileShell/Tests/CmuxMobileShellTests/MobileShellCompositePreviewTests.swift
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
1 participant
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
groups, so an omitted field no longer looks like an authoritative empty list.Related context: #9326
Testing
test-ios.ymlonfix-ios-group-reconnect-flat:MobileShellCompositePreviewTestscompiled and passed, including the new preservation cases. Run: https://github.com/manaflow-ai/cmux/actions/runs/30706027185WorkspaceListTableCoordinatorDropTests.swiftcompile errors, and unrelated flakyCmuxMobileShelltests outside this change.gflatpassed and producedcmux DEV gflat.app. Run: https://github.com/manaflow-ai/cmux/actions/runs/30706097063Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by cubic
Preserves iOS workspace group headers during reconnects and when
groupsis omitted or temporarily empty, so sections don’t disappear. Still clears stale headers on a connected, authoritative ungrouped snapshot.CmuxMobileRPC: addedgroupsFieldWasPresenttoMobileSyncWorkspaceListResponseand set it viacontainer.contains(.groups)to distinguish omitted vs emptygroups.CmuxMobileShell: preserves last groups while reconnecting/rebinding or disconnected; accepts an empty group snapshot only when both iPhone and Mac are connected and no workspace has agroupID.groups, disconnected empty refresh preservation, and connected empty/ungrouped clearing.Written for commit 9f4e7b7. Summary will update on new commits.
Summary by CodeRabbit