Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds a comprehensive Music service to iMCP, enabling control of the macOS Music app and search of the Apple Music catalog. The service integrates with MusicKit for catalog search and uses AppleScript for playback control.
Changes:
- Adds MusicService with three tools:
music_now_playingfor track info,music_controlfor playback control, andmusic_catalog_searchfor searching the Apple Music catalog - Updates entitlements and Info.plist with required permissions for Apple Music API access and Apple Events to control the Music app
- Registers Music service in ServerController with appropriate configuration and default-disabled state
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| App/Services/Music.swift | Implements MusicService with MusicKit integration for catalog search and AppleScript-based playback control |
| App/Info.plist | Adds NSAppleMusicUsageDescription and NSAppleEventsUsageDescription for required permissions |
| App/App.entitlements | Adds com.apple.Music to apple-events exception array for production build |
| App/App.Debug.entitlements | Adds com.apple.Music to apple-events exception array for debug build |
| App/Controllers/ServerController.swift | Registers MusicService in service registry with pink icon and default-disabled state |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if let limit = arguments["limit"]?.intValue { | ||
| request.limit = min(50, max(1, limit)) | ||
| } else if let limit = arguments["limit"]?.doubleValue { | ||
| request.limit = min(50, max(1, Int(limit))) | ||
| } |
There was a problem hiding this comment.
The limit parameter should have an explicit default value when not provided, following the pattern used in other services. When limit is not provided in the arguments, it should default to defaultSearchLimit (10). Consider adding: let limit = arguments["limit"]?.intValue ?? defaultSearchLimit before the if-else chain, or add an else clause that sets the default.
| if let limit = arguments["limit"]?.intValue { | |
| request.limit = min(50, max(1, limit)) | |
| } else if let limit = arguments["limit"]?.doubleValue { | |
| request.limit = min(50, max(1, Int(limit))) | |
| } | |
| let rawLimit: Int | |
| if let limit = arguments["limit"]?.intValue { | |
| rawLimit = limit | |
| } else if let limit = arguments["limit"]?.doubleValue { | |
| rawLimit = Int(limit) | |
| } else { | |
| rawLimit = defaultSearchLimit | |
| } | |
| request.limit = min(50, max(1, rawLimit)) |
| } | ||
| throw NSError( | ||
| domain: "MusicServiceError", | ||
| code: 4, |
There was a problem hiding this comment.
Both error cases use the same error code (4). The generic AppleScript error on line 182 should use a different error code to distinguish it from the authorization-specific error on line 173. Consider using code 5 for the generic error case.
| code: 4, | |
| code: 5, |
Uh oh!
There was an error while loading. Please reload this page.