Repository navigation
iOS: add Mac power controls - #8137
lawrencecchen wants to merge 3 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughMac power status and controls are added across the host service, mobile RPC layer, iOS shell client, and Mac detail UI, with capability negotiation, authorization checks, localization, and tests for decoding, service behavior, and admission rules. ChangesMac power control
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant MacComputerDetailView
participant MobileShellComposite
participant TerminalController
participant MacPowerService
participant PowerAssertionHolder
MacComputerDetailView->>MobileShellComposite: invoke Mac power RPC method
MobileShellComposite->>TerminalController: send mac.power request
TerminalController->>MacPowerService: execute status or control operation
MacPowerService->>PowerAssertionHolder: set keep-awake assertion
PowerAssertionHolder-->>MacPowerService: assertion state or error
MacPowerService-->>TerminalController: return status or service error
TerminalController-->>MobileShellComposite: return RPC result
MobileShellComposite-->>MacComputerDetailView: update UI state
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 Mac power controls to the iOS Mac detail screen — sleep displays, toggle a cmux-owned keep-awake IOKit assertion, and toggle Low Power Mode — with a new
Confidence Score: 5/5Safe to merge — the auth chain, RPC encoding, IOKit assertion lifecycle, actor isolation, and i18n are all correct and well-tested. The RPC response format was verified end-to-end: No files require special attention. Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant iOS as iOS MacComputerDetailView
participant Shell as MobileShellComposite
participant RPC as MobileCoreRPCClient
participant Auth as MobileHostService (Mac)
participant TC as TerminalController
participant PS as MacPowerService
participant IOKit as IOKit / SleepyPowerControls
iOS->>Shell: macPowerStatus() / setMacKeepAwake() / setMacLowPowerMode() / sleepMacDisplay()
Shell->>RPC: "sendRequest(mac.power.*)"
RPC->>Auth: irohAdmission / stackBearer path
Auth->>Auth: isPrivilegedMacPowerMethod? → Stack auth required
Auth->>Auth: ticketAuthorizationError? → mac-wide ticket required
Auth-->>RPC: authorized
RPC->>TC: v2MobileMacPower(method, params)
TC->>PS: status() / setKeepAwake() / setLowPowerMode() / sleepDisplay()
PS->>IOKit: IOPMAssertionCreate/Release / setLowPowerMode / sleepDisplayNow
IOKit-->>PS: result
PS-->>TC: MacPowerStatus
TC-->>RPC: "{ok: true, result: {keep_awake_enabled, low_power_enabled}}"
RPC-->>Shell: Data
Shell->>Shell: MobileMacPowerStatus.decode(data)
Shell-->>iOS: MobileMacPowerStatus
iOS->>iOS: "update @State macPowerStatus / macPowerError"
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant iOS as iOS MacComputerDetailView
participant Shell as MobileShellComposite
participant RPC as MobileCoreRPCClient
participant Auth as MobileHostService (Mac)
participant TC as TerminalController
participant PS as MacPowerService
participant IOKit as IOKit / SleepyPowerControls
iOS->>Shell: macPowerStatus() / setMacKeepAwake() / setMacLowPowerMode() / sleepMacDisplay()
Shell->>RPC: "sendRequest(mac.power.*)"
RPC->>Auth: irohAdmission / stackBearer path
Auth->>Auth: isPrivilegedMacPowerMethod? → Stack auth required
Auth->>Auth: ticketAuthorizationError? → mac-wide ticket required
Auth-->>RPC: authorized
RPC->>TC: v2MobileMacPower(method, params)
TC->>PS: status() / setKeepAwake() / setLowPowerMode() / sleepDisplay()
PS->>IOKit: IOPMAssertionCreate/Release / setLowPowerMode / sleepDisplayNow
IOKit-->>PS: result
PS-->>TC: MacPowerStatus
TC-->>RPC: "{ok: true, result: {keep_awake_enabled, low_power_enabled}}"
RPC-->>Shell: Data
Shell->>Shell: MobileMacPowerStatus.decode(data)
Shell-->>iOS: MobileMacPowerStatus
iOS->>iOS: "update @State macPowerStatus / macPowerError"
Reviews (2): Last reviewed commit: "Fix Mac power service actor-isolated con..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| nonisolated private static func isPrivilegedMacPowerMethod(_ method: String) -> Bool { | ||
| method == "mac.power.status" || method.hasPrefix("mac.power.") | ||
| } |
There was a problem hiding this comment.
isPrivilegedMacPowerMethod
method == "mac.power.status" is always true when method.hasPrefix("mac.power.") is true, so the first arm of the || is dead code. A reader may believe the two conditions guard different sets — for example, that the prefix test covers a broader set than just mac.power.status — when they are actually identical in coverage. The function can be simplified to just the hasPrefix guard.
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!
| nonisolated private static func isPrivilegedMacPowerMethod(_ method: String) -> Bool { | ||
| method == "mac.power.status" || method.hasPrefix("mac.power.") | ||
| } |
There was a problem hiding this comment.
hasPrefix("mac.power.") is broader than the explicit ticket-auth list
isPrivilegedMacPowerMethod uses method.hasPrefix("mac.power.") for the .irohAdmission path, so every future mac.power.* method added to the v2MobileMacPower switch automatically requires Stack auth over Iroh. However, the ticket-authorization switch in MobileHostService+TicketAuthorization.swift uses an explicit four-method list. Any new mac.power.* method added to the handler but omitted from the ticket-auth list falls to the default case and bypasses the mac-wide ticket check on the .stackBearer path. The two lists are structurally asymmetric: one is implicitly open, the other is explicitly closed. A comment or shared constant set documenting the intended scope of mac.power.* methods would prevent the two enforcement points from silently diverging.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/MacComputerDetailView.swift`:
- Around line 541-558: Update the catch block in runPowerStatusAction to await
refreshMacPower() before assigning macPowerError, so the status refresh cannot
clear the mutation failure message. Preserve the existing localized error text
and refresh behavior.
- Around line 120-123: Update the .task modifier in MacComputerDetailView to use
an Equatable array containing isForeground and store.supportsMacPowerControl as
its ID instead of interpolating them into a string. Preserve the existing guard
and refreshMacPower behavior and the same invalidation semantics.
In `@Sources/Mobile/MobileHostService.swift`:
- Around line 1094-1096: Update isPrivilegedMacPowerMethod to remove the
redundant exact-match comparison and rely solely on
method.hasPrefix("mac.power.").
In `@Sources/PowerAssertionHolder.swift`:
- Around line 8-13: Mark the assertionID property in PowerAssertionHolder as
nonisolated(unsafe) so deinit can access it without violating Swift 6 strict
concurrency isolation. Keep its existing type, initialization, and usage
unchanged.
🪄 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: f37e5f98-727c-496a-addf-c5c6be998112
📒 Files selected for processing (20)
Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileMacPowerError.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileMacPowerStatus.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite+MacPower.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swiftPackages/iOS/CmuxMobileShell/Tests/CmuxMobileShellTests/MobileMacPowerStatusTests.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MacComputerDetailView.swiftSources/MacPowerService.swiftSources/MacPowerStatus.swiftSources/Mobile/MobileHostService+Capabilities.swiftSources/Mobile/MobileHostService+TicketAuthorization.swiftSources/Mobile/MobileHostService.swiftSources/PowerAssertionHolder.swiftSources/PowerAssertionHolding.swiftSources/TerminalController.swiftcmux.xcodeproj/project.pbxprojcmuxTests/MacPowerServiceTests.swiftcmuxTests/MobileHostAuthorizationTests.swiftcmuxTests/MobileHostIrohAdmissionTests.swiftios/CHANGELOG.mdios/cmux/Resources/Localizable.xcstrings
| .task(id: "\(isForeground)-\(store.supportsMacPowerControl)") { | ||
| guard isForeground, store.supportsMacPowerControl else { return } | ||
| await refreshMacPower() | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🔵 Trivial | 💤 Low value
Prefer an array for the .task ID to avoid string allocations.
Using string interpolation for the .task identifier allocates a new string on every render pass of this view. Since both properties are booleans, you can pass them as an Equatable array to achieve the exact same invalidation semantics with less overhead.
💡 Proposed refactor
- .task(id: "\(isForeground)-\(store.supportsMacPowerControl)") {
+ .task(id: [isForeground, store.supportsMacPowerControl]) {
guard isForeground, store.supportsMacPowerControl else { return }
await refreshMacPower()
}📝 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.
| .task(id: "\(isForeground)-\(store.supportsMacPowerControl)") { | |
| guard isForeground, store.supportsMacPowerControl else { return } | |
| await refreshMacPower() | |
| } | |
| .task(id: [isForeground, store.supportsMacPowerControl]) { | |
| guard isForeground, store.supportsMacPowerControl else { return } | |
| await refreshMacPower() | |
| } |
🤖 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/MacComputerDetailView.swift`
around lines 120 - 123, Update the .task modifier in MacComputerDetailView to
use an Equatable array containing isForeground and store.supportsMacPowerControl
as its ID instead of interpolating them into a string. Preserve the existing
guard and refreshMacPower behavior and the same invalidation semantics.
| private func runPowerStatusAction( | ||
| _ action: @escaping @MainActor () async throws -> MobileMacPowerStatus | ||
| ) { | ||
| guard !isMacPowerBusy else { return } | ||
| isMacPowerBusy = true | ||
| macPowerError = nil | ||
| Task { | ||
| defer { isMacPowerBusy = false } | ||
| do { | ||
| let status = try await action() | ||
| macPowerStatus = status | ||
| } catch { | ||
| macPowerError = L10n.string("mobile.macPower.error", defaultValue: "Couldn't update Mac power controls.") | ||
| await refreshMacPower() | ||
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Prevent refreshMacPower() from wiping out the mutation error message.
If the backend mutation (e.g., setMacLowPowerMode) fails, the catch block correctly sets macPowerError to notify the user. However, await refreshMacPower() is called immediately afterward. Because refreshMacPower() sets macPowerError = nil upon a successful status fetch, the error message meant for the user is instantly wiped out.
Swap the order so the UI resynchronizes the toggle status before you assign the failure message for the mutation.
🐛 Proposed fix
private func runPowerStatusAction(
_ action: `@escaping` `@MainActor` () async throws -> MobileMacPowerStatus
) {
guard !isMacPowerBusy else { return }
isMacPowerBusy = true
macPowerError = nil
Task {
defer { isMacPowerBusy = false }
do {
let status = try await action()
macPowerStatus = status
} catch {
- macPowerError = L10n.string("mobile.macPower.error", defaultValue: "Couldn't update Mac power controls.")
await refreshMacPower()
+ macPowerError = L10n.string("mobile.macPower.error", defaultValue: "Couldn't update Mac power controls.")
}
}
}📝 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.
| private func runPowerStatusAction( | |
| _ action: @escaping @MainActor () async throws -> MobileMacPowerStatus | |
| ) { | |
| guard !isMacPowerBusy else { return } | |
| isMacPowerBusy = true | |
| macPowerError = nil | |
| Task { | |
| defer { isMacPowerBusy = false } | |
| do { | |
| let status = try await action() | |
| macPowerStatus = status | |
| } catch { | |
| macPowerError = L10n.string("mobile.macPower.error", defaultValue: "Couldn't update Mac power controls.") | |
| await refreshMacPower() | |
| } | |
| } | |
| } | |
| private func runPowerStatusAction( | |
| _ action: `@escaping` `@MainActor` () async throws -> MobileMacPowerStatus | |
| ) { | |
| guard !isMacPowerBusy else { return } | |
| isMacPowerBusy = true | |
| macPowerError = nil | |
| Task { | |
| defer { isMacPowerBusy = false } | |
| do { | |
| let status = try await action() | |
| macPowerStatus = status | |
| } catch { | |
| await refreshMacPower() | |
| macPowerError = L10n.string("mobile.macPower.error", defaultValue: "Couldn't update Mac power controls.") | |
| } | |
| } | |
| } |
🤖 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/MacComputerDetailView.swift`
around lines 541 - 558, Update the catch block in runPowerStatusAction to await
refreshMacPower() before assigning macPowerError, so the status refresh cannot
clear the mutation failure message. Preserve the existing localized error text
and refresh behavior.
| nonisolated private static func isPrivilegedMacPowerMethod(_ method: String) -> Bool { | ||
| method == "mac.power.status" || method.hasPrefix("mac.power.") | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Remove redundant exact-match check.
The method == "mac.power.status" check is redundant because "mac.power.status".hasPrefix("mac.power.") will already evaluate to true.
💡 Proposed refactor
- nonisolated private static func isPrivilegedMacPowerMethod(_ method: String) -> Bool {
- method == "mac.power.status" || method.hasPrefix("mac.power.")
- }
+ nonisolated private static func isPrivilegedMacPowerMethod(_ method: String) -> Bool {
+ method.hasPrefix("mac.power.")
+ }📝 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.
| nonisolated private static func isPrivilegedMacPowerMethod(_ method: String) -> Bool { | |
| method == "mac.power.status" || method.hasPrefix("mac.power.") | |
| } | |
| nonisolated private static func isPrivilegedMacPowerMethod(_ method: String) -> Bool { | |
| method.hasPrefix("mac.power.") | |
| } |
🤖 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 `@Sources/Mobile/MobileHostService.swift` around lines 1094 - 1096, Update
isPrivilegedMacPowerMethod to remove the redundant exact-match comparison and
rely solely on method.hasPrefix("mac.power.").
| private var assertionID = IOPMAssertionID(0) | ||
| var isEnabled: Bool { assertionID != 0 } | ||
|
|
||
| deinit { | ||
| if assertionID != 0 { IOPMAssertionRelease(assertionID) } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Fix Swift 6 strict concurrency violation in deinit.
In Swift 6, deinit for actor-isolated types is evaluated as nonisolated because it can theoretically execute on any thread when the last reference drops. Accessing the @MainActor-isolated var assertionID inside deinit will trigger a strict concurrency compiler error.
Since assertionID is a simple primitive that is only safely mutated on the main actor and deinit runs exclusively after all other references are gone, you can resolve the isolation error by marking the property nonisolated(unsafe).
🛠 Proposed fix
- private var assertionID = IOPMAssertionID(0)
+ private nonisolated(unsafe) var assertionID = IOPMAssertionID(0)📝 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.
| private var assertionID = IOPMAssertionID(0) | |
| var isEnabled: Bool { assertionID != 0 } | |
| deinit { | |
| if assertionID != 0 { IOPMAssertionRelease(assertionID) } | |
| } | |
| private nonisolated(unsafe) var assertionID = IOPMAssertionID(0) | |
| var isEnabled: Bool { assertionID != 0 } | |
| deinit { | |
| if assertionID != 0 { IOPMAssertionRelease(assertionID) } | |
| } |
🤖 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 `@Sources/PowerAssertionHolder.swift` around lines 8 - 13, Mark the assertionID
property in PowerAssertionHolder as nonisolated(unsafe) so deinit can access it
without violating Swift 6 strict concurrency isolation. Keep its existing type,
initialization, and usage unchanged.
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)
Sources/TerminalController.swift (1)
351-366: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer a default parameter over a convenience initializer.
Since all other parameters in the designated initializer already have default values, you can remove the
convenience initand provide a default value formacPowerServicedirectly. This aligns with idiomatic Swift and simplifies the initializer structure.♻️ Proposed refactor
- private convenience init() { - self.init(macPowerService: MacPowerService()) - } - private init( passwordStore: SocketControlPasswordStore = SocketControlPasswordStore(), transport: SocketTransport = SocketTransport(), listenerPolicy: SocketListenerPolicy = SocketListenerPolicy(), socketClientPreauthorizationLimiter: SocketClientPreauthorizationLimiter = .init( maximumConcurrentClaims: 32 ), remoteProxyBroker: any RemoteProxyBrokering = RemoteProxyBroker( tunnelProvider: RemoteDaemonProxyTunnelProvider(strings: .appLocalized, ptyBridgeStrings: AppRemotePTYBridgeStrings()) ), - macPowerService: MacPowerService + macPowerService: MacPowerService = MacPowerService() ) {🤖 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 `@Sources/TerminalController.swift` around lines 351 - 366, Remove the parameterless convenience initializer and give the designated TerminalController initializer’s macPowerService parameter a MacPowerService default value. Preserve the existing dependency defaults and initialization behavior while simplifying construction through the designated init.
🤖 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 `@Sources/TerminalController.swift`:
- Around line 351-366: Remove the parameterless convenience initializer and give
the designated TerminalController initializer’s macPowerService parameter a
MacPowerService default value. Preserve the existing dependency defaults and
initialization behavior while simplifying construction through the designated
init.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: ba903d1f-f1a5-47cf-9792-b8e07fe8e9ce
📒 Files selected for processing (2)
Sources/MacPowerService.swiftSources/TerminalController.swift
lawrencecchen
added
the
stale-revisit
This branch was successfully deployed
2 participants
Summary
caffeinateprocessesSupersedes the implementation approach in #6902 by Austin Wang while preserving its mobile power-control product direction. Fixes #6482.
Verification
swift test --package-path Packages/iOS/CmuxMobileShell --filter MobileMacPowerStatusTestsgit diff --checkWhat to test
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by cubic
Add Mac power controls to the iOS Mac details screen: sleep displays, toggle a cmux-owned Keep Awake, and toggle Low Power Mode via new mobile RPCs with strict authorization. Improves remote control from iOS while keeping the Mac reachable.
New Features
mac.power.control.v1; addsmac.power.status,mac.power.sleep_display,mac.power.keep_awake.set,mac.power.low_power.set.MacPowerServiceusingPowerAssertionHolder(IOKit) and existingSleepyPowerControls; returns authoritativeMacPowerStatus.Bug Fixes
MacPowerServiceto avoid cross-actor initialization and ensure safe power RPC handling.Written for commit 877012b. Summary will update on new commits.
Summary by CodeRabbit
New Features
Tests
Documentation