FE-528: Fit Petrinaut zoom out to size of net#8611
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
PR SummaryMedium Risk Overview
Adds Reviewed by Cursor Bugbot for commit edc2ecc. Bugbot is set up for automated code reviews on this repo. Configure here. |
github-actions
bot
added
area/deps
|
|
vilkinsons
changed the title
🤖 Augment PR SummarySummary: Improves Petrinaut zoom-out behavior by dynamically constraining the minimum zoom based on the current net bounds and viewport size. Changes:
🤖 Was this summary useful? React with 👍 or 👎 |
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Min zoom ratchets up, permanently preventing zoom-out
- The min-zoom calculation now derives from previous
minZoomstate (with a one-time initialization cap) so zooming in no longer ratchets the floor upward and users can zoom back out.
- The min-zoom calculation now derives from previous
Or push these changes by commenting:
@cursor push dd118ea3c1
Preview (dd118ea3c1)
diff --git a/libs/@hashintel/petrinaut/src/views/SDCPN/sdcpn-view.tsx b/libs/@hashintel/petrinaut/src/views/SDCPN/sdcpn-view.tsx
--- a/libs/@hashintel/petrinaut/src/views/SDCPN/sdcpn-view.tsx
+++ b/libs/@hashintel/petrinaut/src/views/SDCPN/sdcpn-view.tsx
@@ -51,7 +51,7 @@
(
instance: PetrinautReactFlowInstance | null,
canvasContainer: React.RefObject<HTMLDivElement | null>,
- setMinZoom: (minZoom: number) => void,
+ setMinZoom: React.Dispatch<React.SetStateAction<number>>,
) => {
const minMaxCoords = instance?.getNodesBounds(instance.getNodes());
const viewportSize = canvasContainer.current?.getBoundingClientRect();
@@ -62,12 +62,17 @@
viewportSize.width / minMaxCoords.width,
);
const currentZoom = instance?.getViewport().zoom;
- // Don't reduce the zoom level below the users current zoom
- const safeZoom = currentZoom
- ? Math.min(currentZoom, newZoom * ZOOM_PADDING)
- : newZoom * ZOOM_PADDING;
+ setMinZoom((currentMinZoom) => {
+ // Don't increase min zoom beyond the currently configured minimum.
+ // On first initialization (min zoom still 0), cap to the user's current zoom.
+ if (currentMinZoom === 0) {
+ return currentZoom
+ ? Math.min(currentZoom, newZoom * ZOOM_PADDING)
+ : newZoom * ZOOM_PADDING;
+ }
- setMinZoom(safeZoom);
+ return Math.min(currentMinZoom, newZoom * ZOOM_PADDING);
+ });
}
},
100,This Bugbot Autofix run was free. To enable autofix for future PRs, go to the Cursor dashboard.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 922e1fb. Configure here.
| useEffect(() => { | ||
| if (canvasContainer.current) { | ||
| const resizeObserver = new ResizeObserver(() => { | ||
| fitZoomToNodes(reactFlowInstance, canvasContainer, setMinZoom); | ||
| }); | ||
|
|
||
| resizeObserver.observe(canvasContainer.current); | ||
|
|
||
| return () => { | ||
| resizeObserver.disconnect(); | ||
| }; | ||
| } | ||
| }, [canvasContainer, reactFlowInstance]); |
There was a problem hiding this comment.
Maybe we can use a library or at least put this hook into its own file: it would be easier to read this component. (Not that it's particularly easy to read at the moment)
It is equivalent to https://usehooks-ts.com/react-hook/use-resize-observer
There was a problem hiding this comment.
Ok sorry I had not read properly.
It is not equivalent, but IMO it would be better to have a hook returning the dimensions as state of the React component.
e.g.
const { width, height } = useResizeObserver(canvasContainer.current)
| setMinZoom: (minZoom: number) => void, | ||
| ) => { | ||
| const nodesSize = instance?.getNodesBounds(instance.getNodes()); | ||
| const viewportSize = canvasContainer.current?.getBoundingClientRect(); |
There was a problem hiding this comment.
getBoundingClientRect is called again here, and this method triggers a layout/reflow, so is really expensive.
It would be better to have it only called in the ResizeObserver (or not even needed actually, given ResizeObserver should return that already).
|
|
||
| // This sets the min zoom (ie the max you can zoom out to) to be slightly larger than the total size of the current net. | ||
| // We also avoid shrinking the zoom to be lower than the current zoom level to avoid changing the zoom without user input | ||
| const fitZoomToNodes = useMemo( |
There was a problem hiding this comment.
Why. useMemo and not useCallback
(or even remove it completely given we should have React Compiler)
| () => | ||
| debounce( |
There was a problem hiding this comment.
Maybe we can have a useDebounceCallback
(like https://usehooks-ts.com/react-hook/use-debounce-callback)
Or useDebounceValue https://usehooks-ts.com/react-hook/use-debounce-value
| canvasContainer: React.RefObject<HTMLDivElement | null>, | ||
| setMinZoom: (minZoom: number) => void, | ||
| ) => { | ||
| const nodesSize = instance?.getNodesBounds(instance.getNodes()); |
There was a problem hiding this comment.
Same as for getBoundingClientRect, this is I suppose expensive, and would be better derived directly in the body of the component.
Like:
const nodesSize = instance?.getNodesBounds(instance.getNodes());Though, this would not be reactive.
So maybe we can just compute that ourselves without relying on React Flow. (By just creating a function that would give us that just using SDCPNContext)
|
Ideally we want to define everything as a graph of derived states, and not rely on ReactFlow for calculations: const RESIZE_DEBOUNCE = 200;
const canvasDimensions = useDebounceValue(useResizeObserver(canvasContainer.current), RESIZE_DEBOUNCE)
const petrinetDimensions = computeNetDimensions(sdcpn)
const minZoom = computeMinZoom(sdcpn, canvasDimensions) |
There are actually 3 seperate values we are using to define the new minzoom value: the size of the viewport, the size of the net, and the users current zoom value. The biggest performance hits are during the resize and zoom events, where these values change continuously and if not debounced will trigger large amounts of re-renders. That means that if we split the state into its corresponding parts: a) the zoom and viewport size values will not be perfectly accurate if used by other functions as they will be debounced. This could be confusing. And b) because we are debouncing both values seperately, we will trigger double the rerenders, so its going to be less performant |
That said, it may be still be better to do it your way vs adding a second observable if we need to reuse the viewport values for some other function so not totally against it. |
The only debounced value here would be the canvas dimensions (trigger by resizing). Change in user zoom should not trigger re-calculation of const RESIZE_DEBOUNCE = 200;
const canvasDimensions = useDebounceValue(useResizeObserver(canvasContainer.current), RESIZE_DEBOUNCE)
const petrinetDimensions = computeNetDimensions(sdcpn)
const minZoom = computeMinZoom(sdcpn, canvasDimensions)
const actualMinZoom = Math.min(userZoom, minZoom) |
3 participants
🌟 What is the purpose of this PR?
This PR introduces improves the way that zoom is handled in petrinaut. Rather than a fixed value, which might be too small to view large nets, or an infinite zoom out, which make it easy to lose your net if you zoom out too far, I'm introducing a dynamic zoom that maxes out at a level just large enough to view the entire net, but no larger.
There's a bit of extra logic to handle debouncing, screen resizing, and a seamless UX so that users don't notice when we change the maximum zoom underneath them.
Pre-Merge Checklist 🚀
🚢 Has this modified a publishable library?
This PR:
📜 Does this require a change to the docs?
🕸️ Does this require a change to the Turbo Graph?
❓ How to test this?