Repository navigation
iOS: mobile browser panes P1 (WKWebView surface) - #5652
Conversation
…evice registry Render the merged #5626 device registry as a hierarchical tree: each registered device (Mac/host) expands to its cmux app instances (tags), and a tag expands to that build's workspaces; tapping a workspace opens it via the existing path. Surfaces the registry list to the UI (DeviceRegistryRefreshing.listDevices), adds a RegistryDevice/RegistryAppInstance value model, store.registryDevices + loadRegistryDevices + connectToRegistryInstance (connect-on-tap a non-connected tag via its routes), and a DeviceTreeView reachable from Settings. Keeps the flat workspace list and the multi-Mac switcher as the fallback paths. Online state: the connected device shows live macConnectionStatus; others show registry last-seen (best-effort, no per-host ping yet; the attach ticket carries no tag, so per-tag liveness is a TODO). Expansion persists via @AppStorage. Localized en+ja. Pure decode + expansion-codec tests. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… (autoreview P1s) P1-1: loadRegistryDevices now captures the requesting user id and discards a result that lands after a sign-out + different-user sign-in, so a slow registry load can't leak a previous user's team devices into the new user's tree (mirrors loadPairedMacs's user guard). P1-2: attribute live workspaces to the ONE instance whose route matches the live connection (instanceMatchesActiveRoute), not every tag on the connected device. A multi-tag Mac now shows workspaces only under the connected build; the other tags offer Connect instead of mirroring the wrong build's workspaces, so a workspace can no longer be opened under the wrong tag. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…(autoreview) P1: connectToRegistryInstance now captures the previously-active Mac and, when the destructive connect fails to land on the target route, reconnects it (mirrors switchToMac). Tapping a stale/offline registry tag no longer drops a healthy live session; the user is left where they were. P2: add store.deviceTreeDevices, which honors the documented best-effort fallback: the registry list when loaded, otherwise the locally paired Macs synthesized into the same device→instance shape. The tree now sources from it and loads paired Macs first, so the Devices sheet stays usable (and connectable) during a registry outage instead of showing the empty state. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…itch (autoreview P1) The previous rollback excluded the tapped device id (copied from switchToMac), which is wrong here: a Mac runs multiple tagged builds, so tapping another tag on the currently-connected device must still be able to reconnect that device's active route when the new tag is stale/offline. Capture the active paired Mac regardless of device id so a same-device tag-switch failure restores the live session instead of stranding the user disconnected. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…eview P1) listDevices() now returns a 3-way outcome (ok / authRejected / transientFailure) instead of an optional, so the store can distinguish a transient blip (keep the tree) from a 401/403 auth/scope rejection (clear it). The registry is team-scoped, so a token/scope change must not leave a previous scope's team-device names/tags/ routes visible; on authRejected the store clears registryDevices and the tree falls back to local paired Macs. Transient failures (5xx, network, malformed body) keep the current tree to avoid blip-blanking. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…anual-ticket active device (autoreview P2) P2 (sheet stack): move the device tree to a top-level sheet on the workspace list (a Devices toolbar button) instead of nesting it under the Settings sheet, so selecting a workspace dismisses straight back to the workspace shell and reveals the opened workspace rather than leaving Settings covering it. Removes the duplicate Settings 'Devices' entry. P2 (manual-ticket active): connectedMacDeviceID now falls back to the active paired Mac's real device id when the live ticket is a synthetic manual one (host without mobile.attach_ticket.create). The registry connect path persists the real device as active, so the tree now marks it connected and shows its live workspaces instead of hiding them. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Add a phone-local WKWebView browser pane as a sibling of the terminal
surface in the iOS companion app. New `CmuxMobileBrowser` package owns
the browser surface state, URL resolver, store, and the WKWebView host;
`CmuxMobileShellUI` presents it from the same workspace toolbar menu that
creates terminals ("New Browser"), and a close action returns to the
terminal.
Browser state lives in a dedicated workspace-keyed `BrowserSurfaceStore`
injected from the app root, not in `MobileShellComposite`, because a
browser has no Mac-side counterpart and must survive workspace.updated
re-syncs (unlike terminals).
P1 ships: address bar, navigate, back/forward/reload/stop, page title,
determinate loading progress, default persistent WKWebsiteDataStore.
P2 (cookie/localStorage sync with the Mac) and P3 (passkeys) are
deferred; see plans/feat-ios-mobile-browser/DESIGN.md.
Co-Authored-By: Claude Opus 4.8 <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:
📝 WalkthroughWalkthroughAdds an in-app mobile browser package with observable per-pane state and per-workspace store, omnibox URL resolver, SwiftUI pane and WKWebView bridge, tests, package/target wiring, app-root browser store injection, workspace UI integration, ATS web-content exception, localization, and a registry-backed device tree with parsing, models, UI, persistence, and tests. ChangesMobile Browser Feature
Device Registry & Device Tree
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (5 errors, 1 warning)
✅ Passed checks (15 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 |
![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: dd4ac6406a
ℹ️ 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".
| func applyPendingWork() { | ||
| guard let webView else { return } | ||
| if let url = state.consumeLoadRequest() { | ||
| webView.load(URLRequest(url: url)) | ||
| } | ||
| if let command = state.consumeCommand() { | ||
| run(command, on: webView) |
There was a problem hiding this comment.
When the user taps Back/Forward/Reload/Stop, or submits an already-schemed URL such as https://example.com, the action only changes pendingCommand/loadRequest; those fields are not read by any SwiftUI body, so Observation does not invalidate the representable and updateUIView is not called to consume this work. In those scenarios no other observed state changes, so the WKWebView never receives the navigation command/load request.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Dismissing as a false positive: pendingCommand and loadRequest ARE read inside the representable's updateUIView (applyPendingWork -> consumeLoadRequest/consumeCommand), and SwiftUI's Observation tracking covers representable update methods the same way it covers body, so setting either property invalidates the representable and updateUIView runs to consume the work. This is the same pattern the terminal's GhosttySurfaceRepresentable uses, and BrowserSurfaceState's doc comment documents the contract. Back/Forward/Reload and schemed-URL loads were exercised in dogfood. If beta dogfood shows otherwise we will revisit.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Greptile SummaryAdds a phone-local WKWebView browser pane to the iOS companion app as a workspace-level alternative to the terminal surface, wired through the existing toolbar picker menu. A new
Confidence Score: 5/5Safe to merge once dogfood sign-off lands; no regressions introduced to terminal or workspace wiring. All new code is iOS-only, isolated in a new package with 34 passing unit tests covering the pure state. The WKWebView coordinator correctly tears down KVO observations and delegate references in dismantleUIView; the @observable store is owned at app-root lifetime and correctly survives Mac re-syncs. ATS exception is properly scoped to WKWebView web content only. Localization is complete (en + ja) for all new strings. The open items from prior review threads are style-level concerns that do not affect correctness. No files require special attention beyond the open threads on MobileBrowserPane.swift (ownership annotation) and MobileBrowserView.swift (error surfacing). Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant MobileBrowserPane
participant BrowserSurfaceState
participant Coordinator
participant WKWebView
User->>MobileBrowserPane: tap New Browser
MobileBrowserPane->>BrowserSurfaceStore: openBrowser(workspaceID)
BrowserSurfaceStore-->>MobileBrowserPane: BrowserSurfaceState
MobileBrowserPane->>MobileBrowserView: init(state:)
MobileBrowserView->>WKWebView: makeUIView
MobileBrowserView->>Coordinator: attach(webView)
Coordinator->>WKWebView: observe KVO properties
Coordinator->>WKWebView: load(URLRequest)
WKWebView-->>Coordinator: didStartProvisionalNavigation
Coordinator->>BrowserSurfaceState: navigationDidStart()
BrowserSurfaceState-->>MobileBrowserPane: "isLoading=true"
WKWebView-->>Coordinator: KVO estimatedProgress
Coordinator->>BrowserSurfaceState: estimatedProgress update
WKWebView-->>Coordinator: didFinish
Coordinator->>BrowserSurfaceState: navigationDidFinish()
User->>MobileBrowserPane: tap Close
MobileBrowserPane->>BrowserSurfaceStore: closeBrowser(workspaceID)
BrowserSurfaceStore-->>WorkspaceDetailView: "activeBrowser=nil"
MobileBrowserView->>Coordinator: dismantleUIView detach()
Coordinator->>WKWebView: invalidate observations
Reviews (6): Last reviewed commit: "Merge remote-tracking branch 'origin/mai..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| public struct ID: RawRepresentable, Hashable, Sendable { | ||
| /// The backing identifier string. | ||
| public var rawValue: String |
There was a problem hiding this comment.
BrowserSurfaceStore keys surfacesByWorkspace by workspace.id.rawValue (a String), and BrowserSurfaceState.id is itself stored as a let constant — but ID.rawValue being var means callers can mutate an ID value after it has been hashed into a Set or used as a Dictionary key, producing silent lookup failures. An identifier type should be immutable at the value level.
| public struct ID: RawRepresentable, Hashable, Sendable { | |
| /// The backing identifier string. | |
| public var rawValue: String | |
| public struct ID: RawRepresentable, Hashable, Sendable { | |
| /// The backing identifier string. | |
| public let rawValue: String |
Rule Used: Flag Swift fixes that patch symptoms while leaving... (source)
| private func failNavigation(with error: any Error) { | ||
| // A cancelled load reports `NSURLErrorCancelled`. This is not a | ||
| // failure to surface; it happens on a user stop AND when a new | ||
| // navigation replaces an in-flight one. Mirror the web view's real | ||
| // `isLoading` rather than forcing `false`, so the chrome stays in the | ||
| // loading state when a replacement navigation is still in flight. | ||
| let nsError = error as NSError | ||
| if nsError.domain == NSURLErrorDomain, nsError.code == NSURLErrorCancelled { | ||
| state.isLoading = webView?.isLoading ?? false | ||
| if !state.isLoading { state.estimatedProgress = 0 } | ||
| return | ||
| } | ||
| state.navigationDidFail(message: error.localizedDescription) | ||
| } |
There was a problem hiding this comment.
failNavigation calls state.navigationDidFail(message:) which populates lastErrorMessage, but MobileBrowserPane.body has no UI path that reads or displays that field — the progress bar simply disappears and the page stays blank. A user who navigates to an offline site or mistyped domain gets no feedback. An error banner, inline message, or at minimum a reload button hint under the chrome bar should consume lastErrorMessage in MobileBrowserPane.
| /// The browser surface state this pane drives and reflects. | ||
| @State private var state: BrowserSurfaceState |
There was a problem hiding this comment.
@State is the SwiftUI idiom for values a view creates and owns. BrowserSurfaceState is owned by BrowserSurfaceStore and passed in from WorkspaceDetailView — the view doesn't create it. The correct SwiftUI 6 property wrapper for an externally-provided @Observable reference that also needs bindings (here, $state.addressText) is @Bindable. Using @State silently ignores any replacement object passed from the parent if the .id() modifier does not change, and it misleads future readers about ownership. Since .id(browser.id.rawValue) does guard against surface replacement, this is not currently broken, but it violates the intended @Observable ownership contract.
| /// The browser surface state this pane drives and reflects. | |
| @State private var state: BrowserSurfaceState | |
| /// The browser surface state this pane drives and reflects. | |
| @Bindable private var state: BrowserSurfaceState |
Rule Used: Flag SwiftUI changes that can cause stale state, b... (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| public init(state: BrowserSurfaceState, onClose: @escaping () -> Void) { | ||
| _state = State(initialValue: state) | ||
| self.onClose = onClose | ||
| } |
There was a problem hiding this comment.
@Bindable, the stored property is no longer a State wrapper, so the _state = State(initialValue: state) initializer form is replaced with a direct assignment.
| public init(state: BrowserSurfaceState, onClose: @escaping () -> Void) { | |
| _state = State(initialValue: state) | |
| self.onClose = onClose | |
| } | |
| public init(state: BrowserSurfaceState, onClose: @escaping () -> Void) { | |
| self.state = state | |
| self.onClose = onClose | |
| } |
Rule Used: Flag SwiftUI changes that can cause stale state, b... (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
![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/CmuxMobileBrowser/Sources/CmuxMobileBrowser/MobileBrowserView.swift`:
- Around line 193-206: In failNavigation(with:) replace the raw
error.localizedDescription passed to state.navigationDidFail with a sanitized,
user-friendly mapping for common URLError codes (handle
NSURLErrorNotConnectedToInternet, NSURLErrorCannotFindHost, NSURLErrorTimedOut,
and a default generic message), using the NSError from the incoming error to
switch on domain/code; keep the existing NSURLErrorCancelled early-return
behavior and only send the mapped messages to state.navigationDidFail(message:),
avoiding inclusion of vendor domains/codes in user-facing text.
🪄 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: 25eae0ab-15db-4f8b-96e7-e0c8fe2941f6
📒 Files selected for processing (15)
Packages/CmuxMobileBrowser/Package.swiftPackages/CmuxMobileBrowser/Sources/CmuxMobileBrowser/BrowserSurfaceState.swiftPackages/CmuxMobileBrowser/Sources/CmuxMobileBrowser/BrowserSurfaceStore.swiftPackages/CmuxMobileBrowser/Sources/CmuxMobileBrowser/BrowserURLResolver.swiftPackages/CmuxMobileBrowser/Sources/CmuxMobileBrowser/MobileBrowserPane.swiftPackages/CmuxMobileBrowser/Sources/CmuxMobileBrowser/MobileBrowserView.swiftPackages/CmuxMobileBrowser/Tests/CmuxMobileBrowserTests/BrowserSurfaceStateTests.swiftPackages/CmuxMobileBrowser/Tests/CmuxMobileBrowserTests/BrowserSurfaceStoreTests.swiftPackages/CmuxMobileBrowser/Tests/CmuxMobileBrowserTests/BrowserURLResolverTests.swiftPackages/CmuxMobileShellUI/Package.swiftPackages/CmuxMobileShellUI/Sources/CmuxMobileShellUI/CMUXMobileAppView.swiftPackages/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceDetailView.swiftios/Config/Info.plistios/cmux/Resources/Localizable.xcstringsios/cmuxPackage/Package.swift
| private func failNavigation(with error: any Error) { | ||
| // A cancelled load reports `NSURLErrorCancelled`. This is not a | ||
| // failure to surface; it happens on a user stop AND when a new | ||
| // navigation replaces an in-flight one. Mirror the web view's real | ||
| // `isLoading` rather than forcing `false`, so the chrome stays in the | ||
| // loading state when a replacement navigation is still in flight. | ||
| let nsError = error as NSError | ||
| if nsError.domain == NSURLErrorDomain, nsError.code == NSURLErrorCancelled { | ||
| state.isLoading = webView?.isLoading ?? false | ||
| if !state.isLoading { state.estimatedProgress = 0 } | ||
| return | ||
| } | ||
| state.navigationDidFail(message: error.localizedDescription) | ||
| } |
There was a problem hiding this comment.
Consider sanitizing navigation error messages.
error.localizedDescription can expose URLError internal codes and domains (e.g., "(NSURLErrorDomain error -999)"). As per coding guidelines, user-facing browser errors should use "cmux/product terms" and avoid raw vendor error details.
For P1 this is acceptable, but consider mapping common URLError cases to user-friendly messages with clear next actions:
NSURLErrorNotConnectedToInternet→ "No internet connection. Check your network settings."NSURLErrorCannotFindHost→ "Cannot reach this site. Check the address and try again."NSURLErrorTimedOut→ "The site took too long to respond. Try again."
🤖 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/CmuxMobileBrowser/Sources/CmuxMobileBrowser/MobileBrowserView.swift`
around lines 193 - 206, In failNavigation(with:) replace the raw
error.localizedDescription passed to state.navigationDidFail with a sanitized,
user-friendly mapping for common URLError codes (handle
NSURLErrorNotConnectedToInternet, NSURLErrorCannotFindHost, NSURLErrorTimedOut,
and a default generic message), using the NSError from the incoming error to
switch on domain/code; keep the existing NSURLErrorCancelled early-return
behavior and only send the mapped messages to state.navigationDidFail(message:),
avoiding inclusion of vendor domains/codes in user-facing text.
Source: Coding guidelines
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4afe97d6e1
ℹ️ 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".
| VStack(spacing: 0) { | ||
| chromeBar | ||
| progressLine | ||
| MobileBrowserView(state: state) |
There was a problem hiding this comment.
When a load fails (for example offline, DNS failure, or a local dev server refusing connection), MobileBrowserView.failNavigation records state.lastErrorMessage, but this pane never reads or renders that value, so the user just sees the stale/blank web view after loading stops with no indication of what happened. Since lastErrorMessage is documented as user-facing state, add an error banner/overlay or otherwise surface it from the pane.
Useful? React with 👍 / 👎.
# Conflicts: # .github/swift-file-length-budget.tsv # ios/cmux/Resources/Localizable.xcstrings
# Conflicts: # Packages/CmuxMobileShellUI/Sources/CmuxMobileShellUI/CMUXMobileAppView.swift # ios/cmux/Resources/Localizable.xcstrings
…ng current Mirrors the .ok path's account-switch guard: a stale 401 from a signed-out session that lands after a different user signed in no longer blanks the new user's device tree. Addresses the Greptile P1 on the PR. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
# Conflicts: # ios/cmux/Resources/Localizable.xcstrings
# Conflicts: # ios/cmux/Resources/Localizable.xcstrings
…ios-mobile-browser # Conflicts: # ios/cmux/Resources/Localizable.xcstrings
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ios-mobile-browser
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fba6d256ef
ℹ️ 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".
| let tag: String | ||
| let routes: [CmxAttachRoute] | ||
|
|
||
| var hasReachableRoute: Bool { !routes.isEmpty } |
There was a problem hiding this comment.
When a registry instance contains only routes the current phone cannot use for this connect path, such as a valid .iroh peer or .websocket URL route, this returns true just because the array is non-empty, so the tree renders Connect/Connect to view. The tap then reaches connectToRegistryInstance, which immediately returns because firstReconnectHostPortRoute only accepts supported host/port routes, leaving the button as a no-op. Gate this affordance on the same supported host/port selection or surface it as unreachable.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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/CmuxMobileShell/Sources/CmuxMobileShell/DeviceRegistryService.swift`:
- Around line 237-238: In DeviceRegistryService (where you build the registry
entry using instance.tag) you currently test a trimmed value but persist
instance.tag untrimmed; change this to store the trimmed tag instead. Compute a
trimmedTag = instance.tag?.trimmingCharacters(in: .whitespacesAndNewlines), then
set tag to trimmedTag?.isEmpty == false ? trimmedTag! : "default" (or safely
unwrap/guard) so the stored tag is the trimmed string or "default". Update the
assignment that references instance.tag to use trimmedTag to ensure consistent
tag identity.
In `@Packages/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift`:
- Around line 1171-1173: Current guard that checks isSignedIn and
identityProvider?.currentUserID == requestingUserID should not clear
registryDevices on a stale response; remove the assignment registryDevices = []
inside that guard and simply return early when the response is stale. Locate the
guard around the response handler (the block referencing isSignedIn,
identityProvider?.currentUserID and requestingUserID) and delete the line that
sets registryDevices = [], leaving only the return so late/stale completions
don't wipe out a newer user's populated registryDevices.
In `@Packages/CmuxMobileShellUI/Sources/CmuxMobileShellUI/DeviceTreeView.swift`:
- Around line 100-102: The comparison between device.deviceId and
store.connectedMacDeviceID is currently case-sensitive; normalize both sides
(e.g., lowercased() or using a case-insensitive compare) before deriving
isConnectedDevice so casing mismatches don't mark the connected device as
disconnected—update the comparison at the isConnectedDevice assignment and the
analogous comparison around lines 148-149 (where device.deviceId is compared
again) to use the same normalized/case-insensitive logic, referencing the
device.deviceId and store.connectedMacDeviceID identifiers.
- Around line 34-36: The computed property expansion currently re-parses
expandedStorage on every access (causing repeated full-string parsing in hot
paths like isExpanded); replace it with a cached parsed instance (e.g., a
private stored/lazy variable such as cachedExpansion: DeviceTreeExpansionStore)
and ensure you invalidate/rebuild that cache only when expandedStorage changes
(or compute lazily once and update in the expandedStorage setter/update path).
Update references to expansion (used by isExpanded and related checks) to use
the cachedExpansion to avoid repeated parsing while preserving correctness when
storage mutates.
In `@Packages/CmuxMobileSupport/Sources/CmuxMobileSupport/L10n.swift`:
- Around line 43-56: Rename the confusingly named function
terminalCountWorkspaces(_:) to workspaceCount(_:) and update all call sites
(e.g., usages in DeviceTreeRows.swift) to the new name; specifically change the
public static func terminalCountWorkspaces(_ count: Int) -> String to public
static func workspaceCount(_ count: Int) -> String and preserve the
implementation and localization keys
(string("mobile.deviceTree.workspaceCountFormat.one"/"other", ...)) so behavior
is unchanged while the API matches the existing <thing>Count(_:) naming pattern.
🪄 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: 7ecf54cc-f945-4d8f-b3a9-c42e917818e7
⛔ Files ignored due to path filters (1)
.github/swift-file-length-budget.tsvis excluded by!**/*.tsv
📒 Files selected for processing (12)
Packages/CmuxMobileShell/Sources/CmuxMobileShell/DeviceRegistryRefreshing.swiftPackages/CmuxMobileShell/Sources/CmuxMobileShell/DeviceRegistryService.swiftPackages/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swiftPackages/CmuxMobileShell/Tests/CmuxMobileShellTests/DeviceRegistryListParsingTests.swiftPackages/CmuxMobileShellModel/Sources/CmuxMobileShellModel/RegistryDevice.swiftPackages/CmuxMobileShellUI/Sources/CmuxMobileShellUI/DeviceTreeExpansionStore.swiftPackages/CmuxMobileShellUI/Sources/CmuxMobileShellUI/DeviceTreeRows.swiftPackages/CmuxMobileShellUI/Sources/CmuxMobileShellUI/DeviceTreeView.swiftPackages/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceListView.swiftPackages/CmuxMobileShellUI/Tests/CmuxMobileShellUITests/DeviceTreeExpansionStoreTests.swiftPackages/CmuxMobileSupport/Sources/CmuxMobileSupport/L10n.swiftios/cmux/Resources/Localizable.xcstrings
| tag: instance.tag?.trimmingCharacters(in: .whitespacesAndNewlines).isEmpty == false | ||
| ? instance.tag! : "default", |
There was a problem hiding this comment.
Store the trimmed instance tag, not the raw string.
On Line 237-238, you validate tag using a trimmed value but persist instance.tag! untrimmed. Tags like " stable " survive with whitespace and can break tag identity/comparisons downstream.
Suggested fix
- let instances = (device.instances ?? []).map { instance in
+ let instances = (device.instances ?? []).map { instance in
+ let trimmedTag = instance.tag?.trimmingCharacters(in: .whitespacesAndNewlines) ?? ""
RegistryAppInstance(
- tag: instance.tag?.trimmingCharacters(in: .whitespacesAndNewlines).isEmpty == false
- ? instance.tag! : "default",
+ tag: trimmedTag.isEmpty ? "default" : trimmedTag,
routes: (instance.routes ?? []).compactMap(\.value),
lastSeenAt: Self.parseTimestamp(instance.lastSeenAt)
)
}🤖 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/CmuxMobileShell/Sources/CmuxMobileShell/DeviceRegistryService.swift`
around lines 237 - 238, In DeviceRegistryService (where you build the registry
entry using instance.tag) you currently test a trimmed value but persist
instance.tag untrimmed; change this to store the trimmed tag instead. Compute a
trimmedTag = instance.tag?.trimmingCharacters(in: .whitespacesAndNewlines), then
set tag to trimmedTag?.isEmpty == false ? trimmedTag! : "default" (or safely
unwrap/guard) so the stored tag is the trimmed string or "default". Update the
assignment that references instance.tag to use trimmedTag to ensure consistent
tag identity.
| guard isSignedIn, identityProvider?.currentUserID == requestingUserID else { | ||
| registryDevices = [] | ||
| return |
There was a problem hiding this comment.
Do not clear registryDevices when the response belongs to a stale user context.
On Line 1171-1173, a stale completion (old user/session) sets registryDevices = []. If a newer load already populated the current user’s tree, this late stale task can erase it.
Suggested fix
- guard isSignedIn, identityProvider?.currentUserID == requestingUserID else {
- registryDevices = []
- return
- }
+ guard isSignedIn, identityProvider?.currentUserID == requestingUserID else {
+ return
+ }🤖 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/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift`
around lines 1171 - 1173, Current guard that checks isSignedIn and
identityProvider?.currentUserID == requestingUserID should not clear
registryDevices on a stale response; remove the assignment registryDevices = []
inside that guard and simply return early when the response is stale. Locate the
guard around the response handler (the block referencing isSignedIn,
identityProvider?.currentUserID and requestingUserID) and delete the line that
sets registryDevices = [], leaving only the return so late/stale completions
don't wipe out a newer user's populated registryDevices.
| private var expansion: DeviceTreeExpansionStore { | ||
| DeviceTreeExpansionStore(storage: expandedStorage) | ||
| } |
There was a problem hiding this comment.
Avoid re-parsing expansion storage on every row lookup.
Line 34 rebuilds DeviceTreeExpansionStore from the full storage string each time expansion is read. In this view, isExpanded is called repeatedly across devices/instances, so renders do repeated full-string parsing work.
Suggested fix
- private var expansion: DeviceTreeExpansionStore {
- DeviceTreeExpansionStore(storage: expandedStorage)
- }
+ `@State` private var expansion = DeviceTreeExpansionStore(storage: "")
var body: some View {
NavigationStack {
List {
@@
}
+ .onAppear {
+ expansion = DeviceTreeExpansionStore(storage: expandedStorage)
+ }
+ .onChange(of: expandedStorage) { newValue in
+ expansion = DeviceTreeExpansionStore(storage: newValue)
+ }
@@
private func setExpanded(_ id: String, _ expanded: Bool) {
- var store = expansion
- store.setExpanded(id, expanded)
- expandedStorage = store.storage
+ expansion.setExpanded(id, expanded)
+ expandedStorage = expansion.storage
}As per coding guidelines, this is a production scalable-data path and repeated full-collection rescans in hot paths should be flagged.
Also applies to: 122-123, 126-127, 168-169, 173-174, 253-256
🤖 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/CmuxMobileShellUI/Sources/CmuxMobileShellUI/DeviceTreeView.swift`
around lines 34 - 36, The computed property expansion currently re-parses
expandedStorage on every access (causing repeated full-string parsing in hot
paths like isExpanded); replace it with a cached parsed instance (e.g., a
private stored/lazy variable such as cachedExpansion: DeviceTreeExpansionStore)
and ensure you invalidate/rebuild that cache only when expandedStorage changes
(or compute lazily once and update in the expandedStorage setter/update path).
Update references to expansion (used by isExpanded and related checks) to use
the cachedExpansion to avoid repeated parsing while preserving correctness when
storage mutates.
Source: Coding guidelines
| let connectedID = store.connectedMacDeviceID | ||
| let isConnectedDevice = device.deviceId == connectedID | ||
| // Live status only exists for the connected device; others are described |
There was a problem hiding this comment.
Normalize device-id comparison before deriving connected state.
Line 101 uses case-sensitive equality, but upstream registry matching is case-insensitive. A casing mismatch will mark the connected device as disconnected, which suppresses active-instance attribution and workspace display for that device.
Suggested fix
- let connectedID = store.connectedMacDeviceID
- let isConnectedDevice = device.deviceId == connectedID
+ let connectedID = store.connectedMacDeviceID?.lowercased()
+ let isConnectedDevice = device.deviceId.lowercased() == connectedIDAlso applies to: 148-149
🤖 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/CmuxMobileShellUI/Sources/CmuxMobileShellUI/DeviceTreeView.swift`
around lines 100 - 102, The comparison between device.deviceId and
store.connectedMacDeviceID is currently case-sensitive; normalize both sides
(e.g., lowercased() or using a case-insensitive compare) before deriving
isConnectedDevice so casing mismatches don't mark the connected device as
disconnected—update the comparison at the isConnectedDevice assignment and the
analogous comparison around lines 148-149 (where device.deviceId is compared
again) to use the same normalized/case-insensitive logic, referencing the
device.deviceId and store.connectedMacDeviceID identifiers.
| /// A localized "N workspaces" count label with singular/plural handling, for | ||
| /// the device tree's per-build workspace summary. | ||
| /// | ||
| /// - Parameter count: The number of workspaces. | ||
| /// - Returns: The localized count phrase. | ||
| public static func terminalCountWorkspaces(_ count: Int) -> String { | ||
| if count == 1 { | ||
| return string("mobile.deviceTree.workspaceCountFormat.one", defaultValue: "1 workspace") | ||
| } | ||
| return String( | ||
| format: string("mobile.deviceTree.workspaceCountFormat.other", defaultValue: "%d workspaces"), | ||
| count | ||
| ) | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Consider renaming to workspaceCount(_:) for API consistency.
The method name terminalCountWorkspaces contains "terminal" but formats workspace counts, creating confusion. The established pattern in this file is <thing>Count(_:) (e.g., terminalCount) and <thing>Name(index:) (e.g., workspaceName, terminalName). Renaming to workspaceCount(_:) would improve discoverability and align with the existing API surface.
♻️ Proposed rename
- /// A localized "N workspaces" count label with singular/plural handling, for
- /// the device tree's per-build workspace summary.
+ /// A localized "N workspaces" count label with singular/plural handling.
///
/// - Parameter count: The number of workspaces.
/// - Returns: The localized count phrase.
- public static func terminalCountWorkspaces(_ count: Int) -> String {
+ public static func workspaceCount(_ count: Int) -> String {
if count == 1 {Then update the call site in DeviceTreeRows.swift:
- return L10n.terminalCountWorkspaces(instance.workspaceCount)
+ return L10n.workspaceCount(instance.workspaceCount)🤖 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/CmuxMobileSupport/Sources/CmuxMobileSupport/L10n.swift` around lines
43 - 56, Rename the confusingly named function terminalCountWorkspaces(_:) to
workspaceCount(_:) and update all call sites (e.g., usages in
DeviceTreeRows.swift) to the new name; specifically change the public static
func terminalCountWorkspaces(_ count: Int) -> String to public static func
workspaceCount(_ count: Int) -> String and preserve the implementation and
localization keys (string("mobile.deviceTree.workspaceCountFormat.one"/"other",
...)) so behavior is unchanged while the API matches the existing
<thing>Count(_:) naming pattern.
# Conflicts: # ios/cmux/Resources/Localizable.xcstrings
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes 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 3efcb18. Configure here.
| browserContent(browser) | ||
| } else { | ||
| detailContent() | ||
| } |
There was a problem hiding this comment.
Browser pane blocks push terminal
Medium Severity
When a workspace has an open browser pane, WorkspaceDetailView always shows the browser and never the terminal, even after the shell store selects a terminal. Agent notification taps only update selectedWorkspaceID / selectedTerminalID and do not close the browser the way the terminal picker does, so the user can remain on the browser instead of the notified terminal. Foreground notification suppression can also treat the terminal as already visible while the browser is still on screen.
Reviewed by Cursor Bugbot for commit 3efcb18. Configure here.
This branch was successfully deployed
1 participant
Adds a phone-local in-app browser pane to the cmux iOS companion app, as a sibling of the terminal surface. Today mobile has only a terminal surface; this is P1 of "Mobile browser panes + passkeys."
Design doc (in cmuxterm-hq, not this repo):
plans/feat-ios-mobile-browser/DESIGN.md.What P1 ships
A real, usable WKWebView browser pane reachable from the same workspace toolbar menu that creates terminals. Open the terminal picker menu (top-right), tap New Browser ("globe"), and the detail view flips from the terminal to a browser pane with an address bar, back/forward, reload/stop, page title, and a determinate loading line. Picking a terminal in that same menu (or New Terminal) closes the browser and returns to the terminal. A close button on the browser chrome does the same.
New
CmuxMobileBrowserpackage:BrowserURLResolver(pure): address-bar input -> URL. Full http(s) URLs pass through; bare hosts ->https://, except localhost/loopback/private-LAN/IPv6-loopback ->http://so local dev servers open; non-URL text -> search; non-web schemes -> search. Query separators are percent-escaped.BrowserSurfaceState(@Observable @MainActor): per-pane URL/title/loading/progress/can-go-back-forward/error, plus one-shot load + nav-command queues consumed by the view.BrowserSurfaceStore(@Observable @MainActor): one optional browser surface per workspace.MobileBrowserView(UIViewRepresentable) +MobileBrowserPane(chrome): the WKWebView host. Loading/title/url/history mirrored viaNSKeyValueObservation+WKNavigationDelegate;WKUIDelegateloadstarget="_blank"links in-place.ios/Config/Info.plist: scopedNSAllowsArbitraryLoadsInWebContentATS exception so the in-app browser can load http/local-dev pages while the app's own API/auth/pairing traffic stays HTTPS-only.Why browser state is separate from the terminal store
A mobile terminal is a mirror of a real Ghostty surface on the paired Mac and is rebuilt on every
workspace.updatedsync. A browser pane has no Mac-side counterpart in P1, so its state lives in a dedicated workspace-keyedBrowserSurfaceStoreinjected from the app root (not inMobileShellComposite), where it survives Mac re-syncs. The affordance is shared (same toolbar menu); only the data model is split, because the two surfaces have genuinely different lifecycles.Package / DAG placement
CmuxMobileBrowseris a sibling ofCmuxMobileTerminal, depending only on the leafCmuxMobileSupport(forL10n).CmuxMobileShellUI(top of the DAG) gains a dependency on it and presents the pane. No cycles. The iOS-only shell packages wire via SwiftPM (cmuxFeature->CmuxMobileShellUI->CmuxMobileBrowser), not pbxproj: only the cross-platformCMUXMobileCoresits incmux.xcodeproj, and the macOS target doesn't reference the shell packages, sonormalize-pbxproj.py/check-pbxproj.shdon't apply here.Tests
34 Swift Testing unit tests for the pure state: URL normalization (http(s)/bare host/localhost-http/loopback/private-LAN/IPv6/search/query-escaping), nav-state + loading transitions, command/load-request queues, and the workspace-scoped store (open/reveal/close/reopen). No WKWebView in tests, no source-shape tests.
Verification
Localization
7 new keys (address placeholder, back/forward/reload/stop/close/new) in
ios/cmux/Resources/Localizable.xcstringswith en + ja.Follow-ups (deferred)
mobileHostHandleRPChandler under the same same-account auth chokepoint as pairing.webcredentialsassociated-domains entitlement + anASAuthorizationControllerbridge (some flows route throughASWebAuthenticationSession).🤖 Generated with Claude Code
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Note
Medium Risk
New WebKit surface and scoped ATS web-content exception increase attack surface for user-chosen URLs; app networking remains HTTPS-only and non-http(s) schemes are blocked in the resolver.
Overview
Introduces a phone-local in-app browser for iOS workspaces: a new
CmuxMobileBrowserSwift package with@Observableper-pane state, a per-workspaceBrowserSurfaceStore(kept outside Mac-synced terminal data), omnibox resolution viaBrowserURLResolver, andWKWebViewhosting inMobileBrowserView/MobileBrowserPane.Shell wiring:
CMUXMobileAppViewowns and injectsBrowserSurfaceStore; on iOS,WorkspaceDetailViewswaps the terminal for the browser when a workspace has an active surface. The terminal picker gains New Browser; choosing a terminal or New Terminal closes the browser and returns to the terminal.Platform:
Info.plistaddsNSAllowsArbitraryLoadsInWebContentso user-navigated pages (HTTP, localhost, private LAN) load in WebKit while app API/auth traffic stays ATS-restricted. Newmobile.browser.*strings (en/ja). Package dependencies updated inCmuxMobileShellUIandios/cmuxPackage.Reviewed by Cursor Bugbot for commit 3efcb18. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Adds a phone-local in-app browser pane (
WKWebView) and a hierarchical device tree (device → tags → workspaces) to iOS. Browse the web next to the terminal and quickly connect to any Mac build to open its workspaces.New Features
BrowserSurfaceStore(per workspace) andBrowserSurfaceState(per pane) injected fromCMUXMobileAppView, surviving Mac re-syncs;MobileBrowserViewmirrorsWKWebView;BrowserURLResolvermaps input (http(s), bare hosts → https with localhost/private-LAN → http, non-URL → search, IPv6 bracketing).GET /api/devices): devices expand to tagged instances, then to workspaces; launched from a Devices button on the workspace list; only the connected build shows live workspaces, tapping another tag connects it first; shows live status or last-seen and persists expansion.Dependencies
CmuxMobileBrowserpackage;CmuxMobileShellUIandios/cmuxPackagedepend on it.Info.plist:NSAllowsArbitraryLoadsInWebContent(WKWebView only) to allow http/local-dev pages.Written for commit 3efcb18. Summary will update on new commits.
Summary by CodeRabbit
New Features
Localization
Chores
Tests