Fix iOS deleted computer recovery empty state - #8712
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds reusable iOS deleted-computer recovery controls, centralizes recovery state and result handling, integrates recovery into device and disconnected-workspace views, changes add-device presentation rules, and adds tests for the updated flow. ChangesDeleted computer recovery
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant DisconnectedWorkspaceShellView
participant DeletedComputerRecoveryButton
participant CMUXMobileShellStore
participant DeviceReload
DisconnectedWorkspaceShellView->>DeletedComputerRecoveryButton: Render recovery action
DeletedComputerRecoveryButton->>CMUXMobileShellStore: recoverForgottenIrohMacFromAccount()
CMUXMobileShellStore-->>DeletedComputerRecoveryButton: Recovery result
DeletedComputerRecoveryButton->>DeviceReload: Reload paired Macs and registry devices
DeviceReload-->>DisconnectedWorkspaceShellView: Updated device state
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 23 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (23 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 |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/DeletedComputerRecoveryButton.swift`:
- Around line 48-51: Centralize account-scoped recovery attempt and status
ownership in the existing store or shared coordinator instead of each
DeletedComputerRecoveryButton instance. In
Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/DeletedComputerRecoveryButton.swift#L48-L51,
remove the per-instance recoveryTask and recoveryAttemptID state and render from
the authoritative snapshot. In
Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/DeviceTreeView.swift#L147-L154,
bind the control to that shared recovery state and action so concurrent surfaces
reflect one cancellable recovery attempt.
In
`@Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/DisconnectedWorkspaceShellView.swift`:
- Around line 145-152: The shouldAutoPresentAddDeviceAfterLoadingSavedMacs gate
must not use stale hasRecoverableDeletedComputers data after a failed refresh.
Update the load state around loadPairedMacs and
showsDeletedComputerRecoveryAction so the recovery marker is explicitly
invalidated or marked unknown when loading fails, and only suppress automatic
Add Computer after a successful current marker lookup confirms recoverable
deleted computers.
- Around line 357-361: Update reloadAfterDeletedComputerRecovery and the
DeletedComputerRecoveryButton recovery flow so successful recoveries are not
reloaded twice: rely on recoverForgottenIrohMacFromAccount for paired Mac and
registry refreshes, while preserving any refresh required when recovery fails.
In
`@Packages/iOS/CmuxMobileShellUI/Tests/CmuxMobileShellUITests/DisconnectedWorkspaceShellRecoveryTests.swift`:
- Around line 20-28: Update
recoverableDeletedComputerSuppressesAutomaticAddComputerSheet so
loadPairedMacs() runs before setting store.hasRecoverableDeletedComputers to
true, or seed a forgotten-computer marker before loading. Preserve the assertion
that shouldAutoPresentAddDeviceAfterLoadingSavedMacs is false.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b24572ce-873d-4454-a9b5-b8b5990a9f37
📒 Files selected for processing (4)
Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/DeletedComputerRecoveryButton.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/DeviceTreeView.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/DisconnectedWorkspaceShellView.swiftPackages/iOS/CmuxMobileShellUI/Tests/CmuxMobileShellUITests/DisconnectedWorkspaceShellRecoveryTests.swift
Greptile SummaryThis PR promotes the deleted-computer Iroh recovery action into the iOS disconnected/empty-state view and refactors the feature into a shared
Confidence Score: 5/5Safe to merge. The recovery logic is well-isolated on the main actor, scope-staleness checks are thorough, and the shared button correctly spans both the store-level and view-level in-progress gates. The typed result enum eliminates the old Boolean ambiguity, pairedMacLoadState prevents premature auto-add-computer on failure or mid-reload, and secondaryAggregationScopeGeneration closes a previously open team-switch window. Test coverage is solid and the prior ordering bug in the UI tests is corrected. No files require special attention beyond cosmetic issues already noted in prior review threads. Important Files Changed
Sequence DiagramsequenceDiagram
participant UI as DeletedComputerRecoveryButton
participant Store as MobileShellComposite
participant Discovery as PersonalIrohDiscovery
UI->>UI: recoverDeletedComputer()
UI->>UI: "recoveryTask = Task"
UI->>Store: await recover()
Store->>Store: guard not isRecoveringDeletedComputer
Store->>Store: "isRecoveringDeletedComputer = true"
Store->>Store: currentScopeSnapshot() / forgottenMacDeviceIDs()
Store->>Discovery: discoverLiveMacs()
Discovery-->>Store: candidates
Store->>Store: isScopeCurrent?
alt scope stale
Store-->>UI: .staleScope
Store->>Store: "defer isRecoveringDeletedComputer = false"
else candidate found
Store->>Store: connectAccountDiscoveredIrohMac
Store->>Store: loadPairedMacs + loadRegistryDevices
Store-->>UI: .recovered
Store->>Store: "defer isRecoveringDeletedComputer = false"
else no candidate
Store-->>UI: .notFound
Store->>Store: "defer isRecoveringDeletedComputer = false"
UI->>Store: await reloadAfterFailure
Note over UI: recoveryTask keeps button disabled
UI->>UI: show failure alert
UI->>UI: "defer recoveryTask = nil"
end
Reviews (5): Last reviewed commit: "Keep recovery button busy through reload" | Re-trigger Greptile |
| @Test func recoverableDeletedComputerSuppressesAutomaticAddComputerSheet() async { | ||
| let store = await shellStore() | ||
| store.hasRecoverableDeletedComputers = true | ||
| await store.loadPairedMacs() | ||
|
|
||
| let view = disconnectedView(store: store) | ||
|
|
||
| #expect(!view.shouldAutoPresentAddDeviceAfterLoadingSavedMacs) | ||
| } |
There was a problem hiding this comment.
Test will always fail:
loadPairedMacs() resets the flag it relies on
The test sets hasRecoverableDeletedComputers = true, then calls await store.loadPairedMacs(). Inside loadPairedMacs(), hasForgottenMacs is derived from forgottenMacDeviceIDs(scope:), which reads from the forgottenMacStore. The store is constructed here with the default InMemoryPairedMacForgottenStore() — empty — so hasForgottenMacs == false, and loadPairedMacs() overwrites hasRecoverableDeletedComputers = false before the assertion. view.shouldAutoPresentAddDeviceAfterLoadingSavedMacs then evaluates to true and the #expect(!...) fails. Moving store.hasRecoverableDeletedComputers = true to after the loadPairedMacs() call, or seeding the forgottenMacStore with a forgotten mac ID, fixes the ordering.
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!
| L10n.string( | ||
| "mobile.computers.recoveringDeleted", | ||
| defaultValue: "Recovering Deleted Computer..." | ||
| ) |
There was a problem hiding this comment.
The
defaultValue fallback for mobile.computers.recoveringDeleted uses three ASCII periods (...) while the string catalog entry and the code it replaced both use the Unicode horizontal ellipsis (…, U+2026). If the key is ever absent from the catalog — e.g., during localisation QA — the fallback renders a visually different string.
| L10n.string( | |
| "mobile.computers.recoveringDeleted", | |
| defaultValue: "Recovering Deleted Computer..." | |
| ) | |
| L10n.string( | |
| "mobile.computers.recoveringDeleted", | |
| defaultValue: "Recovering Deleted Computer…" | |
| ) |
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/Tests/CmuxMobileShellUITests/DisconnectedWorkspaceShellRecoveryTests.swift (1)
40-42: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDo not fall back to the shared
.standarddefaults suite.If
UserDefaults(suiteName:)returns nil, this helper silently shares application defaults with unrelated tests, making results order-dependent. Fail fixture setup instead of using.standard.As per path instructions, test-backed UserDefaults state must be isolated per test.
🤖 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/DisconnectedWorkspaceShellRecoveryTests.swift` around lines 40 - 42, Update the shellStore() test fixture to require the uniquely named UserDefaults suite and fail setup when UserDefaults(suiteName:) returns nil; remove the fallback to UserDefaults.standard so test state remains isolated.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift`:
- Line 2384: Guard the hasRecoverableDeletedComputers mutation in the loadAll
error path with isScopeCurrent(scope) before clearing it. Ensure a failed
request from an outdated account or team cannot mutate shared UI state, while
preserving the existing behavior for the current scope.
In
`@Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite`+ForgottenMacRecovery.swift:
- Around line 16-18: Update the recovery flow surrounding
isRecoveringDeletedComputer so an already-active attempt produces a distinct
“already in progress” outcome instead of false. Adjust the recovery method’s
result type and DeletedComputerRecoveryButton handling to preserve false
exclusively for actual recovery failures, while keeping the existing success and
in-progress behaviors explicit.
In
`@Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/DeletedComputerRecoveryButton.swift`:
- Around line 59-69: Update recoverDeletedComputer so the recovery operation has
explicit lifecycle ownership: store it in the shared store or a cancellable task
owned by the view, cancel it when the view disappears, and prevent
reloadAfterFailure or alertMessage updates after cancellation or deallocation.
Preserve the existing recovery and failure-message behavior while ensuring no
untracked Task remains active beyond the caller’s lifecycle.
---
Outside diff comments:
In
`@Packages/iOS/CmuxMobileShellUI/Tests/CmuxMobileShellUITests/DisconnectedWorkspaceShellRecoveryTests.swift`:
- Around line 40-42: Update the shellStore() test fixture to require the
uniquely named UserDefaults suite and fail setup when UserDefaults(suiteName:)
returns nil; remove the fallback to UserDefaults.standard so test state remains
isolated.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: c76a47db-47a4-4431-9563-05566946e1b4
📒 Files selected for processing (6)
Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite+ForgottenMacRecovery.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/DeletedComputerRecoveryButton.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/DeviceTreeView.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/DisconnectedWorkspaceShellView.swiftPackages/iOS/CmuxMobileShellUI/Tests/CmuxMobileShellUITests/DisconnectedWorkspaceShellRecoveryTests.swift
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift`:
- Around line 2384-2386: Update the load outcome handling in the loadAll flow so
a failed paired-Mac load is explicitly recorded as unavailable or failed rather
than clearing hasRecoverableDeletedComputers. On the same store, track whether
paired Macs were successfully loaded, and update
DisconnectedWorkspaceShellView.shouldAutoPresentAddDeviceAfterLoadingSavedMacs
to auto-present only after a successful known-empty load, failing closed when
loading fails or remains unresolved.
In
`@Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite`+ForgottenMacRecovery.swift:
- Around line 5-13: Add a distinct stale/cancelled case to
MobileDeletedComputerRecoveryResult and return it whenever the captured
account/team scope is no longer current during recovery, instead of returning
.notFound. Update DeletedComputerRecoveryButton to ignore this outcome without
triggering reload or the “No deleted computer was recovered” alert, while
preserving existing handling for genuine .notFound results.
In
`@Packages/iOS/CmuxMobileShell/Tests/CmuxMobileShellTests/IrohZeroTouchDiscoveryTests.swift`:
- Line 101: Add a concurrent recovery test in CmuxMobileShellTests covering
recoverForgottenIrohMacFromAccount: suspend the first discovery attempt, start
recovery, invoke a second recovery, assert the second returns
.alreadyInProgress, then resume the suspended discovery and await the first
attempt.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 355580d7-db41-431b-ab5e-fad2824f2dc8
📒 Files selected for processing (5)
Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite+ForgottenMacRecovery.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swiftPackages/iOS/CmuxMobileShell/Tests/CmuxMobileShellTests/IrohZeroTouchDiscoveryTests.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/DeletedComputerRecoveryButton.swiftPackages/iOS/CmuxMobileShellUI/Tests/CmuxMobileShellUITests/DisconnectedWorkspaceShellRecoveryTests.swift
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite`+ForgottenMacRecovery.swift:
- Line 65: Update the continuation predicate used by
connectAccountDiscoveredIrohMac in the forgotten-Mac recovery flow so it
validates the complete captured scope, including the team, rather than only the
user. Ensure persistence is skipped and .staleScope is returned when the team
changes during connection, and add a test covering that team-switch timing.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 1ec027f8-8c6e-4d2f-825c-18d5410fa7a1
📒 Files selected for processing (6)
Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite+ForgottenMacRecovery.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swiftPackages/iOS/CmuxMobileShell/Tests/CmuxMobileShellTests/IrohZeroTouchDiscoveryTests.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/DeletedComputerRecoveryButton.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/DisconnectedWorkspaceShellView.swiftPackages/iOS/CmuxMobileShellUI/Tests/CmuxMobileShellUITests/DisconnectedWorkspaceShellRecoveryTests.swift
| } | ||
| } | ||
| } | ||
| .disabled(isRecovering) |
There was a problem hiding this comment.
Button appears enabled but is unresponsive during post-failure reload
When recover() returns .notFound, the store's defer { isRecoveringDeletedComputer = false } fires immediately, so isRecovering drops to false and the button re-enables. But recoveryTask is still live running reloadAfterFailure(). Any tap during that reload hits guard !isRecovering, recoveryTask == nil → silently returns. The old DeviceTreeView code kept its local isRecoveringDeletedComputer flag true through the full reload cycle; the refactor removed that invariant. The label also stays on the idle title during the reload rather than showing the in-progress text.
| .disabled(isRecovering) | |
| .disabled(isRecovering || recoveryTask != nil) |
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite+ForgottenMacRecovery.swift (1)
35-40: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRevalidate the captured scope before mutating shared recovery state and reporting success.
There are two async gaps where the captured scope can become stale:
- After
forgottenMacDeviceIDs(scope:), the stale task can cancel the new scope’s recovery owner viaconnectionRecoveryOwner.cancel()andinvalidateStoredMacReconnectAttempt().loadPairedMacs()andloadRegistryDevices()can suspend, but the method then returns.recoveredwithout checking whether the account/team scope changed.Add scope checks before the shared-state mutations and after each reload; also cover both timing windows with regression tests.
Proposed fix
let forgottenIDs = await forgottenMacDeviceIDs(scope: scope) + guard await isScopeCurrent(scope) else { return .staleScope } guard !forgottenIDs.isEmpty else { return .notFound } connectionRecoveryOwner.cancel() applyConnectionRecoveryOwnerState() invalidateStoredMacReconnectAttempt() ... await loadPairedMacs() + guard await isScopeCurrent(scope) else { return .staleScope } await loadRegistryDevices() + guard await isScopeCurrent(scope) else { return .staleScope } return .recoveredAs per path instructions, correctness-critical recovery state must use one authoritative scope and fail closed when that scope changes.
Also applies to: 66-70
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite`+ForgottenMacRecovery.swift around lines 35 - 40, The forgotten-Mac recovery flow must revalidate its captured scope after forgottenMacDeviceIDs(scope:) before mutating shared recovery state, and after each loadPairedMacs() and loadRegistryDevices() suspension before returning .recovered. Use the authoritative current-scope check to fail closed when the scope changes, preventing stale tasks from cancelling or invalidating the new recovery owner; add regression tests covering both timing windows.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In
`@Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite`+ForgottenMacRecovery.swift:
- Around line 35-40: The forgotten-Mac recovery flow must revalidate its
captured scope after forgottenMacDeviceIDs(scope:) before mutating shared
recovery state, and after each loadPairedMacs() and loadRegistryDevices()
suspension before returning .recovered. Use the authoritative current-scope
check to fail closed when the scope changes, preventing stale tasks from
cancelling or invalidating the new recovery owner; add regression tests covering
both timing windows.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 0de695ff-88ab-4051-af91-4f38de531f45
📒 Files selected for processing (2)
Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite+ForgottenMacRecovery.swiftPackages/iOS/CmuxMobileShell/Tests/CmuxMobileShellTests/IrohZeroTouchDiscoveryTests.swift
Summary
Verification
git diff --checkjq empty ios/cmux/Resources/Localizable.xcstringsswift test --filter IrohZeroTouchDiscoveryTestsfromPackages/iOS/CmuxMobileShellxcodebuild build -workspace ios/cmux.xcworkspace -scheme CmuxMobileShellUI -destination platform=iOS Simulator,id=9369A943-7279-4969-A73D-5AA001A458AC -derivedDataPath /tmp/cmux-recov-empty-ui-ddxcodebuild build -workspace ios/cmux.xcworkspace -scheme cmux-ios -destination platform=iOS Simulator,id=9369A943-7279-4969-A73D-5AA001A458AC -derivedDataPath /tmp/cmux-recov-empty-app-build-dd CODE_SIGNING_ALLOWED=NONotes
TerminalSurfaceMountOwnershipTestscompile error on main: missingterminalFolderTapEnabled.Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by cubic
Promotes deleted-computer recovery in the iOS disconnected empty state and makes Add Computer secondary. Keeps the recovery button busy through reload, adds explicit results, and only auto‑opens Add Computer after a successful paired‑Mac load.
Bug Fixes
pairedMacLoadState == .loaded, the list is empty, and no recovery exists; hide recovery and suppress auto‑present on.failed..staleScopeon sign‑out/team switch; on.notFoundreload paired Macs + registry and show a localized error.Refactors
MobileShellComposite(isRecoveringDeletedComputer,hasRecoverableDeletedComputers,pairedMacLoadState) and maderecoverForgottenIrohMacFromAccount()returnMobileDeletedComputerRecoveryResult(.recovered,.notFound,.alreadyInProgress,.staleScope). Extracted sharedDeletedComputerRecoveryButtonandDeletedComputerRecoveryFooterused in bothDeviceTreeViewandDisconnectedWorkspaceShellView.Written for commit 946ffa2. Summary will update on new commits.
Summary by CodeRabbit