Handle and work around concurrency warnings #23
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR updates the ComposableUserNotifications package to handle Swift 6 concurrency warnings by adding Sendable conformance to types and using @sendable annotations for closures.
- Added
@preconcurrencyimports to suppress warnings from legacy APIs - Made all public types conform to
Sendableprotocol - Wrapped non-Sendable closures with
UncheckedSendableto maintain thread safety
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| Package.swift | Updated Swift tools version from 5.6 to 6.0 |
| Sources/ComposableUserNotifications/Model.swift | Added Sendable conformance and wrapped closures with UncheckedSendable |
| Sources/ComposableUserNotifications/Interface.swift | Added Sendable conformance and @sendable annotations to function types |
| Sources/ComposableUserNotifications/LiveKey.swift | Made Delegate class final and Sendable, wrapped closures with UncheckedSendable |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
|
||
| define udid_for | ||
| $(shell xcrun simctl list --json devices available $(1) | jq -r '.devices | to_entries | map(select(.value | add)) | sort_by(.key) | .[] | select(.key | contains("$(2)")) | .value | last.udid') | ||
| $(shell xcrun simctl list devices available '$(1)' | grep '$(2)' | sort -r | head -1 | awk -F '[()]' '{ print $$(NF-3) }') |
There was a problem hiding this comment.
The new shell command is complex and fragile. The awk field extraction $$(NF-3) assumes a specific format that may break if simulator output changes. Consider adding error handling or using a more robust parsing approach.
| $(shell xcrun simctl list devices available '$(1)' | grep '$(2)' | sort -r | head -1 | awk -F '[()]' '{ print $$(NF-3) }') | |
| $(shell xcrun simctl list devices available '$(1)' | grep '$(2)' | sort -r | head -1 | grep -Eo '[0-9A-Fa-f-]{36}' | head -1) |
|
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. WalkthroughThe PR upgrades the Swift package to Swift 6.0 tooling, updates CI to macOS 15 with Xcode 16.4, modernizes build device matching in the Makefile, and adds comprehensive Sendable conformance and concurrency attributes across the public API surface. Test discovery manifests for non-Objective-C environments are removed. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant UserNotificationClient
participant Delegate
participant UNUserNotificationCenter
Note over Client,UNUserNotificationCenter: With Sendable Conformance
Client->>UserNotificationClient: Call @Sendable closure property
activate UserNotificationClient
UserNotificationClient->>Delegate: Route through AsyncStream (UncheckedSendable)
activate Delegate
Delegate->>UNUserNotificationCenter: Invoke UserNotifications API
activate UNUserNotificationCenter
UNUserNotificationCenter-->>Delegate: Response
deactivate UNUserNotificationCenter
rect rgba(200, 150, 100, 0.1)
Delegate->>Delegate: Wrap completion handler in @Sendable
end
Delegate-->>UserNotificationClient: Yield result
deactivate Delegate
UserNotificationClient-->>Client: Return async value
deactivate UserNotificationClient
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
Sources/ComposableUserNotifications/Model.swift (5)
26-28: Equatable is incorrect when rawValue is nil; compare struct fields as fallbackTwo Notifications built via init(date:request:) both have nil rawValue and currently compare equal, regardless of contents.
Apply:
- public static func == (lhs: Notification, rhs: Notification) -> Bool { - lhs.rawValue == rhs.rawValue - } + public static func == (lhs: Notification, rhs: Notification) -> Bool { + if let l = lhs.rawValue, let r = rhs.rawValue { return l === r } + return lhs.date == rhs.date && lhs.request == rhs.request + }
75-77: Same Equatable pitfall for Request; add identity-or-structural comparisonAvoid “nil rawValue == nil rawValue” collapsing distinct values.
Apply:
- public static func == (lhs: Request, rhs: Request) -> Bool { - lhs.rawValue == rhs.rawValue - } + public static func == (lhs: Request, rhs: Request) -> Bool { + if let l = lhs.rawValue, let r = rhs.rawValue { return l === r } + return lhs.identifier == rhs.identifier + && lhs.content === rhs.content + && lhs.trigger == rhs.trigger + }
127-143: Push.rawValue is never set; == collapses all values to equalThe initializer doesn’t assign rawValue, and equality checks only rawValue. This makes every Push equal.
Apply:
public struct Push: Equatable, Sendable { - nonisolated(unsafe) public var rawValue: UNPushNotificationTrigger? + nonisolated(unsafe) public var rawValue: UNPushNotificationTrigger? @@ - public init(rawValue: UNPushNotificationTrigger) { - self.repeats = rawValue.repeats - } + public init(rawValue: UNPushNotificationTrigger) { + self.rawValue = rawValue + self.repeats = rawValue.repeats + } @@ - public init(repeats: Bool) { - self.repeats = repeats - } + public init(repeats: Bool) { + self.rawValue = nil + self.repeats = repeats + } @@ - public static func == (lhs: Push, rhs: Push) -> Bool { - lhs.rawValue == rhs.rawValue - } + public static func == (lhs: Push, rhs: Push) -> Bool { + if let l = lhs.rawValue, let r = rhs.rawValue { return l === r } + return lhs.repeats == rhs.repeats + }
174-176: TimeInterval equality should also fall back to structural fieldsRaw-only equality yields false positives when either side was constructed without rawValue.
Apply:
- public static func == (lhs: TimeInterval, rhs: TimeInterval) -> Bool { - lhs.rawValue == rhs.rawValue - } + public static func == (lhs: TimeInterval, rhs: TimeInterval) -> Bool { + if let l = lhs.rawValue, let r = rhs.rawValue { return l === r } + return lhs.repeats == rhs.repeats + && lhs.timeInterval == rhs.timeInterval + }
208-210: Calendar equality: add structural fallbackSame rationale as TimeInterval/Request.
Apply:
- public static func == (lhs: Calendar, rhs: Calendar) -> Bool { - lhs.rawValue == rhs.rawValue - } + public static func == (lhs: Calendar, rhs: Calendar) -> Bool { + if let l = lhs.rawValue, let r = rhs.rawValue { return l === r } + return lhs.repeats == rhs.repeats + && lhs.dateComponents == rhs.dateComponents + }
♻️ Duplicate comments (1)
Makefile (1)
51-53: Make UDID parsing resilient; avoid brittle awk field offsetsUse a UUID regex and extended grep. This also addresses the earlier fragility concern.
Apply:
-$(shell xcrun simctl list devices available '$(1)' | grep '$(2)' | sort -r | head -1 | awk -F '[()]' '{ print $$(NF-3) }') +$(shell xcrun simctl list devices available '$(1)' \ + | grep -E "$(2)" \ + | head -1 \ + | grep -Eo '[0-9A-Fa-f-]{36}' \ + | head -1)Optional: if jq is acceptable, the JSON path is even safer:
# $(shell xcrun simctl list devices --json available '$(1)' | jq -r '.devices[][].udid' | head -1)
🧹 Nitpick comments (3)
.github/workflows/ci.yml (1)
25-26: Prefer setup-xcode over hard-coded xcode-select pathHard-coding /Applications/Xcode_16.4.app is brittle across runner images. Use a maintained action to select Xcode by version.
Example:
- - name: Select Xcode 16.4 - run: sudo xcode-select -s /Applications/Xcode_16.4.app + - name: Select Xcode 16.4 + uses: maxim-lobanov/setup-xcode@v1 + with: + xcode-version: '16.4'Optionally add SPM cache to speed builds:
- name: Cache SPM uses: actions/cache@v4 with: path: | ~/.swiftpm .build key: ${{ runner.os }}-spm-${{ hashFiles('**/Package.resolved') }} restore-keys: | ${{ runner.os }}-spm-Makefile (1)
5-6: TV/watch filters are very broad; consider narrowing to a stable default deviceFiltering on “TV” / “Watch” may match unintended entries. Consider targeting a canonical device (e.g., latest Apple TV 4K, Apple Watch Series).
Example (patternized):
PLATFORM_TVOS = tvOS Simulator,id=$(call udid_for,tvOS,Apple TV 4K) PLATFORM_WATCHOS = watchOS Simulator,id=$(call udid_for,Apple Watch Series)Sources/ComposableUserNotifications/LiveKey.swift (1)
115-120: Mirror the same main‑actor handoff for response completion.Keep completion callsite semantics consistent across delegate methods.
- completionHandler: { [handler = UncheckedSendable(completionHandler)] in - handler.value() - } + completionHandler: { [handler = UncheckedSendable(completionHandler)] in + if Thread.isMainThread { + handler.value() + } else { + DispatchQueue.main.async { handler.value() } + } + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
.github/workflows/ci.yml(1 hunks)Makefile(2 hunks)Package.swift(1 hunks)Sources/ComposableUserNotifications/Interface.swift(3 hunks)Sources/ComposableUserNotifications/LiveKey.swift(5 hunks)Sources/ComposableUserNotifications/Model.swift(11 hunks)Tests/ComposableUserNotificationsTests/ComposableUserNotificationsTests.swift(0 hunks)Tests/ComposableUserNotificationsTests/XCTestManifests.swift(0 hunks)
💤 Files with no reviewable changes (2)
- Tests/ComposableUserNotificationsTests/ComposableUserNotificationsTests.swift
- Tests/ComposableUserNotificationsTests/XCTestManifests.swift
🧰 Additional context used
🪛 actionlint (1.7.8)
.github/workflows/ci.yml
24-24: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🔇 Additional comments (9)
Package.swift (1)
1-1: Swift tools 6.0 bump looks goodMatches the concurrency updates elsewhere. No action needed.
Sources/ComposableUserNotifications/Model.swift (2)
150-160: Nice: Sendable wrapper for nextTriggerDateCapturing the ObjC method as a closure via UncheckedSendable is a pragmatic workaround.
Please confirm this compiles cleanly across all package platforms in Swift 6 mode (Xcode 16.4). If any warnings persist, I can help adjust the capture.
481-483: Region Sendable + rawValue nonisolated is consistentLooks good and matches TCA patterns.
Sources/ComposableUserNotifications/Interface.swift (2)
1-1: @preconcurrency import is appropriate herePrevents retroactive Sendable issues with UserNotifications. LGTM.
11-25: Sendable annotations across API surface look consistentClosures and DelegateAction are marked @sendable; equality intentionally ignores the completion handlers. Looks correct.
Consider adding brief doc comments noting that completion handlers must be invoked on the main actor to match UNUserNotificationCenter delegate expectations.
Sources/ComposableUserNotifications/LiveKey.swift (4)
3-3: LGTM on @preconcurrency import.This is the right lever to silence pre‑annotated SDK concurrency warnings while keeping the rest of the file strict.
84-84: AsyncStream.Continuation is Sendable—no action required.AsyncStream.Continuation is documented as Sendable and Swift 6 continues to treat AsyncStream continuation APIs as safe-to-send. The
Delegateclass can safely conform toSendableprovided its other stored properties are also immutable and Sendable.
69-77: Confirm delegate lifecycle management intent.The review comment is substantively correct. The variable
centeris in scope at line 7 and is accessible within theclient.delegateclosure. The current code does not capture or restore the previous delegate, which the review appropriately flags as a potential issue.The review's suggestion is syntactically sound for Swift and feasible to implement. However, the developer must clarify their intended behavior:
- If the caller owns the delegate lifecycle exclusively (recommended unless there are other delegates in the system), apply the suggested refactor to restore the previous delegate on stream termination.
- If cleanup is not needed, confirm this assumption and the review can be closed.
The review comment appropriately requests this verification and provides both the preferred solution and the alternative option. No action is needed from you until you determine your delegate management intent.
100-103: Reject this refactor—the library should not force main thread dispatch.Apple's documentation does not specify which thread the willPresent completion handler is invoked on. However, this does not justify the library adding unconditional dispatch-to-main.
The library correctly defers threading responsibility to consumers. The completion handler is wrapped in
UncheckedSendableand yielded throughAsyncStream, allowing consumers to call it from their chosen execution context. If a consumer requires main thread execution (e.g., for UI updates), they already have the tools:@MainActor,.mainThreadExecutor, or explicit dispatch. The README examples show consumers calling completion directly without expecting main thread dispatch.Silently forcing dispatch-to-main would:
- Introduce an undocumented behavior change with no API contract
- Add unnecessary synchronization overhead for consumers not requiring it
- Violate the principle that libraries should not dictate threading for callers
- Contradict the library's modern async/await design
The current implementation is correct.
| matrix: | ||
| config: ['debug', 'release'] | ||
| steps: | ||
| - uses: actions/checkout@v3 |
There was a problem hiding this comment.
Update checkout action to v4 (v3 is too old on macos-15)
This will avoid runner/runtime warnings.
Apply:
- - uses: actions/checkout@v3
+ - uses: actions/checkout@v4📝 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.
| - uses: actions/checkout@v3 | |
| - uses: actions/checkout@v4 |
🧰 Tools
🪛 actionlint (1.7.8)
24-24: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🤖 Prompt for AI Agents
.github/workflows/ci.yml around line 24: the workflow uses actions/checkout@v3
which triggers runner/runtime warnings on newer macOS runners; update the step
to use actions/checkout@v4 by replacing the uses reference to
actions/checkout@v4 (ensure no other breaking inputs/options are required by v4
in your workflow).
| @@ -1,9 +1,9 @@ | |||
| CONFIG = debug | |||
| PLATFORM_IOS = iOS Simulator,id=$(call udid_for,iPhone,iOS-16) | |||
| PLATFORM_IOS = iOS Simulator,id=$(call udid_for,iOS,iPhone \d\+ Pro [^M]) | |||
There was a problem hiding this comment.
Fix regex: grep default doesn’t support \d. Use -E and [0-9]+; tighten Pro-not-Max match
Current pattern won’t match digits; it literally searches for "d+". Also ensure we exclude "Pro Max" robustly.
Apply:
-PLATFORM_IOS = iOS Simulator,id=$(call udid_for,iOS,iPhone \d\+ Pro [^M])
+PLATFORM_IOS = iOS Simulator,id=$(call udid_for,iOS,iPhone [0-9]+ Pro( \(|$$))Note: The pattern matches “… Pro (” or end-of-line, excluding “Pro Max …”.
📝 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.
| PLATFORM_IOS = iOS Simulator,id=$(call udid_for,iOS,iPhone \d\+ Pro [^M]) | |
| PLATFORM_IOS = iOS Simulator,id=$(call udid_for,iOS,iPhone [0-9]+ Pro( \(|$$)) |
🤖 Prompt for AI Agents
In Makefile around line 2, the grep pattern uses the unsupported \d and can
accidentally match "Pro Max"; change to use extended regex (grep -E) with [0-9]+
for digits and tighten the "Pro" match to exclude "Pro Max" — e.g., use grep -E
'iPhone [0-9]+ Pro($| )' or use grep -E 'iPhone [0-9]+ Pro' combined with a
subsequent grep -v 'Pro Max' to ensure only "Pro" (not "Pro Max") models are
matched.
| public var notificationCategories: @Sendable () async -> Set<UNNotificationCategory> = unimplemented( | ||
| "\(Self.self).deliveredNotifications") |
There was a problem hiding this comment.
Typo in unimplemented placeholder
Use the correct label to aid debugging.
- public var notificationCategories: @Sendable () async -> Set<UNNotificationCategory> = unimplemented(
- "\(Self.self).deliveredNotifications")
+ public var notificationCategories: @Sendable () async -> Set<UNNotificationCategory> = unimplemented(
+ "\(Self.self).notificationCategories")📝 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.
| public var notificationCategories: @Sendable () async -> Set<UNNotificationCategory> = unimplemented( | |
| "\(Self.self).deliveredNotifications") | |
| public var notificationCategories: @Sendable () async -> Set<UNNotificationCategory> = unimplemented( | |
| "\(Self.self).notificationCategories") |
🤖 Prompt for AI Agents
In Sources/ComposableUserNotifications/Interface.swift around lines 38–39, the
unimplemented placeholder message uses the wrong label
("deliveredNotifications"); update it to use the actual property name so the
failure message is accurate—replace the placeholder string with
"\(Self.self).notificationCategories" (or the exact property identifier) when
calling unimplemented.
1 participant
Summary by CodeRabbit