feat(studio): draggable layer reorder with z-index persistence#1216
Merged
Conversation
…tence Layers panel now sorts siblings by computed z-index (descending) to reflect visual stacking order. Users can drag layer rows to reorder them within a sibling group — on drop, sequential z-index values are assigned and persisted via the existing inline-style patch pipeline with a single preview reload. - sortLayersByZIndex: recursive sibling-group sort by computed z-index - useLayerDrag: pointer-capture drag gesture with 4px threshold, insertion indicator line, and depth-constrained sibling reorder - handleDomZIndexReorderCommit: batch z-index commit with coalesced undo entry and single skipRefresh=false on the final patch
miguel-heygen
force-pushed
the
feat/layers-panel-drag-reorder
branch
from
1c7ef5e to
581ba85
Compare
- Guard drag initiation against locked compositions by checking data-timeline-locked ancestors in isLayerDraggable - Show not-allowed cursor and reduced opacity on non-draggable layer rows - Fire toast when attempting to drag a layer with no same-depth siblings - Preserve z-index spacing on reorder by redistributing existing values instead of flattening to sequential integers - Auto-set position:relative on unpositioned elements when z-index is applied so the stacking order actually takes visual effect - Add tests for isLayerDraggable (anonymous, id, selector, locked, free)
Fallow audit reportFound 17 findings. Duplication (11)
Health (6)
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.
Summary: drag-to-reorder on the Layers panel. sortLayersByZIndex flips sibling chunks to descending computed z-index; useLayerDrag runs a pointer-capture gesture with a 4px threshold and an insertion-line indicator; handleDomZIndexReorderCommit writes the new z-index values onto siblings as inline styles, auto-promotes position: static → relative where needed, and batches the writes through commitPositionPatchToHtml with a shared coalesceKey so the undo stack treats the whole reorder as one entry. PR body is precise and the test plan is the right shape.
One real bug in the reorder math (concern #1), one layout-side-effect to think about (concern #2), CI is red on two checks, and a few smaller things. No blockers — concern #1 is fixable cheaply and would normally be a blocker, but since it silently reverts to status quo on reload (no data loss, no visible regression to layers that aren't z-tied), I'm tagging it as a concern with a sharp fix path rather than a blocker. Your call.
Concerns
- Tied z-index reorders silently persist nothing (
LayersPanel.tsx:194–218). ThehandleReordermath handles theallSamecase but falls through on partial ties. Walk-through: siblings with computed z[3, 2, 2, 1], user drags the second2above the first2.existingValues = [3, 2, 2, 1]→sorted = [3, 2, 2, 1]→allSame = false→zValues = sorted = [3, 2, 2, 1]. The reordered positions get assigned[3, 2, 2, 1]again — both tied elements stay atz=2. CSS resolves ties by DOM order (unchanged by this PR), so paint order is identical to before; and on next iframe reloadsortLayersByZIndex's tiebreaker (b.domIndex - a.domIndex) restores the original panel order, so the drag visually vanishes too. Fix: when any duplicates exist insorted(not just all-same), generate fresh sequential ints (reordered.map((_, i) => reordered.length - i)), or interleave new ints between non-tied neighbors so only the tied subrange churns. A single unit test ([2, 1, 2]drag last to front → assert resulting z-values give a stable strict order) would have caught it; same test would lock the chosen fix. position: static→position: relativeauto-promote is a hidden layout-context change (useDomEditCommits.ts:551–556). Necessary for z-index to take effect on static elements — totally pragmatic — butrelativealso makes the element a containing block for absolute descendants. If a static element hasposition: absolutechildren that were positioning relative to a further-up ancestor, this PR silently captures them on first reorder. Most studio elements probably aren'tstatic(the panel-edit defaults push everything torelative/absolute), but worth either gating the promote onelement.querySelector(":scope > [style*='position: absolute'], :scope > [style*='position:absolute']")returning null, or just adding a Test-plan line for "static element with abs children, reorder, abs children stay where they were."- Two CI checks red:
- File size check —
useDomEditSession.tsis 602 / 600 lines (the newhandleDomZIndexReorderCommitdestructure plus theuseGsapSelectionHandlersimport nudges it over). The extraction of GSAP handlers intouseGsapSelectionHandlers.tsis a great refactor and almost gets you there; another small extraction (e.g. pulling thehandleGsapAware*adapters into their own hook) closes the gap without touching the bot of the new feature. - Fallow audit — the bot comment lists the new findings: a couple of test-file dup groups (low-stakes) and a couple of new high-CRAP-score callouts on
LayersPanel.tsx(seekToLayer, the row-render arrow). Mostly minor; theseekToLayercyclomatic 12 is the result of the matched-id-fallback walk that's been carried forward unchanged. Worth either suppressing or splitting the row render's inline className ternary mess.
- File size check —
- Visual feedback during cross-depth drag is misleading. Depth-constrained reorder is the right call (PR body item: "Cross-depth drag (child to uncle level) should be prevented"), but the implementation just leaves the insertion line anchored to the original sibling band even when the pointer is hovering over a different parent group entirely. Users will read that as "the drag is broken." Either swap the cursor to
not-allowedonceclientYfalls outside[siblingRects[0].top, last.bottom], or fade the indicator. Cheap UX win for a feature that's hard to teach otherwise.
Nits
- No keyboard-driven reorder. The row already has
role="button"+tabIndex={0}+ Enter/Space for select. AnArrowUp/ArrowDownwith a modifier (Option-Up / Option-Down would mirror Figma) that callshandleReorder({ fromIndex, toIndex })directly would close the a11y gap on drag-and-drop, which is famously the worst pattern for AT users. The reorder math is already factored out — wiring it would be ~10 lines. (nit) - Touch / pen path not in the test plan. Pointer events make it work for free in theory; in practice the 4px threshold + scroll-container's
touch-action(which I didn't see set on the scroll container) often conflicts with vertical-scroll gestures on tablet. Worth a quick iPad pass given Studio occasionally gets used there, and considertouch-action: pan-yon the row so a touch-drag doesn't accidentally scroll. (nit) - Optimistic DOM write before patch confirms (
useDomEditCommits.ts:547).entry.element.style.zIndex = ...runs beforecommitPositionPatchToHtmlreturns. If the patch fails (server 500, file-read race, etc.) the iframe DOM is one step ahead of source. The reorder will revert on next preview reload, but mid-session the user sees stale state. Error is toasted, so not silent — minor. (nit) - Test coverage gap on the reorder math.
sortLayersByZIndexandisLayerDraggableare nicely covered. ThehandleReordermath (the location of concern #1 above) is untested;findSiblingIndices,computeInsertionPos,computeInsertionLineYare also pure functions ripe for a few cases each. Pure-function units would cover ~80% of the bug surface here. (nit) findSiblingIndices(useLayerDrag.ts:48–68) could simplify the start-walk tofor (let i = layerIndex - 1; i >= 0; i--) if (visibleLayers[i].depth < depth) { start = i + 1; break; }— same logic, nostart--; check; start++; breakshuffle. (style nit)useDomEditSession.tsrefactor is bundled into a feature PR. The extraction of GSAP handlers intouseGsapSelectionHandlersis clean and orthogonal to drag-reorder. Splitting that into its own PR ahead of this one would makegit blameon the feature work cleaner, and would isolate the file-size-check fix. Not a blocker; just a "next time" thought. (nit)isLayerDraggable's ancestor walk usesparentElement, which stops cleanly at the iframe document root. Worth a one-line comment noting that's deliberate (so a future reader doesn't "fix" it by walkingparentNodeand accidentally reach into the host page). (nit)getElementZIndexswallows everything insidetry/catchand falls back to0. The0collapses to "auto / no-z" semantics, but ifgetComputedStylethrows because the element is detached (which is the case on cross-iframe gotchas), you can't distinguish "deliberately 0" from "couldn't read." Doesn't matter for sorting today; worth anullreturn + caller fallback if the codepath ever feeds something stricter. (nit)
Questions
- Was the auto-promote from
static→relativediscussed with the team? It's a sensible default for the z-index reorder to take effect, but it's a layout-context change. If the answer is "yes, all studio-managed elements are non-static already and this is just a safety net," consider a debugconsole.warnso the case is visible if it ever fires. - The tied-z behaviour (concern #1) — intentional ("don't churn already-tied z-indices, let DOM order decide") or oversight? If intentional, lock it with a test + a comment explaining the no-op semantics so the next person debugging "my drag didn't stick" has a pointer.
- Multi-select reorder — explicitly out of scope for this PR, or coming next? The current selection model is single, so this is a future-question; if multi-drag is on the roadmap, the
entriesshape inhandleDomZIndexReorderCommitis already roughly the right primitive to extend. - Coalesce-key shape
z-reorder:${entries.map((e) => e.id ?? e.selector ?? "el").join(":")}— for two rapid-fire reorders on the same sibling group, this collapses into a single undo entry even if the user did them as two distinct intents. Is the coalesce window time-bounded somewhere upstream, or is it purely key-based? (If purely key-based, rapid double-drags could be unundo-able as separate steps.)
What I didn't verify
- The
commitPositionPatchToHtml→editHistory.recordEditpipeline's exact coalesce semantics — trusted the PR body's "coalesced undo entry per reorder" claim. - The
setPointerCapturerelease onpointercancel(e.g., OS gesture interruption, iframe reload mid-drag). Thetry/catcharoundreleasePointerCaptureis defensively shaped. - Whether the iframe
loadevent firing mid-drag (HMR, post-commit reload, etc.) cleanly cancels the gesture. ThedragRefis only cleared onpointerup/pointercancel. - Live behaviour with
data-timeline-lockedancestors inside nested compositions — only the direct-ancestor walk was traced. - Touch / pen behaviour on tablet (no setup).
- Manual test-plan items all unchecked — assumed Miguel will run them locally pre-merge; not blocking on it.
- The
useGsapSelectionHandlersextraction inuseDomEditSession.ts— read the call-site only; assumed mechanical hoist.
— Review by Rames D Jusso
- Fall back to sequential z-index when any duplicates exist in the sibling set, not just when all values are identical — fixes silent no-op reorder when tied values preserve DOM-order stacking - Trim useDomEditSession.ts from 602 to 600 lines (CI file-size gate)
james-russo-rames-d-jusso
left a comment
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
Round 2 follow-up on a8be5baf.
The headline bug is closed cleanly:
- Tied-z fix (
LayersPanel.tsx:200–206) —allSame→hasDupescheck across the sorted array. Any adjacent duplicate triggers full sequential renumbering (reordered.map((_, i) => reordered.length - i)); strictly-monotonic z sets keep the existing-values reuse path (so reorders inside[10, 5, 1]still preserve the value structure). Walked the cases:[3, 2, 2, 1]→hasDupes=true→zValues=[4, 3, 2, 1](formerly-tied pair gets distinct values, drag persists) ✓[3, 3, 3, 3]→hasDupes=true→zValues=[4, 3, 2, 1](matches the old all-same branch) ✓[3, 2, 1]→hasDupes=false→zValues=[3, 2, 1](preserves the user's gap structure) ✓[5, 5]→hasDupes=true→zValues=[2, 1](two-element tie is broken) ✓
- File size trim (
useDomEditSession.ts:538–544) — removed two blank lines aroundRefs/Callbackscomment dividers, brings the file from 602 → 600. Pragmatic, gets CI green; longer-term you'll probably want to extract another small chunk if that file keeps growing. Not blocking.
One tiny follow-up that would normally be a "do it": the tied-z fix is the kind of thing a [2, 1, 2] → drag last to front unit test (against handleReorder or a hoisted pure helper) would lock against future regressions. The PR doesn't add one. Not a blocker — the visual inspection is straightforward and the existing sortLayersByZIndex suite would catch the symptom on next reload — but if this code gets touched again, the next reader has nothing pinning the semantics. Worth ~5 lines of test.
Round-1 items intentionally deferred (no objection):
position: static→relativeauto-promote — not touched, fine; if it ever bites in practice we can scope-gate later. The CSS-y "I want z-index to apply" reading is correct.- Cross-depth-drag indicator UX — also not touched, fine for v1.
- Keyboard-driven reorder, touch test plan, optimistic-DOM ordering, the smaller nits — all carry forward as future polish.
CI is mid-flight at review time; File size check is already SUCCESS (the round-1 red), Fallow audit is re-running on a8be5baf. Assuming both land green, looks good to me.
— Review by Rames D Jusso
Cover the [2, 1, 2] case where tied z-index values fall back to reverse DOM order — locks the hasDupes fix against regressions.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Changes
sortLayersByZIndex: recursive sibling-group sort by computed z-index with DOM-order tiebreakeruseLayerDrag: pointer-capture drag gesture with 4px threshold, insertion indicator line, depth-constrained sibling reorderhandleDomZIndexReorderCommit: batch z-index commit throughcommitPositionPatchToHtmlwith shared coalesceKeyTest plan