fix: Only build files on OSs that are relevant#75
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughPlus Ultra! This PR adds conditional compilation guards ( Changes
Sequence Diagram(s)Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #75 +/- ##
=====================================
Coverage 5.85% 5.85%
=====================================
Files 71 71
Lines 3640 3640
=====================================
Hits 213 213
Misses 3427 3427 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
Sources/CareKitEssentials/Cards/watchOS/DigitalCrown/DigitalCrownView.swift (1)
73-76: Fear not! I spotted a small villain hiding in your code, young hero!This appears to be a pre-existing copy-paste error—
isHeaderPaddedis used for the footer's padding condition instead ofisFooterPadded. When these values differ (e.g., in the initializer at lines 130-131), the footer padding won't behave as intended!🐛 Proposed fix
footer - .if(isHeaderPadded) { + .if(isFooterPadded) { $0.padding([.horizontal, .bottom]) }Sources/CareKitEssentials/Extensions/OCKAnyEventStore.swift (1)
9-83: PLUS ULTRA: The guard must extend to ALL wrappers!Your concern is spot-on! The core implementation in
OCKAnyEventStore.swiftis properly guarded by#if canImport(os.log), but the wrappers that call it are not:
OCKAnyEvent.swift(lines 23–24): Wrapper is guarded only by#if canImport(SwiftUI), notos.log. On platforms with SwiftUI but withoutos.log, this wrapper will be compiled and will call a method that doesn't exist—compilation failure.
CareStoreFetchedResult.swift(lines 15–23): No guard whatsoever. This extension is unconditionally declared and will unconditionally try to calltoggleBooleanOutcomemethods that don't exist on platforms withoutos.log—critical compilation failure.The wrapper methods in both files must also be guarded by
#if canImport(os.log)to prevent symbol availability mismatches.Sources/CareKitEssentials/Cards/Shared/Chart/Protocols/CareKitEssentialChartable.swift (1)
9-16: Consider addingcanImport(Charts)to align with established patterns in the codebase.While the current
#if canImport(SwiftUI)guard works correctly—since your minimum deployment targets (iOS 18.0, macOS 15.0, watchOS 11.0) all exceed Charts' requirements—other files likeOCKOutcomeValue+Plottable.swiftuse#if canImport(Charts)for Charts-specific code. Adding it here would be more explicit and defensive:Suggested change
-#if canImport(SwiftUI) +#if canImport(SwiftUI) && canImport(Charts)Sources/CareKitEssentials/Cards/Shared/Chart/Data/CKEDataSeriesConfiguration.swift (1)
118-140: WAIT, YOUNG HERO! I've spotted a VILLAINOUS BUG lurking in your initializer!Look closely at line 139 - you're assigning
xAxisLabeltoself.yAxisLabelinstead ofself.xAxisLabel! ThexAxisLabelproperty at line 54 is NEVER being assigned in the initializer, which means it will always benilregardless of what value the caller provides!🐛 Proposed fix to correctly assign xAxisLabel
self.legendTitle = legendTitle + self.xAxisLabel = xAxisLabel self.yAxisLabel = yAxisLabel self.showMarkWhenHighlighted = showMarkWhenHighlightedSources/CareKitEssentials/Extensions/OCKAnyEvent.swift (1)
160-162: YOUNG HERO, YOUR SENSES DO NOT DECEIVE YOU! The danger is REAL!The
image()method at lines 131-133 returnsImage?(a SwiftUI type) but stands UNGUARDED without#if canImport(SwiftUI)! This is a CRITICAL ISSUE that will cause compilation failure on non-SwiftUI platforms like Linux! The file itself shows the proper pattern—other SwiftUI types likeTextare correctly guarded. Theimage()method must follow suit!🦸 The Plus Ultra fix to guard the image() method
+ `#if` canImport(SwiftUI) func image() -> Image? { Image.asset(self.task.asset) } + `#endif`
🤖 Fix all issues with AI agents
In `@Sources/CareKitEssentials/Models/ScheduleUtility.swift`:
- Around line 9-13: The build breaks because OCKAnyEvent.detail calls
ScheduleUtility.scheduleLabel(for:) but ScheduleUtility is only compiled inside
`#if` canImport(SwiftUI); fix by guarding the OCKAnyEvent.detail implementation
with the same `#if` canImport(SwiftUI) conditional (or alternatively remove the
SwiftUI-only gate from Models/ScheduleUtility.swift if ScheduleUtility should be
available on all targets); locate the detail property in
Extensions/OCKAnyEvent.swift and wrap its declaration/usage of
ScheduleUtility.scheduleLabel(for: self) in `#if` canImport(SwiftUI) ... `#endif` so
non-SwiftUI targets no longer reference ScheduleUtility.
There was a problem hiding this comment.
Pull request overview
This pull request adds conditional compilation guards to ensure files are only built on platforms where their dependencies are available. The changes wrap platform-specific code with #if canImport() directives and remove unused code from tests.
Changes:
- Added conditional compilation guards (
#if canImport(SwiftUI),#if canImport(Charts),#if canImport(os.log)) to platform-specific files - Removed unused
expectedGoalvariables from LinearCareTaskProgressTests - Removed unnecessary Swift version guards in favor of @available attributes
Reviewed changes
Copilot reviewed 55 out of 55 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| Tests/CareKitEssentialsTests/LinearCareTaskProgressTests.swift | Removed unused expectedGoal variables from three test methods |
| Sources/CareKitEssentials/Models/ScheduleUtility.swift | Added #if canImport(SwiftUI) guard around entire file |
| Sources/CareKitEssentials/Models/PeriodComponent.swift | Removed unused SwiftUI import and Swift version guards |
| Sources/CareKitEssentials/Extensions/View+Default.swift | Added #if canImport(SwiftUI) guard |
| Sources/CareKitEssentials/Extensions/OCKStore.swift | Added conditional #if canImport(os.log) around utility methods |
| Sources/CareKitEssentials/Extensions/OCKOutcomeValue+Plottable.swift | Added #if canImport(Charts) guard |
| Sources/CareKitEssentials/Extensions/OCKAnyEventStore.swift | Added #if canImport(os.log) guard |
| Sources/CareKitEssentials/Extensions/OCKAnyEvent.swift | Added conditional imports and guards for SwiftUI-specific properties |
| Sources/CareKitEssentials/Extensions/Logger.swift | Added #if canImport(os.log) guard |
| Sources/CareKitEssentials/Extensions/Image.swift | Added #if canImport(SwiftUI) guard |
| Sources/CareKitEssentials/Extensions/ChartContent+Default.swift | Added #if canImport(Charts) guard |
| Sources/CareKitEssentials/Extensions/CGFloat.swift | Added #if canImport(SwiftUI) guard |
| Sources/CareKitEssentials/Environment/CardEnabledEnvironmentKey.swift | Added #if canImport(SwiftUI) guard |
| Sources/CareKitEssentials/Cards/watchOS/DigitalCrown/* | Updated guards from #if os(watchOS) to #if canImport(SwiftUI) && os(watchOS) |
| Sources/CareKitEssentials/Cards/Shared/SliderLog/* | Updated guards from #if !os(watchOS) to #if canImport(SwiftUI) && !os(watchOS) |
| Sources/CareKitEssentials/Cards/Shared/* | Added #if canImport(SwiftUI) guards to various view files |
| Sources/CareKitEssentials/Cards/Shared/Chart/* | Added #if canImport(SwiftUI) guards and removed Swift version guards |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Sources/CareKitEssentials/Cards/Shared/Chart/CareKitEssentialChartBodyView.swift
Outdated
Show resolved
Hide resolved
Sources/CareKitEssentials/Cards/Shared/EventViews/EventQueryContentView.swift
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/CareKitEssentials/Extensions/OCKAnyEvent.swift (1)
27-42: Keepdetailaccessible outside the SwiftUI guard—it's a core property with no UI dependency.The
detailproperty returns a plainString?and has zero SwiftUI dependencies. Moving it behind#if canImport(SwiftUI)gates it away from non-UI targets (CLI/server code), which breaks the API's accessibility. All current usages in the codebase access it from UI components, but the property itself should remain available everywhere.Relocate
detailoutside the guard and gate only thedetailTextandinstructionsTextproperties that actually depend on SwiftUI'sTexttype. This preserves backward compatibility at full PLUS ULTRA power!💡 Suggested adjustment
+/// Denotes the time and date of the event. +var detail: String? { + ScheduleUtility.scheduleLabel(for: self) +} + `#if` canImport(SwiftUI) -/// Denotes the time and date of the event. -var detail: String? { - ScheduleUtility.scheduleLabel(for: self) -} var detailText: Text? { guard let detail else { return nil } return Text(detail) }
🤖 Fix all issues with AI agents
In `@Sources/CareKitEssentials/Extensions/OCKAnyEvent.swift`:
- Around line 11-13: The image() -> Image? function returns a SwiftUI type but
is not conditionally compiled; wrap the entire image() implementation in the
same `#if` canImport(SwiftUI) / `#endif` guard used for detailText and
instructionsText so non-SwiftUI builds don't reference Image, and provide a
non-SwiftUI fallback signature (e.g., return nil or an alternate API) for the
else branch to keep the API surface consistent; update the function named
image() in OCKAnyEvent to be inside that conditional compilation block.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/CareKitEssentials/Extensions/OCKAnyEvent.swift (1)
27-42: Consider movingdetailoutside the SwiftUI guard, Hero!The
detailproperty returnsString?and relies only onScheduleUtility.scheduleLabel(for:), which has no SwiftUI dependencies. By placing it inside the#if canImport(SwiftUI)block, you're unnecessarily restricting its availability on non-SwiftUI platforms (like server-side Swift), which appears to be the very use case this PR aims to support! Plus Ultra!The
detailTextandinstructionsTextproperties correctly need the guard since they return SwiftUITexttypes.💪 Proposed fix to restore availability on all platforms
+ /// Denotes the time and date of the event. + var detail: String? { + ScheduleUtility.scheduleLabel(for: self) + } + `#if` canImport(SwiftUI) - /// Denotes the time and date of the event. - var detail: String? { - ScheduleUtility.scheduleLabel(for: self) - } - var detailText: Text? { guard let detail else { return nil } return Text(detail) } var instructionsText: Text? { guard let instructions else { return nil } return Text(instructions) } `#endif`
♻️ Duplicate comments (1)
Sources/CareKitEssentials/Extensions/OCKAnyEvent.swift (1)
160-164: EXCELLENT WORK, Young Hero! You've addressed the previous review concern! Plus Ultra!The
image()function returns a SwiftUIImage?type and is now properly wrapped with#if canImport(SwiftUI), ensuring non-SwiftUI builds won't break. This is exactly the fix that was needed!
1 participant
This pull request adds conditional compilation guards to ensure files are only built on platforms where their dependencies are available. The changes wrap platform-specific code with
#if canImport()directives and remove unused code from tests.Changes:
#if canImport(SwiftUI),#if canImport(Charts),#if canImport(os.log)) to platform-specific filesexpectedGoalvariables from LinearCareTaskProgressTests