Skip to content

feat(Groups): CollapsibleGroup#259

Merged
draedful merged 21 commits intomainfrom
collapse_group
Apr 15, 2026
Merged

feat(Groups): CollapsibleGroup#259
draedful merged 21 commits intomainfrom
collapse_group

Conversation

@draedful
Copy link
Copy Markdown
Collaborator

@draedful draedful commented Mar 2, 2026

No description provided.

@draedful draedful requested a review from Antamansid as a code owner March 2, 2026 17:06
@gravity-ui-bot
Copy link
Copy Markdown
Contributor

Preview is ready.

@draedful draedful force-pushed the collapse_group branch 3 times, most recently from 7063130 to 0244f06 Compare March 13, 2026 14:34
@draedful draedful changed the title WIP: CollapsibleGroup feat(Groups): CollapsibleGroup Apr 3, 2026
};
```

- **`key`** (required) — unique identifier for the area within the component. If an area with the same key already exists, it is replaced instead of duplicated. Also used to persist the area's local state across render cycles.
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Less details please

blockPort.isDelegated; // false
```

#### How CollapsibleGroup uses delegation
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove it parts

if (canvasVisible !== undefined) {
viewState?.setHiddenBlock(Boolean(canvasVisible));
// Manual control: delegate canvas rendering when canvasVisible=false
viewState?.setRenderDelegated(!canvasVisible);
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

check is this real needs, because Block will remove self hitbox

width: rect.width + GROUP_PAD * 2,
height: rect.height + GROUP_PAD * 2,
},
collapsed: !!(Math.floor(Math.random() * 10) % 2),
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's unnecessary here. Now, if you work with a story there are groups that open/close at random because of this line, even if you don't touch them

Comment thread src/components/canvas/groups/CollapsibleGroup.ts
const sourcePort = this.$sourcePortState.value;
const targetPort = this.$targetPortState.value;
if (!sourcePort.lookup && !targetPort.lookup) {
return [sourcePort.$point.value, targetPort.$point.value];
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I understand this is breaking change, but hardly anyone signed up for this at all.

* (React overlay mode, hitbox kept). Both flags make isVisible() return false,
* but only blockHidden removes the hitbox.
*/
private blockHidden = false;
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we put it in the state? I see that this has already been done in the connections, but here we have to store data both in the staff and here, and even synchronize them.

@draedful
Copy link
Copy Markdown
Collaborator Author

Code review (branch vs main)

PR: feat(Groups): CollapsibleGroup — large feature: CollapsibleGroup, port delegation, group-collapse-change events, Evented eventedArea, Block visibility/connection hiding, extensive e2e.

Strengths

  • Clear public API (collapse / expand), cancelable group-collapse-change, documented behavior (no built-in dblclick — app wires UX).
  • Port delegation design (delegate / undelegate, savedPoint) is coherent; Group drag guard during isDragging avoids rect fighting block bounds.

Issues (confidence ≥80%)

  1. Collapsed group drag vs grid snapping: CollapsibleGroup.handleDrag updates collapsedRect with per-frame deltaX/deltaY, while Group.handleDrag moves rect using snapped absolute position from diffX/diffY. If SNAPPING_GRID_SIZE > 1, collapsedRect and rect can diverge after a snap jump.

/**
* When dragging a collapsed group, also move the collapsedRect.
*/
public override handleDrag(diff: DragDiff, context: DragContext): void {
const state = this.state as T;
if (state.collapsed && state.collapsedRect) {
const newCollapsedRect = {
x: state.collapsedRect.x + diff.deltaX,
y: state.collapsedRect.y + diff.deltaY,
width: state.collapsedRect.width,
height: state.collapsedRect.height,
};
// Update collapsedRect in the store so it persists
this.groupState.updateGroup({
collapsedRect: newCollapsedRect,
} as Partial<T>);
// Move group edge ports — use getRect so port positions stay consistent
// with where delegatePorts() originally placed them (both use getRect).
this.updateGroupPortPositions(this.getRect(newCollapsedRect));
}
// Let base Group handle the rest (update rect, hitbox, onDragUpdate)
super.handleDrag(diff, context);
}

public override handleDrag(diff: DragDiff, _context: DragContext): void {
if (!this.dragStartRect || !this.lastSnappedPos) {
return;
}
const { x: newX, y: newY } = this.snapPosition(
this.dragStartRect.x + diff.diffX,
this.dragStartRect.y + diff.diffY
);
const deltaX = newX - this.lastSnappedPos.x;
const deltaY = newY - this.lastSnappedPos.y;
this.lastSnappedPos = { x: newX, y: newY };
const rect = {
x: newX,
y: newY,
width: this.state.rect.width,
height: this.state.rect.height,
};
this.setState({ rect });
this.updateHitBox(rect);
if (deltaX !== 0 || deltaY !== 0) {
this.props.onDragUpdate(this.props.id, { deltaX, deltaY });
}

  1. E2E suite collapse, move, expand has setup but no tests — the describe only contains beforeEach; drag-after-collapse scenarios are not executed in CI.

// ---------------------------------------------------------------------------
// Collapse → move group → expand (draggable groups)
// ---------------------------------------------------------------------------
test.describe("collapse, move, expand", () => {
test.beforeEach(async ({ page }) => {
graphPO = new GraphPageObject(page);
await graphPO.initialize({
blocks: BLOCKS,
connections: CONNECTIONS,
settings: {
canDrag: "all",
canDragCamera: false,
},
});
// Use withBlockGrouping so $groupsBlocksMap is populated (required for updateBlocksOnDrag)
await graphPO.page.evaluate(
({ groupRect }) => {
const { CollapsibleGroup, BlockGroups } = (window as any).GraphModule;
const graph = window.graph;
const CollapsibleBlockGroups = BlockGroups.withBlockGrouping({
groupingFn: (blocks: any[]) => {
const result: Record<string, any[]> = {};
blocks.forEach((block: any) => {
const groupId = block.$state.value.group;
if (groupId) {
if (!result[groupId]) result[groupId] = [];
result[groupId].push(block);
}
});
return result;
},
mapToGroups: (_key: string, { rect }: any) => ({
id: _key,
rect: {
x: rect.x - 20,
y: rect.y - 20,
width: rect.width + 40,
height: rect.height + 40,
},
component: CollapsibleGroup,
}),
});
graph.addLayer(CollapsibleBlockGroups, { draggable: true, updateBlocksOnDrag: true });
// Register dblclick handler
graph.on("dblclick", (event: any) => {
const target = event.detail?.target;
if (target instanceof CollapsibleGroup) {
if (target.isCollapsed()) {
target.expand();
} else {
target.collapse();
}
}
});
},
{ groupRect: GROUP_RECT }
);
await graphPO.waitForFrames(5);
});
});

E2E / DX suggestions (non-blocking)

  • Add preventDefault test for expand (symmetry with collapse).
  • Cover collapseDirection / default rect placement beyond custom getCollapseRect.
  • Optional: React zoomed-in path for collapsed groups if consumers rely on HTML blocks.

🤖 Generated with Claude Code

If this review was useful, react with 👍; otherwise 👎.

@draedful draedful merged commit 4f88cdd into main Apr 15, 2026
6 checks passed
@draedful draedful deleted the collapse_group branch April 15, 2026 21:03
@gravity-ui gravity-ui bot mentioned this pull request Apr 15, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants