Fix onPreferenceChange modifier on Xcode 16.2 #115
Conversation
WalkthroughThe changes introduce concurrency improvements to the Changes
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🧹 Outside diff range and nitpick comments (1)
Sources/UBUserInterface/SwiftUI/Popup/UBPopupPreferenceKey.swift (1)
Line range hint
29-35: Consider marking the content closure as @mainactorThe content closure might capture references that should only be accessed on the main thread. Consider explicitly marking it as
@MainActor:- @ViewBuilder let content: () -> AnyView + @ViewBuilder let content: @MainActor () -> AnyView
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Sources/UBUserInterface/SwiftUI/Popup/UBPopupPreferenceKey.swift(2 hunks)Sources/UBUserInterface/SwiftUI/Popup/UBPopupStyle.swift(1 hunks)Sources/UBUserInterface/SwiftUI/Popup/UBPopupWrapper.swift(1 hunks)
🔇 Additional comments (4)
Sources/UBUserInterface/SwiftUI/Popup/UBPopupStyle.swift (1)
12-12: LGTM! Appropriate use of Sendable protocol
The addition of Sendable conformance is correct as all properties are implicitly Sendable. This change ensures thread-safe passing of style information across concurrency domains.
Sources/UBUserInterface/SwiftUI/Popup/UBPopupPreferenceKey.swift (2)
23-23: LGTM! Appropriate use of @mainactor
The @MainActor attribute ensures UI-related operations are performed on the main thread, which is crucial for SwiftUI view updates.
Line range hint 40-45: LGTM! Correct implementation of nonisolated operator
The nonisolated keyword is necessary here to allow equality checks from any context, which is required for PreferenceKey.reduce to work properly.
Sources/UBUserInterface/SwiftUI/Popup/UBPopupWrapper.swift (1)
28-34: Verify UBPopupWindowManager thread safety
Ensure that UBPopupWindowManager is properly marked with @MainActor since it handles UI operations.
✅ Verification successful
Thread safety is properly implemented
The UBPopupWindowManager class is already correctly marked with @MainActor attribute, ensuring all UI operations are performed on the main thread. The implementation is thread-safe as required.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for UBPopupWindowManager definition to verify @MainActor usage
ast-grep --pattern 'class UBPopupWindowManager {
$$$
}'
Length of output: 59
Script:
#!/bin/bash
# Let's try a broader search pattern to find UBPopupWindowManager class definition
rg -l "class.*UBPopupWindowManager"
# Also search for the implementation details
rg "UBPopupWindowManager" -A 5
Length of output: 2754
Script:
#!/bin/bash
# Let's get the full content of UBPopupWindowManager.swift to verify thread safety implementation
cat Sources/UBUserInterface/SwiftUI/Popup/UBPopupWindowManager.swift
Length of output: 1792
| DispatchQueue.main.async { | ||
| if let popupPreference, popupPreference.isPresented.wrappedValue { | ||
| UBPopupWindowManager.shared.showPopup(isPresented: popupPreference.isPresented, style: popupPreference.customStyle ?? style, content: popupPreference.content) | ||
| } else { | ||
| UBPopupWindowManager.shared.hideWindow() | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove redundant DispatchQueue.main.async
The onPreferenceChange modifier already executes on the main thread, and UBPopupPreference is marked with @MainActor. Therefore, wrapping the code in DispatchQueue.main.async is redundant and could cause unnecessary animation delays.
Remove the redundant dispatch:
.onPreferenceChange(UBPopupPreferenceKey.self) { popupPreference in
- DispatchQueue.main.async {
- if let popupPreference, popupPreference.isPresented.wrappedValue {
- UBPopupWindowManager.shared.showPopup(isPresented: popupPreference.isPresented, style: popupPreference.customStyle ?? style, content: popupPreference.content)
- } else {
- UBPopupWindowManager.shared.hideWindow()
- }
- }
+ if let popupPreference, popupPreference.isPresented.wrappedValue {
+ UBPopupWindowManager.shared.showPopup(isPresented: popupPreference.isPresented, style: popupPreference.customStyle ?? style, content: popupPreference.content)
+ } else {
+ UBPopupWindowManager.shared.hideWindow()
+ }
}📝 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.
| DispatchQueue.main.async { | |
| if let popupPreference, popupPreference.isPresented.wrappedValue { | |
| UBPopupWindowManager.shared.showPopup(isPresented: popupPreference.isPresented, style: popupPreference.customStyle ?? style, content: popupPreference.content) | |
| } else { | |
| UBPopupWindowManager.shared.hideWindow() | |
| } | |
| } | |
| if let popupPreference, popupPreference.isPresented.wrappedValue { | |
| UBPopupWindowManager.shared.showPopup(isPresented: popupPreference.isPresented, style: popupPreference.customStyle ?? style, content: popupPreference.content) | |
| } else { | |
| UBPopupWindowManager.shared.hideWindow() | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
Sources/UBUserInterface/SwiftUI/Popup/UBPopupWrapper.swift (1)
29-39: 🛠️ Refactor suggestionRemove unnecessary thread checking and dispatching
This issue is similar to a previous comment regarding redundant use of
DispatchQueue.main.async. TheonPreferenceChangemodifier is already executed on the main thread, so checkingThread.isMainThreadand usingDispatchQueue.main.syncis redundant and can introduce unnecessary complexity or potential deadlocks.Simplify the code by directly calling
popupPreferenceChangedwithinMainActor.assumeIsolated:.onPreferenceChange(UBPopupPreferenceKey.self) { popupPreference in - if Thread.isMainThread { - MainActor.assumeIsolated { - popupPreferenceChanged(popupPreference: popupPreference) - } - } else { - Log.reportError("onPreferenceChange called on non-main thread") - DispatchQueue.main.sync { - MainActor.assumeIsolated { - popupPreferenceChanged(popupPreference: popupPreference) - } - } - } + MainActor.assumeIsolated { + popupPreferenceChanged(popupPreference: popupPreference) + } }
🧹 Nitpick comments (2)
Sources/UBUserInterface/SwiftUI/Popup/UBPopupWrapper.swift (2)
45-50: MarkpopupPreferenceChangedwith@MainActorfor claritySince
popupPreferenceChangedperforms UI updates, it's best practice to mark it with@MainActor. This ensures the method is always executed on the main thread and eliminates the need to manually assume isolation.Apply this diff to annotate the method:
+@MainActor private func popupPreferenceChanged(popupPreference: UBPopupPreference?) { if let popupPreference, popupPreference.isPresented.wrappedValue { UBPopupWindowManager.shared.showPopup( isPresented: popupPreference.isPresented, style: popupPreference.customStyle ?? style, content: popupPreference.content ) } else { UBPopupWindowManager.shared.hideWindow() } }🧰 Tools
🪛 SwiftLint (0.57.0)
[Warning] 46-46: Line should be 120 characters or less; currently it has 170 characters
(line_length)
46-46: Line exceeds the maximum recommended lengthLine 46 is 170 characters long, exceeding the recommended maximum of 120 characters. This can affect code readability.
Consider formatting the line to improve readability:
if let popupPreference, popupPreference.isPresented.wrappedValue { - UBPopupWindowManager.shared.showPopup(isPresented: popupPreference.isPresented, style: popupPreference.customStyle ?? style, content: popupPreference.content) + UBPopupWindowManager.shared.showPopup( + isPresented: popupPreference.isPresented, + style: popupPreference.customStyle ?? style, + content: popupPreference.content + ) } else { UBPopupWindowManager.shared.hideWindow() }🧰 Tools
🪛 SwiftLint (0.57.0)
[Warning] 46-46: Line should be 120 characters or less; currently it has 170 characters
(line_length)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Sources/UBUserInterface/SwiftUI/Popup/UBPopupWrapper.swift(2 hunks)
🧰 Additional context used
🪛 SwiftLint (0.57.0)
Sources/UBUserInterface/SwiftUI/Popup/UBPopupWrapper.swift
[Warning] 46-46: Line should be 120 characters or less; currently it has 170 characters
(line_length)
| } else { | ||
| UBPopupWindowManager.shared.hideWindow() | ||
| Log.reportError("onPreferenceChange called on non-main thread") | ||
| DispatchQueue.main.sync { |
There was a problem hiding this comment.
Ev. wäre async hier besser, sonst haben wir möglicherweise einen Deadlock
There was a problem hiding this comment.
--> Async it should be oder so
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
Sources/UBUserInterface/SwiftUI/Popup/UBPopupWrapper.swift (2)
29-41: LGTM: Robust thread safety implementationThe thread safety implementation is well-structured and addresses potential concurrency issues. The use of
MainActor.assumeIsolatedis appropriate for UI updates.Consider making the error message more descriptive:
- Log.reportError("onPreferenceChange called on non-main thread") + Log.reportError("UBPopupWrapper.onPreferenceChange was called on a background thread, which may cause UI inconsistencies")
46-46: Consider improving readability by splitting the long lineThe line exceeds the recommended length of 120 characters.
Consider reformatting for better readability:
- UBPopupWindowManager.shared.showPopup(isPresented: popupPreference.isPresented, style: popupPreference.customStyle ?? style, content: popupPreference.content) + UBPopupWindowManager.shared.showPopup( + isPresented: popupPreference.isPresented, + style: popupPreference.customStyle ?? style, + content: popupPreference.content + )🧰 Tools
🪛 SwiftLint (0.57.0)
[Warning] 46-46: Line should be 120 characters or less; currently it has 170 characters
(line_length)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Sources/UBUserInterface/SwiftUI/Popup/UBPopupWrapper.swift(2 hunks)
🧰 Additional context used
🪛 SwiftLint (0.57.0)
Sources/UBUserInterface/SwiftUI/Popup/UBPopupWrapper.swift
[Warning] 46-46: Line should be 120 characters or less; currently it has 170 characters
(line_length)
🔇 Additional comments (2)
Sources/UBUserInterface/SwiftUI/Popup/UBPopupWrapper.swift (2)
11-11: LGTM: Required import for logging functionality
The addition of UBFoundation import is necessary for the error logging capability.
44-50: LGTM: Well-structured method extraction
The extraction of popup preference handling logic into a separate private method improves code organization and reusability while maintaining the original functionality.
🧰 Tools
🪛 SwiftLint (0.57.0)
[Warning] 46-46: Line should be 120 characters or less; currently it has 170 characters
(line_length)
Summary by CodeRabbit
New Features
Sendableprotocol in popup styles.Bug Fixes
Documentation