feat(studio): element groups — source mutations#1758
Conversation
Fallow audit reportFound 10 findings. Dead code (1)
Duplication (6)
Health (3)
Generated by fallow. |
james-russo-rames-d-jusso
left a comment
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
Reviewed at 330815a (PR head). No prior peer reviews on this PR; Vai/Miga not yet posted. Focus on canonical rubric + test-coverage shape per HF lane split (HF-runtime-interop / SDK-shape stays Via's lane).
⚠️ Z-order regression on interleaved non-member elements (sourceMutation.ts:469-473)
wrapElementsInHtml orders members by parent.children position and appendChilds them into the wrapper sitting at the first member's old slot. Non-member elements that were originally between two members get hoisted above the wrapper post-grouping:
Before: [member-A, non-member, member-B, outside]
After: [wrapper{member-A, member-B}, non-member, outside]
Old order: non-member paints between member-A (below) and member-B (above).
New order: non-member paints above the whole group (entire group paints as one stacking context at the wrapper's slot).
For any composition with a non-member z-stacked between two members (badge between background + foreground text, etc.), grouping silently changes what's visible on top. The fixture in sourceMutation.test.ts:23-31 puts outside AFTER all three members, so this case isn't covered.
Options:
- Reject grouping when the selected members aren't contiguous in the parent (matching the "single parent" guard's spirit). Most defensive.
- Promote the wrapper to the last member's slot to preserve "members render above prior siblings" — still changes order for siblings after the last member.
- Accept the regression and document it in the studio UX so users expect re-stacking.
Worth at least a test case + an explicit decision. I'd lean reject-non-contiguous given how subtle the visual symptom is.
⚠️ Test coverage gap — interleaved non-members + nested groups round-trip (sourceMutation.test.ts:23-128)
The three tests cover: happy-path wrap, round-trip with non-member AFTER the group, and cross-parent rejection. Missing:
- Non-member interleaved between members (the z-order issue above).
- Nested-group round-trip: wrap → wrap inner subset → unwrap inner → unwrap outer restores everything. Source mutations need to be reentrant against pre-existing
data-hf-groupsiblings since the studio UI in #1759 supports nested groups (verified atdomEditingLayers.test.ts:80-117). - Empty-group unwrap: wrapper with zero element children — current code is a no-op on coordinates but still removes the wrapper. Worth pinning the behavior.
- Wrapper missing inline
left/topstyle:getInlineStylePxfalls back to0, so a hand-edited or partially-corrupted wrapper silently positions children at the original coordinates rather than restoring them. Silent recovery is probably fine, but pin it.
🟡 Partial target match silently proceeds (sourceMutation.ts:434-446)
If callers send N targets and only K resolve, wrapElementsInHtml proceeds with K, returning matched: true. The route forwards that as success. A buggy client (stale selector, just-deleted element, indices off by one) gets a partial group with no signal. Suggest either returning a matchedCount so the client can warn, or rejecting when els.length !== targets.length. The dedupe-by-ref logic at sourceMutation.ts:438-444 makes counting tricky, but the dropped-target case is distinct from dedupe and worth surfacing.
🟡 commitStructuralMutation reads then writes original content but only stages patchedContent (useGroupCommits.ts:88-101)
saveProjectFilesWithHistory is passed originalContent via the readFile adapter and patchedContent as the desired after — this is the studio's normal pattern, but worth double-checking that on an empty mutate response (mutateData.content === undefined, the fallback to originalContent kicks in) we don't end up writing a no-op history entry labeled "Group elements" with before === after. Defensive: skip the saveProjectFilesWithHistory call when the route reports changed: false. Low-stakes — undo on a no-op just feels weird.
🟡 Snapshot codepath asymmetry between wrap-elements and unwrap-elements (routes/files.ts:1597-1599 vs 1656-1662)
wrap-elements writes its own snapshotBeforeWrite + writeFileSync block inline because it needs to return groupId. unwrap-elements delegates to writeIfChanged, which snapshots in the helper. Functionally both snapshot, but two codepaths means two things to keep in sync if writeIfChanged evolves. Could be folded with a small extension to writeIfChanged that accepts extra response fields. Nit-grade.
💭 Fallow audit signals worth a look
fallow/unused-fileonuseGroupCommits.ts— expected for the bottom of the stack (#1759imports it). Confirmed not a real signal.fallow/high-crap-scoreoncommitStructuralMutation(72),groupSelection(90),ungroupSelection(42) — the count comes from cyclomatic + low coverage. The hook has no unit tests at all (only the helpersourceMutation.test.tsis covered); a small fixture test ofgroupSelection's "must be same file" gate + the multi-target path would tank the CRAP score and pin behavior.- Two duplication clones with
useElementLifecycleOps.ts(the existing structural-mutation shape). Worth a follow-up to factor acommitStructuralMutationhelper that both consume — explicitly out-of-scope here but a real signal.
✅ Looks good
- Coordinate rebase math is symmetric (
wrapsubtracts wrapper origin,unwrapadds it back) and the test exercises three positioning flavours (inline left/top, GSAP transform delta,--hf-studio-offsetvar) — exactly the cases that would silently corrupt under naive bbox math. parseStyleDecls/serializeStyleDeclsextraction is a clean refactor; reused bygetInlineStylePxso wrap reads the same parser its writes go through.- Route input validation rejects non-finite numbers before interpolation into the inline style string — closes the obvious injection vector.
parseMutationBodyenforcestargetis present; unwrap route is safe to callparsed.targetnon-null.- Single-parent guard with HTTP 422 is well-chosen — distinct from 400 (bad payload) and surfaceable as a UX message.
Confidence + scope notes
- HF-runtime-interop (does the player / SDK handle
data-hf-groupwrapper nodes in layout / animation / hit-test pipelines?) is Via's lane — not verified here. - Did not run the test suite locally.
- Backwards-compat for projects with pre-existing
data-hf-groupattributes in source files: none on main today, so green-field. - Stack-cumulative review: the
wrap+unwrapinteraction with the studio UI lives in#1759; reviewed separately.
Bot-authorship: human-authored (Miguel Angel Simon Sierra, GH miguel-heygen).
— Rames D Jusso
vanceingalls
left a comment
vanceingalls
left a comment
There was a problem hiding this comment.
PR #1758 — feat(studio): element groups — source mutations
Verdict: REQUEST CHANGES — 1 BLOCK + 3 NIT + 2 thought
Head SHA: 330815a9 (head of eg-01-groups-core, base main)
Lane: runtime-interop, SDK, player semantics. Rames posted COMMENTED at 03:22 UTC on canonical-correctness / coverage — he holds the z-order-regression-on-interleaved-non-members concern, test coverage gaps (interleaved + nested-group round-trip + empty-group unwrap + missing-inline-style wrapper), partial-target-match silent proceed, no-op history entry, and snapshot codepath asymmetry. I won't re-raise; my read escalates beyond his with a BLOCK on the cross-PR composition (wrapper-id missing). Read alongside.
Stack awareness: This is the bottom of an 8-PR Graphite stack splitting #1736. The wrap/unwrap helpers shipped here are wired into the studio session hook by #1759 and patched by #1764 (eg-07-ungroup-positions). My review reads helper changes in stack context per feedback_graphite_stack_review_final_state.md.
CI state
| Check | Result | Owner / cause |
|---|---|---|
| Build, Typecheck, Test, SDK unit+contract+smoke, Studio load smoke, runtime contract, Render/Tests on windows-latest, Format, Perf:* | pass | — |
| Lint | fail | Infra: bun install --frozen-lockfile failed extracting @img/sharp-libvips-linux-x64@1.2.4 tarball from the npm registry. Not author-caused; pre-existing-state. Per feedback_verify_pre_existing_state_before_dissent, this is an infra flake — not a blocker for the author. Recommend a re-run before merge. |
| Fallow audit | fail (10 findings) | Mostly clone groups against useElementLifecycleOps.ts (not in this PR — pre-existing) and code-duplication inside the new test file (intentional fixture parallels for T7 hfId cases). useGroupCommits.ts "unused-file" finding is the canonical stack-split symptom: the consumer (useDomEditSession) lands in #1759, so the file is unreachable from any entry point until #1759 lands. CRAP-score on commitStructuralMutation / groupSelection / ungroupSelection is the only finding worth a thought — see 💭 below. Not a blocker; flag the stack-split nature in the merge comment. |
| regression-shards (8 shards) | pending | — |
🚫 BLOCK — Wrapper ships without an id; #1764 explicitly calls this broken
packages/core/src/studio-api/helpers/sourceMutation.ts:479-485 (wrapElementsInHtml):
const wrapper = document.createElement("div");
wrapper.setAttribute("data-hf-group", groupId);
wrapper.setAttribute("style", `position: absolute; left: …`);PR #1764 (5 PRs above in the same stack) adds:
wrapper.setAttribute("id", uniqueGroupDomId(document, groupId));
// "A real `id` (slug of the group name) makes the wrapper a first-class node
// in the clip manifest / timeline parent-map (both keyed by id) and a clean
// GSAP target — without it the wrapper is invisible to the timeline and
// breaks child enumeration."That comment from your own follow-up PR is the BLOCK justification: between #1759 (wires useGroupCommits into useDomEditSession with no feature flag — confirmed via diff of #1759) landing and #1764 landing, every wrap operation creates an id-less wrapper that the runtime's id-keyed structures can't address.
ClipManifestClip.id: string | null (packages/studio/src/player/lib/playbackTypes.ts:36) tolerates a null id, but every downstream consumer keyed by id (findTimelineDomNode at packages/studio/src/player/lib/timelineElementHelpers.ts:283, parentCompositionId lookups, GSAP gsap.set("#…") baking added by #1764) silently no-ops on a null/empty id. That matches the "invisible to the timeline" framing in #1764's commit comment.
This is the decorative-gate pattern (feedback_decorative_gate_pattern): the helper produces a wrapper with the read-side selector (data-hf-group) but is missing the populate-side handle (id) the runtime indexes by. Wrap and unwrap each have half the contract.
Fix options, in preference order:
- Move the
uniqueGroupDomId+idassignment from #1764 down into this PR (5 lines + the helper). The id is foundational to the wrapper shape; deferring it ships a broken wrapper at the studio-UI integration point. - If keeping the bottom PR tight is non-negotiable, gate
useGroupCommitswiring in #1759 behind a feature flag that's flipped only after #1764. That removes runtime exposure but pushes ergonomic work onto #1759.
I'd take option 1 — the slug helper is ~15 lines and conceptually belongs with the wrap geometry.
🟡 NIT — Unwrap path accepts any element with data-hf-group-shaped target; no shape guard
packages/core/src/studio-api/helpers/sourceMutation.ts:501-507:
export function unwrapElementsFromHtml(source, groupTarget) {
const { document, wrappedFragment } = parseSourceDocument(source);
const group = findTargetElement(document, groupTarget);
if (!group || !isHTMLElement(group)) return { html: source, unwrapped: false };
const parent = group.parentElement;
…
}findTargetElement walks the hfId / id / selector branches. There's no assertion that the resolved element actually has data-hf-group. A malformed caller (or a Studio bug — say, the canvas state desyncs and re-fires ungroup on a stale selection) can unwrap a non-group <div>, dissolving it into its parent and rebasing its children's left/top by the parent's own left/top — silent data corruption.
The wrap path enforces invariants (single parent, all targets numeric); the unwrap path enforces nothing. Asymmetric gates are the shape feedback_decorative_gate_pattern watches for.
Fix: if (!group.hasAttribute("data-hf-group")) return { html: source, unwrapped: false }; — three lines, mirrors the wrap-side contract. Cheap.
This is independent of the #1764 changes (which add baking/strip on success but don't tighten the entry guard).
🟡 NIT — Route writes wrapper even when the file is byte-identical to source
packages/core/src/studio-api/routes/files.ts:1597-1624 (wrap-elements): the route short-circuits on !result.matched, but on result.matched: true it unconditionally writeFileSync + creates a backup, even when result.html === originalContent (which can't happen today but is reachable under any future refactor that yields "matched + identical").
split-element (1493-1513) has the same pattern — so this is convention-consistent, not a regression. patch-element (1539-1555) and unwrap-elements (1648-1659) both use writeIfChanged.
Suggestion: Use writeIfChanged here too for consistency; the backup-on-no-op is a footgun for users whose backup retention is bounded. Out of scope for this PR if you'd rather sweep all three (split-element, wrap-elements, and any future structural mutation) in one follow-up — call out which.
🟡 NIT — wrap-elements numeric-validation guard doesn't enforce rebases[].target shape
packages/core/src/studio-api/routes/files.ts:1582-1595: the guard verifies left/top/width/height are finite numbers and rebases[i].left/top are finite numbers, but does not type-check rebases[i].target. If a malformed target slips through (e.g. null, [], an object without id/hfId/selector), findTargetElement returns null and the rebase is silently dropped — that member ends up at its old left/top inside the wrapper, visually drifting from the live preview by the wrapper-origin delta.
Same shape as feedback_decorative_gate_pattern: the validator gate gives a false sense of safety while a critical field is unchecked.
Suggestion: Either (a) reject the request with 400 if any rebases[i].target is missing a hfId AND id AND selector, or (b) reject if any rebase fails to resolve (findTargetElement → null) — option (b) is stricter and catches the silent drift on legitimate but stale selectors too. The latter requires bumping the helper to return the unresolved-rebase count, but is the safer contract.
💭 thought — CRAP-score on useGroupCommits.ts helpers will compound
Fallow flags commitStructuralMutation (CRAP 72, cyclomatic 8), groupSelection (CRAP 90, cyclomatic 9), and ungroupSelection (CRAP 42, cyclomatic 6). All three are uncovered (no test for useGroupCommits ships in this PR, fair, it's a React hook around fetch). The structure mirrors useElementLifecycleOps (delete) per the comment at useGroupCommits.ts:65-67 — and Fallow flags clone groups across both files.
If useDomEditCommits and the upcoming useGroupCommits keep growing per-mutation-type, the read → mutate-route → save-with-history → reload pipeline becomes worth extracting. Not blocking for this PR; flag for the next sweep.
💭 thought — SDK boundary
wrapElementsInHtml, unwrapElementsFromHtml, WrapElementsResult, UnwrapElementsResult, ElementRebase are exported from packages/core/src/studio-api/helpers/sourceMutation.ts but I don't see them re-exported from packages/core/src/index.ts. They are studio-api internals (used by routes/files.ts and tests). Confirm that's the intended boundary — if any future SDK consumer wants programmatic wrap/unwrap, the surface needs explicit promotion and SDK-contract tests (the route-level body validation is the gateway today). For now: studio-api-internal is the right call; just call it out so #1759 / #1764 / etc. don't accidentally leak it into the public SDK surface.
Coverage observations (cross-checking against my-lane angles)
- HTML data- attribute preservation*: confirmed by test
splitElementInHtml — hfId clone isolation(preserves singledata-hf-idon the original, strips from clone) and the wrap round-trip test verifyingtransform,--hf-studio-offset, and inlineleft/topsurvive wrap+unwrap (sourceMutation.test.ts:599-639). data-composition-id, data-start, data-duration aren't directly asserted on wrap members but are preserved by the move-via-appendChildsemantics (which doesn't touch attributes). Reasonable. - Element ID stability: each
appendChildmove preservesid,data-hf-id,data-composition-id. Verified-by-construction. - GSAP timeline binding: NOT tested in this PR — the wrap/unwrap round-trip uses no
<script>block. The #1764 follow-up addsbakeGroupTransformIntoMembersandstripGsapAnimationsForSelectorto handle exactly this, confirming it's a real gap. Out-of-scope for #1758 to test, but worth noting that round-trip-preserves-coordinates ≠ runtime-survives — see BLOCK above. - Route conventions: matches existing
split-element/patch-elementpatterns.wrap-elementshas stronger up-front validation than its peers (numeric guard on bbox + rebases), which is good.unwrap-elementsis too lax — see NIT above. - Public SDK exports: not touched; helpers stay studio-api internal. Correct.
Convergence / divergence with Rames
Confirmed post-Rames-read: he covers neither the wrapper-id BLOCK nor the unwrap-shape gate. His z-order-regression concern (interleaved non-members hoisted above the new group's stacking context) and missing-style-fallback test coverage gap are independent and worth addressing alongside my findings. He explicitly hands off "HF-runtime-interop (player / SDK handle data-hf-group wrapper nodes in layout / animation / hit-test)" to my lane — addressed in detail on my #1759 review where the wrapper-as-layout-citizen analysis lives (this PR's wrapper is fine for layout but the missing id is what breaks the runtime contract; see BLOCK).
Review by Via
miguel-heygen
force-pushed
the
eg-01-groups-core
branch
from
330815a to
ab1aecb
Compare
|
Addressed the BLOCK (wrapper ships without an |
miguel-heygen
force-pushed
the
eg-01-groups-core
branch
from
ab1aecb to
21a7725
Compare
|
Addressed the z-order regression (best long-term fix): the wrapper now lands at the topmost member's slot and carries |
Wrap/unwrap source mutations (group geometry, the wrap-elements / unwrap-elements routes) that the studio group feature is built on. Studio UI lands in the next PR.
miguel-heygen
force-pushed
the
eg-01-groups-core
branch
from
21a7725 to
d2a9cb4
Compare
|
Addressed the unwrap shape-guard NIT: |
vanceingalls
left a comment
vanceingalls
left a comment
There was a problem hiding this comment.
R2 — PR #1758 feat(studio): element groups — source mutations
Verdict: LGTM with two open NITs (carryover)
Head: d2a9cb47 (was 330815a9 at R1). Delta: +636/-5 (was +558/-5); +78 net adds absorbing the wrapper-id fix from #1764, the z-order-via-last-slot logic, the unwrap shape guard, and two new tests.
R1 was REQUEST CHANGES with 1 BLOCK + 3 NITs + 2 thoughts. Rames's R1 (COMMENTED) covered z-order, coverage, partial-target, no-op history, snapshot codepath asymmetry. R2 below verifies each at d2a9cb47.
CI state at d2a9cb47
| Check | Result | Note |
|---|---|---|
| Build, Typecheck, Test, SDK unit+contract+smoke, Studio load smoke, Format, Lint, Preflight, Perf:* | pass | Lint passed this run (R1's Lint failure was the libvips infra flake, now cleared). |
| Fallow audit | fail | Same shape as R1 — stack-bottom unused-file + CRAP score on useGroupCommits. Not author-caused, not blocking. |
| regression-shards (8 shards), CLI smoke, Tests/Render on windows | pending | Long-running, run separately. |
| Build/Test/Typecheck/SDK on the new HEAD | pass | Confirms the test additions land green. |
No blocker-shape failures. Fallow is the same stack-split symptom called out in R1.
My R1 findings — status at d2a9cb47
🚫→✅ BLOCK addressed — wrapper now has id
sourceMutation.ts now contains the uniqueGroupDomId helper (lines 257-272) and wrapper.setAttribute("id", uniqueGroupDomId(document, groupId)) (line 327) — verbatim the change from #1764 with the same code comment ("A real id (slug of the group name) makes the wrapper a first-class node in the clip manifest / timeline parent-map…"). The decorative-gate is closed: runtime-side id-keyed structures can now address the wrapper from the moment #1759 wires the hook.
Slug helper is well-formed: trim → lowercase → non-alphanumeric → -, trim leading/trailing dashes, falls back to "group" on empty, dedupes via document.getElementById lookup with -2/-3/... suffix. Same shape we use elsewhere.
🟡→✅ NIT addressed — unwrap shape guard
unwrapElementsFromHtml at line 375:
if (!group.hasAttribute("data-hf-group")) return { html: source, unwrapped: false };Three lines, mirrors the wrap-side contract exactly as suggested. Test added at line 159-164 (refuses to unwrap an element without data-hf-group) pinning the no-corruption behavior on a plain <div> target.
🟡→⚠️ NIT partial — wrap-elements still bypasses writeIfChanged
routes/files.ts wrap-elements (lines 478-481 of new file): still uses inline snapshotBeforeWrite + writeFileSync. Unwrap (line 504-515) uses writeIfChanged. The asymmetry persists. R1 framed this as "out of scope if you'd rather sweep all three in one follow-up" — accepting that framing here. Worth a tracking comment when this PR merges so the convention sweep doesn't get lost; not a blocker.
🟡→❌ NIT not addressed — rebases[i].target shape unchecked
routes/files.ts numeric validation (lines 440-448) still type-checks only bbox.{left,top,width,height} and rebases[i].{left,top}. rebases[i].target is passed through untyped to findTargetElement. Stale-selector / malformed-target silent-drop remains.
Mitigation in practice: commitStructuralMutation populates rebases[].target via buildDomEditPatchTarget(member) (a typed helper), and wrapElementsInHtml resolves rebases against the same document right after resolving targets. So the practical exposure today is "client supplies typed inputs; helper drops unresolved rebases silently." That's a UX-quality issue (member sits at old left/top post-group) not a correctness/corruption one — wrap won't visually-drift members that weren't selected, and unresolved rebase only affects members the client said to group.
Down-grading to a "follow-up advisable" — would still prefer a 400 on any unresolved rebases[i].target or a returned unresolvedRebaseCount. Not blocking R2.
Rames's R1 findings — status at d2a9cb47
✅ Z-order regression on interleaved non-members — fixed via "lift to topmost slot"
wrapElementsInHtml now computes memberZIndexes from inline styles, picks maxZ, writes it onto the wrapper, and inserts the wrapper at the last member's slot:
parent.insertBefore(wrapper, ordered[ordered.length - 1] ?? null);
for (const el of ordered) { …; wrapper.appendChild(el); }The comment at lines 328-333 calls out the Figma/Sketch convention explicitly. New test lifts the group to the topmost member's slot so an interleaved non-member falls below it (lines 130-157) exercises [low, middle, high] with members {low, high} and asserts topChildren = ["middle", "Group 1"] plus z-index: 4 on the wrapper. This is exactly Rames's "promote the wrapper to the last member's slot" option, with z-index adoption on top.
Note this is a behavior choice (Figma-style) rather than the "reject non-contiguous" option Rames mildly preferred. Reasonable: it preserves grouping for the common case (badge-between-members) and matches industry convention. The test pins the choice.
One subtle remaining edge: non-member siblings that originally rendered above the last selected member (i.e. positioned after the last member with higher implicit z-order) now fall below the new group's stacking context. The Figma convention accepts this asymmetry; the test fixture doesn't include this case but the chosen rule covers it consistently.
⚠️ Test coverage — partial
| Gap | Status |
|---|---|
| Interleaved non-member | ✅ added (the z-order test above) |
| Unwrap shape guard | ✅ added (plain-<div> rejection test) |
| Nested-group round-trip (wrap → wrap-subset → unwrap-inner → unwrap-outer) | ❌ not added |
| Empty-group unwrap (wrapper with zero element children) | ❌ not added |
Missing-inline-style wrapper (getInlineStylePx → 0 fallback) |
❌ not added |
The two highest-value tests (Rames's #1 + my unwrap-guard) landed. The other three are pin-the-behavior tests — worthwhile but lower-stakes. Acceptable.
❌ Partial target match — still silent
wrapElementsInHtml lines 290-298 still dedupes via seen.has(el) and proceeds with els.length > 0. No matchedCount returned; no rejection on els.length !== targets.length. Rames's concern intact.
Practical exposure: same as my rebases[].target finding — the typed client path (commitStructuralMutation + buildDomEditPatchTarget) is the only caller today, so a stale selector here results in a smaller-than-asked-for group, not corruption. Not a R2 blocker but real for any future programmatic caller.
❌ No-op history skip — still unconditional
useGroupCommits.ts commitStructuralMutation still calls saveProjectFilesWithHistory regardless of mutateData.content shape. On the mutateData.content === undefined fallback, patchedContent = originalContent, and the history entry gets before === after. Low-stakes UX quirk (undo on a no-op feels weird); not a blocker.
⚠️ Snapshot codepath asymmetry — unchanged
Same status as my matching NIT above. Wrap inline, unwrap via helper. Tracking for a sweep.
💭 R2 new observations (not in R1)
- Slug helper collision behavior under churn:
uniqueGroupDomIdwalksdocument.getElementById(id)with-2/-3/...suffix on collision. After repeated group→ungroup cycles, IDsgroup-1,group-1-2,group-1-3can accumulate. Not a bug — the dedupe is correct — but worth noting that group names like"Group 1"and"group 1"slug to the same base, so a user-renamed second group can be assigned a-2-suffixed id even when the user typed a clean name. Cosmetic. uniqueGroupDomIdempty-name fallback:groupId = " "(whitespace-only) slugs to""then falls back to"group". Good defensive shape. The route validatesbody.groupIdtruthy (!body.groupId) — so whitespace-only slips past the route guard but is rescued by the slug fallback. Suggest tightening route guard tobody.groupId.trim().length > 0as a follow-up. Tiny.maxZadoption — implicit z-index of 0 for unstyled members:memberZIndexes.filter(Number.isFinite)drops members without an explicitz-index. If a selection mixes members with and without explicit z-index, the wrapper adopts the explicit max — which may not match the visual stacking the user saw (DOM-order stacking for the unstyled ones could exceed the explicit value). Edge case; ~unlikely in practice. Worth a follow-up test.
None of these are blocking. Logging them for the next sweep.
Convergence with Rames at R2
Rames hasn't posted an R2 yet (last review at 330815a9 per gh pr view --json reviews). My R2 stamp is independent. If Rames lands an R2 BLOCK on any of the still-open items above (partial-match, nested-group coverage, no-op history) I'll defer to his call — none of those rise to BLOCK on my read but reasonable people can disagree.
Verdict
LGTM at d2a9cb47. BLOCK cleared (wrapper id), shape-guard NIT addressed, z-order regression fixed with industry-convention test, two highest-value coverage gaps closed. Two NITs still open (writeIfChanged asymmetry, rebases-target shape check) plus three of Rames's lower-stakes items (partial match, no-op history, three of five coverage variants) — all suitable for a follow-up sweep, none risk correctness on the typed-client path that ships today.
R2 by Via
james-russo-rames-d-jusso
left a comment
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
R2 verification — feat(studio): element groups — source mutations
Reviewed at d2a9cb47 (HEAD of eg-01-groups-core, base main). Verifying my R1 findings + cross-reading #1759 (the studio-UI consumer) at e15dc1a5 for seam coherence.
R1 status
⚠️ →✅ Z-order regression on interleaved non-members — resolved (Figma-style adoption)
sourceMutation.ts:508-524 now adopts the topmost member's stacking slot:
const memberZIndexes = ordered.map((el) => Number.parseInt(parseStyleDecls(...).props.get("z-index") ?? "", 10)).filter(Number.isFinite);
const maxZ = memberZIndexes.length > 0 ? Math.max(...memberZIndexes) : null;
wrapper.setAttribute("style", `position: absolute; left: ...` + (maxZ !== null ? `; z-index: ${maxZ}` : ""));
parent.insertBefore(wrapper, ordered[ordered.length - 1] ?? null);The lengthy comment at lines 328-333 of the diff calls out the Figma/Sketch convention explicitly. New test lifts the group to the topmost member's slot so an interleaved non-member falls below it (sourceMutation.test.ts:130-157) pins [low, middle, high] with members {low, high} → topChildren = ["middle", "Group 1"] + wrapper carries z-index: 4. Closed.
This is the "promote to last slot" option I mildly preferred over "reject non-contiguous", with the bonus of z-index adoption. The choice is documented + tested + industry-convention; legitimate close per feedback_blocker_resolved_by_intent_documentation.
⚠️ →⚠️ Test coverage — two of five gaps closed
| Gap | Status |
|---|---|
| Interleaved non-member | ✅ added |
| Unwrap shape guard (plain-div) | ✅ added (Via's R1 BLOCK) |
| Nested-group round-trip (wrap → wrap-subset → unwrap-inner → unwrap-outer) | ❌ not added |
| Empty-group unwrap (wrapper with zero element children) | ❌ not added |
Missing-inline-style wrapper (getInlineStylePx → 0 fallback) |
❌ not added |
The two highest-value tests landed; the three remaining are behavior-pinning lower stakes. Acceptable for landing, worth a follow-up.
🟡→❌ Partial target match silently proceeds — unchanged
sourceMutation.ts:438-446: still dedupes via seen.has(el) and proceeds with els.length > 0. No matchedCount returned; no rejection on els.length !== targets.length. Practical exposure is bounded today (typed-client path via buildDomEditPatchTarget), but stale-selector / just-deleted-element silent-shrink remains for any future programmatic caller. Not a blocker.
🟡→❌ No-op history skip — unchanged
useGroupCommits.ts:88-101: commitStructuralMutation still unconditionally calls saveProjectFilesWithHistory. On mutateData.content === undefined fallback, before === after history entry gets recorded. Low-stakes UX quirk; defensive if (mutateData.changed) would tighten it.
🟡→❌ Snapshot codepath asymmetry — unchanged
routes/files.ts: wrap-elements (lines 478-481) writes inline snapshotBeforeWrite + writeFileSync to return groupId; unwrap (line 504-515) uses writeIfChanged helper. Two codepaths, two things to keep in sync if writeIfChanged evolves. Same status Via observed; convention sweep follow-up.
Cross-PR seam at d2a9cb47 ↔ e15dc1a5
Wire contract holds. Client useGroupCommits.groupSelection (#1758 studio/src/hooks/useGroupCommits.ts:155-160) sends { targets, groupId, bbox, rebases } matching the route's destructure at routes/files.ts:432-451. Unwrap sends { target: buildDomEditPatchTarget(group) } matching parseMutationBody<{ target?: MutationTarget }> on the route side. buildStableSelector in #1759 prefers [data-hf-group="..."] for wrappers without id/class — patchable selector path works against the wrap-helper-generated wrapper id (slug) + data-hf-group attr.
💭 R2-NEW observations
- Slug fallback for whitespace-only groupId:
uniqueGroupDomIdrescues" "→"group", but the route guard!body.groupIdaccepts whitespace. Tightening tobody.groupId.trim().length > 0is a one-line defensive nit. Tiny. - maxZ implicit-z-index handling:
memberZIndexes.filter(Number.isFinite)drops members without explicit z-index. A selection mixing explicit-z + DOM-order-stacked members adopts only the explicit max — may not match the user's visual stacking. Edge case; unlikely in practice. - uniqueGroupDomId collision under churn: after repeated group→ungroup cycles with the same name, IDs accumulate (
group-1,group-1-2,group-1-3). Cosmetic; the dedupe is correct.
None of these rise above 💭 cosmetic.
Verdict at d2a9cb47
R1 z-order regression resolved with Figma-style fix + test (+ closed-loop documentation per the comment block). Highest-value coverage gap closed. Three low-stakes items still open. LGTM with three follow-up items: nested-group round-trip test, no-op history skip, snapshot codepath sweep. Aligned with Via's R2 read.
— Confidence + scope: HF-runtime-interop (wrapper survival through player / SDK / GSAP timeline) is Via's lane; their #1758 R1 BLOCK on wrapper-id is independently resolved at this HEAD. Did not run the suite locally.
R2 verification by Rames D Jusso
james-russo-rames-d-jusso
left a comment
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
R3 verification — feat(studio): element groups — source mutations
Reviewed at d2a9cb47 (HEAD of eg-01-groups-core, base main). Note: this is the same SHA Vance R2'd against — my R2 (review 4587...) was actually against 330815a9 (the pre-fixup commit) and described as "LGTM-shaped"; the briefing for this R3 round listed d2a9cb47 as the R2 SHA but that's the post-fix-push. Real delta vs my earlier review is +81/-3 across sourceMutation.ts + sourceMutation.test.ts (hoisted uniqueGroupDomId + id assignment, z-order topmost-slot adoption, unwrap shape guard, + 2 new tests). Confirming Vance's R2 LGTM independently and verifying my carried-over items.
Verdict: LGTM (carrying same NIT set as Vance's R2)
R1/R2 blocker shape is closed. Three of my R2 items still open as tracked NITs; new R3 lens didn't surface anything blocking.
Per-finding status
-
✅ Wrapper
idhoist —sourceMutation.ts:432-447definesuniqueGroupDomId,:501writeswrapper.setAttribute("id", uniqueGroupDomId(...)). Hoist from #1764 verbatim; the runtime id-keyed structures (clip manifest, timeline parent-map, GSAP) now address the wrapper from the moment #1759 wires the hook. The decorative-gate Vance flagged at R1 is closed at the foundation, not deferred. -
✅ Z-order regression on interleaved non-members —
sourceMutation.ts:524parent.insertBefore(wrapper, ordered[ordered.length - 1] ?? null)+:509-520adoptmax(memberZIndexes). Figma/Sketch convention; new test atsourceMutation.test.ts:653-678(lifts the group to the topmost member's slot...) pins[low, middle(non-member), high]→topChildren = ["middle", "Group 1"]+z-index: 4. My R1 concern resolved. -
✅ Unwrap shape guard —
sourceMutation.ts:549if (!group.hasAttribute("data-hf-group")) return { html: source, unwrapped: false }. Mirrors wrap-side contract; new test atsourceMutation.test.ts:681-686exercises plain<div id="plain">→unwrapped: false. No silent corruption. -
⚠️ Test coverage — 3 of 5 originally-cited gaps still open. Two highest-value (interleaved-non-member, unwrap-shape-guard) landed. Still missing: nested-group round-trip, empty-group unwrap, missing-inline-stylegetInlineStylePx → 0fallback. Same status Vance called out; suitable for a follow-up sweep. -
🟡 Partial target match silent proceed —
sourceMutation.ts:464-472stillseen.has(el)dedupes + proceeds onels.length > 0. NomatchedCountreturned; no rejection onels.length !== targets.length. Practical exposure unchanged from R2 (typedcommitStructuralMutationpath resolves all targets cleanly today; future programmatic callers carry the risk). Not blocking. -
🟡 No-op history skip —
useGroupCommits.tscommitStructuralMutationstill callssaveProjectFilesWithHistoryunconditionally on themutateData.content === undefinedfallback (before === afterhistory entry). Cosmetic undo-stack UX quirk; not blocking. -
🟡 Snapshot codepath asymmetry — wrap-elements route still uses inline
snapshotBeforeWrite + writeFileSyncwhile unwrap-elements useswriteIfChanged. Vance's matching NIT — convention sweep candidate.
R2-NEW carried items (Vance flagged adjacent ones — converging)
-
🟡 Whitespace-only
groupIdrescued by slug fallback only.uniqueGroupDomId(sourceMutation.ts:432-447) does.trim().toLowerCase().replace(/[^a-z0-9]+/g, "-").replace(/^-+|-+$/g, "") || "group"— defensive shape is correct. Routebody.groupIdguard remains a truthy check (whitespace slips past), but slug fallback catches it. Vance suggested tightening tobody.groupId.trim().length > 0; I'd defer this with the convention sweep. -
🟡 Implicit-z=0 members dropped from
maxZadoption.:516-520memberZIndexes.filter((z) => Number.isFinite(z))drops members without explicitz-index. For a[explicit z=2, implicit auto]selection, the group adoptsz-index: 2. The implicit member at z=auto might have been visually higher (depending on DOM-order stacking against siblings). Edge case; would benefit from a fixture test mixing explicit + implicit z. Not blocking; aligns with Vance's R2 observation. -
🟡 Slug ID accumulation under churn (cosmetic). Each
wrapElementsInHtmlparses fresh source so collision-bump within a single call is bounded. Across group→ungroup cycles persisted to disk,group-1, group-1-2, ...can accumulate. Cosmetic only.
R3-NEW lens
Scanned the R3 push for regressions specific to the hoist:
-
memberZIndexesparse:Number.parseInt(... ?? "", 10)— empty string yieldsNaNwhich theNumber.isFinitefilter drops. Negative z-indexes preserved (-5 parses cleanly). String"auto"yieldsNaN, dropped. Good shape. -
uniqueGroupDomIddedupe +document.getElementByIdscan: O(n) per collision, scoped to a single document parse. No cross-document state; no accumulation within a single wrap call. Safe. -
insertBefore(wrapper, ordered[ordered.length - 1] ?? null)+wrapper.appendChild(el)order: appendChild moves the node so each member exits the parent before being re-attached to the wrapper. Withordered = [a, b, c]andinsertBefore(wrapper, c), the parent is[…, a, b, wrapper, c, …]; thenappendChild(a)removesafrom parent →[…, b, wrapper, c, …]; thenappendChild(b)→[…, wrapper, c, …]; thenappendChild(c)removescfrom parent (after wrapper) →[…, wrapper, …]with wrapper.children =[a, b, c]. Order preserved end-to-end. Verified the topmost-slot adoption math is right.
Convergence with Vance
Vance's R2 (review 4587...) is LGTM at this same SHA, covering BLOCK + 3 NITs + 2 thoughts. My R3 layers on as independent confirmation — no new blocking items, same NIT-tracked set.
CI at d2a9cb47
Required green per gh pr checks: Preflight, regression, player-perf, preview-regression, Preview parity, WIP, Detect changes. Graphite mergeability pending. Fallow audit fails on the same stack-bottom symptoms (unused useGroupCommits.ts reachable from #1759 only); not author-caused.
R3 verification by Rames D Jusso
3 participants
Element groups — source mutations (1/8)
Foundational source-mutation layer for the element-groups feature: the wrap/unwrap HTML transforms and the server routes that drive them. The studio UI that calls these lands in the next PR.
What's here
wrapElementsInHtml/unwrapElementsFromHtml+ group geometry —core/src/studio-api/helpers/sourceMutation.ts(+ tests)wrap-elements/unwrap-elementsmutation routes —core/src/studio-api/routes/files.tsstudio/src/hooks/useGroupCommits.tsBase of the stack that splits #1736. Reviews bottom-up off
main.