fix(studio): the server picks an added effect's element id; one client for patch and gsap writes

[miguel-heygen]

What

"Add effect" on an element with no id of its own no longer lets two elements share an id, and Studio's patch-element and gsap-mutations writes each go through one client.

1. The server picks the new id. Studio used to choose the id from the live preview, set it on the element, and remove it again when the write failed. The preview and the saved file can disagree, which caused two bugs. A new patch-element operation, ensure-id, fixes both: the server keeps the element's existing id, or writes the proposed id made unique against every HTML file in the project. Sub-compositions share one preview document, so an id another composition file holds counts as taken. The scan, the patch and the write run in one synchronous step, so two adds in different files can't both pick the same id. Every patch-element reply now names the element's id (elementId). Studio sets that id on the preview only once the reply arrives, so nothing needs rolling back. This is the same split already used for clip split, duplicate and freeze frame, where the server picks the id.
2. patch-element client. assignGsapTargetAutoIdIfNeeded built its own file-mutations/patch-element request. It now posts through postPatchElement and returns the id the file holds, or null. A refusal is toasted with the server's reason. A reply naming no id (the element isn't in the file) now toasts "Couldn't assign element id: element not found in "; main returned silently there.
3. gsap-mutations client. mutateGsapScript, mutateGsapScriptBatch and postGsapMutation each built the same POST. requestGsapMutation in gsapMutationClient.ts is now the one request, and each caller keeps its own response handling.
4. One unique-id helper on the server. nextUniqueId in sourceMutation.ts now serves split, duplicate, freeze frame, group and ensure-id. Before, there were four copies of the same base, base-2, ... loop.

Why

Review of the first version found both id bugs:

• A lost reply after a saved id. The file kept the id while the preview dropped it, so the next add on a sibling saved the same id again: two elements with id="div".
• Two overlapping adds on the same element. The first add's refusal stripped the id the second add had just saved.
• Two overlapping adds in different composition files (found at 67381aaf). Each file was deduped alone, so both could save id="div" into one preview. A lost reply in one file did the same to a later add in another.

Both bugs come from one place: the code choosing the id could not see the file. Two copies of one request also drift apart; they already handled a refusal differently.

How

• The refused-id toast now uses the patch client's message. With a server reason, "Couldn't save animation: " becomes "Couldn't save edit: ". Without one, "Couldn't save animation: GSAP mutation failed with status " becomes "Couldn't save edit".
• Studio and the studio server ship together, so no older server ever receives ensure-id.
• Known limit: the server makes the id unique across the project's HTML files, not against the live preview. An id that exists only in the preview (added by a script at runtime) can still repeat until the next reload. Nothing is written twice to the project.
• Every add effect on an id-less element now reads the project's HTML files once. That is cheap at today's project sizes; a cache keyed on file versions is the upgrade if it ever shows up in a profile.

Before

An id-less element whose id write the server refuses (a forced 409 "file changed" on patch-element), on main:

After

The same refusal on this branch (captured before the ensure-id change; the refusal path and its toast are unchanged by it):

Test plan

• useGsapAnimationOps.test.tsx replays each sequence through the real patchElementInHtml against an in-memory file:
  • The lost reply then the sibling add leaves div and div-2, and the sibling's effect targets #div-2. With the previous Studio code the file held div twice.
  • The refused-then-saved overlapping adds on one element leave the preview id equal to the file's. With the previous Studio code the preview had no id while the file held div-2.
  • Overlapping adds on siblings never share an id, and a saved, refused, network-failed or no-project add leaves the preview matching the file.
• Server tests for ensure-id:
  • sourceMutation.test.ts: writes the proposed id, makes it unique against the file, keeps an existing id unchanged, reports no id for a missing target.
  • files.test.ts: the route returns elementId on both the changed and the unchanged reply, and an id saved in compositions/b.html makes the same proposal in index.html save as div-2. That test fails at 67381aaf, and fails again if the scan is dropped or limited to root files. A second test puts ids in another file straight after a quote and after a string ending in "/id=; both must count.
  • All of these fail on the previous server.
• useDomEditCommitsHelpers.test.ts: request shape (method, write token, ensure-id body), the returned id, null plus a toast for a missing target or a reply without an id, the server's reason on a refusal, a non-finite target refused before any request, a network failure passed through.
• useGsapScriptCommits.test.tsx: the write token on the gsap request, and a refused write's reason reaching a toast. Removing the token or the refusal check now fails a test (both passed the whole hooks suite before).
• The touched server and studio test files pass 3 runs in a row. Type checks for studio and studio-server are clean, and oxfmt, oxlint, fallow, comment citations and the comment ratchet all pass.
• Before/After: built Studio on main and on this branch, patch-element answered with a 409 by request interception, "+ Add effect" on the first of two id-less .card elements.

Follow-ups, not in this PR:

