feat(studio): non-destructive crop + cross-project asset view#1971
feat(studio): non-destructive crop + cross-project asset view#1971miguel-heygen wants to merge 1 commit into
Conversation
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
miguel-heygen
force-pushed
the
feat/media-use-02-cli
branch
from
1ea25cd to
4d696ed
Compare
miguel-heygen
force-pushed
the
feat/media-use-03-studio
branch
from
0d8ee64 to
26c3cc9
Compare
miguel-heygen
force-pushed
the
feat/media-use-03-studio
branch
from
26c3cc9 to
cd08335
Compare
vanceingalls
left a comment
vanceingalls
left a comment
There was a problem hiding this comment.
Verdict: LGTM — no blockers, one small nit worth a look before merge.
The scene, as I read it at cd08335d. A non-destructive crop is introduced whose persistence is nothing more elaborate than a clip-path: inset(a b c d [round r]) inline style — the very grammar CSS already renders and that the existing HTML style-patcher already round-trips (confirmed by the new fixture in packages/studio-server/src/helpers/sourceMutation.test.ts:60). No data-crop-* attribute, no bespoke renderer contract, no schema change: the compositor does the clipping natively at composite time, so pixel accuracy is inherited for free. That answers, and quietly closes, the first two runtime-interop worries I brought to the case.
Alongside it, a cross-project asset browser lands at packages/studio-server/src/routes/globalAssets.ts:22. It reads only ~/.media/manifest.jsonl, accepts no path parameters, and — the crucial detail — toPublicAsset at packages/studio-server/src/routes/globalAssets.ts:38 explicitly strips cached_path before the record ever crosses the network boundary, with an accompanying regression test at packages/studio-server/src/routes/globalAssets.test.ts:52. Path-traversal is a non-starter: there is no user input, only a fixed home-relative manifest read.
Finding-by-finding
F1 — Crop persistence contract. ✅ Verified. DomEditCropHandles.finishCropGesture at packages/studio/src/components/editor/DomEditCropHandles.tsx:144 routes through the standard onStyleCommit("clip-path", buildInsetClipPathSides(...)) — the same one every other style edit uses. buildInsetClipPathSides at packages/studio/src/components/editor/clipPathHelpers.ts:98 emits shorthand-collapsed inset() values that patchElementInHtml already understands. Zero new persistence surface; the renderer, being the browser's own compositor reading a native CSS property, requires no work at all. The claim that crop is non-destructive holds up: clip-path masks, it does not resample.
F2 — canCrop reachability across every capability leg. ✅ Verified. New field on DomEditCapabilities at packages/core/src/editing/affordances.ts:12. I walked all four return paths in resolveCapabilities (lines 109, 125, 139, 177) — every branch sets canCrop explicitly, and the tests at packages/core/src/editing/affordances.test.ts:50-69 pin the four interesting cohorts (root=false, sub-comp-host-in-master=true, script-generated=false, locked-comp=false). No dead code, no phantom branch. The useCropOverlay availability publisher (packages/studio/src/hooks/useCropMode.ts:57) ANDs on selection.capabilities.canCrop, and the store setter (packages/studio/src/player/store/playerStore.ts:259) atomically forces cropMode: false whenever availability flips off — no way to strand the UI in an armed-but-uncroppable state.
F3 — Crop math and edge clamping. ✅ Verified. resolveCropInsetFromEdgeDrag (packages/studio/src/components/editor/domEditOverlayCrop.ts:59) correctly divides deltas by scaleX/scaleY to convert overlay pixels to element pixels, and clampInset at line 54 prevents opposing sides from crossing. resolveCropInsetFromMoveDrag at line 91 keeps left+right and top+bottom totals invariant so the crop window preserves size while sliding — tested at packages/studio/src/components/editor/domEditOverlayCrop.test.ts:96, including the extreme-drag clamp case. Video-natural-size versus display-size doesn't arise here because insets are stored in authored element pixels, not video pixels — the compositor scales them alongside the element. That's the right choice.
F4 — Overlay rewrite regressions. ✅ No regressions surfaced. DomEditOverlay.tsx went from 108 lines of overlay logic to 105 lines of slightly different overlay logic; the "rewrite" reading of the +108/-105 is misleading because most is import-list churn and prop threading. Non-cropped elements skip the crop-outline child (packages/studio/src/components/editor/DomEditOverlay.tsx:761 — {cropOutlineInsetPx && (…)}), so their chrome remains the plain-border path (resolveBoxChromeClass third branch at packages/studio/src/components/editor/domEditOverlayShape.ts:36). Event-handler cleanup: DomEditCropHandles at DomEditCropHandles.tsx:92 restores the committed clip-path on unmount via effect cleanup, and useCropOverlay at useCropMode.ts:35 removes its keydown listener symmetrically. The useEffect(() => { if (!cropMode) bumpAfterExit(); }, [cropMode]) at useCropMode.ts:52 does pattern-match the "useEffect-for-state-syncing" anti-pattern from the shared rubric, but it is doing something React cannot express otherwise — forcing a re-render one frame after a sibling component's cleanup mutates the DOM style — so the escape hatch is warranted; a comment already justifies it.
F5 — Persistence race on rapid release + save. ✅ Verified safe. finishCropGesture at DomEditCropHandles.tsx:154 awaits the onStyleCommit promise chain before it re-lifts the clip, and onStyleCommit is the same synchronous-write path the rest of the panel uses — no debounce layer sits in front of a keyboard save, so a Ctrl+S the instant after pointerup queues after the commit, not around it. Cancel path (cancelCropGesture at line 162) reverts insets to gesture.startInsets without touching the source, which is what you want.
F6 — Cross-PR pull from #1972 media-use skill. ✅ None. As predicted — studio UI does not import from the media-use skill. GlobalAssetsView fetches over HTTP; the server side reads its own manifest file. Cleanly decoupled.
F7 — Small nit: hover-box legibility over a hard-clipped element. DomEditOverlay.tsx:662 now applies hugRectForElement to the hover ring, which is correct for cropped elements — the ring hugs the visible sliver rather than the invisible bounds. Fine. But when insets fully collapse (width===0 or height===0, which the crop-math clamp explicitly permits at domEditOverlayCrop.ts:27 with Math.max(0, …)), the hover ring degenerates to a 0×0 div and disappears. Not a correctness bug — the element is invisible anyway — but a user who cropped to zero cannot un-hover-select their own element. Consider a floor of ~4px or falling back to the raw bounds when the hugged rect goes degenerate. Author call.
Verdict, restated
The persistence surface is smaller than the PR title suggests — clip-path: inset(...) is native CSS and the existing patcher already handles it, so there is nothing new for the producer or the renderer to learn. The capability plumbing is complete, the math is unit-tested including the clamp corners, the server route is read-only over a fixed path with the sensitive field stripped and a regression test to keep it stripped, and the overlay rewrite is more accurately described as an extension — the non-cropped codepath is untouched. Ship it. F7 is a suggestion; do not gate on it.
Review by Via (runtime-interop lens)
2 participants
Non-destructive visual crop (persists as
clip-path: inset(), one undo/step, media file untouched); every overlay surface (selection, hover, snap, marquee, handles) respects the crop. Plus the cross-project asset view (studio-server globalAssets).Stack: media-use v2 (3/5).
📚 Stack — media-use v2 (split from #1923, merge bottom-up)