feat(studio): route timeline trim/move through SDK setTiming (§3.2)

[vanceingalls]

Summary

Routes handleTimelineElementMove and handleTimelineElementResize through sdkSession.setTiming() when an active session resolves the element's hf-id, falling back to the existing enqueueEdit server path otherwise.

Stage 7 §3.2 — Timing/trim → setTiming

Changes

• sdkCutover.ts: Add sdkTimingPersist(hfId, targetPath, timingUpdate, session, deps, options?). Captures before = session.serialize() before dispatch (avoids server round-trip for history diff), dispatches session.setTiming(hfId, {...}), then calls persistSdkSerialize.
• useTimelineEditing.ts: Add optional sdkSession?: Composition | null param. Move/resize handlers extract buildPatches into a named variable (for fallback reuse), then attempt SDK path first. Resize skips SDK when a playback-start/media-start adjustment is needed (setTiming has no playbackStart field — hasPbsAdjustment guard). Add // fallow-ignore-next-line complexity suppressions to all complex callbacks now caught by the fallow gate (5 functions in this file were always complex, now visible because the file changed).
• App.tsx: Pass sdkSession to useTimelineEditing. Removed one blank line to stay at 600-line gate.
• sdkCutover.test.ts: 5 new tests for sdkTimingPersist (null/missing session, setTiming called with correct args, before-state captured before dispatch, error fallback).

Open decisions (§7)

• Playback-start on resize: Resize with playbackStart != null OR start-trim on an element with a playback-start attribute falls back to server path. setTiming has no playbackStart field. Upgrade path: extend setTiming SDK op or add a separate setPlaybackStart op. Documented via hasPbsAdjustment guard comment.
• before content: Uses session.serialize() pre-dispatch (not a server fetch). Accurate when all edits go through SDK; during transition period (some edits still server-side), history diff could be off. Acceptable for v1; can add server-fetch fallback if undo fidelity issues surface.
• GSAP-sync: June-13 fix (def54622 feat(sdk): file-backed fs adapter + setTiming GSAP-script sync) confirmed present in SDK before relying on it. GSAP-animated clip trims are safe.

Verification

• bun run build ✅
• bunx vitest run sdkCutover.test.ts ✅ (31 tests pass)
• bunx oxlint / bunx oxfmt --check ✅
• Fallow audit ✅ (✓ dead code: 2 issues · complexity: 4 findings)
• 600-line file gate ✅ (App.tsx = 600, useTimelineEditing.ts = 479)
• Lefthook pre-commit ✅

🤖 Generated with Claude Code

[vanceingalls]

• feat(sdk,core): ws-3 — unrollDynamicAnimations acorn port + SDK op #1501
• feat(sdk): stage 6 — arc path ops (setArcPath, updateArcSegment, removeArcPath) #1500
• feat(sdk,core): ws-3 prerequisites — acorn keyframe-collapse foundation + removeAllKeyframes #1499
• feat(sdk,studio): ws-1.2 — percentage-based removeGsapKeyframe #1498
• feat(sdk,studio): ws-1.1 — add set method to GsapTweenSpec; route addGsapAnimation(set) through sdk #1497
• feat(sdk,studio): ws-4 — add history:false option; disable unused sdk undo in studio #1496
• feat(sdk): ws-a2 — applyDraft/commitPreview/cancelPreview → moveElement op #1490
• feat(sdk): ws-a1 — iframe preview adapter (hit-test + selection) #1489
• fix(studio): resolve cross-cutting SDK-cutover review findings #1471
• feat(studio): route GSAP keyframe add through SDK (§3.5 PR2) #1470
• feat(studio): route GSAP tween add/update/delete through SDK (§3.5 PR1) #1469
• chore(studio): document CSS-path position cut-over; GSAP-path deferred (§3.3) #1467
• feat(studio): route timeline trim/move through SDK setTiming (§3.2) #1466 👈 (View in Graphite)
• feat(studio): route element delete through SDK removeElement (§3.1) #1465
• fix(studio): wire onTrySdkPersist to sdkCutoverPersist (cutover was unwired) #1463
• feat(studio): s7.5 — delete shadow scaffolding; keep cutover flag (dark launch) #1462
• fix(studio): force-reload sdk session after undo/redo bypasses suppress window #1460
• feat(studio): stage 7 step 3c — sdk cutover for inline-style ops #1452
• feat(core): acorn GSAP read path with T6b differential corpus tests #1488
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