• A refused id write still resolves addGsapAnimation normally, so the add handler resets an element's manual path offset although no effect was added (useGsapSelectionHandlers.ts).
• file-mutations/remove-elements still has two inline clients (useTimelineDeleteOps.ts, useElementLifecycleOps.ts) that handle a refusal differently.
• No test makes a freeze-frame or group id collide. Both now go through the shared nextUniqueId, which the ensure-id and split tests pin.

[github-actions]

Edit accuracy: accurate 1556 (base branch 1556), smooth 1417 of those

The gate passes.
Smoothness is reported in the artifact, not gated. A case fails only if it fails 2 of 3 runs.

Quarantined, measured but not gated (0)

Unstable (1)

• crop-idle-center-r30-nested-z200: tracking 0.02, pressJump 0, drop 40.03, reload 40, render 0.26, renderKey -, undo false, teleport true / tracking 0.02, pressJump 0, drop 0.04, reload 0.04, render 0.26, renderKey -, undo true, teleport true / tracking 0.02, pressJump 0, drop 0.04, reload 0.04, render 0.26, renderKey -, undo true, teleport true

[somanshreddy]

Reviewed at 497ed032.

This is a comment, not an approval. I found no blockers. The refactor preserves behaviour on the three routes it touches, and the rollback closes the refused, network and no-project cases it names. I have one real gap in the rollback and two smaller notes.

Findings, most severe first

1. The rollback can strip an id that a second, saved add wrote (low-medium, nonblocking). useGsapAnimationOps.ts:143 removes the id with no condition: if (!assigned) selection.element.removeAttribute("id");. Here is how that goes wrong. Two adds run on the same element from the same selection, which the PR body already lists as reachable. Add 1 mints div on the element. Add 2 still sees selection.id as undefined, so it mints div-2 over it (gsapScriptCommitHelpers.ts:37). If add 1's write is then refused while add 2's is saved, add 1's finally removes div-2. The file holds div-2 and a tween for #div-2, but the preview element has no id. The next "Add effect" on that element then mints and writes a new id over div-2, which leaves that tween targeting nothing. That's the same stray-id problem, coming from the other direction.
I reproduced it with a scratch test in useGsapAnimationOps.test.tsx's harness: add 1's fetch is held and then answers 409, add 2's answers {changed: true}. The request bodies were ["div", "div-2"] and element.id was div-2 before add 1 settled. After it settled, I got expected null to be 'div-2'.
The fix is one line: if (!assigned && selection.element.getAttribute("id") === autoId) selection.element.removeAttribute("id");. With it the scratch test passes, and so do the 17 tests in the two changed files. It also means a rollback can never remove an id this call didn't put there. A separate Codex pass found this race on its own and rated it blocking. I rate it lower because it only happens inside the same-element double add, which the PR body already says is broken (it orphans the first tween even without a refusal). Still, it's a one-line fix, so I'd take it in this PR along with a same-element test. The existing overlap test uses two different elements.

2. "One client per write route" is broader than what this PR delivers (low). The title and the first line of "What" say every Studio write route now has one client. The diff consolidates two: patch-element and gsap-mutations/-batch. file-mutations/remove-elements still has two inline clients, at useTimelineDeleteOps.ts:134 and useElementLifecycleOps.ts:160. They already handle a refusal differently: one throws a plain Error, the other throws createStudioSaveHttpError. That's the same drift your "Why" describes. useFileManager.ts:253/277 also post to /files/ twice. I'd either narrow the title to the two routes or note remove-elements as a follow-up.

3. Two guards on the shared GSAP client aren't pinned by any test (low). Removing ...studioWriteHeaders() from requestGsapMutation (gsapMutationClient.ts:101) still passes 1397/1397 tests across all of src/hooks. So does removing the !res.ok branch from the merged mutateGsapScript (useGsapScriptCommits.ts:56). Both used to exist in three copies, so this gap isn't new, but the write token now lives in exactly one place. Losing it isn't cosmetic. studioWriteHeaders (utils/studioFileVersion.ts:74) is what stops the server echo of Studio's own write from looking like an external change. Without it, every GSAP edit would trigger a full preview reload, which shows up as a flash. One fetch-spy assertion on the header would catch the first. A refused batch or single-route response asserting GsapMutationHttpError (or the toast) would catch the second.

4. Nit: route and body aren't tied together in the type. requestGsapMutation(projectId, route, filePath, body: unknown) (gsapMutationClient.ts:91) lets a caller pair gsap-mutations-batch with a single mutation, or the other way round. The old batch helper built the { mutations } envelope itself. Today's one batch caller is pinned by a test (M8 below), but an overload or a discriminated union would make the wrong pairing a compile error. Codex raised this; I confirmed the signature.

5. Nit. The id write's URL is now built with buildProjectApiPath (useDomEditCommitsHelpers.ts:185), which throws Invalid project ID synchronously. On an invalid id the old code sent the request and showed a toast; now the call rejects without a toast, and the finally still removes the id. In practice pid comes from the same ref useDomEditPersist already passes to postPatchElement, so I don't think this is reachable. I'm noting it only because it's a change in behaviour.

Mutation table

