fix: lastSnapshotTime is always 0 when using manual snapshots #239
Conversation
c298lee
changed the title
| 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.
| @@ -394,7 +396,6 @@ export class CanvasManager implements CanvasManagerInterface { | |||
| 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 |
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