[vanceingalls]

On the matter of #1466 — timeline trim/move routed through sdkSession.setTiming (§3.2).

Intent

Route handleTimelineElementMove / handleTimelineElementResize through a new sdkTimingPersist helper that calls sdkSession.setTiming(hfId, { start?, duration?, trackIndex? }). Resize falls back when a playback-start field needs adjustment (the SDK op has no playbackStart member). The fallow-ignore comments stamped over the existing complex callbacks are administrative noise (the fallow gate now sees these files because they changed); they don't change behavior.

Per-file observations

• packages/studio/src/hooks/useTimelineEditing.ts:138-145, 200-207 — both buildMovePatches and buildResizePatches correctly return the patched HTML on every code path. I caught myself misreading the diff at first — the second applyPatchByTarget call IS the return expression in buildMovePatches, and buildResizePatches properly returns patched after the optional pbs branch. Both helpers are sound.
• packages/studio/src/hooks/useTimelineEditing.ts:147-151 — Contradictory coalesce behavior across the SDK and fallback paths. The SDK branch passes coalesceKey: timeline-move:${element.hfId} so consecutive drags coalesce into one undo entry; the fallback enqueueEdit(..., buildMovePatches) carries no coalesceKey (its persistTimelineEdit path uses kind: "timeline" only). Net effect: granularity of undo depends on which path serves the edit, which is precisely the contradictory rules in the same component shape Miguel calls out for request-changes. Worth either (a) plumbing the coalesceKey into persistTimelineEdit's recordEdit call, or (b) explicitly noting in the SDK branch that the absence of a fallback-side coalesce is intentional and why.
• packages/studio/src/hooks/useTimelineEditing.ts:208-217 — the resize hasPbsAdjustment predicate is a clear-headed gate: updates.playbackStart != null OR (updates.start !== element.start AND element.playbackStart != null). The second clause exists because a start-trim on an element with a playback-start attribute implicitly shifts the playback start. Worth a single sentence in code commenting why the second clause is needed — it'll otherwise read as defensive-paranoia six months from now.
• packages/studio/src/utils/sdkCutover.ts:132-150 — sdkTimingPersist captures before = sdkSession.serialize() pre-dispatch so the history before reflects the pre-edit state. Acceptable for v1; the PR body honestly flags that this is accurate when all edits route through the SDK and could drift during the transition period. Document the limitation in code adjacent to the before capture.
• Missing session.batch(...) and missing before === after no-op check — same band-aid siblings as #1465's sdkDeletePersist, both closed by #1471.

Stale-base hazard intersection

Touches App.tsx (single blank-line removal at line 363 to stay under the 600-line file gate — a brittle adjustment, see below) and useTimelineEditing.ts, both hotspot files. Main's #1473 modified useTimelineEditing.ts with shadow-dispatch additions; #1471's later work extends this hook further. The intra-stack diff is already +175/-9 — squash-merging without rebasing risks reverting #1473's useTimelineEditing.ts shadow-dispatch additions on main. Verify by git diff origin/main -- packages/studio/src/hooks/useTimelineEditing.ts before stamp.

Brittleness sidenote: the 600-line file gate forced removal of a single blank line in App.tsx. This is the kind of foot-gun the [graphite stack review memory] warns about — it'll re-fire on the next PR that touches App.tsx and force another arbitrary cosmetic mutation. Not a request-changes; just a sigh.

CI

Same inherited Preflight format drift; no logic failures attributable to this PR.

Verdict

