Refactor: More state management simplification #5721
Conversation
dosubot
bot
added
the
size:L
|
The function expression → declaration change is half stylistic and half precautionary. The bug shows up when the event listener holds on to a reference that's further down in scope and gets wiped. |
| rafId = null | ||
| }) | ||
| } | ||
| const { resume: handleTransformUpdate } = useRafFn(updateVisibility, { |
There was a problem hiding this comment.
Shall we expose the pause() function? We can pause when the TransformPane emits the "stopped transforming" event (or something similar).
There was a problem hiding this comment.
I actually want to get rid of this entirely and just have the nodes react to changes in the transformPane (throttled)
There was a problem hiding this comment.
Isn't that what was happening already?
There was a problem hiding this comment.
Nope, it's querying the document with every call.
There was a problem hiding this comment.
Looking at it, I initially thought handleTransformUpdate was only called when a transform event is emitted by TransformPane which would then trigger an updateVisibility.
I thought resume from useRafFn essentially started calling the rafFn every frame until pause is called. So now, we are resume when transform is emitted -> starting a call every frame. But before, we just called on every frame that a transform is emitted. Is that wrong?
There was a problem hiding this comment.
Well, the TransformPane is emitting update event every frame anyway, because it's using useCanvasTransformSync which just simply uses its own rafFn that runs every frame, so I'm incorrect afterall.
There was a problem hiding this comment.
I tested by adding console.count call in handleTransformUpdate
There was a problem hiding this comment.
You were right locally, but that does explain why this code didn't tank performance relative to baseline like it would have otherwise 😁
DrJKL
force-pushed
the
drjkl/feat/note-node-3
branch
from
a4fd9b6 to
84f8aa8
Compare
🎭 Playwright Test Results✅ All tests passed! ⏰ Completed at: 09/22/2025, 07:42:46 PM UTC 📈 Summary
📊 Test Reports by Browser
🎉 Click on the links above to view detailed test results for each browser configuration. |
DrJKL
force-pushed
the
drjkl/feat/note-node-3
branch
from
d6eb6d8 to
32af5a7
Compare
DrJKL
force-pushed
the
drjkl/feat/note-node-3
branch
from
32af5a7 to
b992c12
Compare
Summary
Remove more procedural synchronization in favor of using reactive references.
Review Focus
In testing it seems to work the same, but let me know if I missed something.
┆Issue is synchronized with this Notion page by Unito