Skip to content

fix(studio): warn on anonymous timeline clips#533

Merged
miguel-heygen merged 3 commits intomainfrom
fix/studio-editable-id-warnings
Apr 30, 2026
Merged

fix(studio): warn on anonymous timeline clips#533
miguel-heygen merged 3 commits intomainfrom
fix/studio-editable-id-warnings

Conversation

@miguel-heygen
Copy link
Copy Markdown
Collaborator

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

Problem

Studio timeline editing still had two rough edges that made the latest alpha feel less polished when testing it like a video editor would:

  • Timeline clips for anonymous DOM nodes could surface internal fallback identities like __node__index_*, which made the timeline look broken instead of authored.
  • Elements without a stable id could still appear in the timeline and canvas editor, but authors did not get direct lint guidance that those elements are weaker targets for Studio and agent edits.

What this fixes

  • Adds a non-blocking studio_missing_editable_id lint warning for timeline-visible elements that do not have an id.
  • Makes the warning point to the exact element and recommend stable, human-readable ids such as hero-title or scene-1-card.
  • Stops using synthetic node-index ids as runtime clip identity for anonymous DOM nodes.
  • Gives anonymous clips readable labels from authored metadata, composition ids, DOM ids, class names, asset filenames, text content, or a simple ordinal fallback.
  • Keeps those labels display-only in Studio and uses key-first identity for matching, dragging, resizing, and manifest merge preservation.
  • Covers the duplicate-label case where two anonymous clips both render as Card but still stay separate timeline entries.

Root cause

The runtime manifest used synthetic node-index ids as both identity and display fallback for timeline nodes that had no stable author-provided id. Studio then treated those internal values as user-facing clip names.

The first pass improved the display label, but it also risked using that friendly label as internal identity. Two anonymous clips with the same label could then collapse into the same logical timeline element. The fix separates display labels from internal identity and prefers the timeline key whenever Studio needs to match an element.

The linter also had correctness checks for render and runtime behavior, but it did not teach authors when a timeline-visible element would be harder for Studio and agents to patch reliably. That left missing ids as a silent authoring quality issue instead of actionable guidance.

Verification

Local checks

  • bun run --cwd packages/core test -- src/lint/rules/core.test.ts src/runtime/timeline.test.ts -> 41 tests pass
  • bun run --cwd packages/studio test -- src/player/hooks/useTimelinePlayer.test.ts src/player/components/timelineTheme.test.ts -> 23 tests pass
  • bun run --cwd packages/core typecheck
  • bun run --cwd packages/studio typecheck
  • bun run --cwd packages/studio build -> passes with the existing Vite chunk-size warning
  • bunx oxlint $(git diff --name-only origin/main...HEAD) -> 0 warnings, 0 errors
  • bunx oxfmt --check $(git diff --name-only origin/main...HEAD)
  • git diff --check origin/main...HEAD

Browser verification

  • Created a scratch project at /tmp/hf-pr533-conflict-verify with two timed anonymous .card clips that both label as Card.
  • Started the local Studio dev server for pr-533-conflict-verify.
  • Used agent-browser to verify the timeline renders two separate Card clips instead of collapsing duplicate anonymous labels.
  • Used agent-browser to open the Studio lint modal and verify it shows human-readable missing-id warnings, not internal node-index labels.
  • Used agent-browser to click Play after the lint pass and confirm the timeline remains usable.
  • Recorded the tested Studio flow with agent-browser.

Notes

  • Rebased onto current main; conflict resolution preserved both the newer mainline Studio shortcut/lint behavior and this PR's anonymous-clip identity split.
  • GitHub Actions are running on the rebased head.
  • Scratch verification files are intentionally not committed.
  • Local screenshots and recording from this rebase pass are under .codex-artifacts/pr-533-conflict-rebase-2026-04-29/.

@miguel-heygen miguel-heygen marked this pull request as ready for review April 28, 2026 19:50
Copy link
Copy Markdown
Collaborator

@vanceingalls vanceingalls left a comment

Choose a reason for hiding this comment

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

Staff review: requesting changes.

