Repository navigation
Use native Liquid Glass tabs on iPad - #11327
Closed
azooz2003-bit wants to merge 123 commits into
Closed
azooz2003-bit wants to merge 123 commits into
azooz2003-bit wants to merge 123 commits into
Conversation
…oup-actions # Conflicts: # Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceListTableCoordinator.swift
…into feat-ios-group-actions-dogfood
Main's drop tests (from #8602) still passed connectionRecoveryFailed, isRecoveringConnection, and retryConnectionRecovery, which this branch's status-line rework removed from WorkspaceListTable; the package no longer compiled on the merged tree. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
|
Too many files changed for review (123 files, 100 file limit). Bypass the limit by tagging |
|
Caution CodeRabbit couldn't post its review summary. Error details |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
28 issues found across 123 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Sources/Mobile/MobileHostTransportAuthorization.swift">
<violation number="1" location="Sources/Mobile/MobileHostTransportAuthorization.swift:254">
P2: `debugCloseConnections` is a DEBUG-only API added directly to the production registry type. Move this facility into a dedicated DEBUG extension/file so the core transport authorization source does not grow a debug seam.</violation>
<violation number="2" location="Sources/Mobile/MobileHostTransportAuthorization.swift:259">
P2: When a new mobile connection arrives during an all-transport debug disconnect, the snapshot’s entries remain active until each shutdown finishes, allowing connections to survive the operation. Retire the selected entries atomically before awaiting shutdown, matching the existing `removeAll` paths.</violation>
</file>
<file name="ios/cmuxPackage/Sources/cmuxFeature/MobileIrohConnectionReadinessOwner.swift">
<violation number="1" location="ios/cmuxPackage/Sources/cmuxFeature/MobileIrohConnectionReadinessOwner.swift:146">
P2: When a connection caller is cancelled while activation remains pending, `wait` cannot observe cancellation because this continuation is uncancellable. Add cancellation-aware waiter registration that removes and resumes the cancelled waiter, then check cancellation before returning an outcome.</violation>
</file>
<file name="Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite+WorkspaceActions.swift">
<violation number="1" location="Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite+WorkspaceActions.swift:555">
P2: When a mutation races an in-flight connection attempt, this maps `.connectAttemptGated` to a timeout even though no request timed out. Preserve a distinct gated/in-progress result or wait for the active connection so the UI does not show misleading timeout guidance.</violation>
</file>
<file name="scripts/lib/mobile-attach.sh">
<violation number="1" location="scripts/lib/mobile-attach.sh:175">
P2: When this attach flow runs under a LaunchAgent or other non-login context, `${HOME}` can differ from the user’s actual macOS home, so `cmux-debug-cli.sh` looks in the wrong DerivedData directory and readiness fails. Resolve the real home with `dscl ... NFSHomeDirectory` before invoking the debug CLI, consistently with the app’s `FileManager.homeDirectoryForCurrentUser`.</violation>
</file>
<file name="Packages/Shared/CmuxIrohTransport/Sources/CmuxIrohTransport/CmxIrohTrustBrokerClientError.swift">
<violation number="1" location="Packages/Shared/CmuxIrohTransport/Sources/CmuxIrohTransport/CmxIrohTrustBrokerClientError.swift:40">
P1: When an active host registration refresh receives 401 or 403, this classifier now keeps the endpoint active instead of failing closed after broker authorization rejection. Preserve 401 retries only for initial activation, and keep authorization rejections out of the refresh-preservation predicate.</violation>
</file>
<file name="Packages/Shared/CMUXMobileCore/Sources/CMUXMobileCore/DiagnosticReport.swift">
<violation number="1" location="Packages/Shared/CMUXMobileCore/Sources/CMUXMobileCore/DiagnosticReport.swift:137">
P2: When a recovery attempt is superseded, `lastFailureEvent` still surfaces it as the latest failure. Exclude `.superseded` alongside `.cancelled` so routine replacement churn does not appear as a diagnostic failure.</violation>
</file>
<file name="Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobilePresencePushRecoveryThrottle.swift">
<violation number="1" location="Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobilePresencePushRecoveryThrottle.swift:28">
P2: When a changed-evidence push arrives during an active recovery, `recoverMobileConnection` cannot start another attempt, but this line still advances the throttle. Record throttle history only after the recovery owner accepts a new attempt so a failed pass can be retried on the next eligible heartbeat.</violation>
</file>
<file name="ios/cmuxUITests/cmuxUITests.swift">
<violation number="1" location="ios/cmuxUITests/cmuxUITests.swift:739">
P2: When the group context menu fails to open, this test passes because all workspace actions are absent. Wait for a known group action before checking the excluded actions.</violation>
</file>
<file name="Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceListLayoutPreviewView.swift">
<violation number="1" location="Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceListLayoutPreviewView.swift:392">
P3: These new group-action handlers are no-ops even though the reorder fixture also sets `supportsGroupActions = true` and the comment says all affordances should be dogfoodable. Tapping rename/collapse group mutates `model.groups`, but delete group, ungroup, and pin do nothing, so those menu items render enabled yet have no effect. Wire them against `model.groups` (delete/ungroup remove the group and clear member `groupID`s; pin toggles a pinned flag) or drop the handlers so the items don't appear actionable.</violation>
</file>
<file name="cmuxTests/MobileHostConnectionLifecycleTests.swift">
<violation number="1" location="cmuxTests/MobileHostConnectionLifecycleTests.swift:218">
P2: The `mobile.rpc.ready` publish is not synchronous with the checked send: after `sendResponse` writes the subscribe ack, the response task runs `recordReadinessContribution`->`publishUsableSessionIfReady` in a separate MainActor continuation. `waitForSentBufferCount(3)` returns as soon as the transport records the send, so `#expect(readyEvents.count == 1)` can run before the publish and observe an empty bus, making this test flaky. Await `drainMobileHostMainQueue()` (or poll the retained snapshot with a deadline) after waiting for the third send, before asserting the positive presence.</violation>
</file>
<file name="Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/CMUXMobileRootView.swift">
<violation number="1" location="Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/CMUXMobileRootView.swift:794">
P2: When a configured attach URL has a compatibility mismatch, this branch treats `.needsUserApproval` as failure and immediately falls back to saved-Mac reconnect. Unlike `connectAttachURL`, it never calls `showAddDevice()`, so the pending warning is not presented and the launch pairing cannot be approved. Handle `.needsUserApproval` separately by presenting the add-device sheet and deferring stored-Mac fallback until the user accepts or cancels.</violation>
</file>
<file name="Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellWorkspaceMutationTicketPolicy.swift">
<violation number="1" location="Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellWorkspaceMutationTicketPolicy.swift:28">
P2: When host status is identity-free or Stack-token acquisition is unavailable, this early return treats the public capability bit as proof of account authorization. That enables Mac-scoped mutation affordances and omits the attach token, but the subsequent RPC still requires Stack auth and fails; derive this gate from verified same-account status instead of capability presence alone, or retain ticket-based gating until verification succeeds.</violation>
</file>
<file name="scripts/mobile-dev-launch.sh">
<violation number="1" location="scripts/mobile-dev-launch.sh:214">
P1: When `--attach` is used without `--detach` on a simulator, `simctl launch --console-pty` blocks until the app exits, so execution never reaches this readiness wait or writes a receipt. Omit `--console-pty` whenever readiness is enabled, or run the console attachment separately.</violation>
</file>
<file name="web/app/api/relay/token/route.ts">
<violation number="1" location="web/app/api/relay/token/route.ts:161">
P2: Every valid request performs policy repository work and a binding database lookup before this limiter runs, so floods still drive the expensive path despite the comment claiming duplicate work is bounded. Add a coarse account/endpoint limit before policy and binding lookups, or otherwise rate-limit that work before phase classification.</violation>
</file>
<file name="Packages/iOS/CmuxMobileRPC/Sources/CmuxMobileRPC/MobileCoreRPCSession.swift">
<violation number="1" location="Packages/iOS/CmuxMobileRPC/Sources/CmuxMobileRPC/MobileCoreRPCSession.swift:493">
P2: When the global 16-attempt budget is exhausted by other routes, `beginConnect` also returns `.busy`, so this line reports `connectAttemptGated` even though this route is free. That classification is treated as non-transient and can prevent retrying until a later user action; distinguish global-budget exhaustion from same-route gating, or preserve a transient error for the global case.</violation>
</file>
<file name="Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite+ConnectionRecovery.swift">
<violation number="1" location="Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite+ConnectionRecovery.swift:152">
P1: When an event-stream failure is queued during backgrounding, this replays it through the ordinary recovery entry after foreground has already started a probe, so the definitive failure can be coalesced away and the stale client gets reused. Preserve the expected client and re-enter `recoverDeadConnection`, or otherwise prioritize pending failures and force the event-stream-ended path to redial without probing.</violation>
</file>
<file name="Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobilePrimaryTabScaffold.swift">
<violation number="1" location="Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobilePrimaryTabScaffold.swift:47">
P2: On iOS 26 full-size iPad layouts, this branch drops the New Task control even when `taskComposerAction` is enabled. Render `TaskComposerButton` in the iPad presentation, preserving the existing `.workspaces` condition.</violation>
</file>
<file name="Packages/iOS/CmuxMobileRPC/Sources/CmuxMobileRPC/MobileCoreRPCSession+RequestSettlement.swift">
<violation number="1" location="Packages/iOS/CmuxMobileRPC/Sources/CmuxMobileRPC/MobileCoreRPCSession+RequestSettlement.swift:36">
P3: This newly classified false-bucket case has no test coverage. Add expectCancellationOutranksTransportFailure(.connectAttemptGated) next to the existing .connectionClosed/.requestTimedOut/.invalidResponse cases to guard the cancellation ambiguity for the new classification.</violation>
</file>
<file name="Packages/Shared/CmuxIrohTransport/Sources/CmuxIrohTransport/CmxIrohTrustBrokerClient.swift">
<violation number="1" location="Packages/Shared/CmuxIrohTransport/Sources/CmuxIrohTransport/CmxIrohTrustBrokerClient.swift:120">
P2: When rotation changes only the refresh token, this comparison misses the newer coherent pair and unnecessarily force-refreshes; a transient refresh failure then leaves the request failed despite usable current credentials. Compare both access and refresh tokens before deciding that the snapshot is unchanged.</violation>
</file>
<file name=".github/workflows/reload-build.yml">
<violation number="1" location=".github/workflows/reload-build.yml:201">
P3: Forcing ARCHS=arm64 with ONLY_ACTIVE_ARCH=YES produces an arm64-only simulator binary, but the runner input still allows cmux-aws-macos-15 (Intel). On an Intel runner the built cmux.app will not launch on its x86_64 simulator, so the isolated-verification artifact is unusable there, and ONY_ACTIVE_ARCH+generic destination is non-deterministic about which arch is 'active'. Guard by runner arch or query the sim arch instead of hard-coding; if arm64-only is intended, drop the Intel runner option for this job.</violation>
</file>
<file name="Sources/Mobile/MobileHostService.swift">
<violation number="1" location="Sources/Mobile/MobileHostService.swift:371">
P3: `updateIrohBinding` has no callers, so this added API never updates the route cache and only leaves dead code behind. Remove it or wire the binding activation path through it.</violation>
</file>
<file name="Packages/Shared/CmuxIrohTransport/Tests/CmuxIrohTransportTests/CmxIrohHostRuntimeTests.swift">
<violation number="1" location="Packages/Shared/CmuxIrohTransport/Tests/CmuxIrohTransportTests/CmxIrohHostRuntimeTests.swift:561">
P3: None of the waiter machinery added to HostRuntimeBindingRecorder is ever used: the new waitForCount(_:), waitForCount(_:timeout:), waiters, and cancelWaiter have no callers (only record() and count() are used), so the continuation-resume block in record() can never be reached. Remove the unused waiters state and waiter methods unless a caller is planned.</violation>
</file>
<file name="cmuxTests/MobileWorkspaceListFidelityTests.swift">
<violation number="1" location="cmuxTests/MobileWorkspaceListFidelityTests.swift:352">
P3: The `configured != storedOnly` assertion injects `groupIconSymbols: [groupID: "hammer.fill"]` directly into `summaryHashForTesting`, so it only proves the hash combines a hand-supplied dict. It does not exercise the observer's real config-resolution path (`currentGroupIconSymbols(for:)`), so the test name overstates what it covers. Compute the configured hash through the observer's resolved symbols (or add an assertion on the resolved `currentGroupIconSymbols`) so the config→hash integration is actually guarded.</violation>
</file>
<file name="Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobilePrimarySearchCoordinator.swift">
<violation number="1" location="Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobilePrimarySearchCoordinator.swift:40">
P3: beginSearch(for:) has no production callers anywhere in the repo; only the new unit test exercises it. The iPad custom bottom control that this method is documented to serve never invokes it, so the feature it adds is currently dead in production. Wire the control's tap to beginSearch(for:) (or remove the method and its test until it is used).</violation>
</file>
<file name="Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/WorkspaceCreatePinnedContext.swift">
<violation number="1" location="Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/WorkspaceCreatePinnedContext.swift:61">
P2: For a spec-less (legacy) create, classifying connectAttemptGated as ambiguous makes caughtErrorDisposition return .preserveSuccess, and the caller returns .success(()) even though the workspace.create request was never sent to the host. connectAttemptGated is raised mid-connect by the route-scoped connectAttemptRegistry when ANY connect for that Mac route is in flight (MobileCoreRPCClient.swift:123 keys the attempt by route only), so a concurrent reconnect/attach can gate the create without any create having been issued or in progress. The user is then told a workspace was created when none exists. The disposition unit test only asserts the classification in isolation; it does not exercise gating against a non-create operation. Consider leaving connectAttemptGated out of the ambiguous set (surfaceError/failClosed) or verifying an actual create is in flight before preserving success.</violation>
</file>
<file name="Packages/iOS/CmuxMobileRPC/Tests/CmuxMobileRPCTests/MobileRPCTransportConnectEventTests.swift">
<violation number="1" location="Packages/iOS/CmuxMobileRPC/Tests/CmuxMobileRPCTests/MobileRPCTransportConnectEventTests.swift:180">
P3: waitUntilObserved() waits with an unbounded withCheckedContinuation that is only resumed when a .failed(.cancelled) event arrives. If the session ever stops emitting that event (e.g. a regression in the abandon/close path), the continuation is never resumed and the test harness cancels the task, surfacing a confusing "SWIFT TASK CONTINUATION MISUSE: leaked its continuation" trap instead of a clear assertion failure on a missing cancellation outcome. Make the wait bounded (inject a timeout into waitUntilObserved or wrap it) so a missing event fails the test cleanly with a diagnostic.</violation>
</file>
<file name="Packages/iOS/CmuxMobileRPC/Sources/CmuxMobileRPC/MobileRPCConnectAttemptRegistry.swift">
<violation number="1" location="Packages/iOS/CmuxMobileRPC/Sources/CmuxMobileRPC/MobileRPCConnectAttemptRegistry.swift:110">
P2: Mutating `routeStates` while iterating over it with `for (key, state) in routeStates` triggers Swift's exclusivity enforcement. When the loop body executes (any entry matching the filter), the `routeStates[key] = nil` modify access overlaps the read access the iterator holds, producing a runtime trap ("Simultaneous accesses ... but modification requires exclusive access"). Collect the keys first, then remove them in a separate loop.
Note also that, because `store(_:forKey:)` already prunes any state with `activeLeaseID == nil && physicalCleanupTasks.isEmpty`, no entry ever matches this filter, so the method is currently a no-op and does not actually drop the route-health strikes the caller (`MobileShellComposite+ConnectionRecovery.startObservingNetworkPathChanges`) expects it to clear on a network change.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Comment on lines
+40
to
+42
| return statusCode == 401 | ||
| || statusCode == 403 | ||
| || statusCode == 408 |
There was a problem hiding this comment.
P1: When an active host registration refresh receives 401 or 403, this classifier now keeps the endpoint active instead of failing closed after broker authorization rejection. Preserve 401 retries only for initial activation, and keep authorization rejections out of the refresh-preservation predicate.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Packages/Shared/CmuxIrohTransport/Sources/CmuxIrohTransport/CmxIrohTrustBrokerClientError.swift, line 40:
<comment>When an active host registration refresh receives 401 or 403, this classifier now keeps the endpoint active instead of failing closed after broker authorization rejection. Preserve 401 retries only for initial activation, and keep authorization rejections out of the refresh-preservation predicate.</comment>
<file context>
@@ -28,7 +28,18 @@ public enum CmxIrohTrustBrokerClientError:
+ // endpoint rebuild. A genuinely dead session clears auth state
+ // through the coordinator, which stops the runtime through the
+ // lifecycle owner instead.
+ return statusCode == 401
+ || statusCode == 403
+ || statusCode == 408
</file context>
Suggested change
| return statusCode == 401 | |
| || statusCode == 403 | |
| || statusCode == 408 | |
| return statusCode == 408 |
| fi | ||
| echo "==> launching $BUNDLE_ID on $TARGET (signed in as $SIGN_IN_ACCOUNT_LABEL${ATTACH_URL:+, auto-pairing})" | ||
| READINESS_STARTED_MS="" | ||
| if [[ -n "$READINESS_CURSOR" ]]; then |
There was a problem hiding this comment.
P1: When --attach is used without --detach on a simulator, simctl launch --console-pty blocks until the app exits, so execution never reaches this readiness wait or writes a receipt. Omit --console-pty whenever readiness is enabled, or run the console attachment separately.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At scripts/mobile-dev-launch.sh, line 214:
<comment>When `--attach` is used without `--detach` on a simulator, `simctl launch --console-pty` blocks until the app exits, so execution never reaches this readiness wait or writes a receipt. Omit `--console-pty` whenever readiness is enabled, or run the console attachment separately.</comment>
<file context>
@@ -192,6 +210,10 @@ if [[ -n "$AUTH_CREDENTIALS_FILE" ]]; then
fi
echo "==> launching $BUNDLE_ID on $TARGET (signed in as $SIGN_IN_ACCOUNT_LABEL${ATTACH_URL:+, auto-pairing})"
+READINESS_STARTED_MS=""
+if [[ -n "$READINESS_CURSOR" ]]; then
+ READINESS_STARTED_MS="$(cmux_attach_monotonic_milliseconds)"
+fi
</file context>
| guard foregroundRefreshIsActive, | ||
| let trigger = pendingInactiveRecoveryTrigger else { return } | ||
| pendingInactiveRecoveryTrigger = nil | ||
| recoverMobileConnection(trigger: trigger) |
There was a problem hiding this comment.
P1: When an event-stream failure is queued during backgrounding, this replays it through the ordinary recovery entry after foreground has already started a probe, so the definitive failure can be coalesced away and the stale client gets reused. Preserve the expected client and re-enter recoverDeadConnection, or otherwise prioritize pending failures and force the event-stream-ended path to redial without probing.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite+ConnectionRecovery.swift, line 152:
<comment>When an event-stream failure is queued during backgrounding, this replays it through the ordinary recovery entry after foreground has already started a probe, so the definitive failure can be coalesced away and the stale client gets reused. Preserve the expected client and re-enter `recoverDeadConnection`, or otherwise prioritize pending failures and force the event-stream-ended path to redial without probing.</comment>
<file context>
@@ -128,6 +145,13 @@ extension MobileShellComposite {
+ guard foregroundRefreshIsActive,
+ let trigger = pendingInactiveRecoveryTrigger else { return }
+ pendingInactiveRecoveryTrigger = nil
+ recoverMobileConnection(trigger: trigger)
+ }
+
</file context>
| /// no id is supplied, through the same connection-owned close path used | ||
| /// by production failures. The snapshot is taken under the registry lock; | ||
| /// no lock is held while transport shutdown awaits. | ||
| func debugCloseConnections(connectionID: UUID?) async -> [UUID] { |
There was a problem hiding this comment.
P2: debugCloseConnections is a DEBUG-only API added directly to the production registry type. Move this facility into a dedicated DEBUG extension/file so the core transport authorization source does not grow a debug seam.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Sources/Mobile/MobileHostTransportAuthorization.swift, line 254:
<comment>`debugCloseConnections` is a DEBUG-only API added directly to the production registry type. Move this facility into a dedicated DEBUG extension/file so the core transport authorization source does not grow a debug seam.</comment>
<file context>
@@ -245,6 +245,39 @@ final class MobileHostConnectionRegistry: @unchecked Sendable {
+ /// no id is supplied, through the same connection-owned close path used
+ /// by production failures. The snapshot is taken under the registry lock;
+ /// no lock is held while transport shutdown awaits.
+ func debugCloseConnections(connectionID: UUID?) async -> [UUID] {
+ let selected = debugConnectionSnapshot(connectionID: connectionID)
+ let ordered = selected.sorted {
</file context>
| let ordered = selected.sorted { | ||
| $0.0.uuidString < $1.0.uuidString | ||
| } | ||
| for (_, connection) in ordered { |
There was a problem hiding this comment.
P2: When a new mobile connection arrives during an all-transport debug disconnect, the snapshot’s entries remain active until each shutdown finishes, allowing connections to survive the operation. Retire the selected entries atomically before awaiting shutdown, matching the existing removeAll paths.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Sources/Mobile/MobileHostTransportAuthorization.swift, line 259:
<comment>When a new mobile connection arrives during an all-transport debug disconnect, the snapshot’s entries remain active until each shutdown finishes, allowing connections to survive the operation. Retire the selected entries atomically before awaiting shutdown, matching the existing `removeAll` paths.</comment>
<file context>
@@ -245,6 +245,39 @@ final class MobileHostConnectionRegistry: @unchecked Sendable {
+ let ordered = selected.sorted {
+ $0.0.uuidString < $1.0.uuidString
+ }
+ for (_, connection) in ordered {
+ await connection.close(reason: "debug transport disconnect")
+ }
</file context>
| ) | ||
| } | ||
|
|
||
| func updateIrohBinding(_ binding: CmxIrohBrokerBindingMetadata) { |
There was a problem hiding this comment.
P3: updateIrohBinding has no callers, so this added API never updates the route cache and only leaves dead code behind. Remove it or wire the binding activation path through it.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Sources/Mobile/MobileHostService.swift, line 371:
<comment>`updateIrohBinding` has no callers, so this added API never updates the route cache and only leaves dead code behind. Remove it or wire the binding activation path through it.</comment>
<file context>
@@ -368,6 +368,10 @@ final class MobileHostService {
)
}
+ func updateIrohBinding(_ binding: CmxIrohBrokerBindingMetadata) {
+ MobileHostPublicStatusCache.update(irohBinding: binding)
+ }
</file context>
| func record() { recordedCount += 1 } | ||
| func count() -> Int { recordedCount } | ||
|
|
||
| func waitForCount(_ count: Int, timeout: Duration) async -> Bool { |
There was a problem hiding this comment.
P3: None of the waiter machinery added to HostRuntimeBindingRecorder is ever used: the new waitForCount(:), waitForCount(:timeout:), waiters, and cancelWaiter have no callers (only record() and count() are used), so the continuation-resume block in record() can never be reached. Remove the unused waiters state and waiter methods unless a caller is planned.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Packages/Shared/CmuxIrohTransport/Tests/CmuxIrohTransportTests/CmxIrohHostRuntimeTests.swift, line 561:
<comment>None of the waiter machinery added to HostRuntimeBindingRecorder is ever used: the new waitForCount(_:), waitForCount(_:timeout:), waiters, and cancelWaiter have no callers (only record() and count() are used), so the continuation-resume block in record() can never be reached. Remove the unused waiters state and waiter methods unless a caller is planned.</comment>
<file context>
@@ -542,9 +542,62 @@ actor TestIrohHostBroker: CmxIrohHostBrokerServing {
- func record() { recordedCount += 1 }
func count() -> Int { recordedCount }
+
+ func waitForCount(_ count: Int, timeout: Duration) async -> Bool {
+ if recordedCount >= count { return true }
+ return await withTaskGroup(of: Bool.self) { group in
</file context>
| groups: manager.workspaceGroups, | ||
| selectedTabID: manager.selectedTabId | ||
| ) | ||
| let configured = MobileWorkspaceListObserver.summaryHashForTesting( |
There was a problem hiding this comment.
P3: The configured != storedOnly assertion injects groupIconSymbols: [groupID: "hammer.fill"] directly into summaryHashForTesting, so it only proves the hash combines a hand-supplied dict. It does not exercise the observer's real config-resolution path (currentGroupIconSymbols(for:)), so the test name overstates what it covers. Compute the configured hash through the observer's resolved symbols (or add an assertion on the resolved currentGroupIconSymbols) so the config→hash integration is actually guarded.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At cmuxTests/MobileWorkspaceListFidelityTests.swift, line 352:
<comment>The `configured != storedOnly` assertion injects `groupIconSymbols: [groupID: "hammer.fill"]` directly into `summaryHashForTesting`, so it only proves the hash combines a hand-supplied dict. It does not exercise the observer's real config-resolution path (`currentGroupIconSymbols(for:)`), so the test name overstates what it covers. Compute the configured hash through the observer's resolved symbols (or add an assertion on the resolved `currentGroupIconSymbols`) so the config→hash integration is actually guarded.</comment>
<file context>
@@ -262,6 +262,110 @@ struct MobileWorkspaceListFidelityTests {
+ groups: manager.workspaceGroups,
+ selectedTabID: manager.selectedTabId
+ )
+ let configured = MobileWorkspaceListObserver.summaryHashForTesting(
+ tabs: manager.tabs,
+ groups: manager.workspaceGroups,
</file context>
| /// Starts search for the visible primary destination. iPad uses a custom | ||
| /// bottom control instead of selecting the transient search tab, so the | ||
| /// scope must be chosen before the searchable navigation stack presents. | ||
| func beginSearch(for scope: MobilePrimarySearchScope) { |
There was a problem hiding this comment.
P3: beginSearch(for:) has no production callers anywhere in the repo; only the new unit test exercises it. The iPad custom bottom control that this method is documented to serve never invokes it, so the feature it adds is currently dead in production. Wire the control's tap to beginSearch(for:) (or remove the method and its test until it is used).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobilePrimarySearchCoordinator.swift, line 40:
<comment>beginSearch(for:) has no production callers anywhere in the repo; only the new unit test exercises it. The iPad custom bottom control that this method is documented to serve never invokes it, so the feature it adds is currently dead in production. Wire the control's tap to beginSearch(for:) (or remove the method and its test until it is used).</comment>
<file context>
@@ -34,6 +34,14 @@ final class MobilePrimarySearchCoordinator {
+ /// Starts search for the visible primary destination. iPad uses a custom
+ /// bottom control instead of selecting the transient search tab, so the
+ /// scope must be chosen before the searchable navigation stack presents.
+ func beginSearch(for scope: MobilePrimarySearchScope) {
+ self.scope = scope
+ setPresentation(true)
</file context>
| } | ||
| } | ||
|
|
||
| func waitUntilObserved() async { |
There was a problem hiding this comment.
P3: waitUntilObserved() waits with an unbounded withCheckedContinuation that is only resumed when a .failed(.cancelled) event arrives. If the session ever stops emitting that event (e.g. a regression in the abandon/close path), the continuation is never resumed and the test harness cancels the task, surfacing a confusing "SWIFT TASK CONTINUATION MISUSE: leaked its continuation" trap instead of a clear assertion failure on a missing cancellation outcome. Make the wait bounded (inject a timeout into waitUntilObserved or wrap it) so a missing event fails the test cleanly with a diagnostic.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Packages/iOS/CmuxMobileRPC/Tests/CmuxMobileRPCTests/MobileRPCTransportConnectEventTests.swift, line 180:
<comment>waitUntilObserved() waits with an unbounded withCheckedContinuation that is only resumed when a .failed(.cancelled) event arrives. If the session ever stops emitting that event (e.g. a regression in the abandon/close path), the continuation is never resumed and the test harness cancels the task, surfacing a confusing "SWIFT TASK CONTINUATION MISUSE: leaked its continuation" trap instead of a clear assertion failure on a missing cancellation outcome. Make the wait bounded (inject a timeout into waitUntilObserved or wrap it) so a missing event fails the test cleanly with a diagnostic.</comment>
<file context>
@@ -158,3 +159,28 @@ import Testing
+ }
+ }
+
+ func waitUntilObserved() async {
+ guard !observed else { return }
+ await withCheckedContinuation { continuation in
</file context>
Collaborator
Author
|
Superseded by the clean main-based PR for this iPad UI change: https://github.com/manaflow-ai/cmux/pull/new/fix-ipad-native-tabs-clean |
Collaborator
Author
|
Superseded by the clean main-based PR for this iPad UI change. |
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Verification
12B9C900-5FCF-47E9-AA03-BF55C5A2F6A2artifacts/verify-ui/ipado-sidebar-style-auth.pngNeed help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by cubic
Uses SwiftUI's native adaptive iPad tab presentation instead of the app-owned primary rail, and hardens mobile reconnection so in-flight attempts are no longer restarted, starved, or mislabeled as timeouts.
Connection Recovery
Workspace Group Actions
Written for commit 46b6120. Summary will update on new commits.