Runner: vitest with NODE_ENV=test. Each mutant ran first against the six related hook files (158 tests). The survivors were re-run against all of src/hooks (145 files, 1397 tests, control run green).

#	Mutation	Result
M1	Remove the rollback (removeAttribute in finally)	caught: refused, network and no-project tests
M2	Move if (!pid) return above the try	caught: no-project test
M3	Send the id write through an inline studioApiFetch (same URL, body, token)	caught: both toast-copy tests
M4	Rethrow the already-toasted refusal instead of returning false	caught: 3 tests
M5	Drop || result.matched === true	caught: "file already holds the id"
M6	Disable the unsafe-value guard	caught: non-finite target test
M7	Send the batch to gsap-mutations	caught: useGsapScriptCommits batch test
M8	Send the batch body as a bare array instead of { mutations }	caught: same test
M9	Drop !res.ok throw in mutateGsapScript	survived (1397/1397)
M10	Drop the write token from requestGsapMutation	survived (1397/1397)
M11	postGsapMutation through a byte-identical inline client	survived, as expected: it's equivalent, so "one client" can't be checked by a behavioural test
M12	Rethrow raw transport errors from postGsapMutation	caught: timelineTimingSync transport test
probe	Same-element overlap, first refused, second saved	fails at head (finding 1); passes with the guarded rollback

Things I checked that hold up

