Skip to content

Conversation

@tomerqodo
Copy link

@tomerqodo tomerqodo commented Jan 22, 2026

Benchmark PR from qodo-benchmark#208

Summary by CodeRabbit

  • Refactor
    • Improved internal concurrency handling to enhance callback management across the application.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link

coderabbitai bot commented Jan 22, 2026

Walkthrough

This PR systematically removes the @Sendable concurrency constraint from closure and callback parameters across BrowserKit and firefox-ios, while preserving @MainActor isolation. Changes affect approximately 40 files spanning protocol definitions, public APIs, and implementations in UI, networking, state management, and testing layers. Several changes also modify return types or control flow (e.g., DispatchQueueHelper generic overload now returns T? instead of T, MainThreadThrottler timing adjustment).

Changes

Cohort / File(s) Summary
DispatchQueue Utilities
BrowserKit/Sources/Common/Utilities/DispatchQueueHelper.swift, BrowserKit/Sources/Common/Utilities/DispatchQueueInterface.swift
Removed @Sendable from closure parameters; generic ensureMainThread overload now returns T? instead of T, with updated control flow to return values when on main thread and nil when dispatching asynchronously.
UI & State Management
BrowserKit/Sources/MenuKit/MenuElement.swift, BrowserKit/Sources/Redux/Reducer.swift, BrowserKit/Sources/Shared/UIDeviceDetails.swift
Removed @Sendable from action closure and completion handler types across MenuElement, Reducer typealias, and getMainThreadDataSynchronously.
WebView & JavaScript
BrowserKit/Sources/WebEngine/WKWebview/WKEngineWebView.swift, BrowserKit/Tests/WebEngineTests/Mock/MockWKEngineWebView.swift, firefox-ios/Client/TabManagement/Tab.swift
Removed @Sendable from completion handler closures in evaluateJavaScript methods and session cookie callbacks.
Authentication
firefox-ios/Client/Frontend/AuthenticationManager/AppAuthenticator.swift, firefox-ios/Client/AccountSyncHandler.swift, firefox-ios/RustFxA/FirefoxAccountSignInViewController.swift
Removed @Sendable from completion handlers in authentication state checks, device owner authentication, and Debouncer action callbacks.
Web Delegates & Context Menus
firefox-ios/Client/Frontend/Browser/BrowserViewController/Extensions/BrowserViewController+WebViewDelegates.swift, firefox-ios/Client/Frontend/Browser/BrowserViewController/WebEngineIntegration/BrowserWebUIDelegate.swift, firefox-ios/Client/Frontend/Browser/BrowserViewController/Actions/WebContextMenuActionsProvider.swift
Removed @Sendable from multiple WKUIDelegate completion handlers (JavaScript alerts/inputs, context menu configuration, media capture permissions) and context menu action closures.
Search & Engine Management
firefox-ios/Client/Frontend/Browser/SearchEngines/SearchEngineProvider.swift, firefox-ios/Client/Frontend/Browser/SearchEngines/SearchEnginesManager.swift
Removed @Sendable from SearchEngineCompletion typealias and deleteCustomEngine completion closure.
Bookmarks & History
firefox-ios/Client/Frontend/Library/Bookmarks/Legacy/BookmarksPanelViewModel.swift, firefox-ios/Client/Frontend/Library/ClearHistorySheetProvider.swift, firefox-ios/Client/Frontend/Library/HistoryPanel/HistoryPanelViewModel.swift
Removed @Sendable from pin/unpin action success handlers, history deletion completion callbacks, and search result completions.
Password Management & Settings
firefox-ios/Client/Frontend/PasswordManagement/PasswordManagerViewModel.swift, firefox-ios/Client/Frontend/Settings/PasswordDetailViewController.swift, firefox-ios/Client/Frontend/Autofill/CreditCard/CreditCardSettingsView/CreditCardInputViewModel.swift
Removed @Sendable from login query/save completions and password detail sync callbacks.
UI Components & Home
firefox-ios/Client/Frontend/Home/Homepage/SectionHeader/LabelButtonHeaderView.swift, firefox-ios/Client/Frontend/Widgets/PhotonActionSheet/SingleActionViewModel.swift
Removed @Sendable from button action and view model handler closures (tapHandler, customRender, customHeight).
Tab & Browser State
firefox-ios/Client/Frontend/Browser/TabScrollController/TabProviderAdapter.swift, firefox-ios/Client/Frontend/Browser/RelayController.swift, firefox-ios/Client/Frontend/Browser/MainMenuActionHelper.swift
Removed @Sendable from onLoadingStateChanged property, RelayPopulateCompletion typealias, and toolbar/menu action completions.
Download & Storage
firefox-ios/Client/Frontend/Browser/DownloadHelper/DownloadHelper.swift, firefox-ios/Storage/Queue.swift, firefox-ios/Storage/SQL/SQLiteQueue.swift
Removed @Sendable from download action and queued tabs completion handlers.
Threading & Utility
firefox-ios/Client/Utils/MainThreadThrottler.swift
Removed @Sendable from throttle completion; also adjusted control flow to update lastExecutionTime before guard evaluation.
Extensions & Navigation
firefox-ios/Extensions/ShareTo/InitialViewController.swift, firefox-ios/RustFxA/FxAWebViewController.swift, firefox-ios/Client/Application/WebServer.swift
Removed @Sendable from share item and navigation decision handlers; removed final keyword from WebServer class.
Onboarding
firefox-ios/Client/Frontend/Onboarding/Models/UpdateViewModel.swift
Removed @Sendable from hasSyncableAccount completion.
Test Mocks
firefox-ios/firefox-ios-tests/Tests/ClientTests/Frontend/Homepage/Mock/MockThrottler.swift, firefox-ios/firefox-ios-tests/Tests/ClientTests/Mocks/MockAppAuthenticator.swift, firefox-ios/firefox-ios-tests/Tests/ClientTests/Mocks/MockProfile.swift, firefox-ios/firefox-ios-tests/Tests/ClientTests/Mocks/MockTabProviderProtocol.swift, firefox-ios/firefox-ios-tests/Tests/ClientTests/Mocks/MockTabWebView.swift
Updated mock signatures to match removal of @Sendable from completion handlers and callback closures.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~35 minutes

