-
Notifications
You must be signed in to change notification settings - Fork 0
Remove FXIOS-14593 [BVC] Clean up qr code related code from BVC #19
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: cursor_combined_20260121_qodo_grep_cursor_copilot_1_base_remove_fxios-14593_bvc_clean_up_qr_code_related_code_from_bvc_pr206
Are you sure you want to change the base?
Conversation
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.
Cursor Bugbot has reviewed your changes and found 3 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
| windowUUID: state.windowUUID, | ||
| browserViewType: state.browserViewType, | ||
| displayView: .readerMode, | ||
| displayView: .readerModeLongPressAction, |
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.
Reader mode tap triggers wrong action after refactor
High Severity
The handleShowReaderModeAction function now sets displayView: .readerModeLongPressAction instead of displayView: .readerMode. This causes tapping the reader mode button to call toggleReaderModeLongPressAction() (which adds the page to reading list) instead of toggleReaderMode() (which enters/exits reader mode). Users will no longer be able to toggle reader mode via the button.
| case .available: true // will be enabled after action gets executed | ||
| default: false | ||
| case .available: false // will be enabled after action gets executed | ||
| default: true |
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.
Reader mode telemetry reports inverted enabled state
Medium Severity
The recordReaderModeTelemetry function inverts the isReaderModeEnabled value. When readerModeState is .available (meaning reader mode will be enabled after tap), the code now reports false. The comment still says "will be enabled after action gets executed" but the code does the opposite, causing telemetry to report incorrect reader mode usage data.
| var url = tabManager(for: windowUUID).selectedTab?.url | ||
| if let currentURL = url { | ||
| url = (currentURL.isWebPage() && !currentURL.isReaderModeURL) ? url : nil | ||
| url = (currentURL.isWebPage() || currentURL.isReaderModeURL) ? url : nil |
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.
URL handling logic inverted for reader mode pages
Medium Severity
The cancelEditMode URL filtering logic changed from (currentURL.isWebPage() && !currentURL.isReaderModeURL) to (currentURL.isWebPage() || currentURL.isReaderModeURL). The original code explicitly excluded reader mode URLs (setting them to nil), while the new code explicitly includes them. This behavioral change affects how the address bar displays when canceling edit mode on reader mode pages, and appears unrelated to the QR code cleanup purpose of this PR.
Benchmark PR from qodo-benchmark#206
Note
Removes the legacy QR code scanning feature and its integrations across the app.
Route.AppAction.showQRCode), andRouteBuilderhandlingGeneralBrowserActionType/BrowserViewControllerState.DisplayTypecases, plus related UI flow fromBrowserViewControllerToolbarTelemetry.qrCodeButtonTapped, Glean probestoolbar.qr_scan_button_tappedandqr_code.scanned) and correspondingTelemetryWrappermappingshandleShowReaderModeActionnow usesdisplayView: .readerModeLongPressAction; updates cancel-edit URL logic to include reader mode URLs and inverts reader mode telemetry flag logicAPP_VERSIONto147.2Written by Cursor Bugbot for commit 75b1c94. Configure here.