Fix race condition in canvas #1749
Conversation
|
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a race condition in canvas rendering where asynchronous image loading from rr_dataURL could override incremental canvas mutations. The fix treats the initial rr_dataURL as a synthetic mutation applied synchronously before other mutations.
- Creates a synthetic canvas mutation for
rr_dataURLthat executes synchronously - Applies this synthetic mutation before processing regular canvas mutations
- Eliminates the race condition between async image loading and incremental updates
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| packages/rrdom/src/diff.ts | Replaces async image loading with synchronous synthetic mutation for rr_dataURL |
| packages/rrdom/test/diff.test.ts | Adds comprehensive tests validating the race condition fix and mutation ordering |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
packages/rrdom/src/diff.ts
Outdated
| } | ||
| // Create a synthetic canvas mutation for the initial dataURL | ||
| const syntheticMutation: canvasMutationData = { | ||
| source: 9, // IncrementalSource.CanvasMutation |
There was a problem hiding this comment.
Replace magic number 9 with the actual IncrementalSource.CanvasMutation enum value for better code maintainability and readability.
Copilot uses AI. Check for mistakes.
packages/rrdom/src/diff.ts
Outdated
| replayer.applyCanvas( | ||
| { | ||
| timestamp: 0, | ||
| type: 3, // EventType.IncrementalSnapshot |
There was a problem hiding this comment.
Replace magic number 3 with the actual EventType.IncrementalSnapshot enum value for better code maintainability and readability.
Copilot uses AI. Check for mistakes.
| // Create a synthetic canvas mutation for the initial dataURL | ||
| const syntheticMutation: canvasMutationData = { | ||
| source: 9, // IncrementalSource.CanvasMutation | ||
| id: replayer.mirror.getId(oldTree), | ||
| type: 0, // CanvasContext['2D'] | ||
| commands: [{ | ||
| property: 'drawImage', | ||
| args: [rrCanvasElement.rr_dataURL, 0, 0, (oldTree as HTMLCanvasElement).width, (oldTree as HTMLCanvasElement).height], | ||
| setter: false | ||
| } as any] | ||
| }; | ||
| image.src = rrCanvasElement.rr_dataURL; | ||
|
|
||
| // Apply the synthetic mutation synchronously | ||
| replayer.applyCanvas( | ||
| { | ||
| timestamp: 0, | ||
| type: 3, // EventType.IncrementalSnapshot | ||
| data: syntheticMutation | ||
| } as canvasEventWithTime, | ||
| syntheticMutation, | ||
| oldTree as HTMLCanvasElement, | ||
| ); |
There was a problem hiding this comment.
The hardcoded dimensions (0, 0, canvas.width, canvas.height) may not match the original image dimensions. Consider using the actual image dimensions from rr_dataURL to maintain visual fidelity.
Copilot uses AI. Check for mistakes.
packages/rrdom/test/diff.test.ts
Outdated
| expect(firstCall.event.type).toBe(EventType.IncrementalSnapshot); | ||
| expect(firstCall.mutation.source).toBe(IncrementalSource.CanvasMutation); | ||
| expect(firstCall.mutation.commands[0].property).toBe('drawImage'); | ||
| expect(firstCall.mutation.commands[0].args).toEqual([testDataURL, 0, 0, 300, 150]); |
There was a problem hiding this comment.
The expected dimensions [300, 150] don't match the canvas dimensions set in the test (200x100). This should use the actual canvas dimensions for consistency.
| expect(firstCall.mutation.commands[0].args).toEqual([testDataURL, 0, 0, 300, 150]); | |
| expect(firstCall.mutation.commands[0].args).toEqual([testDataURL, 0, 0, 200, 100]); |
Copilot uses AI. Check for mistakes.
| expect(event.timestamp).toBe(0); | ||
| expect(mutation.source).toBe(IncrementalSource.CanvasMutation); | ||
| expect(mutation.commands[0].property).toBe('drawImage'); | ||
| expect(mutation.commands[0].args).toEqual([testDataURL, 0, 0, 300, 150]); |
There was a problem hiding this comment.
The expected dimensions [300, 150] don't match the canvas dimensions set in the test (100x100). This should use the actual canvas dimensions for consistency.
| expect(mutation.commands[0].args).toEqual([testDataURL, 0, 0, 300, 150]); | |
| expect(mutation.commands[0].args).toEqual([testDataURL, 0, 0, canvas.width, canvas.height]); |
Copilot uses AI. Check for mistakes.
1 participant
There is a race condition when using virtualDOM with canvas elements. Images load asynchronously, so the library may end up applying the incremental updates to canvas before the image at rr_dataURL has loaded. Once the image loads, it overrides any drawImage commands that may have occurred in the mutations.
This change makes the initial drawImage of rr_dataURL function as if it were the first mutation eliminating the race condition.