The changes follow a highly repetitive pattern (removing @Sendable constraints) across many files, reducing per-file review complexity. However, the scope (~40 files across two codebases) and need to verify alignment of related signatures throughout the codebase requires careful attention. Several files also include functional changes (DispatchQueueHelper return type modification, MainThreadThrottler timing adjustment, WebServer finality removal) that warrant additional scrutiny.

Poem

🐰 Constraints relaxed, the closures now roam,
No Sendable shackles on threads back home,
MainActor keeps guard where concurrency flows,
Lighter the burdens our closures now know!

🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 16.90% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely summarizes the main change: removing @Sendable from closures marked as @MainActor for Swift 6 compatibility.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
firefox-ios/Client/Frontend/Browser/BrowserViewController/Extensions/BrowserViewController+WebViewDelegates.swift (1)

1329-1352: Inconsistent main-thread dispatch in handleServerTrust.

There's an inconsistency in how completionHandler is invoked:

  • Line 1343: Called directly inside dispatchQueue.async without ensuring main thread
  • Line 1349: Wrapped in ensureMainThread

Since completionHandler is @MainActor, both call sites should consistently use ensureMainThread to avoid relying on implicit actor hop behavior.

Suggested fix for consistency
             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
             }
🤖 Fix all issues with AI agents
In `@firefox-ios/Client/Utils/MainThreadThrottler.swift`:
- Around line 25-29: In throttle(completion:) the guard must run before updating
lastExecutionTime so the elapsed-time check uses the previous timestamp; move
the assignment to lastExecutionTime to after the guard (or compute now first and
compare against the existing lastExecutionTime) and only update
lastExecutionTime when you actually allow execution, then
DispatchQueue.main.async(execute: completion) as before; reference symbols:
throttle(completion:), lastExecutionTime, threshold, and
DispatchQueue.main.async.
🧹 Nitpick comments (1)
firefox-ios/Client/Frontend/Widgets/PhotonActionSheet/SingleActionViewModel.swift (1)

