fix(studio): enable timeline resize for all elements, improve perf and UX#978
Conversation
…d UX Enable trim-start and trim-end for all authored timeline elements (divs, sections, compositions) — not just video/audio/img. The deterministic-window gate was overly restrictive since all non-implicit elements have authored data-start/data-duration that define their timeline window. Replace iframe reload after resize/move with direct DOM attribute patching via patchIframeDomTiming(). This eliminates playhead-jump-to-zero, visual blinking, and race conditions from file-watcher echoes. File persistence runs in a serialized background queue (persistTimelineEdit + enqueueEdit) so rapid edits don't overwrite each other. Add mediabunny-based media probe service (mediaProbe.ts) for fast metadata extraction from file headers. Timeline elements missing sourceDuration are enriched asynchronously without waiting for DOM loadedmetadata events. Tune the runtime media preloader: lower lazy threshold from 6 to 3 clips, add 3s lookbehind window for reverse scrub, adaptive promoted-clip cap. Deduplicate getTimelineEditCapabilities — computed once in TimelineCanvas and passed as a prop to TimelineClip instead of recomputing per clip. Remove dead PlaybackAdapter re-export from useTimelinePlayer — all consumers import directly from playbackTypes.
Fallow audit reportFound 22 findings. Duplication (6)
Health (16)
Generated by fallow. |
jrusso1020
left a comment
jrusso1020
left a comment
There was a problem hiding this comment.
Read the changed files in full (useTimelineEditing.ts, timelineEditing.ts, mediaProbe.ts, mediaPreloader.ts, usePreviewPersistence.ts, useTimelinePlayer.ts, plus the smaller patches). Posting as COMMENT (not stamping) per merge policy. Same preview-vs-render parity surface as hf#965 — focused on that.
Strong points
- Serialized
enqueueEditqueue is the right pattern. Rapid sequential resizes can't race against each other on file writes — each enqueued edit chains througheditQueueRef.current.then(...). Clean. - Optimistic iframe DOM patch + background file persistence is the correct UX shape. The "no playhead jump, no blinking" claim follows directly from skipping the iframe reload.
usePreviewPersistencerename +pendingTimelineEditPathRef— the suppression of file-change-echoes for in-flight timeline edits (thechangedPath.endsWith(pendingPath)check) is the right shape. Bonus: the timer was bumped from 1200ms → 4000ms to accommodate the queue's slower write path.mediaProbe.tshas cache + in-flight dedup withinflight.delete(key)cleanup in the.then— proper async pattern; concurrent probes for the same URL share one fetch.- mediaPreloader tuning — lookbehind window for reverse scrub, adaptive
MAX_PROMOTEDbased on clip count, threshold lowered to 3 — all reasonable. Test file was refactored to use acreateTestFixturehelper (net -126 lines is mostly boilerplate consolidation, not test deletion). - Verified the capability gate change: dropping
hasDeterministicWindowandcanOffsetTrimClipStartlets divs/sections with finite duration trim. For media withoutmedia-start/playback-start, the resize handler inuseTimelineEditing.tsonly synthesizesfallbackPlaybackStartwhencurrentPlaybackStart != null, so media without offset attrs get pure shift semantics (data-start moves, source starts at the new t). That's a defensible "all-elements resizable" semantic — the gate functions were over-strict for divs/sections.
Substantive concerns
1. Iframe DOM patch → runtime pickup mechanism — preview-vs-render parity question
patchIframeDomTiming calls el.setAttribute("data-start", ...) etc. on the iframe element. The HyperFrames runtime IIFE binds to those attributes at startup (initSandboxRuntimeModular reads them) and computes a scheduling timeline. Does the runtime react to subsequent data-* attribute mutations?
If yes (MutationObserver watching the clip elements, or per-tick attribute re-read) — the optimistic UI works.
If no — the iframe DOM shows the new attribute values, but the runtime keeps using the cached schedule. The user sees the visible attribute change but the preview's playback timing is unchanged. They make more edits, get confused.
The PR test plan claims "Playhead position preserved after resize/move" — implying it works. But the mechanism isn't described in the PR body. Two asks:
- Either add a 1-2 line comment in
patchIframeDomTimingdocumenting which runtime mechanism picks up the changes (link to the MutationObserver / rebind hook so the next reader doesn't have to trace), OR - If the runtime doesn't auto-react, the iframe patch needs to call into the runtime (
window.__hfForceTimelineRebind()?) to force a re-schedule. Otherwise the optimistic UI is showing stale state.
Worth verifying interactively before merge.
2. Concurrent-probe race in useTimelinePlayer setElements
needsProbe.map(async (el) => {
const result = await probeMediaUrl(el.src!);
if (!result) return;
const current = usePlayerStore.getState().elements; // fresh
// ...
const patched = current.slice();
patched[idx] = { ...current[idx], sourceDuration: result.duration };
setElements(patched); // last writer wins
});Classic lost-update race. If probe A and probe B resolve concurrently:
- A reads
current = [a0, b0, c0], computes patched[a1, b0, c0] - B reads
current = [a0, b0, c0](still! A hasn't called setElements yet), computes patched[a0, b1, c0] - A calls
setElements([a1, b0, c0]) - B calls
setElements([a0, b1, c0])— loses A's update
Fix shape: either use a functional update if setElements supports one (setElements(prev => ...)), or use a per-element setter that doesn't replace the whole array. Probably rare in practice (header probes resolve in ~10ms, similar-time collisions are unlikely), but the data corruption is silent and would manifest as "some media elements never get sourceDuration."
3. mediabunny dependency — MPL-2.0 + 9.69 MB unpacked
Verified via npm registry:
- License: MPL-2.0 (Mozilla Public License 2.0) — weak copyleft, file-level. Compatible with Apache 2.0 (hyperframes' license) for use as a dependency, but distribution requires preserving MPL notices and providing source for modified mediabunny files (if any). For a
npm installconsumer, this is fine; worth noting in CREDITS.md. - Unpacked size: 9.69 MB — substantial for a studio runtime dependency.
Two concrete asks:
- Confirm MPL-2.0 is acceptable per HeyGen's OSS policy (Apache 2.0 + MIT have been the prior pattern per
CREDITS.md). - Tree-shaking check — does the studio bundle actually pull in all 9.69 MB, or only the
Input/UrlSource/ALL_FORMATSsubset? Bundle-size impact onbun run build:studioworth measuring.
4. Silent error swallowing in two places
editQueueRef.current.then(...).catch((error) => console.error(\[Timeline] Failed to persist: ${label}`, error))` — a queued edit that fails surfaces only to console. User sees the optimistic iframe update and assumes save succeeded. Worth a toast or status-bar indicator on persistence failure.patchIframeDomTiming'stry/catchswallows cross-origin / mid-navigation errors with a comment "file is already saved" — but the file save is enqueued, NOT yet saved when the patch attempts. There's a window where the iframe patch silently fails AND the queued file save hasn't run yet. Probably rare (same-origin local dev) but worth tightening the comment.
5. Test-file rewrite scope
mediaPreloader.test.ts is +75/-201 (net -126 lines). I traced through: the deletion is from extracting a createTestFixture helper that replaces the per-test boilerplate. Same test cases are covered, just with less repetition. ✓ No test coverage loss.
The threshold-related test names were updated correctly ("is not lazy when fewer than 6" → "is not lazy when fewer than 3" etc.), matching the constant change. ✓
Cross-PR check vs hf#965
The hf#965 fix (activateSiblingTimelines on renderSeek) is on the render path. This PR's iframe DOM patch is on the preview path. Both paths now have separate post-edit refresh mechanisms — worth a follow-up to verify they don't drift (e.g., if the studio adds a new timing attribute, both the renderer's seek-reapply and the studio's iframe-patch need updates). The two paths share TIMING_ATTR_MAP semantics in concept but not in code.
Non-blocking
- Fallow audit failure on this PR — assumed pre-existing complexity flags, not new. Not gating per the repo convention.
formatTimelineAttributeNumber(track)fordata-track-index—trackis an integer per the type, butformatTimelineAttributeNumberis a generic number formatter. Worth a quick verify that integer track indices don't get serialized as"0.0". Probably already handled (integers serialize as integers under most format functions), just flagging.editQueueRefon component unmount — if the queue has pending writes when the component unmounts, the writes still fire (correct), but thependingTimelineEditPathRef.currentmutation may log a React strict-mode warning. Test in dev with strict mode if not already.
CI: required all green per merge state. Fallow audit failure non-gating. Regression-shards in progress; shard-7 (sub-composition-video) worth a final eyeball before merge given the timeline-attribute changes.
No blockers from my side; main asks are: (a) document the iframe-patch→runtime sync mechanism (or add the rebind call), (b) fix the lost-update race in useTimelinePlayer setElements, (c) confirm MPL-2.0 acceptable + measure bundle impact of mediabunny.
— Rames Jusso
vanceingalls
left a comment
vanceingalls
left a comment
There was a problem hiding this comment.
Additive review — Rames Jusso already covered the iframe-patch→runtime pickup question, the lost-update race in useTimelinePlayer probe completion, mediabunny license/bundle-size, silent error swallowing in editQueueRef.catch, and the test-file rewrite scope. Reading the same six files; calling out gaps not in Rames's review.
Strengths (not duplicating Rames)
persistTimelineEdit(packages/studio/src/hooks/useTimelineEditing.ts:111-137) — clean extraction. Single-responsibility, takes abuildPatchesclosure so Move and Resize share the file-write/timestamp/history scaffolding without splaying it across two ~80-line callbacks. The two old handlers were each ~80 lines of near-identical scaffolding; now each is ~15 lines of intent.patchIframeDomTiming'sTIMING_ATTR_MAP(useTimelineEditing.ts:69-73) — declarative map keyed byTimelineElementfield name →data-*attr. Easy to extend when a new timing attribute lands.
Important — not in Rames's review
1. Rollback path in Timeline.tsx:868-887 is now unreachable
packages/studio/src/player/components/Timeline.tsx:868-887 (resize) and :909-929 (move) call the handler and wrap with Promise.resolve(handler(...)).catch(rollback). The rollback is what reverts the optimistic updateElement on persistence failure.
Before this PR, the handlers were async and threw on save failure, so .catch fired. After this PR, the handlers are synchronous and return undefined — Promise.resolve(undefined).catch(...) never fires. The editQueueRef.current.then(...).catch(console.error) (useTimelineEditing.ts:196-198) swallows the error inside the queue and never propagates it back.
Failure mode: save fails (disk full, file locked, network), iframe DOM has been mutated by patchIframeDomTiming, player-store has been mutated by updateElement (Timeline.tsx:868-872, :913-916), file write fails silently. Console gets [Timeline] Failed to persist: Resize timeline clip, user sees the clip in the new position/size, but on next reload from disk the clip snaps back. No toast, no rollback.
This is a stronger framing of Rames's "silent error swallowing" — the issue isn't just "no toast," it's "the rollback contract the caller in Timeline.tsx relies on no longer fires." Fix shape: either (a) have enqueueEdit return the queued promise so callers can .catch on the actual save outcome, or (b) move the rollback inside the enqueueEdit.catch (it needs updateElement + the pre-edit snapshot — Timeline.tsx already has both).
2. pendingTimelineEditPathRef is single-slot + endsWith is loose
packages/studio/src/hooks/usePreviewPersistence.ts:170-173:
const pendingPath = pendingTimelineEditPathRef?.current;
if (pendingPath && changedPath.endsWith(pendingPath)) {
pendingTimelineEditPathRef!.current = null;
return;
}Two issues stacked:
a. Single-slot ref: persistTimelineEdit sets pendingTimelineEditPathRef.current = targetPath (useTimelineEditing.ts:125) before each save. If the user edits clip-on-fileA then immediately clip-on-fileB before fileA's save's SSE event has fired, the ref is overwritten and fileA's echo is no longer suppressed — that file-change event will trigger reloadPreview(), defeating the whole "no iframe reload" goal of the PR.
b. endsWith matching: a pending edit on index.html will silently suppress a file-change event for legitimate/different/index.html (a teammate's edit, a server-side asset write, etc.). The 4-second recentDomEditSave window already exists as a second-layer guard, but the endsWith match short-circuits even outside that window.
Fix shape: use a Set<string> of full paths (with a TTL cleanup) and exact-match. The current pattern of "remove the ref the first time we see a matching path" is brittle if SSE fires twice for the same change (debounce / dev-mode HMR can do that).
3. LAZY_THRESHOLD 6 → 3 is a hot-path behavior change that the PR-body validates only on the high-element case
packages/core/src/runtime/mediaPreloader.ts:6-9 — threshold drops from 6 to 3. The PR's test plan says "verified in real Hyperframes studio with 17-element composition" — 17 is well above either threshold. The interesting case is the 4-5 clip composition that was previously in eager mode and is now in lazy mode.
For a 4-clip composition, the new behavior is:
- Lazy mode activates (
isLazy() === true). - Window-based eviction kicks in (
syncWindownow runs every tick). MAX_PROMOTED = min(6, 4 + floor(4/10))= 4 — all 4 clips fit, defense-in-depth never triggers.
So functionally for 3-5 clips the behavior is similar to eager (all promoted), but with the additional per-tick syncWindow work and an extra onActivation callback. That's not a regression, but the comment in the source ("medium compositions (4-5 heavy videos) saturate browser memory") implies the team has seen real saturation at 4-5; the test plan doesn't capture that case. Worth a regression test that pins a 4-clip composition's memory/promoted-count after this change.
4. Capability gate change unblocks GSAP/CSS-animated div/section trims without a matching animation-duration update
packages/studio/src/player/components/timelineEditing.ts:244-248 — the new getTimelineEditCapabilities allows canTrimEnd and canTrimStart on any patchable element with finite duration, including authored divs/sections whose visual duration is driven by a GSAP/CSS animation. Trimming data-duration from 5s to 3s now succeeds at the studio level — the clip on the timeline shows 3s — but if the underlying animation is 5s, the runtime animation will still run for 5s.
The PR has buildTimelineElementAgentPrompt (still on the codebase) that previously triggered for these blocked clips, telling the user to update the authored animation as well. Now these clips are unblocked at the studio level, so the user can create the data-duration/animation-duration mismatch without ever seeing that prompt.
This is consistent with the PR's stated direction ("all authored timeline elements are now resizable"), but it shifts a class of correctness problem from the studio's "blocked" toast onto the runtime/render path. A small regression test that pins getTimelineEditCapabilities for an explicitly-authored <section> with timingSource: "authored" and a GSAP-bearing inner subtree would be useful — even if the answer is "we allow it and the user is on their own to update the GSAP timeline" that should be the documented design.
Nits
canOffsetTrimClipStart(packages/studio/src/player/components/timelineEditing.ts:204-220) is now dead code outside its own test file —getTimelineEditCapabilitiesno longer calls it. Either re-wire it intocanTrimStart(e.g., gate playback-offset semantics vs pure shift semantics) or drop both the function and its tests.patchIframeDomTiming's comment "file is already saved" (useTimelineEditing.ts:92-93) is incorrect — at the timepatchIframeDomTimingruns, the save has only been enqueued, not yet executed. Tighten to "file save is enqueued; iframe patch is best-effort". Rames mentioned this loosely; calling out the exact wording.editQueueRefkeeps the queue alive across remounts (it's auseRef). If the hook is dismounted and remounted (e.g., project switch), pending writes for the old projectId continue to fire via the capturedpidclosure.useTimelineEditing.ts:181readsprojectIdRef.currentat enqueue time but the closure pins it; that's the correct shape. Just flagging that the queue's lifetime crosses project boundaries.
CI status (verbatim, head 0769dc4b)
- Required (per
repos/heygen-com/hyperframes/rules/branches/main): Semantic PR title, Test: runtime contract, Typecheck, Build, regression, Test, Render on windows-latest, Tests on windows-latest. - Passing required: Semantic PR title, Test: runtime contract, Typecheck, Build, Test. ✓
- Pending required:
regression-shards (shard-1..8),Render on windows-latest,Tests on windows-latest. - Failing (non-required):
Fallow audit— 24 findings, mostly high-CRAP-score on functions this PR touched (patchIframeDomTiming,findIframeElement, the inlined resize closure,handleTimelineAssetDrop). Not gating but signals that the inlinedenqueueEditclosures could be lifted out for readability. mergeStateStatus:BLOCKED— reviewer-gate (REVIEW_REQUIRED), not a CI failure.
Verdict
Verdict: COMMENT
Reasoning: Rames already left a substantive review with strong overlap on the core concerns; my pass adds four additive items (Timeline.tsx rollback path break, single-slot pendingPath + endsWith looseness, missing 4-5-clip regression test for the threshold change, GSAP-animation mismatch on unblocked trims) plus three nits. None are hard blockers on their own — the rollback-path break (#1) is the most concrete correctness regression and worth a small fix before merge. Deferring final verdict to maintainer; the four items above are worth at least a tracker if not in this PR.
— Vai
Extract resolveResizePlaybackStart, simplify patchIframeDomTiming to accept attr tuples, inline findIframeElement. Reduces CRAP scores in the resize handler lambda and DOM patching functions.
…rden edit suppression Restore rollback path: enqueueEdit now returns the queued promise so Promise.resolve(handler(...)).catch(rollback) in useTimelineClipDrag fires correctly on save failure. Handlers return the promise chain. Fix lost-update race in probe enrichment: use zustand's functional setState so concurrent probe completions each read the latest state atomically instead of all reading the same stale snapshot. Harden file-change suppression: pendingTimelineEditPathRef is now a Set<string> with exact-match lookup instead of single-slot + endsWith. Multiple concurrent edits on different files are all suppressed correctly. Remove dead canOffsetTrimClipStart function and its tests — no longer called after the capability gate simplification. Document runtime sync mechanism: added comment explaining that the runtime re-reads data attributes on each sync tick (init.ts:1324-1368). Fix comment wording in patchIframeDomTiming catch block.
jrusso1020
left a comment
jrusso1020
left a comment
There was a problem hiding this comment.
Re-review on 366fcc2a. Major findings from my and Vai's first passes are all addressed:
- ✅ Iframe DOM patch → runtime sync mechanism now documented inline in
patchIframeDomTiming: "The runtime re-reads data-start/data-duration from the DOM on each sync tick (packages/core/src/runtime/init.ts:1324-1368), so attribute mutations here are picked up automatically on the next frame without a rebind call." Exactly the pointer I asked for — saves the next reader the trace. - ✅ Probe race in
useTimelinePlayer— converted to Zustand functionalsetState((state) => ...)form. The closure readsstate.elementsatomically inside the setter; no lost-update race possible across concurrent probes. - ✅ Broken rollback path (Vai's load-bearing finding) —
enqueueEditnow returns the rawqueuedpromise while still assigningeditQueueRef.current = queued.catch(log). Two-level chain: queue continues unblocked on failure, AND the caller'sPromise.resolve(handler(...)).catch(rollback)inTimeline.tsx:868-887now receives a real rejecting promise. BothhandleTimelineElementMoveandhandleTimelineElementResizeexplicitlyreturn enqueueEdit(...). ✓ - ✅
pendingTimelineEditPathRefsingle-slot + endsWith — changed toSet<string>+ exact-match.has()/.delete(). Concurrent edits to different files no longer lose suppression; loose path-prefix collisions across nested dirs are eliminated. - ✅
canOffsetTrimClipStartdead code — removed (along with its 37-line test). - ✅ Stale catch comment — now reads "file save is enqueued; iframe patch is best-effort." Accurate.
Still open
- CREDITS.md not updated for mediabunny (MPL-2.0). Not addressed in this round. MPL-2.0 dependency is new for this repo (prior pattern in CREDITS.md is Apache 2.0 + dep acknowledgements; mediabunny is the first MPL dep). One-line addition under "Thanks" should be enough.
- No regression test for
LAZY_THRESHOLD4-5 clip case. The new comment inmediaPreloader.tsexplicitly cites "medium compositions (4-5 heavy videos) saturate browser memory before the preloader kicked in" as the motivation — but no test exercises that exact range. Existing tests are at 2 (below), 3 (at), 8 (above). Ait("activates lazy mode at 4 elements")test would lock in the new threshold without much new boilerplate. - Vai's GSAP/CSS-animated
div/sectioncapability-gate concern — surfacing thedata-durationvs animation-duration mismatch onto the runtime is the larger architectural conversation. Probably the right call to defer to a follow-up rather than scope-creep this PR, but worth a tracking issue so it's not lost. Not blocking the merge as long as the failure mode is "animation overruns or undershoots" rather than "renderer crashes." - mediabunny bundle-size impact — not measurable from a code review surface; needs an actual
bun run build:studiocomparison. Worth a one-off check before flipping the studio default for users on slow connections. Non-blocking.
CI
At 366fcc2a: most required checks haven't reported yet (push happened ~5 min ago). Pre-fix run on 0769dc4b had required-checks green / Fallow audit failure (non-gating) / regression-shards in progress. Worth letting the shard-7 run on this SHA finish before merge given the timing-attribute touch.
Verdict from me
The four pieces of substantive feedback (rollback, probe race, suppression-Set, runtime-sync mechanism) are cleanly resolved. The remaining open items are documentation/coverage/measurement rather than correctness. None block merge from my read — defer to Miguel + James on the timing.
— Rames Jusso
vanceingalls
left a comment
vanceingalls
left a comment
There was a problem hiding this comment.
Re-reviewing at 366fcc2a against my prior comments at 0769dc4b. Diff: useTimelineEditing.ts (+75/-78), usePreviewPersistence.ts (+3/-4), useTimelinePlayer.ts (+7/-6), App.tsx (+1/-1), timelineEditing.ts (-12), timelineEditing.test.ts (-37).
Status of prior findings
1. Rollback path — ADDRESSED
useTimelineEditing.ts:184-209: enqueueEdit now returns the raw queued promise (line 208), and both handlers return enqueueEdit(...) (lines 225, 250). Promise.resolve(handler(...)).catch(rollback) in the Timeline drag callsite is reachable again. The editQueueRef.current = queued.catch(...) line keeps the queue alive while still surfacing the rejection to the immediate caller — correct pattern.
2. Single-slot pendingTimelineEditPathRef + endsWith — ADDRESSED
App.tsx:120: useRef(new Set<string>()). useTimelineEditing.ts:135: pendingTimelineEditPathRef.current.add(targetPath). usePreviewPersistence.ts:170-172: .has(changedPath) exact-match, .delete(changedPath) on consumption. Concurrent edits on different files are independently suppressed; endsWith false-positives are gone.
3. LAZY_THRESHOLD 6→3 regression coverage — PARTIAL
mediaPreloader.test.ts adds "activates lazy mode at exactly LAZY_THRESHOLD (3 elements)", "is not lazy with 2 elements (below threshold)", and the __HF_LAZY_PRELOAD_THRESHOLD override. Boundary-activation is well-covered. The original concern — memory/promoted-count for the 4–5 clip "saturation" case the source comment calls out — still has no targeted test. Not a blocker; worth a follow-up that pins promoted-count at 4 clips with a window-eviction scenario.
4. Capability gate / GSAP-animation mismatch — PARTIAL
timelineEditing.ts capability logic unchanged in this push; the dead canOffsetTrimClipStart is now removed (good). The commit message claims runtime-sync was documented at init.ts:1324-1368. The visibility-loop there respects data-duration and live compTimeline.duration() for composition hosts — correct. But for a leaf GSAP-animated <section>, trimming data-duration from 5s→3s still produces a 3s timeline-window for a 5s inner animation with no authoring path to update it. The studio side no longer surfaces the previously-blocked toast that prompted the user to fix the animation. Calling this PARTIAL — runtime path explained, but the authored-animation-duration update is still on the user with no guidance. Worth a small toast or doc note when canTrimEnd === true on timingSource: "authored" elements with inner GSAP timelines.
5. Nits — ADDRESSED
canOffsetTrimClipStart and its test deleted (12 + 37 lines). Comment in patchIframeDomTiming now reads "file save is enqueued; iframe patch is best-effort" (useTimelineEditing.ts:77).
Bonus — probe lost-update race
useTimelinePlayer.ts:126-132 switches probe enrichment to usePlayerStore.setState((state) => {...}). Each concurrent probe completion now reads the latest state.elements atomically — earlier completions are no longer clobbered. Clean fix.
Verdict
4 of 5 substantively addressed (1, 2, 5 fully; 3 boundary covered but saturation case still untested; 4 runtime path documented but authored-GSAP update guidance missing). The two PARTIALs are improvement opportunities, not correctness blockers given the existing test coverage and runtime fallback behavior. Approving.
Required CI at re-review time: Build, Typecheck, Test, Test: runtime contract, Semantic PR title, Lint, Format, CLI smoke, Preflight, Smoke: global install, Preview parity, preview-regression, player-perf — all pass. Pending: all 8 regression-shards, Render on windows-latest, Tests on windows-latest. Non-required Fallow audit still failing (CRAP findings; non-gating).
Review by Vai (re-review)
vanceingalls
left a comment
vanceingalls
left a comment
There was a problem hiding this comment.
Re-reviewing at 366fcc2a against my prior comments at 0769dc4b. Diff: useTimelineEditing.ts (+75/-78), usePreviewPersistence.ts (+3/-4), useTimelinePlayer.ts (+7/-6), App.tsx (+1/-1), timelineEditing.ts (-12), timelineEditing.test.ts (-37).
Status of prior findings
1. Rollback path — ADDRESSED
useTimelineEditing.ts:184-209: enqueueEdit now returns the raw queued promise (line 208), and both handlers return enqueueEdit(...) (lines 225, 250). Promise.resolve(handler(...)).catch(rollback) in the Timeline drag callsite is reachable again. The editQueueRef.current = queued.catch(...) line keeps the queue alive while still surfacing the rejection to the immediate caller — correct pattern.
2. Single-slot pendingTimelineEditPathRef + endsWith — ADDRESSED
App.tsx:120: useRef(new Set<string>()). useTimelineEditing.ts:135: pendingTimelineEditPathRef.current.add(targetPath). usePreviewPersistence.ts:170-172: .has(changedPath) exact-match, .delete(changedPath) on consumption. Concurrent edits on different files are independently suppressed; endsWith false-positives are gone.
3. LAZY_THRESHOLD 6→3 regression coverage — PARTIAL
mediaPreloader.test.ts adds "activates lazy mode at exactly LAZY_THRESHOLD (3 elements)", "is not lazy with 2 elements (below threshold)", and the __HF_LAZY_PRELOAD_THRESHOLD override. Boundary-activation is well-covered. The original concern — memory/promoted-count for the 4–5 clip "saturation" case the source comment calls out — still has no targeted test. Not a blocker; worth a follow-up that pins promoted-count at 4 clips with a window-eviction scenario.
4. Capability gate / GSAP-animation mismatch — PARTIAL
timelineEditing.ts capability logic unchanged in this push; the dead canOffsetTrimClipStart is now removed (good). The commit message claims runtime-sync was documented at init.ts:1324-1368. The visibility-loop there respects data-duration and live compTimeline.duration() for composition hosts — correct. But for a leaf GSAP-animated <section>, trimming data-duration from 5s→3s still produces a 3s timeline-window for a 5s inner animation with no authoring path to update it. The studio side no longer surfaces the previously-blocked toast that prompted the user to fix the animation. Calling this PARTIAL — runtime path explained, but the authored-animation-duration update is still on the user with no guidance. Worth a small toast or doc note when canTrimEnd === true on timingSource: "authored" elements with inner GSAP timelines.
5. Nits — ADDRESSED
canOffsetTrimClipStart and its test deleted (12 + 37 lines). Comment in patchIframeDomTiming now reads "file save is enqueued; iframe patch is best-effort" (useTimelineEditing.ts:77).
Bonus — probe lost-update race
useTimelinePlayer.ts:126-132 switches probe enrichment to usePlayerStore.setState((state) => {...}). Each concurrent probe completion now reads the latest state.elements atomically — earlier completions are no longer clobbered. Clean fix.
Verdict
4 of 5 substantively addressed (1, 2, 5 fully; 3 boundary covered but saturation case still untested; 4 runtime path documented but authored-GSAP update guidance missing). The two PARTIALs are improvement opportunities, not correctness blockers given the existing test coverage and runtime fallback behavior. Approving.
Required CI at re-review time: Build, Typecheck, Test, Test: runtime contract, Semantic PR title, Lint, Format, CLI smoke, Preflight, Smoke: global install, Preview parity, preview-regression, player-perf — all pass. Pending: all 8 regression-shards, Render on windows-latest, Tests on windows-latest. Non-required Fallow audit still failing (CRAP findings; non-gating).
Review by Vai (re-review)
Add mediabunny (MPL-2.0) to CREDITS.md third-party licenses section. Add regression test for 4-5 clip compositions under the lowered lazy threshold — verifies lazy mode activates and no spurious eviction churn occurs when all clips fit within the promoted cap.
3 participants
Summary
All authored timeline elements are now resizable — divs, sections, compositions get trim-start and trim-end handles, not just video/audio/img. The
isDeterministicTimelineWindowgate was overly restrictive for elements with authoreddata-start/data-duration.No more iframe reload on resize/move — replaced
reloadPreview()with direct DOM attribute patching viapatchIframeDomTiming(). The preview stays stable: no playhead jump to 0, no blinking, no crossfade flash. File persistence runs in a serialized background queue so rapid edits don't overwrite each other.mediabunny integration — added
mediaProbe.tsservice that uses mediabunny'sInput+UrlSourceto extract media duration from file headers. Timeline elements missingsourceDurationare enriched asynchronously without waiting for DOMloadedmetadataevents.Runtime media preloader tuned — lowered lazy threshold from 6 to 3 clips, added 3s lookbehind window for reverse scrub, adaptive promoted-clip cap (
min(6, 4 + floor(clips/10))).Deduplicated capabilities computation —
getTimelineEditCapabilitiescomputed once in TimelineCanvas and passed as prop to TimelineClip instead of recomputing per clip per render.Cleaned up dead code — removed unused
PlaybackAdapterre-export from useTimelinePlayer, extracted sharedpersistTimelineEdit+createTestFixturehelpers to reduce duplication.Test plan