Refactor FXIOS-14485 FXIOS-14472 [Swift 6 Migration] Turn on Swift 6 at the Project Level, as well as Client and ClientTest targets #29
Conversation
…otations: `@escaping @mainactor @Sendable` (matching WKWebView APIs)
WalkthroughThis pull request adds concurrency safety annotations ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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: 3
🤖 Fix all issues with AI agents
In `@firefox-ios/Client.xcodeproj/project.pbxproj`:
- Around line 27948-27954: The contributor PR workflow is still using Xcode 13.1
which does not support SWIFT_VERSION = 6.0; update the contributor PR CI
workflow to run with Xcode 16.2 or later (e.g., set the workflow's
xcode-version/matrix entry or actions/setup-xcode call to "16.2" or higher) so
the build environment matches the production Xcode 16.2 that supports Swift 6.0;
ensure any matrix entries, macOS runner selection, or setup-xcode steps in the
contributor PR workflow are updated accordingly.
In
`@firefox-ios/Client/Frontend/Browser/BrowserViewController/WebEngineIntegration/BrowserWebUIDelegate.swift`:
- Line 42: The completionHandler parameter declaration in BrowserWebUIDelegate's
method is missing the `@Sendable` annotation; update the parameter signature so
the closure is declared as `@Sendable` `@MainActor` () -> Void (matching the other
completionHandler parameters at lines with similar signatures) to maintain
consistency across BrowserWebUIDelegate and align with the existing annotated
completion handlers.
In
`@firefox-ios/Client/Frontend/Browser/TabScrollController/TabProviderAdapter.swift`:
- Around line 32-35: The computed property on TabProviderAdapter named
onLoadingStateChanged is declared as (`@MainActor` `@Sendable` () -> Void)? but it
delegates to Tab.onWebViewLoadingStateChanged which currently lacks `@Sendable`;
update the declaration of Tab.onWebViewLoadingStateChanged to include `@Sendable`
so the types match (i.e., change Tab.onWebViewLoadingStateChanged's type to
(`@MainActor` `@Sendable` () -> Void)?), then rebuild to ensure the getter/setter in
TabProviderAdapter can safely forward without annotation mismatch.
🧹 Nitpick comments (4)
firefox-ios/Client/Frontend/Browser/DownloadHelper/DownloadToast.swift (2)
109-113: Consider adding@Sendablefor consistency with other callbacks in this PR.The
completionparameter has@MainActor@escaping`` but other similar completion handlers in this PR (e.g., inBrowserViewController+WebViewDelegates.swift, `OnboardingService.swift`) also include `@Sendable`. For consistency and full Swift 6 compliance, consider adding `@Sendable`.♻️ Proposed change
init( downloadProgressManager: DownloadProgressManager, theme: Theme, - completion: `@MainActor` `@escaping` (Bool) -> Void + completion: `@escaping` `@MainActor` `@Sendable` (Bool) -> Void ) {
192-193: Magic number should be extracted to a named constant.The button size
40is hardcoded in two places. According to the AI summary, aUX.buttonSizeconstant was removed. Consider reintroducing a constant for maintainability and to avoid magic numbers.♻️ Proposed change
+private enum UX { + static let buttonSize: CGFloat = 40 +} + class DownloadToast: Toast, DownloadProgressDelegate { // ... existing code ... - closeButton.heightAnchor.constraint(equalToConstant: 40), - closeButton.widthAnchor.constraint(equalToConstant: 40), + closeButton.heightAnchor.constraint(equalToConstant: UX.buttonSize), + closeButton.widthAnchor.constraint(equalToConstant: UX.buttonSize),firefox-ios/Client/Frontend/Onboarding/Protocols/OnboardingService.swift (2)
275-277: Inconsistent@Sendableannotation between caller and callee.
handleOpenInstructionsPopup(line 210) declares its completion as@Sendable@escaping@mainactor, but `presentDefaultBrowserPopup` (line 277) only uses `@escaping `@MainActor. For consistency and to properly propagate the@Sendablerequirement, consider adding@Sendablehere as well.♻️ Proposed fix
private func presentDefaultBrowserPopup( from popupViewModel: OnboardingInstructionsPopupInfoModel<OnboardingInstructionsPopupActions>, - completion: `@escaping` `@MainActor` () -> Void + completion: `@Sendable` `@escaping` `@MainActor` () -> Void ) {
215-217: Missing concurrency annotations onhandleReadPrivacyPolicycompletion.Unlike
handleOpenInstructionsPopup(line 210) which has@Sendable@escaping@MainActor``, this method's completion is only@escaping () -> Void. For consistency with the migration pattern used elsewhere in this file, consider adding the same annotations.♻️ Proposed fix
- private func handleReadPrivacyPolicy(from url: URL, completion: `@escaping` () -> Void) { + private func handleReadPrivacyPolicy(from url: URL, completion: `@Sendable` `@escaping` `@MainActor` () -> Void) { presentPrivacyPolicy(from: url, completion: completion) }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
BrowserKit/Sources/OnboardingKit/Models/OnboardingFlowViewModel.swiftfirefox-ios/Client.xcodeproj/project.pbxprojfirefox-ios/Client/Coordinators/Launch/LaunchCoordinator.swiftfirefox-ios/Client/Frontend/Autofill/AutofillAccessoryViewButtonItem.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/DownloadToast.swiftfirefox-ios/Client/Frontend/Browser/TabScrollController/TabProviderAdapter.swiftfirefox-ios/Client/Frontend/Onboarding/Protocols/OnboardingService.swiftfirefox-ios/firefox-ios-tests/Tests/ClientTests/Mocks/MockTabProviderProtocol.swift
🧰 Additional context used
🧬 Code graph analysis (2)
firefox-ios/Client/Coordinators/Launch/LaunchCoordinator.swift (1)
firefox-ios/Client/Frontend/Onboarding/Protocols/OnboardingService.swift (1)
handleAction(60-140)
firefox-ios/Client/Frontend/Browser/BrowserViewController/Actions/WebContextMenuActionsProvider.swift (3)
firefox-ios/Client/Frontend/Browser/BrowserViewController/Extensions/BrowserViewController+WebViewDelegates.swift (1)
addTab(253-282)firefox-ios/Client/TabManagement/TabManager.swift (1)
addTab(108-118)firefox-ios/Client/TabManagement/TabManagerImplementation.swift (3)
addTab(337-343)addTab(345-356)addTab(380-389)
🔇 Additional comments (32)
firefox-ios/Client/Frontend/Browser/BrowserViewController/Actions/WebContextMenuActionsProvider.swift (2)
30-31: Good concurrency annotation for addTab callback.The
@MainActor@Sendableclosure aligns with the UI-bound usage and Swift 6 concurrency requirements.
43-44: Good concurrency annotation for private-tab callback.The
@MainActor@Sendableclosure matches the UI action execution context and enforces safe isolation.firefox-ios/Client/Frontend/Autofill/AutofillAccessoryViewButtonItem.swift (1)
61-65: Good alignment with MainActor for UI callback.The updated closure annotation matches UI-thread expectations and improves concurrency safety.
firefox-ios/Client/Coordinators/Launch/LaunchCoordinator.swift (1)
271-281: Task wrapping for actor isolation looks correct.The
Taskwrapper is appropriate here for Swift 6 migration since it allows calling into the@MainActor-isolatedhandleActionmethod from within the closure. The completion handler is correctly passed through and will be invoked on the main actor as expected.BrowserKit/Sources/OnboardingKit/Models/OnboardingFlowViewModel.swift (2)
12-16: LGTM - MainActor isolation for callbacks is well-structured.The explicit
@MainActorannotations on callback closure types ensure type-level actor isolation guarantees, which is appropriate for UI-related callbacks in an@MainActor-isolated class. The pattern(@mainactor(String) -> Void)?for optional closures correctly propagates the isolation requirement.Also applies to: 27-27, 30-33
38-46: Initializer parameters correctly mirror property types.The
@MainActor@escaping`` annotations on the initializer parameters align with the stored property declarations, maintaining consistency throughout the view model.firefox-ios/Client/Frontend/Onboarding/Protocols/OnboardingService.swift (1)
65-67: Concurrency annotations onhandleActioncompletion look correct.The
@Sendable@escaping@MainActor`` combination ensures the completion handler is both sendable across actor boundaries and executes on the main actor, which is appropriate for this UI-related callback.firefox-ios/Client/Frontend/Browser/BrowserViewController/Extensions/BrowserViewController+WebViewDelegates.swift (6)
79-79: Correct concurrency annotation for spam callback.The
@escaping@mainactor@Sendable`` annotation properly ensures the callback can be safely captured and called across actor boundaries while executing on the main actor.
94-107: WKUIDelegate JavaScript alert panel handlers correctly annotated.The completion handlers for
runJavaScriptAlertPanelWithMessage,runJavaScriptConfirmPanelWithMessage, andrunJavaScriptTextInputPanelWithPromptall correctly use@escaping@mainactor@Sendable``, matching the Swift 6 requirements for WKUIDelegate methods.
153-170: Context menu and media capture delegate methods properly updated.The completion handlers for
contextMenuConfigurationForElementandrequestMediaCapturePermissionForcorrectly include@MainActor@Sendable`` annotations, aligning with WKUIDelegate Swift 6 requirements.Also applies to: 182-196
313-319:addTabclosure correctly annotated for context menu actions.The
@escaping@mainactor@Sendable`` annotation ensures theaddTabclosure can be safely used within the context menu action provider while maintaining main actor isolation.
704-709: WKNavigationDelegate handlers properly migrated to Swift 6.The
decidePolicyFor navigationResponse,didReceive challenge, andhandleServerTrustmethods all have their completion/decision handlers correctly annotated with@escaping@mainactor@Sendable``, ensuring proper actor isolation for navigation delegate callbacks.Also applies to: 1052-1057, 1329-1333
1234-1235: External alert completion handler correctly annotated.The
showExternalAlertcompletion handler uses@escaping@mainactor@Sendable``, which is consistent with the pattern used throughout this file.firefox-ios/firefox-ios-tests/Tests/ClientTests/Mocks/MockTabProviderProtocol.swift (1)
15-15: LGTM!The
@MainActor@Sendable`` annotations correctly align with the updatedTabProviderProtocoldefinition, ensuring Swift 6 concurrency safety for the mock implementation.firefox-ios/Client/Frontend/Browser/TabScrollController/TabProviderAdapter.swift (1)
14-14: LGTM!The
@MainActor@Sendable`` annotations correctly ensure Swift 6 concurrency safety for the loading state callback, guaranteeing main-thread isolation and safe cross-boundary transfers.firefox-ios/Client.xcodeproj/project.pbxproj (17)
27988-27994: No additional concerns in this hunk.
28254-28260: No additional concerns in this hunk.
28463-28469: No additional concerns in this hunk.
28497-28503: No additional concerns in this hunk.
28691-28697: No additional concerns in this hunk.
29776-29782: No additional concerns in this hunk.
29813-29819: No additional concerns in this hunk.
29826-29832: No additional concerns in this hunk.
30100-30106: No additional concerns in this hunk.
30145-30151: No additional concerns in this hunk.
30261-30267: No additional concerns in this hunk.
30427-30433: No additional concerns in this hunk.
30464-30470: No additional concerns in this hunk.
30477-30483: No additional concerns in this hunk.
30969-30975: No additional concerns in this hunk.
31002-31008: No additional concerns in this hunk.
31015-31021: No additional concerns in this hunk.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| SWIFT_UPCOMING_FEATURE_ISOLATED_DEFAULT_VALUES = YES; | ||
| SWIFT_UPCOMING_FEATURE_NONFROZEN_ENUM_EXHAUSTIVITY = YES; | ||
| SWIFT_UPCOMING_FEATURE_REGION_BASED_ISOLATION = YES; | ||
| SWIFT_VERSION = 5.0; | ||
| SWIFT_VERSION = 6.0; | ||
| TARGETED_DEVICE_FAMILY = "iPhone, iPad"; | ||
| TEST_TARGET_NAME = Client; | ||
| VALIDATE_PRODUCT = YES; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for Xcode/toolchain version specifications in CI and config files
echo "=== Searching for Xcode/toolchain version pins ==="
rg -n --hidden -g '!.git' -g '!node_modules' '(?i)(xcode.?version|swift.?version|developer.?dir|toolchain)' . 2>/dev/null | head -50
echo -e "\n=== Checking for CI workflow files ==="
fd -e yml -e yaml -e json '(workflow|ci|circleci|github|fastlane)' .
echo -e "\n=== Checking common CI directories ==="
ls -la .github/workflows/ 2>/dev/null || echo ".github/workflows not found"
ls -la .circleci/ 2>/dev/null || echo ".circleci not found"
ls -la fastlane/ 2>/dev/null || echo "fastlane not found"Repository: qodo-benchmark/firefox-ios-combined-coderabbit
Length of output: 7872
🏁 Script executed:
#!/bin/bash
# Check GitHub Actions workflows for Xcode version specifications
echo "=== Checking firefox-ios UI tests workflow ==="
grep -n -i "xcode\|swift" .github/workflows/firefox-ios-ui-tests.yml | head -20
echo -e "\n=== Checking firefox-ios build workflow ==="
grep -n -i "xcode\|swift" .github/workflows/firefox-ios-build-contributor-pr.yml
echo -e "\n=== Checking for macOS version specifications ==="
grep -n "macos-version\|runs-on" .github/workflows/firefox-ios-ui-tests.yml | head -10Repository: qodo-benchmark/firefox-ios-combined-coderabbit
Length of output: 1696
🏁 Script executed:
#!/bin/bash
# Check what firefox-ios-build-contributor-pr.yml actually does
cat .github/workflows/firefox-ios-build-contributor-pr.ymlRepository: qodo-benchmark/firefox-ios-combined-coderabbit
Length of output: 1681
🏁 Script executed:
#!/bin/bash
# Quick verification: confirm Xcode 13.1 max Swift version
cat << 'EOF' > /tmp/check_xcode_versions.txt
Xcode Version | Max Swift Version | Release Date
13.1 | Swift 5.5 | Dec 2021
15.4 | Swift 5.9 | June 2023
16.x | Swift 6.0 | Sept 2024
EOF
cat /tmp/check_xcode_versions.txt
# Also check if there are any references to how the build should handle different Xcode versions
echo -e "\n=== Checking for any build version handling ==="
find . -name "*.sh" -o -name "*.yml" | xargs grep -l "SWIFT_VERSION\|swift-version" | head -5Repository: qodo-benchmark/firefox-ios-combined-coderabbit
Length of output: 357
Update contributor PR workflow to support Swift 6.0 toolchain.
The main CI (Xcode 16.2) is properly configured for Swift 6.0, but the contributor PR workflow (firefox-ios-build-contributor-pr.yml) uses Xcode 13.1, which only supports Swift 5.5. This will cause all external contributor PRs to fail with Swift 6.0 compilation errors. Update the workflow to use Xcode 16.2 or later to match the production build environment.
🤖 Prompt for AI Agents
In `@firefox-ios/Client.xcodeproj/project.pbxproj` around lines 27948 - 27954, The
contributor PR workflow is still using Xcode 13.1 which does not support
SWIFT_VERSION = 6.0; update the contributor PR CI workflow to run with Xcode
16.2 or later (e.g., set the workflow's xcode-version/matrix entry or
actions/setup-xcode call to "16.2" or higher) so the build environment matches
the production Xcode 16.2 that supports Swift 6.0; ensure any matrix entries,
macOS runner selection, or setup-xcode steps in the contributor PR workflow are
updated accordingly.
| runJavaScriptAlertPanelWithMessage message: String, | ||
| initiatedByFrame frame: WKFrameInfo, | ||
| completionHandler: @escaping @MainActor @Sendable () -> Void | ||
| completionHandler: @escaping @MainActor () -> Void |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "BrowserWebUIDelegate.swift" -type fRepository: qodo-benchmark/firefox-ios-combined-coderabbit
Length of output: 197
🏁 Script executed:
cat -n ./firefox-ios/Client/Frontend/Browser/BrowserViewController/WebEngineIntegration/BrowserWebUIDelegate.swift | head -120Repository: qodo-benchmark/firefox-ios-combined-coderabbit
Length of output: 4953
🌐 Web query:
Apple WebKit WKUIDelegate runJavaScriptAlertPanelWithMessage Swift 6 Sendable requirement
💡 Result:
In Xcode 16 / Swift 6, a number of WebKit delegate APIs were re-imported with stricter concurrency annotations. For WKUIDelegate’s JavaScript panels, the completionHandler is now main-actor isolated (and main-actor-isolated closures are treated as @Sendable in Swift 6), so your delegate method must match that signature exactly (or use the new async overload). [1], [2]
Fix (sync completionHandler form)
Update your implementation to:
func webView(
_ webView: WKWebView,
runJavaScriptAlertPanelWithMessage message: String,
initiatedByFrame frame: WKFrameInfo,
completionHandler: `@escaping` `@MainActor` () -> Void
) {
let alert = UIAlertController(title: nil, message: message, preferredStyle: .alert)
alert.addAction(UIAlertAction(title: "OK", style: .default) { _ in
Task { `@MainActor` in completionHandler() }
})
present(alert, animated: true)
}If you keep the old @escaping () -> Void signature, it may still compile in some configurations but won’t satisfy the protocol requirement, so the method may not be called. [1], [3]
Alternative fix (use the async overload)
Some SDKs also offer an async version of these delegate methods; implementing that overload avoids the completion-handler annotations altogether. [3]
If you must support older Xcodes too
Use conditional compilation to provide both signatures:
`#if` compiler(>=6)
func webView(_ webView: WKWebView, runJavaScriptAlertPanelWithMessage message: String,
initiatedByFrame frame: WKFrameInfo,
completionHandler: `@escaping` `@MainActor` () -> Void) { /*...*/ }
`#else`
func webView(_ webView: WKWebView, runJavaScriptAlertPanelWithMessage message: String,
initiatedByFrame frame: WKFrameInfo,
completionHandler: `@escaping` () -> Void) { /*...*/ }
`#endif`Sources:
[1] Swift Forums discussion: global-actor-isolated closures (e.g. @MainActor) are implicitly @Sendable in Swift 6 (SE-0434 behavior). (forums.swift.org)
[2] Apple Developer Forums example of WebKit completion handlers gaining @MainActor and @Sendable in Xcode 16. (developer.apple.com)
[3] Stack Overflow report that WKUIDelegate JavaScript panel method stopped being called in Xcode 16 unless using updated/async signature. (stackoverflow.com)
Citations:
- 1: https://forums.swift.org/t/is-there-any-real-difference-between-mainactor-sendable-and-mainactor/72525?utm_source=openai
- 2: https://developer.apple.com/forums/thread/764198?utm_source=openai
- 3: https://stackoverflow.com/questions/79247639/ios-swift-wkuidelegate-methods-not-called-xcode-16
Add @Sendable annotation for consistency with other completion handlers in this file.
The completionHandler on line 42 lacks @Sendable while all other similar completion handler parameters in this file (lines 56, 71, 89, 107) include it. In Swift 6, main-actor-isolated closures are implicitly @Sendable (per SE-0434), but explicitly annotating it maintains consistency and clarity across the delegate implementation.
Suggested change
func webView(
_ webView: WKWebView,
runJavaScriptAlertPanelWithMessage message: String,
initiatedByFrame frame: WKFrameInfo,
- completionHandler: `@escaping` `@MainActor` () -> Void
+ completionHandler: `@escaping` `@MainActor` `@Sendable` () -> Void
) {📝 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.
| completionHandler: @escaping @MainActor () -> Void | |
| func webView( | |
| _ webView: WKWebView, | |
| runJavaScriptAlertPanelWithMessage message: String, | |
| initiatedByFrame frame: WKFrameInfo, | |
| completionHandler: `@escaping` `@MainActor` `@Sendable` () -> Void | |
| ) { |
🤖 Prompt for AI Agents
In
`@firefox-ios/Client/Frontend/Browser/BrowserViewController/WebEngineIntegration/BrowserWebUIDelegate.swift`
at line 42, The completionHandler parameter declaration in
BrowserWebUIDelegate's method is missing the `@Sendable` annotation; update the
parameter signature so the closure is declared as `@Sendable` `@MainActor` () ->
Void (matching the other completionHandler parameters at lines with similar
signatures) to maintain consistency across BrowserWebUIDelegate and align with
the existing annotated completion handlers.
| var onLoadingStateChanged: (@MainActor @Sendable () -> Void)? { | ||
| get { tab.onWebViewLoadingStateChanged } | ||
| set { tab.onWebViewLoadingStateChanged = newValue } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for the onWebViewLoadingStateChanged property declaration in Tab class
rg -n -A2 'onWebViewLoadingStateChanged' --type swift -g '!*Tests*' -g '!*Mock*'Repository: qodo-benchmark/firefox-ios-combined-coderabbit
Length of output: 1450
🏁 Script executed:
#!/bin/bash
# Verify the full context of both properties to confirm the type annotation mismatch
echo "=== Tab.swift property (line 438) ==="
sed -n '438p' firefox-ios/Client/TabManagement/Tab.swift
echo -e "\n=== TabProviderAdapter.swift property (lines 32-35) ==="
sed -n '32,35p' firefox-ios/Client/Frontend/Browser/TabScrollController/TabProviderAdapter.swiftRepository: qodo-benchmark/firefox-ios-combined-coderabbit
Length of output: 430
Type annotation mismatch: missing @Sendable on Tab.onWebViewLoadingStateChanged.
The computed property in TabProviderAdapter declares the type as (@mainactor @sendable () -> Void)?, but the underlying Tab.onWebViewLoadingStateChanged property is only typed as (@mainactor () -> Void)? (missing @Sendable). The getter/setter cannot safely delegate to a property with an incompatible type annotation. Update Tab.onWebViewLoadingStateChanged to include the @Sendable annotation to match the delegating property's contract.
🤖 Prompt for AI Agents
In
`@firefox-ios/Client/Frontend/Browser/TabScrollController/TabProviderAdapter.swift`
around lines 32 - 35, The computed property on TabProviderAdapter named
onLoadingStateChanged is declared as (`@MainActor` `@Sendable` () -> Void)? but it
delegates to Tab.onWebViewLoadingStateChanged which currently lacks `@Sendable`;
update the declaration of Tab.onWebViewLoadingStateChanged to include `@Sendable`
so the types match (i.e., change Tab.onWebViewLoadingStateChanged's type to
(`@MainActor` `@Sendable` () -> Void)?), then rebuild to ensure the getter/setter in
TabProviderAdapter can safely forward without annotation mismatch.
Benchmark PR from agentic-review-benchmarks#3
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.