97-117: Note: copy() omits customRender and customHeight.

This is pre-existing behavior unrelated to the current refactor, but worth noting: the copy() function only propagates tapHandler but not customRender or customHeight. If this is intentional, consider adding a brief comment; otherwise, these properties could be included for completeness.

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 8b0fbc5 and 38810ec.

📒 Files selected for processing (40)
  • BrowserKit/Sources/Common/Utilities/DispatchQueueHelper.swift
  • BrowserKit/Sources/Common/Utilities/DispatchQueueInterface.swift
  • BrowserKit/Sources/MenuKit/MenuElement.swift
  • BrowserKit/Sources/Redux/Reducer.swift
  • BrowserKit/Sources/Shared/UIDeviceDetails.swift
  • BrowserKit/Sources/WebEngine/WKWebview/WKEngineWebView.swift
  • BrowserKit/Tests/WebEngineTests/Mock/MockWKEngineWebView.swift
  • firefox-ios/Client/AccountSyncHandler.swift
  • firefox-ios/Client/Application/WebServer.swift
  • firefox-ios/Client/Frontend/AuthenticationManager/AppAuthenticator.swift
  • firefox-ios/Client/Frontend/Autofill/CreditCard/CreditCardSettingsView/CreditCardInputViewModel.swift
  • firefox-ios/Client/Frontend/Browser/BrowserViewController/Actions/WebContextMenuActionsProvider.swift
  • firefox-ios/Client/Frontend/Browser/BrowserViewController/Extensions/BrowserViewController+WebViewDelegates.swift
  • firefox-ios/Client/Frontend/Browser/BrowserViewController/WebEngineIntegration/BrowserWebUIDelegate.swift
  • firefox-ios/Client/Frontend/Browser/DownloadHelper/DownloadHelper.swift
  • firefox-ios/Client/Frontend/Browser/MainMenuActionHelper.swift
  • firefox-ios/Client/Frontend/Browser/RelayController.swift
  • firefox-ios/Client/Frontend/Browser/SearchEngines/SearchEngineProvider.swift
  • firefox-ios/Client/Frontend/Browser/SearchEngines/SearchEnginesManager.swift
  • firefox-ios/Client/Frontend/Browser/TabScrollController/TabProviderAdapter.swift
  • firefox-ios/Client/Frontend/Home/Homepage/SectionHeader/LabelButtonHeaderView.swift
  • firefox-ios/Client/Frontend/Library/Bookmarks/Legacy/BookmarksPanelViewModel.swift
  • firefox-ios/Client/Frontend/Library/ClearHistorySheetProvider.swift
  • firefox-ios/Client/Frontend/Library/HistoryPanel/HistoryPanelViewModel.swift
  • firefox-ios/Client/Frontend/Onboarding/Models/UpdateViewModel.swift
  • firefox-ios/Client/Frontend/PasswordManagement/PasswordManagerViewModel.swift
  • firefox-ios/Client/Frontend/Settings/PasswordDetailViewController.swift
  • firefox-ios/Client/Frontend/Widgets/PhotonActionSheet/SingleActionViewModel.swift
  • firefox-ios/Client/TabManagement/Tab.swift
  • firefox-ios/Client/Utils/MainThreadThrottler.swift
  • firefox-ios/Extensions/ShareTo/InitialViewController.swift
  • firefox-ios/RustFxA/FirefoxAccountSignInViewController.swift
  • firefox-ios/RustFxA/FxAWebViewController.swift
  • firefox-ios/Storage/Queue.swift
  • firefox-ios/Storage/SQL/SQLiteQueue.swift
  • firefox-ios/firefox-ios-tests/Tests/ClientTests/Frontend/Homepage/Mock/MockThrottler.swift
  • firefox-ios/firefox-ios-tests/Tests/ClientTests/Mocks/MockAppAuthenticator.swift
  • firefox-ios/firefox-ios-tests/Tests/ClientTests/Mocks/MockProfile.swift
  • firefox-ios/firefox-ios-tests/Tests/ClientTests/Mocks/MockTabProviderProtocol.swift
  • firefox-ios/firefox-ios-tests/Tests/ClientTests/Mocks/MockTabWebView.swift
