-
Notifications
You must be signed in to change notification settings - Fork 0
Remove FXIOS-14593 [BVC] Clean up qr code related code from BVC #35
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: coderabbit_full_base_remove_fxios-14593_bvc_clean_up_qr_code_related_code_from_bvc_pr9
Are you sure you want to change the base?
Conversation
WalkthroughThis pull request removes QR code functionality from the Firefox iOS codebase. The changes eliminate the QR code shortcut option, toolbar action, scanning capability, and associated routing, telemetry tracking, and tests across the application. 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
firefox-ios/Client/Frontend/Browser/BrowserViewController/State/BrowserViewControllerState.swift (1)
558-568: Remove the unused.readerModedisplay type case and dead code path.This change unifies reader mode handling by mapping both tap (
showReaderMode) and long-press (addToReadingListLongPressAction) actions to the.readerModeLongPressActiondisplay type. However, this leaves dead code:
- The
case .readerModein theDisplayTypeenum (line 32) is never set by any reducer- The corresponding switch case in
BrowserViewController(line 2872-2875) is unreachableRemove both the unused
case readerModefrom theDisplayTypeenum and the unreachable switch case fromBrowserViewController.swift.firefox-ios/Client/Frontend/Browser/Toolbars/Redux/ToolbarMiddleware.swift (1)
520-529: Fix reader mode telemetry logic and parameter swap bug.The
recordReaderModeTelemetrylogic has two issues:
Incorrect mapping for
.unavailablestate: Currently maps totrue, but should map tofalsesince reader mode cannot be enabled when unavailable. Only.activeshould map totrue.Critical parameter swap in
ToolbarTelemetry.readerModeButtonTapped(): The function receivesisPrivateandisEnabledbut passes them toGleanMetrics.Toolbar.ReaderModeButtonTappedExtrawith swapped parameters:
enabled: isPrivate(should beisEnabled)isPrivate: isEnabled(should beisPrivate)This corrupts all reader mode button tap telemetry data.
🧹 Nitpick comments (1)
firefox-ios/Client/Frontend/Browser/Toolbars/ToolbarTelemetry.swift (1)
8-13: Consider keepinggleanWrapperprivate.The
gleanWrapperproperty was changed fromprivateto internal (default) visibility. Since dependency injection is already possible via the initializer parameter, exposing the property externally appears unnecessary and reduces encapsulation.If this change was made for testing purposes (e.g., to verify the wrapper was set correctly), consider using
private(set)instead to allow reading but prevent external modification, or keep it fully private if external access isn't required.♻️ Suggested change to maintain encapsulation
struct ToolbarTelemetry { - let gleanWrapper: GleanWrapper + private let gleanWrapper: GleanWrapperOr if read access is needed for testing:
struct ToolbarTelemetry { - let gleanWrapper: GleanWrapper + private(set) var gleanWrapper: GleanWrapper
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
firefox-ios/Client/Application/QuickActions.swiftfirefox-ios/Client/Configuration/version.xcconfigfirefox-ios/Client/Coordinators/Browser/BrowserCoordinator.swiftfirefox-ios/Client/Coordinators/Router/DeeplinkInput.swiftfirefox-ios/Client/Coordinators/Router/Route.swiftfirefox-ios/Client/Coordinators/Router/RouteBuilder.swiftfirefox-ios/Client/Frontend/Browser/BrowserViewController/Actions/GeneralBrowserAction.swiftfirefox-ios/Client/Frontend/Browser/BrowserViewController/State/BrowserViewControllerState.swiftfirefox-ios/Client/Frontend/Browser/BrowserViewController/Views/BrowserViewController.swiftfirefox-ios/Client/Frontend/Browser/Toolbars/Redux/ToolbarActionConfiguration.swiftfirefox-ios/Client/Frontend/Browser/Toolbars/Redux/ToolbarMiddleware.swiftfirefox-ios/Client/Frontend/Browser/Toolbars/ToolbarTelemetry.swiftfirefox-ios/Client/Glean/probes/metrics.yamlfirefox-ios/Client/Glean/probes/toolbar.yamlfirefox-ios/Client/Telemetry/TelemetryWrapper.swiftfirefox-ios/firefox-ios-tests/Tests/ClientTests/Coordinators/BrowserCoordinatorTests.swiftfirefox-ios/firefox-ios-tests/Tests/ClientTests/Coordinators/ShortcutRouteTests.swiftfirefox-ios/firefox-ios-tests/Tests/ClientTests/Mocks/MockBrowserViewController.swiftfirefox-ios/firefox-ios-tests/Tests/ClientTests/Toolbar/ToolbarMiddlewareTests.swift
💤 Files with no reviewable changes (15)
- firefox-ios/Client/Telemetry/TelemetryWrapper.swift
- firefox-ios/firefox-ios-tests/Tests/ClientTests/Coordinators/ShortcutRouteTests.swift
- firefox-ios/Client/Coordinators/Router/RouteBuilder.swift
- firefox-ios/firefox-ios-tests/Tests/ClientTests/Toolbar/ToolbarMiddlewareTests.swift
- firefox-ios/Client/Coordinators/Router/Route.swift
- firefox-ios/Client/Glean/probes/toolbar.yaml
- firefox-ios/Client/Frontend/Browser/BrowserViewController/Views/BrowserViewController.swift
- firefox-ios/Client/Coordinators/Browser/BrowserCoordinator.swift
- firefox-ios/firefox-ios-tests/Tests/ClientTests/Mocks/MockBrowserViewController.swift
- firefox-ios/Client/Frontend/Browser/Toolbars/Redux/ToolbarActionConfiguration.swift
- firefox-ios/Client/Glean/probes/metrics.yaml
- firefox-ios/Client/Application/QuickActions.swift
- firefox-ios/firefox-ios-tests/Tests/ClientTests/Coordinators/BrowserCoordinatorTests.swift
- firefox-ios/Client/Coordinators/Router/DeeplinkInput.swift
- firefox-ios/Client/Frontend/Browser/BrowserViewController/Actions/GeneralBrowserAction.swift
🧰 Additional context used
🧬 Code graph analysis (1)
firefox-ios/Client/Frontend/Browser/Toolbars/Redux/ToolbarMiddleware.swift (2)
firefox-ios/Client/TabManagement/Tab.swift (1)
currentURL(1022-1024)BrowserKit/Sources/Common/Extensions/URLExtension.swift (1)
isWebPage(297-300)
🔇 Additional comments (2)
firefox-ios/Client/Configuration/version.xcconfig (1)
1-1: LGTM!Version bump to 147.2 is appropriate for this feature removal PR.
firefox-ios/Client/Frontend/Browser/Toolbars/Redux/ToolbarMiddleware.swift (1)
482-488: LGTM!Good fix to preserve reader mode URLs when canceling edit mode. Since
isWebPage()only matches http/https/data schemes, reader mode URLs would previously be incorrectly set tonil. This change ensures the URL bar correctly retains the reader mode URL.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Benchmark PR from agentic-review-benchmarks#9
Summary by CodeRabbit
Removed Features
Version Update
✏️ Tip: You can customize this high-level summary in your review settings.