Skip to content

Commit a4cb46c

Browse files
kubeclaude
andcommitted
H-5655: Fix selection conflicts and multi-node drag, add integration tests
Remove dual selection management between onNodeClick/onEdgeClick and ReactFlow's internal handleNodeClick. Selection now flows exclusively through ReactFlow → onNodesChange → useApplyNodeChanges → setSelection, fixing meta+click multi-select which was broken by conflicting handlers. Fix multi-node drag position commit by reading change.position directly from ReactFlow events instead of stale draggingStateByNodeId closure. Add integration test file using real EditorProvider + controlled SDCPNContext covering click selection, meta+click toggle, pane clear, single/multi-node drag, and keyboard delete. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 parent 15eab0d commit a4cb46c

6 files changed

Lines changed: 926 additions & 92 deletions

File tree

libs/@hashintel/petrinaut/src/state/editor-provider.tsx

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -78,7 +78,14 @@ export const EditorProvider: React.FC<EditorProviderProps> = ({ children }) => {
7878
setActiveBottomPanelTab: (tab) =>
7979
setState((prev) => ({ ...prev, activeBottomPanelTab: tab })),
8080
setSelection: (selection: SelectionMap) =>
81-
setState((prev) => ({ ...prev, selection })),
81+
setState((prev) => {
82+
const visibilityChanged =
83+
(prev.selection.size === 0) !== (selection.size === 0);
84+
if (visibilityChanged) {
85+
triggerPanelAnimation();
86+
}
87+
return { ...prev, selection };
88+
}),
8289
selectItem: (item: SelectionItem) => {
8390
setState((prev) => {
8491
const wasEmpty = prev.selection.size === 0;

libs/@hashintel/petrinaut/src/views/SDCPN/hooks/use-apply-node-changes.ts

Lines changed: 26 additions & 46 deletions
Original file line numberDiff line numberDiff line change
@@ -19,21 +19,16 @@ import {
1919
export function useApplyNodeChanges() {
2020
const { getItemType, updatePlacePosition, updateTransitionPosition } =
2121
use(SDCPNContext);
22-
const {
23-
draggingStateByNodeId,
24-
updateDraggingStateByNodeId,
25-
setSelection,
26-
selection,
27-
} = use(EditorContext);
22+
const { updateDraggingStateByNodeId, setSelection, selection } =
23+
use(EditorContext);
2824
const { compactNodes } = use(UserSettingsContext);
2925
const dims = compactNodes ? compactNodeDimensions : classicNodeDimensions;
3026

3127
return (changes: (NodeChange | EdgeChange)[]) => {
32-
const positionUpdates: Array<{
28+
const positionCommits: Array<{
3329
id: string;
3430
position: { x: number; y: number };
3531
}> = [];
36-
3732
let selectionChanged = false;
3833

3934
// Check if current selection has any non-node items (types, etc.)
@@ -84,30 +79,14 @@ export function useApplyNodeChanges() {
8479
position: change.position ?? { x: 0, y: 0 },
8580
},
8681
}));
87-
} else {
88-
const lastPosition = draggingStateByNodeId[change.id]?.position;
89-
90-
if (!lastPosition) {
91-
// we've had a dragging: false with no preceding dragging: true, so the node has not been dragged anywhere.
92-
continue;
93-
}
94-
95-
/**
96-
* When dragging stops, we receive a change event with 'dragging: false' but no position.
97-
* We use the last position we received to report the change to the consumer.
98-
*/
99-
positionUpdates.push({
82+
} else if (change.position) {
83+
// Drag ended for this node. Use `change.position` directly rather than
84+
// reading from `draggingStateByNodeId`, because the closure may be stale:
85+
// ReactFlow syncs `onNodesChange` to its store via useEffect, so between
86+
// rapid mouse events the callback may reference an older render's state.
87+
positionCommits.push({
10088
id: change.id,
101-
position: lastPosition,
102-
});
103-
104-
// Clear the dragging state for this node now that the drag is complete
105-
// and the position has been collected for commit to the SDCPN store.
106-
// Keeping stale positions here would cause them to be re-applied
107-
// if ReactFlow emits a spurious position change after an undo.
108-
updateDraggingStateByNodeId((existing) => {
109-
const { [change.id]: _, ...rest } = existing;
110-
return rest;
89+
position: change.position,
11190
});
11291
}
11392
}
@@ -118,22 +97,23 @@ export function useApplyNodeChanges() {
11897
setSelection(newSelection);
11998
}
12099

121-
// Apply position updates to SDCPN store
122-
for (const { id, position } of positionUpdates) {
123-
// Get item type to determine whether it's a place or transition
124-
const itemType = getItemType(id);
125-
126-
if (itemType === "place") {
127-
updatePlacePosition(id, {
128-
x: position.x + dims.place.width / 2,
129-
y: position.y + dims.place.height / 2,
130-
});
131-
} else if (itemType === "transition") {
132-
updateTransitionPosition(id, {
133-
x: position.x + dims.transition.width / 2,
134-
y: position.y + dims.transition.height / 2,
135-
});
100+
// Commit final positions from drag-end changes to the SDCPN store.
101+
if (positionCommits.length > 0) {
102+
for (const { id, position } of positionCommits) {
103+
const itemType = getItemType(id);
104+
if (itemType === "place") {
105+
updatePlacePosition(id, {
106+
x: position.x + dims.place.width / 2,
107+
y: position.y + dims.place.height / 2,
108+
});
109+
} else if (itemType === "transition") {
110+
updateTransitionPosition(id, {
111+
x: position.x + dims.transition.width / 2,
112+
y: position.y + dims.transition.height / 2,
113+
});
114+
}
136115
}
116+
updateDraggingStateByNodeId(() => ({}));
137117
}
138118
};
139119
}

0 commit comments

Comments
 (0)