Refactor FXIOS-14466 FXIOS-14463 [Swift 6 Migration] Migrate ShareTo and BrowserKit Package to Swift 6 #23
Conversation
…es are enabled in Swift 6.2 by default.
…t does not seem to require @mainactor initialization anymore (and calling `startMockImageServer` on the main thread will causes the addHandler callback to crash for not being called on the main thread).
…currency warnings.
WalkthroughA comprehensive Swift 6 migration updating toolchain versions from 5.x to 6.0/6.2, removing experimental concurrency flags from SwiftPM configuration, making closures explicitly Sendable for stricter concurrency checking, and refactoring async tests to synchronous patterns while adding nonisolated(unsafe) annotations for non-thread-safe components. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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
🤖 Fix all issues with AI agents
In `@BrowserKit/Package.swift`:
- Line 1: Replace the incorrect swift-tools-version header in
BrowserKit/Package.swift (the line "// swift-tools-version: 6.2") with the
project's supported toolchain version to match the rest of the repo (use "//
swift-tools-version: 5.10" to align with Xcode 26.1 / Swift 5.10); update only
that top-of-file directive so the package manifest matches other manifests
(e.g., MozillaRustComponents) and CI will use the correct Swift toolchain.
In `@BrowserKit/Tests/SiteImageViewTests/SVGImageProcessorTests.swift`:
- Around line 26-37: The test invokes
DefaultSiteImageDownloader.downloadImage(with:options:){...} asynchronously but
never waits for the callback; add an XCTestExpectation (e.g., let exp =
expectation(description: "download") ), fulfill it inside both the .success and
.failure branches, and call wait(for: [exp], timeout: ...) after initiating the
download so the assertions under SVGImageProcessor are actually executed; update
the test that uses SVGImageProcessor() to create and wait on the expectation.
🧹 Nitpick comments (2)
BrowserKit/Sources/ExecutableContentBlockingGenerator/MainContentBlockerGenerator.swift (1)
5-11: Prefer scoping the non-thread-safe generator tomain()
nonisolated(unsafe)exposes a global non-thread-safe instance. Sincemain()is the only caller, localizing the generator removes the unsafe global without changing behavior.♻️ Proposed refactor
`@main` public struct MainContentBlockerGenerator { - // FXIOS-14548 ContentBlockerGenerator is not thread safe - nonisolated(unsafe) static let generator = ContentBlockerGenerator.factory() - // Static main needs to be used for executable, providing an instance so we can // call it from a terminal public static func main() { - generator.generateLists() + // FXIOS-14548 ContentBlockerGenerator is not thread safe + let generator = ContentBlockerGenerator.factory() + generator.generateLists() } }BrowserKit/Tests/WebEngineTests/WKUIHandlerTests.swift (1)
38-40: Align test closures with production API isolation requirementsThe
requestMediaCapturePermissionmethod expects@MainActorisolation on the handler. Since the test class is already@MainActor, adding this annotation to the closure types makes the isolation explicit and matches the production API.Suggested alignment
- let decisionHandler: `@Sendable` (WKPermissionDecision) -> Void = { (decision: WKPermissionDecision) in + let decisionHandler: `@Sendable` `@MainActor` (WKPermissionDecision) -> Void = { (decision: WKPermissionDecision) in XCTAssertEqual(decision, .prompt) }- let decisionHandler: `@Sendable` (WKPermissionDecision) -> Void = { (decision: WKPermissionDecision) in + let decisionHandler: `@Sendable` `@MainActor` (WKPermissionDecision) -> Void = { (decision: WKPermissionDecision) in XCTAssertEqual(decision, .deny) expectation.fulfill() }- let decisionHandler: `@Sendable` (WKPermissionDecision) -> Void = { (decision: WKPermissionDecision) in + let decisionHandler: `@Sendable` `@MainActor` (WKPermissionDecision) -> Void = { (decision: WKPermissionDecision) in XCTAssertEqual(decision, .deny) expectation.fulfill() }Also applies to: 50-53, 65-68
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
BrowserKit/Package.swiftBrowserKit/Sources/ExecutableContentBlockingGenerator/MainContentBlockerGenerator.swiftBrowserKit/Tests/SiteImageViewTests/SVGImageProcessorTests.swiftBrowserKit/Tests/WebEngineTests/WKUIHandlerTests.swiftfirefox-ios/Client.xcodeproj/project.pbxprojfirefox-ios/Client/Frontend/Home/Homepage/SectionHeader/LabelButtonHeaderView.swift
🔇 Additional comments (7)
firefox-ios/Client/Frontend/Home/Homepage/SectionHeader/LabelButtonHeaderView.swift (1)
97-113: The wrapper closure is actually required, not optional.moreButtonActionis typed as@Sendable@MainActor``, butActionButtonViewModel.touchUpActionexpects only `@MainActor` (without `@Sendable`). A `@Sendable` closure cannot be directly assigned to a parameter that doesn't require it—the wrapper removes the `@Sendable` requirement. Removing this wrapper would cause a type mismatch compilation error.Likely an incorrect or invalid review comment.
firefox-ios/Client.xcodeproj/project.pbxproj (6)
28493-28496: LGTM for this build configuration.
29783-29786: LGTM for this build configuration.
30091-30094: LGTM for this build configuration.
30408-30411: LGTM for this build configuration.
30938-30941: LGTM for this build configuration.
28008-28011: Clarify intentionality of remaining Swift 5.x overrides.
Setting Swift 6.0 for FirefoxStaging is fine, but verification shows 14 remaining instances of SWIFT_VERSION = 5.0 elsewhere in the project and a baseline of SWIFT_VERSION = 5.10 infirefox-ios/Client/Configuration/Common.xcconfig. Please confirm whether these are intentional differences across targets or if they should also be upgraded to 6.0.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| @@ -1,4 +1,4 @@ | |||
| // swift-tools-version: 5.10 | |||
| // swift-tools-version: 6.2 | |||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Locate Swift/Xcode version pins and CI references
fd -a '\.xcode-version|\.tool-versions|\.swift-version'
rg -n "xcode|swift" -g '*.yml' -g '*.yaml' -g '*.json'Repository: qodo-benchmark/firefox-ios-combined-coderabbit
Length of output: 50402
🏁 Script executed:
# Check if BrowserKit/Package.swift exists and its content
find . -path "*/BrowserKit/Package.swift" -o -path "*/Package.swift" | head -20Repository: qodo-benchmark/firefox-ios-combined-coderabbit
Length of output: 214
🏁 Script executed:
# Look for all Package.swift files to understand the project structure
fd -t f "Package.swift" | head -30Repository: qodo-benchmark/firefox-ios-combined-coderabbit
Length of output: 206
🏁 Script executed:
# Check the actual Xcode version requirements in bitrise.yml more carefully
rg -A 5 -B 5 "swift.tools.version|6\.2" bitrise.ymlRepository: qodo-benchmark/firefox-ios-combined-coderabbit
Length of output: 4087
🏁 Script executed:
# Read BrowserKit/Package.swift to confirm swift-tools-version
cat -n BrowserKit/Package.swift | head -20Repository: qodo-benchmark/firefox-ios-combined-coderabbit
Length of output: 677
🏁 Script executed:
# Search for swift-tools-version in all Package.swift files
rg "swift-tools-version" --type swiftRepository: qodo-benchmark/firefox-ios-combined-coderabbit
Length of output: 567
🏁 Script executed:
# Check if there's any documentation or configuration about Swift version requirements
rg -i "swift.*version|toolchain" --type md --type txt -g "README*" -g "CONTRIBUTING*" -g ".github*" | head -30Repository: qodo-benchmark/firefox-ios-combined-coderabbit
Length of output: 445
🏁 Script executed:
# Check GitHub workflows to see if they specify Swift/Xcode versions
fd -t f ".github" -o "workflows" | xargs ls -la 2>/dev/null | head -20Repository: qodo-benchmark/firefox-ios-combined-coderabbit
Length of output: 1456
🏁 Script executed:
# Look for any Swift version pinning or documentation files
fd -t f "\.(yml|yaml|txt|md)$" -x rg -l "6\.2|swift.*6" {}Repository: qodo-benchmark/firefox-ios-combined-coderabbit
Length of output: 103
🏁 Script executed:
# Check if there's a root README with version information
cat README.md | head -100Repository: qodo-benchmark/firefox-ios-combined-coderabbit
Length of output: 3135
🏁 Script executed:
# Look for version documentation or requirements files
fd -t f "\.(md|txt)$" | xargs grep -l "Xcode\|Swift" | head -10Repository: qodo-benchmark/firefox-ios-combined-coderabbit
Length of output: 3179
🏁 Script executed:
# Check the .github directory for workflow files
find .github -type f -name "*.yml" -o -name "*.yaml" 2>/dev/null | xargs cat 2>/dev/null | grep -i "xcode\|swift" | head -20Repository: qodo-benchmark/firefox-ios-combined-coderabbit
Length of output: 1678
Update BrowserKit/Package.swift swift-tools-version to match project requirements
The swift-tools-version: 6.2 in BrowserKit/Package.swift is incompatible with the project's documented toolchain (Xcode 26.1, Swift 5.10 per README.md). This will cause build failures in CI workflows using Xcode 26.1. Align BrowserKit's swift-tools-version with the other package manifests (Package.swift: 5.7, MozillaRustComponents: 5.10) or update the project-wide toolchain requirements if Swift 6.2 is intended.
🤖 Prompt for AI Agents
In `@BrowserKit/Package.swift` at line 1, Replace the incorrect
swift-tools-version header in BrowserKit/Package.swift (the line "//
swift-tools-version: 6.2") with the project's supported toolchain version to
match the rest of the repo (use "// swift-tools-version: 5.10" to align with
Xcode 26.1 / Swift 5.10); update only that top-of-file directive so the package
manifest matches other manifests (e.g., MozillaRustComponents) and CI will use
the correct Swift toolchain.
| let siteDownloader = DefaultSiteImageDownloader() | ||
| siteDownloader.downloadImage(with: mockedURL, options: [.processor(SVGImageProcessor())]) { result in | ||
| switch result { | ||
| case .success(let result): | ||
| XCTAssertEqual(result.originalData, imageData) | ||
| XCTAssertEqual(result.url, mockedURL) | ||
| XCTAssertEqual(result.image.size, expectedRasterSize) | ||
| exp.fulfill() | ||
| case .failure(let error): | ||
| XCTFail("Should not have an error: \(error) \(error.errorDescription ?? "")") | ||
| exp.fulfill() | ||
| } | ||
| } | ||
|
|
||
| await fulfillment(of: [exp], timeout: 2.0) | ||
| } |
There was a problem hiding this comment.
Await the async download in this test.
This test no longer waits for the async callback, so it can pass without executing assertions (or be flaky). Add an expectation and wait like the other tests.
✅ Proposed fix
- let siteDownloader = DefaultSiteImageDownloader()
- siteDownloader.downloadImage(with: mockedURL, options: [.processor(SVGImageProcessor())]) { result in
+ let exp = expectation(description: "Image download and parse")
+ let siteDownloader = DefaultSiteImageDownloader()
+ siteDownloader.downloadImage(with: mockedURL, options: [.processor(SVGImageProcessor())]) { result in
switch result {
case .success(let result):
XCTAssertEqual(result.originalData, imageData)
XCTAssertEqual(result.url, mockedURL)
XCTAssertEqual(result.image.size, expectedRasterSize)
+ exp.fulfill()
case .failure(let error):
XCTFail("Should not have an error: \(error) \(error.errorDescription ?? "")")
+ exp.fulfill()
}
}
+ wait(for: [exp], timeout: 2.0)📝 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.
| let siteDownloader = DefaultSiteImageDownloader() | |
| siteDownloader.downloadImage(with: mockedURL, options: [.processor(SVGImageProcessor())]) { result in | |
| switch result { | |
| case .success(let result): | |
| XCTAssertEqual(result.originalData, imageData) | |
| XCTAssertEqual(result.url, mockedURL) | |
| XCTAssertEqual(result.image.size, expectedRasterSize) | |
| exp.fulfill() | |
| case .failure(let error): | |
| XCTFail("Should not have an error: \(error) \(error.errorDescription ?? "")") | |
| exp.fulfill() | |
| } | |
| } | |
| await fulfillment(of: [exp], timeout: 2.0) | |
| } | |
| let exp = expectation(description: "Image download and parse") | |
| let siteDownloader = DefaultSiteImageDownloader() | |
| siteDownloader.downloadImage(with: mockedURL, options: [.processor(SVGImageProcessor())]) { result in | |
| switch result { | |
| case .success(let result): | |
| XCTAssertEqual(result.originalData, imageData) | |
| XCTAssertEqual(result.url, mockedURL) | |
| XCTAssertEqual(result.image.size, expectedRasterSize) | |
| exp.fulfill() | |
| case .failure(let error): | |
| XCTFail("Should not have an error: \(error) \(error.errorDescription ?? "")") | |
| exp.fulfill() | |
| } | |
| } | |
| wait(for: [exp], timeout: 2.0) | |
| } |
🤖 Prompt for AI Agents
In `@BrowserKit/Tests/SiteImageViewTests/SVGImageProcessorTests.swift` around
lines 26 - 37, The test invokes
DefaultSiteImageDownloader.downloadImage(with:options:){...} asynchronously but
never waits for the callback; add an XCTestExpectation (e.g., let exp =
expectation(description: "download") ), fulfill it inside both the .success and
.failure branches, and call wait(for: [exp], timeout: ...) after initiating the
download so the assertions under SVGImageProcessor are actually executed; update
the test that uses SVGImageProcessor() to create and wait on the expectation.
Benchmark PR from qodo-benchmark#220
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.