fix: lastSnapshotTime is always 0 when using manual snapshots#239
fix: lastSnapshotTime is always 0 when using manual snapshots#239
Conversation
c298lee
changed the title
billyvg
left a comment
billyvg
left a comment
There was a problem hiding this comment.
Are there tests for manual snapshot?
Otherwise, code changes look good. Could improve the tests a bit
| await waitForRAF(ctx.page); | ||
|
|
||
| // should yield a frame for only first change because it's only 5 fps | ||
| assertSnapshot(stripBase64(ctx.events)); |
There was a problem hiding this comment.
Instead of a snapshot I would use an assertion that we only have 1 canvas mutation frame so that the check is very explicit (snapshot changes are easily ignored).
I would also add additional timeouts so that we know it continues to work.
| canvasElement?: HTMLCanvasElement, | ||
| ) { | ||
| const timeBetweenSnapshots = 1000 / fps; | ||
| let lastSnapshotTime = 0; |
There was a problem hiding this comment.
subtle, i guess when it's called more than once (user is interacting with the page? or no... when our mobile replays are loading we can play/pause really quickly!) it gets cleared out and the new set of closures start at 0 again.
There was a problem hiding this comment.
yes, we recommend putting the snapshot function in the paint loop, so everytime it was repainted, it would be reset to 0
|
Merging this without manual snapshot tests since we want to fix this bug sooner, will add tests in a followup PR |
3 participants
When taking manual snapshots, the lastSnapshot time was resetting to 0, causing a snapshot to always be taken. This change moves lastSnapshotTime into the class and ensures that the fps is adhered to.
Fixes getsentry/sentry-javascript#15334