CLUE-568: Super nodes (grouping) in Dataflow - #2910
Conversation
Foundation for grouping dataflow blocks into labeled "super nodes".
- Add GroupModel { id, label, nodeIds, collapsed } and a groups map on the
DataflowProgramModel (flat refs; nodes stay in the flat map for the rete
engine). createGroup (>=2 ungrouped nodes, auto label "Group N"),
ungroupGroups, setLabel/setCollapsed, getGroupForNode, and auto-dissolve when
a member is removed and membership drops below 2. Additive + backward-
compatible (old snapshots load with groups={}), no version bump.
- Emit grouped nodes as Graphviz subgraph clusters (labeled) in the AI
summarizer so the AI understands groupings; ungrouped output unchanged.
Model + graphviz tests (11) pass. UI (toolbar actions + expanded/collapsed
rendering with proxy sockets) is the next chunk.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- ReteManager: groupSelectedNodes, ungroupSelectedGroups, toggleGroupCollapsed, and canGroupSelection/getSelectedGroupIds helpers driving button enablement. - Register "group"/"ungroup" toolbar buttons (mirroring DeleteNodeButton and the drawing tile's Group/Ungroup enable logic). Buttons appear where a unit's dataflow.tools config lists them. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- ReteManager: groups/nodes accessors + getGroupScreenBounds (screen-space bounds around member nodes from rete's measured node views + area transform). - DataflowGroupsOverlay: draws a labeled, editable, collapsible box around each expanded group's members, positioned over .flow-tool and tracking pan/zoom/drag (observes liveProgramZoom + member node positions). Mounted in dataflow-program. Compiles; existing dataflow tests still pass. NOTE: the overlay's coordinate math and reactivity need visual verification/tuning on the running app. Collapsed rendering (proxy sockets) is the next chunk. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- Default toolbar: add "group"/"ungroup" to the dataflow tile's default tools in app-config.json (so they appear without per-unit config). - Overlay: label bar spans the box width and wraps upward (left-aligned; labels act as code comments); collapsed groups render a labeled chip with an expand control so they stay reopenable; getGroupScreenBounds uses the live MST node position (fixes a box using a node's pre-move position). - Selection: dataflow tile now handles its own selection (tileHandlesOwnSelection + a capture-phase handler keeping it the single selected tile), so Cmd/Ctrl/Shift-clicking nodes no longer toggles the tile out of selection and hides the toolbar. Rete accumulates node selection on Shift as well as Ctrl/Cmd. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- Collapsing a group hides its member nodes (declaratively via CustomDataflowNode when in a collapsed group) and their connection wires (reaction in ReteManager toggling connection-view visibility), so the collapsed chip stands alone. Re-applied after reconciliation so re-created views stay hidden. - Robust group bounds across collapse/expand: getGroupScreenBounds falls back to default node dimensions when a member isn't measurable, and the overlay recomputes one frame after collapse/expand/membership changes so it reads real post-layout sizes (fixes the expanded box coming back too small). Next: step 2 - proxy sockets so collapsed groups' boundary wires re-route to the chip instead of hiding. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… boundary wires When a group is collapsed, render it as a node-styled box that auto-sizes to its wrapping label, with input sockets on the left edge / output sockets on the right (one per boundary connection). Boundary wires route from each external node's socket dot to the corresponding group-node socket; both wire ends are measured against the overlay element so they share an origin and land on target. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…warn Add a focused unit test for the boundary-connection classifier (inputs enter the group, outputs leave, wholly-internal/external ignored). Capture the group id before the auto-dissolve assertion so the test no longer reads id off a removed MST node. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Left-drag on the empty canvas now rubber-band-selects the nodes it intersects (Shift/Ctrl/Cmd-drag adds to the current selection), making it easy to select members to group. Background drag no longer pans; panning moves to the arrow keys (Shift = larger step), scoped to the last-interacted canvas and yielding to node keyboard-nav / text editing. Adds a testable rectsIntersect helper. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #2910 +/- ##
==========================================
- Coverage 85.98% 85.79% -0.20%
==========================================
Files 918 920 +2
Lines 52711 53108 +397
Branches 13939 14054 +115
==========================================
+ Hits 45326 45564 +238
- Misses 7370 7528 +158
- Partials 15 16 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
collaborative-learning
 |
||||||||||||||||||||||||||||
| Project |
collaborative-learning
|
| Branch Review |
CLUE-568-super-nodes-in-dataflow
|
| Run status |
|
| Run duration | 03m 26s |
| Commit |
|
| Committer | Leslie Bondaryk |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
 |
0
|
 |
0
|
 |
0
|
 |
0
|
 |
4
|
| View all changes introduced in this branch ↗︎ | |
There was a problem hiding this comment.
Pull request overview
Adds “super node” grouping to the Dataflow editor (model + UI + selection interactions) and extends the AI Graphviz export to preserve group structure as DOT clusters.
Changes:
- Introduces
groupsonDataflowProgramModelwith create/ungroup, auto-labeling, auto-dissolve, and snapshot round-tripping. - Adds UI/interaction support: group/ungroup toolbar buttons, groups overlay (expanded box + collapsed chip with proxy sockets/wires), marquee selection, and arrow-key panning.
- Updates AI summarizer Graphviz output to emit grouped nodes inside
subgraph cluster_*blocks, plus unit tests.
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/plugins/dataflow/nodes/rete-manager.tsx | Adds modifier-based selection accumulation, marquee selection, arrow-key panning, group APIs, collapsed-member wire hiding, and group bounds helpers. |
| src/plugins/dataflow/nodes/rete-manager-groups.test.ts | Unit tests for getGroupBoundaryConnections() classification. |
| src/plugins/dataflow/nodes/marquee-selection.ts | New marquee selection controller and rect intersection helper. |
| src/plugins/dataflow/nodes/marquee-selection.test.ts | Unit tests for rectsIntersect(). |
| src/plugins/dataflow/nodes/dataflow-node.tsx | Hides member nodes when their group is collapsed (via CSS class). |
| src/plugins/dataflow/nodes/dataflow-node.scss | Adds .collapsed-hidden { display: none; }. |
| src/plugins/dataflow/model/dataflow-program-model.ts | Adds GroupModel, groups map, label incrementing, create/ungroup actions, and auto-dissolve on member removal. |
| src/plugins/dataflow/model/dataflow-program-model.test.ts | Tests for group behaviors and snapshot compatibility. |
| src/plugins/dataflow/dataflow-toolbar-registration.tsx | Registers Group/Ungroup toolbar buttons with icons. |
| src/plugins/dataflow/dataflow-registration.ts | Sets tileHandlesOwnSelection: true to keep toolbar visible during multi-select interactions. |
| src/plugins/dataflow/components/ui/dataflow-groups-overlay.tsx | New overlay rendering expanded group boxes and collapsed group chips + boundary wires. |
| src/plugins/dataflow/components/ui/dataflow-groups-overlay.scss | Styling for overlay, group boxes, collapsed group chip, and dashed boundary wires. |
| src/plugins/dataflow/components/dataflow-tile.tsx | Ensures canvas interaction keeps the tile selected (toolbar stays visible). |
| src/plugins/dataflow/components/dataflow-program.tsx | Mounts the groups overlay in the program UI. |
| src/plugins/dataflow/components/dataflow-program.scss | Adds positioning context and marquee rectangle styling. |
| src/clue/app-config.json | Adds group/ungroup tools to the dataflow toolbar config. |
| shared/ai-summarizer/tile-summarizers/dataflow-to-graphviz.ts | Emits grouped nodes inside Graphviz clusters and escapes cluster labels. |
| shared/ai-summarizer/tile-summarizers/dataflow-to-graphviz.test.ts | Tests Graphviz cluster emission for grouped nodes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Marquee (rubber-band) selection over the dataflow canvas. A left-drag starting on the empty | ||
| // canvas draws a rectangle and selects every node it intersects; panning moves to space-drag or | ||
| // middle-mouse (see the drag guard in rete-manager). Node dragging is untouched (each node stops | ||
| // pointerdown propagation, so those drags never reach this controller). |
| let collapsedKey = ""; | ||
| reteManager?.groups.forEach(group => { | ||
| collapsedKey += `${group.id}:${group.collapsed}:${group.nodeIds.length};`; | ||
| group.nodeIds.forEach(observePosition); | ||
| const bounds = reteManager.getGroupScreenBounds([...group.nodeIds]); | ||
| if (!bounds) return; | ||
|
|
||
| if (!group.collapsed) { | ||
| items.push( | ||
| <ExpandedGroupBox key={group.id} group={group} bounds={bounds} reteManager={reteManager} readOnly={readOnly} /> | ||
| ); | ||
| return; | ||
| } | ||
|
|
||
| const { inputs, outputs } = reteManager.getGroupBoundaryConnections([...group.nodeIds]); | ||
| [...inputs, ...outputs].forEach(b => observePosition(b.externalNodeId)); |
There was a problem hiding this comment.
Fixed (2c496c5). The re-measure signature now includes group.label — its wrapping changes the collapsed node's height and shifts the socket rows — and the exposed-socket identity (each member socket + its boundary connection ids), so the effect reruns after a label edit or a boundary change.
| // Marquee (rubber-band) selection over the dataflow canvas. A left-drag starting on the empty | ||
| // canvas draws a rectangle and selects every node it intersects; panning moves to space-drag or | ||
| // middle-mouse (see the drag guard in rete-manager). Node dragging is untouched (each node stops | ||
| // pointerdown propagation, so those drags never reach this controller). |
There was a problem hiding this comment.
Fixed (9917c60) — updated the header comment: panning is on the arrow keys (background drag-pan is disabled and this controller is passed isPanGesture: () => false).
…lloc) - escapeDotString escapes newlines to \n so multi-line group labels stay valid in DOT label="..." (+ test). - Correct marquee-selection header comment (panning is on the arrow keys, not space-drag/middle-mouse). - Avoid a per-node-render array allocation in the collapsed-member check by iterating the groups map directly and breaking on first match. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…kets A collapsed group now surfaces every member socket that isn't consumed inside the group — including open (unconnected) ones — instead of only sockets that already had a boundary wire. This keeps the collapsed group connectable (e.g. a member with a dangling output shows that open output). getGroupBoundaryConnections is replaced by getGroupInterface, which enumerates each member's rete sockets and classifies them (internal / external / open); external-facing sockets still route a dashed boundary wire, open ones show a dot only. Also: the wire re-measure signature now includes the group label (its wrapping changes socket positions) and the exposed-socket identity so wires re-track after a label edit or a boundary change (Copilot); proxy sockets are sized/styled to match the node sockets (24px, #949494, white ring) and the expand/collapse triangle is enlarged to match. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
tealefristoe
left a comment
tealefristoe
left a comment
There was a problem hiding this comment.
This is a huge, complex feature that has a lot of problems. I expect there will be at least one more round of revisions needed after this, probably more.
The big fixes needed in this round:
- Major cleanup in
dataflow-groups-overlay.tsx. I mostly called things out in comments, but didn't flag everything, so please make sure to apply suggestions to similar situations. - Improve data structures in
dataflow-program-model.tsto improve the efficiency of several functions, which should eliminate O(n^2) functions added in this PR. - Reduce duplicated code, especially in
rete-manager.tsx.
If you want, I can go through and make these changes. It's a lot less work for me to just change the code than to describe the problems and explain the fixes. The advantage to spelling them out is that you will hopefully learn from them and avoid the same problems in the future, but if you're just pasting what I wrote into claude, it would be better if I changed the code directly.
| * A "super node" grouping: a labeled, collapsible container that references member node ids. | ||
| * Members stay in the flat `nodes` map (so the rete engine still sees a flat graph); the group is | ||
| * a parallel organizational layer. Adding this map is backward-compatible — old snapshots without | ||
| * `groups` load with an empty map, so no version bump is needed. |
There was a problem hiding this comment.
| * A "super node" grouping: a labeled, collapsible container that references member node ids. | |
| * Members stay in the flat `nodes` map (so the rete engine still sees a flat graph); the group is | |
| * a parallel organizational layer. Adding this map is backward-compatible — old snapshots without | |
| * `groups` load with an empty map, so no version bump is needed. | |
| * A "super node" grouping: a labeled, collapsible container that references member node ids. | |
| * Members stay in the flat `nodes` map (so the rete engine still sees a flat graph); the group is | |
| * a parallel organizational layer. |
| .model("Group", { | ||
| id: types.identifier, | ||
| label: types.string, | ||
| nodeIds: types.array(types.string), |
There was a problem hiding this comment.
Since there's no need for ordering here, it would be better to use types.map instead of types.array. There are a couple of reasons for this:
- It makes several operations O(1) instead of O(n) (like
removeNodeId). - It's less brittle, especially when it comes to things like merging changes made by different users in group docs.
This change will probably require a bunch of changes throughout this PR, but hopefully they will be straight forward.
| // Create a "super node" group from the given node ids. Requires ≥2 nodes that exist and are | ||
| // not already in a group; returns the new group (or undefined if the request is invalid). | ||
| createGroup(nodeIds: string[], label?: string) { | ||
| const validIds = nodeIds.filter(nodeId => self.nodes.has(nodeId) && !self.getGroupForNode(nodeId)); |
There was a problem hiding this comment.
By changing GroupModel.nodeIds to a types.map, getGroupForNode becomes O(# of groups) (before it was O(# of groups * # of nodes). That makes this function O(nodeIds.length * # of groups)... still O(n^2). Not great.
My recommendation is to keep track of group membership within each node. You should add groupId: types.maybe(types.string) to DataflowNodeModel. In createGroup, you will need to set the groupId of each contained node to the new group's id. When deleting a group, you will have to set each node's groupId to undefined. And when removing a node from a group, you'll also need to set its groupId to undefined. So this change will require a bit more bookkeeping.
The advantage is that getGroupForNode becomes O(1), and createGroup becomes O(nodeIds.length)--no more O(n^2). I think efficiency improvements like that are well worth a little extra bookkeeping.
| return self.groups.get(id); | ||
| }, | ||
| ungroupGroups(ids: string[]) { | ||
| ids.forEach(groupId => self.groups.delete(groupId)); |
There was a problem hiding this comment.
You should define ungroupGroup(id: string) in an actions block above this one, then this function becomes:
| ids.forEach(groupId => self.groups.delete(groupId)); | |
| ids.forEach(groupId => self.ungroupGroup(groupId)); |
In needs to be in a separate action block so you can use self. Within the same block you'd have to use this, which can cause subtle bugs.
ungroupGroup should delete the groupId from self.groups, and also set the groupId of each contained node to undefined (see previous comment).
| const group = self.getGroupForNode(nodeId); | ||
| if (group) { | ||
| group.removeNodeId(nodeId); | ||
| if (group.nodeIds.length < 2) self.groups.delete(group.id); |
There was a problem hiding this comment.
| if (group.nodeIds.length < 2) self.groups.delete(group.id); | |
| if (group.nodeIds.length < 2) self.ungroupGroup(group.id); |
|
|
||
| // Observe pan/zoom + node positions so the measurement effect re-runs on any canvas change. | ||
| const zoom = reteManager?.mstContent.liveProgramZoom; | ||
| const scale = zoom?.scale ?? 1, dx = zoom?.dx ?? 0, dy = zoom?.dy ?? 0; |
There was a problem hiding this comment.
| const scale = zoom?.scale ?? 1, dx = zoom?.dx ?? 0, dy = zoom?.dy ?? 0; | |
| const scale = zoom?.scale ?? 1; | |
| const dx = zoom?.dx ?? 0; | |
| const dy = zoom?.dy ?? 0; |
| const positionKeys: string[] = []; | ||
| const observePosition = (id: string) => { | ||
| const n = reteManager?.nodes.get(id); | ||
| if (n) positionKeys.push(`${isNaN(n.liveX) ? n.x : n.liveX},${isNaN(n.liveY) ? n.y : n.liveY}`); |
There was a problem hiding this comment.
| if (n) positionKeys.push(`${isNaN(n.liveX) ? n.x : n.liveX},${isNaN(n.liveY) ? n.y : n.liveY}`); | |
| if (n) positionKeys.push(`${n.currentX},${n.currentY}`); |
See comment on rete-manager.tsx line 1102 about currentX/Y.
| // Signature of everything that can move a wire endpoint; re-measure after layout when it changes. | ||
| const signature = `${scale}:${dx}:${dy}|${collapsedKey}|${positionKeys.join("|")}`; | ||
|
|
||
| useEffect(() => { |
There was a problem hiding this comment.
| useEffect(() => { | |
| // Update wires from collapsed groups when necessary | |
| useEffect(() => { |
Claude generally adds a bunch of garbage comments, but this is a case where a comment is warranted. useEffects have no names, so unless it's very obvious what they're doing, a comment is usually the right move. Otherwise, an engineer has to read through the whole effect with no context to figure out what it's doing.
| }); | ||
|
|
||
| interface IProps { | ||
| reteManager?: ReteManager; |
There was a problem hiding this comment.
| reteManager?: ReteManager; | |
| reteManager: ReteManager; |
You're already only rendering this component if reteManager is truthy. Requiring the reteManager allows you to simplify the code throughout the component.
There was a problem hiding this comment.
This is an incredibly complex file that will be very difficult to modify in the future and will likely result in bugs. I left a bunch of comments throughout it, but I also wanted to point out a high level issue. At this point it's probably not worth addressing, because it would require a major refactor that would probably result in bugs, but I do think it's worth pointing out the consequences of the big architecture decision that was made.
The big choice here is making all of the groups and connected wires dependent on all of the other groups and wires, rather than separating them by group. That could have been tricky, since the wires connect different components. But rendering them all in one mass means:
- This will run more slowly, since a change to one group (or anything associated with it) results in rerendering all groups, rather than just the one or possibly two affected by the change.
- This could result in weird, hard to trace bugs, where changing something in one part of the dataflow tile results in a change in a completely different part of the tile. Hopefully there aren't any bugs like this right now, but keeping updates and rendering between different groups separate would avoid this whole class of bugs.
What & why
Students learning algorithm design should break complicated Dataflow programs into smaller, identifiable parts they can name and string together. This adds grouping to the Dataflow node editor: select 2+ blocks, group them into a labeled "super node," collapse/expand it, ungroup it, and have the AI understand the grouping. Groups are a comprehension/organization aid — they don't change how the program executes (all member nodes still run).
Ticket: CLUE-568
What's included
groupsmap onDataflowProgramModel(GroupModel: id, label, nodeIds, collapsed). No version bump (legacy snapshots load withgroups={}). Auto-dissolves a group when membership drops below 2.programToGraphvizwraps each group's members in asubgraph cluster_<id> { label=… }so boundary edges cross the cluster naturally.Notes / follow-ups
Testing
getGroupBoundaryConnectionsclassifier;rectsIntersectmarquee helper.tsc --noEmitclean, ESLint clean, dataflow + ai-summarizer Jest suites green (15 suites / 174 tests).🤖 Generated with Claude Code