-
Notifications
You must be signed in to change notification settings - Fork 233
fix(data-modeling): throttle diagram updates to avoid frequent render issues COMPASS-9738 #7395
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
… with interactions involving frequent rendering COMPASS-9738
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! It would be lovely if this can be turned into a component-agnostic hook, so we don't have to list all the props in 4 different places 🙂
[handleNodesConnect] | ||
); | ||
|
||
// Throttling mechanism for diagram content updates |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suuuper nit, but I would strongly suggest to move it into its own hook outside of render method just for the ease of navigating in this code and around this logic (it can be in the same file, outside the render)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved into a generic hook in compass-components. Would it be better to keep it in data-modeling as there aren't any other consumers of the hook yet?
Errr I might need to update the useMemo usage that's outside of the hook. Probably nicer to have that encapsulated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR introduces throttling to prevent frequent re-renders of diagram components that were causing ReactFlow state update bugs. The implementation throttles diagram prop updates with a 250ms delay to stabilize rendering performance.
- Adds a new
useThrottledProps
hook that throttles prop updates with configurable refresh rate - Refactors diagram component to use throttled props instead of direct prop passing
- Consolidates diagram props into a memoized object for cleaner prop management
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
File | Description |
---|---|
packages/compass-components/src/hooks/use-throttled-props.tsx | New hook implementation for throttling prop updates with timeout management |
packages/compass-components/src/index.ts | Export the new useThrottledProps hook |
packages/compass-data-modeling/src/components/diagram-editor.tsx | Refactors diagram component to use throttled props and consolidates prop handling |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
COMPASS-9738
This pr introduces a throttle on the updating of props passed to the diagram to prevent frequent re-renders (250ms). I found going less than 250ms would cause the issue to come back when I opened fairly slow to render/update diagrams (many large nodes). It's worth mentioning here that running a dev branch here will be slower than GA. So by having something that works in dev in terms of slowness should be good.
When interacting with the diagram quickly it currently can encounter bugs with ReactFlow state updates. Here's a reproduction that currently can happen:
The node disappears currently on main. With these changes and the diagramming version bump in the other open pr it does not disappear.
This is a bit of a catch-all approach. Ideally we should be restricting the events which would cause a re-render to only happen in one iteration.
Looks like COMPASS-9737 can still occur if adding enough nodes quickly (will be very hard to repro not on local dev, maybe some slow machines).