Show iOS terminal loading diagnostics - #7198
lawrencecchen wants to merge 6 commits into
Conversation
|
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 iOS terminal-loading diagnostics overlay, wires it into ChangesTerminal Loading Diagnostics Overlay
Estimated code review effort: 4 (Complex) | ~45 minutes Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (3 errors, 1 warning)
✅ Passed checks (21 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}
Greptile SummaryAdds an iOS terminal loading diagnostics overlay to
Confidence Score: 5/5iOS-only UI change with no auth, data model, or persistence changes; connection refresh reuses existing store APIs and all new user-facing strings are fully localized. The overlay is purely presentational: the 10-second deadline uses structured-concurrency sleep with correct cancellation via .task(id:), SwiftUI state is managed cleanly as value types, the diagnostic Mac-matching logic is scoped to display and does not gate any write path, and all four model states are covered by focused unit tests. No files require special attention. The WorkspaceDetailView+TerminalLoadingDiagnostics.swift pairedMacAliasIDs branching is the most complex logic path, but it is exercised end-to-end through the existing store APIs. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[WorkspaceDetailView body] --> B{Terminal selected?}
B -- Yes --> C[Render terminal]
B -- No --> D{loadingDiagnosticsConnectionStatus}
D -- .connected --> E[TerminalPalette.background + overlay]
E --> F[TerminalLoadingDiagnosticsOverlay]
F --> G{.task id=deadlineTaskID}
G --> H[ContinuousClock.sleep 10s]
H -- Cancelled --> I[Task restarts on status/count/generation change]
H -- Completed + still empty --> J[terminalMetadataTimedOut = true]
F --> K{model.isLoading?}
K -- true --> L[Spinner + Loading terminals...]
K -- false --> M[Terminal icon + No terminals yet]
F --> N[Refresh button]
N --> O[refreshGeneration += 1]
O --> G
N --> P[store.switchToMac + reconnectOrRefresh]
F --> Q[Create Terminal button]
D -- .reconnecting / .unavailable --> R[TerminalDisconnectedOverlay]
%%{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"}}}%%
flowchart TD
A[WorkspaceDetailView body] --> B{Terminal selected?}
B -- Yes --> C[Render terminal]
B -- No --> D{loadingDiagnosticsConnectionStatus}
D -- .connected --> E[TerminalPalette.background + overlay]
E --> F[TerminalLoadingDiagnosticsOverlay]
F --> G{.task id=deadlineTaskID}
G --> H[ContinuousClock.sleep 10s]
H -- Cancelled --> I[Task restarts on status/count/generation change]
H -- Completed + still empty --> J[terminalMetadataTimedOut = true]
F --> K{model.isLoading?}
K -- true --> L[Spinner + Loading terminals...]
K -- false --> M[Terminal icon + No terminals yet]
F --> N[Refresh button]
N --> O[refreshGeneration += 1]
O --> G
N --> P[store.switchToMac + reconnectOrRefresh]
F --> Q[Create Terminal button]
D -- .reconnecting / .unavailable --> R[TerminalDisconnectedOverlay]
Reviews (7): Last reviewed commit: "Add terminal loading retry state" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| value: "\(resolvedMacName) · \(connectionStatus.label)", | ||
| tone: tone(for: connectionStatus) | ||
| ), | ||
| TerminalLoadingDiagnosticsRow( | ||
| id: "terminals", | ||
| label: L10n.string("mobile.terminal.loading.terminals", defaultValue: "Terminals"), | ||
| value: terminalStatusText(count: terminalCount), | ||
| tone: terminalCount > 0 ? .good : .pending | ||
| ), | ||
| TerminalLoadingDiagnosticsRow( | ||
| id: "tailscale", | ||
| label: L10n.string("mobile.terminal.loading.tailscale", defaultValue: "Tailscale"), | ||
| value: tailnetStatusText(tailnetStatus), | ||
| tone: tone(for: tailnetStatus) | ||
| ), | ||
| TerminalLoadingDiagnosticsRow( | ||
| id: "route", | ||
| label: L10n.string("mobile.terminal.loading.route", defaultValue: "Route"), | ||
| value: routeText(activeRoute: activeRoute, storedRouteDescription: storedRouteDescription), | ||
| tone: activeRoute == nil && Self.nonEmpty(storedRouteDescription) == nil ? .warning : .neutral | ||
| ), | ||
| ] | ||
|
|
||
| if let detail = Self.nonEmpty(connectionErrorGuidance) ?? Self.nonEmpty(connectionError) { | ||
| rows.append(TerminalLoadingDiagnosticsRow( | ||
| id: "network", | ||
| label: L10n.string("mobile.terminal.loading.network", defaultValue: "Network"), | ||
| value: detail, | ||
| tone: .warning | ||
| )) | ||
| } | ||
|
|
||
| return Self( | ||
| title: L10n.string("mobile.terminal.loading.title", defaultValue: "Loading terminals"), | ||
| message: String( | ||
| format: L10n.string( | ||
| "mobile.terminal.loading.messageFormat", | ||
| defaultValue: "Waiting for %@ to send terminal metadata for this workspace." | ||
| ), | ||
| resolvedMacName | ||
| ), | ||
| rows: rows | ||
| ) | ||
| } | ||
|
|
||
| private static func nonEmpty(_ value: String?) -> String? { | ||
| let trimmed = value?.trimmingCharacters(in: .whitespacesAndNewlines) | ||
| return trimmed?.isEmpty == false ? trimmed : nil | ||
| } | ||
|
|
||
| private static func terminalStatusText(count: Int) -> String { | ||
| guard count > 0 else { | ||
| return L10n.string("mobile.terminal.loading.terminalsWaiting", defaultValue: "No terminal list yet") | ||
| } | ||
| return L10n.terminalCount(count) | ||
| } | ||
|
|
||
| private static func tailnetStatusText(_ status: TailnetStatus?) -> String { | ||
| switch status { | ||
| case .active: | ||
| return L10n.string("mobile.terminal.loading.tailscale.active", defaultValue: "Active") | ||
| case .inactiveOrNotInstalled: | ||
| return L10n.string("mobile.terminal.loading.tailscale.inactive", defaultValue: "Off or not installed") | ||
| case .unknown: | ||
| return L10n.string("mobile.terminal.loading.tailscale.unknown", defaultValue: "Unknown") | ||
| case nil: | ||
| return L10n.string("mobile.terminal.loading.tailscale.notChecked", defaultValue: "Not checked") | ||
| } | ||
| } | ||
|
|
||
| private static func routeText(activeRoute: CmxAttachRoute?, storedRouteDescription: String?) -> String { | ||
| if let activeRoute { | ||
| return "\(routeKindText(activeRoute.kind)) · \(endpointText(activeRoute.endpoint))" |
There was a problem hiding this comment.
Both the mac-row value ("\(resolvedMacName) · \(connectionStatus.label)", line 45) and the active-route value ("\(routeKindText(activeRoute.kind)) · \(endpointText(activeRoute.endpoint))", line 117) embed the · separator and the name–status ordering directly via string interpolation, so translators can't change the layout or separator for a given locale. This is inconsistent with the stored-route case immediately below, which already uses a proper format key (mobile.terminal.loading.routeStoredFormat → "Saved route · %@") and with the message field (mobile.terminal.loading.messageFormat → "Waiting for %@ to…"). Both of these composite strings need format keys in the catalog alongside entries for every locale the catalog already covers.
Rule Used: Flag production user-facing text that is not fully... (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!
![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: 9ef9630451
ℹ️ 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".
9ef9630 to
5e8564a
Compare
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found and verified against the latest diff
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5e8564af2e
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6c1f8454fc
ℹ️ 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".
| .lineLimit(1) | ||
| .truncationMode(.middle) |
There was a problem hiding this comment.
When row.value contains connectionErrorGuidance, several existing guidance strings are full sentences such as the reachability/Tailscale instructions, so forcing every value to one line with middle truncation hides the actual next steps on an iPhone-width overlay. Consider allowing the network/warning value to wrap or rendering guidance below the label while keeping short route/status rows single-line.
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: 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/CmuxMobileShellUI/Sources/CmuxMobileShellUI/TerminalLoadingDiagnosticsOverlay.swift`:
- Around line 44-52: The localized template formatting in
TerminalLoadingDiagnosticsOverlay uses String(format:) with values returned from
L10n.string, which should be changed to locale-aware formatting. Update the
affected value builders in TerminalLoadingDiagnosticsOverlay (including the
macStatusFormat, messageFormat, routeActiveFormat, and routeStoredFormat usages)
to use String.localizedStringWithFormat instead of String(format:), while
keeping the same labels and resolved arguments. Use the existing formatting
sites in the overlay as the guide and preserve the current localized strings and
argument order handling.
🪄 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: c0e74517-ad8d-4232-a0c8-5f9747b2afb7
📒 Files selected for processing (5)
Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/TerminalLoadingDiagnosticsOverlay.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceDetailView.swiftPackages/iOS/CmuxMobileShellUI/Tests/CmuxMobileShellUITests/TerminalLoadingDiagnosticsModelTests.swiftResources/Localizable.xcstringsios/cmux/Resources/Localizable.xcstrings
There was a problem hiding this comment.
1 issue found across 5 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 337c98e2fa
ℹ️ 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".
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
22407d0 to
db487aa
Compare
7ba7336 to
0e218ae
Compare
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/Tests/CmuxMobileShellUITests/TerminalLoadingDiagnosticsModelTests.swift (1)
55-74: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winNo assertion on
title/messagefor disconnected states.
inactiveTailscaleAndSavedRouteSurfaceNetworkGuidanceexercisesconnectionStatus: .unavailablewithconnectionError/connectionErrorGuidanceset, but only asserts row values/tones — it never checksmodel.title/model.message/model.isLoading. This is exactly the combination that (per the sibling comment onTerminalLoadingDiagnosticsModel.swift) currently falls through to the generic "Loading terminals" copy withisLoading == false. Adding those assertions here would have caught the header/isLoading mismatch for disconnected Macs.🤖 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/Tests/CmuxMobileShellUITests/TerminalLoadingDiagnosticsModelTests.swift` around lines 55 - 74, Add coverage in TerminalLoadingDiagnosticsModelTests by asserting the header state for the disconnected case in inactiveTailscaleAndSavedRouteSurfaceNetworkGuidance, not just the rows. Verify TerminalLoadingDiagnosticsModel’s title, message, and isLoading when connectionStatus is .unavailable with connectionError and connectionErrorGuidance set, so this test catches the fallback-to-loading copy and false isLoading mismatch.
🤖 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/CmuxMobileShellUI/Sources/CmuxMobileShellUI/TerminalLoadingDiagnosticsOverlay.swift`:
- Around line 143-169: The readiness check in updateTerminalMetadataDeadline()
is driven by a fixed sleep instead of an actual metadata/state signal. Replace
the ContinuousClock().sleep(for:) timeout gating with a real cancellation-aware
readiness source from the workspace/session flow, such as a state transition,
async sequence, notification, or callback that indicates terminal metadata has
settled. Keep the existing deadlineTaskID and terminalMetadataTimedOut behavior,
but make the timeout depend on that real signal rather than a magic delay.
In
`@Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceDetailView`+TerminalLoadingDiagnostics.swift:
- Around line 41-45: The refreshLoadingDiagnosticsConnection() method currently
starts an unstored fire-and-forget Task for store.reconnectOrRefresh(), so
repeated calls can overlap reconnect attempts. Update the
WorkspaceDetailView+TerminalLoadingDiagnostics flow to give this work a
meaningful lifecycle by storing the task on the owning view model/store,
cancelling any in-flight task before starting a new one, or otherwise guarding
against duplicate reconnect/refresh calls. Keep the fix localized around
refreshLoadingDiagnosticsConnection() and the reconnectOrRefresh path so only
one refresh operation can run at a time.
---
Outside diff comments:
In
`@Packages/iOS/CmuxMobileShellUI/Tests/CmuxMobileShellUITests/TerminalLoadingDiagnosticsModelTests.swift`:
- Around line 55-74: Add coverage in TerminalLoadingDiagnosticsModelTests by
asserting the header state for the disconnected case in
inactiveTailscaleAndSavedRouteSurfaceNetworkGuidance, not just the rows. Verify
TerminalLoadingDiagnosticsModel’s title, message, and isLoading when
connectionStatus is .unavailable with connectionError and
connectionErrorGuidance set, so this test catches the fallback-to-loading copy
and false isLoading mismatch.
🪄 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: da973560-a757-4830-b7d6-a5d2157b37f9
📒 Files selected for processing (8)
Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/TerminalLoadingDiagnosticsModel.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/TerminalLoadingDiagnosticsOverlay.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/TerminalLoadingDiagnosticsRow.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceDetailView+TerminalLoadingDiagnostics.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceDetailView.swiftPackages/iOS/CmuxMobileShellUI/Tests/CmuxMobileShellUITests/TerminalLoadingDiagnosticsModelTests.swiftResources/Localizable.xcstringsios/cmux/Resources/Localizable.xcstrings
|
|
||
| private var deadlineTaskID: String { | ||
| [ | ||
| workspace.id.rawValue, | ||
| String(workspace.terminals.count), | ||
| connectionStatusKey(effectiveConnectionStatus), | ||
| String(refreshGeneration), | ||
| ].joined(separator: ":") | ||
| } | ||
|
|
||
| private func updateTerminalMetadataDeadline() async { | ||
| terminalMetadataTimedOut = false | ||
| guard effectiveConnectionStatus == .connected, | ||
| workspace.terminals.isEmpty else { | ||
| return | ||
| } | ||
| do { | ||
| try await ContinuousClock().sleep(for: Self.terminalMetadataTimeout) | ||
| } catch { | ||
| return | ||
| } | ||
| guard effectiveConnectionStatus == .connected, | ||
| workspace.terminals.isEmpty else { | ||
| return | ||
| } | ||
| terminalMetadataTimedOut = true | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔵 Trivial | ⚖️ Poor tradeoff
Sleep-based readiness wait for terminal metadata.
updateTerminalMetadataDeadline() uses ContinuousClock().sleep(for:) to decide when to show "no terminals yet" messaging. Per coding guidelines for non-test Swift files, readiness waits should be driven by "a real cancellation-aware scheduler, timer abstraction, async sequence, callback, notification, or state transition" rather than a fixed sleep gating a data-availability decision — even when wrapped in a cancellable .task(id:). Consider deriving the timeout from an actual metadata-fetch-settled signal (e.g., a state transition emitted by the connection/session store) instead of a magic 10s sleep, so this doesn't race with slow/flaky metadata fetches.
As per coding guidelines: "Do not use Task.sleep in production Swift code just because it is inside an async function... retry backoff, keepalive loops, readiness waits, delayed dispatch, and polling must use a real cancellation-aware scheduler, timer abstraction, async sequence, callback, notification, or state transition."
🤖 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/TerminalLoadingDiagnosticsOverlay.swift`
around lines 143 - 169, The readiness check in updateTerminalMetadataDeadline()
is driven by a fixed sleep instead of an actual metadata/state signal. Replace
the ContinuousClock().sleep(for:) timeout gating with a real cancellation-aware
readiness source from the workspace/session flow, such as a state transition,
async sequence, notification, or callback that indicates terminal metadata has
settled. Keep the existing deadlineTaskID and terminalMetadataTimedOut behavior,
but make the timeout depend on that real signal rather than a magic delay.
Source: Coding guidelines
| func refreshLoadingDiagnosticsConnection() { | ||
| Task { | ||
| await store.reconnectOrRefresh() | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔵 Trivial | ⚖️ Poor tradeoff
Untracked fire-and-forget Task for reconnect/refresh.
refreshLoadingDiagnosticsConnection() launches a new, unstored Task on every call (each Refresh tap), with no cancellation of a prior in-flight task. If reconnectOrRefresh() isn't internally idempotent/de-duplicated, repeated taps could trigger overlapping reconnect attempts.
As per coding guidelines: "Flag fire-and-forget Task { ... } work with meaningful lifecycle that is not stored, cancelled, or tied to a caller-owned operation."
Consider storing the task (e.g., on the store/view model) and cancelling/ignoring a new request while one is in flight.
🤖 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`+TerminalLoadingDiagnostics.swift
around lines 41 - 45, The refreshLoadingDiagnosticsConnection() method currently
starts an unstored fire-and-forget Task for store.reconnectOrRefresh(), so
repeated calls can overlap reconnect attempts. Update the
WorkspaceDetailView+TerminalLoadingDiagnostics flow to give this work a
meaningful lifecycle by storing the task on the owning view model/store,
cancelling any in-flight task before starting a new one, or otherwise guarding
against duplicate reconnect/refresh calls. Keep the fix localized around
refreshLoadingDiagnosticsConnection() and the reconnectOrRefresh path so only
one refresh operation can run at a time.
Source: Coding guidelines
adcf168 to
91caa2d
Compare
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 91caa2d. Configure here.
91caa2d to
2858674
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
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)
274-288: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winDuplicate reconnect/switch logic now that a shared helper exists.
The
TerminalDisconnectedOverlayretry closure (lines 278-286) re-implements the exact sameswitchToMac(...)/reconnectOrRefresh()sequence thatWorkspaceDetailView+TerminalLoadingDiagnostics.swift'srefreshLoadingDiagnosticsConnection()now encapsulates and that is wired to the newTerminalLoadingDiagnosticsOverlay's refresh button. Two separate call sites maintaining identical reconnect logic risks drift (e.g., if one is fixed for the fire-and-forget-Task concern above but the other isn't).As per coding guidelines' shared behavior policy: "When a behavior is exposed through multiple entrypoints... implement one shared action/model path and verify every entrypoint that should invoke it. Do not patch one surface while leaving the others with duplicated logic."
♻️ Suggested consolidation
.overlay { // Show a reconnecting/offline state instead of a black terminal. if loadingDiagnosticsConnectionStatus != .connected { TerminalDisconnectedOverlay(status: loadingDiagnosticsConnectionStatus, host: host) { - Task { - if let macDeviceID = workspace.macDeviceID, - !macDeviceID.isEmpty, - await store.switchToMac(macDeviceID: macDeviceID) { - return - } - await store.reconnectOrRefresh() - } + refreshLoadingDiagnosticsConnection() } } }🤖 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 274 - 288, The TerminalDisconnectedOverlay retry closure is duplicating the same reconnect/switch flow already centralized in refreshLoadingDiagnosticsConnection(). Update WorkspaceDetailView to call that shared helper from the overlay action instead of re-implementing the Task with switchToMac(macDeviceID:) and reconnectOrRefresh(), so TerminalDisconnectedOverlay and TerminalLoadingDiagnosticsOverlay both use the same behavior path.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/CmuxMobileShellUI/Sources/CmuxMobileShellUI/TerminalLoadingDiagnosticsModel.swift`:
- Around line 5-43: Mark TerminalLoadingDiagnosticsModel as nonisolated because
it is a pure value type and should not implicitly inherit MainActor isolation
under Swift 6. Update the struct declaration itself, and keep both initializers
in place while ensuring the TerminalLoadingDiagnosticsModelBuilder path and the
direct property initializer remain free of actor affinity so the model can be
constructed off-main without unnecessary hops.
In
`@Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/TerminalLoadingDiagnosticsRow.swift`:
- Around line 2-7: `TerminalLoadingDiagnosticsRow` is a pure value
`Identifiable` model that may implicitly inherit `@MainActor` under Swift 6
isolation. Update the struct declaration itself to be explicitly nonisolated so
it can be passed freely between actor contexts without unnecessary hops. Keep
the change scoped to `TerminalLoadingDiagnosticsRow` and preserve its current
stored properties and protocol conformances.
In
`@Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceDetailView`+TerminalLoadingDiagnostics.swift:
- Around line 39-45: The loading diagnostics properties currently hide
connection error and guidance whenever workspace.macDeviceID is nil, which
causes the overlay to lose network diagnostics for that workspace shape. Update
WorkspaceDetailView+TerminalLoadingDiagnostics so
loadingDiagnosticsConnectionError and loadingDiagnosticsConnectionErrorGuidance
use the same nil/empty fallback logic as activeLoadingDiagnosticsRoute, instead
of requiring a non-nil macDeviceID, and keep the existing store.connectionError
and store.connectionErrorGuidance values visible when appropriate.
---
Outside diff comments:
In
`@Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceDetailView.swift`:
- Around line 274-288: The TerminalDisconnectedOverlay retry closure is
duplicating the same reconnect/switch flow already centralized in
refreshLoadingDiagnosticsConnection(). Update WorkspaceDetailView to call that
shared helper from the overlay action instead of re-implementing the Task with
switchToMac(macDeviceID:) and reconnectOrRefresh(), so
TerminalDisconnectedOverlay and TerminalLoadingDiagnosticsOverlay both use the
same behavior path.
🪄 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: b846549b-3fbf-45c2-a743-aec477e3b476
📒 Files selected for processing (10)
Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/TerminalLoadingDiagnosticsModel.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/TerminalLoadingDiagnosticsModelBuilder.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/TerminalLoadingDiagnosticsOverlay.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/TerminalLoadingDiagnosticsRow.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/TerminalLoadingDiagnosticsTone.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceDetailView+TerminalLoadingDiagnostics.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceDetailView.swiftPackages/iOS/CmuxMobileShellUI/Tests/CmuxMobileShellUITests/TerminalLoadingDiagnosticsModelTests.swiftResources/Localizable.xcstringsios/cmux/Resources/Localizable.xcstrings
| struct TerminalLoadingDiagnosticsModel: Equatable { | ||
| let title: String | ||
| let message: String | ||
| let rows: [TerminalLoadingDiagnosticsRow] | ||
| let isLoading: Bool | ||
|
|
||
| init( | ||
| workspaceName: String, | ||
| terminalCount: Int, | ||
| macName: String?, | ||
| connectionStatus: MobileMacConnectionStatus, | ||
| tailnetStatus: TailnetStatus?, | ||
| activeRoute: CmxAttachRoute?, | ||
| storedRouteDescription: String?, | ||
| connectionError: String?, | ||
| connectionErrorGuidance: String?, | ||
| loadingTimedOut: Bool = false | ||
| ) { | ||
| self = TerminalLoadingDiagnosticsModelBuilder( | ||
| workspaceName: workspaceName, | ||
| terminalCount: terminalCount, | ||
| macName: macName, | ||
| connectionStatus: connectionStatus, | ||
| tailnetStatus: tailnetStatus, | ||
| activeRoute: activeRoute, | ||
| storedRouteDescription: storedRouteDescription, | ||
| connectionError: connectionError, | ||
| connectionErrorGuidance: connectionErrorGuidance, | ||
| loadingTimedOut: loadingTimedOut | ||
| ).makeModel() | ||
| } | ||
|
|
||
| init(title: String, message: String, rows: [TerminalLoadingDiagnosticsRow], isLoading: Bool) { | ||
| self.title = title | ||
| self.message = message | ||
| self.rows = rows | ||
| self.isLoading = isLoading | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
Consider marking this pure value struct nonisolated.
TerminalLoadingDiagnosticsModel is a plain data struct with no UI/actor-affinity, but under Swift 6 MainActor-by-default isolation it can implicitly inherit @MainActor, coupling a value-only type to the main actor unnecessarily and forcing hops when built off-main.
♻️ Suggested fix
-struct TerminalLoadingDiagnosticsModel: Equatable {
+nonisolated struct TerminalLoadingDiagnosticsModel: Equatable {As per coding guidelines, "Report a failure when the diff introduces or materially expands ... pure value model structs that remain implicitly @MainActor when they should be marked nonisolated."
📝 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.
| struct TerminalLoadingDiagnosticsModel: Equatable { | |
| let title: String | |
| let message: String | |
| let rows: [TerminalLoadingDiagnosticsRow] | |
| let isLoading: Bool | |
| init( | |
| workspaceName: String, | |
| terminalCount: Int, | |
| macName: String?, | |
| connectionStatus: MobileMacConnectionStatus, | |
| tailnetStatus: TailnetStatus?, | |
| activeRoute: CmxAttachRoute?, | |
| storedRouteDescription: String?, | |
| connectionError: String?, | |
| connectionErrorGuidance: String?, | |
| loadingTimedOut: Bool = false | |
| ) { | |
| self = TerminalLoadingDiagnosticsModelBuilder( | |
| workspaceName: workspaceName, | |
| terminalCount: terminalCount, | |
| macName: macName, | |
| connectionStatus: connectionStatus, | |
| tailnetStatus: tailnetStatus, | |
| activeRoute: activeRoute, | |
| storedRouteDescription: storedRouteDescription, | |
| connectionError: connectionError, | |
| connectionErrorGuidance: connectionErrorGuidance, | |
| loadingTimedOut: loadingTimedOut | |
| ).makeModel() | |
| } | |
| init(title: String, message: String, rows: [TerminalLoadingDiagnosticsRow], isLoading: Bool) { | |
| self.title = title | |
| self.message = message | |
| self.rows = rows | |
| self.isLoading = isLoading | |
| } | |
| } | |
| nonisolated struct TerminalLoadingDiagnosticsModel: Equatable { | |
| let title: String | |
| let message: String | |
| let rows: [TerminalLoadingDiagnosticsRow] | |
| let isLoading: Bool | |
| init( | |
| workspaceName: String, | |
| terminalCount: Int, | |
| macName: String?, | |
| connectionStatus: MobileMacConnectionStatus, | |
| tailnetStatus: TailnetStatus?, | |
| activeRoute: CmxAttachRoute?, | |
| storedRouteDescription: String?, | |
| connectionError: String?, | |
| connectionErrorGuidance: String?, | |
| loadingTimedOut: Bool = false | |
| ) { | |
| self = TerminalLoadingDiagnosticsModelBuilder( | |
| workspaceName: workspaceName, | |
| terminalCount: terminalCount, | |
| macName: macName, | |
| connectionStatus: connectionStatus, | |
| tailnetStatus: tailnetStatus, | |
| activeRoute: activeRoute, | |
| storedRouteDescription: storedRouteDescription, | |
| connectionError: connectionError, | |
| connectionErrorGuidance: connectionErrorGuidance, | |
| loadingTimedOut: loadingTimedOut | |
| ).makeModel() | |
| } | |
| init(title: String, message: String, rows: [TerminalLoadingDiagnosticsRow], isLoading: Bool) { | |
| self.title = title | |
| self.message = message | |
| self.rows = rows | |
| self.isLoading = isLoading | |
| } | |
| } |
🤖 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/TerminalLoadingDiagnosticsModel.swift`
around lines 5 - 43, Mark TerminalLoadingDiagnosticsModel as nonisolated because
it is a pure value type and should not implicitly inherit MainActor isolation
under Swift 6. Update the struct declaration itself, and keep both initializers
in place while ensuring the TerminalLoadingDiagnosticsModelBuilder path and the
direct property initializer remain free of actor affinity so the model can be
constructed off-main without unnecessary hops.
Source: Coding guidelines
| struct TerminalLoadingDiagnosticsRow: Equatable, Identifiable { | ||
| let id: String | ||
| let label: String | ||
| let value: String | ||
| let tone: TerminalLoadingDiagnosticsTone | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
Identifiable value struct should likely be nonisolated.
This new Identifiable value struct is exactly the pattern called out by the actor-isolation guideline: under Swift 6 MainActor-by-default isolation, an Identifiable pure-data struct can implicitly inherit @MainActor, forcing actor hops for what should be a freely-passable value type (e.g. between a background builder and ForEach rows).
♻️ Suggested fix
-struct TerminalLoadingDiagnosticsRow: Equatable, Identifiable {
+nonisolated struct TerminalLoadingDiagnosticsRow: Equatable, Identifiable {As per coding guidelines: "Report a failure when the diff introduces or materially expands Codable, Identifiable, Sendable, or pure value model structs that remain implicitly @MainActor when they should be marked nonisolated."
📝 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.
| struct TerminalLoadingDiagnosticsRow: Equatable, Identifiable { | |
| let id: String | |
| let label: String | |
| let value: String | |
| let tone: TerminalLoadingDiagnosticsTone | |
| } | |
| nonisolated struct TerminalLoadingDiagnosticsRow: Equatable, Identifiable { | |
| let id: String | |
| let label: String | |
| let value: String | |
| let tone: TerminalLoadingDiagnosticsTone | |
| } |
🤖 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/TerminalLoadingDiagnosticsRow.swift`
around lines 2 - 7, `TerminalLoadingDiagnosticsRow` is a pure value
`Identifiable` model that may implicitly inherit `@MainActor` under Swift 6
isolation. Update the struct declaration itself to be explicitly nonisolated so
it can be passed freely between actor contexts without unnecessary hops. Keep
the change scoped to `TerminalLoadingDiagnosticsRow` and preserve its current
stored properties and protocol conformances.
Source: Coding guidelines
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/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceDetailView.swift (2)
274-291: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDisconnected-overlay retry closure duplicates
refreshLoadingDiagnosticsConnection()logic.The inline closure here (
switchToMacthenreconnectOrRefresh) reimplements the exact same sequence that the newrefreshLoadingDiagnosticsConnection()helper (inWorkspaceDetailView+TerminalLoadingDiagnostics.swift) already encapsulates and is used by the loading overlay's Refresh button. Route both call sites through the single helper to keep the reconnect behavior consistent and avoid future drift between the two entrypoints.♻️ Proposed fix
if loadingDiagnosticsConnectionStatus != .connected { TerminalDisconnectedOverlay( status: loadingDiagnosticsConnectionStatus, host: workspace.macDisplayName ?? host ) { - Task { - if let macDeviceID = workspace.macDeviceID, - !macDeviceID.isEmpty, - await store.switchToMac(macDeviceID: macDeviceID) { - return - } - await store.reconnectOrRefresh() - } + refreshLoadingDiagnosticsConnection() } }🤖 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 274 - 291, The disconnected-overlay retry action in WorkspaceDetailView is duplicating the reconnect sequence already centralized in refreshLoadingDiagnosticsConnection() from WorkspaceDetailView+TerminalLoadingDiagnostics.swift. Update the TerminalDisconnectedOverlay closure to call that helper instead of inlining switchToMac(macDeviceID:) followed by reconnectOrRefresh(), so both retry entrypoints share the same behavior and stay in sync.
245-291: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winUse the same scoped status for the pill and the overlay.
MobileMacConnectionStatusPillstill uses the rawconnectionStatus, while the full-screen overlay usesloadingDiagnosticsConnectionStatus; when the workspace is tied to a different Mac, the overlay can show.unavailablewhile the pill still shows the workspace’s reconnecting/offline state. Pass the scoped status through here, or hide the pill in the unavailable case.🤖 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 245 - 291, The status pill and the disconnected overlay are using different connection states, which can make them disagree when the workspace is scoped to a different Mac. In WorkspaceDetailView, align MobileMacConnectionStatusPill with the same scoped value used by the full-screen overlay (loadingDiagnosticsConnectionStatus), or suppress the pill when that status is .unavailable. Keep the change localized to the existing overlay/pill wiring so both UI states stay consistent.
♻️ Duplicate comments (3)
Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/TerminalLoadingDiagnosticsOverlay.swift (1)
153-169: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftSleep-based readiness wait to gate "no terminals yet" state.
updateTerminalMetadataDeadline()still usesContinuousClock().sleep(for:)to decide when to flipterminalMetadataTimedOut. Same concern raised in a prior review pass on this file: this races with slow/flaky metadata fetches and doesn't derive from a real settled signal.As per coding guidelines: "Do not use
Task.sleepin production Swift code just because it is inside an async function... retry backoff, keepalive loops, readiness waits, delayed dispatch, and polling must use a real cancellation-aware scheduler, timer abstraction, async sequence, callback, notification, or state transition."🤖 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/TerminalLoadingDiagnosticsOverlay.swift` around lines 153 - 169, The readiness gate in updateTerminalMetadataDeadline() still relies on a fixed sleep, which can race with slow or flaky terminal metadata loading. Replace the ContinuousClock().sleep(for:) wait in TerminalLoadingDiagnosticsOverlay with a real cancellation-aware readiness signal or state transition tied to workspace/terminal metadata changes, and keep the existing terminalMetadataTimedOut updates based on that signal instead of elapsed time.Source: Coding guidelines
Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceDetailView+TerminalLoadingDiagnostics.swift (2)
46-54: 🩺 Stability & Availability | 🟡 Minor | ⚖️ Poor tradeoffUntracked fire-and-forget
Taskfor reconnect/refresh.Every call to
refreshLoadingDiagnosticsConnection()(each Refresh tap) spawns a new unstoredTask, with no cancellation of a prior in-flight one. Repeated taps could overlapswitchToMac/reconnectOrRefreshcalls.As per coding guidelines: "Flag fire-and-forget
Task { ... }work with meaningful lifecycle that is not stored, cancelled, 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`+TerminalLoadingDiagnostics.swift around lines 46 - 54, The refreshLoadingDiagnosticsConnection() method is launching a fire-and-forget Task without any lifecycle control, so repeated refreshes can overlap. Update this logic to keep the Task tied to a meaningful lifecycle by storing it on the WorkspaceDetailView+TerminalLoadingDiagnostics state, canceling any in-flight task before starting a new one, and/or reusing a caller-owned async path. Use the refreshLoadingDiagnosticsConnection() entry point and the existing switchToMac(macDeviceID:) and reconnectOrRefresh() calls as the place to add cancellation and serialization.Source: Coding guidelines
38-44: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winConnection error/guidance still dropped for nil
macDeviceIDworkspaces.
loadingDiagnosticsConnectionError/loadingDiagnosticsConnectionErrorGuidancegate onloadingDiagnosticsMatchesForegroundMac(workspace.macDeviceID), which returnsfalsewhenevermacDeviceIDis nil/empty (see the guard at line 57-60). This means the overlay silently loses connection error/guidance for that workspace shape, same gap flagged in a prior review pass.activeLoadingDiagnosticsRoute(lines 27-36) already has the correct nil-fallback pattern (falling back tostore.activeRoutewhen connected) — mirror that here instead of unconditionally returningnil.🛠️ Proposed fix
var loadingDiagnosticsConnectionError: String? { - loadingDiagnosticsMatchesForegroundMac(workspace.macDeviceID) ? store.connectionError : nil + guard let macDeviceID = workspace.macDeviceID, !macDeviceID.isEmpty else { + return loadingDiagnosticsConnectionStatus == .connected ? store.connectionError : nil + } + return loadingDiagnosticsMatchesForegroundMac(macDeviceID) ? store.connectionError : nil } var loadingDiagnosticsConnectionErrorGuidance: String? { - loadingDiagnosticsMatchesForegroundMac(workspace.macDeviceID) ? store.connectionErrorGuidance : nil + guard let macDeviceID = workspace.macDeviceID, !macDeviceID.isEmpty else { + return loadingDiagnosticsConnectionStatus == .connected ? store.connectionErrorGuidance : nil + } + return loadingDiagnosticsMatchesForegroundMac(macDeviceID) ? store.connectionErrorGuidance : nil }🤖 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`+TerminalLoadingDiagnostics.swift around lines 38 - 44, The connection error and guidance are still being dropped for workspaces with a nil or empty macDeviceID because loadingDiagnosticsConnectionError and loadingDiagnosticsConnectionErrorGuidance only return values when loadingDiagnosticsMatchesForegroundMac(workspace.macDeviceID) is true. Update the WorkspaceDetailView+TerminalLoadingDiagnostics logic to mirror the nil-fallback behavior used by activeLoadingDiagnosticsRoute: keep using the foreground-Mac check when macDeviceID is present, but fall back to store.connectionError and store.connectionErrorGuidance for the nil/empty macDeviceID workspace shape instead of returning nil.
🤖 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/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceDetailView.swift`:
- Around line 274-291: The disconnected-overlay retry action in
WorkspaceDetailView is duplicating the reconnect sequence already centralized in
refreshLoadingDiagnosticsConnection() from
WorkspaceDetailView+TerminalLoadingDiagnostics.swift. Update the
TerminalDisconnectedOverlay closure to call that helper instead of inlining
switchToMac(macDeviceID:) followed by reconnectOrRefresh(), so both retry
entrypoints share the same behavior and stay in sync.
- Around line 245-291: The status pill and the disconnected overlay are using
different connection states, which can make them disagree when the workspace is
scoped to a different Mac. In WorkspaceDetailView, align
MobileMacConnectionStatusPill with the same scoped value used by the full-screen
overlay (loadingDiagnosticsConnectionStatus), or suppress the pill when that
status is .unavailable. Keep the change localized to the existing overlay/pill
wiring so both UI states stay consistent.
---
Duplicate comments:
In
`@Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/TerminalLoadingDiagnosticsOverlay.swift`:
- Around line 153-169: The readiness gate in updateTerminalMetadataDeadline()
still relies on a fixed sleep, which can race with slow or flaky terminal
metadata loading. Replace the ContinuousClock().sleep(for:) wait in
TerminalLoadingDiagnosticsOverlay with a real cancellation-aware readiness
signal or state transition tied to workspace/terminal metadata changes, and keep
the existing terminalMetadataTimedOut updates based on that signal instead of
elapsed time.
In
`@Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceDetailView`+TerminalLoadingDiagnostics.swift:
- Around line 46-54: The refreshLoadingDiagnosticsConnection() method is
launching a fire-and-forget Task without any lifecycle control, so repeated
refreshes can overlap. Update this logic to keep the Task tied to a meaningful
lifecycle by storing it on the WorkspaceDetailView+TerminalLoadingDiagnostics
state, canceling any in-flight task before starting a new one, and/or reusing a
caller-owned async path. Use the refreshLoadingDiagnosticsConnection() entry
point and the existing switchToMac(macDeviceID:) and reconnectOrRefresh() calls
as the place to add cancellation and serialization.
- Around line 38-44: The connection error and guidance are still being dropped
for workspaces with a nil or empty macDeviceID because
loadingDiagnosticsConnectionError and loadingDiagnosticsConnectionErrorGuidance
only return values when
loadingDiagnosticsMatchesForegroundMac(workspace.macDeviceID) is true. Update
the WorkspaceDetailView+TerminalLoadingDiagnostics logic to mirror the
nil-fallback behavior used by activeLoadingDiagnosticsRoute: keep using the
foreground-Mac check when macDeviceID is present, but fall back to
store.connectionError and store.connectionErrorGuidance for the nil/empty
macDeviceID workspace shape instead of returning nil.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: d49c3aa7-6ca9-4b1a-bb29-3cc1c83ade3e
📒 Files selected for processing (10)
Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/TerminalLoadingDiagnosticsModel.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/TerminalLoadingDiagnosticsModelBuilder.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/TerminalLoadingDiagnosticsOverlay.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/TerminalLoadingDiagnosticsRow.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/TerminalLoadingDiagnosticsTone.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceDetailView+TerminalLoadingDiagnostics.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceDetailView.swiftPackages/iOS/CmuxMobileShellUI/Tests/CmuxMobileShellUITests/TerminalLoadingDiagnosticsModelTests.swiftResources/Localizable.xcstringsios/cmux/Resources/Localizable.xcstrings
lawrencecchen
added
the
stale-revisit
This branch was successfully deployed
2 participants
Summary
Verification
CMUX_ALLOW_LOCAL_XCODEBUILD=1 ios/scripts/reload.sh --tag ldterm --simulator "iPhone 17"./ios/scripts/reload-cloud.sh --tag ldterm --device-id E4058DA9-F4C7-52DD-951D-0354061B8E89CMUX_PORT=3942 ./scripts/reload-cloud.sh --tag ldtermNote: focused package tests are blocked locally by the hq guard for
xcodebuildmcp test, and direct SwiftPM test resolution hits the existing macOS platform mismatch for this package graph.Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Note
Low Risk
iOS-only UI and presentation logic with unit tests on the model builder; reconnect/refresh paths reuse existing store APIs without changing attach or auth behavior.
Overview
Replaces the blank iOS workspace detail state (Mac connected but no terminal surface yet) with a
TerminalLoadingDiagnosticsOverlaythat explains what is happening and what to do next.The overlay builds copy and tone-coded rows (Mac, terminals, Tailscale, route, optional network guidance) via
TerminalLoadingDiagnosticsModel/ModelBuilder, reads Tailscale from the environment, and after 10s with zero terminals switches from a spinner to “No terminals yet” with Refresh (resets the timer and callsswitchToMac+reconnectOrRefresh) and Create Terminal.WorkspaceDetailView+TerminalLoadingDiagnosticsscopes connection status, active route, and store errors to the workspace’s Mac (foreground Mac, active ticket, or paired aliases); otherwise the workspace Mac is treated as unavailable andTerminalDisconnectedOverlayuses that same status and the workspace Mac display name.Reviewed by Cursor Bugbot for commit 2858674. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Adds an iOS terminal loading diagnostics overlay for connected Macs when terminals haven’t loaded yet. After 10s it switches to “No terminals yet” with Refresh/Create actions, and routes/network diagnostics are scoped to the active or paired Mac only.
TerminalLoadingDiagnosticsOverlayand model (with builder): spinner with a 10s timeout that flips to an empty state; tone-dot rows for Mac, Terminals, Tailscale, and Route; optional network guidance; Refresh/Create Terminal buttons with accessibility IDs. Refresh resets the timer, switches to the workspace’s Mac, and callsreconnectOrRefresh.WorkspaceDetailView: usestailscaleStatusMonitor; computesloadingDiagnosticsConnectionStatusand shows the overlay only when it’s Connected; scopes active route and connection errors to the matching foreground/active/paired Mac; falls back to a saved route from paired Mac metadata; shares the status withTerminalDisconnectedOverlayand uses the workspace’s Mac name for the host label.mobile.terminal.loading.*strings.Written for commit 2858674. Summary will update on new commits.
Summary by CodeRabbit