feat(studio): drag assets from the sidebar onto the timeline#464
feat(studio): drag assets from the sidebar onto the timeline#464miguel-heygen merged 7 commits intomainfrom
Conversation
jrusso1020
left a comment
There was a problem hiding this comment.
Verdict: approve on code merit — wait on regression-shards before merge
Clean architecture. The split between pure helpers (timelineAssetDrop.ts, sourceMutation.ts) and integration (App.tsx, Timeline.tsx) is the right shape, and the test coverage targets exactly the helpers that define the critical invariants — getTimelineAssetKind / buildTimelineAssetId / resolveTimelineAssetSrc / buildTimelineAssetInsertHtml / insertTimelineAssetIntoSource on the asset-drop side, shouldHandleTimelineDeleteKey / getDefaultDroppedTrack / resolveTimelineAssetDrop on the timeline interaction side, and removeElementFromHtml on the server-side mutation side.
What this closes from the Loom feedback
hf#463 explicitly deferred Jake's #2 (scene expansion) and #3 (drag-drop imports). This PR ships #3 cleanly + adds Delete/Backspace timeline clip removal (a natural companion that falls out of the same source-mutation plumbing). That's a tight, complete follow-up.
Architecture wins worth calling out
- Server-side DOM mutation via LinkeDOM (
removeElementFromHtml) — right call over client-side string surgery. Handles full-doc HTML vs fragment correctly (parseSourceDocumentsniffs<!doctype|<htmland wraps fragments into a synthetic document, then returnsbody.innerHTMLon fragments ordocument.toString()on full docs). No-op semantics when the target isn't found (returns source unchanged) is the right defensive shape. - Custom MIME type (
TIMELINE_ASSET_MIME = "application/x-hyperframes-asset") on the asset drag payload prevents collision with generic file drops.Timeline.handleAssetDragOvercorrectly short-circuits on neither-files-nor-asset, then branchesdropEffect = "copy"only on asset. - Delete in-flight guard (
deleteInFlightRef) prevents rapid-fire double-delete.suppressClickRef = trueafter delete prevents a spurious click from reselecting the deleted-now-nonexistent clip. resolveTimelineAssetSrchandles sub-composition path rewriting (asset dropped incompositions/scene-a.htmlgets path rewritten to../assets/photo.png) — one of those things that silently breaks authoring if wrong.- z-index rewriting on drop and delete keeps the layer ordering monotonic per-track without holes. O(n-per-track) work per mutation is fine at typical composition sizes.
Non-blocking notes
- Regex-based
collectHtmlIds.\bid="([^"]+)"catchesid="..."on elements, but it also catchesdata-id="..."/aria-describedby-id="..."/ any attribute whose name ends inid. That meansbuildTimelineAssetId's dedupe may treat a non-id attribute value as an "existing id" and suffix unnecessarily. Defensive-wrong (adds an extra_2suffix that wouldn't have been needed) rather than correctness-wrong. A tiny tightening to\s+id="([^"]+)"or(?:^|\s)id="([^"]+)"would avoid the false-positive class. Minor. - Delete-key
windowlistener composes with other global handlers.event.preventDefault()stops the browser's default (Backspace→navigate-back) but doesn'tstopPropagation(), so if another window-level keydown listener exists (file-tree row, command palette, etc.) it still fires. Consider addingevent.stopPropagation()afterpreventDefault()once you confirm no downstream handler actually wants to chain. Not blocking since theshouldHandleTimelineDeleteKeyinput/contenteditable guard handles the most common cases. resolveDroppedAssetDuration3-second timeout. Reasonable default. On slow network or cold disk, an honest probe could take longer than 3s and fall back to the 5-second video default, producing a clip that doesn't match source duration. User can trim afterwards; the current default is a sensible UX tradeoff.insertTimelineAssetIntoSourcethrows if nodata-composition-idroot — good error but an unguarded throw could surface as an unhandled rejection inhandleTimelineAssetDropwhich would only land in theshowToastcatch if wrapped. Traced the call site; it IS wrapped in the try/catch, so toast-message path handles it. ✓
Browser-test ask — same capability gap as #424/#463
My session doesn't have Playwright/Puppeteer wired in, so I can't run the interactive drag-drop or Delete-key verification. Your Loom screenshot + webm capture is what we have; the pure-helper test coverage + CI green on everything non-regression-shard is strong code-merit signal. For the final pass (especially the z-index-rewrite under multi-clip tracks, and the path-rewrite under nested sub-compositions), a manual human-at-browser check is still the right gate.
CI state
Format / Typecheck / Lint / Test / Build / Test: runtime contract / Smoke: global install / Tests on windows-latest / Render on windows-latest / CodeQL / player-perf / Perf (fps/scrub/drift/parity/load) — all green. Regression-shards (8 of 10) still in-flight at review time — hold merge until those complete.
No blockers. Ship once regression is green.
Review by hyperframes
jrusso1020
left a comment
There was a problem hiding this comment.
Follow-up on the 4 new commits since initial approval
Took another pass over 82a70a0 (external drops), 461ed9a (media-validation), 67fceaf (vite body-bridge), and 9754c3b (live child-timeline duration). Verdict: all four are clean, well-scoped, and each is clearly motivated — approval from the prior review carries through on my end. Holding final merge on regression-shards completing.
Per-commit notes
9754c3b — live child timelines for runtime visibility (the regression fix)
Nicely targeted. Parameterizing includeAuthoredTimingAttrs on resolveDurationForElement and flipping it to false at the composition-host visibility site is the minimum-blast-radius shape. Everywhere else (data-composition-id root, captions, etc.) keeps trusting data-hf-authored-duration; only the visibility-cutoff path now prefers the live child timeline's .duration(). The test covers the exact invariant that broke: authored=14, live=8, renderSeek(9) → hidden. ✓
Subtle thing worth being aware of: if a child's registered timeline happens to be mis-sized (e.g. a paused tl with duration 0 because tweens haven't been added yet), visibility will cut earlier than the author expected. The current resolver.resolveDurationForElement implementation presumably handles null/0 via the authored fallback, but a line of comment over this call site noting "we intentionally prefer live over authored here because authored lies when sub-compositions shrink dynamically" would save a future reader the five-minute archaeology. Not blocking.
67fceaf — preserve binary request bodies in the Vite bridge
Correct fix for the right reason. chunk.toString() was silently doing UTF-8 decoding of multipart boundaries mid-binary-payload; Buffer.concat preserves bytes. The readNodeRequestBody helper's typeof chunk === "string" branch is defensive paranoia (Node IncomingMessage yields Buffers unless .setEncoding() is called), but it's harmless and future-proofs against a transform-stream wrapper. Test covers both the binary-preservation and empty-body paths. ✓
One small thing: body = bytes.byteLength > 0 ? bytes : undefined; in vite.config.ts collapses "empty body" to undefined before handing to new Request(...). That matches the old string-based behavior (empty string → undefined) so no behavior change, but if any downstream route expected an empty-but-present body (e.g. a POST with Content-Length: 0 that was still meaningful), this would swallow it. In practice, multipart uploads and JSON POSTs always have bytes, so the tradeoff is fine.
461ed9a — reject invalid media uploads
Good split: validateUploadedMedia (path-based) + validateUploadedMediaBuffer (buffer-based with a tempfile) — the buffer variant's basename(fileName) safely strips path traversal, and the /tmp/hyperframes-upload-* temp dir is cleaned up in finally. The ENOENT-on-ffprobe early-return-OK is the right call for dev boxes without ffmpeg; production should have ffmpeg available and will get the real stream check.
The surfaced-to-UI error text ("Unsupported media skipped: ...") in App.tsx's upload handler matches the PR-description wording. ✓
One nit: on an ffprobe status-nonzero failure you return "ffprobe failed to read the media file" without including the stderr or exit code. For a user who's dropped an unsupported codec (e.g., ProRes RAW), the toast says "ffprobe failed" and they're left guessing. Not blocking for this PR — later we can pass the first line of result.stderr into the reason string.
82a70a0 — external file drops onto timeline
Reasonable shape. buildTimelineFileDropPlacements (pure, testable) plus uploadProjectFiles + resolveDroppedAssetDuration composed in App.tsx. The 3-second metadata-probe timeout keeps the drop interaction snappy; the fallback to DEFAULT_TIMELINE_ASSET_DURATION[kind] on timeout / error / non-finite duration is sensible UX.
One concern worth calling out (carry-over from my initial review, still stands on the new code):
collectHtmlIdsregex\bid="([^"]+)"is the same false-positive source. It still catchesdata-id,aria-describedby-id, etc. It's used inbuildTimelineAssetId's dedupe, so the worst case is defensive-wrong (an unnecessary_2suffix), not correctness-wrong. A tighten to(?:^|\s|<\w[^>]*\s)id="([^"]+)"or a LinkeDOM pass would kill the false-positive class. Low priority but worth putting on the cleanup list since this regex is now used on both drop-from-assets and drop-from-external paths.
Another small observation:
- *
media.src = \/api/projects/${projectId}/preview/${assetPath}`;* inresolveDroppedAssetDuration—assetPathis user-controllable file name (post-upload), and path segments with spaces or#would break URL parsing. The upload route presumably sanitizes names (finalNameinfiles.ts) but a defensiveencodeURI(assetPath)` here would be cheap insurance. Very low priority.
CI state (as of this comment)
All non-regression checks green on 9754c3b:
- Format / Typecheck / Lint / Test / Build / Test: runtime contract / Smoke / Tests on windows-latest / Render on windows-latest / CodeQL / player-perf / Perf (fps/scrub/drift/parity/load)
Regression-shards (all 10 shards) are in-progress — triggered at 00:27:47Z on the new head. Not blocking yet; will watch those and confirm when they land.
Tl;dr — code looks good, holding merge on regression-shards completing.
— hyperframes
Problem
Studio still broke down in three concrete authoring flows around timeline assets:
While implementing direct external drops, another real bug showed up:
raycast.mp4fromDownloadswere being rejected as unsupported media in Studio dev because the Vite API bridge was corrupting multipart request bodies before they reached the upload routeWhat this fixes
Timeline asset placement from inside Studio
Direct external file drops onto the timeline
Delete key support
Delete/BackspaceBinary upload fix for media files
Downloadsno longer get rejected asUnsupported media skippedjust because the dev bridge corrupted the request bodyRoot cause
There were really two separate gaps:
1. Asset placement / deletion workflow gaps
The timeline and asset systems already existed, but they were disconnected:
AssetsTabonly supported copy/import flowsTimelineonly handled raw file import, not positioned placement for existing assets2. Binary upload corruption in Studio dev
The Studio Vite API bridge rebuilt non-GET request bodies like this:
chunk.toString()Requestfrom that string bodyThat works for text, but it corrupts multipart binary uploads. By the time the upload route wrote the received file and ran
ffprobe, otherwise valid MP4s had already been mangled in-flight.Behavior
index.htmlinserts the asset into the root compositiondata-startDeleteon a selected clip removes that clip from the underlying HTML source and clears selection in StudioVerification
Local checks
bunx oxlint packages/core/src/studio-api/helpers/sourceMutation.ts packages/core/src/studio-api/helpers/sourceMutation.test.ts packages/core/src/studio-api/helpers/mediaValidation.ts packages/core/src/studio-api/helpers/mediaValidation.test.ts packages/core/src/studio-api/routes/files.ts packages/studio/src/App.tsx packages/studio/src/components/nle/NLELayout.tsx packages/studio/src/components/sidebar/AssetsTab.tsx packages/studio/src/player/components/Timeline.tsx packages/studio/src/player/components/Timeline.test.ts packages/studio/src/utils/timelineAssetDrop.ts packages/studio/src/utils/timelineAssetDrop.test.ts packages/studio/vite.config.ts packages/studio/vite.request-body.ts packages/studio/vite.request-body.test.tsbunx oxfmt --checkon the touched filesbun run --filter @hyperframes/core typecheckbun run --filter @hyperframes/studio typecheckbun test packages/core/src/studio-api/helpers/sourceMutation.test.ts packages/core/src/studio-api/helpers/mediaValidation.test.ts packages/studio/src/player/components/Timeline.test.ts packages/studio/src/utils/timelineAssetDrop.test.ts packages/studio/vite.request-body.test.tsBrowser / live verification
Verified against a live local Studio fixture:
Deleteremoves it from both the live timeline and the saved source HTMLraycast.mp4now succeed through the live Studio upload route instead of being rejected as unsupported mediaNotes
timeline-trio-verifyandtimeline-overlap-debugprojects used for verification are local-only and are not part of this PR