fix(studio): stop injecting inline z-index on all clips during timeline edits#959
Merged
miguel-heygen merged 1 commit intoMay 19, 2026
Merged
Conversation
…ne edits Timeline move, delete, and asset-drop operations were looping over every clip in the file and writing style="z-index: N" derived from an inverted data-track-index mapping. This silently overrode the author's CSS z-index — contradicting the documented contract that data-track-index does not affect visual layering — and persisted the corruption in the source HTML. Remove the z-index injection loops from all three timeline commit paths. Move and delete now only patch timing/track attributes on the affected clip. Asset drop still sets z-index on the newly created element via the generated HTML, without touching existing clips. Delete the now-unused buildTrackZIndexMap helper. Also fix patchInlineStyleInTag to handle self-closing void elements: the old code produced malformed `<img ... / style="z-index: 7">` because it didn't account for the trailing `/` before appending the style attribute. Closes #958
Fallow audit reportFound 25 findings. Duplication (14)
Health (11)
Generated by fallow. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| // Add one | ||
| const newTag = | ||
| tag.replace(/>$/, "") + ` style="${prop}: ${escapeStyleAttributeValue(value, '"')}"`; | ||
| const selfClosing = /\s*\/$/.test(tag); |
| const newTag = | ||
| tag.replace(/>$/, "") + ` style="${prop}: ${escapeStyleAttributeValue(value, '"')}"`; | ||
| const selfClosing = /\s*\/$/.test(tag); | ||
| const base = selfClosing ? tag.replace(/\s*\/$/, "") : tag; |
vanceingalls
approved these changes
May 19, 2026
Collaborator
vanceingalls
left a comment
vanceingalls
left a comment
There was a problem hiding this comment.
Solid surgical fix. Root cause framing in the PR body matches the diff: three commit paths were injecting an inverted data-track-index -> z-index mapping onto every clip, overriding the documented "CSS is the layering source of truth" contract. Removing those loops in useTimelineEditing.ts and dropping buildTrackZIndexMap is the right call.
Calibrated strengths
sourcePatcher.ts:206-208— the self-closing fix is correct under the existingfindTagByTargetregex semantics.[^>]*captures up to but not including>, so for<img ... />the capturedtagends in... /. The oldtag.replace(/>$/, "")was a no-op; the newselfClosingdetect +${base} ... " /"recipe restores<img ... style="..." />cleanly, withreplaceTagAtMatchre-prepending the>that's still inhtml.slice(match.end). Verified mentally for both branches (with-existing-style and without).timelineZIndexInjection.test.tsreproduces the exact failure mode from issue #958 (inverted layering on the bg-video/title case) before asserting the fix. Good practice — the test would catch a regression if someone re-introduced the loop.- Test rename / placement: keeping
sourcePatcher.test.tsfor unit-level void-element coverage and adding atimelineZIndexInjection.test.tsfor the behavioral reproduction is the right split.
Findings
- important —
CodeQLCI is failing on this PR withjs/polynomial-redoson the new\s*\/$regexes (sourcePatcher.ts:206-207). Practically a false positive: the inputtagis bounded by the upstream[^>]*capture and the regex has a single linear\s*quantifier with no nested ambiguity, plus this is local-file edit code, not network-attacker input. But it's the new required-check failure introduced by this diff. Quickest path to green: rewrite without the regex, e.g.Same behavior, no regex, CodeQL stops complaining. Worth doing before merge so the rule stays useful as a tripwire.const trimmed = tag.trimEnd(); const selfClosing = trimmed.endsWith("/"); const base = selfClosing ? trimmed.slice(0, -1).trimEnd() : tag;
- important — Backwards compat for projects that already have stale-injected inline
z-indexon disk from the prior bug. The fix prevents new injection but doesn't clean up existing artifacts; those projects will still render with inverted layering until someone manually deletes the inline styles. Not a blocker for this PR, but worth a follow-up: either a one-shot migration on project open, or a Studio toast/notice when inlinez-indexis detected on a.clip. Filing an issue is enough — does not need to land in this PR. - nit —
useTimelineEditing.ts:305andblockInstaller.ts:127:Math.max(1, relevantElements.length + 1)is always>= 2. The clamp is a no-op;relevantElements.length + 1is sufficient. Trivial. - nit — The new asset-drop z-index policy (new clip =
length + 1, i.e. on top) is a reasonable default but is now implicit and undocumented. Worth a one-line comment inhandleTimelineAssetDropandaddBlockToProjectcalling out "new clips get the highest current z-index; existing clips are not modified" so the next reader doesn't reverse-engineer it. Also consider whether CSSz-indexauthor intent on the inserted asset (none yet, since it's brand-new) should be derived from a registered convention rather than a Studio default — but probably overkill for this PR.
CI status (verbatim)
Fallow audit | CI | FAILURE— body shows only pre-existing CRAP-score + duplication minor findings inuseTimelineEditing.ts/sourcePatcher.ts; nothing new introduced by this diff (the diff in fact reduces code in the flagged functions). Non-blocking.CodeQL | (top-level) | FAILURE—js/polynomial-redosx2 onsourcePatcher.ts:206-207. Discussed above.Render on windows-latest | Windows render verification | IN_PROGRESS— pending, not decisive.- All other required checks (Build, Test, Typecheck, Lint, Format, CLI smoke, regression, preview-regression, player-perf) are SUCCESS at head
4916d658.
Verdict: APPROVE
Reasoning: Bug fix is correct, well-tested, and the diff is appropriately scoped. The CodeQL failure is the only real concern — a 4-line rewrite to drop the regex keeps the static-analysis tripwire honest and clears the required check. Backwards-compat note is a follow-up, not a blocker.
Review by Vai
Collaborator
Author
Merge activity
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
patchInlineStyleInTagto handle self-closing void elements (<img />,<audio />) — the old code produced malformed<img ... / style="z-index: 7">outputbuildTrackZIndexMaphelper and its testsRoot cause
Three timeline operations (
handleTimelineElementMove,handleTimelineElementDelete,handleTimelineAssetDrop) looped over every clip in the file on each commit and injectedstyle="z-index: N"derived from an inverteddata-track-indexmapping viabuildTrackZIndexMap. This overrode the author's CSS z-index — contradicting the documented contract thatdata-track-indexdoes not affect visual layering — and persisted the corruption in the source HTML.Test plan
<img ... style="..." />output (not<img ... / style="...">)data-startchanged on the dragged clip with zero inline z-index injectionsCloses #958