Refactor FXIOS-13122 [Swift 6] Remove @Sendable from closures marked as @MainActor#33
Conversation
WalkthroughThis pull request systematically removes the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes 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
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
BrowserKit/Sources/MenuKit/MenuElement.swift (1)
7-20: Change closure type to@Sendable@mainactor() -> Voidfor properSendableconformance.
MenuElementconforms toSendablebut theactionproperty is typed as(@mainactor() -> Void)?. In Swift 6, a@MainActorclosure is not implicitlySendable—it must be explicitly marked as@Sendable. Update the property and initializer parameter to use(@sendable@mainactor() -> Void)?to satisfy theSendableprotocol requirement.Suggested change
// Line 20 public let action: (`@Sendable` `@MainActor` () -> Void)? // Line 37 (initializer parameter) action: (`@Sendable` `@MainActor` () -> Void)?firefox-ios/Client/Frontend/Browser/BrowserViewController/Extensions/BrowserViewController+WebViewDelegates.swift (1)
1329-1351: Potential actor isolation issue:completionHandlercalled from background queue.At Line 1343,
completionHandler(.performDefaultHandling, nil)is called directly insidedispatchQueue.async { ... }, which runs on a background thread. ThecompletionHandleris marked@MainActor, so calling it from a non-main-actor context may violate Swift 6's strict concurrency model.Line 1348-1349 correctly wraps the call in
ensureMainThread, but Line 1343 does not.🔧 Proposed fix to ensure main actor isolation
guard let trust = challenge.protectionSpace.serverTrust, let cert = SecTrustCopyCertificateChain(trust) as? [SecCertificate], self.profile.certStore.containsCertificate(cert[0], forOrigin: origin) else { - completionHandler(.performDefaultHandling, nil) + ensureMainThread { + completionHandler(.performDefaultHandling, nil) + } return }firefox-ios/Client/Frontend/Browser/BrowserViewController/WebEngineIntegration/BrowserWebUIDelegate.swift (1)
38-50: Change@Sendableto@MainActorto match protocol definition.Line 42 incorrectly uses
@Sendable () -> Voidfor the completion handler, violating theWKUIHandlerprotocol definition which requires@MainActor () -> Void. This inconsistency with the confirm panel (line 56) and all other implementations in the codebase suggests an oversight.Update line 42 from:
completionHandler: `@escaping` `@Sendable` () -> Voidto:
completionHandler: `@escaping` `@MainActor` () -> Void
🤖 Fix all issues with AI agents
In `@BrowserKit/Sources/Common/Utilities/DispatchQueueInterface.swift`:
- Line 13: MockDispatchQueue’s ensureMainThread signature must match the
protocol DispatchQueueInterface: change the implementation in MockDispatchQueue
(the ensureMainThread method) to accept an escaping closure annotated with
`@MainActor` and `@convention`(block) and the same return type (Swift.Void). In
short, update MockDispatchQueue.ensureMainThread(execute:) to: func
ensureMainThread(execute work: `@escaping` `@MainActor` `@convention`(block) () ->
Swift.Void) so it conforms to the DispatchQueueInterface protocol.
In `@firefox-ios/Client/Utils/MainThreadThrottler.swift`:
- Around line 26-28: The throttle logic in MainThreadThrottler is wrong because
lastExecutionTime is set before the guard, making
lastExecutionTime.timeIntervalSinceNow near zero and causing the guard to always
return; move the assignment so you check the guard first (use threshold and
lastExecutionTime as-is), then when the guard passes set lastExecutionTime =
Date() and finally call DispatchQueue.main.async(execute: completion) so
completion actually runs.
🧹 Nitpick comments (1)
firefox-ios/Client/Application/WebServer.swift (1)
16-19: Consider keepingWebServerfinalwhile it remains@unchecked Sendable.Removing
finalwidens subclassing surface; subclasses could add non‑sendable or mutable state that undermines the unchecked sendability and singleton assumptions. If subclassing isn’t required, restoringfinalkeeps the safety envelope tight.🔧 Suggested tweak
-class WebServer: WebServerProtocol, `@unchecked` Sendable { +final class WebServer: WebServerProtocol, `@unchecked` Sendable {
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (40)
BrowserKit/Sources/Common/Utilities/DispatchQueueHelper.swiftBrowserKit/Sources/Common/Utilities/DispatchQueueInterface.swiftBrowserKit/Sources/MenuKit/MenuElement.swiftBrowserKit/Sources/Redux/Reducer.swiftBrowserKit/Sources/Shared/UIDeviceDetails.swiftBrowserKit/Sources/WebEngine/WKWebview/WKEngineWebView.swiftBrowserKit/Tests/WebEngineTests/Mock/MockWKEngineWebView.swiftfirefox-ios/Client/AccountSyncHandler.swiftfirefox-ios/Client/Application/WebServer.swiftfirefox-ios/Client/Frontend/AuthenticationManager/AppAuthenticator.swiftfirefox-ios/Client/Frontend/Autofill/CreditCard/CreditCardSettingsView/CreditCardInputViewModel.swiftfirefox-ios/Client/Frontend/Browser/BrowserViewController/Actions/WebContextMenuActionsProvider.swiftfirefox-ios/Client/Frontend/Browser/BrowserViewController/Extensions/BrowserViewController+WebViewDelegates.swiftfirefox-ios/Client/Frontend/Browser/BrowserViewController/WebEngineIntegration/BrowserWebUIDelegate.swiftfirefox-ios/Client/Frontend/Browser/DownloadHelper/DownloadHelper.swiftfirefox-ios/Client/Frontend/Browser/MainMenuActionHelper.swiftfirefox-ios/Client/Frontend/Browser/RelayController.swiftfirefox-ios/Client/Frontend/Browser/SearchEngines/SearchEngineProvider.swiftfirefox-ios/Client/Frontend/Browser/SearchEngines/SearchEnginesManager.swiftfirefox-ios/Client/Frontend/Browser/TabScrollController/TabProviderAdapter.swiftfirefox-ios/Client/Frontend/Home/Homepage/SectionHeader/LabelButtonHeaderView.swiftfirefox-ios/Client/Frontend/Library/Bookmarks/Legacy/BookmarksPanelViewModel.swiftfirefox-ios/Client/Frontend/Library/ClearHistorySheetProvider.swiftfirefox-ios/Client/Frontend/Library/HistoryPanel/HistoryPanelViewModel.swiftfirefox-ios/Client/Frontend/Onboarding/Models/UpdateViewModel.swiftfirefox-ios/Client/Frontend/PasswordManagement/PasswordManagerViewModel.swiftfirefox-ios/Client/Frontend/Settings/PasswordDetailViewController.swiftfirefox-ios/Client/Frontend/Widgets/PhotonActionSheet/SingleActionViewModel.swiftfirefox-ios/Client/TabManagement/Tab.swiftfirefox-ios/Client/Utils/MainThreadThrottler.swiftfirefox-ios/Extensions/ShareTo/InitialViewController.swiftfirefox-ios/RustFxA/FirefoxAccountSignInViewController.swiftfirefox-ios/RustFxA/FxAWebViewController.swiftfirefox-ios/Storage/Queue.swiftfirefox-ios/Storage/SQL/SQLiteQueue.swiftfirefox-ios/firefox-ios-tests/Tests/ClientTests/Frontend/Homepage/Mock/MockThrottler.swiftfirefox-ios/firefox-ios-tests/Tests/ClientTests/Mocks/MockAppAuthenticator.swiftfirefox-ios/firefox-ios-tests/Tests/ClientTests/Mocks/MockProfile.swiftfirefox-ios/firefox-ios-tests/Tests/ClientTests/Mocks/MockTabProviderProtocol.swiftfirefox-ios/firefox-ios-tests/Tests/ClientTests/Mocks/MockTabWebView.swift
🧰 Additional context used
🧬 Code graph analysis (9)
firefox-ios/Storage/Queue.swift (2)
firefox-ios/Storage/SQL/SQLiteQueue.swift (1)
getQueuedTabs(24-34)firefox-ios/firefox-ios-tests/Tests/ClientTests/Mocks/MockProfile.swift (1)
getQueuedTabs(93-98)
firefox-ios/firefox-ios-tests/Tests/ClientTests/Frontend/Homepage/Mock/MockThrottler.swift (1)
firefox-ios/Client/Utils/MainThreadThrottler.swift (1)
throttle(25-29)
firefox-ios/Storage/SQL/SQLiteQueue.swift (1)
firefox-ios/firefox-ios-tests/Tests/ClientTests/Mocks/MockProfile.swift (1)
getQueuedTabs(93-98)
firefox-ios/firefox-ios-tests/Tests/ClientTests/Mocks/MockTabWebView.swift (1)
firefox-ios/Client/TabManagement/Tab.swift (1)
getSessionCookies(858-860)
firefox-ios/Client/Frontend/Library/HistoryPanel/HistoryPanelViewModel.swift (1)
firefox-ios/Client/Frontend/Library/HistoryPanel/HistoryPanel+Search.swift (1)
performSearch(53-57)
BrowserKit/Sources/Common/Utilities/DispatchQueueInterface.swift (2)
BrowserKit/Sources/Common/Utilities/DispatchQueueHelper.swift (2)
ensureMainThread(10-20)ensureMainThread(22-33)firefox-ios/firefox-ios-tests/Tests/ClientTests/Mocks/MockDispatchQueue.swift (1)
ensureMainThread(30-33)
BrowserKit/Sources/Common/Utilities/DispatchQueueHelper.swift (1)
BrowserKit/Sources/Common/Utilities/DispatchQueueInterface.swift (2)
ensureMainThread(38-48)async(24-29)
firefox-ios/firefox-ios-tests/Tests/ClientTests/Mocks/MockAppAuthenticator.swift (1)
firefox-ios/Client/Frontend/AuthenticationManager/AppAuthenticator.swift (1)
getAuthenticationState(29-46)
firefox-ios/firefox-ios-tests/Tests/ClientTests/Mocks/MockProfile.swift (1)
firefox-ios/Storage/SQL/SQLiteQueue.swift (1)
getQueuedTabs(24-34)
🔇 Additional comments (61)
firefox-ios/Client/Frontend/Library/Bookmarks/Legacy/BookmarksPanelViewModel.swift (1)
118-122: Looks good —@MainActorisolation remains clear.Removing
@Sendablehere aligns with the Swift 6 cleanup and keeps the callback constrained to the main actor.firefox-ios/Client/AccountSyncHandler.swift (1)
19-19: Looks good:@MainActorisolation is preserved.firefox-ios/Client/Frontend/Browser/MainMenuActionHelper.swift (3)
125-127: Signature change looks consistent with@MainActorusage.
164-167: Completion handler isolation remains correct.
543-545: @mainactor closure type is appropriate here.firefox-ios/Client/Frontend/Autofill/CreditCard/CreditCardSettingsView/CreditCardInputViewModel.swift (1)
258-280: LGTM!The removal of
@Sendablewhile retaining@MainActoron the completion closure is correct for Swift 6 concurrency. The@MainActorannotation already guarantees main-thread isolation, and the implementation correctly usesensureMainThreadblocks when invoking the completion handler.firefox-ios/firefox-ios-tests/Tests/ClientTests/Mocks/MockTabProviderProtocol.swift (1)
15-15: LGTM!The mock correctly mirrors the protocol change. Since the class is
@MainActorisolated, removing@Sendableis appropriate—the closure is only set and invoked within the main actor context.firefox-ios/Client/Frontend/Browser/TabScrollController/TabProviderAdapter.swift (2)
14-14: LGTM!Removing
@Sendablefrom the protocol property is correct. Since the protocol is@MainActorisolated, the closure is inherently constrained to the main actor context, making@Sendableredundant.
32-35: LGTM!The implementation correctly reflects the protocol change. The closure type now matches the relaxed concurrency constraint while maintaining
@MainActorisolation.firefox-ios/Client/TabManagement/Tab.swift (2)
857-860: LGTM —@MainActorcompletion remains clear and aligned.The change cleanly drops redundant
@Sendablewhile preserving main‑actor isolation.
1210-1216: LGTM — updated override signature stays consistent with Swift 6 intent.The completion handler remains
@MainActor, and the removal of@Sendableis consistent with the refactor goal.firefox-ios/Client/Frontend/Home/Homepage/SectionHeader/LabelButtonHeaderView.swift (1)
95-121: LGTM! Appropriate removal of redundant@Sendableconstraint.Removing
@Sendablefrom this@MainActor-isolated UI callback closure is correct. SincemoreButtonActionis a button tap handler that:
- Is already constrained to execute on the main actor
- Is configured from UI code that's inherently on the main thread
- Is passed to
ActionButtonViewModel.touchUpAction, which already has the matching type(@mainactor(UIButton) -> Void)?The
@Sendablerequirement was unnecessarily restrictive for callers, and the change is consistent withActionButtonViewModel's parameter definition.firefox-ios/Client/Utils/MainThreadThrottler.swift (1)
9-9: LGTM on@Sendableremoval.Removing
@Sendablefrom@MainActorclosures aligns with Swift 6 concurrency patterns—@MainActoralready guarantees isolation on the main actor, making@Sendableredundant. The protocol and implementation signatures are now consistent withMockThrottler.Also applies to: 25-25
firefox-ios/Client/Application/WebServer.swift (1)
55-62: LGTM —@MainActorhandler without@Sendableis consistent.This aligns with the goal of relaxing unnecessary sendability on main‑actor closures while keeping the handler isolation intact.
firefox-ios/RustFxA/FirefoxAccountSignInViewController.swift (1)
305-326: LGTM!The removal of
@Sendablefrom the@MainActorcompletion closure is correct. Since@MainActoralready isolates execution to the main actor, the@Sendableattribute was redundant. The completion is properly invoked within the nested callback chain.firefox-ios/Client/Frontend/Browser/SearchEngines/SearchEnginesManager.swift (1)
278-289: LGTM!The removal of
@Sendableis appropriate here. TheSearchEnginesManagerclass is already@MainActorisolated, so the completion closure executes within the same actor context. No cross-boundary sendability is required.firefox-ios/Client/Frontend/Library/ClearHistorySheetProvider.swift (4)
23-31: LGTM!The removal of
@Sendablefrom thedidCompleteclosure is correct. The class is@MainActorisolated, and the closure is invoked from UIKit alert action handlers which run on the main thread.
52-59: LGTM!Consistent with the pattern applied throughout this class.
61-84: LGTM!The
didCompleteclosure is correctly typed as@MainActorwithout@Sendable, matching the class isolation context.
86-105: LGTM!Consistent signature change. The closure is invoked within the
@MainActorcontext of the class.BrowserKit/Sources/Shared/UIDeviceDetails.swift (1)
38-50: LGTM!The removal of
@Sendablefrom theworkclosure is appropriate. The function explicitly handles main-thread execution viaDispatchQueue.main.syncorMainActor.assumeIsolated, and@MainActorsufficiently constrains the closure's execution context. The return typeT: Sendableconstraint correctly ensures the returned value can be safely transferred.firefox-ios/Extensions/ShareTo/InitialViewController.swift (1)
98-109: LGTM!The removal of
@Sendableis correct. The completion closure is invoked withinDispatchQueue.main.async, and the@MainActorannotation ensures proper main-thread execution without requiring cross-boundary sendability.BrowserKit/Sources/Redux/Reducer.swift (1)
9-9: Removal of@SendablefromReducertypealias is safe.The Redux architecture uses reducers exclusively as static properties or local captures invoked within the
Store's@MainActorcontext. No code passesReducerinstances across actor boundaries, and no explicitSendableconstraints exist in the codebase. The@MainActorannotation is sufficient to ensure proper concurrency isolation.firefox-ios/Client/Frontend/Onboarding/Models/UpdateViewModel.swift (1)
76-81: LGTM!The removal of
@Sendableis appropriate here. The closure remains@MainActor-isolated and the implementation correctly invokes it viaensureMainThread, ensuring main thread execution.firefox-ios/RustFxA/FxAWebViewController.swift (1)
158-179: LGTM!The
@Sendableremoval from thedecisionHandleris consistent with the PR objectives. The@preconcurrency import WebKiton line 5 appropriately handles any signature differences with Apple's SDK during the Swift 6 migration, allowing this simplified closure type while maintaining correct behavior.firefox-ios/firefox-ios-tests/Tests/ClientTests/Frontend/Homepage/Mock/MockThrottler.swift (1)
13-18: LGTM!The mock signature correctly matches the real implementation in
MainThreadThrottler.swift(lines 24-28), ensuring consistent protocol conformance between production code and test doubles.firefox-ios/Client/Frontend/Browser/DownloadHelper/DownloadHelper.swift (1)
63-65: LGTM!The removal of
@Sendablefrom theokActionparameter is appropriate. Since the method is already@MainActor-isolated and the closure is invoked within this context (line 108), the explicit@Sendableannotation was redundant.firefox-ios/Client/Frontend/Browser/SearchEngines/SearchEngineProvider.swift (1)
8-21: LGTM!The
@Sendableremoval fromSearchEngineCompletiontypealias is correct. Since this is used as a method parameter type (line 21), it doesn't affect theSearchEngineProviderprotocol'sSendableconformance—that applies to conforming types' stored state, not method parameter closures.firefox-ios/Client/Frontend/PasswordManagement/PasswordManagerViewModel.swift (2)
83-83: Signature update is consistent with@MainActorusage.No behavioral impact beyond relaxing sendability constraints.
162-162: Signature update is consistent with@MainActorusage.No behavioral impact beyond relaxing sendability constraints.
BrowserKit/Sources/WebEngine/WKWebview/WKEngineWebView.swift (2)
74-79: Protocol signature change looks good.Keeps
@MainActorisolation while relaxing sendability.
85-90: Extension forwarding signature looks good.Matches protocol change without altering behavior.
BrowserKit/Tests/WebEngineTests/Mock/MockWKEngineWebView.swift (1)
136-140: Mock signature update aligns with new protocol.Consistent with the production API change.
firefox-ios/Client/Frontend/AuthenticationManager/AppAuthenticator.swift (3)
22-25: Protocol signatures updated consistently.No behavioral change; only sendability relaxation.
29-30: Implementation signature matches protocol.Maintains
@MainActorisolation.
48-50: Implementation signature matches protocol.Maintains
@MainActorisolation.firefox-ios/Client/Frontend/Widgets/PhotonActionSheet/SingleActionViewModel.swift (2)
44-57: Closure property types updated consistently.No change in behavior; still main-actor bound.
74-76: Initializer parameter types updated consistently.Matches the property types without functional changes.
firefox-ios/firefox-ios-tests/Tests/ClientTests/Mocks/MockProfile.swift (1)
93-97: Mock signature update aligns with production API.No logic change; test behavior preserved.
firefox-ios/Storage/SQL/SQLiteQueue.swift (1)
24-34: LGTM!The removal of
@Sendableis consistent with theTabQueueprotocol definition and mock implementations. The@MainActorannotation already provides the necessary isolation guarantees, and the implementation correctly usesMainActor.assumeIsolatedafter dispatching to the main queue.firefox-ios/Client/Frontend/Browser/RelayController.swift (1)
10-10: LGTM!The removal of
@SendablefromRelayPopulateCompletionis appropriate since the typealias is used within the@MainActor-isolatedRelayControllerclass and protocol methods. The main-actor isolation provides sufficient guarantees without the additional@Sendablerequirement.firefox-ios/Client/Frontend/Library/HistoryPanel/HistoryPanelViewModel.swift (1)
130-158: LGTM!The removal of
@SendablefromperformSearch's completion closure is correct. The@MainActorannotation ensures the closure runs on the main actor, making@Sendableredundant. The implementation properly usesMainActor.assumeIsolatedafter dispatching via.uponQueue(.main).Note that
reloadData(line 95) correctly retains@Sendablewithout@MainActorsince its completion is called from a background queue.firefox-ios/Storage/Queue.swift (1)
8-12: LGTM!The protocol method signature change is consistent with the implementations in
SQLiteQueueandMockProfile. Removing@Sendablewhile retaining@MainActoris the correct approach—the main-actor isolation provides the necessary thread-safety guarantees for the completion closure.firefox-ios/firefox-ios-tests/Tests/ClientTests/Mocks/MockTabWebView.swift (1)
112-114: LGTM!The mock correctly matches the base class signature from
Tab.getSessionCookies. The removal of@Sendableis consistent with the parent class change, and the@MainActorisolation is preserved.firefox-ios/Client/Frontend/Settings/PasswordDetailViewController.swift (1)
402-417: LGTM!The removal of
@Sendablefrom the completion closure is appropriate. The@MainActorannotation ensures the closure executes on the main actor, which aligns with theDispatchQueue.main.asyncdispatch at line 405. Since this is an internal method, the change has no API compatibility impact.BrowserKit/Sources/Common/Utilities/DispatchQueueHelper.swift (2)
10-20: LGTM!The removal of
@Sendablewhile preserving@MainActor@convention(block)is correct and aligns with the protocol definition inDispatchQueueInterface.swift.
22-32: Return type addition is not a breaking change.The generic
ensureMainThread<T>function signature change to returnT?is not a breaking API change. All existing callers pass Void-returning closures and do not attempt to capture any return value, so the addition of the optional return type does not break their usage. This is an enhancement to support returning values when called from the main thread, but existing code patterns continue to work without modification.Likely an incorrect or invalid review comment.
firefox-ios/Client/Frontend/Browser/BrowserViewController/Actions/WebContextMenuActionsProvider.swift (2)
31-31: LGTM!The
@Sendableremoval fromaddTabclosures is consistent. The class is already@MainActorisolated, so these closures don't need the additional@Sendableconstraint.Also applies to: 44-44
162-164: Consistent annotation update.The
writeToPhotoAlbumclosure correctly retains@MainActorwhile removing@Sendable. Note thatgetImageData's inner callback on Line 163 correctly keeps@Sendablesince it's invoked asynchronously from a different context.firefox-ios/firefox-ios-tests/Tests/ClientTests/Mocks/MockAppAuthenticator.swift (1)
14-18: LGTM!The mock's signatures correctly mirror the production
AppAuthenticatorAPI (as shown in the relevant snippet). TheensureMainThreadwrapper properly ensures completion handlers execute on the main actor.Also applies to: 24-34
firefox-ios/Client/Frontend/Browser/BrowserViewController/WebEngineIntegration/BrowserWebUIDelegate.swift (2)
52-80: LGTM!The
@Sendableremoval fromrunJavaScriptConfirmPanelWithMessageandrunJavaScriptTextInputPanelWithPromptcompletion handlers is consistent with the PR objective.
86-116: LGTM!The
@Sendableremoval fromcontextMenuConfigurationForElementandrequestMediaCapturePermissionForhandlers aligns with the PR's goal of relaxing concurrency constraints while preserving@MainActorisolation.firefox-ios/Client/Frontend/Browser/BrowserViewController/Extensions/BrowserViewController+WebViewDelegates.swift (9)
76-92: LGTM!The
handleJavaScriptAlerthelper'sspamCallbackparameter correctly uses@MainActorwithout@Sendable.
94-140: LGTM!All JavaScript panel delegate methods (
runJavaScriptAlertPanelWithMessage,runJavaScriptConfirmPanelWithMessage,runJavaScriptTextInputPanelWithPrompt) consistently use@MainActorclosures without@Sendable.
153-196: LGTM!The
contextMenuConfigurationForElementandrequestMediaCapturePermissionFordelegate methods correctly adopt@MainActorclosures.
200-204: LGTM!The
handleSpammedJSAlerthelper's callback parameter correctly uses@MainActor.
313-319: LGTM!The
createActionsmethod'saddTabclosure parameter aligns with the updatedWebContextMenuActionsProvidersignatures.
704-710: LGTM!The navigation response policy decision handler correctly uses
@MainActor.
931-933: LGTM!The
downloadActionclosure correctly uses@MainActorfor UI updates.
1052-1057: LGTM!The authentication challenge completion handler correctly uses
@MainActor.
1234-1235: LGTM!The
showExternalAlertcompletion handler correctly uses@MainActor.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| execute work: @escaping @Sendable @convention(block) () -> Void) | ||
|
|
||
| func ensureMainThread(execute work: @escaping @MainActor @Sendable @convention(block) () -> Swift.Void) | ||
| func ensureMainThread(execute work: @escaping @MainActor @convention(block) () -> Swift.Void) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find and examine DispatchQueueInterface.swift
find . -name "DispatchQueueInterface.swift" -type fRepository: qodo-benchmark/firefox-ios-combined-coderabbit
Length of output: 156
🏁 Script executed:
# Find all mock dispatch queue implementations
find . -name "*Mock*Dispatch*" -o -name "*Dispatch*Mock*" | head -20Repository: qodo-benchmark/firefox-ios-combined-coderabbit
Length of output: 325
🏁 Script executed:
# Search for ensureMainThread method signatures
rg -n "func ensureMainThread" --type swift -A 2Repository: qodo-benchmark/firefox-ios-combined-coderabbit
Length of output: 1928
🏁 Script executed:
# Check if MockDispatchQueue conforms to DispatchQueueInterface
rg -A 5 "class MockDispatchQueue" firefox-ios/firefox-ios-tests/Tests/ClientTests/Mocks/MockDispatchQueue.swiftRepository: qodo-benchmark/firefox-ios-combined-coderabbit
Length of output: 299
🏁 Script executed:
# Check the full DispatchQueueInterface protocol
cat -n BrowserKit/Sources/Common/Utilities/DispatchQueueInterface.swiftRepository: qodo-benchmark/firefox-ios-combined-coderabbit
Length of output: 2398
🏁 Script executed:
# Verify that the mock's signature is indeed incompatible by checking Swift conformance rules
# Get full context around the mock implementation
sed -n '25,40p' firefox-ios/firefox-ios-tests/Tests/ClientTests/Mocks/MockDispatchQueue.swiftRepository: qodo-benchmark/firefox-ios-combined-coderabbit
Length of output: 492
MockDispatchQueue must be updated to match the protocol signature.
The protocol (line 13) and extension (line 38) signatures are consistent. However, MockDispatchQueue does not properly conform to the updated protocol—its ensureMainThread implementation lacks the required @MainActor and @convention(block) attributes:
Protocol requires: func ensureMainThread(execute work: `@escaping` `@MainActor` `@convention`(block) () -> Swift.Void)
Mock provides: func ensureMainThread(execute work: `@escaping` () -> Void)
Update the mock implementation in firefox-ios/firefox-ios-tests/Tests/ClientTests/Mocks/MockDispatchQueue.swift (line 30) to include the missing attributes.
🤖 Prompt for AI Agents
In `@BrowserKit/Sources/Common/Utilities/DispatchQueueInterface.swift` at line 13,
MockDispatchQueue’s ensureMainThread signature must match the protocol
DispatchQueueInterface: change the implementation in MockDispatchQueue (the
ensureMainThread method) to accept an escaping closure annotated with `@MainActor`
and `@convention`(block) and the same return type (Swift.Void). In short, update
MockDispatchQueue.ensureMainThread(execute:) to: func ensureMainThread(execute
work: `@escaping` `@MainActor` `@convention`(block) () -> Swift.Void) so it conforms
to the DispatchQueueInterface protocol.
| lastExecutionTime = Date() | ||
| guard threshold <= 0 || lastExecutionTime.timeIntervalSinceNow < -threshold else { return } | ||
| DispatchQueue.main.async(execute: completion) |
There was a problem hiding this comment.
Critical: Throttle logic is broken—completion will never execute.
Setting lastExecutionTime = Date() before the guard makes lastExecutionTime.timeIntervalSinceNow effectively zero. The condition ~0 < -threshold is always false for any positive threshold (e.g., 0.35), so the guard always returns early and completion is never dispatched.
The assignment should occur after the guard passes.
🐛 Proposed fix
func throttle(completion: `@escaping` `@MainActor` () -> Void) {
+ guard threshold <= 0 || lastExecutionTime.timeIntervalSinceNow < -threshold else { return }
lastExecutionTime = Date()
- guard threshold <= 0 || lastExecutionTime.timeIntervalSinceNow < -threshold else { return }
DispatchQueue.main.async(execute: completion)
}🤖 Prompt for AI Agents
In `@firefox-ios/Client/Utils/MainThreadThrottler.swift` around lines 26 - 28, The
throttle logic in MainThreadThrottler is wrong because lastExecutionTime is set
before the guard, making lastExecutionTime.timeIntervalSinceNow near zero and
causing the guard to always return; move the assignment so you check the guard
first (use threshold and lastExecutionTime as-is), then when the guard passes
set lastExecutionTime = Date() and finally call
DispatchQueue.main.async(execute: completion) so completion actually runs.
2 participants
Benchmark PR from agentic-review-benchmarks#7
Summary by CodeRabbit
@Sendableconstraints from completion handlers and callback closures throughout the codebase while preserving@MainActorannotations. This change relaxes concurrency requirements across dispatch queues, UI delegates, managers, and related utilities.✏️ Tip: You can customize this high-level summary in your review settings.