🧰 Additional context used
🧬 Code graph analysis (7)
firefox-ios/firefox-ios-tests/Tests/ClientTests/Mocks/MockTabWebView.swift (1)
firefox-ios/Client/TabManagement/Tab.swift (1)
  • getSessionCookies (858-860)
firefox-ios/firefox-ios-tests/Tests/ClientTests/Frontend/Homepage/Mock/MockThrottler.swift (1)
firefox-ios/Client/Utils/MainThreadThrottler.swift (1)
  • throttle (25-29)
BrowserKit/Sources/Common/Utilities/DispatchQueueHelper.swift (1)
BrowserKit/Sources/Common/Utilities/DispatchQueueInterface.swift (2)
  • ensureMainThread (38-48)
  • async (24-29)
firefox-ios/Storage/Queue.swift (1)
firefox-ios/Storage/SQL/SQLiteQueue.swift (1)
  • getQueuedTabs (24-34)
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)
firefox-ios/Client/TabManagement/Tab.swift (1)
firefox-ios/firefox-ios-tests/Tests/ClientTests/Mocks/MockTabWebView.swift (1)
  • getSessionCookies (112-114)
🔇 Additional comments (60)
firefox-ios/Client/Frontend/Home/Homepage/SectionHeader/LabelButtonHeaderView.swift (1)

95-100: LGTM — signature change matches PR intent.
Removing @Sendable while keeping @MainActor is consistent with Swift 6 cleanup and doesn’t affect call sites here.

firefox-ios/Extensions/ShareTo/InitialViewController.swift (1)

98-108: LGTM — @Sendable removal is safe here.

@MainActor remains and the implementation still marshals to the main queue, so dropping @Sendable doesn’t introduce new risks.

firefox-ios/Client/Frontend/PasswordManagement/PasswordManagerViewModel.swift (2)

83-84: LGTM: @MainActor-only completion is appropriate here.


162-163: LGTM: completion closure aligns with MainActor isolation.

firefox-ios/Client/Frontend/Browser/SearchEngines/SearchEngineProvider.swift (1)

8-8: LGTM — @MainActor completion remains isolated.

No concerns with removing @Sendable here.

firefox-ios/Client/Frontend/Browser/SearchEngines/SearchEnginesManager.swift (1)

278-278: LGTM — completion still runs on MainActor.

Signature change is consistent with the PR goal.

firefox-ios/RustFxA/FirefoxAccountSignInViewController.swift (1)

305-305: LGTM — @Sendable removal is consistent with @MainActor isolation.

firefox-ios/Client/Frontend/Browser/BrowserViewController/Actions/WebContextMenuActionsProvider.swift (1)

30-41: LGTM! Clean Swift 6 concurrency refactor.

The removal of @Sendable from @MainActor-isolated closures is correct. In Swift 6, combining @Sendable with @MainActor is redundant since actor isolation already constrains how the closure can be used. The changes:

  1. addOpenInNewTab / addOpenInNewPrivateTab: The addTab closures are properly main-actor isolated.
  2. addSaveImage: The writeToPhotoAlbum closure correctly removes @Sendable while getImageData's inner callback appropriately retains @Sendable since it's not actor-isolated.

Also applies to: 43-54, 161-184

