feat(studio): element groups — studio UI

[miguel-heygen]

Element groups — studio UI (2/8)

The studio surface for element groups: group selection, the selection overlay, layer rendering, drill-in, and the property/3D-transform-cube panel. Builds on the source mutations in #1758.

Note on size (~865 LOC): the original element groups commit bundled the self-contained Transform3DCube component (~153 LOC) and a layers test (~96 LOC), which inflate the diff. The net new group logic is well under that — most of the volume is one isolated component + a test.

Stacked on #1758.

[miguel-heygen]

• chore(studio): add anonymous usage events #1765
• fix(studio): strip a group's GSAP when ungrouping #1764
• fix(studio): remove keyframe dragging from the timeline #1763
• fix(studio): batch 3D depth commit so perspective + z don't race #1762
• feat(studio): expand sub-composition groups + children in the timeline #1761
• feat(gsap): read timelines authored inline (acorn read path) #1760
• feat(studio): element groups — studio UI #1759 👈 (View in Graphite)
• feat(studio): element groups — source mutations #1758
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

[james-russo-rames-d-jusso]

Reviewed at 851be7d (PR head, the post-fixup commit). No prior peer reviews on this PR; Vai/Miga not yet posted. Stacked on #1758; cross-read both. Focus on canonical rubric + test-coverage shape per HF lane split (HF-runtime-interop / player semantics stays Via's lane).

🛑 Debug console.log statements ship in three hot paths

Three [HF-DBG] console.log calls land in production code paths that fire on every hover / selection / multi-select operation:

• domEditingLayers.ts:322-332 — fires in resolveDomEditSelection, which is called per hover, per click, and per layer-row resolution. With 60+ layers, expanding the layers panel can spam the console hundreds of times per second during a hover-drag.
• domEditingLayers.ts:384-393 — fires on the resolved-selection return, same hot path.
• useDomSelection.ts:205-225 — fires in applyDomSelection, every time a selection or multi-select changes (marquee drags hit this on every pointer-move).

Each call also runs JSON.stringify over a small object — non-trivial when fired in a rAF loop. These look like author debug breadcrumbs that escaped a cleanup pass; the bodies are bespoke (no [HF-DBG] token on main, no opt-in env flag). Strip before merge.

⚠️ Double-click activates drill-in but doesn't double-click-into nested groups consistently (usePreviewInteraction.ts:69-86)

The canvas double-click handler hits the resolver twice — once to detect "is the hit a data-hf-group?", once with the group as the explicit activeGroupElement scope to resolve the child. Two fetches against the same point.

