Skip to content

fix(studio): only expose front trim for offsettable clips#413

Merged
miguel-heygen merged 5 commits intomainfrom
fix/studio-front-trim-affordance
Apr 22, 2026
Merged

fix(studio): only expose front trim for offsettable clips#413
miguel-heygen merged 5 commits intomainfrom
fix/studio-front-trim-affordance

Conversation

@miguel-heygen
Copy link
Copy Markdown
Collaborator

@miguel-heygen miguel-heygen commented Apr 22, 2026

Summary

  • hide the leading trim handle for timeline clips that cannot offset their own content
  • keep leading trim available for media clips backed by playback offset metadata or source duration
  • map visual row priority like a normal timeline editor: top timeline rows render above lower rows

Why This Is Needed

Generic GSAP/DOM timeline clips do not have a playback-offset model like media clips do.

That means a left trim affordance on those clips is misleading today:

  • users reasonably expect front trim to remove the beginning of the animation
  • the current model can only shorten the clip window, not start the motion halfway through

Instead of exposing a control that implies unsupported behavior, this PR keeps true front trim only on clips that can actually offset their content.

The PR also fixes the stacking convention so the timeline matches normal editor expectations:

  • visually higher track row = higher render priority
  • visually lower track row = lower render priority

Current Flow By Element Type

Generic motion / DOM clips

Examples: section, div, aside, GSAP-driven cards and overlays.

Current supported flow:

  • drag the whole clip horizontally to change data-start
  • right-trim to shorten the end of the clip window
  • move between tracks to change data-track-index

Not supported yet:

  • true front trim that removes the beginning of the animation itself

Behavior after this PR:

  • no interactive left trim handle is shown
  • right trim still works
  • horizontal move still works

Media clips

Examples: video / audio clips, or wrappers carrying data-media-start / data-playback-start.

Current supported flow:

  • drag the whole clip horizontally to change data-start
  • left trim advances clip start and playback offset together
  • right trim shortens data-duration

Behavior after this PR:

  • both left and right trim handles remain available
  • left trim persists data-start plus data-media-start / data-playback-start
  • right trim persists data-duration

Z-Index Rule

This PR now follows the normal timeline-editor convention:

  • top visual row on the timeline = highest z-index
  • lower visual rows = lower z-index

Concretely, because Studio renders tracks in ascending numeric order from top to bottom, lower numeric track values now map to higher z-index values.

Validation

Automated

  • bun test packages/studio/src/player/components/timelineEditing.test.ts packages/studio/src/player/components/Timeline.test.ts packages/studio/src/player/store/playerStore.test.ts packages/studio/src/utils/sourcePatcher.test.ts
  • bun run --filter @hyperframes/studio typecheck

Browser verification

Verified with agent-browser on timeline-edit-playground:

  • generic motion clips no longer expose an interactive left trim handle
  • media clips still expose both trim handles
  • left trim on media-card persisted data-start and data-media-start
  • right trim on media-card persisted data-duration only
  • moving title-card from the bottom row to the top row persisted the highest z-index for the top-row clips
  • recordings:
    • /tmp/trim-fix-artifacts/trim-flow.webm
    • /tmp/trim-fix-artifacts/z-index-flow.webm

@miguel-heygen miguel-heygen marked this pull request as ready for review April 22, 2026 14:24
Copy link
Copy Markdown
Collaborator

@jrusso1020 jrusso1020 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM — clean, well-scoped fix with good defense-in-depth.

What I checked

  • Predicate (timelineEditing.ts canOffsetTrimClipStart): logic is correct. playbackStart != null properly allows playbackStart === 0, sourceDuration > 0 rejects zero-length sources, and tag.toLowerCase() is safely defensive.
  • Preview vs. source consistency: getPreviewElement only overrides start/duration/playbackStart, preserving tag/playbackStartAttr/sourceDuration — so the UI check in TimelineClip (on previewElement) and the handler guard in Timeline.tsx (on el) will always agree.
  • Defense in depth: hiding the handle visually and early-returning from onResizeStart is the right call — either one alone would be brittle.
  • Test rename of buildTrackZIndexMap from "maps sorted tracks onto stable positive z-index values" → "maps higher track numbers onto higher z-index values" is a real improvement: describes the invariant instead of the mechanism.

Minor, non-blocking

  1. Consider a one-line comment at the handler guard so a future refactor doesn't delete it as "redundant":
    // Defense in depth: the handle is also hidden in TimelineClip when this is false.
    if (edge === "start" && !canOffsetTrimClipStart(el)) return;
  2. No integration test for the handler guard — the predicate unit tests + browser verification are probably sufficient for a visual-affordance change, but a cheap Timeline.test.ts case asserting the early-return for a section clip would be belt-and-suspenders.

Nice browser verification artifact too. Ship it.

@miguel-heygen miguel-heygen merged commit 2cf3558 into main Apr 22, 2026
20 checks passed
Copy link
Copy Markdown
Collaborator Author

Merge activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants