feat: add XcodeKit Framework Support #263
Conversation
WalkthroughAdded special-case handling for "XcodeKit.framework": loadMetadata forces its SDKSource to Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant SystemFrameworkMetadataProvider
Caller->>SystemFrameworkMetadataProvider: loadMetadata(sdkName, status, platform, sdkSource)
alt sdkName == "XcodeKit.framework"
alt platform == macOS
SystemFrameworkMetadataProvider->>SystemFrameworkMetadataProvider: set sdkSource = .developer
SystemFrameworkMetadataProvider->>SystemFrameworkMetadataProvider: resolve sdkPath -> /Library/Frameworks/XcodeKit.framework
SystemFrameworkMetadataProvider-->>Caller: return SystemFrameworkMetadata(name, path, status, .developer)
else
SystemFrameworkMetadataProvider-->>Caller: throw unsupportedSDKPlatform(sdk, platform, [macOS])
end
else
SystemFrameworkMetadataProvider-->>Caller: proceed with existing loadMetadata logic
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
dosubot
bot
added
size:XS
|
@fortmarek Any other changes I need to make? |
fortmarek
left a comment
fortmarek
left a comment
There was a problem hiding this comment.
Would you mind updating also the tests for the changes you've made?
| if name == "XcodeKit.framework" && platform == .macOS { | ||
| let sdkRootPath = try AbsolutePath(validating: "/Library/Frameworks") | ||
| return sdkRootPath.appending(component: name) | ||
| } |
There was a problem hiding this comment.
I'm pretty sure this is not the correct path. This needs to be prefixed with the xcodeDeveloperSdkRootPath
There was a problem hiding this comment.
This needs to be prefixed with the
xcodeDeveloperSdkRootPath
There are frameworks at 3 different locations. The xcodeDeveloperSdkRootPath and xcodeSdkRootPath paths don't work for XcodeKit.
| Framework | Location (inside /Applications/Xcode.app/Contents/Developer/) |
|---|---|
| XcodeKit | Library/Frameworks |
| XCTest | Platforms/MacOSX.platform/Developer/Library/Frameworks |
| CloudKit | Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk/System/Library/Frameworks |
XcodeGraph/Sources/XcodeGraph/Models/Platform.swift
Lines 148 to 153 in 9ae987d
XcodeGraph/Sources/XcodeGraph/Models/Platform.swift
Lines 133 to 138 in 9ae987d
dosubot
bot
added
size:S
Added the test for Edit: Based on @coderabbitai's comments...
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Tests/XcodeMetadataTests/SystemFrameworkMetadataProviderTests.swift (1)
109-129: Add negative tests for unsupported combinationsTo lock behavior down, consider adding:
- XcodeKit with source .system (should throw)
- XcodeKit with non-macOS platforms (e.g., .iOS, .tvOS; should throw)
This will prevent regressions if the provider logic changes.
If helpful, I can draft these test cases once we confirm the exact error type thrown for these scenarios.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Sources/XcodeMetadata/Providers/SystemFrameworkMetadataProvider.swift(1 hunks)Tests/XcodeMetadataTests/SystemFrameworkMetadataProviderTests.swift(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Sources/XcodeMetadata/Providers/SystemFrameworkMetadataProvider.swift
🧰 Additional context used
📓 Path-based instructions (2)
Tests/**/*
📄 CodeRabbit Inference Engine (AGENT.md)
Unit tests for each module should be placed in the corresponding
Tests/directories
Files:
Tests/XcodeMetadataTests/SystemFrameworkMetadataProviderTests.swift
**/*.swift
📄 CodeRabbit Inference Engine (AGENT.md)
Use Swift 6.0.3+ with StrictConcurrency enabled
Files:
Tests/XcodeMetadataTests/SystemFrameworkMetadataProviderTests.swift
🧠 Learnings (4)
📓 Common learnings
Learnt from: CR
PR: tuist/XcodeGraph#0
File: AGENT.md:0-0
Timestamp: 2025-08-07T14:29:08.174Z
Learning: Applies to Sources/XcodeGraph/Models/**/* : Core data structures like Project, Target, Scheme, and BuildConfiguration should be implemented in `Sources/XcodeGraph/Models/`
Learnt from: CR
PR: tuist/XcodeGraph#0
File: AGENT.md:0-0
Timestamp: 2025-08-07T14:29:08.174Z
Learning: Applies to Sources/XcodeMetadata/Providers/**/* : Binary metadata extraction for frameworks and libraries should be implemented in `Sources/XcodeMetadata/Providers/`
Learnt from: CR
PR: tuist/XcodeGraph#0
File: AGENT.md:0-0
Timestamp: 2025-08-07T14:29:08.174Z
Learning: Applies to Tests/Fixtures/**/* : XCFramework fixtures for metadata testing should be placed in `Tests/Fixtures/`
📚 Learning: 2025-08-07T14:29:08.174Z
Learnt from: CR
PR: tuist/XcodeGraph#0
File: AGENT.md:0-0
Timestamp: 2025-08-07T14:29:08.174Z
Learning: Applies to Tests/Fixtures/**/* : XCFramework fixtures for metadata testing should be placed in `Tests/Fixtures/`
Applied to files:
Tests/XcodeMetadataTests/SystemFrameworkMetadataProviderTests.swift
📚 Learning: 2025-08-07T14:29:08.174Z
Learnt from: CR
PR: tuist/XcodeGraph#0
File: AGENT.md:0-0
Timestamp: 2025-08-07T14:29:08.174Z
Learning: Applies to Sources/XcodeMetadata/Providers/**/* : Binary metadata extraction for frameworks and libraries should be implemented in `Sources/XcodeMetadata/Providers/`
Applied to files:
Tests/XcodeMetadataTests/SystemFrameworkMetadataProviderTests.swift
📚 Learning: 2025-08-07T14:29:08.174Z
Learnt from: CR
PR: tuist/XcodeGraph#0
File: AGENT.md:0-0
Timestamp: 2025-08-07T14:29:08.174Z
Learning: Linux support is available with limited testing; XcodeGraphMapperTests and XcodeMetadataTests require Xcode utilities
Applied to files:
Tests/XcodeMetadataTests/SystemFrameworkMetadataProviderTests.swift
🔇 Additional comments (2)
Tests/XcodeMetadataTests/SystemFrameworkMetadataProviderTests.swift (2)
109-129: LGTM: Solid coverage for XcodeKit developer path on macOSThis test accurately captures the expected developer-relative path for XcodeKit and guards the newly supported case.
121-126: No change needed: developer-relative semantics are correctThe test for
XcodeKit.frameworkexpects the path"/Library/Frameworks/XcodeKit.framework", matching how other developer‐sourced frameworks are represented (e.g."/Platforms/.../Developer/Library/Frameworks/..."). The special‐case insdkPathusingAbsolutePath(validating: "/Library")produces that same developer‐relative string, not an absolute system root lookup. Paths remain consistently anchored to the DEVELOPER_DIR across all tests.
dosubot
bot
added
size:M
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 5113 @navtoj Thanks for adding the test and implementing the error handling for unsupported platforms! The changes look good overall. I noticed one potential issue in the implementation: on line 67 of if sdkName == "XcodeKit.framework" {
guard platform == .macOS else {
throw SystemFrameworkMetadataProviderError
.unsupportedSDKPlatform(sdk: sdkName, platform: platform, supported: [.macOS])
}
source = .developer // This reassigns a function parameter
}Since
The test coverage for the XcodeKit case and the error handling for unsupported platforms are well implemented! The error message clearly indicates which platforms are supported. 🧠 Learnings used |
|
Tip For best results, initiate chat on the files or code changes. @navtoj Thank you for implementing the requested changes! Based on the updates you've made: ✅ Added test for The changes look good and properly address the feedback from both fortmarek and myself. The test coverage for the new XcodeKit framework path functionality is now in place, and the code structure is more consistent with the existing pattern. 🧠 Learnings used |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 1881 🏁 Script executed: Length of output: 1422 @navtoj Great work on adding the test and implementing the error handling! I can see you've also moved the XcodeKit-specific path logic into the However, I notice that the parameter reassignment issue on line 70 is still present: source = .developer // This reassigns a function parameterSince the test already passes Consider removing the 🧠 Learnings used |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 2598 🏁 Script executed: Length of output: 2605 @navtoj Great work on addressing the feedback! The tests look comprehensive, and the error handling for unsupported platforms is well implemented. I notice that on line 70, you're still reassigning the
if sdkName == "XcodeKit.framework" {
guard platform == .macOS else {
throw SystemFrameworkMetadataProviderError
.unsupportedSDKPlatform(sdk: sdkName, platform: platform, supported: [.macOS])
}
guard source == .developer else {
throw SystemFrameworkMetadataProviderError
.unsupportedSDKSource(sdk: sdkName, source: source, supported: [.developer])
}
}
if sdkName == "XcodeKit.framework" {
guard platform == .macOS else {
throw SystemFrameworkMetadataProviderError
.unsupportedSDKPlatform(sdk: sdkName, platform: platform, supported: [.macOS])
}
// XcodeKit.framework only exists in the developer directory, override the source
source = .developer
}The rest of the changes look solid - the test coverage is good and the path logic in the switch block is clean! 👍 🧠 Learnings used |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
Sources/XcodeMetadata/Providers/SystemFrameworkMetadataProvider.swift (2)
16-18: Tighten error message consistency (platform display and naming)Use
platform.caseValuefor the single platform (as you already do for the supported list) and rename the local var to reflect platforms, not types.- case let .unsupportedSDKPlatform(sdk, platform, supported): - let supportedTypes = supported.map { $0.caseValue }.joined(separator: ", ") - return "The Platform type of \(platform) is not currently supported for \(sdk) - only \(supportedTypes) are supported." + case let .unsupportedSDKPlatform(sdk, platform, supported): + let supportedPlatforms = supported.map { $0.caseValue }.joined(separator: ", ") + return "The platform \(platform.caseValue) is not supported for \(sdk) - only \(supportedPlatforms) are supported."
65-71: Avoid duplicating magic string “XcodeKit.framework” and improve readabilityThe special-casing appears twice (in
loadMetadataandsdkPath). Consider centralizing the check and using a named constant.+private let xcodeKitFrameworkName = "XcodeKit.framework" @@ - if sdkName == "XcodeKit.framework" { + if sdkName == xcodeKitFrameworkName { @@ - let xcodeDeveloperSdkRootPath = name == "XcodeKit.framework" ? "Library" : platform.xcodeDeveloperSdkRootPath + let xcodeDeveloperSdkRootPath = name == xcodeKitFrameworkName ? "Library" : platform.xcodeDeveloperSdkRootPathAlso applies to: 85-85
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Sources/XcodeMetadata/Providers/SystemFrameworkMetadataProvider.swift(3 hunks)Tests/XcodeMetadataTests/SystemFrameworkMetadataProviderTests.swift(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Tests/XcodeMetadataTests/SystemFrameworkMetadataProviderTests.swift
🧰 Additional context used
📓 Path-based instructions (2)
**/*.swift
📄 CodeRabbit Inference Engine (AGENT.md)
Use Swift 6.0.3+ with StrictConcurrency enabled
Files:
Sources/XcodeMetadata/Providers/SystemFrameworkMetadataProvider.swift
Sources/XcodeMetadata/Providers/**/*
📄 CodeRabbit Inference Engine (AGENT.md)
Binary metadata extraction for frameworks and libraries should be implemented in
Sources/XcodeMetadata/Providers/
Files:
Sources/XcodeMetadata/Providers/SystemFrameworkMetadataProvider.swift
🧠 Learnings (7)
📓 Common learnings
Learnt from: CR
PR: tuist/XcodeGraph#0
File: AGENT.md:0-0
Timestamp: 2025-08-07T14:29:08.174Z
Learning: Applies to Sources/XcodeMetadata/Providers/**/* : Binary metadata extraction for frameworks and libraries should be implemented in `Sources/XcodeMetadata/Providers/`
Learnt from: CR
PR: tuist/XcodeGraph#0
File: AGENT.md:0-0
Timestamp: 2025-08-07T14:29:08.174Z
Learning: Applies to Sources/XcodeGraph/Models/**/* : Core data structures like Project, Target, Scheme, and BuildConfiguration should be implemented in `Sources/XcodeGraph/Models/`
Learnt from: CR
PR: tuist/XcodeGraph#0
File: AGENT.md:0-0
Timestamp: 2025-08-07T14:29:08.174Z
Learning: Linux support is available with limited testing; XcodeGraphMapperTests and XcodeMetadataTests require Xcode utilities
Learnt from: CR
PR: tuist/XcodeGraph#0
File: AGENT.md:0-0
Timestamp: 2025-08-07T14:29:08.174Z
Learning: Applies to Tests/Fixtures/**/* : XCFramework fixtures for metadata testing should be placed in `Tests/Fixtures/`
📚 Learning: 2025-08-07T14:29:08.174Z
Learnt from: CR
PR: tuist/XcodeGraph#0
File: AGENT.md:0-0
Timestamp: 2025-08-07T14:29:08.174Z
Learning: Applies to Sources/XcodeMetadata/Providers/**/* : Binary metadata extraction for frameworks and libraries should be implemented in `Sources/XcodeMetadata/Providers/`
Applied to files:
Sources/XcodeMetadata/Providers/SystemFrameworkMetadataProvider.swift
📚 Learning: 2025-08-07T14:29:08.174Z
Learnt from: CR
PR: tuist/XcodeGraph#0
File: AGENT.md:0-0
Timestamp: 2025-08-07T14:29:08.174Z
Learning: Applies to Tests/Fixtures/**/* : XCFramework fixtures for metadata testing should be placed in `Tests/Fixtures/`
Applied to files:
Sources/XcodeMetadata/Providers/SystemFrameworkMetadataProvider.swift
📚 Learning: 2025-08-07T14:29:08.174Z
Learnt from: CR
PR: tuist/XcodeGraph#0
File: AGENT.md:0-0
Timestamp: 2025-08-07T14:29:08.174Z
Learning: Applies to Sources/XcodeGraph/Models/**/* : Core data structures like Project, Target, Scheme, and BuildConfiguration should be implemented in `Sources/XcodeGraph/Models/`
Applied to files:
Sources/XcodeMetadata/Providers/SystemFrameworkMetadataProvider.swift
📚 Learning: 2025-08-07T14:29:08.174Z
Learnt from: CR
PR: tuist/XcodeGraph#0
File: AGENT.md:0-0
Timestamp: 2025-08-07T14:29:08.174Z
Learning: Applies to Sources/XcodeGraph/Graph/**/* : Graph representation with dependencies and relationships should be implemented in `Sources/XcodeGraph/Graph/`
Applied to files:
Sources/XcodeMetadata/Providers/SystemFrameworkMetadataProvider.swift
📚 Learning: 2025-08-07T14:29:08.174Z
Learnt from: CR
PR: tuist/XcodeGraph#0
File: AGENT.md:0-0
Timestamp: 2025-08-07T14:29:08.174Z
Learning: Applies to Sources/XcodeGraphMapper/Mappers/**/* : Conversion logic from XcodeProj to XcodeGraph models should be implemented in `Sources/XcodeGraphMapper/Mappers/`
Applied to files:
Sources/XcodeMetadata/Providers/SystemFrameworkMetadataProvider.swift
📚 Learning: 2025-08-07T14:29:08.174Z
Learnt from: CR
PR: tuist/XcodeGraph#0
File: AGENT.md:0-0
Timestamp: 2025-08-07T14:29:08.174Z
Learning: Linux support is available with limited testing; XcodeGraphMapperTests and XcodeMetadataTests require Xcode utilities
Applied to files:
Sources/XcodeMetadata/Providers/SystemFrameworkMetadataProvider.swift
🧬 Code Graph Analysis (1)
Sources/XcodeMetadata/Providers/SystemFrameworkMetadataProvider.swift (2)
Sources/XcodeGraphMapper/Mappers/Targets/PBXTarget+PlatformInference.swift (1)
platform(29-38)Sources/XcodeGraph/Models/DeploymentTargets.swift (1)
macOS(65-67)
🔇 Additional comments (1)
Sources/XcodeMetadata/Providers/SystemFrameworkMetadataProvider.swift (1)
9-9: Good addition: explicit platform-specific error caseAdding
unsupportedSDKPlatformimproves diagnosability and keeps the API precise.
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 3494 @navtoj Excellent work! I can confirm that you've successfully addressed all the feedback: ✅ Edit 2 fix confirmed: You've resolved the parameter reassignment issue by using a local variable ✅ Test coverage: The tests for both the happy path ( ✅ Code consistency: The implementation now follows the existing switch/case pattern consistently. The changes properly handle the XcodeKit framework path as requested in issue #262. Great job on the iterative improvements! 🎯 🧠 Learnings used |
|
Tip For best results, initiate chat on the files or code changes. @navtoj Thanks for the comprehensive update! I can confirm that all the changes you've made based on the feedback are excellent: ✅ The test for The implementation properly addresses issue #262 and the code quality is solid. Great job on iterating through the feedback! 👍 🧠 Learnings used |
fortmarek
left a comment
fortmarek
left a comment
There was a problem hiding this comment.
Thanks for updating the tests, this looks good now 🙂
fortmarek
changed the title
|
@fortmarek I think its because I updated this line after the lint test failed. |
|
@fortmarek All checks passed. I just need to update branch now? 
Which option should I pick? Edit 1 Soo.. I picked "Update with merge commit" and its asking for "workflow approval" again. Was I not supposed to press the "Update branch" button? |
As a first-time contributor, the workflow needs to be approved every time a new commit is pushed to the branch. I'll merge this once the CI passes 🙂 |
2 participants
Resolves: #262
Summary by CodeRabbit
Bug Fixes
Tests