fix: harden studio timeline editing and local renders#463
fix: harden studio timeline editing and local renders#463miguel-heygen merged 5 commits intomainfrom
Conversation
jrusso1020
left a comment
There was a problem hiding this comment.
Verdict: approve — but do not merge until Tests on windows-latest is green
Clean scoping. The 6 fixes map cleanly to 6 distinct issues, and the test coverage (11 new Timeline assertions + 119 lines of timelineEditing tests + 37 lines of CompositionsTab tests + 61 lines of producer-autobuild tests) is proportional to the surface touched.
On the user feedback that triggered this: the Loom covers three asks. This PR addresses #1 (can't adjust scenes — capability gate was using "spans multiple scenes" as a wrong proxy) and explicitly defers #2 (scene expansion) and #3 (asset drop-onto-timeline) — the PR body is up-front about that, and the title ("harden studio timeline editing and local renders") matches the fix-the-broken-parts framing. Deferring the feature asks for a follow-up is the right call; conflating reliability fixes with new authoring UX would make this diff unreviewable.
Architecture spot-checks
Capability gating fix is exactly right. Dropping "spans multiple scenes" as a proxy was overdue — for directly patchable media (data-start / data-duration / seeded data-media-start), Studio IS the safe author of record regardless of scene spans. The block now only fires on clips that genuinely lack a patch path.
Floating TimelineEditorNotice (156 new lines) with constrained pointerEvents is the right shape for the "discoverability UI shouldn't steal pointer events from the timeline underneath it" class of bug. Without seeing the diff in detail I'm trusting that pointer-events: none on the card with pointer-events: auto on the dismiss button is the mechanism — which is the canonical pattern for this case.
Composition-host visibility respecting authored trimmed duration — the one-line change in packages/core/src/runtime/init.ts (includeAuthoredTimingAttrs: true) is the right seam. The runtime was sanitizing away the authored duration after live-host attrs got stripped; surfacing it back via this option unblocks preview from ignoring timeline trims.
On-demand @hyperframes/producer build in dev (vite.producer.ts / vite.producer.test.ts) is scoped to missing-dist-entry, gated behind existsSync, and uses execFileSync with arg arrays (not a shell). Won't leak into prod builds; won't run when dist exists.
🚨 Windows test failure — concrete diagnosis
Tests on windows-latest is red. Looked at the job and cross-referenced the diff — the cause is in packages/studio/vite.producer.test.ts:
expect(resolveProducerDistEntry("/repo/packages/studio")).toBe(
"/repo/packages/producer/dist/index.js",
);The production resolveProducerDistEntry uses node:path's resolve(), which on Windows produces backslash-separated paths (C:\repo\packages\producer\dist\index.js) — so the POSIX-style string assertion fails. Same trap on the workspace-root test and the execFileSync cwd: "/repo" expectation inside the "builds producer" test.
Two line-diffs to fix it:
// Option A — wrap the expected string with resolve() so both sides normalize identically:
expect(resolveProducerDistEntry("/repo/packages/studio")).toBe(
resolve("/repo/packages/producer/dist/index.js"),
);
// Option B — switch the production code to path.posix.resolve if you want stable POSIX
// paths everywhere. Safer if anything downstream relies on the string shape.Option A is the minimum-change fix for just the tests; option B is the minimum-surprise fix for any future consumer that inspects these strings. Either works. Render-on-windows-latest is green so the production code itself isn't Windows-broken — this is specifically a test-assertion issue.
Non-blocking notes
- Jake/Leo deferred asks. Both scene-expansion and asset-drop should go on the backlog with tracking issues — if they stay as "future work" in a Slack Loom, they'll get rediscovered by the next user who hits the same moment.
- The on-demand producer build uses
stdio: "pipe". If the build fails, the error surface is opaque to the Studio dev server — a user hitting the first-render flow would see the request fail without seeing whybun run --filter @hyperframes/producer buildcouldn't complete. Logging the stderr when the build fails (or using"inherit"in dev) would cut a debugging step.
Stamp
Code merit → approve, conditional on the Windows test fix landing before merge. Don't flip the merge button until Tests on windows-latest is green. Once it is, the PR is ready to ship without another review pass from me.
Review by hyperframes
vanceingalls
left a comment
There was a problem hiding this comment.
I checked the existing feedback before posting this and skipped the Windows path-assertion issue plus the stdio: \"pipe\" observability note since those are already covered.
Two additional concerns I don't think are called out yet:
packages/studio/src/player/components/timelineEditing.ts+packages/studio/src/player/components/Timeline.tsx
resolveBlockedTimelineEditIntent() prioritizes blocked resize zones before it checks canMove, and Timeline returns early on any blocked intent. That means clips that are intentionally movable but not start-trimmable can lose part or all of their draggable surface.
The branch's own test suite classifies composition hosts this way:
canMove: truecanTrimStart: falsecanTrimEnd: true
So grabbing the left edge now resolves to \"resize-start\" and gets swallowed by the blocked-edit path instead of starting a move. For narrow clips (width <= handleWidth), the entire clip can fall into that blocked zone, leaving no draggable area at all.
Relevant spots:
packages/studio/src/player/components/timelineEditing.ts:252-259packages/studio/src/player/components/Timeline.tsx:1198-1218packages/studio/src/player/components/timelineEditing.test.ts:297-309
packages/studio/vite.config.ts
getProducerModule() memoizes _producerModulePromise, but if the first ensureProducerDist() / dynamic import attempt fails, that rejected promise is cached forever. After that, every later local render attempt reuses the same rejection until the Studio dev server is restarted.
Relevant spot:
packages/studio/vite.config.ts:81-97
I reran the targeted @hyperframes/studio tests and typecheck locally with Bun; both passed, so these look like uncovered regressions rather than restating the already-known failing test issue.
|
@vanceingalls addressed both points in
Fresh verification after the fix:
|
Problem
Studio timeline editing had a cluster of real regressions that showed up together while testing the new timeline flow:
spans multiple scenes) even when Studio had a direct patch path fordata-start/data-durationShift+clickrange editing was intended to work, but the real pointer flow could swallow the action and never open the edit popover@hyperframes/producer/dist/index.jsalready existed1920x1080stage and could show dead space for narrower scenesWhat this fixes
This PR makes the Studio timeline behave according to the actual product bar: only block edits when Studio does not have a safe authored patch path.
Timeline editing
0, writingdata-media-starton first trimShift+clickreliably open the full-range edit popover through the real pointer-capture flowTimeline discoverability and interaction polish
Runtime preview correctness
Local render resilience
@hyperframes/producerbuild artifacts by building the producer package on demand in dev before importing itSidebar composition previews
1920x1080, which removes the blank-space framing bug on scene cardsRoot causes
There were three main failures behind the user-facing bugs:
1. Capability gating used the wrong proxy
The earlier logic treated “overlaps multiple scenes” as equivalent to “not safely editable.” That is not true for plain authored media clips that already have a direct patch target and deterministic timing attributes.
The fix removes that proxy and keeps capability gating tied to actual patchability and deterministic timing support.
2. Runtime visibility and interaction flows were inconsistent with authored edits
Shift+clicklost its pending clip selection through the shared pointer-capture pathThat combination made the timeline feel unreliable even when the source patch had actually been written.
3. Studio dev render assumed prebuilt workspace artifacts
The Studio dev server imported the published
@hyperframes/producerentrypoint directly. In a worktree withoutpackages/producer/dist/, the first local render failed immediately.The fix preserves the normal package import path but ensures the dist exists first in local dev.
Verification
Local checks
bunx oxlint packages/core/src/runtime/init.ts packages/studio/src/App.tsx packages/studio/src/components/nle/NLELayout.tsx packages/studio/src/components/nle/TimelineEditorNotice.tsx packages/studio/src/components/sidebar/CompositionsTab.tsx packages/studio/src/components/sidebar/CompositionsTab.test.ts packages/studio/src/player/components/Timeline.tsx packages/studio/src/player/components/Timeline.test.ts packages/studio/src/player/components/TimelineClip.tsx packages/studio/src/player/components/timelineEditing.ts packages/studio/src/player/components/timelineEditing.test.ts packages/studio/src/player/hooks/useTimelinePlayer.ts packages/studio/src/player/hooks/useTimelinePlayer.test.ts packages/studio/vite.config.ts packages/studio/vite.producer.ts packages/studio/vite.producer.test.tsbun run --filter @hyperframes/studio typecheckbun test packages/studio/vite.producer.test.ts packages/studio/src/components/sidebar/CompositionsTab.test.ts packages/studio/src/player/components/Timeline.test.ts packages/studio/src/player/components/timelineEditing.test.ts packages/studio/src/player/hooks/useTimelinePlayer.test.tsBrowser verification
Verified against a live local Studio fixture with two scene hosts plus background audio:
0:00Shift+clickopens the edit popover for full clip ranges1920x1080Local render verification
Verified local Studio render via
POST /api/projects/timeline-trio-verify/render:@hyperframes/producer/dist/index.jspackages/studio/vite.producer.test.tsas a smoke/regression test for the exact missing-dist fallback (dist present -> no build,dist missing -> build before import)Notes
timeline-trio-verifyproject used for local browser/render verification is local-only and not part of this PR