[ui] Support manual node and group positioning in the Asset Graph#33681
[ui] Support manual node and group positioning in the Asset Graph#33681vidiyala99 wants to merge 1 commit intodagster-io:masterfrom
Conversation
Greptile SummaryThis PR adds interactive drag-and-drop node and group repositioning to the Asset Graph, with positions persisted per-view in Confidence Score: 4/5Safe to merge after addressing the localStorage revert bug in useStateWithStorage; remaining findings are style/UX improvements. One P1 bug introduced in useStateWithStorage causes dragged node positions to silently revert when localStorage is full rather than keeping them in memory as intended. This directly breaks the persistence guarantee advertised in the PR description. All other findings are P2 and do not block the primary user path. js_modules/ui-core/src/hooks/useStateWithStorage.tsx — the catch block does not achieve its stated in-memory state intent.
|
| Filename | Overview |
|---|---|
| js_modules/ui-core/src/hooks/useStateWithStorage.tsx | Adds try/catch around localStorage writes, but on failure the version bump causes state to re-read the old localStorage value, silently reverting the update instead of keeping it in memory as the comment claims. |
| js_modules/ui-core/src/asset-graph/useNodeDrag.ts | New drag-and-drop hook wiring mouse events to ephemeral position state; functionally correct but contains substantial code duplication across the three drag paths. |
| js_modules/ui-core/src/asset-graph/usePositionOverrides.ts | New state-management hook for persisting position overrides to localStorage; pruning logic correctly avoids re-render loops but never prunes stale group overrides. |
| js_modules/ui-core/src/asset-graph/applyLayoutOverrides.ts | New utility that applies manual position overrides to a Dagre layout; correctly clones nodes/groups, recomputes edges via the shared computeEdgeEndpoints helper, and recalculates canvas extents. |
| js_modules/ui-core/src/asset-graph/AssetGraphExplorer.tsx | Correctly wires overrides, effectiveLayout, drag handlers, and the Reset toolbar button; raw layout is used for viewport change-detection so overrides don't trigger unwanted re-centering. |
| js_modules/ui-core/src/asset-graph/layout.ts | Extracts edge-endpoint computation into the exported computeEdgeEndpoints helper and adds the required linkNodeIds field to AssetGraphLayout; refactor is clean and backwards-compatible. |
| js_modules/ui-core/src/asset-graph/AssetNode.tsx | Adds isManuallyPositioned prop and ManualPositionDot indicator to all three node variants; change is purely additive and well-contained. |
| js_modules/ui-core/src/graph/asyncGraphLayout.ts | Adds linkNodeIds: new Set() to EMPTY_LAYOUT to satisfy the updated AssetGraphLayout type; minimal and correct. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[User mousedown on node/group] --> B{left button?}
B -- No --> Z[ignore]
B -- Yes --> C[stopPropagation - prevent SVGViewport pan]
C --> D[capture startScreen, startSVG, cursorOffset from effectiveLayout]
D --> E[document mousemove listener]
E --> F{moved > DRAG_THRESHOLD_PX?}
F -- No --> E
F -- Yes --> G[setDraggingNodeId / setDraggedNodePositions throttled @ 60fps]
G --> H[edgesForRender useMemo recomputes edges in real-time]
H --> I[SVG renders ephemeral node/group positions]
I --> E
E --> J[document mouseup]
J --> K[onCommitPosition / onCommitMultiplePositions]
K --> L[usePositionOverrides setOverrides to localStorage]
L --> M[overrides state updates]
M --> N[effectiveLayout useMemo - applyPositionOverrides]
N --> O[SVG renders persisted positions - ManualPositionDot shown]
O --> P{Reset clicked?}
P -- Yes --> Q[clearOverrides - remove from localStorage]
Q --> R[Dagre auto-layout restored]
P -- No --> O
L --> S{localStorage full?}
S -- Yes --> T[write fails - version bumped - state re-reads old localStorage value - position silently reverts]
Reviews (1): Last reviewed commit: "[ui] Add manual node positioning to the ..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| try { | ||
| if (next === undefined) { | ||
| window.localStorage.removeItem(key); | ||
| } else { | ||
| window.localStorage.setItem(key, JSON.stringify(next)); |
There was a problem hiding this comment.
When localStorage.setItem throws (e.g. QuotaExceededError), the catch block swallows the error, then setVersion((v) => v + 1) is called. Because state is derived from getJSONForKey(key), bumping the version causes the UI to re-read from localStorage — which still contains the old value since the write failed. The net effect is that the new position the user just dragged to is silently discarded and the node snaps back to its previous stored position.
The comment "continue with in-memory state" is misleading; this hook has no separate in-memory fallback. To honour that intent, you'd need to track an in-memory override when the write fails:
} catch {
// QuotaExceededError: persist in-memory only. Skip the localStorage round-trip.
// State will be correct for this session but won't survive a reload.
setVersion((v) => v + 1);
return next;
}Or, more robustly, keep a separate inMemoryOverride state that takes precedence over the localStorage read inside state.
| useEffect(() => { | ||
| const nodeIds = new Set(Object.keys(graphData.nodes)); | ||
| const stale = Object.keys(overrides).filter((id) => !nodeIds.has(id) && !isGroupId(id)); | ||
| if (stale.length === 0) { | ||
| return; | ||
| } | ||
| setOverrides((prev) => { | ||
| const pruned = {...prev}; | ||
| stale.forEach((id) => delete pruned[id]); | ||
| return pruned; | ||
| }); |
There was a problem hiding this comment.
The pruning effect only removes stale node overrides (!isGroupId(id)), but collapsed-group overrides (e.g. "mygroup@repo:loc") are kept forever. If a group is renamed or removed from the codebase, its override entry lingers in localStorage indefinitely.
Over time — especially in large monorepos where groups are frequently reorganised — this accumulates dead entries that can bloat the stored JSON and eventually contribute to the QuotaExceededError caught elsewhere.
Consider cross-referencing the group IDs currently in the layout to prune stale group overrides as well, or at least document that group overrides are intentionally long-lived.
| } | ||
|
|
||
| const nodeLayout = layout.nodes[nodeId]; | ||
| if (!nodeLayout) { | ||
| return; | ||
| } | ||
|
|
||
| const startScreen = viewport.getOffsetXY(e); | ||
| if (!startScreen) { | ||
| return; | ||
| } | ||
|
|
||
| const startSVG = viewport.screenToSVGCoords(startScreen); | ||
| const nodeStartPos: IPoint = {x: nodeLayout.bounds.x, y: nodeLayout.bounds.y}; | ||
|
|
||
| // Delta between cursor and node top-left at drag start | ||
| const cursorOffset: IPoint = { | ||
| x: startSVG.x - nodeStartPos.x, | ||
| y: startSVG.y - nodeStartPos.y, | ||
| }; | ||
|
|
||
| // Use a local flag per drag session so the capture-phase click canceller | ||
| // always reads this session's state, even if a new session starts before | ||
| // the setTimeout fires. | ||
| let didDrag = false; | ||
|
|
||
| const onMove = throttle((moveEvent: MouseEvent) => { | ||
| const viewport = viewportRef.current; | ||
| if (!viewport) { | ||
| return; | ||
| } | ||
| const offset = viewport.getOffsetXY(moveEvent); | ||
| if (!offset) { | ||
| return; | ||
| } | ||
| const svgPos = viewport.screenToSVGCoords(offset); | ||
|
|
||
| if (!didDrag) { | ||
| // Use screen-space distance for scale-invariant threshold | ||
| const screenDx = offset.x - startScreen.x; | ||
| const screenDy = offset.y - startScreen.y; | ||
| if (Math.sqrt(screenDx * screenDx + screenDy * screenDy) < DRAG_THRESHOLD_PX) { | ||
| return; | ||
| } | ||
| } | ||
|
|
||
| didDrag = true; | ||
| const pos = {x: svgPos.x - cursorOffset.x, y: svgPos.y - cursorOffset.y}; | ||
| setDraggingNodeId(nodeId); | ||
| setDraggedNodePositions({[nodeId]: pos}); | ||
| }, 1000 / 60); | ||
|
|
||
| // Capture-phase click listener: cancels the click event that fires after a drag. | ||
| // Uses closure-local `didDrag` (not a ref) so it reads this session's state correctly. | ||
| const onCancelClick = (clickEvent: MouseEvent) => { | ||
| if (didDrag) { | ||
| clickEvent.stopImmediatePropagation(); | ||
| clickEvent.preventDefault(); | ||
| } | ||
| }; | ||
|
|
||
| const onUp = (upEvent: MouseEvent) => { | ||
| document.removeEventListener('mousemove', onMove); | ||
| document.removeEventListener('mouseup', onUp); | ||
| onMove.cancel(); | ||
| // Remove click canceller after a tick so it fires before removal | ||
| setTimeout(() => { | ||
| document.removeEventListener('click', onCancelClick, {capture: true}); | ||
| }); | ||
| activeListenersRef.current = activeListenersRef.current.filter( | ||
| (fn) => fn !== removeListeners, | ||
| ); | ||
|
|
||
| if (didDrag) { | ||
| const viewport = viewportRef.current; | ||
| if (viewport) { | ||
| const offset = viewport.getOffsetXY(upEvent); | ||
| if (offset) { | ||
| const svgPos = viewport.screenToSVGCoords(offset); | ||
| onCommitPosition(nodeId, { | ||
| x: svgPos.x - cursorOffset.x, | ||
| y: svgPos.y - cursorOffset.y, | ||
| }); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| setDraggingNodeId(null); | ||
| setDraggedNodePositions({}); | ||
| }; | ||
|
|
||
| // Cleanup function that only removes listeners (no setState — safe for unmount) | ||
| const removeListeners = () => { | ||
| document.removeEventListener('mousemove', onMove); | ||
| document.removeEventListener('mouseup', onUp); | ||
| document.removeEventListener('click', onCancelClick, {capture: true}); | ||
| onMove.cancel(); | ||
| }; | ||
|
|
||
| document.addEventListener('mousemove', onMove); | ||
| document.addEventListener('mouseup', onUp); | ||
| document.addEventListener('click', onCancelClick, {capture: true}); | ||
| activeListenersRef.current.push(removeListeners); | ||
| }, | ||
| [layout, viewportRef, onCommitPosition], | ||
| ); | ||
|
|
||
| const onGroupMouseDown = useCallback( | ||
| (groupId: string, childNodeIds: string[], e: ReactMouseEvent) => { | ||
| // Only handle left mouse button — right-click opens context menu | ||
| if (e.button !== 0) { | ||
| return; | ||
| } | ||
| e.stopPropagation(); | ||
|
|
||
| const viewport = viewportRef.current; |
There was a problem hiding this comment.
The onNodeMouseDown handler, the collapsed-group branch of onGroupMouseDown, and the expanded-group branch each re-implement the same pattern:
startScreen/startSVG/cursorOffsetsetuponMovethrottle withDRAG_THRESHOLD_PXcheckonCancelClickcapture-phase listeneronUpcommit + cleanupremoveListeners+activeListenersRefmanagement
The node and collapsed-group handlers are nearly identical (~80 lines each). Consider extracting a shared createDragSession({ getStartBounds, onMove, onCommit }) helper that handles the document listener lifecycle and threshold logic, leaving each handler to only specify what's unique (single vs. multi-node commit, bounds source).
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| key={group.id} | ||
| {...group.bounds} | ||
| className="group" | ||
| onMouseDown={(e) => { | ||
| const childIds = (groupedAssets[group.id] || []).map((n) => n.id); | ||
| onGroupMouseDown(group.id, childIds, e); | ||
| }} |
There was a problem hiding this comment.
Individual asset nodes correctly receive cursor: dragPos ? 'grabbing' : 'grab' while being dragged or hovered. Expanded group headers and collapsed group foreignObject elements do not, so there is no visual cue that groups are also draggable. Consider adding a cursor: 'grab' style to the group foreignObject elements (and cursor: 'grabbing' when draggingNodeId === group.id) to match the per-node experience.
vidiyala99
force-pushed
the
feature/asset-graph-manual-positioning
branch
from
1202a54 to
0649973
Compare
vidiyala99
force-pushed
the
feature/asset-graph-manual-positioning
branch
from
0649973 to
94f4114
Compare
1 participant
Summary & Motivation
This PR adds support for manual node and group positioning in the Asset Graph. Users can now drag nodes or whole groups to customize their layout, with positions persisted in local storage.
Key features:
Interactive Drag-and-Drop: Single nodes and groups (both collapsed and expanded) can be moved by clicking and dragging.
Real-time Edge Updating: Edges dynamically recalculate as you drag to follow the nodes seamlessly.
Persistence: Manual overrides are saved to localStorage, partitioned by the specific view (view type and job/pipeline).
Manual Overrides Indicator: A subtle blue dot appears on nodes that have been manually moved to indicate they are no longer in their automatic position.
Reset to Auto-layout: A new "Reset" button in the toolbar allows users to clear manual overrides and revert to the default Dagre layout.
Stale Override Pruning: Automatic pruning ensures that manual overrides for nodes no longer in the graph are cleaned up.
Test Plan
Unit tests:
applyLayoutOverrides.test.ts: Verified math for position application, edge recalculation, and canvas extent updates.
computeEdgeEndpoints.test.ts: Verified logic for both vertical and horizontal edge endpoints.
usePositionOverrides.test.tsx: Verified state management, persistence, and pruning logic.
Manual verification:
Verified dragging single nodes and group headers in the Global Asset Lineage and Asset Group views.
Confirmed that edges follow nodes correctly during drag operations.
Verified that manual positions are persisted across page reloads.
Tested the "Reset to auto-layout" functionality.
Changelog
[ui] Add support for manual node and group positioning in the Asset Graph.