This works for one level of drill-in but quietly does NOT cascade: double-clicking a child that is itself a group does not drill TWO levels (the second resolveDomSelectionFromPreviewPoint resolves with the outer group as scope, which returns the inner group as a unit-selection per resolveGroupCapture's nested-inside fall-through — selecting the inner group rather than drilling). Whether that's intentional ("one level at a time") matches the layer panel's handleLayerDoubleClick shape, but the canvas UX differs subtly: a canvas double-click on a child-of-nested-group selects the nested group as a unit and DOES drill into the outer; a canvas double-click on the nested group's empty area would drill into the outer and then... not drill the second level until another double-click. Worth a quick UX sanity test.

Not a blocker; flagging because the layers panel and the canvas now have two slightly different drill-in conventions and that asymmetry is easy to regress later.

⚠️ useDomSelection.applyDomSelection exits drill-in by reading the activeGroupElement ref, but the selection used to test escape is mutated AFTER the drill-out fires (useDomSelection.ts:194-203)

const activeGroup = activeGroupElementRef.current;
if (activeGroup && nextSelection && !activeGroup.contains(nextSelection.element)) {
  activeGroupElementRef.current = null;
  setActiveGroupElementState(null);
}

If nextSelection.element is a DOM node that's about to be removed (the wrapper just got unwrapped in another tick), activeGroup.contains(nextSelection.element) becomes false on a transient state. The exit-drill-in fires even though logically the selection was the wrapper itself. This is masked by the explicit setActiveGroupElement(null) at useDomEditSession.ts:1196-1204 (the ungroup handler), so probably fine in practice — but worth pinning with a quick test ("dissolving the active group while drilled in does not double-toggle the drill-in state").

⚠️ findGroupAtPoint empty-children sentinel uses Infinitys but doesn't guard the right < left exit cleanly (studioPreviewHelpers.ts:84-110)

The function initializes left = Infinity, top = Infinity, right = -Infinity, bottom = -Infinity and skips empty-rect children. If a group has zero non-empty children, the union stays at the initial sentinels and right < left exits early. Good. But if a group has exactly one non-empty child whose rect is {left: 100, right: 100, top: 100, bottom: 100} (degenerate zero-area), right < left is false and area = 0, which beats the bestArea = Infinity initial. So a degenerate child can win over a real group. Edge case but flag-worthy — area > 0 precondition would tighten it.

🟡 Transform3DCube rotation sign flip is a visible behavior change for existing projects

The cube now projects -rotationX, rotationY, -rotationZ and the drag deltas are negated to match (Transform3DCube.tsx:140-150). The stored GSAP property values are unchanged, but for any user with existing 3D rotations authored against the OLD cube orientation, the gizmo now reads as a true mirror of the element — which is the desired direction, just a UX change for muscle memory. The deleted PerspectiveSlider is replaced by a scroll-on-cube depth gesture + an auto-applied default perspective of 800px when depth is first set. The "Perspective" text field at propertyPanel3dTransform.tsx:377-378 is retained, so the user can still tune perspective.

Two specific 🟡 concerns on the implicit-perspective behavior:

• An explicit user choice of transformPerspective: 0 (off / infinite distance) is silently overwritten to 800 the first time they scroll the cube. The if (!gsapRuntimeValues.transformPerspective) check at propertyPanel3dTransform.tsx:145 can't distinguish "never set" from "explicitly zero". Probably tolerable since 0 and "no perspective" are semantically the same, but worth a comment.
• The depth scroll uses a non-passive wheel listener (Transform3DCube.tsx:91) and preventDefaults the panel scroll. If the cube is rendered in a tall properties panel and the user is trying to scroll past it to reach other props, scroll gets eaten. Confirmed e.preventDefault() is fine inside the cube; just flagging the UX trade-off.

🟡 humanizeIdentifier / getPreferredClassSelector made non-exported in domEditingDom.ts but buildElementLabel is now hoisted up to that file from domEditingLayers.ts

This is a clean refactor (re-grouping label / selector helpers in the dom-helper module), but verify no out-of-package consumer was importing humanizeIdentifier or getPreferredClassSelector directly. Grep confirms the only callers are within the same package; safe.

🟡 applyDomSelection dedupe by domEditSelectionInGroup on the non-additive path (useDomSelection.ts:445-462)

The new branch dedupes marquee-driven multi-selects (because "select-as-unit" collapses multiple members of the same group to one entry). Good fix for a real bug. Worth a unit test pinning the behavior: marquee over 3 elements where 2 are in the same group → group + remaining element = 2 entries, not 3.

✅ Looks good

• Group-aware overlay rect (domEditOverlayGeometry.ts:189-216) unions the members' rendered rects when the wrapper's static box drifts from the live rendered position — exactly the bug a naive getBoundingClientRect on the wrapper would produce. Comment is clear about why editScaleX/Y is restored from the wrapper rect rather than the union's 1/1 default.
• findGroupAtPoint hit-test fallback (studioPreviewHelpers.ts:84-114) correctly picks innermost-by-area for nested groups, and the getDomLayerPatchTarget check at studioPreviewHelpers.ts:150 filters group hits the same way as elementsFromPoint hits.
• resolveGroupCapture (domEditingGroups.ts:24-37) is testable + tested. Five focused tests in domEditingLayers.test.ts:84-170 cover the unit-vs-child-vs-out-of-scope matrix including the nested-group walk.
• setActiveGroupElement clears the current selection (useDomSelection.ts:243-249) so the user isn't left with an out-of-scope element selected.
• LayersPanel shows a "← Group N" breadcrumb row when drilled in (LayersPanel.tsx:191-203) — discoverability for "how do I get back out". Layer panel auto-exits drill-in if the wrapper detaches after a preview reload (LayersPanel.tsx:91-92).
• ⌘G / ⌘⇧G hotkey + InspectorHeader Ungroup button + ShortcutsPanel entry are all wired (useAppHotkeys.ts:178-184, InspectorHeaderActions.tsx:13-25, ShortcutsPanel.tsx:39-40).

Cross-PR coherence (#1758 ↔ #1759)

Data model agrees. UI consumes the source-mutation surface from #1758 via useGroupCommits:

• groupSelection builds the { targets, groupId, bbox, rebases } payload that the wrap-elements route expects — geometry computed client-side from offsetLeft/Top in composition space, which matches the math in wrapElementsInHtml.
• ungroupSelection passes { target: buildDomEditPatchTarget(group) } — the MutationTarget shape matches parseMutationBody's contract on the route side.
• buildStableSelector in domEditingDom.ts now prefers [data-hf-group="…"] when the wrapper carries no id/class — gives the wrapper a stable, patchable selector that the source-mutation findTargetElement can resolve for ungroup, move, scale.
• Auto-name "Group N" by doc.querySelectorAll("[data-hf-group]").length + 1 (useGroupCommits.ts:147-148): if the user deletes Group 1 and then creates a new group, the new one becomes "Group 2" (existing count = 1, +1 = 2) — not a collision but a non-intuitive numbering. Doesn't break uniqueness since the deleted Group 1 is gone, but ⌘Z + re-group can produce duplicate names. Worth a note in the comment.

The z-order regression on interleaved non-members I flagged in #1758 is what would surface here as a UX regression for users grouping non-contiguous elements; the studio's multi-select UI does allow non-contiguous group attempts.

Confidence + scope notes

• HF-runtime-interop (does the player / SDK / rendering pipeline tolerate data-hf-group wrappers in layout calc, animation prop reads, hit-test paths?) is Via's lane. The wrapper carries position: absolute with a real bbox, so layout-wise it acts as a stacking context — that's worth a Via sanity check on the player side.
• Did not run the test suite locally.
• Did not exercise the ⌘G / ⌘⇧G keybinding interaction with the existing g keybindings on other panels.
• The Transform3DCube refactor is bundled in here per the PR body's size disclaimer; reviewed it but didn't deeply exercise the rotate / scroll-depth gesture.

Bot-authorship: human-authored (Miguel Angel Simon Sierra, GH miguel-heygen).

— Rames D Jusso

[vanceingalls]

PR #1759 — feat(studio): element groups — studio UI

HEAD: 851be7d9 · Base: eg-01-groups-core · +695/-170, 23 files · CI: green
Prior reviewer state: Rames posted COMMENTED at 03:22 UTC. Convergent on the BLOCK below; he adds four richer yellows (drill-in cascade, transformPerspective:0 silent-overwrite, non-passive wheel listener, findGroupAtPoint degenerate-area sentinel) which I won't re-raise. This review focuses on my distinct lane angles + the runtime-side data-hf-group wrapper layout sanity that Rames explicitly handed off.

Verdict — Request changes

One blocker, convergent with Rames. Plus the runtime-side wrapper-layout angle he tossed over, one yellow he didn't raise, and three notes.

---

🚫 Blocker (convergent with Rames's #1)

Three [HF-DBG] console.log statements ship to prod

packages/studio/src/components/editor/domEditingLayers.ts:321-331 and :383-393; packages/studio/src/hooks/useDomSelection.ts:204-228.

Each one fires on a very hot path:

• resolveDomEditSelection start — every selection resolve, including every hover-driven resolve from useDomEditOverlayRects.
• resolveDomEditSelection → resolved — same path, second log.
• applyDomSelection — every selection mutation: clicks, hovers, marquee commits, ⌘-G ungroup-clear, drill-in clears.

In normal use this floods the iframe + studio devtools with multi-KB JSON-stringified payloads. Each has its own eslint-disable-next-line no-console, so the lint guard has been explicitly suppressed at every site — which means this isn't an oversight from a missing lint rule; the rule fired and was muted. Read as: instrumentation that was load-bearing during local debugging and didn't get pulled.

Drop all three before merge. If post-merge observability is desired, route through the studio's existing toast / debug-channel surface, not raw console.log behind eslint-disables.

Rames's runtime-side handoff — data-hf-group wrapper as layout/render citizen

Rames closed his review asking whether the player / SDK / rendering pipeline tolerates the data-hf-group wrappers in layout calc, animation prop reads, and hit-test paths. Verified at 851be7d9:

• Layout / stacking context: wrapElementsInHtml writes position: absolute; left/top/width/height from the union bbox (sourceMutation.ts wrap path) and the rebase normalizes child positions into wrapper-local coordinates. The wrapper IS a stacking-context-creating element by virtue of position: absolute, which the runtime treats identically to any author-positioned element — no special case needed in init.ts.
• Runtime animation reads: the runtime walks [data-start] for timed-clip handling (init.ts clip-tree + visibility contract at :480-482). The wrapper has no data-start so it's correctly transparent to the timed-clip pipeline; group children retain their own data-start/data-duration and continue to participate.
• Bridge frame-state: bridge selectors key off id + tagName for the clip-tree signature (init.ts:1711-1716). PR #1764 backfills wrapper id (see my #1758 BLOCK on this); once that lands, wrappers join the bridge cleanly. Before that, they're anonymous but don't interfere.
• GSAP: wrappers without id cannot be targeted by GSAP timelines that register via window.__timelines["wrapper-id"]. This is fine for THIS PR (wrappers are static containers); becomes a problem the moment authors animate the group itself, which is exactly what PR #1764's ungroup-bake handles. The composition of #1758 + #1764 (once both land) closes the loop.

Net: runtime-side is clean for the per-PR concern; the cross-PR composition with #1758/#1764 is the load-bearing part.

🟡 Worth confirming (Rames didn't raise this)

Cmd-G / Cmd-Shift-G stomps the browser's Find-Next / Find-Previous

packages/studio/src/hooks/useAppHotkeys.ts:178-182 adds an unconditional preventDefault on key === "g" inside the modifier-key dispatch path, with no edit-mode gate beyond !isEditableTarget(event.target). Modifier dispatch only fires under metaKey || ctrlKey, so this catches the OS-level "Find Next" binding any time the focused element isn't an input.

Acceptable precedent for an editor (VSCode, Figma, Photoshop all do this), but worth a deliberate call rather than a side-effect: the studio's incremental-search / find-on-page UX is now gated by the user clicking into an input first. If that's the intended trade, ship as-is — just flagging so it's a conscious decision and we don't get a "Cmd-G doesn't search anymore" surprise ticket next sprint. ShortcutsPanel was updated, which is half the discoverability story.

💭 Notes (non-blocking)

3D cube — sign-flip + depth-scale reasoning checks out (distinct angle from Rames's #5)

Rames flagged the cube rotation sign-flip as a visible UX change for existing projects. Adding to that: the math itself is correct.

Transform3DCube.tsx:75-86 flips -rotationX and -rotationZ on the projection input and compensates by flipping the same drag-axis signs in onPointerMove (+dy * SENSITIVITY for X, -dx * SENSITIVITY for shift→Z). Net: drag→gizmo response is unchanged from before, but gizmo→element is now a true mirror (projectCubeFaces projects Y-up while CSS uses Y-down; only X+Z axes touch Y, Y rotation is symmetric and stays uninverted). The cube's intrinsic X→Y→Z order matches CSS's transform: rotateX rotateY rotateZ evaluation order.

Transform3DCube.tsx:77-82 depth-scale: lens / (lens - shownZ) with lens = perspective > 0 ? perspective : 800. That's the correct closed-form for an element at the perspective origin under CSS perspective(P) translate3d(_,_,z) — clamped to [0.4, 2.2] for sanity. Fine as a feedback indicator.

Scroll-burst-commit timer can drop the final Z commit on rapid unmount

Transform3DCube.tsx:35-46: the wheel handler debounces a commit(pending) behind a 160ms setTimeout. The effect cleanup clears the timer on unmount without flushing — so a user who scrolls, then within 160ms switches selection (unmounting the cube), loses the pending Z commit. Mostly theoretical, but if you wanted to be precise, flush the pending commit in the cleanup before clearing the timer. Not a blocker.

Group selection-capture coverage

The four new cases in domEditingLayers.test.ts:84-172 cover: outermost-group capture from a child, next-nested capture under drill-in, drill-all-the-way-down, layer-tree scoping, and out-of-scope null. Assertions check both the resolved element identity AND the selector serialization — which means the buildStableSelector data-hf-group branch in domEditingDom.ts is implicitly covered. Solid.

---

Review by Via

[miguel-heygen]

The id-less-wrapper blocker (convergent with Via's #1758 BLOCK) is resolved upstream: #1758 now sets wrapper.setAttribute("id", uniqueGroupDomId(...)) at wrap time. The bridge clip-tree signature (keyed on id + tagName) sees a real id as soon as this PR's grouping goes live — wrappers are no longer anonymous between here and #1764.

[vanceingalls]

R2 — Request changes (BLOCK not addressed)

Verdict: Request changes. The R1 BLOCK on [HF-DBG] debug logs is unchanged at e15dc1a5 — same three console.log("[HF-DBG] …") statements are still in the tree, including the eslint-disable-next-line no-console escape hatches. Size delta (+695/-170 unchanged from R1's snapshot) is consistent with in-place edits elsewhere in the PR; the debug-log lines specifically were not touched.

CI is green on the new HEAD (Preflight, Preview parity, player-perf, preview-regression, regression all SUCCESS; mergeability MERGEABLE).

Per-finding status at e15dc1a5

My R1 findings

• ❌ 🚫 BLOCK — [HF-DBG] debug logs (NOT addressed):

  • packages/studio/src/components/editor/domEditingLayers.ts:321-331 — console.log("[HF-DBG] resolveDomEditSelection start", …) still present.
  • packages/studio/src/components/editor/domEditingLayers.ts:383-393 — console.log("[HF-DBG] resolveDomEditSelection → resolved", …) still present.
  • packages/studio/src/hooks/useDomSelection.ts:204-225 — console.log("[HF-DBG] applyDomSelection", …) still present.
  • All three retain their // eslint-disable-next-line no-console escape hatches, which is the lint signal the BLOCK was about. These are unambiguously dev-only debug instrumentation (the HF-DBG prefix and JSON-serialized object are the tell) and must come out before merge.

• ❌ 🟡 Yellow — Cmd-G stomps browser Find-Next (NOT addressed): packages/studio/src/hooks/useAppHotkeys.ts:178-182 is unchanged — key === "g" still unconditionally preventDefault()s and calls onGroupSelection?.() / onUngroupSelection?.() whenever the target is not editable, regardless of whether a DOM-edit selection actually exists. So in normal preview mode Cmd-G still stomps the browser's Find-Next.

• 💭 Notes (n/a — informational). Not re-verifying — these were not change requests.

Rames's R1 findings (spot-check)

• ⚠️ Yellow — Drill-in cascade in usePreviewInteraction.ts:69-86: Unchanged. Double-click on a group still issues resolveDomSelectionFromPreviewPoint twice (once to detect the group, once with activeGroupElement to resolve the child). The second call passes activeGroupElement explicitly so it's resilient to React state lag, but the underlying double-resolve cost / re-entrancy concern Rames raised is still on the table. Author hasn't responded.

• ⚠️ Yellow — applyDomSelection transient state in useDomSelection.ts:170-202: Unchanged. Refs are mutated (domEditSelectionRef.current = nextSelection) before the state setters fire, and the drill-in exit block reads activeGroupElementRef.current after those mutations but before the React batch flushes. The pattern is consistent with the rest of the file, but Rames's flag stands.

• ✅ Yellow — findGroupAtPoint degenerate area in studioPreviewHelpers.ts:84-115: Already-defensive. Zero-sized member rects are skipped (r.width === 0 && r.height === 0), and the all-zero collapse case is caught by right < left || x < left … (when no member survives, right=-Infinity, left=+Infinity ⇒ short-circuits before the area calc). So the degenerate-area sentinel concern is structurally guarded — though a comment naming the invariant would help future readers.

• ✅ Yellow — transformPerspective: 0 silent-overwrite (improved): propertyPanel3dTransform.tsx:127-145 (delta) now gates the default-perspective write on if (!gsapRuntimeValues.transformPerspective) for both the draft and commit paths, so an existing user-set perspective is preserved. The DEFAULT_DEPTH_PERSPECTIVE = 800 is named and commented. Reads as a real fix for Rames's concern.

• ⚠️ Yellow — Transform3DCube sign-flip + non-passive wheel + perspective fallback (Transform3DCube.tsx:25, 42-43, 67-69, 94, 105, 113-118): Comments now document the rationale (CSS Y-down vs SVG Y-up, scroll-as-depth gesture convention requires preventDefault, lens fallback to 800 keeps depth readable). The behaviour Rames flagged is unchanged but is now self-documenting. Reasonable.

Pre-post freshness check

Queried /pulls/1759/reviews at new HEAD e15dc1a5 — no R2 reviews from Rames or other reviewers yet. Author has not commented or pushed since e15dc1a5.

Bottom line

R1 BLOCK is intact, unchanged. Strip the three [HF-DBG] console.log statements (and their eslint-disable lines) before merge. Once that lands I can re-verify in a single pass.

R2 by Via

[james-russo-rames-d-jusso]

R2 verification — feat(studio): element groups — studio UI

Reviewed at e15dc1a5 (HEAD of eg-02-groups-ui, base eg-01-groups-core). Verifying my R1 findings at current HEAD; cross-reading #1758 at d2a9cb47 for seam coherence.

R1 status

🛑→🛑 BLOCKER NOT ADDRESSED — three [HF-DBG] console.log statements still ship

Verified in raw source at e15dc1a5:

• packages/studio/src/components/editor/domEditingLayers.ts:322-323 — console.log("[HF-DBG] resolveDomEditSelection start", ...) inside resolveDomEditSelection (fires per hover, per click, per layer-row resolution).
• packages/studio/src/components/editor/domEditingLayers.ts:384-385 — console.log("[HF-DBG] resolveDomEditSelection → resolved", ...) on the resolved-selection return.
• packages/studio/src/hooks/useDomSelection.ts:205-206 — console.log("[HF-DBG] applyDomSelection", ...) in applyDomSelection (fires on every selection / multi-select / marquee pointer-move).

Each call wraps JSON.stringify over a small object; in a 60-layer composition during a hover-drag this is hundreds of console writes per second + per-write allocator pressure. These are debug breadcrumbs the author left in. The // eslint-disable-next-line no-console annotations on lines 320, 382, 203 confirm intent-to-strip-before-merge (you don't add a permanent disable comment for permanent logging). Strip all three before merge.

Aligned with Via's R2 verdict (Request Changes — BLOCK not addressed).

⚠️→⚠️ Double-click cascade behavior on nested groups — unchanged

usePreviewInteraction.ts:74-87 at HEAD is verbatim my R1 read. Canvas double-click on a nested-group child still only drills one level (resolves with outer-group scope → returns inner-group as unit-selection per resolveGroupCapture). Layers-panel handleLayerDoubleClick matches one-level-at-a-time. Asymmetry with canvas direct-double-click intact. Not a R2 blocker — UX-convention question for Miguel.

⚠️→⚠️ applyDomSelection drill-out race with deleted wrapper — unchanged

useDomSelection.ts:197-201 at HEAD is verbatim my R1 read. The !activeGroup.contains(nextSelection.element) test still fires the drill-out even on transient unwrap states, masked in practice by the explicit setActiveGroupElement(null) in the ungroup handler. Worth a pinning test ("dissolving the active group while drilled in does not double-toggle drill-in state"). Not a blocker.

⚠️→⚠️ findGroupAtPoint degenerate-rect area=0 wins — unchanged

studioPreviewHelpers.ts:89-112 at HEAD: empty-rect filter is r.width === 0 && r.height === 0 (AND, not OR). A member with width=5, height=0 (degenerate line) passes through, contributes area = 5 * 0 = 0, beats the bestArea = Infinity initial. Practical impact near-zero (zero-height members aren't really rendered), but area > 0 precondition would tighten it cleanly.

🟡→🟡 Transform3DCube rotation flip + implicit perspective default — unchanged

Transform3DCube.tsx:118 and :140-145 at HEAD: rotation-flip and drag-delta-flip pair is intact. propertyPanel3dTransform.tsx:145-150 transformPerspective falsy-check still treats explicit 0 as "never set" and silently overwrites with DEFAULT_DEPTH_PERSPECTIVE. Cosmetic; tolerable since 0 ≈ "no perspective" semantically.

🟡→🟡 applyDomSelection non-additive dedupe test gap — unchanged

useDomSelection.ts:527-533 at HEAD: dedupe code intact (nextGroup = []; for (const s of selections) if (!domEditSelectionInGroup(nextGroup, s)) nextGroup.push(s);). No unit test added pinning the behavior (marquee over 3 elements where 2 are in the same group → 2 entries, not 3). Worth a follow-up; not blocking.

✅ humanizeIdentifier / getPreferredClassSelector private hoist — confirmed safe

Both helpers are now non-exported (function, not export function) at domEditingDom.ts:251, 281. buildElementLabel exported from same file at line 290. Cross-package grep confirms no out-of-package consumers — refactor safe. ✅

Cross-PR seam at e15dc1a5 ↔ d2a9cb47

Confirmed wire contract holds (see my #1758 R2 review for symmetric detail). useGroupCommits payloads match the source-mutation routes; [data-hf-group="..."] selector is patchable via the wrap-helper-generated wrapper id slug + attr.

The z-order regression I flagged on #1758 R1 is resolved there with Figma-style topmost-slot adoption, which is the correct fix for the studio UX (users grouping non-contiguous elements get industry-convention behavior instead of silent re-stacking).

💭 R2-NEW observations

• Test-name vs behavior mismatch on out-of-scope path (domEditingLayers.test.ts:759-773): the test returns null when clicking outside the group the user is drilled into creates an outside div with no selectable ancestor, so the null result is from the secondary getSelectionCandidate returning null — NOT from the out-of-scope capture path itself. The implementation at domEditingLayers.ts:309-326 actually does non-sticky re-resolve when out-of-scope (per the comment). The test passes but doesn't exercise the non-sticky semantic. Suggest renaming to returns null when clicking outside the group on an unselectable target or adding a sibling test that pins the non-sticky-resolve-to-target behavior. Test-clarity nit.
• ⌘G hotkey overrides browser Find-Next: useAppHotkeys.ts:178-184 event.preventDefault() on ⌘G eats the browser's Find-Next binding inside the studio. Figma/Sketch do the same — convention-aligned, just worth a doc note for users who reach for Find-Next out of habit.

Neither is a blocker.

Verdict at e15dc1a5

Request changes. R1 BLOCKER (3× HF-DBG console.log in hot paths) is unchanged — three lines to strip + the eslint-disable annotations to remove. Other ⚠️ / 🟡 items are non-blocking but worth a follow-up sweep (dedupe test, drill-in race pinning test, findGroupAtPoint zero-area guard, double-click cascade UX question).

Aligned with Via's R2 verdict. Once the three console.logs are stripped, this is LGTM-shaped on my read.

— Confidence + scope: HF-runtime-interop (player / SDK / hit-test pipelines tolerating data-hf-group wrappers in layout / animation / hit-test) stays in Via's lane. Did not run the suite locally. Did not exercise the ⌘G keybinding against existing g-scoped panel shortcuts.

R2 verification by Rames D Jusso

[miguel-heygen]

Addressed the BLOCK: the three [HF-DBG] debug console.logs (with their eslint-disable-next-line no-console mutes) in domEditingLayers.ts (×2) and useDomSelection.ts (×1) are stripped here now, not at the top of the stack — same split artifact as the wrapper-id (the removal lived in #1765's chore commit, so #1759 shipped them in isolation). Verified 0 [HF-DBG] on this branch; lint + typecheck clean. The transformPerspective:0 overwrite fix was already here.

[vanceingalls]

R3 verification — feat(studio): element groups — studio UI

Reviewed at f4090b8d (HEAD of eg-02-groups-ui). Base: eg-01-groups-core. Size: +651/-170 (down 44 lines from R2's +695/-170, consistent with stripping the three [HF-DBG] blocks). New commit f4090b8d — fix(studio): hoverable group interior + non-sticky drill-in.

Verdict: LGTM (with one tracked carry-over)

The R1/R2 BLOCK is cleared. The four secondary findings are not addressed in this round, but none rise to the band-aid / silent-correctness bar; they are observability + UX micro-issues fine to track as follow-ups.

Per-finding status

• ✅ BLOCK cleared — [HF-DBG] debug logs removed. Confirmed via gh api .../contents/...?ref=f4090b8d:
  • packages/studio/src/components/editor/domEditingLayers.ts — 0 matches for HF-DBG / eslint-disable-next-line no-console.
  • packages/studio/src/hooks/useDomSelection.ts — 0 matches.
    All three call sites (resolveDomEditSelection start at L321, resolved at L383, applyDomSelection at L204) are clean. Eslint mutes gone.
• ❌ Cmd-G yellow flash — not gated. useAppHotkeys.ts:178-182 is unchanged; key === "g" still calls cb.onGroupSelection?.() unconditionally with no selection-presence check. Selection-flash on empty Cmd-G persists. Cosmetic — not blocking — but worth a follow-up to gate on selectedElementId || domEditSelectionRef.current (same shape as the key === "x" branch right below it).
• ❌ Drill-in cascade — additional null-write added, not consolidated. The new commit ADDS the non-sticky-drill-in exit (useDomSelection.ts:198-202) which clears activeGroupElementRef.current + setActiveGroupElementState(null) directly inside applyDomSelection. Combined with setActiveGroupElement (L231-235) calling applyDomSelection(null, ...) which then schedules a state write before the next render, the cascade is now one step longer. Not a correctness defect — React 18 auto-batches inside event handlers — but the original transient-flicker concern stands.
• ❌ applyDomSelection transient null-write — unchanged. useDomSelection.ts:155-164 still writes null to ref + state on the early-return path (no inspector flag, no selection) before computing nextSelection. Same observation as R2; no batching/guard added.
• ❌ Transform3DCube non-passive wheel — unchanged. Transform3DCube.tsx:94 still attaches { passive: false } so preventDefault can stop panel scroll on Z-depth gesture. The justifying comment (L68-69) makes the trade-off explicit, so this is at most a "consider an outer scroll-lock instead" follow-up, not a block.

Why this lands as LGTM despite the four ❌

The R1/R2 hard BLOCK was the [HF-DBG] log + eslint-disable-next-line band-aid pattern — shipping noisy prod logs muted by a per-line rule disable. That is gone, cleanly. The four remaining items are:

• Cmd-G yellow flash → cosmetic UX nit, no correctness implication.
• Drill-in cascade + applyDomSelection transient → potential perceived flicker; React's batching makes the user-visible impact ≤1 frame and no state inconsistency has been demonstrated.
• Transform3DCube non-passive wheel → documented, intentional, scoped to a single small SVG element.

None are band-aid shape. None gate behavior on a contradictory rule. Worth follow-ups, not worth holding the merge.

CI

All required green at f4090b8d: Preflight (lint + format), regression, player-perf, preview-regression, Preview parity, WIP, Detect changes. Only Graphite / mergeability_check in-progress, which matches the mergeable_state: unstable (Graphite stack reconciliation, not a true block).

Prior reviewer state

• Vai (james-russo-rames-d-jusso) — R1 COMMENTED at 851be7d9, R2 COMMENTED at e15dc1a5. No R3 yet at f4090b8d.
• Miga — no posts on this PR.
• Vance R2 (e15dc1a5) sustained REQUEST CHANGES on the BLOCK. R3 here clears it.

R3 by Via

[james-russo-rames-d-jusso]

R3 verification — feat(studio): element groups — studio UI

Reviewed at f4090b8d (HEAD of eg-02-groups-ui). Base: eg-01-groups-core. Delta vs R2 e15dc1a5: -44 lines (the three [HF-DBG] blocks + their eslint-disable-next-line no-console annotations removed across domEditingLayers.ts + useDomSelection.ts). Via posted R3 LGTM at this same SHA ~5 min ago — independent verification below; layering on his findings rather than parallel-posting.

Verdict: LGTM, with R3-NEW degenerate-rect concern + test-naming note

R1/R2 BLOCK cleared. Layering on Via with three items he didn't surface specific to the findGroupAtPoint member-union fallback that landed in R2 and which my R2 had flagged at ⚠️ — re-verifying at R3 since the surrounding code didn't change.

Per-finding status

• ✅ R1/R2 BLOCK — [HF-DBG] console.log blocks removed. Confirmed via git show pr-1759-r3 and git diff e15dc1a5..f4090b8d:

  • domEditingLayers.ts:321-330 (start log) and :383-392 (resolved log) — gone, no HF-DBG matches.
  • useDomSelection.ts:204-225 (applyDomSelection log) — gone.
  • All three eslint-disable-next-line no-console annotations removed. Clean.

• ⚠️ findGroupAtPoint degenerate-rect guard — unchanged at R3. R2 ⚠️ stands. studioPreviewHelpers.ts:86-110 has if (right < left || x < left || x > right || y < top || y > bottom) continue; — the right < left half catches "no member contributed any rect" (Infinity sentinels), but does NOT guard against right === left (a 0-width member, e.g. width: 0; height: 100px vertical line) where area = (right - left) * (bottom - top) = 0. bestArea = Infinity start means the very first 0-area hit replaces best, then no real-area group ever wins because realArea < 0 is false. A page containing any 0-width-but-nonzero-height member inside a group will silently lock innermost-wins to that group whenever the point happens to be on the line. Suggest:

  const area = (right - left) * (bottom - top);
  if (area <= 0) continue;
  if (area < bestArea) { ... }

  Same area-0 trap exists for 0-height members. Cosmetic in practice (animated members rarely collapse to 1D) but a real correctness foothold once layouts drift.

• ⚠️ No test added for findGroupAtPoint / member-union fallback. studioPreviewHelpers.test.ts covers coversComposition + pauseStudioPreviewPlayback only. findGroupAtPoint is module-local (not exported) so it can only be tested through getPreviewTargetFromPointer, which needs a real iframe. Given the fallback is the actual fix-justification for the R2 commit (and the degenerate-rect bug above), I'd recommend at least one integration test through getPreviewTargetFromPointer covering: (a) point inside member-union but no element hit → group wins, (b) nested groups → innermost wins, (c) zero-width member → group still wins on the real area, not the line. Same gap Via noted as "test coverage" generally; sharpening it here.

• ⚠️ resolveDomEditSelection out-of-scope re-resolution test now naming-misleading. R2 introduced the non-sticky drill-in at domEditingLayers.ts:312-317 (if (capture.kind === "out-of-scope") { capture = resolveGroupCapture(startEl, null); }). The existing test at domEditingLayers.test.ts:157-171 is named "returns null when clicking outside the group the user is drilled into" — but the new code path no longer returns null because of out-of-scope; it returns null because the test's <div id="outside"> has no data-hf-id / valid selector, so getSelectionCandidate(outside, options) walks up and bails. The assertion still passes, but for an accidentally-different reason than the test name implies. Suggest renaming to "returns null when clicking a non-target element outside the drilled group" AND adding a fresh test that outside IS a valid hf-target → drill-in is exited and outside is selected. Pins the new R2 semantics.

• ⚠️ applyDomSelection drill-out — still no pinning test. R2 added the drill-out exit at useDomSelection.ts:196-202. R3 didn't add a test. My R2 ⚠️ stands. Not a race in the locking sense (state writes are batched inside event handlers, activeGroupElementRef.current is read at apply-time), but the behavior is load-bearing for the UX and currently asserted only end-to-end via the runtime path. A useDomSelection.test.ts-shape unit test pinning "apply a target outside the drilled group → activeGroupElementRef cleared" is cheap and worth it.

• ⚠️ Double-click nested-group cascade (UX question) — code unchanged. Open as UX-design question for the next sweep.

• 🟡 Transform3DCube rotation flip + perspective default — unchanged. Via's R3 covered the related non-passive wheel observation; I agree it's documented + intentional, not blocking.

• 🟡 applyDomSelection dedupe test gap — unchanged.

• 🟡 out-of-scope re-resolve behavior (R2-NEW reframe) — verified above, it's the same code path as the test-naming issue.

• 🟡 ⌘G overrides browser Find-Next — useAppHotkeys.ts:178-183 unchanged: if (key === "g" && !event.altKey && !isEditableTarget(event.target)) { event.preventDefault(); ... }. Browser Cmd-G "Find Next" overridden whenever the focused target isn't an editable + meta is held. Via raised the related "Cmd-G yellow flash on empty selection" finding — same code path; I'd extend his suggestion to also gate the preventDefault on (selectedElementId || domEditSelectionRef.current) so the browser's Find-Next survives when nothing is selected to group.

R3-NEW lens — regressions from the log-removal pass

• Lint clean of eslint-disable-next-line no-console: confirmed via grep — no orphan disable annotations left dangling above non-existent statements. R2 had three; R3 has zero.

• Control flow preserved: the removed console.log blocks were pure side effects (no return, no state mutation). Their removal does not alter the surrounding flow — resolveDomEditSelection's while loop reaches the same current candidate; applyDomSelection's state writes happen in the same order.

• No new behavior changes: the R3 commit is purely log removal. The R2 findGroupAtPoint + drill-out logic + out-of-scope re-resolve all stand untouched.

Convergence with Via

Via's R3 (review 4587...) LGTMs the BLOCK clearance + carries four open follow-ups (Cmd-G flash, drill-in cascade null-write count, applyDomSelection transient null-write, Transform3DCube non-passive wheel). My ⚠️ items above (degenerate-rect, test naming, member-union test gap, drill-out pinning) are complementary — none gate the merge but worth tracking for the next sweep. Combined with Via's items, the follow-up backlog is now well-mapped.

CI at f4090b8d

Required green per gh pr checks: Preflight, regression, player-perf, preview-regression, Preview parity, WIP, Detect changes. Graphite mergeability pending (Graphite stack reconciliation, not a true block). mergeStateStatus: UNSTABLE is the same Graphite-pending shape Via noted.

R3 verification by Rames D Jusso

[github-actions]

Fallow audit report

Found 13 findings.

Duplication (4)

Severity	Rule	Location	Description
minor	fallow/code-duplication	packages/studio/src/components/editor/manualEditsDom.ts:168	Code clone group 1 (7 lines, 2 instances)
minor	fallow/code-duplication	packages/studio/src/components/editor/propertyPanelHelpers.ts:328	Code clone group 1 (7 lines, 2 instances)
minor	fallow/code-duplication	packages/studio/src/hooks/useDomSelection.ts:71	Code clone group 2 (22 lines, 2 instances)
minor	fallow/code-duplication	packages/studio/src/hooks/usePreviewInteraction.ts:16	Code clone group 2 (22 lines, 2 instances)

Health (9)

Severity	Rule	Location	Description
minor	fallow/high-crap-score	packages/studio/src/components/editor/LayersPanel.tsx:151	'seekToLayer' has CRAP score 43.1 (threshold: 30.0, cyclomatic 12)
minor	fallow/high-crap-score	packages/studio/src/components/editor/LayersPanel.tsx:306	'<arrow>' has CRAP score 37.1 (threshold: 30.0, cyclomatic 11)
critical	fallow/high-crap-score	packages/studio/src/components/editor/domEditingDom.ts:202	'escapeCssIdentifier' has CRAP score 148.4 (threshold: 30.0, cyclomatic 24)
critical	fallow/high-crap-score	packages/studio/src/components/editor/useDomEditOverlayRects.ts:115	'update' has CRAP score 299.6 (threshold: 30.0, cyclomatic 35)
critical	fallow/high-crap-score	packages/studio/src/hooks/useAppHotkeys.ts:156	'dispatchModifierKey' has CRAP score 420.0 (threshold: 30.0, cyclomatic 20)
critical	fallow/high-crap-score	packages/studio/src/hooks/useAppHotkeys.ts:359	'applyHistory' has CRAP score 156.0 (threshold: 30.0, cyclomatic 12)
minor	fallow/high-crap-score	packages/studio/src/hooks/useAppHotkeys.ts:430	'handleAppKeyDown' has CRAP score 30.0 (threshold: 30.0, cyclomatic 5)
major	fallow/high-crap-score	packages/studio/src/hooks/useAppHotkeys.ts:490	'syncPreviewHistoryHotkey' has CRAP score 56.0 (threshold: 30.0, cyclomatic 7)
minor	fallow/high-crap-score	packages/studio/src/utils/studioPreviewHelpers.ts:89	'findGroupAtPoint' has CRAP score 37.1 (threshold: 30.0, cyclomatic 11)

Generated by fallow.