editor: session multi-select groups (Ctrl+G) - #571
Conversation
|
Addressed Bugbot findings on this PR:
|
|
Addressed the latest Bugbot finding:
|
Aymericr
left a comment
There was a problem hiding this comment.
The core of this is good work, and the part I want to single out is the layering: session-groups.ts is pure, takes liveIds as a parameter rather than reaching for a store, and every function returns new arrays instead of mutating. That's why I could probe its behavior directly, and it's the reason the 25 tests are worth something. wiki/architecture/selection-groups.md naming persistent groups as explicitly out of scope is also the right instinct — it stops the next person assuming these save.
I ran the gates: bun run check fails on 9 errors, all mechanical (check:fix clears them). check-types fails on two real errors, both one-liners:
selection-manager.tsx:775—useRef<SelectionModifierKeys>({meta, ctrl, shift})is missingalt, which you added to the type atselection-routing.ts:13.floorplan-registry-layer.tsx:597—nextSelectedIdsisAnyNodeId[], but the toggle branch feeds itcurrentSelectedIds.filter(...), which widens tostring[]. Theelsebranch already casts; the toggle branch needs the same treatment (or better, type the local asAnyNodeId[]and cast once at theexpandSessionSelectionForNodeboundary, since that helper returnsstring[]).
main typechecks clean, so both are from this branch. Your three new test files pass (25/25) — bun run test doesn't exist on your base; #548 added it after you branched, so a rebase will pick up the harness (you're 18 commits behind, and #579 moved to Next 16.3).
One design issue I'd like changed before this lands: delete is destructive to group membership, and undo doesn't restore it.
removeDeletedIdsFromSessionGroups (use-session-groups.ts:45) commits the pruned result back into the store. I ran the pure helpers to confirm what that costs:
group [a,b,c]; delete a -> stored group becomes [b,c]
undo (a is live again) -> stored group is still [b,c]
click a -> expands to null (a is no longer a member)
group [a,b,c]; delete a and b -> group drops below the 2-member floor, removed entirely
undo -> group is gone for good
Deleting one member of a group and pressing Cmd+Z silently leaves it outside the group; delete two and the group is unrecoverable. Since these are session-only, there's no way to get it back short of re-grouping.
The fix is to delete code rather than add it. Pruning is already lazy at read time — pruneSessionGroups filters on liveIds (:80) and expandSessionGroupMembers re-filters (:172) — so if the store simply keeps full membership and every read filters, undo works with no hook on any delete path. Same probe with nothing committed:
delete a, click b -> [b,c] (a correctly excluded while dead)
delete a, click a -> [a,b,c] (a isn't clickable while deleted, so unreachable)
undo, click a -> [a,b,c] (membership never lost)
delete a and b -> null (below the floor, correctly inert)
undo -> [a,b,c] (group intact)
That drops removeDeletedIdsFromSessionGroups and its two call sites in group-actions.ts (:566, :586), and closes a coverage gap those hooks have anyway: deleteNodes is also called from editor-commands.tsx:154, zone-panel/index.tsx:165 and parametric-inspector.tsx:115, none of which call the prune hook — so membership is already inconsistent depending on how you delete. A read-time filter is correct for all of them at once. The one thing to keep committing is clearGroups() on scene load and version preview (editor/index.tsx:1202, :1240) — different node ids entirely, so stale membership there is genuinely garbage.
While you're in that file: pruneSessionGroupsToScene (:41) has no callers anywhere in the repo. It should go too.
A relevancy question I'd like your read on. packages/core already has a Collection type — {id, name, color?, nodeIds, controlNodeId?} (packages/core/src/schema/collections.ts) — with full store actions (createCollection, addToCollection, …) and a CollectionsPopover. It persists in project JSON, and it's nearly what a session group is, minus the persistence and the click-to-expand. The popover is exported from packages/editor but mounted by no app, so collections are effectively dormant UI over a live data model.
I don't think that makes this PR redundant — expand-on-click is the actual feature here and collections have no selection semantics at all — but two parallel grouping concepts in the same editor will confuse users the moment collections get a real surface, and your wiki page doesn't mention them. Before merging I'd want either a paragraph in selection-groups.md on how the two relate (and which one a user should reach for), or an argument that session groups should read/write collections and get persistence for free. Your call, and I'd genuinely like your opinion — you've been in this code more recently than the collections work.
Smaller notes, none blocking:
- The capture-phase handler is justified, and I checked the neighbours:
floorplan-hotkey-handlers.tsx:41,floorplan-group-move.tsx:384andfloorplan-registry-move-overlay.tsx:436all use capture too, so this matches the local idiom. Two details though. Thedisabledguard is handled (the early return at:176covers the whole effect), bute.stopPropagation()on a capture listener kills the event for every handler below, including those three. Ctrl/Cmd+G isn't a chord any of them claim, so it's fine today — worth a comment saying so, because it won't be obvious to whoever adds the next capture listener. Ande.code === 'KeyG' || e.key.toLowerCase() === 'g'is broader than the comment claims:e.codeis the layout-stable check, and thee.keyfallback re-introduces exactly the layout dependence the comment says it avoids. Pick one. contenteditablein the typing guard is a genuine fix beyond the stated scope — the bubble handler at:217had the same hole. Good catch; worth calling out in the description so it isn't mistaken for drive-by scope.- Alt+click: I checked for collisions and the modifier is free in selection context (existing Alt bindings are all snap-bypass in measurement/number inputs), and both surfaces route it —
floorplan-registry-layer.tsx:630gates onevent.altKey && !(meta||ctrl||shift), and 3D goes throughresolveSelectedIdsForNodeClick. Parity is respected, which is what I'd have checked first. getSelectionModifierKeysinfloorplan-panel.tsx:842still builds{meta, ctrl, shift}with noalt. It's structurally compatible so it typechecks, but it means 2D paths going through that helper can't see Alt. Worth aligning while the type is fresh.
To summarize what I need: fix the two type errors, run check:fix, rebase onto main, replace the destructive prune with read-time filtering (dropping the two delete hooks and the dead export), and add a line to the wiki page on collections. The pure-module design is what makes the prune change small — it's a deletion, not a rewrite. Happy to look again quickly once it's up.
Add editor-only session selection groups so multi-select furniture/items can be regrouped with Ctrl/Cmd+G and reselected by plain click. Group/Ungroup icons sit on the multi-select floating pill and side panel. Not scene-graph; not saved with the project.
Clear session groups on scene load, prune memberships on cut, and pass a proper options object for floorplan Cmd/Ctrl+click toggle.
Preview only applied the preview graph; groups from the edit session could still expand selection. Clear groups on preview entry.
Deleting a group member committed the pruned membership back to the store, so Cmd+Z restored the node outside its group — and deleting two of three dropped the group past the two-member floor for good, unrecoverable since session groups are not in the undo history. Every read already filtered against the live scene, so keeping full membership and filtering only at read fixes undo without new state: the delete hooks in group-actions and the unused pruneSessionGroupsToScene export both go away, and the several other deleteNodes callers that never called the hook are now correct too. Renames pruneSessionGroups to liveSessionGroups to say what it is. Also: - add the missing `alt` to SelectionModifierKeys initializers, and track Alt in the 3D modifier ref so Alt+click actually reaches the 3D path - thread expand + Alt through resolveFloorplanBackgroundSelection, the 2D background hit-test path that bypassed group expand entirely - narrow the Ctrl/Cmd+G capture handler to e.code, and note why its stopPropagation is safe against the sibling capture listeners - document how session groups relate to the persisted collections concept Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
100dcfd to
c804e3f
Compare
|
I've pushed the changes to your branch ( One correction to my review above. I said the collections UI had no consumers. That's wrong — What I changed: The prune fix, as described. Tests: replaced the Alt didn't actually work in 3D. Beyond the type error, A third click path was missing expand. 2D has two routes, not one: registry entries go through Keyboard. Dropped the I left your |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort 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 c804e3f. Configure here.
| canSelectElementFloorplanGeometry, | ||
| canSelectFloorplanZones, | ||
| currentSelectedIds: useViewer.getState().selection.selectedIds, | ||
| expandIdsForNode: expandSessionSelectionForNode, |
There was a problem hiding this comment.
Marquee click skips group expand
Medium Severity
When the floor-plan selection tool is marquee, registry entry handlers are disabled and no-drag clicks go through toggleFloorplanSelection, which always commits a single id. That path never calls expandSessionSelectionForNode and never honors Alt isolate, so plain click does not reselect a session group in marquee mode even though click-tool, 3D, and tree paths do.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit c804e3f. Configure here.
| } else { | ||
| groupCurrentSelection() | ||
| } | ||
| } |
There was a problem hiding this comment.
Ctrl+G leaks clean Ctrl tap
Medium Severity
The capture-phase Ctrl/Cmd+G handler calls stopPropagation, so the bubble handleKeyDown never clears ctrlTapClean when G is pressed. Releasing Ctrl/Meta then still looks like a clean tap and can call cycleGridSnapStep whenever a snap context is active.
Reviewed by Cursor Bugbot for commit c804e3f. Configure here.


