Repository navigation
iOS: show which features a Mac update unlocks when the connected Mac is older - #7960
Conversation
The banner's inputs (host capabilities, resolved Mac version) and the computed gap signature are otherwise invisible when diagnosing why the indicator did or did not show; sync.transport already sets the precedent. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
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 Mac version parsing, capability-based update hint evaluation, per-device dismissal persistence, host synchronization, debug controls, localized SwiftUI presentation, and test coverage. ChangesMac update hint
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant MobileHostService
participant MobileShellComposite
participant MobileMacUpdateAdvisor
participant MobileMacUpdateHintDismissalStore
participant WorkspaceShellView
participant MacUpdateHintIndicatorButton
MobileHostService->>MobileShellComposite: provide capabilities and Mac version
MobileShellComposite->>MobileMacUpdateAdvisor: calculate update hint
MobileMacUpdateAdvisor-->>MobileShellComposite: return hint or nil
MobileShellComposite->>MobileMacUpdateHintDismissalStore: check dismissal signature
MobileShellComposite-->>WorkspaceShellView: expose current hint
WorkspaceShellView->>MacUpdateHintIndicatorButton: render toolbar indicator
MacUpdateHintIndicatorButton->>MobileShellComposite: dismiss hint
MobileShellComposite->>MobileMacUpdateHintDismissalStore: persist dismissal
Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (2 errors, 1 warning)
✅ Passed checks (22 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 an iOS hint that explains which features a Mac update unlocks. The main changes are:
Confidence Score: 5/5This looks safe to merge.
Important Files Changed
Reviews (10): Last reviewed commit: "Share one injectable dismissal store acr..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| return | ||
| } | ||
|
|
||
| let resolvedMacDeviceID = macDeviceID ?? "unknown" |
There was a problem hiding this comment.
When both the status response and attach ticket lack a device ID, every Mac uses the persistent key for "unknown". Dismissing a gap on one unidentified Mac therefore suppresses the same gap on another unidentified Mac, breaking the store's per-Mac contract; avoid persisting until a stable host identity is available.
Rule Used: Flag correctness-critical detection/identity deriv... (source)
| public internal(set) var macUpdateHint: MobileMacUpdateHint? | ||
| @ObservationIgnored var macUpdateHintMacDeviceID: String? | ||
| @ObservationIgnored var macUpdateHintShownSignatures: Set<String> = [] |
There was a problem hiding this comment.
The foreground-host switch replaces activeTicket and supportedHostCapabilities without clearing this new connection-scoped state. Until the new host returns another status response, Mac A's banner can appear under Mac B's name, and a quick dismissal is persisted for Mac A; clear the hint during the host transition before rebuilding it from Mac B's status.
Rule Used: Flag Swift fixes that patch symptoms while leaving... (source)
|
|
||
| macUpdateHint = hint | ||
| macUpdateHintMacDeviceID = resolvedMacDeviceID | ||
| guard macUpdateHintShownSignatures.insert(hint.dismissalSignature).inserted else { return } |
There was a problem hiding this comment.
The shown-event gate uses only the gap signature. If Mac A and Mac B have the same missing capabilities, both banners appear but only Mac A emits ios_mac_update_hint_shown, so multi-Mac sessions are undercounted.
| guard macUpdateHintShownSignatures.insert(hint.dismissalSignature).inserted else { return } | |
| let shownSignature = "\(resolvedMacDeviceID):\(hint.dismissalSignature)" | |
| guard macUpdateHintShownSignatures.insert(shownSignature).inserted else { return } |
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!
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@ios/cmux/Resources/Localizable.xcstrings`:
- Around line 3863-3877: Update the localized values for
mobile.macUpdateHint.dismiss in the en and ja stringUnit entries to describe
suppression for the current feature gap, such as “these features,” rather than
the entire Mac version. Preserve the existing dismissal persistence behavior and
translated states.
In
`@Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileMacUpdateCapabilityRequirement.swift`:
- Around line 35-60: Add a registry invariant test covering
MobileMacUpdateCapabilityRequirement.standard that filters entries with a
declared release version and asserts each has a non-nil firstReleasedMacVersion.
Ensure the test exercises every released capability, including the
workspaceGroups path, so malformed MobileMacAppVersion(parsing:) literals fail
immediately.
In
`@Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite`+MacUpdateHint.swift:
- Around line 36-48: Update the Mac update hint flow around resolvedMacDeviceID
to require a non-empty stable macDeviceID before calling
MobileMacUpdateHintDismissalStore or persisting/showing the hint; when
unavailable, fail closed without evaluating the hint. Update
dismissMacUpdateHint to remove its "unknown" fallback and only dismiss using a
valid device ID, preserving existing behavior for identified devices.
🪄 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: 2518d2ee-1696-43d6-8b88-9abf30dbd9ba
📒 Files selected for processing (20)
Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileMacAppVersion.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileMacUpdateAdvisor.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileMacUpdateCapabilityRequirement.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileMacUpdateFeature.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileMacUpdateHintDismissalStore.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite+MacUpdateHint.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swiftPackages/iOS/CmuxMobileShell/Tests/CmuxMobileShellTests/MobileMacAppVersionTests.swiftPackages/iOS/CmuxMobileShell/Tests/CmuxMobileShellTests/MobileMacUpdateAdvisorTests.swiftPackages/iOS/CmuxMobileShell/Tests/CmuxMobileShellTests/MobileMacUpdateHintDismissalStoreTests.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobileMacUpdateFeatureDisplay.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobileMacUpdateHintBanner.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceListConnectionChrome.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceListView.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceShellView.swiftPackages/iOS/CmuxMobileShellUI/Tests/CmuxMobileShellUITests/MobileMacUpdateFeatureDisplayTests.swiftPackages/iOS/CmuxMobileShellUI/Tests/CmuxMobileShellUITests/WorkspaceListConnectionChromeTests.swiftSources/Mobile/MobileHostBuildIdentity.swiftSources/Mobile/MobileHostService+Capabilities.swiftios/cmux/Resources/Localizable.xcstrings
| public static let standard: [MobileMacUpdateCapabilityRequirement] = [ | ||
| .init( | ||
| capability: "workspace.actions.v1", | ||
| feature: .workspaceActions, | ||
| firstReleasedMacVersion: MobileMacAppVersion(parsing: "0.64.15") | ||
| ), | ||
| .init( | ||
| capability: "workspace.read_state.v1", | ||
| feature: .workspaceReadState, | ||
| firstReleasedMacVersion: MobileMacAppVersion(parsing: "0.64.16") | ||
| ), | ||
| .init( | ||
| capability: "workspace.close.v1", | ||
| feature: .workspaceClose, | ||
| firstReleasedMacVersion: MobileMacAppVersion(parsing: "0.64.16") | ||
| ), | ||
| .init( | ||
| capability: "workspace.groups.v1", | ||
| feature: .workspaceGroups, | ||
| firstReleasedMacVersion: MobileMacAppVersion(parsing: "0.64.16") | ||
| ), | ||
| .init(capability: "workspace.move.v1", feature: .workspaceMove, firstReleasedMacVersion: nil), | ||
| .init(capability: "workspace.group_actions.v1", feature: .workspaceGroupActions, firstReleasedMacVersion: nil), | ||
| .init(capability: "workspace.create_in_group.v1", feature: .workspaceCreateInGroup, firstReleasedMacVersion: nil), | ||
| .init(capability: "workspace.group_create.v1", feature: .workspaceGroupCreate, firstReleasedMacVersion: nil), | ||
| ] |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether MobileMacUpdateAdvisorTests already validates the standard registry.
rg -n 'standard' Packages/iOS/CmuxMobileShell/Tests/CmuxMobileShellTests/MobileMacUpdateAdvisorTests.swiftRepository: manaflow-ai/cmux
Length of output: 329
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Requirement source =="
sed -n '1,140p' Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileMacUpdateCapabilityRequirement.swift
echo
echo "== Version parser =="
rg -n 'init\\(parsing:|struct MobileMacAppVersion|enum MobileMacAppVersion|firstReleasedMacVersion' Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell -S
echo
echo "== Existing advisor tests around standard registry =="
sed -n '1,240p' Packages/iOS/CmuxMobileShell/Tests/CmuxMobileShellTests/MobileMacUpdateAdvisorTests.swiftRepository: manaflow-ai/cmux
Length of output: 3241
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '110,180p' Packages/iOS/CmuxMobileShell/Tests/CmuxMobileShellTests/MobileMacUpdateAdvisorTests.swiftRepository: manaflow-ai/cmux
Length of output: 1623
Add a registry invariant test for released capabilities
MobileMacAppVersion(parsing:) can return nil, and the current advisor test only covers the .workspaceGroups hint path. Add a test that asserts every standard entry with a declared release version still has a non-nil firstReleasedMacVersion, so a typo in one of the literals fails fast instead of silently making that capability look unreleased.
🤖 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/MobileMacUpdateCapabilityRequirement.swift`
around lines 35 - 60, Add a registry invariant test covering
MobileMacUpdateCapabilityRequirement.standard that filters entries with a
declared release version and asserts each has a non-nil firstReleasedMacVersion.
Ensure the test exercises every released capability, including the
workspaceGroups path, so malformed MobileMacAppVersion(parsing:) literals fail
immediately.
| terminalScrollbackPrefetchStatesBySurfaceID = [:] | ||
| terminalOutputTransport = .rawBytes | ||
| supportedHostCapabilities = [] | ||
| clearMacUpdateHint() |
There was a problem hiding this comment.
resetTerminalOutputTracking(), but promoteSecondaryToForeground() switches to an existing secondary connection without using this reset or recomputing the hint. When Mac A has a visible hint and Mac B is promoted, A's hint can remain visible under B's name until another status response arrives. Dismissing it during that interval records A's gap for A's stored device ID. Clear or recompute the hint as part of secondary promotion before exposing the new foreground host.
Rule Used: Flag Swift fixes that patch symptoms while leaving... (source)
The workspace-list toolbar renders items with its own monochrome tint, so without an explicit .tint the indicator loses the color that marks it as an update hint rather than a neighboring control. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Resolves the supportedHostCapabilities access-level conflict (main relaxed it to internal(set) for secondary promotion) and recomputes the Mac-update hint when a secondary Mac is promoted to foreground, since promotion reuses the live client without a fresh status probe. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift (2)
1189-1193: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winClear Mac update hint state on sign-out.
The supplied
signOut()path resets the active Mac and workspace state but does not clearmacUpdateHintormacUpdateHintShownSignatures. A previous account’s Mac/version/features can remain visible and the next account can inherit the session gate. Clear both at the account boundary.🤖 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.swift` around lines 1189 - 1193, Update signOut() near the existing workspace and selection resets to also clear macUpdateHint and macUpdateHintShownSignatures. Reset both values at sign-out so no prior account’s Mac update data or session gate carries into the next account.
6095-6109: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftValidate host identity before publishing capabilities and hints.
This code updates observable capabilities and the Mac-update hint before
applyHostReportedIdentityvalidates the reported device and instance tag. A stale route serving another tagged build can publish the wrong feature set/version before rejection. Normalize empty IDs as missing and complete identity validation before mutating capability or hint state.🤖 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.swift` around lines 6095 - 6109, Update the host-handshake flow around applyHostReportedIdentity to normalize empty device and instance IDs as missing, validate the reported identity first, and only then assign supportedHostCapabilities or call updateForegroundWorkspaceActionCapabilities and refreshMacUpdateHint. Ensure invalid or stale tagged routes cannot publish capabilities or update hints before identity validation succeeds.
🤖 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 3735-3740: Remove the production-only test seam
refreshRoutesFromRegistryForTesting from MobileShellComposite.swift. Update the
test target to call the internal refreshRoutesFromRegistry helper through
`@testable` import, or relocate the wrapper into the Tests target without changing
production Sources.
- Around line 2286-2295: Update the reuse condition in the switch-to-Mac logic
around foregroundMacDeviceID, connectionState, and remoteClient so it relies
directly on
MobileMacInstanceTagAuthority.sameStoredAuthority(refreshedTarget.instanceTag,
activeMacInstanceTag). Ensure legacy reuse succeeds only when both instance tags
are absent, and missing authority never qualifies a tagged connection as
reusable.
- Around line 3241-3253: Eliminate repeated full-store scans in the aggregation
revalidation paths around the paired-Mac lookups in the aggregation loop. Load
one keyed batch snapshot before iterating, or use a targeted authoritative
lookup by macDeviceID, and reuse it at the affected validation sites (including
the paths around lines 3322, 3348, and 3441). Preserve all existing post-await
subscription, scope, forgotten-device, and instance-tag authority checks.
In
`@Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite`+SecondaryPromotion.swift:
- Around line 5-8: Update the file-scoped secondaryPromotionLog declaration to
use nonisolated private let, preserving its existing Logger configuration and
category.
- Around line 52-54: Update the secondaryPromotionLog.info call in the secondary
promotion flow to mark macID as private instead of public, preserving the
existing reuse diagnostic while ensuring the stable device identifier remains
redacted in production logs.
- Around line 18-30: The guard-failure cleanup in the secondary promotion flow
can cancel a newer subscription that replaced the captured sub during the
awaited load. In the guard’s failure branch, only cancel and remove
secondaryMacSubscriptions[macID] when the currently stored subscription is
identical to sub; otherwise leave the replacement subscription intact.
---
Outside diff comments:
In
`@Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift`:
- Around line 1189-1193: Update signOut() near the existing workspace and
selection resets to also clear macUpdateHint and macUpdateHintShownSignatures.
Reset both values at sign-out so no prior account’s Mac update data or session
gate carries into the next account.
- Around line 6095-6109: Update the host-handshake flow around
applyHostReportedIdentity to normalize empty device and instance IDs as missing,
validate the reported identity first, and only then assign
supportedHostCapabilities or call updateForegroundWorkspaceActionCapabilities
and refreshMacUpdateHint. Ensure invalid or stale tagged routes cannot publish
capabilities or update hints before identity validation succeeds.
🪄 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: e2624efe-e68b-41b0-a2a9-4abc30e3cee0
📒 Files selected for processing (3)
Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite+SecondaryPromotion.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swiftios/cmux/Resources/Localizable.xcstrings
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift (2)
1189-1193: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winClear Mac update hint state on sign-out.
The supplied
signOut()path resets the active Mac and workspace state but does not clearmacUpdateHintormacUpdateHintShownSignatures. A previous account’s Mac/version/features can remain visible and the next account can inherit the session gate. Clear both at the account boundary.🤖 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.swift` around lines 1189 - 1193, Update signOut() near the existing workspace and selection resets to also clear macUpdateHint and macUpdateHintShownSignatures. Reset both values at sign-out so no prior account’s Mac update data or session gate carries into the next account.
6095-6109: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftValidate host identity before publishing capabilities and hints.
This code updates observable capabilities and the Mac-update hint before
applyHostReportedIdentityvalidates the reported device and instance tag. A stale route serving another tagged build can publish the wrong feature set/version before rejection. Normalize empty IDs as missing and complete identity validation before mutating capability or hint state.🤖 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.swift` around lines 6095 - 6109, Update the host-handshake flow around applyHostReportedIdentity to normalize empty device and instance IDs as missing, validate the reported identity first, and only then assign supportedHostCapabilities or call updateForegroundWorkspaceActionCapabilities and refreshMacUpdateHint. Ensure invalid or stale tagged routes cannot publish capabilities or update hints before identity validation succeeds.
🤖 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 3735-3740: Remove the production-only test seam
refreshRoutesFromRegistryForTesting from MobileShellComposite.swift. Update the
test target to call the internal refreshRoutesFromRegistry helper through
`@testable` import, or relocate the wrapper into the Tests target without changing
production Sources.
- Around line 2286-2295: Update the reuse condition in the switch-to-Mac logic
around foregroundMacDeviceID, connectionState, and remoteClient so it relies
directly on
MobileMacInstanceTagAuthority.sameStoredAuthority(refreshedTarget.instanceTag,
activeMacInstanceTag). Ensure legacy reuse succeeds only when both instance tags
are absent, and missing authority never qualifies a tagged connection as
reusable.
- Around line 3241-3253: Eliminate repeated full-store scans in the aggregation
revalidation paths around the paired-Mac lookups in the aggregation loop. Load
one keyed batch snapshot before iterating, or use a targeted authoritative
lookup by macDeviceID, and reuse it at the affected validation sites (including
the paths around lines 3322, 3348, and 3441). Preserve all existing post-await
subscription, scope, forgotten-device, and instance-tag authority checks.
In
`@Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite`+SecondaryPromotion.swift:
- Around line 5-8: Update the file-scoped secondaryPromotionLog declaration to
use nonisolated private let, preserving its existing Logger configuration and
category.
- Around line 52-54: Update the secondaryPromotionLog.info call in the secondary
promotion flow to mark macID as private instead of public, preserving the
existing reuse diagnostic while ensuring the stable device identifier remains
redacted in production logs.
- Around line 18-30: The guard-failure cleanup in the secondary promotion flow
can cancel a newer subscription that replaced the captured sub during the
awaited load. In the guard’s failure branch, only cancel and remove
secondaryMacSubscriptions[macID] when the currently stored subscription is
identical to sub; otherwise leave the replacement subscription intact.
---
Outside diff comments:
In
`@Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift`:
- Around line 1189-1193: Update signOut() near the existing workspace and
selection resets to also clear macUpdateHint and macUpdateHintShownSignatures.
Reset both values at sign-out so no prior account’s Mac update data or session
gate carries into the next account.
- Around line 6095-6109: Update the host-handshake flow around
applyHostReportedIdentity to normalize empty device and instance IDs as missing,
validate the reported identity first, and only then assign
supportedHostCapabilities or call updateForegroundWorkspaceActionCapabilities
and refreshMacUpdateHint. Ensure invalid or stale tagged routes cannot publish
capabilities or update hints before identity validation succeeds.
🪄 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: e2624efe-e68b-41b0-a2a9-4abc30e3cee0
📒 Files selected for processing (3)
Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite+SecondaryPromotion.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swiftios/cmux/Resources/Localizable.xcstrings
🛑 Comments failed to post (6)
Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift (3)
2286-2295: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Fail closed when stored instance authority is missing.
refreshedTarget.instanceTag == nilis treated as proof that the current tagged connection is reusable. With multiple tagged builds on one Mac,switchToMaccan return success without dialing the requested instance and route actions to the wrong build. UsesameStoredAuthoritydirectly; legacy reuse should require both tags to be absent.As per path instructions, correctness-critical identity must use one reliable source and missing signals must fail closed.
🤖 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.swift` around lines 2286 - 2295, Update the reuse condition in the switch-to-Mac logic around foregroundMacDeviceID, connectionState, and remoteClient so it relies directly on MobileMacInstanceTagAuthority.sameStoredAuthority(refreshedTarget.instanceTag, activeMacInstanceTag). Ensure legacy reuse succeeds only when both instance tags are absent, and missing authority never qualifies a tagged connection as reusable.Source: Path instructions
3241-3253: 🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift
Avoid O(M²) paired-Mac store scans during aggregation.
The outer aggregation loop processes every Mac, while these revalidation paths call
loadAll(...).first(where:)again for each Mac. For roughly 1,000 Macs this becomes repeated full-store I/O and O(M²) work. Reuse one keyed batch snapshot or add a targeted authoritative lookup by device ID while retaining post-await revalidation.As per path instructions, scalable production collections must avoid repeated full scans and rescans.
Also applies to: 3322-3329, 3348-3358, 3441-3458
🤖 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.swift` around lines 3241 - 3253, Eliminate repeated full-store scans in the aggregation revalidation paths around the paired-Mac lookups in the aggregation loop. Load one keyed batch snapshot before iterating, or use a targeted authoritative lookup by macDeviceID, and reuse it at the affected validation sites (including the paths around lines 3322, 3348, and 3441). Preserve all existing post-await subscription, scope, forgotten-device, and instance-tag authority checks.Sources: Coding guidelines, Path instructions
3735-3740: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Remove the
ForTestingwrapper from production Sources.
refreshRoutesFromRegistryForTestingis a test-only seam under productionSources. Exercise the internal helper from the test target via@testable import, or move the wrapper intoTests.As per path instructions, production Swift under
Sources/must not add test-only or debug-only seams.🤖 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.swift` around lines 3735 - 3740, Remove the production-only test seam refreshRoutesFromRegistryForTesting from MobileShellComposite.swift. Update the test target to call the internal refreshRoutesFromRegistry helper through `@testable` import, or relocate the wrapper into the Tests target without changing production Sources.Source: Path instructions
Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite+SecondaryPromotion.swift (3)
5-8: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Declare the file-scoped logger nonisolated.
This logger does not require MainActor access. Use
nonisolated private letto comply with the repository’s Swift 6 isolation guidance.🤖 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`+SecondaryPromotion.swift around lines 5 - 8, Update the file-scoped secondaryPromotionLog declaration to use nonisolated private let, preserving its existing Logger configuration and category.Source: Coding guidelines
18-30: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Do not cancel a replacement subscription on stale failure.
subis captured before the awaited store read. If a refresh replaces it while that read is suspended and the guard fails, this cleanup cancels whichever subscription is currently stored, potentially destroying the newer subscription. Guard cleanup withsecondaryMacSubscriptions[macID] === sub.🤖 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`+SecondaryPromotion.swift around lines 18 - 30, The guard-failure cleanup in the secondary promotion flow can cancel a newer subscription that replaced the captured sub during the awaited load. In the guard’s failure branch, only cancel and remove secondaryMacSubscriptions[macID] when the currently stored subscription is identical to sub; otherwise leave the replacement subscription intact.
52-54: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Keep the Mac device ID private in logs.
macIDis a stable device identifier but is logged with.public. Use.privateor log a non-identifying diagnostic.As per coding guidelines, dynamic identifiers must remain redacted in production logs.
🤖 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`+SecondaryPromotion.swift around lines 52 - 54, Update the secondaryPromotionLog.info call in the secondary promotion flow to mark macID as private instead of public, preserving the existing reuse diagnostic while ensuring the stable device identifier remains redacted in production logs.Source: Coding guidelines
…chrome-gated indicator Fail closed when neither the status payload nor the attach ticket carries a Mac device id, so anonymous hosts cannot share a dismissal record. Key the shown-analytics session gate by mac id + signature so two Macs with the same gap each count. Hide the toolbar indicator while reauth/recovery/offline chrome is active (new WorkspaceListConnectionChrome.showsMacUpdateHintIndicator, tested). Soften the dismiss copy to "Don't Show Again" since a changed gap re-arms the hint by design. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ecovery Structured review caught that the registry's target releases predate the version fields themselves: mobile.host.status gained mac_app_version in 0.64.16 and attach tickets gained macAppVersion in 0.64.17, so a released 0.64.15 Mac reports no version anywhere and the production hint could never fire. When no explicit version exists, the advisor now infers the version as the newest firstReleasedMacVersion among registry capabilities the host DOES advertise (a released Mac advertising a 0.64.15 capability is at least 0.64.15); hosts advertising no registered capability stay silent, and an unparseable explicit version still suppresses inference. Inferred versions use a body copy that names only the target version, never asserting the Mac's current version. Also refresh the hint from the full-timeout status recovery path (scheduleHostIdentityAdoptionIfNeeded), which decodes a complete status payload but previously applied only theme and identity, leaving the hint absent or stale after a slow transport probe. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Rename the shown event to ios_mac_update_hint_eligible (it fires when the model computes a visible hint, not when the toolbar indicator renders), tag both events with mac_app_version_inferred so inferred lower bounds cannot pollute version-segmented metrics, and move the recovery-path refresh into the MacUpdateHint extension so the over-budget composite stays at its recorded length. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit e4d64c0. Configure here.
| // free in the common case and keeps the phone's colors in sync with | ||
| // the Mac even when the probe could not. | ||
| self.applyTerminalTheme(payload.theme) | ||
| self.refreshMacUpdateHintFromRecoveredStatus(payload) |
There was a problem hiding this comment.
Stale status updates hint
Medium Severity
After the slow mobile.host.status recovery request finishes, the handler updates the Mac update hint without checking that remoteClient is still the same client that sent the request. A response from a superseded connection can populate macUpdateHint for the wrong Mac while the UI shows another host’s name.
Reviewed by Cursor Bugbot for commit e4d64c0. Configure here.
@ObservationIgnored cannot annotate a multi-variable declaration (the previous commit failed to compile), so the per-session bookkeeping moves into a MacUpdateHintSessionState reference type owned by the extension, which also keeps the over-budget composite at its recorded length. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Split MobileMacUpdateHint into its own file and fold the caseless MobileMacUpdateAdvisor namespace enum into a failable initializer on the owning type. Convert the MobileMacUpdateFeatureDisplay static namespace into displayName/bodyText extensions on the owning types. Consciously kept: the immutable .standard registry constant (declaration data with an explicit lint allowance, not runtime state) and the dismissal store's private static helpers (per the no-free-functions ruling; the store itself carries injected UserDefaults state). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Both paths constructed MobileMacUpdateHintDismissalStore() ad hoc, binding the composite to process-wide UserDefaults.standard despite the store's injection seam. The store now lives on MacUpdateHintSessionState so lookup and dismissal share one instance and tests/previews can swap in a suite-scoped store. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This branch was successfully deployed
1 participant
When the connected Mac lacks mobile host capabilities that this iOS build knows how to use, and those capabilities first shipped in a released Mac version newer than the Mac's reported version, the workspace-list toolbar shows a subtle teal up-arrow indicator (same UX pattern as the alt-screen notice). Tapping it opens a popover naming the Mac, its version, and the exact features an update adds (for example: "MacBook Pro (2) is on cmux 0.64.15. Updating to 0.64.16 or later adds: Mark workspaces read or unread, Close workspaces, and Workspace groups."), with a "Don't show again for this version" action. No generic nag: the indicator needs a concrete capability gap, a parseable Mac version, and a released Mac version that closes the gap, otherwise nothing shows.
Mechanism:
MobileMacUpdateAdvisor(pure, in CmuxMobileShell) compares themobile.host.statuscapability set andmac_app_version(falling back to the attach ticket's version) against a compiled registry mapping each mobile-gated capability to the first released Mac version that advertises it (workspace.actions.v1-> 0.64.15;read_state/close/groups-> 0.64.16;move/group_actions/create_in_group/group_create-> nil = unreleased, never claimed). Unknown, missing, or suffixed (nightly/prerelease) versions parse to nil and suppress the hint, so the indicator never lies.MacUpdateHintIndicatorButtonmounts in the workspace-list trailing toolbar only while a hint exists; dismissal persists permacDeviceID+ gap signature (sorted missing capability ids + minimum version) and re-arms only when the gap changes.ios_mac_update_hint_shown/_dismissedanalytics fire through the existing emitter. All new strings are localized in English and Japanese. A DEBUG-only Mac seam (CMUX_DEBUG_SUPPRESS_MOBILE_CAPS,CMUX_DEBUG_MOBILE_APP_VERSION) lets a dev Mac impersonate an older host for dogfood; Release behavior is unchanged.The first pass shipped this as a list banner; after owner dogfood feedback it was reworked into the toolbar indicator + popover (b29224a), reusing the alt-screen notice presentation.
Verification:
swift teston CmuxMobileShell (428 tests) andCmuxMobileShellUITestsvia xcodebuild on an isolated simulator (80 tests) green, including advisor decision tables (older/equal/newer/unknown/prerelease/unreleased/mixed), version parsing, and dismissal-store scoping. Verified live on a dedicated simulator against a tagged Mac impersonating 0.64.14 and 0.64.15: glyph shown with exact popover copy, absent against an up-to-date Mac, dismissal instant and persisted, re-armed when the gap set changed, and a capability whose registry version equals the Mac's version is truthfully omitted. Two independent verifier sessions (one per UI iteration) reproduced the test runs and approved the screenshot/log evidence.🤖 Generated with Claude Code
Note
Low Risk
Mostly additive UX and pure version/capability logic with conservative fail-closed rules; connection and dismissal paths are covered by unit tests, with no auth or data-plane changes in Release.
Overview
Adds a truthful Mac update advisor on iOS: when
mobile.host.status(or attach ticket) shows the connected Mac is older than a released capability the app knows about, the workspace list can surface a teal toolbar indicator with a popover listing the minimum cmux version and unlocked features.Model: New
MobileMacAppVersion, a capability→first-ship-version registry (MobileMacUpdateCapabilityRequirement.standard), andMobileMacUpdateHintthat only fires on concrete gaps—unreleased capabilities staynil, unparseable/nightly versions suppress hints, and missing version can be inferred from advertised caps without stating a false current version in copy. Per-Mac dismissal persists viaMobileMacUpdateHintDismissalStorekeyed by gap signature.Shell wiring:
MobileShellCompositeexposesmacUpdateHint, refreshes on host status (including recovery path), clears on disconnect, and recomputes on secondary Mac promotion so the wrong host’s hint does not stick. Analytics:ios_mac_update_hint_eligible/_dismissed.UI:
MacUpdateHintIndicatorButtonin the workspace toolbar when connection chrome is idle; localized EN/JA strings. DEBUG Mac seams (CMUX_DEBUG_SUPPRESS_MOBILE_CAPS,CMUX_DEBUG_MOBILE_APP_VERSION) for dogfooding older hosts.Reviewed by Cursor Bugbot for commit 8de2ae1. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit