Improve Simulator discoverability on macOS and iOS - #9888
Conversation
|
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:
📝 WalkthroughWalkthroughAdds Simulator picker controls, serialized stream selection, deterministic DEBUG preview fixtures, UI tests, shared store ownership, and the built-in Simulator surface-tab action. ChangesSimulator discoverability
Vendor pointer update
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant WorkspaceDetailView
participant SimulatorPickerControl
participant MobileShellComposite
participant MobileSimulatorStreamSelectionCoordinator
WorkspaceDetailView->>SimulatorPickerControl: render simulator picker state
SimulatorPickerControl->>WorkspaceDetailView: select target panel
WorkspaceDetailView->>MobileShellComposite: request simulator transition
MobileShellComposite->>MobileSimulatorStreamSelectionCoordinator: enqueue selection
MobileSimulatorStreamSelectionCoordinator->>MobileShellComposite: stop previous and start target stream
Possibly related PRs
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (3 errors, 2 warnings)
✅ Passed checks (20 passed)
✨ Finishing Touches 💡 1📝 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceDetailView.swift (1)
959-970: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftSerialize simulator stream transitions.
Each selection starts an independent, unowned task. During rapid A → B → C selection, the B task can resume and start B after the C task has stopped B. The local store then shows C as active while B also runs remotely.
Move this transition into a caller-owned coordinator or store operation that serializes the latest intent and cancels or supersedes older requests.
As per coding guidelines, do not create fire-and-forget
Task { ... }work with meaningful lifecycle unless it is stored, cancellable, or tied to a caller-owned operation.🤖 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/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceDetailView.swift` around lines 959 - 970, Replace the unowned Task transition around stopMobileSimulatorStream and startMobileSimulatorStream with a caller-owned, stored coordinator or store operation that serializes selection intents. Ensure rapid selections cancel or supersede older transitions so only the latest panel’s stream is active, and expose lifecycle cancellation rather than creating fire-and-forget work.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 `@ios/cmuxUITests/cmuxUITests.swift`:
- Around line 7681-7684: Update the test lifecycle around setUpWithError and
tearDownWithError to restore XCUIDevice.shared.orientation to its prior/default
state and terminate the two application during teardown. Ensure cleanup runs
after every test, including failed tests, so shared device state and the fixture
process do not leak into later tests.
In
`@Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/SimulatorPickerMenuValue.swift`:
- Around line 11-14: Update the initializer logic in SimulatorPickerMenuValue so
activePanelID is validated against the filtered self.rows rather than the
original rows parameter, ensuring it becomes nil whenever
supportsSimulatorStream is false and preserving valid IDs only for available
panels.
---
Outside diff comments:
In
`@Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceDetailView.swift`:
- Around line 959-970: Replace the unowned Task transition around
stopMobileSimulatorStream and startMobileSimulatorStream with a caller-owned,
stored coordinator or store operation that serializes selection intents. Ensure
rapid selections cancel or supersede older transitions so only the latest
panel’s stream is active, and expose lifecycle cancellation rather than creating
fire-and-forget work.
🪄 Autofix
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: 001c6a3c-0e17-4642-b11b-787ba3c1c596
📒 Files selected for processing (11)
Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite+SimulatorPreview.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/SimulatorDiscoverabilityPreviewView.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/SimulatorPickerControl.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/SimulatorPickerMenuValue.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceDetailView+AgentChat.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceDetailView.swiftPackages/iOS/CmuxMobileShellUI/Tests/CmuxMobileShellUITests/SimulatorPickerMenuValueTests.swiftSources/CmuxConfig.swiftcmuxTests/CmuxConfigTests.swiftios/cmuxPackage/Sources/cmuxFeature/CMUXMobileRootScene.swiftios/cmuxUITests/cmuxUITests.swift
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 `@vendor/bonsplit`:
- Line 1: Update the vendor/bonsplit submodule reference so it points to a
commit available on the configured submodule remote, either by pushing commit
8230030608931784878dc46b8b8581756e7a2d69 to the remote branch or changing the
superproject pointer to an already-pushed commit.
🪄 Autofix
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: 4c1c33c9-825d-42da-9fe0-21770e061879
📒 Files selected for processing (13)
Packages/iOS/CmuxMobileShell/Package.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite+SimulatorStream.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileSimulatorStreamSelectionCoordinator.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShellDebugSupport/MobileShellComposite+SimulatorPreview.swiftPackages/iOS/CmuxMobileShell/Tests/CmuxMobileShellTests/MobileSimulatorStreamSelectionCoordinatorTests.swiftPackages/iOS/CmuxMobileShellUI/Package.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/SimulatorDiscoverabilityPreviewView.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/SimulatorPickerMenuValue.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceDetailView+AgentChat.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceDetailView.swiftios/cmuxUITests/cmuxUITests.swiftvendor/bonsplit
| @@ -1 +1 @@ | |||
| Subproject commit 529913b7a49c39669414578f1f32ccf2df47f0f8 | |||
| Subproject commit 8230030608931784878dc46b8b8581756e7a2d69 | |||
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
path="vendor/bonsplit"
sha="8230030608931784878dc46b8b8581756e7a2d69"
remote="$(git -C "$path" remote get-url origin)"
git -C "$path" cat-file -e "$sha^{commit}"
git -C "$path" fetch --quiet "$remote" '+refs/heads/*:refs/remotes/verify/*'
if ! git -C "$path" branch -r --contains "$sha" | grep -q 'verify/'; then
echo "Commit $sha is not reachable from a remote branch."
exit 1
fiRepository: manaflow-ai/cmux
Length of output: 239
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Top-level git status:"
git status --short --branch || true
echo
echo "Tracked item at vendor/bonsplit (tracked-submodule/ls-files):"
git ls-files -s vendor/bonsplit || true
git ls-files -p vendor/bonsplit | head -20 || true
echo
echo "Untracked/vendor/bonsplit directory files if present:"
find vendor/bonsplit -maxdepth 2 -type f 2>/dev/null | sed -n '1,40p' || true
echo
echo "Recent diff summary:"
git diff --stat HEAD~1..HEAD || git diff --stat || trueRepository: manaflow-ai/cmux
Length of output: 3467
🌐 Web query:
git submodule commit not reachable remote branch fresh checkout submodule issue
💡 Result:
The error fatal: reference is not a tree or failure to find a submodule commit during a fresh checkout occurs because Git submodules are pinned to specific commit hashes in the superproject [1][2][3]. If that exact commit hash is no longer reachable in the submodule's remote repository (e.g., due to a force-push, history rewrite, or because the commit was only ever local and never pushed), the superproject cannot complete the checkout [4][5][6]. To resolve this issue, consider the following strategies: 1. Recover the missing commit (Best case) If the commit still exists in a colleague's local clone or another environment, have them create a new branch at that specific commit and push it to the remote repository [4]. Once the commit is reachable via a branch, the submodule can be updated normally [4]. 2. Point to a valid, existing commit If the original commit is permanently lost, you must update the superproject to point to a valid, existing commit in the submodule [1][5][6]. - Navigate into the submodule directory: cd path/to/submodule [7][5][6] - Fetch the latest data: git fetch [6] - Checkout a stable, existing commit or branch: git checkout [1][7][6] - Return to the superproject and record the new commit: cd../ && git add path/to/submodule && git commit -m "Update submodule to valid commit" [1][7][5][6] 3. Use --remote to follow a branch (If applicable) If you do not strictly require the old, missing commit and simply want the submodule to track the latest state of its remote branch, you can force the update to ignore the pinned SHA-1 and pull from the remote branch instead [8][9]: git submodule update --init --remote --recursive [8] Note: This requires the submodule to have a properly configured branch in its.gitmodules file or.git/config [9][10]. Prevention To prevent this error in the future, always ensure that commits created inside a submodule are pushed to their remote repository before pushing the changes to the parent superproject [6][2]. You can automate this by setting the global configuration to push submodules automatically: git config --global push.recurseSubmodules on-demand [2]
Citations:
- 1: https://stackoverflow.com/questions/52813222/submodule-commited-branch-no-longer-available
- 2: https://deadends.dev/git/submodule-update-failed/
- 3: https://support.circleci.com/hc/en-us/articles/23781921297947-Understanding-the-fatal-reference-is-not-a-tree-Error-in-Git
- 4: https://stackoverflow.com/questions/77886342/submodule-commits-are-missing-locally
- 5: https://stackoverflow.com/questions/2155887/git-submodule-head-reference-is-not-a-tree-error
- 6: https://www.deployhq.com/blog/using-submodules-in-deploy
- 7: https://stackoverflow.com/questions/73628265/commit-not-found-when-syncing-submodule
- 8: https://stackoverflow.com/questions/42417294/why-does-git-fail-to-fetch-specific-valid-submodule-for-a-given-commit-and-how-t
- 9: https://github.com/git/git/blob/7649bfbaa2be9472e2c9ee2fab7525887d4c0fc9/Documentation/git-submodule.txt
- 10: https://stackoverflow.com/questions/68682838/update-submodules-some-tracking-branches-some-not-tracking-branches-with-re
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Top-level git status:"
git status --short --branch || true
echo
echo "Tracked item at vendor/bonsplit:"
git ls -s vendor/bonsplit 2>/dev/null || true
git ls-files --stage vendor/bonsplit 2>/dev/null || true
git ls-files -p vendor/bonsplit 2>/dev/null | sed -n '1,20p' || true
echo
echo "Untracked vendor/bonsplit directory files if present:"
find vendor/bonsplit -maxdepth 2 -type f 2>/dev/null | sed -n '1,40p' || true
echo
echo "Recent diff summary:"
git diff --stat HEAD~1..HEAD || git diff --stat || true
echo
echo "Diff around vendor/bonsplit:"
git diff -- vendor/bonsplit | sed -n '1,80p' || trueRepository: manaflow-ai/cmux
Length of output: 1325
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Remote URLs:"
git remote -v
echo
echo "Bonsplit submodule status/config:"
git submodule status vendor/bonsplit || true
git config -f vendor/bonsplit/.git/config --list 2>/dev/null | sed -n '/submodule/,/\\./p' || true
git -C vendor/bonsplit config --list --local 2>/dev/null | sed -n '1,120p' || true
git -C vendor/bonsplit remote -v 2>/dev/null || true
echo "HEAD in vendor/bonsplit:"
git -C vendor/bonsplit rev-parse HEAD 2>/dev/null || true
echo
echo "Superproject remote branches containing or nearby the new pinned SHA (if it still exists locally):"
if git rev-parse --verify "$sha^{commit}" >/dev/null 2>&1; then
git branch -r --contains "$sha" | sed -n '1,40p' || true
else
echo "Pinned SHA not present in the local bare object database"
fi
for remote in $(git remote); do
echo "--- $remote ---"
git ls-remote "$remote" --quiet --heads | awk '$2 ~ /('"$sha"')$|(^'"$sha"')$/' | sed -n '1,20p' || true
git ls-remote "$remote" --quiet --tags | awk '$2 ~ /('"$sha"')$|(^'"$sha"')$/' | sed -n '1,20p' || true
doneRepository: manaflow-ai/cmux
Length of output: 879
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sha="8230030608931784878dc46b8b8581756e7a2d69"
subdir="vendor/bonsplit"
subremote="$(git -C "$subdir" remote get-url origin || true)"
echo "Submodule checkout files:"
find "$(git rev-parse --path-format=absolute --git-dir=.)" modules/vendor/bonsplit -maxdepth 1 -type f 2>/dev/null | sed -n '1,20p' || true
find "$subdir/.git/modules/vendor/bonsplit/objects" -maxdepth 3 -type f 2>/dev/null | sed -n '1,40p' || true
echo
echo "Locally resolve/available check:"
if git -C "$subdir" cat-file -e "$sha^{commit}" 2>/dev/null; then
echo "SUBMODULE_LOCAL_COMMIT_EXISTS"
git -C "$subdir" branch -r --contains "$sha" 2>/dev/null | sed -n '1,40p' || true
else
echo "SUBMODULE_LOCAL_COMMIT_MISSING"
fi
echo
echo "Submodule origin HEAD/tag/object from remote refs:")
if [ -n "$subremote" ]; then
for ref in HEAD master main; do
echo "--- remote $subremote/HEAD ---"
git ls-remote "$subremote" "$ref" "$ref^{}" | sed -n '1,20p' || true
done
echo "--- exact pinned sha refs/tags/HEAD/object on subremote ---"
git ls-remote "$subremote" "$sha" "$sha^{}" | sed -n '1,20p' || true
for ref in "refs/heads/$sha" "refs/tags/$sha" "refs/heads/*"; do
git ls-remote "$subremote" "$sha" "$sha^{}" >> /tmp/verify_submodule_refs.out || true
done
else
echo "NO_SUBMODULE_ORIGIN"
fiRepository: manaflow-ai/cmux
Length of output: 300
Ensure vendor/bonsplit is updated to a pushed commit.
vendor/bonsplit points to 8230030608931784878dc46b8b8581756e7a2d69, and that commit is not present in this checked-out submodule tree. Push the intended submodule commit to the submodule remote branch or update the superproject pointer to a pushed commit so fresh checkouts and CI can fetch the submodule.
🤖 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 `@vendor/bonsplit` at line 1, Update the vendor/bonsplit submodule reference so
it points to a commit available on the configured submodule remote, either by
pushing commit 8230030608931784878dc46b8b8581756e7a2d69 to the remote branch or
changing the superproject pointer to an already-pushed commit.
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)
ios/cmuxUITests/cmuxUITests.swift (1)
7726-7727: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winWait for the UI state before both assertions.
After
panelB.tap(), Line 7727 can run beforeMobileSimulatorPickerreceives the new value. The assertion can fail intermittently. At Line 7735,existscan befalsebefore the workspace chrome finishes rendering, so the test can pass without proving that the picker is omitted. Wait for a value predicate after the tap and use a deadline-bounded absence check after the terminal picker is ready.Proposed test synchronization
panelB.tap() + let selectionExpectation = XCTNSPredicateExpectation( + predicate: NSPredicate(format: "value == %@", "iPhone B"), + object: twoPicker + ) + wait(for: [selectionExpectation], timeout: 4) XCTAssertEqual(twoPicker.value as? String, "iPhone B") ... - XCTAssertFalse(app.buttons["MobileSimulatorPicker"].exists) + XCTAssertFalse( + app.buttons["MobileSimulatorPicker"].waitForExistence(timeout: 2) + )As per coding guidelines, UI tests must await real completion signals or use deadline-bounded polls of real predicates before assertions.
Also applies to: 7734-7735
🤖 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 `@ios/cmuxUITests/cmuxUITests.swift` around lines 7726 - 7727, Update the UI test around panelB.tap() and the terminal picker assertion to synchronize with real UI state: wait for a value predicate confirming MobileSimulatorPicker has changed to “iPhone B” before asserting it, then wait until the workspace chrome is ready and perform a deadline-bounded poll confirming the picker’s exists predicate remains false. Ensure both assertions only run after their corresponding state transitions are complete.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 `@ios/cmuxUITests/cmuxUITests.swift`:
- Around line 7726-7727: Update the UI test around panelB.tap() and the terminal
picker assertion to synchronize with real UI state: wait for a value predicate
confirming MobileSimulatorPicker has changed to “iPhone B” before asserting it,
then wait until the workspace chrome is ready and perform a deadline-bounded
poll confirming the picker’s exists predicate remains false. Ensure both
assertions only run after their corresponding state transitions are complete.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: ef133c6f-b73f-48d4-97a5-ef539172bc8a
📒 Files selected for processing (3)
Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/CMUXMobileAppView.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/SimulatorDiscoverabilityPreviewView.swiftios/cmuxUITests/cmuxUITests.swift
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit ee2df25. Configure here.
| ) | ||
|
|
||
| #expect(active.activePanelID == "sim-b") | ||
| #expect(active.targetPanelID == nil) |
There was a problem hiding this comment.
Test/implementation target mismatch
Medium Severity
The renamed test now expects targetPanelID to be nil when a Simulator panel is already active, but SimulatorPickerMenuValue.targetPanelID still returns activePanelID ?? rows.first?.id. That assertion fails, and SimulatorPickerControl stays enabled for re-taps that re-activate an already streaming panel back to .starting.
Reviewed by Cursor Bugbot for commit ee2df25. Configure here.
1 participant
Summary
Testing
./scripts/reload.sh --tag simvisIssues
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Note
Medium Risk
Changes touch iOS Simulator stream RPC ordering, shared store ownership across SwiftUI, and process-lifetime terminal init—important paths but heavily tested and scoped to mobile shell/simulator UX.
Overview
iOS adds a one-tap Mac Simulators control in workspace chrome when the host supports streaming and the workspace has panels; multi-panel setups still use the terminal picker. Simulator selection and RPC start/stop now go through the composite-owned
MobileSimulatorStreamStoreand a newMobileSimulatorStreamSelectionCoordinatorso rapid switches drain to the latest intent instead of overlapping Mac RPCs. Reactivating a panel keeps locked, stalled, and disconnected overlays honest.App structure introduces
MobileShellUISession/CMUXMobileAppSessionViewso SwiftUI holds one graph (browser stores, composite-owned simulator store, startup coordinator). Ghostty initializes once viaGhosttyRuntimeOwnerat composition root, with a retry UI on failure and app-owned iOS config loading.macOS only optimizes Simulator device-picker label building (linear duplicate-name detection). Config tests assert missing
surfaceTabBardefaults do not add a Simulator tab button; explicit config stays authoritative.Tests add deterministic Simulator discoverability fixtures, coordinator/store coverage, and hosted UI tests for picker visibility, stream edge states, and renderer retry.
Reviewed by Cursor Bugbot for commit f7097c9. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Improves Simulator discoverability on iOS with a one‑tap control and unifies stream selection/lifecycle to keep starts/stops ordered and reliable. Terminal rendering is now process‑lifetime with retry, iOS loads Ghostty config from an app‑owned path, and macOS keeps the default tab bar without a Simulator button.
New Features
MobileSimulatorStreamStorewithselectMobileSimulatorStream(...)/clearMobileSimulatorStreamSelection(...);MobileSimulatorStreamSelectionCoordinatordrains cross‑panel transitions in order.SimulatorPickerMenuValue), correct accessibility, and a deterministic fixture viaSimulatorDiscoverabilityPreviewView(fromCmuxMobileShellDebugSupport).MobileShellUISessionandCMUXMobileAppSessionView; terminal uses process‑lifetimeGhosttyRuntimeOwnerwith a retry UI; iOS loads Ghostty config from an app‑owned path.Bug Fixes
Written for commit ee2df25. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes
Tests