• The request is identical before and after for gsap-mutations, gsap-mutations-batch and patch-element: same method, URL encoding, Content-Type, write token, and JSON body (batch is still { mutations }). Each caller keeps its own response handling. A synchronous throw in requestGsapMutation still lands inside postGsapMutation's try.
• The matched rule is sound against the current server. patch-element (studio-server/src/routes/files.ts:3036) returns matched: true, changed: false only when the patched HTML equals the original. id is in ALLOWED_HTML_ATTRS and isn't URI-bearing, so html-attribute never silently skips it. Today, matched && !changed therefore means the file already holds that id. That depends on the server's skip behaviour, so if id ever leaves the allowlist, this rule needs another look.
• Correction (2026-10-04), after tai's 5403728750: I originally wrote here that a dropped response after the server saved the id recovers on its own. That's only true for the same element. The mint checks uniqueness against the live preview only (while (doc.getElementById(id)), gsapScriptCommitHelpers.ts:33). Once the rollback strips the saved id from the preview, the next "Add effect" on a sibling can mint the same id. The source then holds two elements with that id, and the write-token echo suppresses the reload that would show it. So the network-lost-response path needs reconciliation, such as a re-read of the file or server-side uniqueness, not just the guarded rollback from #1. I confirmed the mint code at source; tai traced the sequence, and I haven't run it.
• There is one id-mint site (ensureElementAddressable) and one caller of it (addGsapAnimation). A saved id stays on the element even if the tween commit afterwards fails, which is right because the file holds it. The id write records no history entry, before or after. Serialisation through serializeStudioFileMutation and the SDK skip on autoId are unchanged.
• The toast copy change is the one you documented: "Couldn't save animation: …" becomes "Couldn't save edit: …".
• CI: every check at 497ed0329 has completed, with each one passing or path-skipped (Windows, skills, catalog, perf shards). None are red or still running. The edit-accuracy gate shows accurate 1556 vs base 1556, with 0 regressed, unstable or newly passing. The base it read is b71aa9338, which is the latest commit to touch baseline.json (main is one commit ahead and doesn't change it), so the rebase claim holds. Because the ratchet only flags pass→fail flips, I also compared the measured per-case results.json against base baseline.json. No case/metric pair (tracking, drop, reload, render) moved more than 0.05 px across the 1562 cases, and the 6 already-failing crop-scale render cases match exactly (0.99 ×4, 0.78 ×2). Keep in mind the bench measures drag gestures, not "Add effect", so it doesn't test the id path either way.

What I ran

• bun install --frozen-lockfile, then built parsers, lint, studio-server and core.
• The two changed test files: 17/17. The 13 hook files around them: 246/246. All of src/hooks: 1397/1397.
• The 12 mutants and the overlap probe above, each reverted afterwards.
• gh pr checks and per-job check-runs at the head SHA, and the edit-accuracy gate artifact.
• A static Codex pass (codex exec, read-only, raw diff and PR body only, none of my findings) ran and completed in about 7 minutes. It returned three findings. I checked each one at source: the rollback race (it matches my #1, found separately), the untested write token (matches my #3), and the route/body typing (#4).

— Somu

[terencecho]

The consolidation of patch-element and GSAP mutation requests preserves their write headers and payload shapes. I independently checked Somu's COMMENT-only review and found a second consequence of the new rollback that makes this a request for changes, not a safe merge-gate stamp.

Blocker — a lost response after a successful id write can persist duplicate HTML ids (packages/studio/src/hooks/useGsapAnimationOps.ts:123–143, packages/studio/src/hooks/gsapScriptCommitHelpers.ts:30–38). Add an effect to id-less element A when its class matches a sibling: the client mints div, the server saves it, but the browser's request rejects before receiving the response. assigned remains false and the new finally removes A's live id, even though its source file has id="div". The self-write token suppresses the file-change reload (studioFileVersion.ts:65–77, useExternalFileChangeCoordinator.ts:424–433). A subsequent add on sibling B therefore sees no live div, mints div again, and the server's HTML-attribute mutation (sourceMutation.ts:279–284) accepts it without an id-uniqueness check. The saved file now has two id="div" elements and #div is no longer an unambiguous animation target. On the base version, A retained its live id after the lost response, so B would mint div-2. A failure before a server write needs rollback; a failure after the write has unknown outcome and must reconcile the persisted source/reload before freeing the id, or the server must reject duplicate ids. Please test a post-commit response failure followed by adding to the sibling.

Somu also reproduced the separate same-element overlapping-add case in <https://github.com/heygen-com/hyperframes/pull/4962#pullrequestreview-5403582444|their review>: the first failed add's unconditional removeAttribute can erase the second add's newly minted div-2, which the queued second write subsequently saves. The file writer queue serializes network writes, not the synchronous DOM id mint; the GSAP soft reload does not restore the missing preview id. A conditional removal only when the element still holds this call's autoId, plus a same-element failure/success regression test, addresses that case, but does not address the cross-element lost-response collision above. Their title-scope and test-pinning notes remain in their review; I am not repeating them as blockers.

At the reviewed head, active required CI checks passed or were path-skipped; edit-accuracy coverage does not exercise this Add-effect sequence. I did not run tests locally or claim a live server/network reproduction.

Verdict: REQUEST CHANGES
Reasoning: The new unconditional rollback can leave the live DOM inconsistent with saved source, and an ambiguous successful write can turn the next ordinary edit into duplicate persisted IDs. Both paths need ownership-aware or server-reconciled cleanup before a stamp clears the merge gate.

— Review by tai (pr-review)

[jrusso1020]

Requesting changes. This builds on the two reviews already here. The overlap race they found isn't limited to the preview: a later add then saves a wrong change to the user's file.

From stripped id to a broken file. Reproduced with the hook harness for the client side and the real studio-server routes for the server side.

1. Adds A and B overlap on one id-less element. ensureElementAddressable reads the stale selection.id snapshot (gsapScriptCommitHelpers.ts:22), so A mints div and B mints div-2 over it on the live element.
2. A's id write is refused, while B's is saved and commits tl.from("#div-2"). A's finally (useGsapAnimationOps.ts:142-144) then strips div-2 from the live element.
3. The preview never gets the id back. commitMutation(..., { softReload: true }) → applySoftReload only kills and re-runs the timeline script on the existing DOM and restores inline styles; it doesn't re-sync ids. Only the cannot-soft-reload cases do a full reload. So B's tween plays nothing, and the natural next move is another "Add effect".
4. That third add, with a fresh selection (id: el.id || undefined, domEditingElement.ts:140), mints div, the first free id in the live document. It posts the id write by hfId. On the server, findTargetElement finds the element by hfId, and setAttribute("id", "div") overwrites div-2, returning changed: true.

The saved file then reads:

<div id="div" class="card" data-hf-id="hf-1">A</div>
tl.from("#div-2", { opacity: 0, duration: 2, ease: "power2.out" }, 0);   // targets nothing
tl.from("#div",   { opacity: 0, duration: 2, ease: "power2.out" }, 0);

B's saved animation is orphaned. It also disappears from the element's animation list, which matches selectors against the live document.

On main, the same sequence leaves div-2 on the element. The third add sends no id write and commits #div-2, so the file stays consistent. I ran the same test against main's useGsapAnimationOps.ts and gsapScriptCommitHelpers.ts to confirm.

Fix for this path: if (!assigned && selection.element.id === autoId) selection.element.removeAttribute("id");. With it, all 17 tests in the two changed files pass, including the no-project and network cases, and the sequence above keeps div-2 without sending a third id write. This doesn't cover the case where the save went through but its response was lost, raised in the other review; that one still needs reconciliation. Please add the overlap sequence as a test.

Smaller points:

• matched doesn't mean the target element is the one holding this id. patchElementInHtml resolves by hfId, then by id, then by selector[selectorIndex]. With an hfId missing from the file, the selector fallback matched .card[0] and returned changed: false, matched: true. The new === true || matched rule (useDomEditCommitsHelpers.ts:235) would report the id as saved on whatever element the fallback hit. The same fallback already lets changed: true land on the wrong element on main, so this isn't new. It's worth knowing before relying on matched.
• Untested header. Dropping Content-Type: application/json from requestGsapMutation leaves the tests around it green (86/86). It's probably harmless, since the server doesn't check the header.

What checked out:

• The shared GSAP request sends the same URL, headers and body as the three builders it replaces, and the batch body is covered.
• The toast text changes as described.
• Changed files 17/17, surrounding hook files 86/86, all of src/hooks 1396/1397 (the one failure is a puppeteer launch in a browser test this PR doesn't touch).
• tsc is clean.
• These deliberate breaks each turned a test red: swallowing every error in assignGsapTargetAutoIdIfNeeded, the wrong route in postGsapMutation, and dropping the refusal toast.

— Rames

[miguel-heygen]

@terencecho @somanshreddy thanks, both findings were real and had one root: the code choosing the id could see only the preview, not the file. Fixed at 67381aa by moving the choice to the server instead of patching the rollback.

• Lost reply after a saved id (tai). A new patch-element op, ensure-id, keeps the element's id or makes the proposed one unique against the file, and the reply names the id the file holds. Studio sets the preview id from that reply only, so the rollback is gone. useGsapAnimationOps.test.tsx replays your sequence through the real patchElementInHtml: the lost reply, then an add on the sibling. The previous code left the file with div twice. Now it leaves div and div-2, and the sibling's effect targets #div-2.
• Same-element overlap (Somu Initial repo setup #1). A test runs the refused-then-saved pair on one element. Before the fix, the preview had no id while the file held div-2. Now the preview id equals the file's.
• Somu initial code #2. Title and body now name the two routes this PR covers. remove-elements is listed as a follow-up.
• Somu feat(core): add compiler entry point and runtime composition fixes #3. Two tests in useGsapScriptCommits.test.tsx cover the write token and the refused-write toast. Your mutants M9 and M10 each fail a test now.
• Somu feat(studio): consolidate into single OSS-ready NLE editor #4 and fix(cli): update dev command for consolidated studio #5. Not changed. The batch route's only caller is pinned by a test, and the invalid-project path isn't reachable.

Server tests cover ensure-id and the route's elementId on both of its replies. They fail on the previous server.

[terencecho]

At 67381aaf, the two same-element failures in my earlier review are fixed: the client no longer installs/removes a speculative preview id, and ensure-id preserves an already-saved id even if its response was lost. I cannot lift changes requested yet because this fix leaves a cross-file ID collision that the previous head avoided.

Blocker — overlapping adds in different composition files can persist the same auto-ID into one preview document. Select an id-less div with a non-unique selector in composition A and start Add effect; before its ensure-id response arrives, select a similar element in inlined composition B and start another. ensureElementAddressable now only proposes div (gsapScriptCommitHelpers.ts:22–29), and useGsapAnimationOps.ts:128–145 does not reserve an ID on the shared preview DOM until after each request. Both calls can therefore propose div. The mutation coordinator queues per targetPath, so A and B do not serialize (studioFileMutationCoordinator.ts:9–21). Each server ensure-id checks only its parsed source file via nextUniqueId(document, ...) (studio-server/src/helpers/sourceMutation.ts:295–297,326–330), accepts div, and returns it. Both source files now contain id="div"; a preview with inlined subcompositions puts them in the same document. On the prior reviewed head, the first proposal synchronously installed its ID on the preview (gsapScriptCommitHelpers.ts:30–38 at 497ed032), so the second proposed div-2 instead.

This is not just an HTML-validity concern: when B is selected, writeTargetSelector prefers selection.id and returns #div (gsapShared.ts:271–280); MotionPathOverlay passes it to useMotionPathData (MotionPathOverlay.tsx:96–99), whose iframe-wide querySelector resolves A's first matching element (useMotionPathData.ts:142–153). The overlay can therefore measure the wrong element's home position and misplace B's motion-path edit. gsapRuntimeKeyframes.ts:330–347 similarly resolves the first document match before looking at timelines. I am not claiming ordinary composition-scoped GSAP playback targets the wrong element. Please maintain preview-wide uniqueness for generated IDs across the asynchronous save (without reintroducing the same-element cleanup bug), and test overlapping adds to separate inlined composition files with a source-unique editing read.

This is a source-level cross-file sequence; I did not run a live two-composition integration reproduction or local tests. The earlier required-check run passed, but a newer run was in progress when I checked. Rames's prior changes request remains a separate review at the old head.

— Review by tai (pr-review)

[somanshreddy]

Re-check at 67381aaf covering the 497ed032→67381aaf delta (4 commits); full PR not re-reviewed.

This is a comment, not an approval. Moving the id choice to the server fixes the three id items from the last round. I found no blocker of my own. I ran tai's new cross-file finding (5404129289) and confirm it; details under finding 1.

Prior items

Item	Status	Evidence
(a) A refused add strips the id that an overlapping saved add gave the same element	Fixed	The rollback is gone. The client sets the id only from the reply (useGsapAnimationOps.ts:140-145). Two adds on one element both propose div. The server keeps the first one's id for the second (sourceMutation.ts:295-297). Pinned by "a refused add never strips…".
(b) A lost reply frees a saved id, and a sibling re-mints it (tai's blocker)	Fixed for siblings in the same file	ensure-id dedupes against the file, not the preview. A retry on the first element gets its saved div back, and the sibling gets div-2. Pinned by "a reply lost after the id is saved…", which runs the server's real patchElementInHtml. After a lost reply, the preview still lacks the saved id until the next add on that element or a reload. It's harmless for same-file siblings, but see finding 1 for other files.
(c) Title overclaim	Fixed	The title is now scoped to "patch and gsap writes".
(d) Surviving mutants: write token, !res.ok	Fixed	Both are caught now (D1, D2 below), by useGsapScriptCommits.test.tsx.

Findings

1. Confirming tai (5404129289): overlapping adds in two different composition files can both save id="div" (medium, should-fix; a regression against base). Credit to tai. I checked each link:

• The dedupe only looks at the target file: nextUniqueId runs over the parsed target file (sourceMutation.ts:295-297, :326).
• Sub-compositions share the preview document: the runtime appends mounted content into the host (core/src/runtime/compositionLoader.ts:539/543).
• The motion-path lookup searches the whole document: useMotionPathData.ts:144 and MotionPathOverlay.tsx:194 call contentDocument.querySelector("#div").

I ran a scratch hook test using the real patchElementInHtml for each file, with the first file's reply held. Both elements ended up id="div", both files saved id="div", querySelectorAll("#div").length === 2, and querySelector("#div") returned the first element.

It's a regression because base (b71aa9338, gsapScriptCommitHelpers.ts:40) and 497ed032 set the proposed id on the preview at mint time. A second mint made concurrently then saw it and took div-2. I confirmed that by reading the source; I didn't run the probe at the old head. Possible fixes: reserve in-flight proposals on the client (for example, a pending-id set that ensureElementAddressable also checks), or have the server dedupe across the composition files that share the preview. Runtime GSAP scripts are wrapped per composition root (compositionScoping.ts), so playback is probably scoped. I haven't verified that.

2. Nit: elementId reports the target, but ensure-id writes opTarget. sourceMutation.ts:321 returns htmlEl's id. If a caller ever sends childSelector, the reply would name the parent's id. The batch routes also accept ensure-id but never return elementId. Studio sends neither today.

Answers to the delta questions

• Serialization: read, patch and write run synchronously in one handler (files.ts:3036-3108), with a compare-and-swap re-read at :528 and 409 → re-read, re-mint and retry. Two in-process requests, from two tabs or two elements, can't interleave on one file.
• Namespace and case: a scratch probe with linkedom deduped ids inside <template>, nested <template> and SVG (div → div-2). It treats DIV and div as distinct, which is the same rule as the client's getElementById. A quote in the value comes out escaped. ensure-id skips isSafeAttributeValue, but for id that check always returns true, so nothing is lost.
• Shared helper: I compared the new nextUniqueId against the 4 implementations it replaces (duplicate, freeze, split, group) on 28 inputs, including template and empty-base cases. The outputs are identical.
• Route: elementId is added to both replies. It is omitted when not matched, and the client's if (elementId) treats that as not found and shows a new toast.

Mutation results (17 run, 17 caught, 0 survived)

I ran the server set (5 files, 217 tests) and the studio set (4 changed hook files, 68 tests). The tree was clean after each revert.

• Server: S1 writes the proposal without dedupe. S2 overwrites an existing id. S3 changes while→if in nextUniqueId. S4 starts the suffix at 1. S5 and S6 drop elementId from the route's unchanged and changed replies. S7 drops elementId from the helper's unchanged return.
• Client: C1 builds the tween selector from the proposal. C2 never sets the saved id on the preview. C3 sets the proposal instead. C4 sends html-attribute. C5 falls back to the proposal when the reply names no id. C6 restores the live mint. C7 drops the pid guard.
• Old survivors: D1 drops the !res.ok throw (was M9). D2 drops the write token (was M10). D3 drops the refused-write toast.

CI at 67381aaf

At posting time, gh pr checks shows 72 pass and 22 pending, including Test (studio), 20 edit-accuracy shards, the timeline viewport gate and the global-install smoke test. The check-runs API shows 144 success, 23 in progress, 0 skipped and 0 failed. None are red so far, but CI isn't finished.

What I ran

• Control runs: studio-server src (67 files, 1075 passed, 2 skipped) and studio src/hooks (145 files, 1400/1400), with NODE_ENV=test.
• Scratch probes (since deleted): namespace and case, helper equivalence, and the cross-file overlap.
• The 17 mutants above.
• Own pass only; no Codex.

— Somu

[jrusso1020]

Requesting changes at 67381aaf. My blocker from 497ed032 is fixed. One regression from the fix remains: tai found it, and Somu reproduced it.

My blocker is fixed. The client no longer sets or removes an id before the write. It proposes autoId through ensure-id, the server keeps any id the element already has in the file or writes nextUniqueId against the file, and the client applies the reported elementId. So a third add can no longer overwrite a saved id, and the #div-2 tween keeps its target. The test "a refused add never strips the id an overlapping saved add gave the same element" holds the refused write while the second add completes, so the two really overlap. On the server, "keeps an id the element already holds and changes nothing" pins the no-overwrite rule.

Remaining blocker: two adds in different composition files can save the same id. I confirmed tai's sequence in the source:

• main and 497ed032 reserved the proposed id on the preview element as soon as it was minted (gsapScriptCommitHelpers.ts:40), so a second add anywhere in the preview minted div-2. This head only proposes the id.
• serializeStudioFileMutation queues per file, so writes to two files don't wait for each other.
• nextUniqueId sees only the file being written.

So if two adds on id-less elements in different composition files overlap, both save id="div". Somu reproduced that with a scratch test. useMotionPathData then resolves #div with a document-wide querySelector and can measure the wrong element. The same happens with a lost response: file A saves div, the preview never learns it, and a later add in file B proposes div again. It needs tight timing or a lost reply, but main didn't do this.

To clear this, any one of these works:

• Reserve pending and saved ids on the client: propose against the preview DOM plus a set of in-flight proposals, and add the saved elementId to it. That brings back main's preview-wide uniqueness without the strip-on-failure step that caused my blocker.
• Have the server dedupe across the project's composition files.
• Add the regression test tai asked for: overlapping adds in two inlined composition files get different ids.

A source-file filter in useMotionPathData, like the one studioMotion.ts resolveTarget already uses, would also make the overlay safe against ids an author repeats across sub-compositions. That's a nice-to-have, not a substitute for the fix.

Nit (carried over): elementId belongs to whatever element findTargetElement resolved. A selection without an hfId that falls back to selector[selectorIndex] could pick a different element and copy its id onto the selected one in the preview. Somu's childSelector note is the same kind of mismatch.

Ran: studio hooks tests (useGsapAnimationOps, useDomEditCommitsHelpers, useGsapScriptCommits, newTweenTarget) 68/68, and studio-server sourceMutation, files, duplicateElement and freezeFrame 176/176. CI at this head was still running when I posted (72 passed, 22 pending, none failed).

— Rames

[miguel-heygen]

@terencecho @jrusso1020 @somanshreddy thanks, the cross-file collision was real. Fixed at 25a555ce on the server, where the id is already chosen.

• Overlapping adds in two composition files. When a patch-element request carries ensure-id, the route now collects every id in the project's HTML files and nextUniqueId treats those as taken, as well as the ids in the file being written (routes/files.ts, projectHtmlIds; sourceMutation.ts, nextUniqueId). The scan, the patch and the write run synchronously in one handler with no await between them, so of two overlapping adds the second always sees the first one's saved id and gets div-2. No client change: Studio still proposes, and applies the elementId the reply names.
• Lost reply, then an add in another file. The same scan reads file A from disk, so B's proposal of div comes back div-2 even though the preview never learned A's id.
• Test. files.test.ts "never lets ensure-id save an id another composition file in the preview holds": compositions/b.html saves div, then the same proposal in index.html must save div-2. It fails at 67381aaf. Two mutants also fail it: passing no taken set, and scanning root files only. The touched server files pass 219/219 three runs in a row; tsc, oxfmt and oxlint are clean.
• Reading ids from other files. A regex, so it counts an id written straight after a quote or slash, and a string that ends in "/id= can't hide a real id later in the file (second test, red on both earlier versions of the regex).
• Scope of the scan. Every .html file outside hidden and vendor folders, a superset of what any one preview loads. Counting too many ids only costs a suffix. An id that exists only in the live preview (added by a runtime script) is still not seen; that is in the body as a known limit.

Not changed here, and why:

• The batch routes (patch-element-batches) still dedupe ensure-id against the one file. They never return elementId, so a caller there couldn't learn the id it got, and no client sends ensure-id to them. If one ever needs it, it should get the reply field and the same scan together.
• Ids an author already wrote twice across files (two id="title") are kept: renaming an author's ids is a separate decision, not this fix.
• Duplicate, split, freeze frame and group still pick ids unique in one file only. That is the same on main; I'm raising it as its own change.
• childSelector with ensure-id reporting the parent's id (Somu), and an hfId-less selection resolving another element by selector[selectorIndex] (Rames): Studio sends neither for ensure-id today. The second is the general match-mismatch every patch op shares, so it belongs in its own change.
• A source-file filter in useMotionPathData (Rames): agreed it's worth having against author-repeated ids, but it doesn't change this fix; I'll raise it as a follow-up.

[terencecho]

Approving 25a555ce. My blocker from 67381aaf (two adds in different composition files both saving id="div") is fixed at the server, where the collision was being decided.

What I verified

• ensure-id on patch-element now also avoids every id found in the project's HTML files: projectHtmlIds (routes/files.ts:~379–395) feeds nextUniqueId(document, base, takenIds) (sourceMutation.ts:~327–335). The set is built inside the retry loop at files.ts:~3085, right before patchElementInHtml, so a 409 retry re-reads it.
• It is race-safe for the case I described: the read of the target file, the id scan, the patch and writeMutationResult (snapshotBeforeWrite → readFileSync compare → writeFileWithReceipt, files.ts:538–553) have no await between them, so a second add in another file runs after the first write is on disk and sees its id. The new test drives that sequence (file b gets div, then file a gets div-2).
• The id regex errs toward over-reserving (it can match inside scripts or comments, which only yields div-2, never a collision), and the second test pins its edge cases (quote-ended string, no space before id). Over-reserving does not affect the client: it uses the returned id for the preview and tween selector, as before.
• ensure-id has one shipped producer (useDomEditCommitsHelpers.ts:229, via patch-element), so the motion-path overlay's document-wide #id lookups can no longer land on a duplicate created by this flow.

Non-blocking

• patch-element-batches accepts the same operation shape and calls patchElementInHtml without takenIds (files.ts:370), so an API caller sending ensure-id there still gets file-local uniqueness. Nothing in the repo does that today; passing takenIds there (or rejecting ensure-id in batches) would close it.
• Each ensure-id request synchronously reads every project HTML file. That is fine for the rare Add-effect gesture, but worth caching if it ever moves onto a hotter path.

I read the code and tests but did not run them locally. At review time the 8 required checks present had passed and nothing was red, but 32 checks were still running (including edit-accuracy), so this approval is on code merit, not CI completion. Rames's and my earlier changes-requested reviews are on older heads; mine is superseded by this one.

— Review by tai (pr-review)

[somanshreddy]

Re-check at 25a555ce covering the 67381aaf→25a555ce delta (4 commits, +97/−5); full PR not re-reviewed. This is a comment, not an approval.

The cross-file duplicate id from tai's CR 5404129289 (I confirmed it in 5404145186) is fixed. ensure-id now also avoids every id found in the project's .html files (files.ts projectHtmlIds, passed to nextUniqueId as takenIds). In the patch-element handler there's no await between the read, the scan, the patch and writeMutationResult, so two adds handled by one server can't interleave. The 409 retry re-scans. The scan may reserve too much (an id= inside a script or a title also counts), but that's harmless: it only makes the suffix larger.

Lows (none block):

1. Only double-quoted ids are tested. Changing m[1] ?? m[2] ?? m[3] to m[1], which drops single-quoted and unquoted ids from another file, still passes. One more row in the "another file's ids" test would pin it.
2. Other routes still pick ids from one file only. patch-element-batches (files.ts:370, as tai noted) doesn't pass takenIds, and neither do the duplicate, freeze, split and group callers of nextUniqueId. That was already true on main and isn't a regression. Studio sends ensure-id only through the single route.
3. Each add now reads every project HTML file synchronously on the request thread. That's fine for normal projects, but worth knowing if a project keeps many generated .html files outside dot-folders.

Mutations (7 run, 5 caught):

• Caught: dropping takenIds?.has, passing undefined from the route, the quote/slash prefix class, the consuming-vs-lookahead value match (the 25a555c case), and gating the scan off.
• Survived: the m[1]-only capture (low 1) and dropping the hidden/vendor filter. The vendor filter only affects over-reservation, so its survival is expected.

Tests: files.test.ts plus sourceMutation.test.ts 161/161 locally (--environment node). At posting time CI shows 60 pass, 30 pending (including edit-accuracy), 1 skipping, none red.

— Somu

[jrusso1020]

Approving at 25a555ce. The cross-file blocker from my review at 67381aaf (tai's finding, which Somu reproduced) is fixed. My earlier same-element blocker stays fixed.

Cross-file ids. When the request carries ensure-id, the patch-element route now collects every id in the project's HTML files (projectHtmlIds) and passes them to nextUniqueId. A proposal that another composition file already holds moves on to div-2.

• Overlapping requests: inside the attempt loop there's no await between projectHtmlIds, patchElementInHtml and writeMutationResult. So in one server process, two overlapping adds to different files can't both read the project before either writes. The second one sees the first one's id.
• Lost reply: file A saves div, the preview never learns it, and file B proposes div. The server sees div in file A and returns div-2.
• Tests: "never lets ensure-id save an id another composition file in the preview holds" is the two-file case I asked for. The quote and slash cases pin the regex.
• Regex: a false match only costs an extra suffix. What matters is a real id it misses. It matches id after whitespace, a quote or /, with quoted or unquoted values, case-insensitive. It skips data-id, which isn't an id. I couldn't come up with a valid attribute it misses.

Nits, not blocking

• The batch routes (patch-element-batches, patch-elements-batch) still call patchElementInHtml without takenIds, so an ensure-id sent there is deduped only within its file. Studio sends ensure-id only through the single-element route today.
• projectHtmlIds walks and reads every project HTML file on each ensure-id request. That's fine at project sizes, but it's the first place to look if "Add effect" gets slow on a large project.
• Carried over: elementId belongs to whatever element findTargetElement resolved, and Somu's childSelector note is the same kind of mismatch.

Ran: studio-server sourceMutation, files, duplicateElement, freezeFrame and safePath 185/185 at this head. The client-side code hasn't changed since 67381aaf, where the studio hooks tests passed 68/68. CI at this head: 94 passed, none failed, including the edit-accuracy gate. tai has approved this head, and Somu's comment finds nothing blocking.

— Rames