firefox-ios/RustFxA/FxAWebViewController.swift (1)

158-179: LGTM!

The removal of @Sendable from the decisionHandler closure parameter is correct for Swift 6 compatibility. Since the closure is already annotated with @MainActor, it's confined to the main actor, making @Sendable redundant. The method implementation correctly calls decisionHandler in all code paths, and the @preconcurrency import WebKit at line 5 appropriately handles backward compatibility.

firefox-ios/Client/Frontend/Library/ClearHistorySheetProvider.swift (1)

23-25: LGTM! Consistent removal of @Sendable from @MainActor closures.

The removal of @Sendable across all four method signatures (showClearRecentHistory, setupActions, addDeleteSomeData, addDeleteEverythingOption) is correct for Swift 6 compatibility. Since the class is already @MainActor and the closures are explicitly marked @MainActor, the @Sendable annotation is redundant—@MainActor isolation provides the necessary thread-safety guarantees.

Also applies to: 52-54, 61-63, 86-88

firefox-ios/Client/Frontend/Widgets/PhotonActionSheet/SingleActionViewModel.swift (2)

44-56: LGTM! Correct Swift 6 refactoring.

Removing @Sendable from @MainActor closures is appropriate. In Swift 6, @MainActor-isolated closures are implicitly Sendable due to their actor isolation, making the explicit annotation redundant. The struct's Sendable conformance remains valid.


74-76: Initializer parameters correctly updated.

The parameter types now match the corresponding property types, maintaining consistency throughout the struct.

firefox-ios/Client/Frontend/Browser/RelayController.swift (1)

10-10: LGTM!

Removing @Sendable from RelayPopulateCompletion is appropriate since the closure is exclusively used within @MainActor-isolated contexts (the RelayController class itself is @MainActor-isolated). The completion handlers are invoked on the main actor, so @Sendable is unnecessary.

BrowserKit/Sources/Shared/UIDeviceDetails.swift (1)

38-50: LGTM!

Removing @Sendable from the work closure parameter is correct. The closure is executed synchronously via MainActor.assumeIsolated or DispatchQueue.main.sync, meaning it never crosses isolation boundaries asynchronously. The T: Sendable constraint on the return type is sufficient to ensure the result can be safely returned.

firefox-ios/Client/Frontend/Library/HistoryPanel/HistoryPanelViewModel.swift (1)

130-158: LGTM!

Removing @Sendable from the performSearch completion is appropriate. The completion is always invoked within MainActor.assumeIsolated after dispatching to .main queue (line 137), ensuring main-actor isolation without requiring @Sendable.

Note that reloadData (line 95) and fetchData (line 214) correctly retain @Sendable on their closures since those are called from background contexts (DispatchQueue.global()).

firefox-ios/Client/AccountSyncHandler.swift (1)

19-29: LGTM!

Removing @Sendable from the action closure is correct. The Debouncer class is @MainActor-isolated, so the Task created on line 24 inherits this isolation. The closure executes entirely within the main actor context and doesn't cross isolation boundaries.

firefox-ios/firefox-ios-tests/Tests/ClientTests/Mocks/MockTabWebView.swift (1)

112-114: LGTM!

The MockTab.getSessionCookies override correctly matches the updated signature in Tab.swift (shown in relevant snippets). Both use @MainActor @escaping`` without @Sendable, maintaining consistency between the production code and test mock.

firefox-ios/Client/Frontend/Onboarding/Models/UpdateViewModel.swift (1)

76-81: LGTM!

Removing @Sendable from the completion parameter is appropriate. The closure is invoked via ensureMainThread, guaranteeing execution on the main actor. Since the closure doesn't cross isolation boundaries, @Sendable is unnecessary.

BrowserKit/Sources/Redux/Reducer.swift (1)

9-9: LGTM!

Removing @Sendable from the @MainActor-isolated Reducer typealias is correct for Swift 6 compatibility. Since reducers are designed to run exclusively on the main actor for state management, the @Sendable constraint was redundant.