What does this PR do?
Adds editor-only session selection groups so multi-select furniture and structure pieces can be remembered and reselected as a set (Figma-like, without scene-graph parents).
Group N)wiki/architecture/selection-groups.md(+ links from selection-managers / architecture index)Out of scope: persistent scene-graph groups, save/load with the project, MCP tools, named-group rename UI.
Related open PRs (same fork style): #569 mcp layout clearance, #570 light WebGPU preview.
How to test
bun test packages/editor/src/lib/session-groups.test.ts packages/editor/src/lib/selection-routing.test.ts packages/editor/src/lib/contextual-help.test.tsExpect all green.
bun+ Next on port 3002), open a scene with furniture.Group 1 · N.Screenshots / screen recording
N/A attached here — interactive multi-select UX. Optional: short clip of Group icon + Ctrl+G expand-on-click if desired.
Checklist
bun teston the packages above and manual editor multi-selectwiki/architecture/selection-groups.md)mainbranchNote
Medium Risk
Broad changes to selection click paths (3D, 2D, tree, keyboard) but session groups stay out of the scene graph and persistence; main risk is regressions in multi-select modifiers or stale expand after preview/load.
Overview
Adds editor-only session selection groups so a multi-select set can be remembered and re-selected without scene-graph parents or project save.
Create / dissolve: Ctrl/Cmd+G groups 2+ selected nodes (auto
Group N); Ctrl/Cmd+Shift+G dissolves groups that intersect the selection while keeping the current selection. A capture-phase keydown handler on KeyG prevents the browser from stealing Ctrl/Cmd+G.Reselect behavior: Plain click on a member expands to all live group members via
expandSessionSelectionForNode, wired through 3DresolveSelectedIdsForNodeClick, 2D registryapplyEntrySelection, floorplan background hits, and the site tree. Alt+click selects a single member without expanding. Alt is tracked in selection modifier keys alongside meta/ctrl/shift.UI: Optional Group / Ungroup on
NodeActionMenuand the 2D/3D multi-select floating pills and docked multi-selection panel (with group label when the selection matches a session group). Help text and keyboard shortcut docs updated.State: New
session-groupshelpers anduseSessionGroupszustand store; groups are cleared on scene load/switch and version preview. Stored membership is not rewritten when nodes are deleted—reads filter by live scene ids so delete+undo can restore group membership.Docs:
wiki/architecture/selection-groups.mdand links from selection-managers / architecture index.Reviewed by Cursor Bugbot for commit c804e3f. Bugbot is set up for automated code reviews on this repo. Configure here.