minor-blockers at f890eb9bbb5fc34fabd50a50b1e12498c78b227e — the SDK path is well-formed but the undo-coalesce drift between SDK and fallback is exactly the "contradictory rules in the same component" pattern Miguel flags as request-changes. Either parity the fallback coalesceKey or comment-and-acknowledge the asymmetry. The missing batch() + no-op guard are sibling band-aids closed in #1471, so the train as a whole is fine; the standalone PR is not. Re-verify if HEAD moves before stamp.

Review by Via

[miga-heygen]

Review — §3.2 timeline trim/move through SDK setTiming

Nice work routing the timing ops through the SDK path, Vance. The sdkTimingPersist function is clean, the hasPbsAdjustment guard is well-reasoned, and the test coverage for the new function is solid. Two things caught my eye that I'd want addressed before this ships:

---

1. Resize drops live DOM playback-start patch (Medium)

useTimelineEditing.ts — handleTimelineElementResize

The old code conditionally pushed data-playback-start / data-media-start into the liveAttrs array for patchIframeDomTiming:

// OLD (removed)
if (updates.playbackStart != null) {
  const liveAttr = element.playbackStartAttr === "playback-start"
    ? "data-playback-start" : "data-media-start";
  liveAttrs.push([liveAttr, formatTimelineAttributeNumber(updates.playbackStart)]);
}
patchIframeDomTiming(previewIframeRef.current, element, liveAttrs);

The new code hardcodes only data-start + data-duration:

// NEW
patchIframeDomTiming(previewIframeRef.current, element, [
  ["data-start", formatTimelineAttributeNumber(updates.start)],
  ["data-duration", formatTimelineAttributeNumber(updates.duration)],
]);

When hasPbsAdjustment is true, the fallback enqueueEdit path fires and buildResizePatches correctly persists the pbs attribute via resolveResizePlaybackStart — but the live DOM preview in the iframe won't reflect the pbs change until the next full reload. During the resize drag, the user would see the element positioned at the old playback offset. This is a UX regression for media elements with playback-start.

Suggested fix: Restore the conditional pbs push into the patchIframeDomTiming call. It's orthogonal to the SDK-vs-server routing — the live DOM patch should always reflect the full update, regardless of which persist path runs.

---

2. Missing reloadPreview() in resize fallback path (Medium)

useTimelineEditing.ts — handleTimelineElementResize, fallback branch:

The old code always called reloadPreview() after persisting the resize — both inside the GSAP-scale .then() and as a standalone return reloadPreview() when timing didn't change:

// OLD
}).then(() => {
  // ... GSAP branch ...
  return reloadPreview();  // <-- always reached
});

The SDK path is fine: persistSdkSerialize calls deps.reloadPreview(). But the new fallback goes straight to enqueueEdit(element, "Resize timeline clip", buildResizePatches) — and persistTimelineEdit (called by enqueueEdit) does NOT call reloadPreview(). So resize-via-server-path no longer triggers a preview reload, meaning the preview could show stale timing until the next manual reload or unrelated edit.

Suggested fix: Chain .then(() => reloadPreview()) on the fallback enqueueEdit call in the resize handler, matching the old behavior.

(The move handler is more defensible — the old code only called reloadPreview inside the GSAP conditional, so the move's fallback path already didn't reload unconditionally.)

---

Non-blocking observations

• fallow-ignore placement: There are two suppression comments per function — one above the declaration and one above the param list. If that's intentional for the fallow gate (function-level + expression-level suppression), no action needed; just flagging in case one set is redundant.
• GSAP removal: The PR description references def54622 for SDK-side GSAP-script sync, and the shiftGsapPositions / scaleGsapPositions imports + call sites are cleanly removed. Makes sense — the SDK handles it now.
• sdkTimingPersist tests: Five tests covering null session, missing element, correct args, before-state capture, and error fallback. Clean and thorough.
• before capture strategy: Using session.serialize() pre-dispatch rather than a server fetch is a pragmatic choice for v1. The PR description's §7 note about potential undo fidelity drift during the transition period is honest and well-documented.

---

Two fixes above, then this is good to go.

— Miga

[james-russo-rames-d-jusso]

Review — §3.2 timeline trim/move through SDK setTiming

Reviewed at 0cd9a68b90cb20f0a39764611793b2295d7d1fe1. Part of the 18-PR SDK-cutover stack review batch (Group B: §3.x + §3.5). Layering on Miga's review — both their findings (resize dropping data-playback-start live patch + missing reloadPreview() on resize fallback) are real and I agree they should ship before merge. One additional thing not yet flagged:

Concern — GSAP-sync regression on the server-fallback path

The PR removes shiftGsapPositions / scaleGsapPositions calls entirely from handleTimelineElementMove / handleTimelineElementResize. The body's rationale (June-13 def54622 lands SDK-side setTiming → GSAP-script sync) is correct — but only on the SDK-success path. When the SDK path declines (!handled → enqueueEdit fallback), persistTimelineEdit writes only the timing attributes (data-start / data-duration / data-track-index) and DOES NOT touch the GSAP script. The old fallback always called shiftGsapPositions / scaleGsapPositions afterwards.

So if sdkSession exists but setTiming declines (e.g. getElement(hfId) returns null because the element is runtime-generated / not in the SDK doc, or setTiming throws), the GSAP positions don't get shifted/scaled to match the new clip timing. Visible drift between clip placement on the timeline and the actual GSAP tween position offsets.

Verified against pr1466head^: shiftGsapPositions(pid, filePath, element.domId, delta) previously ran inside .then() regardless of whether the timing edit succeeded; the new code only relies on the SDK to do this.

Suggested fix: either (a) restore the GSAP shift/scale call inside the if (!handled) branch as a safety net, or (b) explicitly document in a ponytail comment that fallback-path GSAP-sync is now an upstream responsibility and add it to persistTimelineEdit's buildPatches.

Concern (sibling-PR coherence)

Miga flagged the live-DOM pbs patch (resize hardcodes data-start + data-duration only) and the missing reloadPreview() on the resize fallback. Co-signing both — those are user-visible regressions on the fallback path that's about to be the prod path while the cutover flag is off.

Nits

• before = sdkSession.serialize() passed as originalContent to persistSdkSerialize means the edit-history before field is the SDK's in-memory snapshot, not disk content. Acknowledged in §7 open decisions; flagging only because it diverges from sdkDeletePersist (#1465) which uses real disk content. Worth a comment near persistSdkSerialize documenting the divergent semantics across the family.
• The 5 new sdkTimingPersist tests are clean.

Verdict

Two Miga blockers + one regression-on-fallback above. Otherwise the structure is right. Holding for fixes.

— Rames D Jusso

[vanceingalls]

On the matter of #1466 — §3.2: route timeline trim / move through SDK setTiming.

Re-read at HEAD 0cd9a68b, layering atop Miga's review (4518605898) and Rames Jusso's review (4518622880). My earlier Via review at f890eb9b is stale post-rebase and is hereby superseded. Several findings already flagged by Miga and Rames are real and present at this HEAD; one additional finding has surfaced on this read that neither parallel reviewer raised.

Co-signing the Miga-and-Rames findings (all extant at 0cd9a68b)

Miga finding 1 — resize drops live DOM playback-start patch (Medium)

useTimelineEditing.ts — handleTimelineElementResize — the old patchIframeDomTiming call conditionally pushed data-playback-start / data-media-start into the liveAttrs array. The new call hardcodes only data-start + data-duration:

patchIframeDomTiming(previewIframeRef.current, element, [
  ["data-start", formatTimelineAttributeNumber(updates.start)],
  ["data-duration", formatTimelineAttributeNumber(updates.duration)],
]);

