[ENG-1287] On canvas load, update key image#938
Conversation
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| const getNodeCanvasSettings = (nodeType: string): Record<string, string> => { | ||
| const allNodes = getDiscourseNodes(); | ||
| const canvasSettings = Object.fromEntries( | ||
| allNodes.map((n) => [n.type, { ...n.canvasSettings }]), | ||
| ); | ||
| return canvasSettings[nodeType] || {}; | ||
| }; |
There was a problem hiding this comment.
📝 Info: Repeated getDiscourseNodes() calls inside getNodeCanvasSettings
getNodeCanvasSettings (calcCanvasNodeSizeAndImg.ts:79-85) calls getDiscourseNodes() on every invocation, which reads from the Roam database. In syncCanvasKeyImagesOnLoad, getCanvasNodeKeyImageUrl is called once per surviving shape in the first pass (via Promise.all), and once again per changed shape in the second pass (via calcCanvasNodeSizeAndImg). Each call triggers a full getDiscourseNodes(). For canvases with many shapes, this could be noticeably slow. Consider caching the canvas settings for the duration of the sync operation, or passing them as a parameter.
Was this helpful? React with 👍 or 👎 to provide feedback.
| // Second pass: load images only for shapes whose URL changed, to compute new dimensions. | ||
| await Promise.all( | ||
| changedShapes.map(async ({ shape, title }) => { | ||
| const { w, h, imageUrl } = await calcCanvasNodeSizeAndImg({ | ||
| nodeText: title, | ||
| uid: shape.props.uid, | ||
| nodeType: shape.type, | ||
| extensionAPI, | ||
| }); | ||
| imageUpdates.push({ | ||
| id: shape.id, | ||
| type: shape.type, | ||
| props: { imageUrl, w, h }, | ||
| }); | ||
| }), | ||
| ); |
There was a problem hiding this comment.
📝 Info: Redundant getCanvasNodeKeyImageUrl call in two-pass image sync
The two-pass design in syncCanvasKeyImagesOnLoad calls getCanvasNodeKeyImageUrl in the first pass (line 166) to detect changes, then calls calcCanvasNodeSizeAndImg in the second pass (line 183) which internally calls getCanvasNodeKeyImageUrl again (calcCanvasNodeSizeAndImg.ts:143). For shapes using the query-builder key-image option, this means runQuery is executed twice per changed shape. This is inefficient but not a correctness bug — the second call will produce the same result in all realistic scenarios, and the final state is always consistent. A possible optimization would be to pass the already-fetched imageUrl from the first pass into the second pass and only compute dimensions, avoiding the redundant URL resolution.
Was this helpful? React with 👍 or 👎 to provide feedback.
mdroidian
left a comment
mdroidian
left a comment
There was a problem hiding this comment.
Summary
Functionally this direction makes sense, but the main risk is how much Roam and the browser do on canvas open. With 20+ discourse nodes that use key images, the current flow can trigger a large number of independent resolutions (tree walks, query-builder runs, pulls, and image loads), not a small fixed cost. The highest-impact follow-ups here should be performance-focused before we lean on this in real graphs.
Why this matters
syncCanvasKeyImagesOnLoadrunsgetCanvasNodeKeyImageUrlfor every surviving node in parallel (Promise.allover all shapes). Each call can be quite heavy.- For nodes whose stored URL differs from the resolved URL,
calcCanvasNodeSizeAndImgcallsgetCanvasNodeKeyImageUrlagain, so changed nodes pay for URL resolution twice before dimensions are computed. That’s an easy 2× on the hottest path when many URLs update at once (e.g. after settings or data changes). - The second pass also
loadImages in parallel for every changed shape. That can be fine for a few images but rough on large canvases (many concurrent network decodes, main-thread work).
So the concern isn’t only “many shapes” — it’s many shapes × (possibly expensive per-node Roam work + duplicate resolution + parallel image loads).
Testing / acceptance (please block merge on this)
After the performance work lands, verify on a large canvas, not a 3-node fixture:
- Prefer an existing heavy canvas in graph
plugin-testing-akamatsulab2: open a page known to host a big tldraw canvas. Aim for ≥20 discourse node shapes with distinct key images if possible. - If no suitable page exists, create a test page in that graph with 25+ nodes, each with a different key image (exercises URL resolution + image load paths).
- Report concrete signals in the PR : time-to-interactive or wall time until sync finishes, any UI jank, and whether Roam feels responsive. A before/after comparison on the same canvas is ideal.
| }; | ||
|
|
||
| /** | ||
| * On canvas load: sync discourse node shape titles with Roam and remove shapes whose nodes no longer exist. |
There was a problem hiding this comment.
Need to update the comment to include images
| const urlResults = await Promise.all( | ||
| survivingShapes.map(async (shape) => { | ||
| const title = uidToTitle.get(shape.props.uid) ?? shape.props.title ?? ""; | ||
| const imageUrl = await getCanvasNodeKeyImageUrl({ |
There was a problem hiding this comment.
For each shape this is running getNodeCanvasSettings > getDiscourseNodes() > getDiscourseRelations()
Let's run getDiscourseNodes() once and get the canvas setting directly.
| uid: shape.props.uid, | ||
| nodeType: shape.type, | ||
| extensionAPI, | ||
| }); |
There was a problem hiding this comment.
This is also running an individual query for each shape. This could be hundreds of queries.
We'll have to write a custom query for any nodes that have the key-image option of First image on page. So we can run one single query rather than a query for each node. Those with Query builder reference will probably still be a Promise.all, unfortunately.
| // Second pass: load images only for shapes whose URL changed, to compute new dimensions. | ||
| await Promise.all( | ||
| changedShapes.map(async ({ shape, title }) => { | ||
| const { w, h, imageUrl } = await calcCanvasNodeSizeAndImg({ |
There was a problem hiding this comment.
Ideally we only run this when
imageUrlwas "" and is now not emptyimageUrlwas not empty and is now ""
This way we only need to load the images that we need to measure.
We should leave the w/h if just the image changes. If the image dimensions change, display should have mostly already been taken care of with ENG-1334: port over the key image resizing behavior from Obsidian
| await Promise.all( | ||
| changedShapes.map(async ({ shape, title }) => { | ||
| const { w, h, imageUrl } = await calcCanvasNodeSizeAndImg({ | ||
| nodeText: title, |
There was a problem hiding this comment.
calcCanvasNodeSizeAndImg is running the query for imageUrl a second time here. So for 100 shapes it would be 200 queries (if each shape changed).
As stated in the comment above, let's only run calcCanvasNodeSizeAndImage when we need to set a new w, h.
And let's make sure we lass the imageUrl so we don't need to requery it.
2 participants
https://www.loom.com/share/1dc9445cdf5a4712af0864c579b99d10