Repository navigation
Conversation
pickSnapshotViewport used the largest pageH of the winning viewport as the snapshot size, so one click recorded on a taller page stretched the snapshot for every dot. Use the pageH shared by most clicks instead, raised only as far as the lowest click needs.
|
@drakeo338 is attempting to deploy a commit to the Umami Software Team on Vercel. A member of the Team first needs to authorize it. |
 [Medium impact] The change is limited to snapshot sizing in the heatmap click workflow. Fix the frame-height interaction before merging so clicks below the first screen remain visible. FindingsSummaryChanges
Reviews (1) · Last reviewed commit: "Fix heatmap snapshot height being set by..." · Reviewed by Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| height: bestViewport.height, | ||
| pageW: bestViewport.maxPageW, | ||
| pageH: bestViewport.maxPageH, | ||
| pageH: Math.max(commonPageH, Math.ceil(bestViewport.maxPageY)), |
There was a problem hiding this comment.
With viewportH=900, three clicks at pageH=900, and one click at pageY=1000 with pageH=2239, this returns pageH=1000. But getSnapshotFrameHeight turns heights up to 1125 back into 900, so the canvas clips the click at 1000. Previously, the frame was 2239 tall and showed that click. Update the frame-height rule alongside this change so extending to the deepest click actually keeps it visible.
| it('ignores a single taller outlier pageH', () => { | ||
| const result = pickSnapshotViewport([point(865, 900, 3), point(450, 900), point(449, 2239)]); | ||
|
|
||
| expect(result?.pageH).toBe(900); |
There was a problem hiding this comment.
These tests do not check that count decides the most common height. In the outlier test, 900 wins both by clicks and by number of point rows, so ignoring count would still pass. Add a case where one row has more clicks than several rows at another height. Also cover equal totals to protect the new larger-height tie rule.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
This branch has not been deployed
1 participant
Fixes #4647.
The heatmap snapshot height came from the largest
pageHamong clicks in the chosen viewport, so one click recorded while the page was temporarily taller (an open modal, say) stretched the frame for every dot and shifted bottom-anchored elements away from the recorded clicks.pickSnapshotViewportnow uses thepageHshared by most clicks, raised only as far as the lowest click needs. Added tests for the outlier case; they fail on the old code and pass now.Need help on this PR? Tag
@codesmith-botwith what you need.