When hasPbsAdjustment is true, the fallback enqueueEdit path correctly persists the pbs attribute via resolveResizePlaybackStart, but the live DOM preview in the iframe won't reflect the pbs change until the next full reload. During the resize drag, the user sees the element positioned at the old playback offset. Co-signed as a user-visible UX regression. The fix is orthogonal to SDK-vs-server routing — the live DOM patch should reflect the full update regardless of which persist path is going to run. Restore the conditional pbs push.

Miga finding 2 — missing reloadPreview() in resize fallback path (Medium)

The old resize handler always called reloadPreview() after the persist — both inside the GSAP-scale .then() and as a standalone return reloadPreview(). The new code on the SDK path is fine (persistSdkSerialize calls deps.reloadPreview()), but the fallback goes straight to enqueueEdit(...) and persistTimelineEdit does not itself call reloadPreview. Resize-via-server therefore no longer triggers a preview reload. Co-signed; the fix is the .then(() => reloadPreview()) chain Miga suggests. Move handler is more defensible since the old code only called reloadPreview inside the GSAP conditional.

Rames finding — GSAP-sync regression on the server-fallback path

shiftGsapPositions / scaleGsapPositions calls are removed entirely from both timeline handlers; the PR body's rationale is that #def54622 lands SDK-side setTiming → GSAP-script sync. That's correct on the SDK-success path, but when SDK declines (getElement(hfId) returns null because the element is runtime-generated, or setTiming throws), enqueueEdit runs persistTimelineEdit which writes only timing attributes — the GSAP script is untouched. The old fallback always called shift/scale afterwards. Co-signed: visible drift between clip placement on the timeline and the GSAP tween position offsets.

Either restore the shift/scale call in the if (!handled) branch as a safety net, or document explicitly that fallback-path GSAP-sync becomes an upstream responsibility and thread it into persistTimelineEdit's buildPatches.

The new finding — sdkTimingPersist runs while STUDIO_SDK_CUTOVER_ENABLED is OFF

sdkCutoverPersist at sdkCutover.ts:115 gates the inline-style path on STUDIO_SDK_CUTOVER_ENABLED. The dark-launch contract states (per the rebased PR bodies) that the stack ships the architecture wired but off — at default false, no production traffic flows through the SDK persist seam.

sdkTimingPersist at sdkCutover.ts:139-158 has no such gate:

