Repository navigation
fix(studio): update unreferenced generated media names on replace - #5169
miguel-heygen wants to merge 6 commits into
Conversation
jrusso1020
left a comment
There was a problem hiding this comment.
Reviewed at 22ad7a13.
The filename decoding is careful and well tested, and the stale "Harbor" after a replace is a real bug. My concern is with how the fix gets there: it moves the filename ahead of the element id for every video, audio and image clip (timeline.ts:237-240), not only clips whose id Studio generated. So hand-written ids lose their labels too.
Blocker: authored media ids get renamed. In HyperFrames the id is often the name the author picked, especially in agent-written HTML. I ran it in the runtime test environment:
<video id="a-roll" src="video.mp4" data-start="0" data-duration="20"></video>
<audio id="voiceover" src="assets/tts_8f3a2c.mp3" data-start="0" data-duration="10"></audio>- main:
["A Roll", "Voiceover"] - this head:
["Video", "Tts 8f3a2c"]
That exact a-roll <- video.mp4 / a-roll <- source.mp4 shape is in our own skills/hyperframes-registry/references/wiring-blocks.md:11 and skills/embedded-captions/scripts/make-theme.cjs:583. So the PR body's "a name you typed yourself stays" holds for data-timeline-label / data-label / aria-label, but not for an id typed by hand.
There is also a consistency gap. Studio's own DOM label path still puts the id first (getTimelineElementDisplayLabel, used by parseTimelineFromDOM and the standalone-root fallback). After this change, the two paths name the same clip differently.
Suggested direction (smaller, and fixes the cause): the stale label only happens because Studio's drop generates the id from the filename (buildTimelineAssetId in studio/src/utils/timelineAssetDrop.ts). Then a replace rewrites src but has to keep the id. The Studio write that sets the new src knows the old one. If the id is what buildTimelineAssetId gives for the old file (its slug, optionally _N), the name was generated, and that write can name the clip after the new file. Leave the runtime's label order as it is. Hand-authored ids keep their names, and both label paths stay in agreement. If you'd rather keep the runtime change, it needs a way to tell a generated id from an authored one. Unconditional filename-first is the part I'm objecting to.
Minor: if the runtime change stays, the later filenameFromAssetUrl fallback (timeline.ts:251-252) is now only reachable for non-media kinds. Worth folding into the one branch.
Tests: src/runtime/timeline.test.ts passes 81/81 at this head.
Verdict: REQUEST CHANGES
Reasoning: The fix repairs Studio-generated ids by renaming every media clip after its file, which changes labels authors chose (a-roll becomes "Video"), and that case appears in our own skills.
— Rames
Edit accuracy: accurate 2059 (base branch 2059), smooth 1589 of thoseThe gate passes. Quarantined, measured but not gated (0) |
|
Addressed at 50e8d82. Runtime labels retain authored ID precedence. The source writer now re-mints only an unreferenced ID matching the old source’s generated slug (including collision suffixes); explicit labels and other IDs remain unchanged. Studio insertion, SDK edits, and server edits share the existing mint and replacement rule. Regression coverage includes authored hero-shot, two replacements, undo, and document references. Full lint, audit, package/runtime typechecks, and focused tests pass. The refreshed Before/After captures show real Studio edits and preview reloads on this head, with Library then Sunset while the authored and referenced names stay fixed. |
jrusso1020
left a comment
There was a problem hiding this comment.
Re-review at 50e8d826. My blocker from 22ad7a13 is resolved, so I'm approving.
Prior blocker: authored ids were renamed by filename-first labels. timeline.ts no longer puts the filename ahead of the id. The runtime change is now only the filename decoding, so an authored a-roll on video.mp4 keeps its label. The rename moved to the source writer, where it belongs. replacementTimelineAssetId re-mints the id only when all of these hold:
- the id equals the old file's generated slug, or the slug plus a collision suffix;
- there's no explicit label;
- nothing in the document references the id.
That's the shape I suggested, and it reuses the existing buildTimelineAssetId, now moved to core instead of copied.
What I checked
- Both persistence paths are covered. The SDK cutover path is
handleSetAttributeinmutate.ts, and the server patch path ispatchElementInHtmlinsourceMutation.ts. Those are the two branches inuseDomEditPersist, so everysrcwrite from Studio goes through one of them, including the new img Fill write from #5170. - The SDK keys its patch paths by hf-id (
attrPath(id, …)withid: HfId), and the server stamps hf-ids into every composition. So the selection still resolves after the id changes, and the id change rides in the same forward and inverse patch for undo. - The reference check is deliberately loose: any substring hit in a script, a style or another attribute blocks the rename. A false positive only means no rename, which fails safe.
- Locally, after building core for the new
./timeline-asset-idexport, these pass: coretimelineAssetIdandtimelineMediaLabels(12), sdkmutate.test.ts(114), studio-serversourceMutation.test.ts(71) and studiotimelineAssetDrop(34).
Notes (non-blocking)
- The reference check only sees the edited document. An id used only from another file is invisible to it, so the rename would silently orphan those references. Two cases: a parent composition's script targeting
#harborinside a sub-composition, or an external script such as the<script src="assets/glass-main.js">kind some registry examples use. The exposure is small, because the id has to be Studio-generated and then hand-referenced only from elsewhere. It's worth a comment atreplacementTimelineAssetIdsaying the check is single-document. - An authored id that happens to equal the file's slug (
<video id="intro" src="intro.mp4">) is treated as generated and follows the new file. That's indistinguishable from a Studio drop, and following the file is a reasonable outcome. I'm noting it so it's a known choice. - Nit: in
mutate.ts, the new import sits above the file's doc comment.
Verdict: APPROVE
Reasoning: The authored-id regression is gone because the label precedence is unchanged and the rename is limited to provably generated, unreferenced ids, applied in both writers and covered by tests.
— Rames
Media replacement leaves a generated timeline name tied to the old source. The source writer now re-mints a media ID when it matches the old filename's generated slug, including collision suffixes, and the document has no references to it. IDs that do not match that generated slug, explicit labels, and referenced IDs stay unchanged. Runtime labels retain authored ID precedence.
The existing ID mint is shared by Studio insertion, SDK edits, and server edits. SDK forward and inverse patches include the ID change while preserving the stable edit identity. Reference checks cover scripts, CSS, other attributes, templates, escaped identifiers, and source URL fragments. Ordinary asset paths and identity definitions do not count as references.
Validation: replacement and decoding witnesses fail on main; the authored-ID cases fail against the earlier filename-preference implementation. Tests cover two replacements, collisions, undo, authored labels, and reference preservation. Removing the reference guard makes nine witnesses fail. Full lint, package-subpath verification, audit, formatting, and package/runtime typechecks pass locally.
Before
After replacement on the baseline, the generated name remains Harbor. Authored Hero Shot, referenced Marina, and explicit Opening Shot are preserved.
After
Real HTTP edits through Studio's source writer and preview reload change Harbor to Library. The authored and referenced names remain unchanged.
A second replacement updates Library to Sunset through the same stable edit target.
Known baseline gate red: Studio: timeline viewport gate, the overscan regression tracked in #5151.