feat(studio): canvas context menu and z-order actions (unwired)#2202
Conversation
9d0fa02 to
78c55d1
Compare
3c08ade to
ae82816
Compare
78c55d1 to
ad7cd13
Compare
miga-heygen
left a comment
There was a problem hiding this comment.
Reviewed as part of the 25-PR NLE overhaul stack. Full stack review on #2205 (the keystone). No blockers on this PR. — Miga
vanceingalls
left a comment
There was a problem hiding this comment.
Verdict: LGTM (green) — pure math, unwired, correctly gated
A textbook z-order resolver: parse effective z, sort into render order (z asc, DOM asc), move the target one step or to an end, realize with the minimal patch (single between-value on the distinct-z fast path; renumber to 0..n-1 on the ties path). Every dispatch branch is a pure function of the inputs.
The pointed queries
canvasContextMenuZOrderis a pure function.packages/studio/src/components/editor/canvasContextMenuZOrder.ts:228–:268—resolveZOrderChange(target, action)performs no side effect: it reads the DOM (viagetFamily/readEffectiveZIndex/getBoundingClientRect) and returns a patch list. Actual style mutation is the caller's responsibility. Every helper (:45, :52, :98, :156, :192) is a query, not a command. Test-friendly by design.- Unwired at merge point. Verified against the tree at head SHA
ad7cd1:DomEditOverlay.tsxat this ref does NOT importCanvasContextMenu,canvasContextMenuZOrder, orresolveZOrderChange(grep clean). The.fallowrc.jsoncat :53–:55 also honestly marksCanvasContextMenu.tsxasTEMP(studio-dnd): shipped unwired ahead of the NLE integration; the app-shell swap PR (studio-dnd/pr22) wires the consumers and removes this block.The unwired assertion holds. Batch-C reviewer will separately verify the assertion survives its post-batch state; my scope is the math itself. - Render-order sort is deterministic. :156 —
(a.zIndex - b.zIndex) || (a.domIndex - b.domIndex). Both keys stable functions of the input; no mutated-output tie-break (avoiding the same oscillation trap#2199fixed). Test at :242–:258 pins the tie-break for a three-way z=0 case. - DOM-order ties handled correctly. The comment at :13–:26 is precise about CSS paint reality (later-in-DOM paints on top at equal z) and the resolver at :78–:82 (
paintsAbove) and :156 respects it. The tie-repro tests at :196–:240 cover all four actions × {earlier-in-DOM, later-in-DOM} — every ambiguous case has a deterministic outcome. - Non-painting sibling hygiene. :85 excludes
<audio>/<script>/<style>/<link>/<meta>from the family; test at :263–:307 pins the invariant using an audio sibling with a leaked z=2 that would otherwise pad the renumber. Nice defensive touch — the qa-clean audio artifact would otherwise skew results. - Cross-realm HTMLElement check. :72–:76 uses
node.ownerDocument?.defaultViewto compare against the element's own realm'sHTMLElement, with anodeType === 1fallback. Documented at :64–:71 as the fix for iframe-realm silent-empty-family bug. Good. - z clamped ≥ 0. :174 (
Math.max(0, above.zIndex - 1)) and the renumber path :215–:218 (istarts at 0) both keep the invariant.renumber emits patches only for elements whose z actually changesat :242–:258 confirms.
Nits (non-blocking)
readEffectiveZIndexat :52 is exported but never consumed at this SHA — correctly exempted via.fallowrc.jsoncignoreExportsat :90–:99 with a TEMP marker. Would be removed by studio-dnd/pr22. Fine.
R1 by Via
ae82816 to
bd0c221
Compare
ad7cd13 to
97977b2
Compare
bd0c221 to
b372711
Compare
97977b2 to
143e573
Compare
143e573 to
a087215
Compare
b372711 to
267d84c
Compare
802df15 to
4d17f3c
Compare
34fba38 to
39720e7
Compare
4d17f3c to
138ede6
Compare
39720e7 to
04a063c
Compare
miguel-heygen
left a comment
There was a problem hiding this comment.
Current required CI is not clean: preview-regression is failing at head 138ede6. Please rerun/fix and re-request review once green.
04a063c to
0afc5ec
Compare
138ede6 to
0d1397b
Compare
What: CanvasContextMenu (right-click menu for canvas selections) and canvasContextMenuZOrder (tie-aware bring-forward/send-backward z-order patch computation) with its test suite. Shipped unwired. Why: the z-order rules are the substance; mounting is one line in the later overlay swap. How: new files, compiled against current main. Nothing mounts the menu yet, so .fallowrc.jsonc gains TEMP(studio-dnd) entries (entry registration + ignoreExports) — removed by the app-shell swap PR that wires everything. Test plan: bunx vitest run canvasContextMenuZOrder.test.ts; tsc --noEmit; fallow audit clean.
0afc5ec to
74d5592
Compare
0d1397b to
8229961
Compare
miguel-heygen
left a comment
There was a problem hiding this comment.
Re-reviewed current head and verified the addressed feedback and required behavior. Approval is for this exact head; Graphite mergeability may still be pending.
Superseded by current-head re-review; findings verified addressed.
711859b
into
studio-dnd/pr10-gesture-characterization-tests

What
CanvasContextMenu (right-click menu for canvas selections) and canvasContextMenuZOrder (tie-aware bring-forward/send-backward z-order patch computation) with its test suite. Shipped unwired.
Why
the z-order rules are the substance; mounting is one line in the later overlay swap.
How
new files, compiled against current main. Nothing mounts the menu yet, so .fallowrc.jsonc gains TEMP(studio-dnd) entries (entry registration + ignoreExports) — removed by the app-shell swap PR that wires everything.
Test plan
bunx vitest run canvasContextMenuZOrder.test.ts; tsc --noEmit; fallow audit clean.
Stack position 11/25 — studio NLE overhaul (CapCut-parity timeline + canvas). Graphite manages bases; merge from #2192 upward. Temporary
TEMP(studio-dnd)fallow entries (unwired-component windows) are all removed by #2213 (app-shell swap), whose tree is byte-identical to the fully-verified integration branch.