-
Notifications
You must be signed in to change notification settings - Fork 0
Refactor FXIOS-13122 [Swift 6] Remove @Sendable from closures marked as @MainActor #17
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: cursor_combined_20260121_qodo_grep_cursor_copilot_1_base_refactor_fxios-13122_swift_6_remove_sendable_from_closures_marked_as_mainactor_pr208
Are you sure you want to change the base?
Refactor FXIOS-13122 [Swift 6] Remove @Sendable from closures marked as @MainActor #17
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
| guard threshold <= 0 || lastExecutionTime.timeIntervalSinceNow < -threshold else { return } | ||
| func throttle(completion: @escaping @MainActor () -> Void) { | ||
| lastExecutionTime = Date() | ||
| guard threshold <= 0 || lastExecutionTime.timeIntervalSinceNow < -threshold else { return } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Throttler never executes completions due to reordered logic
High Severity
The reordering of statements in throttle completely breaks the throttler. Setting lastExecutionTime = Date() before the guard check means lastExecutionTime.timeIntervalSinceNow is always approximately 0. The condition 0 < -threshold is always false for positive thresholds, so the guard always fails and completions are never executed. The original logic set lastExecutionTime only after the guard passed.
| ensureMainThread { | ||
| completionHandler(.performDefaultHandling, nil) | ||
| } | ||
| completionHandler(.performDefaultHandling, nil) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inconsistent threading for completion handler in handleServerTrust
Medium Severity
The ensureMainThread wrapper was removed from one completionHandler call but kept on the other. Now completionHandler(.performDefaultHandling, nil) is called directly from within dispatchQueue.async, meaning it executes on a background thread, while completionHandler(.useCredential, credential) is still wrapped with ensureMainThread. This inconsistency could cause threading issues with the WebKit callback.
Benchmark PR from qodo-benchmark#208
Note
Modernizes concurrency annotations by stripping
@Sendablefrom@MainActorclosures/completions throughout BrowserKit, Client, and tests, and aligning call sites and protocols.@MainActor-only closures; updates mocks accordinglyensureMainThreadhelpers; generic variant now returnsT?and only returns a value when already on the main thread@SendablefromReducertypealias; updatesWKEngineWebView.evaluateJavaScriptcompletion typeWebServernon-final; minor cleanup inDispatchQueueInterface.ensureMainThreadMainThreadThrottler.throttleimplementation order (sets timestamp before gating)Written by Cursor Bugbot for commit 38810ec. Configure here.