firefox-ios/firefox-ios-tests/Tests/ClientTests/Mocks/MockProfile.swift (1)

93-98: LGTM!

The mock implementation correctly aligns with the updated TabQueue protocol signature. The Task { @mainactor in ... } dispatch pattern properly ensures main-actor execution for the completion handler.

firefox-ios/firefox-ios-tests/Tests/ClientTests/Mocks/MockTabProviderProtocol.swift (1)

15-15: LGTM!

Appropriate change. Since MockTabProviderProtocol is already @MainActor-isolated (line 9), the callback will only be invoked from the main actor context, making @Sendable unnecessary.

firefox-ios/Storage/SQL/SQLiteQueue.swift (1)

24-34: LGTM!

The implementation correctly aligns with the updated TabQueue protocol. The use of .uponQueue(.main) combined with MainActor.assumeIsolated properly bridges GCD-based main queue execution to Swift's actor model, as noted in the existing FXIOS-13228 comment.

firefox-ios/Storage/Queue.swift (1)

8-12: LGTM!

The protocol change appropriately removes @Sendable while preserving @MainActor isolation on the completion handler. The TabQueue protocol itself remains Sendable, correctly allowing instances to be passed across concurrency domains while ensuring completion callbacks execute on the main actor.

firefox-ios/Client/Application/WebServer.swift (2)

18-18: Clarify intent: final modifier removed from class declaration.

The removal of final is a separate change from the @Sendable refactor. Removing final allows subclassing of WebServer, which may have implications for the singleton pattern used at line 19 and the thread-safety concerns noted in the FIXME comment.

Was this removal intentional? If subclassing is not required, consider keeping final to prevent unintended inheritance and enable compiler optimizations.


55-84: LGTM on the @Sendable removal from the handler closure.

The change correctly removes @Sendable from the outer handler closure while retaining it on the inner responseCompletion. This is appropriate because:

  • The handler is explicitly dispatched via ensureMainThread (line 76), so @MainActor isolation is sufficient
  • The responseCompletion correctly keeps @Sendable since it must be callable from any context back to GCDWebServer
firefox-ios/Client/Frontend/Library/Bookmarks/Legacy/BookmarksPanelViewModel.swift (1)

118-122: LGTM.

Signature update looks consistent with the refactor scope.

firefox-ios/Client/Frontend/Settings/PasswordDetailViewController.swift (1)

402-415: LGTM.

No behavioral changes; aligns with the PR’s @Sendable removal.

firefox-ios/Client/Frontend/Browser/TabScrollController/TabProviderAdapter.swift (2)

8-15: LGTM.

Protocol change is clear and consistent.


32-35: LGTM.

Adapter remains aligned with the protocol update.

BrowserKit/Sources/MenuKit/MenuElement.swift (2)

20-20: LGTM.

Public API change matches the refactor intent.


37-37: LGTM.

Initializer signature is consistent with the property update.

firefox-ios/Client/Frontend/Autofill/CreditCard/CreditCardSettingsView/CreditCardInputViewModel.swift (1)

258-260: LGTM.

Signature update is consistent with the rest of the refactor.

firefox-ios/Client/Frontend/Browser/DownloadHelper/DownloadHelper.swift (1)

64-66: LGTM.

Signature update is straightforward and consistent.

BrowserKit/Sources/WebEngine/WKWebview/WKEngineWebView.swift (2)

74-79: LGTM - Protocol signature correctly updated.

The removal of @Sendable from the completionHandler closure is appropriate since the protocol is already marked @MainActor, which provides sufficient isolation guarantees for Swift 6 concurrency.


84-95: Extension signature matches protocol definition.

The default implementation correctly mirrors the updated protocol signature, maintaining consistency.

firefox-ios/Client/Utils/MainThreadThrottler.swift (1)

9-10: Protocol signature update is correct.

Removing @Sendable while retaining @MainActor is appropriate for Swift 6 compatibility.

