Store canvas position of scene comments. #4791
Conversation
Job #10064: Bundle Size — 62.38MiB (~+0.01%).
Warning Bundle contains 66 duplicate packages – View duplicate packages Bundle metrics
 
Bundle size by type
View job #10064 report View feature/autoconvert-scene-commen... branch activity View project dashboard |
|
Performance test results: |
editor/liveblocks.config.ts
Outdated
| y: number | ||
| sceneId?: string | ||
| sceneX?: number |
There was a problem hiding this comment.
I don't like this! what's the point of these being optional here and then have a separate SceneThreadMetadata where they are declared as non optional?
There was a problem hiding this comment.
The reason is that canvas comments don't have these properties (sceneId, sceneX, sceneY), and we are only allowed to have string, number and boolean properties here, so no way to create a structured type.
There was a problem hiding this comment.
why not CanvasThreadMetadata without these fields and SceneThreadMetadata with these fields, and then ThreadMetadata = CanvasThreadMetadata | SceneThreadMetadata
but better yet, why not make it a discriminated union type with a type: 'canvas' | 'scene'? I see on master there was some notion to do that
There was a problem hiding this comment.
I can not use discriminated union types because I can only add string/boolean/number fields.
Undiscrimated union types are not that nice to work with, but you can be right maybe this solution is better: 68b2418
There was a problem hiding this comment.
It turned out the problem was not discriminated union types, but that I used interfaces and not types.... Here is the proper solution: dcc67bd
| const scenes = useScenes() | ||
| const editThreadMetadata = useEditThreadMetadata() | ||
|
|
||
| threads.forEach(async (t): Promise<void> => { |
There was a problem hiding this comment.
I was about to ask if subsequent calls to this can trample over each other, but I don't think they will because the promises would all be queued up in order.
Problem:
When a scene is deleted, the scene comments are orphaned, and we are not able to position them on the canvas.
Fix:
Change the liveblocks thread representation to store the local position of the thread in the scene (
sceneXandsceneY) and the canvas position (xandy) both.Note, that we can only add strings/numbers/booleans to the thread metadata type, so there is no way to create a more structured typing.
The advantage of this representation is that
xandyalways represents the canvas position of the thread, which was a more ambiguous earlier (canvas position for canvas comments and local position for scene comments).So we do the best effort to position the comment thread in its scene, but if the scene is not available for some reason (e.g. it has been deleted), we still put it on the same canvas position. We do not delete the scene information, so when the user presses undo to put back the scene, the comment thread automatically becomes a scene thread again.
When the scene is moved, we update the canvas location of its scene comments using a React component called CommentMaintainer (which doesn't render anything, its role is just this maintenance step).