Make Tailscale setup banner dismissible - #10057
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:
📝 WalkthroughWalkthroughThe mobile shell now derives explicit Tailscale setup statuses and manages banner visibility in root-owned state. The banner supports dismissal, shell reconnect controls use that state, and iOS manual pairing reads the updated connection method immediately. ChangesTailscale setup prompt
Estimated code review effort: 3 (Moderate) | ~25 minutes Mergeability Score: 🟡 Moderate · up to While this change makes the setup banner dismissible, authorization that is still loading can be mistaken for a pairing requirement, potentially sending users into an unnecessary pairing flow. Merge should wait for that state handling to be corrected; the UI-test fixture concerns are bounded follow-up items. Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant User
participant MobileTailscalePairingRequiredBanner
participant WorkspaceListView
participant CMUXMobileRootView
participant MobileShellComposite
User->>MobileTailscalePairingRequiredBanner: tap dismiss button
MobileTailscalePairingRequiredBanner->>WorkspaceListView: invoke dismissal action
WorkspaceListView->>CMUXMobileRootView: apply prompt dismissal
CMUXMobileRootView->>MobileShellComposite: read tailscaleSetupStatus
MobileShellComposite-->>CMUXMobileRootView: return setup status
CMUXMobileRootView->>WorkspaceListView: update banner visibility
🚥 Pre-merge checks | ✅ 23 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (23 passed)
✨ Finishing Touches 💡 1📝 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.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/CMUXMobileRootView.swift`:
- Around line 447-450: Extract the duplicated isTailscalePairingBannerDismissed
assignment into one private method on the containing view, then replace the
dismiss closures passed to DisconnectedWorkspaceShellView and WorkspaceShellHost
with calls to that method.
🪄 Autofix
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: 5e434b80-f461-487d-acbf-84dc8d4a2587
📒 Files selected for processing (7)
Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/CMUXMobileRootView.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/DisconnectedWorkspaceShellView.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobileTailscalePairingRequiredBanner.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceListView.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceShellHost.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceShellView.swiftios/cmuxUITests/cmuxUITests.swift
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 (3)
Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobileRootPresentationState.swift (1)
125-130: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftDo not infer pairing from a missing authorization result.
Line 130 converts
hasUsableAuthorization == falseintorequiresPairing == true. That result is also false while a known paired Mac is still loading. The shell reports that case as.loadingAuthorization.The root then opens the scanner and latches the banner as required. The prompt state preserves that requirement through loading. If the load later finds a valid grant, the user can remain in an unnecessary pairing flow.
Pass
MobileTailscaleSetupStatusthrough this transition. Open pairing only for.pairingRequired. Select Tailscale and wait for the authoritative load result for.loadingAuthorization.As per coding guidelines, “Correctness-critical facts must come from one reliable structured source.” As per path instructions, “For the Tailscale banner, pairing readiness, dismissal, and reconnect enablement, use one authoritative typed state source rather than independently reading potentially stale store flags.”
🤖 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/MobileRootPresentationState.swift` around lines 125 - 130, Update the setUpTailscale transition in MobileRootPresentationState to accept and use MobileTailscaleSetupStatus as the authoritative state source instead of inferring pairing from hasUsableAuthorization. Open the scanner and return requiresPairing only for .pairingRequired; for .loadingAuthorization, select Tailscale and wait for the authoritative load result, while preserving the existing behavior for an authorized state.Sources: Coding guidelines, Path instructions
ios/cmuxUITests/cmuxUITests.swift (2)
2583-2604: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRequire a real connection-state transition.
Assert that
MobileTerminalMacConnectionStatusisConnectedbeforeserver.stop(), then wait forReconnectingorDisconnected. The terminal fixture does not prove the status element's state, so a pre-existing destination state could satisfy the predicate without proving the server caused the transition.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ios/cmuxUITests/cmuxUITests.swift` around lines 2583 - 2604, Update the test around MobileTerminalMacConnectionStatus to assert its label is Connected before calling server.stop(). After stopping the server, retain the wait for Reconnecting or Disconnected so the test verifies an actual connection-state transition before deleting the saved computer.
2567-2581: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winIsolate the persistent UI-test fixture state.
Manual pairing clears
ui-test-mac’s hidden marker, so the unconditional tap is not the reported issue. However,setUpWithError()does not reset.standardUserDefaults or paired-Mac state. Clear or isolate that state because the fixture assumesui-test-macis the only saved computer.🤖 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 `@ios/cmuxUITests/cmuxUITests.swift` around lines 2567 - 2581, Update setUpWithError() and the launchAppAfterForgettingFinalComputer() fixture setup to clear or isolate shared UserDefaults and paired-Mac state before launching the app. Ensure the fixture starts with only ui-test-mac saved, without relying on state left by previous UI tests, while preserving the existing manual-pairing flow.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 `@ios/cmuxUITests/cmuxUITests.swift`:
- Around line 2583-2604: Update the test around
MobileTerminalMacConnectionStatus to assert its label is Connected before
calling server.stop(). After stopping the server, retain the wait for
Reconnecting or Disconnected so the test verifies an actual connection-state
transition before deleting the saved computer.
- Around line 2567-2581: Update setUpWithError() and the
launchAppAfterForgettingFinalComputer() fixture setup to clear or isolate shared
UserDefaults and paired-Mac state before launching the app. Ensure the fixture
starts with only ui-test-mac saved, without relying on state left by previous UI
tests, while preserving the existing manual-pairing flow.
In
`@Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobileRootPresentationState.swift`:
- Around line 125-130: Update the setUpTailscale transition in
MobileRootPresentationState to accept and use MobileTailscaleSetupStatus as the
authoritative state source instead of inferring pairing from
hasUsableAuthorization. Open the scanner and return requiresPairing only for
.pairingRequired; for .loadingAuthorization, select Tailscale and wait for the
authoritative load result, while preserving the existing behavior for an
authorized state.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 7e8d8e3a-8eb4-4a5f-819e-1367e159cf7b
📒 Files selected for processing (11)
Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite+ReconnectRoutes.swiftPackages/iOS/CmuxMobileShell/Tests/CmuxMobileShellTests/ReconnectRouteSelectionTests.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/CMUXMobileRootView.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/DisconnectedWorkspaceShellView.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobileRootPresentationState.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobileTailscaleSetupPromptState.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceListView.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceShellHost.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceShellView.swiftPackages/iOS/CmuxMobileShellUI/Tests/CmuxMobileShellUITests/MobileRootPresentationStateTests.swiftios/cmuxUITests/cmuxUITests.swift
1 participant
Adds a close control to the iOS Tailscale setup banner.
The first commit is the red behavior test; the second commit implements the fix.
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by cubic
Makes the iOS Tailscale setup banner dismissible and defers opening the pairing scanner until authorization status is known. Previously the banner always persisted and migration could open pairing too early; now users can dismiss it for the session, it returns on relaunch until local authorization succeeds, and reconnect is suppressed while pairing is required.
MobileTailscalePairingRequiredDismiss;MobileTailscalePairingRequiredBannernow requires adismissaction.MobileTailscaleSetupStatus(notSelected,loadingAuthorization,pairingRequired,authorized) and derivestailscalePairingRequiredfrom it.MobileTailscaleSetupPromptStateinCMUXMobileRootViewto track loading/required/authorized and dismissal; latches “required” when selecting Tailscale during migration and follows shell status changes.setUpTailscale(status: MobileTailscaleSetupStatus); only opens the scanner whenstatus == .pairingRequiredto avoid pairing while authorization is still loading.showsTailscalePairingBanner/dismissTailscalePairingBannerthroughWorkspaceShellHost,WorkspaceShellView,WorkspaceListView, andDisconnectedWorkspaceShellViewto gate reconnect while keeping pairing/Settings available without flicker.Written for commit f635439. Summary will update on new commits.
Summary by CodeRabbit
New Features
Tests