Implement Improved Query Scaffolding#365
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the Atlas canvas provider and home page to support dynamic card placement, camera panning, and a dedicated query provider for streaming query results. The code reviewer provided valuable feedback on preventing memory leaks by moving side-effect cleanups from the onMount callback to a useEffect hook, avoiding compatibility issues with Promise.withResolvers, handling potential zero-width values from unmeasured canvas bounds, and guarding against empty query submissions.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
nick-nlb
left a comment
nick-nlb
left a comment
There was a problem hiding this comment.
Overall looks great Paulo! Just the one comment.
|
|
||
| // Resolves with the editor once tldraw mounts. Canvas writes chain off this, | ||
| // so `add` works whether it's called before or after mount | ||
| const [isMounted] = useState(() => { |
There was a problem hiding this comment.
I ran over this a couple of times because the name "isMounted" set me on the wrong track. Although the name is isMounted, this is actually storing in state a Promise to the editor itself. If we do keep this pattern, could we change the name here?
However, this might produce an issue (at least theoretically), between the lifecycle of the outer component and the editor. I don't think there's a way that this could happen with the app as it is, but if, for example, we ever had multiple swappable canvases in the future.
Because the (eventually resolved) promise is stored in state, it is permanent within the lifecycle of this outer component. If the actual canvas inside ever unmounted and another one mounted, the promise would still resolve to the first one.
Although it isn't something that would happen with the single canvas app as it is now, it does create a latent bug and makes the canvas lifecycle fragile.
I do like the previous callback-based queue with useRef, as it was very clear and easy to follow (although there might be some lifecycle fixes there). If we stick with this method, can we resolve the above issue?
There was a problem hiding this comment.
Good feedback! I've made a few tweaks. Still kept the promise based pattern but with improved naming across the file / use of ref. I also addressed the lack of cleanup - so unmount + remount it'll recreate the deferred promise state. Let me know if you have any further thoughts here?
2 participants
Overview
This PR aims at improving interface between Atlas and queries. It should make query streaming a case of just hooking into a singular file (
query_provider).Notable changes:
isLoadingto avoid out of sync cards.withEditorhandling for storing / handling events beforetldrawis mounted; moved to promise based flow.QueryProviderpattern + added improved mocked data support including connectingfollowUpbutton to query provider.