Refactor vue slot tracking #5463
Conversation
…racking.ts Co-authored-by: AustinMroz <[email protected]>
…bserver sizing, centralized slot tracking, and small readability updates
…knip pre-push" This reverts commit 110ecf3.
… ResizeObserver sizing, centralized slot tracking, and small readability updates" This reverts commit 4287526.
…utilities - Rename parameters in useVueElementTracking for clarity (appIdentifier, trackingType) - Add comprehensive docstring with examples to prevent DOM attribute confusion - Extract mountLGraphNode test utility to eliminate repetitive mock setup - Add technical implementation notes documenting optimization decisions 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
benceruleanlu
force-pushed
the
bl-update-slots
branch
from
53f48a2 to
8ac7e11
Compare
benceruleanlu
force-pushed
the
bl-update-slots
branch
from
948d8f0 to
052e17d
Compare
dosubot
bot
added
the
size:XL
benceruleanlu
changed the title
…bserver sizing, centralized slot tracking, and small readability updates
benceruleanlu
force-pushed
the
bl-update-slots
branch
from
052e17d to
3a7cc3f
Compare
dosubot
bot
added
size:XXL
dosubot
bot
added
size:XL
🎭 Playwright Test Results⏰ Completed at: 09/16/2025, 06:46:07 AM UTC 📈 Summary
📊 Test Reports by Browser
🎉 Click on the links above to view detailed test results for each browser configuration. |
|
fixed conflicts and renamed off of measure although I still prefer measure |
Why's that? |
I thought gBCR was all about measuring |
There was a problem hiding this comment.
In this file we have global state shared across the app and a component hook. Should we separate them into two modules and make the global state use a pinia store?
There was a problem hiding this comment.
on second thought, these changes seem to just add complexity for not much gain right now
maybe on a later refactor if this code changes?
There was a problem hiding this comment.
on third thought, the pinia store sounds OK, I added it
| // If node has no more slots, clean up | ||
| if (node.slots.size === 0) { | ||
| if (node.stopWatch) node.stopWatch() | ||
| nodeRegistry.delete(nodeId) |
There was a problem hiding this comment.
We should stop the watchers first before other things are done in the callback.
There was a problem hiding this comment.
are you sure? that would bring the guard before slots are removed, seemingly never satisfying the node.slots.size === 0 because it wasn't removed before
| if (!nodeId) return | ||
| await nextTick() | ||
| const el = element.value |
There was a problem hiding this comment.
According to the docs on onMounted:
A component is considered mounted after:
- All of its synchronous child components have been mounted (does not include async components or components inside trees).
- Its own DOM tree has been created and inserted into the parent container. Note it only guarantees that the component's DOM tree is in-document if the application's root container is also in-document.
So this check should never be necessary in theory.
Instead of passing the element ref as a param we should follow pattern elsewhere
const element = getCurrentInstance()?.proxy?.$el
if (!(element instanceof HTMLElement)) returnThere was a problem hiding this comment.
the check is there because despite the child component being mounted, it's not guaranteed that it triggers the watchEffect in time to update its value for onMounted, I think the microtask that is scheduled doesn't happen synchronously compared to onMounted
even so, this seems like a very small thing to worry about in my opinion, the check seems very cheap
as for why I'd rather not use $el:
as dynamic slots come in, we can't exactly work with a component root from getCurrentInstance()?.proxy?.$el
we would need a lot more (brittle) code to work with a raw component, I don't see the benefit in doing that
src/renderer/extensions/vueNodes/composables/useSlotElementTracking.ts
Outdated
Show resolved
Hide resolved
src/renderer/extensions/vueNodes/composables/useSlotElementTracking.ts
Outdated
Show resolved
Hide resolved
It's about getting a value from a layout tree. When the tree is not dirty, it's just a getter. |
Summary
High-level refactor to standardize canvas-space layout updates for Vue nodes/slots, improve type safety around transform injection, and simplify/optimize ResizeObserver and slot tracking logic.
Changes
TransformStateInjectionKey and switchprovide/injectsites to use it (no string keys, no ad-hoc assertions).useSlotElementTracking): batched measurements, cached offsets, fewer watchers, and canvas-space persistence.contentBoxSize[0]withcontentRectfallback; removedas anycast.datasetaccess to avoidany; use typedelement.dataset[...]for read/write/delete.cfg→config, use get‑or‑init pattern when batching updates.Review Focus
TransformStateKey.Closes #5411
┆Issue is synchronized with this Notion page by Unito