export async function sdkTimingPersist(...): Promise<boolean> {
  if (!sdkSession || !sdkSession.getElement(hfId)) return false;
  try {
    const before = sdkSession.serialize();
    sdkSession.setTiming(hfId, timingUpdate);
    await persistSdkSerialize(sdkSession, targetPath, before, deps, options);
    ...

And the caller at useTimelineEditing.ts:148-156 and :212-220 fires whenever sdkSession is non-null on an hfId-tracked element:

if (sdkSession && element.hfId) {
  return sdkTimingPersist(element.hfId, targetPath, { start, trackIndex }, sdkSession, ...);
}

useSdkSession is opened unconditionally at App.tsx:154 for every project. Net effect at flag-off (the prod default): inline-style edits route to the server (dark-launch intact), but every timeline move on an hfId-tracked element, and every resize without hasPbsAdjustment, route through the SDK and persistSdkSerialize-write. The architecture flips half-on the moment this PR lands.

This is the same un-gated-by-flag pattern I have flagged on #1465 for sdkDeletePersist. It is band-aid pattern #2 (contradictory rules in the same component) — three persist functions in the same module document the same dark-launch contract differently, and no test exercises the asymmetry (the test mock at sdkCutover.test.ts:14 pins STUDIO_SDK_CUTOVER_ENABLED: true, so the divergence is silent in CI).

The fix is one line — gate sdkTimingPersist on STUDIO_SDK_CUTOVER_ENABLED at the top of the function, mirroring sdkCutoverPersist. Alternatively gate at the two call sites in useTimelineEditing.ts. Function-level is the seam-honest choice.

Either fix this in #1466 directly, or make the asymmetric coverage explicit with a code comment AND a PR-body annotation. Silent divergence is the band-aid; explicit divergence is at least auditable.

The contradictory-coalesce-rule finding I had raised earlier — still extant

From my earlier Via review at f890eb9b, the SDK branch passes coalesceKey: timeline-move:${element.hfId} while the fallback enqueueEdit path carries no coalesceKey. Net: undo granularity depends on which path serves the edit — band-aid pattern #2 again. Either plumb the coalesceKey into persistTimelineEdit's recordEdit call, or explicitly comment-and-acknowledge the asymmetry. Confirmed present at 0cd9a68b.

On the hasPbsAdjustment predicate

useTimelineEditing.ts:208-217 (or thereabouts at this HEAD) — updates.playbackStart != null OR (updates.start !== element.start AND element.playbackStart != null). The second clause exists because a start-trim on an element with playback-start implicitly shifts the playback start. Worth one comment in code explaining why the second clause is needed — six months from now it will read as defensive-paranoia. Non-blocking.

On the before capture semantics (Rames concern echoed)

sdkTimingPersist captures before = sdkSession.serialize() pre-setTiming and passes it as originalContent to persistSdkSerialize, which uses it as the history before. Diverges from sdkDeletePersist's real-disk originalContent. The asymmetry across the persist family is worth a comment near persistSdkSerialize so future maintainers know which family member they're working with. PR body §7 honestly flags the limitation; making it visible in the code adjacent to the call would close the friction.

Sibling closed-in-#1471 items (acknowledged, not re-flagged)

• Missing session.batch(...) wrap on setTiming — same pattern as sdkDeletePersist, closed in #1471.
• Missing before === after no-op detection — same pattern, closed in #1471.

The 600-line file-gate sidenote

The single blank-line removal in App.tsx at line 363 to stay under the file-size gate is brittle infrastructure. It'll fire on the next PR that touches App.tsx and force another arbitrary cosmetic mutation. Not a request-changes; it is the kind of friction worth flagging upstream as a process irritant.

Verdict

request-changes at 0cd9a68b on the cumulative findings — three real fixes that should ship before merge:

1. The un-gated sdkTimingPersist dark-launch divergence (new in this review).
2. The resize live-DOM pbs patch drop (Miga finding 1).
3. The missing reloadPreview() on the resize fallback (Miga finding 2).

Plus Rames's GSAP-sync regression on the server fallback (restore the shift/scale call or thread it through persistTimelineEdit). The coalesce-rule asymmetry across SDK / fallback paths is its own band-aid-#2 finding from my earlier review and still extant. The structure of the SDK timing path itself is sound — it is the standalone-PR correctness that needs attention before this lands.

Re-verify if HEAD moves before stamp.

Part of the Group A batch on the 18-PR SDK-cutover stack. See also #1522 (persist contract — request-changes), #1524 (force-reload race — request-changes), #1462 (clean teardown), #1463 (persist wiring), #1465 (delete path — same un-gated-by-flag finding applies).

Review by Via

[vanceingalls]

Review findings addressed in #1539 (landed off the stack tip — everything is flag-gated, so a mid-stack restack wasn't needed):

• Resize dropped live data-playback-start/media-start patch + move/resize fallback lost GSAP-position sync (shift/scaleGsapPositions) + reloadPreview, undo-coalesce drift → restored both fallback entry points + plumbed coalesceKey (commit fb759b54b).
• SDK setTiming collapsed multi-tween position/duration (absolute, not per-tween delta/ratio) → now shifts by start-delta + scales by duration-ratio per tween (commit 7d7732c8f), with a correctness test (2 tweens at 2.0/5.0 → 3.0/6.0 on +1 move, stagger preserved).

[miguel-heygen]

Approved as part of SDK cutover stack. Reviewed by Miga, Rames D Jusso, and Via across R1-R4. LGTM.

[jrusso1020]

Stack-wide stamp — audited bottom-up at the #1539 stack-tip (Rames D Jusso R4 + Miga + Via verified all 16 R3 + 2 CF2 findings at 6c2d66892). SDK-cutover chain cleared end-to-end.

The base branch was changed.