-
Notifications
You must be signed in to change notification settings - Fork 58
fix(plugin-autocapture-browser): apply rage clicks to window over viewport #1459
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: pnpm-migration
Are you sure you want to change the base?
fix(plugin-autocapture-browser): apply rage clicks to window over viewport #1459
Conversation
Fix rage click detection in plugin-autocapture-browser to use page coordinates and apply window-level bounds for frustrationInteractions.rageClicksSwitch rage click region calculations and emitted coordinates from 🖇️ Linked IssuesAddresses AMP-146045 under the AMP-146043 epic by correcting rage click detection to exclude viewport-based misclassification. 📍Where to StartStart with the rage click logic in packages/plugin-autocapture-browser/src/autocapture/track-rage-click.ts, focusing on Macroscope summarized 87c90f1. |
Apply page-based coordinates to rage click detection in
|
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.
Pull request overview
This PR fixes false rage click detection on mobile web by changing the coordinate system from viewport-based (clientX/clientY) to window-based (pageX/pageY). This prevents scrolling gestures from being incorrectly flagged as rage clicks, since each touch during a scroll will be at different window coordinates even if they appear in the same viewport position. Additionally, the PR refactors imports from absolute src/* paths to relative paths.
- Changed rage click bounding box calculations to use window coordinates (pageX/pageY)
- Updated all rage click tests to use pageX/pageY coordinates
- Refactored imports from
src/*to relative paths for better module resolution
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| test-server/scroll-test.html | Adds manual testing page with 15,000px height to verify scroll gesture vs rage click detection |
| packages/plugin-autocapture-browser/test/autocapture-plugin/track-rage-click.test.ts | Updates all test cases to use pageX/pageY instead of clientX/clientY; adds edge case test for out-of-bounds detection with fixed viewport coordinates |
| packages/plugin-autocapture-browser/src/pageActions/triggers.ts | Refactors import from src/helpers to relative path ../helpers |
| packages/plugin-autocapture-browser/src/pageActions/matchEventToFilter.ts | Refactors import from src/helpers to relative path ../helpers |
| packages/plugin-autocapture-browser/src/autocapture/track-rage-click.ts | Changes coordinate system from viewport (clientX/clientY) to window (pageX/pageY) in bounding box calculations and tracked event data; refactors import path |
| packages/plugin-autocapture-browser/src/autocapture/track-dead-click.ts | Refactors import from src/frustration-plugin to relative path ../frustration-plugin |
| packages/plugin-autocapture-browser/src/autocapture/track-click.ts | Refactors imports to relative paths and adds explicit type annotation for click event subscription |
| packages/plugin-autocapture-browser/src/autocapture/track-change.ts | Refactors imports to relative paths and consolidates import statements |
| packages/plugin-autocapture-browser/src/autocapture/track-action-click.ts | Refactors import from src/autocapture-plugin to relative path ../autocapture-plugin |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
packages/plugin-autocapture-browser/test/autocapture-plugin/track-rage-click.test.ts
Show resolved
Hide resolved
packages/plugin-autocapture-browser/test/autocapture-plugin/track-rage-click.test.ts
Outdated
Show resolved
Hide resolved
…cript into AMP-146045-rage-click-fix-mobile-scroll-false-positive
cc96e65 to
66ffd36
Compare
Summary
It's been observed that when a user scrolls quickly on mobile web, it will often trigger a false "Rage Click". This is because a scroll event involves touching the same part of the screen several times.
What this fix does is changes the bounding box requirements for a rage click
clientX,clientY)pageX,pageY)This prevents scrolling events from being registered, because each touch will be in a different part of the window, because the page is scrolling
Extras
import * from 'src/<modulePath>'. Refactored this to use relative paths instead, it was making it so that I couldn't donpx jest <filepath>Checklist
Note
Rage click detection
pageX/pageYinstead ofclientX/clientYand emit click coordinates using window-based values intrack-rage-click.tspageX/pageYbehavior and timing; add type annotation intrack-click.tsRefactors/housekeeping
src/...imports with relative paths across autocapture/pageActions modulestest-server/scroll-test.htmlfor manual scroll testing (enables frustration interactions/rage clicks)Written by Cursor Bugbot for commit 07b1239. This will update automatically on new commits. Configure here.