Repository navigation
Fix iOS deleted Iroh Mac recovery - #8683
Conversation
📝 WalkthroughWalkthroughChangesDeleted Computer Recovery
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant DeviceTreeView
participant MobileShellComposite
participant IrohDiscovery
participant ConnectionRecovery
participant PairedMacStore
DeviceTreeView->>MobileShellComposite: Start deleted-computer recovery
MobileShellComposite->>IrohDiscovery: Discover live account Macs
IrohDiscovery-->>MobileShellComposite: Return forgotten Mac candidates
MobileShellComposite->>ConnectionRecovery: Connect with instance tag
ConnectionRecovery-->>MobileShellComposite: Return connection result
MobileShellComposite->>PairedMacStore: Load paired Macs and registry devices
MobileShellComposite-->>DeviceTreeView: Return recovery result
🚥 Pre-merge checks | ✅ 25✅ Passed checks (25 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 |
Greptile SummaryThis PR adds explicit user-initiated recovery of deleted (forgotten) Iroh Macs on iOS. Passive zero-touch discovery continues to skip forgotten Macs; this new path is the only way to bring one back without re-pairing.
Confidence Score: 4/5Safe to merge with one fix: the connection recovery owner is cancelled before live candidates are confirmed, and a zero-candidates failure leaves any in-progress Mac reconnect silently dropped. The recovery flow, instance-tag enforcement, forgotten-marker lifecycle, and localization are all correctly implemented. The one concrete issue is in recoverForgottenIrohMacFromAccount: connectionRecoveryOwner.cancel() + invalidateStoredMacReconnectAttempt() fire before discoverLiveMacs() is called. If no live candidates match the forgotten ID, the function returns false having already abandoned any in-progress reconnect to an existing (non-forgotten) Mac. Natural re-arming via network-path changes or presence pushes will eventually restore it, but the user sees their primary Mac drop from reconnecting to disconnected as a side effect of tapping a button that found nothing to recover. Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite+ForgottenMacRecovery.swift — the early cancel/invalidate before candidates are confirmed. Important Files Changed
Sequence DiagramsequenceDiagram
participant UI as DeviceTreeView
participant Shell as MobileShellComposite
participant Discovery as PersonalIrohDiscovery
UI->>Shell: recoverForgottenIrohMacFromAccount()
Shell->>Shell: forgottenMacDeviceIDs(scope)
Shell->>Shell: connectionRecoveryOwner.cancel()
Shell->>Shell: invalidateStoredMacReconnectAttempt()
Shell->>Discovery: discoverLiveMacs()
Discovery-->>Shell: [MobileDiscoveredIrohMac]
Shell->>Shell: forgottenIrohRecoveryCandidates(discovered, forgottenIDs)
loop For each candidate
Shell->>Shell: isForgottenMacDeviceID(mac, scope)
Shell->>Shell: connectAccountDiscoveredIrohMac(mac, accountID)
alt Connected and authenticated
Shell->>Shell: "loadPairedMacs() → hasRecoverableDeletedComputers=false"
Shell->>Shell: loadRegistryDevices()
Shell-->>UI: true
else Failed
Shell->>Shell: continue to next candidate
end
end
Shell-->>UI: false
UI->>Shell: reload()
UI->>UI: show failure alert or dismiss spinner
Reviews (2): Last reviewed commit: "fix(ios): tighten deleted Mac recovery r..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| guard forgottenIDs.contains(cmxCanonicalDeviceID(mac.deviceID)) | ||
| || forgottenIDs.contains(pairingID), | ||
| !mac.routes.isEmpty, | ||
| mac.routes.allSatisfy({ $0.kind == .iroh }), | ||
| seen.insert(pairingID).inserted else { continue } |
There was a problem hiding this comment.
allSatisfy(.iroh) route guard is stricter than connectAccountDiscoveredIrohMac's acceptance check
forgottenIrohRecoveryCandidates requires every route to be .iroh, but connectAccountDiscoveredIrohMac only requires at least one Iroh route after storedReconnectRoutes filtering (contains(where: { $0.kind == .iroh })). The fact that connectAccountDiscoveredIrohMac passes mac.routes through storedReconnectRoutes at all implies MobileDiscoveredIrohMac can carry mixed-kind routes. A live forgotten Mac that advertises both an Iroh route and a direct/loopback route would pass connectAccountDiscoveredIrohMac's gate but be dropped here, causing the recovery to silently return false even though a valid Iroh path exists. Changing the guard to mac.routes.contains(where: { $0.kind == .iroh }) aligns the two checks.
| Task { | ||
| let recovered = await store.recoverForgottenIrohMacFromAccount() | ||
| await reload() | ||
| isRecoveringDeletedComputer = false | ||
| if !recovered { | ||
| recoveryAlertMessage = L10n.string( | ||
| "mobile.computers.recoverFailedMessage", | ||
| defaultValue: "No deleted computer was recovered. Open cmux on the Mac, sign in to this same account, and try again." | ||
| ) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Task {} outlives the view and fires connection-state side effects after dismissal
recoverDeletedComputer() launches an unstructured Task that is not tied to the view's lifetime. If the user dismisses the Computers screen while recovery is in progress, recoverForgottenIrohMacFromAccount() continues running: it has already cancelled connectionRecoveryOwner and may complete a full Iroh dial and persist a new Mac record. The resulting reload() (which calls loadPairedMacs() and loadRegistryDevices()) also fires into the dismissed view's store. None of this crashes, but the user sees no feedback and may be surprised that their active reconnect was silently cancelled by a navigation gesture. Storing the task handle and cancelling it onDisappear, or switching to .task(id:) on a trigger value, would scope the side-effectful work to the screen's visible lifetime.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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:
- Around line 47-51: Update isForgottenMacDeviceID to canonicalize the supplied
device ID before performing its direct lookup, matching the normalization used
during candidate selection and preserving legacy marker handling. Add a
regression test covering an uppercase UUID with a legacy canonical marker,
verifying the forgotten-device revalidation succeeds.
- Around line 80-84: Update the candidate filter in the forgotten-Mac recovery
flow to accept Macs with at least one Iroh route, rather than requiring every
route to be Iroh. Preserve the existing non-empty route check, forgotten-ID
matching, and duplicate suppression via seen.insert(pairingID), while ensuring
candidates without any Iroh route remain excluded.
- Around line 55-59: Update the recovery flow surrounding the ifStillCurrent
closure to capture secondaryAggregationScopeGeneration before discovery, then
require the current generation to match the captured value alongside the
existing sign-in and user-ID checks. Ensure recovery returns false when the team
scope changes while the dial is awaiting, preventing the stale scope from
connecting or persisting.
In
`@Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/DeviceTreeView.swift`:
- Around line 248-250: Remove the unconditional reload() call after
recoverForgottenIrohMacFromAccount() succeeds in the recovery flow, since that
method already refreshes paired Macs and registry devices. Preserve the
isRecoveringDeletedComputer state reset and only retain a conditional reload if
a specific result path requires it.
- Around line 184-186: Update the recovery and deletion messaging to clearly
identify this phone as the recovery-action surface: revise the fallback strings
in DeviceTreeView, MacComputerDetailView, and both MacComputerRow fallbacks,
plus the corresponding English and catalog entries for
mobile.computers.removeMessage,
mobile.computers.removeMessageRepresentativeFormat, and
mobile.computers.recoverDeletedFooter in
Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/DeviceTreeView.swift
(184-186),
Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MacComputerDetailView.swift
(247),
Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MacComputerRow.swift
(176-181), and ios/cmux/Resources/Localizable.xcstrings (1506-1512, 1523-1529,
1574-1580); add “on this phone” or equivalent “here” wording while preserving
the existing instructions.
- Around line 243-257: Update recoverDeletedComputer to capture the
authoritative current scope, store the recovery Task for cancellation, and
cancel it when the view lifecycle or scope changes. Before applying reload
results or mutating isRecoveringDeletedComputer and recoveryAlertMessage, verify
the task and scope are still current; otherwise discard the result. Treat
recovered == false as a failure only for the current scope.
🪄 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: 8c56afd6-06ba-46e0-a796-9076b41eb997
📒 Files selected for processing (8)
Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite+ConnectionRecovery.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite+ForgottenMacRecovery.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swiftPackages/iOS/CmuxMobileShell/Tests/CmuxMobileShellTests/IrohZeroTouchDiscoveryTests.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/DeviceTreeView.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MacComputerDetailView.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MacComputerRow.swiftios/cmux/Resources/Localizable.xcstrings
| guard await isForgottenMacDeviceID( | ||
| mac.deviceID, | ||
| instanceTag: mac.instanceTag, | ||
| scope: scope | ||
| ) else { continue } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Canonicalize the forgotten-ID revalidation.
Candidate selection canonicalizes mac.deviceID, but isForgottenMacDeviceID first compares the raw ID. A legacy canonical marker and differently cased UUID can be selected at Line 80, then rejected here. Normalize the direct device-ID lookup in isForgottenMacDeviceID and add an uppercase-UUID recovery regression 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/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite`+ForgottenMacRecovery.swift
around lines 47 - 51, Update isForgottenMacDeviceID to canonicalize the supplied
device ID before performing its direct lookup, matching the normalization used
during candidate selection and preserving legacy marker handling. Add a
regression test covering an uppercase UUID with a legacy canonical marker,
verifying the forgotten-device revalidation succeeds.
| ifStillCurrent: { [weak self] in | ||
| guard let self else { return false } | ||
| return self.isSignedIn | ||
| && self.identityProvider?.currentUserID == scope.userID | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Keep the dial bound to the captured team scope.
This guard only validates the account. If the team changes while the dial awaits, the old-scope recovery can still connect and persist. Capture secondaryAggregationScopeGeneration before discovery and require it here so the attempt fails closed after a scope transition.
Proposed fix
+ let recoveryScopeGeneration = secondaryAggregationScopeGeneration
let forgottenIDs = await forgottenMacDeviceIDs(scope: scope)
...
return self.isSignedIn
&& self.identityProvider?.currentUserID == scope.userID
+ && self.secondaryAggregationScopeGeneration == recoveryScopeGenerationAs per path instructions, recovery eligibility and scope/team identity must come from authoritative scope data and fail closed when it changes.
📝 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.
| ifStillCurrent: { [weak self] in | |
| guard let self else { return false } | |
| return self.isSignedIn | |
| && self.identityProvider?.currentUserID == scope.userID | |
| } | |
| ifStillCurrent: { [weak self] in | |
| guard let self else { return false } | |
| return self.isSignedIn | |
| && self.identityProvider?.currentUserID == scope.userID | |
| && self.secondaryAggregationScopeGeneration == recoveryScopeGeneration | |
| } |
🤖 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 55 - 59, Update the recovery flow surrounding the ifStillCurrent
closure to capture secondaryAggregationScopeGeneration before discovery, then
require the current generation to match the captured value alongside the
existing sign-in and user-ID checks. Ensure recovery returns false when the team
scope changes while the dial is awaiting, preventing the stale scope from
connecting or persisting.
Source: Path instructions
| Text(L10n.string( | ||
| "mobile.computers.recoverDeletedFooter", | ||
| defaultValue: "Deleted computers stay hidden on this phone. To recover one, open cmux on that Mac, sign in to this same account, then tap Recover Deleted Computer." |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Identify the phone as the recovery-action surface.
The shared wording tells users to open cmux on the Mac and then tap “Recover Deleted Computer,” which can imply that the tap happens in the Mac app. Add “on this phone”/“here” to the fallback strings and both catalog locales.
Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/DeviceTreeView.swift#L184-L186: update the recovery footer.Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MacComputerDetailView.swift#L247-L247: update the detail-view fallback.Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MacComputerRow.swift#L176-L181: update both row fallbacks.ios/cmux/Resources/Localizable.xcstrings#L1506-L1512: updatemobile.computers.removeMessage.ios/cmux/Resources/Localizable.xcstrings#L1523-L1529: updatemobile.computers.removeMessageRepresentativeFormat.ios/cmux/Resources/Localizable.xcstrings#L1574-L1580: updatemobile.computers.recoverDeletedFooter.
📍 Affects 4 files
Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/DeviceTreeView.swift#L184-L186(this comment)Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MacComputerDetailView.swift#L247-L247Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MacComputerRow.swift#L176-L181ios/cmux/Resources/Localizable.xcstrings#L1506-L1512ios/cmux/Resources/Localizable.xcstrings#L1523-L1529ios/cmux/Resources/Localizable.xcstrings#L1574-L1580
🤖 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/DeviceTreeView.swift`
around lines 184 - 186, Update the recovery and deletion messaging to clearly
identify this phone as the recovery-action surface: revise the fallback strings
in DeviceTreeView, MacComputerDetailView, and both MacComputerRow fallbacks,
plus the corresponding English and catalog entries for
mobile.computers.removeMessage,
mobile.computers.removeMessageRepresentativeFormat, and
mobile.computers.recoverDeletedFooter in
Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/DeviceTreeView.swift
(184-186),
Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MacComputerDetailView.swift
(247),
Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MacComputerRow.swift
(176-181), and ios/cmux/Resources/Localizable.xcstrings (1506-1512, 1523-1529,
1574-1580); add “on this phone” or equivalent “here” wording while preserving
the existing instructions.
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)
26-47: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPropagate Task cancellation through forgotten Mac recovery.
recoverForgottenIrohMacFromAccount()never checksTask.isCancelled, and the closure passed intoconnectAccountDiscoveredIrohMac(...)doesn’t either. After cancelling the UI task, discovery/re-selection can still dial and persist a pairing, then remove the forgotten marker. Add a cancellation guard after each suspension point and include!Task.isCancelledinifStillCurrent.🤖 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 26 - 47, The forgotten-Mac recovery flow in recoverForgottenIrohMacFromAccount must stop after cancellation: add Task.isCancelled guards immediately after each awaited suspension point before continuing discovery, candidate selection, validation, or recovery. Update the ifStillCurrent closure passed to connectAccountDiscoveredIrohMac to also require !Task.isCancelled, preventing dialing, persistence, and forgotten-marker removal after the task is cancelled.Sources: Coding guidelines, 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 26-47: The forgotten-Mac recovery flow in
recoverForgottenIrohMacFromAccount must stop after cancellation: add
Task.isCancelled guards immediately after each awaited suspension point before
continuing discovery, candidate selection, validation, or recovery. Update the
ifStillCurrent closure passed to connectAccountDiscoveredIrohMac to also require
!Task.isCancelled, preventing dialing, persistence, and forgotten-marker removal
after the task is cancelled.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c9cd6dab-adf3-410e-9d03-fc1d2faf41d6
📒 Files selected for processing (3)
Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite+ForgottenMacRecovery.swiftPackages/iOS/CmuxMobileShell/Tests/CmuxMobileShellTests/IrohZeroTouchDiscoveryTests.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/DeviceTreeView.swift
| connectionRecoveryOwner.cancel() | ||
| applyConnectionRecoveryOwnerState() | ||
| invalidateStoredMacReconnectAttempt() | ||
|
|
||
| let discovered = await personalIrohDiscovery.discoverLiveMacs() | ||
| guard await isScopeCurrent(scope) else { return false } | ||
| let candidates = forgottenIrohRecoveryCandidates( | ||
| from: discovered, | ||
| forgottenIDs: forgottenIDs | ||
| ) |
There was a problem hiding this comment.
connectionRecoveryOwner cancelled before candidates exist
connectionRecoveryOwner.cancel() + invalidateStoredMacReconnectAttempt() fire before discoverLiveMacs() is even called. If discovery returns no matching candidates (Mac B is offline or was truly deleted), the function returns false with no rollback — any in-progress connectionRecoveryOwner attempt for a non-forgotten Mac (Mac A momentarily dropped its connection while Mac B's forgotten record persists) is silently abandoned. The next natural re-arm comes from a network-path change or presence push, but the disruption is observable: the UI goes from "reconnecting" to "disconnected" even though the user's primary Mac is still reachable.
Moving the three-line cancel block to after guard !candidates.isEmpty (or after the first connectAccountDiscoveredIrohMac is determined to be worth calling) scopes the slot takeover to when a real connection attempt is imminent, which matches the beginPairingAttempt() pattern everywhere else.
1 participant
Summary
Verification
swift test --filter IrohZeroTouchDiscoveryTestsgit diff --check origin/main...HEADjq empty ios/cmux/Resources/Localizable.xcstringsen,jacoverage.CMUX_PORT=3842 CMUX_PORT_RANGE=10 CMUX_PORT_END=3851 ./scripts/reload.sh --tag irecov --launch --swift-frontend-workaroundCMUX_PORT=3842 CMUX_PORT_RANGE=10 CMUX_PORT_END=3851 ./ios/scripts/reload.sh --tag irecov --simulator cmux-irecov-0722CMUX_PORT=3842 CMUX_PORT_RANGE=10 CMUX_PORT_END=3851 ./scripts/mobile-dev-launch.sh --tag irecov --simulator cmux-irecov-0722 --ensure-machttp://127.0.0.1:3842/,/handler/sign-in, and/handler/after-sign-in.Dogfood
Tagged macOS build:
http://127.0.0.1:17320/irecoviOS tag:
irecovon simulatorcmux-irecov-0722(0A967C23-FCCE-4140-B0D9-EF9A4C63A0F8).Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by cubic
Enables explicit recovery of deleted Iroh Macs on iOS via same-account discovery. Adds a Recover Deleted Computer action and keeps zero-touch discovery from auto-reviving forgotten Macs.
.irohroutes and matching instance tag, accept mixed-route candidates only when.irohexists (persist.iroh), connect via stored-Mac path, and clear the marker only after authenticated save.hasRecoverableDeletedComputersper scope, set on paired‑Mac load, reset on sign-out and data reload; addedconnectAccountDiscoveredIrohMac.en/jastrings for the recover button, footer, progress, and alert copy.Written for commit 1f78f1c. Summary will update on new commits.
Summary by CodeRabbit
New Features
Documentation
Tests