Recover account Macs deleted before iOS recovery support - #8757
azooz2003-bit wants to merge 2 commits into
Conversation
📝 WalkthroughWalkthroughAdds account-scoped Iroh Mac recovery for iOS, replacing the deleted-computer recovery API and UI. Recovery now supports markerless discovery, scoped candidate filtering, concurrent-run protection, mode-specific localized messaging, and updated unit, UI, and end-to-end tests. ChangesAccount computer recovery
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant AccountComputerRecoveryButton
participant MobileShellComposite
participant MobileIrohMacDiscovering
participant IrohMac
User->>AccountComputerRecoveryButton: Tap account recovery
AccountComputerRecoveryButton->>MobileShellComposite: recoverIrohMacFromAccount()
MobileShellComposite->>MobileIrohMacDiscovering: discoverLiveMacs()
MobileIrohMacDiscovering-->>MobileShellComposite: Eligible live Macs
MobileShellComposite->>IrohMac: Connect and persist pairing
IrohMac-->>MobileShellComposite: Recovery result
MobileShellComposite-->>AccountComputerRecoveryButton: recovered, notFound, or staleScope
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 24 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (24 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/CmuxMobileShellUI/Sources/CmuxMobileShellUI/AccountComputerRecoveryButton.swift (1)
67-80: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDo not present
.unavailableas a recovery action.
.unavailablecurrently renders “Find Account Computer”; recovery then returns.notFoundwhen discovery is unavailable and shows a misleading “No account computer was found” alert. Hide/disable this section for.unavailablerather than mapping it to the find-account copy.Also applies to: 91-117, 128-163
🤖 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/AccountComputerRecoveryButton.swift` around lines 67 - 80, Update the recovery UI and action flow around recoverAccountComputer and its related rendering sections so Account Computer recovery is hidden or disabled when the state is .unavailable. Do not map .unavailable to the “Find Account Computer” copy or invoke recovery; preserve the existing behavior for discoverable and other supported states.
🤖 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/AccountComputerRecoveryButton.swift`:
- Around line 67-80: Update the recovery UI and action flow around
recoverAccountComputer and its related rendering sections so Account Computer
recovery is hidden or disabled when the state is .unavailable. Do not map
.unavailable to the “Find Account Computer” copy or invoke recovery; preserve
the existing behavior for discoverable and other supported states.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 0e309d2d-1b54-4d98-9f7c-0f4b744b4350
📒 Files selected for processing (10)
Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite+AccountMacRecovery.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite+ForgottenMacRecovery.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swiftPackages/iOS/CmuxMobileShell/Tests/CmuxMobileShellTests/IrohZeroTouchDiscoveryTests.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/AccountComputerRecoveryButton.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/DeviceTreeView.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/DisconnectedWorkspaceShellView.swiftPackages/iOS/CmuxMobileShellUI/Tests/CmuxMobileShellUITests/DisconnectedWorkspaceShellRecoveryTests.swiftios/cmux/Resources/Localizable.xcstringsios/cmuxUITests/cmuxUITests.swift
💤 Files with no reviewable changes (1)
- Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite+ForgottenMacRecovery.swift
Greptile SummaryThis PR broadens the iOS account Mac recovery surface: where recovery was previously only exposed when a local "forgotten Mac" deletion marker existed, it is now available whenever the signed-in personal-account Iroh discovery service is present. The deleted-marker path is preserved for copy differentiation and passive-discovery suppression; the new markerless path lets a user find any same-account Mac that was deleted before recovery support was introduced or paired under a different install scope.
Confidence Score: 4/5Safe to merge after fixing the mismatched failure alert title in the find-account flow. The core recovery logic, scope guards, deduplication, and i18n additions are all correct and well-tested. The one concrete defect is that failureTitle in AccountComputerRecoveryButton is not mode-switched, so the findAccountComputer path surfaces a Couldnt recover computer dialog title that contradicts the Find Account Computer label the user just tapped. AccountComputerRecoveryButton.swift needs failureTitle mode-switched; DisconnectedWorkspaceShellView.swift has a semantically inverted nil-store check in showsAccountComputerRecoveryAction. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[User taps recovery button] --> B{accountComputerRecoveryMode}
B -->|unavailable| Z[Button hidden]
B -->|recoverDeletedComputer| C[recoverIrohMacFromAccount]
B -->|findAccountComputer| C
C --> D{isRecoveringAccountComputer?}
D -->|yes| E[returns alreadyInProgress]
D -->|no| F[Set flag = true]
F --> G[Snapshot scope + forgottenIDs + knownIDs]
G --> H[personalIrohDiscovery.discoverLiveMacs]
H --> I{scope still current?}
I -->|no| J[returns staleScope]
I -->|yes| K[accountIrohRecoveryCandidates forgotten first then unpaired]
K --> L[For each candidate connectAccountDiscoveredIrohMac]
L --> M{connected?}
M -->|yes| N[loadPairedMacs + loadRegistryDevices returns recovered]
M -->|no| L
L -->|exhausted| O[returns notFound show alert]
Reviews (1): Last reviewed commit: "fix(ios): recover account Macs without l..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| @@ -108,24 +125,42 @@ struct DeletedComputerRecoveryButton: View { | |||
| } | |||
There was a problem hiding this comment.
.findAccountComputer flow
failureTitle is not mode-switched and always returns "Couldn't recover computer" (mobile.computers.recoverFailedTitle). When the user taps "Find Account Computer" and no Mac is found, the failure dialog reads "Couldn't recover computer" — a description that only fits the .recoverDeletedComputer path. failureMessage is already correctly mode-switched; failureTitle needs the same treatment, with a new "mobile.computers.findAccountFailedTitle" catalog key (e.g., "Couldn't find computer") for the .findAccountComputer / .unavailable branch.
| var showsAccountComputerRecoveryAction: Bool { | ||
| store?.accountComputerRecoveryMode != .unavailable | ||
| } |
There was a problem hiding this comment.
nil store makes showsAccountComputerRecoveryAction return true
store?.accountComputerRecoveryMode != .unavailable evaluates to true when store is nil, because Swift compares Optional.none as not-equal to any .some value. The old code (store?.hasRecoverableDeletedComputers == true) correctly returned false for a nil store. The current guards (if showsAccountComputerRecoveryAction, let store and the guard let store else { return false } in shouldAutoPresentAddDeviceAfterLoadingSavedMacs) prevent any visible bug today, but the semantic inversion could silently produce wrong behavior if a future caller checks showsAccountComputerRecoveryAction before binding store. The safer pattern is store?.accountComputerRecoveryMode.map { $0 != .unavailable } ?? false.
| @Test func failedPairedMacLoadStillOffersAccountRecovery() async throws { | ||
| let store = try await shellStore( | ||
| pairedMacStore: FailingLoadPairedMacStore(), | ||
| personalIrohDiscovery: EmptyAccountIrohDiscovery() | ||
| ) | ||
| store.hasRecoverableDeletedComputers = true | ||
|
|
||
| await store.loadPairedMacs() | ||
| let view = disconnectedView(store: store) | ||
|
|
||
| #expect(store.pairedMacLoadState == .failed) | ||
| #expect(!view.showsDeletedComputerRecoveryAction) | ||
| #expect(store.accountComputerRecoveryMode == .findAccountComputer) | ||
| #expect(view.showsAccountComputerRecoveryAction) | ||
| #expect(!view.shouldAutoPresentAddDeviceAfterLoadingSavedMacs) | ||
| } |
There was a problem hiding this comment.
store.hasRecoverableDeletedComputers = true is set before await store.loadPairedMacs(), but the FailingLoadPairedMacStore causes that load to fail and reset hasRecoverableDeletedComputers to false. The subsequent assertion accountComputerRecoveryMode == .findAccountComputer proves the flag was reset. The true assignment has no effect on the observable outcome and misleads a reader into thinking the flag survives a failed load — which is the opposite of what the test verifies. Removing the dead-code line (or moving it after loadPairedMacs to make the scenario deliberate) would clarify the intent.
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!
lawrencecchen
added
the
stale-revisit
3 participants
Problem
CMUX Internal only exposed account recovery when the current install had a local forgotten-Mac marker. Macs deleted before recovery support, or in another install scope, had no action even though the signed-in account broker could still discover them.
Fix
Verification
swift test --filter IrohZeroTouchDiscoveryTests(18 passed)testMarkerlessAccountRecoveryIsVisibleAndActionableXCUITest (passed)The full iOS test plan remains blocked by the existing unrelated
WorkspaceMacSelectionTestscompile error for missingmacTitlePickerSelection; the focused UI target passes when isolated.Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by cubic
Enable account-based Mac recovery on iOS even without a local deletion marker by scanning live same‑account Iroh devices, improving recovery for Macs deleted before recovery support or across installs.
New Features
recoverIrohMacFromAccount()toMobileShellCompositeto scan same‑account Iroh and authenticate device ID + instance tag before persisting; prioritizes forgotten Macs, then unpaired; returnsMobileAccountComputerRecoveryResultand handles scope changes and in‑progress scans.accountComputerRecoveryModeto drive UI availability and copy; keep forgotten markers for passive suppression and messaging.AccountComputerRecoveryButtonand footer that adapt to mode; shown in Disconnected and Device Tree whenpersonalIrohDiscoveryexists; added EN/JA strings and new accessibility idMobileAccountComputerRecoveryButton.Refactors
recoverForgottenIrohMacFromAccount()andisRecoveringDeletedComputer; addedisRecoveringAccountComputer.DeletedComputerRecovery*views toAccountComputerRecovery*; removedMobileShellComposite+ForgottenMacRecovery.swiftand addedMobileShellComposite+AccountMacRecovery.swift.Written for commit 7db653e. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes
Tests