The display-label fix is the right direction, but this still conflates user-facing labels with internal timeline identity. In packages/studio/src/player/hooks/useTimelinePlayer.ts, anonymous clips now flow through id = clip.id || label and id = el.id || compId || label. That means common timelines with two anonymous clips sharing the same class/label, for example two .card clips, can produce duplicate TimelineElement.id values.

That is risky because other Studio paths still treat id as identity: mergeTimelineElementsPreservingDowngrades builds a Set from element.id, selection/update paths fall back to key ?? id, and several edit/error paths still report or compare element.id. The new label field solves the user-facing name problem, so the internal identity should stay unique or all identity-sensitive paths need to be key-first throughout.

Please add coverage with two anonymous clips that generate the same friendly label and verify they remain distinct through runtime-manifest sync/fallback parsing and are not merged, dropped, or updated together.

@miguel-heygen miguel-heygen force-pushed the fix/studio-editable-id-warnings branch from 788ed66 to e4fd0d7 Compare April 28, 2026 20:54
@miguel-heygen miguel-heygen changed the base branch from next to main April 28, 2026 20:55
@miguel-heygen
Copy link
Copy Markdown
Collaborator Author

@vanceingalls all was addressed btw.

@miguel-heygen miguel-heygen force-pushed the fix/studio-editable-id-warnings branch from e4fd0d7 to 90e9027 Compare April 29, 2026 20:56
Copy link
Copy Markdown
Collaborator

@vanceingalls vanceingalls left a comment

Choose a reason for hiding this comment

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

P1: Root timeline-visible compositions can still trigger studio_missing_editable_id.

The new root skip compares tag.index with rootTag.index, but those indexes come from different strings. tags are extracted from the full source, while findRootTag() extracts from bodyContent, so a normal <html><body>... document does not match. Result: the composition root can get studio_missing_editable_id if it has data-start, even though the rule tries to skip it. The new test around core.test.ts:111 should catch this. Compare by tag.raw, return source-relative indexes from findRootTag, or identify the root from the same tags array.

P2: Cross-window media metadata is still skipped.

resolveMediaElement() now uses the iframe owner window constructors, but applyMediaMetadataFromElement() still checks mediaEl instanceof HTMLMediaElement against the parent window. For preview iframe media this remains false, so sourceDuration and playbackRate are not populated. That weakens the trim bounds that depend on those fields in Timeline.tsx:749. Use the media element owner-window constructor for this check too.

@miguel-heygen
Copy link
Copy Markdown
Collaborator Author

@vanceingalls addressed both review follow-ups in 67efe673.

  • Fixed findRootTag() to return source-relative indexes when it extracts the root from <body>, so the studio_missing_editable_id root skip compares indexes from the same source string and the existing root-with-data-start regression now passes correctly.
  • Fixed applyMediaMetadataFromElement() to use the media element owner-window HTMLMediaElement constructor, matching the cross-window logic in resolveMediaElement(), so iframe media now keeps sourceDuration and playbackRate metadata for trim bounds.
  • Added regression coverage for owner-window media metadata in useTimelinePlayer.test.ts.

Verified:

  • bun run --cwd packages/core test -- src/lint/rules/core.test.ts
  • bun run --cwd packages/studio test -- src/player/hooks/useTimelinePlayer.test.ts
  • bun run --cwd packages/studio typecheck
  • bunx oxlint packages/core/src/lint/utils.ts packages/core/src/lint/rules/core.test.ts packages/studio/src/player/hooks/useTimelinePlayer.ts packages/studio/src/player/hooks/useTimelinePlayer.test.ts
  • bunx oxfmt --check packages/core/src/lint/utils.ts packages/core/src/lint/rules/core.test.ts packages/studio/src/player/hooks/useTimelinePlayer.ts packages/studio/src/player/hooks/useTimelinePlayer.test.ts
  • git diff --check

Re-requesting review now.

@miguel-heygen miguel-heygen merged commit 39b3997 into main Apr 30, 2026
41 checks passed
Copy link
Copy Markdown
Collaborator Author

Merge activity

@miguel-heygen miguel-heygen deleted the fix/studio-editable-id-warnings branch April 30, 2026 05:07
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