firefox-ios/firefox-ios-tests/Tests/ClientTests/Frontend/Homepage/Mock/MockThrottler.swift (1)

13-18: Mock signature correctly updated to match protocol.

The MockThrottler implementation aligns with the updated MainThreadThrottlerProtocol signature.

BrowserKit/Tests/WebEngineTests/Mock/MockWKEngineWebView.swift (1)

136-146: Mock signature correctly updated to match protocol.

The completionHandler parameter type is now consistent with the WKEngineWebView protocol definition.

firefox-ios/Client/TabManagement/Tab.swift (2)

857-860: Signature update is correct.

The @Sendable removal from getSessionCookies completion is appropriate since the Tab class is marked @MainActor and the completion closure retains @MainActor isolation.


1209-1217: Override signature correctly updated for Swift 6.

The #if compiler(>=6) conditional compilation ensures the override matches the updated WKWebView API signature in Swift 6, while the #else branch handles older compilers with the unavailability annotation.

firefox-ios/Client/Frontend/Browser/MainMenuActionHelper.swift (3)

125-126: Signature update is correct.

The completion parameter correctly retains @MainActor isolation while removing the redundant @Sendable constraint within this @MainActor class.


166-188: Signature update is correct.

The dataLoadingCompletion parameter update is appropriate. The completion is invoked via group.notify(queue: .main), ensuring main thread execution consistent with @MainActor.


543-551: Local closure type update is correct.

The action closure type correctly removes @Sendable while maintaining @MainActor isolation, consistent with the broader refactoring pattern.

firefox-ios/Client/Frontend/AuthenticationManager/AppAuthenticator.swift (2)

19-26: LGTM - Protocol signatures correctly updated.

The removal of @Sendable from the @MainActor completion handlers is appropriate. Since these closures are isolated to MainActor, the @Sendable constraint was redundant and potentially conflicting under Swift 6's stricter concurrency model.


28-49: LGTM - Implementation matches protocol.

The AppAuthenticator implementation correctly mirrors the updated protocol signatures. The completion handlers are properly dispatched via DispatchQueue.main.async, ensuring main-thread execution aligns with the @MainActor annotation.

firefox-ios/firefox-ios-tests/Tests/ClientTests/Mocks/MockAppAuthenticator.swift (2)

14-18: LGTM - Mock signature aligns with protocol.

The mock correctly implements the updated AppAuthenticationProtocol signature without @Sendable.


24-34: LGTM - Authentication mock updated consistently.

The authenticateWithDeviceOwnerAuthentication signature matches the protocol, and ensureMainThread properly ensures main-thread execution for the @MainActor closure.

firefox-ios/Client/Frontend/Browser/BrowserViewController/WebEngineIntegration/BrowserWebUIDelegate.swift (2)

52-64: LGTM - WKUIDelegate completion handlers updated.

The @Sendable removal from @MainActor completion handlers is consistent with the PR's Swift 6 compatibility goal. The @preconcurrency import WebKit (line 5) handles any bridging concerns with WebKit's delegate expectations.


86-116: LGTM - Context menu and media capture handlers updated.

Both contextMenuConfigurationForElement and requestMediaCapturePermissionFor completion handlers correctly remove @Sendable while retaining @MainActor isolation.

firefox-ios/Client/Frontend/Browser/BrowserViewController/Extensions/BrowserViewController+WebViewDelegates.swift (7)

76-92: LGTM - JavaScript alert handler updated.

The spamCallback parameter correctly removes @Sendable while keeping @MainActor.


94-140: LGTM - JavaScript panel handlers updated consistently.

All three JavaScript panel methods (runJavaScriptAlertPanelWithMessage, runJavaScriptConfirmPanelWithMessage, runJavaScriptTextInputPanelWithPrompt) have completion handlers correctly updated to remove @Sendable.


153-196: LGTM - Context menu and media capture delegates updated.

The completion/decision handlers for contextMenuConfigurationForElement and requestMediaCapturePermissionFor are correctly updated.


313-319: LGTM - Action callback signature updated.

The addTab closure parameter correctly removes @Sendable while maintaining @MainActor isolation.


926-940: LGTM - Download action closure updated.

The downloadAction closure correctly removes @Sendable while keeping @MainActor.


1052-1098: LGTM - Authentication challenge handler updated.

The completionHandler for didReceive challenge correctly removes @Sendable.


1234-1255: LGTM - External alert completion updated.

The showExternalAlert completion handler correctly removes @Sendable.

BrowserKit/Sources/Common/Utilities/DispatchQueueInterface.swift (2)

13-13: LGTM - Protocol signature updated.

The ensureMainThread method correctly removes @Sendable from the @MainActor closure parameter.


38-48: LGTM - Default implementation updated.

The extension's ensureMainThread implementation correctly mirrors the protocol change, removing @Sendable while preserving the main-thread dispatch logic.

BrowserKit/Sources/Common/Utilities/DispatchQueueHelper.swift (2)

10-20: LGTM - Non-generic ensureMainThread updated.

The @Sendable removal aligns with the DispatchQueueInterface protocol and the broader PR pattern.


22-32: This is not a breaking API change—this function is newly added, not modified.

The generic ensureMainThread<T> function is new to this file. Git history confirms this entire file was just created. There are no existing callers in the codebase capturing or using the return value of this generic version; all ~200+ existing usages in the codebase call the void-returning overload. Since no code currently depends on this function, the optional return type (T?) cannot break any existing callers.

The function design is semantically correct—returning T? is appropriate when work is dispatched asynchronously and no value can be synchronously returned.

Likely an incorrect or invalid review comment.

✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.

Comment on lines +25 to 29
func throttle(completion: @escaping @MainActor () -> Void) {
lastExecutionTime = Date()
guard threshold <= 0 || lastExecutionTime.timeIntervalSinceNow < -threshold else { return }
DispatchQueue.main.async(execute: completion)
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Critical bug: Throttle logic is broken and will never execute the completion.

The execution time is updated before checking the guard condition. Since lastExecutionTime is set to Date() on line 26, lastExecutionTime.timeIntervalSinceNow will be approximately 0. The condition < -threshold (where threshold defaults to 0.35) will never be true, so the completion closure will never be dispatched.

This appears to be a regression introduced alongside the @Sendable removal. The guard should evaluate before updating lastExecutionTime.

🐛 Proposed fix
 func throttle(completion: `@escaping` `@MainActor` () -> Void) {
-    lastExecutionTime = Date()
     guard threshold <= 0 || lastExecutionTime.timeIntervalSinceNow < -threshold else { return }
+    lastExecutionTime = Date()
     DispatchQueue.main.async(execute: completion)
 }
📝 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.

Suggested change
func throttle(completion: @escaping @MainActor () -> Void) {
lastExecutionTime = Date()
guard threshold <= 0 || lastExecutionTime.timeIntervalSinceNow < -threshold else { return }
DispatchQueue.main.async(execute: completion)
}
func throttle(completion: `@escaping` `@MainActor` () -> Void) {
guard threshold <= 0 || lastExecutionTime.timeIntervalSinceNow < -threshold else { return }
lastExecutionTime = Date()
DispatchQueue.main.async(execute: completion)
}
🤖 Prompt for AI Agents
In `@firefox-ios/Client/Utils/MainThreadThrottler.swift` around lines 25 - 29, In
throttle(completion:) the guard must run before updating lastExecutionTime so
the elapsed-time check uses the previous timestamp; move the assignment to
lastExecutionTime to after the guard (or compute now first and compare against
the existing lastExecutionTime) and only update lastExecutionTime when you
actually allow execution, then DispatchQueue.main.async(execute: completion) as
before; reference symbols: throttle(completion:), lastExecutionTime, threshold,
and DispatchQueue.main.async.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants