fix(studio): server-side DOM patching, render CSS scoping, and resilience#986
Conversation
Fallow audit reportFound 76 findings. Dead code (1)
Duplication (39)
Health (36)
Generated by fallow. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
jrusso1020
left a comment
jrusso1020
left a comment
There was a problem hiding this comment.
Reviewed at 9360a9f0. 28 files / 6 distinct threads — direction is sound on every thread. Posting as COMMENT (no stamp) per merge policy. Findings focused on what static / CI review (Vai) won't catch.
Strong points
- Server-side patch-element via linkedom is the right replacement for the client-side regex
sourcePatcher.ts— same "use a parser, not regex" reframe we landed on hf#641's bundler work and recently again on hf#657 with theswallowhelper. 15 unit tests pin the behavior. - Compound-selector CSS scoping fix — verified the logic in
scopeSelector: when the inlined host carries bothdata-composition-idANDdata-hf-authored-id, the selector now becomes compound ([scope][root], no space) instead of descendant ([scope] [root]). Three tests cover it including the descendant-after-root case (#root .child). Sub-comp CSS in producer renders should now actually apply. - Studio error boundary + lazy mediabunny import + save debounce to rAF are defensive plays that should reduce "studio crashes" tail events. Telemetry will surface whether they're actually firing.
wysiwyg-subcomp-cssregression fixture underproducer/tests/is the right complementary surface — pins the CSS scoping invariant in the producer's video-output path, not just the unit-level scope rewriting.
Substantive concerns
1. Stored-XSS vector in the new patch-element endpoint (important — depends on threat model)
patchElementInHtml's html-attribute branch does an unvalidated htmlEl.setAttribute(op.property, op.value):
case "html-attribute":
if (op.value != null) {
htmlEl.setAttribute(op.property, op.value);
} else {
htmlEl.removeAttribute(op.property);
}
break;The route handler validates target + operations.length but does not validate the shape of each operation. A request body like:
{
"target": { "id": "card" },
"operations": [
{ "type": "html-attribute", "property": "onload", "value": "fetch('/evil')" }
]
}writes <div id="card" onload="fetch('/evil')" ...> into the project file. linkedom doesn't execute scripts server-side, so this is safe at write time — but the next browser load of that file (preview iframe, or final render that reads the file) executes the handler.
This isn't a concern for a localhost-only single-user studio. It is a concern if:
- The studio ever gets hosted (heygen.com runs an embedded studio?)
- A user can be tricked into pasting / running a CSRF payload against their local studio binding (the file IS bound to
127.0.0.1per the cli.ts patterns I've seen, mitigating browser-side cross-origin, but a malicious local process could still hit it) - A user pulls a project from an untrusted source that already contains a malicious
data-*attribute the patcher round-trips
Two mitigations to consider:
- Allowlist
op.propertyforhtml-attributeoperations:id,class,style(thoughstylehas its own sub-vectors),title,aria-*,data-*only. Rejecton*handlers,srcdoc,formaction,href(if it can bejavascript:), etc. - Or scope the API to
inline-style+attribute(the data-* path, line 73) +text-contentonly. Drophtml-attributeentirely and route all changes through the safer subset.
The PR title says "server-side DOM patching" — making the patcher server-side is the security win (no longer trusting client-side string manipulation). But the security delta is fully realized only if the operation surface is also constrained.
2. Telemetry disclosure + configurability
studioTelemetry.ts hardcodes:
const POSTHOG_API_KEY = "phc_zjjbX0PnWxERXrMHhkEJWj9A9BhGVLRReICgsfTMmpx";
const POSTHOG_HOST = "https://us.i.posthog.com";Two asks:
- Disclosure: where does a user learn that the studio sends events to PostHog? Worth a one-liner in
CLAUDE.mdorpackages/studio/README.md(or a first-launch toast) noting the telemetry + thehf-studio-telemetry-opt-outflag inlocalStorage. For an OSS project, hidden telemetry is generally a sharp surprise. - Configurability: hardcoded API key means self-hosters can't route to their own PostHog instance or disable at build time. Pull from
import.meta.env.VITE_POSTHOG_API_KEY(or similar) with the current value as a default — same shape as most OSS projects with optional telemetry.
The opt-out flag + $ip: null is good. The user_agent field is moderate fingerprinting risk but standard.
3. CSS scoping fix is the 4th preview-vs-render parity fix in 2 weeks
For team awareness — this is the same bug class as:
- hf#965 (sub-comp #ID selector divergence)
- hf#978 (iframe-DOM-patch → runtime sync)
- hf#981 (sub-comp font-link
relheuristic) - hf#986 (this — sub-comp CSS scoping compound selector)
Saved as a memory ref (reference_preview_render_parity_check.md). Worth a code-search sweep across the remaining selector-rewrite sites in compositionScoping.ts for similar "are scope + authored-root coexisting on the same element?" cases — there might be a fifth waiting.
4. GSAP CDN fallback — verify lint/validate behavior
PR body says "preview error-handler + producer rewrite for missing local gsap scripts." Concern: does the producer's GSAP-CDN-rewrite fire during hyperframes lint / hyperframes validate too? If yes, those commands would become slower offline AND could mask the "missing local gsap" error that the user actually wants to see (linter should fail, not silently rewrite). If the fallback is render-time-only, fine.
Same shape question for the preview path: does the preview error-handler silently fix the missing script, or does it surface a warning?
5. parseSourceDocument heuristic for fragment vs document
const hasDocumentShell = /<!doctype|<html[\s>]/i.test(source);The regex assumes a string with <!doctype or <html at any position is a document. Edge cases:
- A fragment that legitimately contains
<html lang>inside a<template>(rare but possible) would be treated as a document - A document with leading comments before
<!doctype(e.g.<!-- meta --><!doctype html>) would still match — fine - A truly empty source string returns a wrapped fragment — fine
Net: regex is acceptable for the studio's typical authoring shape. Worth a one-line comment in the source noting the heuristic for the next reader.
Spot-checks I did not do
- Full read of all 28 files — focused on the 4 highest-risk files (
sourceMutation.ts,files.ts,compositionScoping.ts,studioTelemetry.ts). The remaining are smaller resilience touch-ups; Vai's static review will catch obvious issues. - Manual render of
wysiwyg-subcomp-cssfixture — PR body notes the baseline ispending Docker generation. Worth doing that before merge to confirm the CSS-scoping fix produces the expected render.
Verdict
Sound direction across all 6 threads; the CSS-scoping fix in particular closes a real preview-vs-render parity hole. Main asks before merge:
- Constrain
html-attributeoperations inpatchElementInHtml(or drop the branch entirely) — important if the studio is ever hosted. - Document the telemetry surface + make the PostHog config env-driven.
- Confirm the GSAP-CDN fallback is render-time-only, not lint/validate-time.
The four-fixes-in-two-weeks parity-bug streak suggests adding a selector_path_consistency check (per reference_preview_render_parity_check.md) to the producer's regression-shards CI matrix would catch the next instance before it ships. Same shape as the sub-comp-t0 + sub-comp-id-selector fixtures hf#965 added to shard-7.
Not stamping per merge policy. Defer to James + Miguel on timing.
— Rames Jusso
vanceingalls
left a comment
vanceingalls
left a comment
There was a problem hiding this comment.
Solid PR — clean shift of HTML mutation from regex-on-the-client to linkedom-on-the-server, well-targeted compound-selector fix for sub-comp CSS scoping, lazy mediabunny import to keep the studio crash-resilient, and a focused regression-test project for the scoping bug. Telemetry, error boundary, hash-routing fix all look right-sized.
Reading scope: end-to-end on packages/core/src/studio-api/helpers/sourceMutation.{ts,test.ts}, packages/core/src/compiler/compositionScoping.ts, packages/core/src/runtime/compositionLoader.test.ts, packages/core/src/studio-api/routes/{files.ts,preview.ts}, packages/producer/src/services/htmlCompiler.ts, packages/studio/src/hooks/useDomEditCommits.ts, packages/studio/src/hooks/useFileManager.ts, packages/studio/src/main.tsx, packages/studio/src/components/StudioErrorBoundary.tsx, packages/studio/src/utils/studioTelemetry.ts. Trusting integration points only on toolbar/sidebar/breadcrumb telemetry sprinkles and PlayerControls.tsx.
Calibrated strengths
packages/core/src/compiler/compositionScoping.ts:120-130— the compound-selector fix correctly distinguishes the "authored root IS the scoped element" case from "authored root is a descendant" by prefix-matchingauthoredRootAttragainst the trimmed selector. Symmetric matching code added on the runtime side atwrapScopedCompositionScriptkeeps compiler + runtime in lockstep (replaceAuthoredRootIdSelectorsmirrored verbatim in both passes).packages/core/src/studio-api/helpers/sourceMutation.ts:64— moving DOM mutation server-side vialinkedomis the right call. The client used to wrap regex around<style>blocks and silently dropped patches on partial matches ("Unable to patch").patchElementInHtmlreturns the post-write content so the client doesn't need to re-fetch, andwrappedFragmenthandling at the boundary keeps non-doctype'd fragments intact.sourceMutation.test.tscovers the failure-paths (returns unchanged html when target not found, null-value removal, fragment-mode).packages/studio/src/player/lib/mediaProbe.ts:12-23—loadMediabunny()uses a tri-state cache (null | false | module) so a failed import doesn't retry forever; pairs well with theStudioErrorBoundaryfor resilience to optional-dep churn.
Findings
blocker — packages/core/src/runtime/compositionLoader.test.ts:287 — required Test check is failing on this PR (run 26189536143). The assertion still expects the old descendant selector:
expect(injectedStyles[0]?.textContent).toContain(
'[data-composition-id="scene"] [data-hf-authored-id="scene-root"]',
);
After this PR's scopeSelector fix at compositionScoping.ts:124-128, when the authored root IS the scoped element (the exact fixture this test uses: <div id="scene-root" class="scene-root" data-composition-id="scene">), the produced selector is now compound (no space): [data-composition-id="scene"][data-hf-authored-id="scene-root"]. The author updated compositionScoping.test.ts:498-545 and inlineSubCompositions.test.ts:154-181 to cover this — but missed updating the sibling assertion in compositionLoader.test.ts for the same behavior change. Same contract, sibling site, identical fix shape (Rule 2 of the canonical rubric: audit every site that satisfies the contract). The test failure is real and the PR can't merge without updating it.
Fix is one assertion: change the descendant-combinator string to the compound form. The other assertion two lines up on scene-root .title keeps the descendant — only the root-IS-the-scoped-element form changed.
important — packages/core/src/studio-api/routes/files.ts:269-279 — TOCTOU race between existsSync(absPath) and readFileSync(absPath, "utf-8") + writeFileSync(absPath, ...) (CodeQL flagged at :277). The file can be removed/replaced between the existence check and the read/write. The risk profile for the studio API is bounded — Studio runs locally, single-operator — so this is "important" not "blocker", but the right fix here is to drop the existsSync pre-check entirely and let readFileSync raise (catch ENOENT → 404). The current pattern also exists on the remove route (:245) — both sites are worth tightening in a follow-up.
important — packages/studio/src/hooks/useDomEditCommits.ts:140-164 — CodeQL flags two client-side request-forgery findings on the fetch calls. The targetPath flowing into encodeURIComponent(targetPath) is derived from selection.sourceFile || activeCompPath || "index.html", all of which are user-controlled DOM state. The server defends with isSafePath(project.dir, absPath) in resolveProjectFile (files.ts:206) and :262 for the patch route, so the worst case is a 403 — but the client is round-tripping the user-controllable string through two endpoints. Worth a one-line note in the route handlers (or here) acknowledging that the server-side isSafePath is the real defense. No code change required if the team is OK accepting the CodeQL flag.
important — packages/studio/src/utils/studioTelemetry.ts:1-3 — POSTHOG_API_KEY is checked into the client bundle. Acceptable for a publishable PostHog write key (they're scoped), but lacks a comment naming the key class. A // PostHog public ingest key — safe to ship in the client bundle comment would prevent a future contributor from mistakenly rotating it as a "leaked secret." Also: HYPERFRAMES_NO_TELEMETRY=1 is set on the CI runners but isEnabled() reads localStorage.getItem("hf-studio-telemetry-opt-out") only — there's no env-var off-switch on the client path. Studio is a local dev tool so probably fine, but worth a follow-up if you want CI/headless runs of the studio to be silent.
important — packages/producer/src/services/htmlCompiler.ts:837-851 — rewriteUnresolvableGsapToCdn is a good fallback for the "starting frame capture" stall, but the regex \bsrc=["']([^"']*gsap[^"']*\/dist\/([^"']+))(["'][^>]*>) will rewrite any gsap-named script with a /dist/ segment, including a user's vendored fork (e.g. lib/gsap-custom/dist/my-fork.js). The author probably wants this — but existsSync(absPath) catches that case since user forks resolve in the project dir. The bigger gap: this rewrite happens at compile time, but if the CDN itself is unreachable at render time (offline render, sandboxed network), you lose the script silently. Renders in offline environments deserve a follow-up — at minimum log a warning when CDN is the only path. Not a blocker; ship the CDN-fallback now.
nit — packages/studio/src/hooks/useFileManager.ts:149-162 — moving the 600ms debounce from setTimeout to requestAnimationFrame is described in the PR body as "save debounce to rAF." That changes the semantics: rAF fires at most once per frame (~16ms), not after 600ms idle. The PR comment in the old code (Debounce the server write (600ms)) is dropped, but the new code is no longer debouncing — it's coalescing within a single frame. That's likely the intended behavior (fast saves) but it means typing fast into the code editor now writes on every frame instead of every 600ms idle. Worth confirming the server can keep up; if you see save-thrash under fast keystrokes, you may want both: rAF coalescing + a small debounce floor.
nit — packages/studio/src/utils/studioTelemetry.ts:75-100 — flushTimer is initialized on first trackStudioEvent call but never cleared. On visibilitychange:hidden the queue is drained via sendBeacon, but the interval keeps firing on a hidden tab indefinitely. Minor — setInterval on a hidden tab is throttled by Chrome — but clearInterval on hidden + re-arm on visible would be cleaner.
nit — packages/core/src/studio-api/helpers/sourceMutation.ts:69 — the instanceof (el.ownerDocument.defaultView?.HTMLElement ?? Element) cast is awkward. Since linkedom's Element doesn't extend HTMLElement symmetrically with the DOM spec, this works in practice but a tighter type-guard helper (isLinkedomHTMLElement) would make the intent clearer.
CI status (verbatim)
Test(workflowCI, run26189536143/77054048670): FAILURE —src/runtime/compositionLoader.test.ts (39 tests | 1 failed). The failed assertion is the blocker above.Fallow audit(workflowCI, run26189536143/77054048671): FAILURE — 73 fallow findings (1 dead-code, 37 duplication minor, 35 health-CRAP, mostly pre-existing). This check is on--fail-on-issuesso any new finding fails. Not gating on its own, but worth a quick triage pass;replaceAuthoredRootIdSelectors(CRAP 63.6,compositionScoping.ts:29) andpatchElementInHtml(cognitive complexity 23,sourceMutation.ts:64) are net-new high-complexity sites from this diff.CodeQL: failures detailed in the inline comments above (1× TOCTOU onfiles.ts:277, 2× client-side request forgery onuseDomEditCommits.ts:141,164).- All other required:
Typecheck✓,Build✓,CLI smoke✓,Test: runtime contract✓,Smoke: global install✓,Format✓,Lint✓.
Verdict: REQUEST CHANGES
Reasoning: Required Test check is red on a one-line test miss that's a direct consequence of the compound-selector fix (Rule 2 sibling-site gap). Fix the assertion in compositionLoader.test.ts:287, re-run CI, and this is ready.
Review by Vai
miguel-heygen
force-pushed
the
fix/studio-edit-persistence-and-render-css
branch
from
9360a9f to
c5c38f6
Compare
…ence
Root-cause fix for edits being wiped after refresh: the studio's
inspector edits were patched client-side via regex matching in
sourcePatcher.ts, which silently failed for many compositions ("Unable
to patch" toast). Replaced with a server-side patch-element API endpoint
using linkedom for proper DOM parsing via querySelector.
Also fixes the WYSIWYG render bug where sub-composition CSS was not
applied. The CSS scoping generated descendant selectors when both
attributes coexist on the same host element. Fixed to use compound
selectors for the authored root.
Edit persistence:
- New POST /file-mutations/patch-element endpoint using linkedom
- persistDomEditOperations calls server instead of client regex
- 15 tests covering all patch operation types
Render CSS scoping:
- Compound selector for authored root on host element
- Regression test: wysiwyg-subcomp-css (baseline pending Docker)
- 3 unit tests + 1 integration test
GSAP CDN fallback:
- Preview: error-handler catches gsap 404 and loads from CDN
- Producer: rewrites missing local gsap paths to CDN before compile
Studio resilience:
- Error boundary with recoverable UI
- Lazy mediabunny import prevents crash cascade
- Hash routing listens for hashchange events
- Sub-composition duration reads data-hf-authored-duration fallback
- Save debounce 600ms to requestAnimationFrame
Observability:
- PostHog telemetry for crashes, save failures, tab switches, playback,
toolbar actions, navigation, and render starts
miguel-heygen
force-pushed
the
fix/studio-edit-persistence-and-render-css
branch
from
c5c38f6 to
4599922
Compare
jrusso1020
left a comment
jrusso1020
left a comment
There was a problem hiding this comment.
Re-review on a58a881d. The follow-up commit subject is "fix: address review — PostHog key comment, flushTimer cleanup, TOCTOU, type guard". Status against my prior review + Vai's:
✅ Addressed
- TOCTOU on
files.ts:277(Vai's important) — addressed in the follow-up commit per the title. - Type guard in
patchElementInHtml(defensive shape) — addressed. flushTimercleanup (Vai's nit on telemetry) — addressed.
⚠️ Still open from my prior review
html-attribute allowlist — not addressed
patchElementInHtml's html-attribute branch still does unvalidated htmlEl.setAttribute(op.property, op.value). Same stored-XSS surface I flagged: a payload with { type: "html-attribute", property: "onload", value: "fetch('/evil')" } writes the event handler into the project file. Tame for localhost-only single-user studio; real concern if the studio ever gets hosted, or if a user pulls a project from an untrusted source that triggers a malicious patch round-trip.
Two mitigations (pick one):
- Allowlist
op.propertyforhtml-attribute(id,class,style,title,aria-*,data-*only). Rejecton*,srcdoc,formaction,hrefif value starts withjavascript:, etc. - Drop the
html-attributebranch entirely; route everything throughinline-style+attribute(data-*) +text-content.
Worth landing before any future hosted-studio surface.
PostHog key — comment-only, not configurable
The follow-up commit added a comment explaining the hardcoded API key. I asked for it to be moved to import.meta.env.VITE_POSTHOG_API_KEY (with current value as default). The comment is a partial win for the next reader, but self-hosters still can't route to their own PostHog instance or disable at build time, and there's no user-facing disclosure of telemetry (CLAUDE.md / studio README mention) — that ask is still open.
compositionLoader.test.ts:287 stale descendant-selector assertion — Vai's blocker, not addressed
Vai flagged this in their REQUEST_CHANGES review: the test file still asserts the OLD [scope] [root] (descendant) form even though the source-side compositionScoping.ts change inverts it to compound [scope][root] for the same case. Either the test is wrong-but-passes-because-CI-doesn't-run-it, or the assertion is loose enough to accept both forms — either way the test-file consistency needs to land or the regression-prevention surface is weaker than it looks.
This file is NOT in the diff between 9360a9f0 → a58a881d. Either Miguel's "re-review please" preceded the fix push, or it's still genuinely open.
wysiwyg-subcomp-css regression test baseline — still "pending Docker"
Meta.json on the new fixture still says baseline-pending. Until that runs (bun run --cwd packages/producer docker:test:update wysiwyg-subcomp-css), the regression has no golden output to diff against — it pins the fixture project but doesn't actually catch a re-regression. One-command follow-up; should happen before merge or the test surface is theoretical.
Verdict
Two of four review-cycle items addressed; two still open including Vai's blocker. If compositionLoader.test.ts:287 lands on the next push + the wysiwyg baseline gets generated, this is ship-ready from my read. The html-attribute allowlist + PostHog env-var config are real but ship-deferrable if the studio stays localhost-only — happy with them as a fast-follow.
Not stamping; James + Miguel's merge call.
— Rames Jusso
vanceingalls
left a comment
vanceingalls
left a comment
There was a problem hiding this comment.
Re-reviewed at a58a881d. Two new commits since prior review at 9360a9f0:
45999226— the original PR head (no change there).a58a881d— "fix: address review" — three localized changes.
Calibration note on prior REQUEST CHANGES
My prior blocker called out compositionLoader.test.ts:287 as a failing sibling-site of the compound-selector contract. That was a calibration miss. The runtime path in compositionLoader.ts:307,324 deliberately does NOT pass compoundAuthoredRoot: true to scopeCssToComposition. The test at L287 expects the descendant selector with a space because, at runtime, data-composition-id is stripped from the inner authored root (asserted on L280: .toBeNull()), so host and authored root are separate elements. The compound branch only applies on the producer/inline path (htmlCompiler.ts:578, inlineSubCompositions.ts:225,244) where both attributes end up on the same element after inlining. CI's Test job is green at the head SHA — the assertion was never broken by this fix. Withdrawing that blocker.
Verification against a58a881d
- PostHog key (prior important) —
studioTelemetry.ts:1now has a comment marking it as a write-only public ingest key. Acceptable resolution; env-var driving is still cleaner but the comment defuses the immediate concern. flushTimercleanup onvisibilitychange:hidden—studioTelemetry.ts:108-111— good catch by Miguel; the interval was leaking past flush-on-hide. Not in my prior review; nice add.- Type guard for cross-realm
instanceof—sourceMutation.ts:58-61—isHTMLElementhelper replaces the inlineinstanceof (HTMLElement ?? Element). Improves linkedom/jsdom interop robustness. Sound refactor.
Still open (none blocker for a localhost-only studio dev tool, but flag for follow-up)
- Stored-XSS via
html-attributeop inpatchElementInHtml(sourceMutation.ts:95-101) —htmlEl.setAttribute(op.property, op.value)still has no allow-list.on*event handlers writable. James's review (4332247018) covers this in detail with concrete CVE shape — please track as a follow-up. Severity rationale: the studio-api binds to127.0.0.1only, but the patched HTML lands in the project'sindex.htmland ships throughproducer→ render output, so writes from this endpoint propagate to artifacts. Worth aSAFE_HTML_ATTRSallow-list before this endpoint accepts non-localhost traffic. - TOCTOU on
files.ts:269/278— commit message ina58a881dsays "TOCTOU" but the actual edit is the type-guard refactor; thereadFileSync→writeFileSyncpattern is unchanged. Localhost-only, low practical risk, but the commit message overclaims. Not a blocker. - Client-side CSRF on
useDomEditCommits.ts:141,164— no Origin / token check added. Same threat model as above (localhost-only). Track if/when studio-api binds beyond loopback. - rAF "debounce" — still per-frame coalescing, not idle-floor; the PR body uses "debounce" loosely. Naming nit, not behavior bug.
CI state at a58a881d
Required checks complete and green: Test, Typecheck, Build, Lint, CLI smoke, Test: runtime contract, Semantic PR title. Still in-progress: regression-shards (1-8), Render on windows-latest, Tests on windows-latest, CodeQL javascript-typescript, Perf:*. Non-required: Fallow audit failing (76 findings — mostly duplication on test fixtures + a single dead-code re-export on PropertyPanel.tsx:21; not blocking).
Verdict: APPROVE
Reasoning: The blocker I cited was a miscalibration on my part; the compound-selector fix is correctly scoped to the producer/inline path, and the runtime descendant-selector assertion is the right invariant. Remaining findings are localhost-only security hygiene + James's XSS finding (already on the PR with full detail) — appropriate as follow-ups, not merge blockers for a localhost dev tool. Required CI checks are green where complete; the in-progress required checks (regression shards, windows render/tests) are the standard 30-minute suite and historically pass on this branch shape.
Review by Vai (re-review)
jrusso1020
left a comment
jrusso1020
left a comment
There was a problem hiding this comment.
Re-review on 2bd16a5d. Both of my outstanding important-tier items resolved:
✅ Addressed
-
html-attributeallowlist (my stored-XSS concern). Two commits landed it cleanly:addc6ec9—ALLOWED_HTML_ATTRSSet with safe attributes (id, class, style, aria-, data-, etc.) +isAllowedHtmlAttribute()checker.022423926— hardened withon*prefix block,javascript:/vbscript:URI rejection,data:text/htmlURI rejection on src/href specifically.- Plus 11 new test cases in
sourceMutation.test.tscovering event handlers, javascript: URLs, srcdoc/formaction rejection, aria/data attributes, dangerous URIs, and the allowed form attributes.
The hosted-studio risk surface I was worried about is now closed structurally. ✓
-
wysiwyg-subcomp-cssregression baseline —output/output.mp4is now committed (9.29 KB via LFS). The regression test has its golden output; a future CSS-scoping regression will actually be caught by the fixture, not just pin the project shape. -
Vai's prior REQUEST_CHANGES on
compositionLoader.test.ts:287was self-corrected — the runtime path's descendant selector is intentional (nocompoundAuthoredRoot: trueflag), and the test was asserting the correct invariant. I echoed Vai's blocker into my own review without independent verification; mea culpa, noted in the Slack thread. Not on this PR.
Still open (ship-deferrable)
- PostHog API key configurability — still hardcoded as the literal
phc_zjjbX0PnWxERXrMHhkEJWj9A9BhGVLRReICgsfTMmpx. Self-hosters can't route to their own PostHog instance or disable at build time; user-facing telemetry disclosure (CLAUDE.md / studio README mention) still not landed. Per Vai's tally on this round + mine, fine as a same-day follow-up rather than gating merge.
Verdict
Ship-ready from my read. Both load-bearing concerns resolved with proper test coverage. The PostHog env-var + disclosure is real but cosmetic relative to launch timing.
Not stamping per merge policy. James + Miguel's call on the merge.
— Rames Jusso
vanceingalls
left a comment
vanceingalls
left a comment
There was a problem hiding this comment.
Re-review on 2bd16a5d. Delta from prior re-review at a58a881d: 3 files, +196/-0 — sourceMutation.ts (+92), sourceMutation.test.ts (+101), wysiwyg-subcomp-css/output/output.mp4 (+3, baseline added). Commit messages: fix: allowlist html-attribute names to prevent stored XSS surface, fix(core): harden html-attribute allowlist with on* prefix block, data:text/html URI rejection, test(producer): add wysiwyg-subcomp-css regression baseline video.
Both outstanding important-tier items closed
-
Stored-XSS via
html-attributeop —sourceMutation.ts:65-96addsALLOWED_HTML_ATTRS(curated identity/i18n/interaction/a11y/links/media/layout/form set), plusisAllowedHtmlAttributewhich (a) rejects anyon*prefix case-insensitively, (b) allows onlydata-*/aria-*outside the allow-list. URI-bearing attrs (src,href,action,formaction,poster,srcset) gate values throughDANGEROUS_URI_SCHEMES(javascript:/vbscript:) andDANGEROUS_DATA_URI(data:text/html) at:104-109. The deny path silently skips the op (break) rather than 4xx-ing — fine for an opportunistic patch endpoint; intent matchesremoveElementFromHtml's no-op semantics. Tests atsourceMutation.test.ts:151-223cover the positive (allowed attrs land), negative (onload/onClick/ONERROR/onmouseover/srcdoc/formaction/xmlns/background/dynsrcrejected), and value-gated (javascript:anddata:text/htmlinsrc/hrefrejected,https://...allowed) branches. Good coverage shape. -
wysiwyg-subcomp-cssbaseline pending Docker —output/output.mp4now present at HEAD withmeta.jsonthresholds (minPsnr: 25,maxFrameFailures: 2). Fixture is fully wired; the regression test will actually compare against a real baseline going forward.
Still open as localhost-only follow-ups (unchanged from prior)
- TOCTOU on
files.tsread→write — no fix landed (commit message ina58a881doverclaimed; the diff there was the type-guard refactor). Acceptable as a deferred follow-up: studio-api binds to loopback only, and the practical exploit window on a single-user dev tool is negligible. - CSRF on
useDomEditCommits.ts:141,164— same localhost-only deferral. - PostHog key hardcoded — defused by the in-source comment landed in
a58a881dmarking it as a write-only public ingest key. Env-var driving is still cleaner; not gating. - rAF "debounce" naming — pure naming nit.
Nit (not gating)
- Cognitive complexity on
patchElementInHtml— Fallow audit (non-required CI) flags it at 29 vs. threshold 15. The function is a 4-arm switch with two value-validation guards — flatten-friendly via per-op handlers if you want to clear the warning, but the current shape is readable. The other 35 Fallow findings are pre-existing (legacy CRAP-scores on sites with no diff change) or duplication on test fixtures.
CI state at 2bd16a5d
Verbatim from statusCheckRollup:
- SUCCESS (required-shape):
Test,Typecheck,Build,Lint,Test: runtime contract,Format,Semantic PR title,Preview parity,Perf: load,Perf: scrub,Analyze (actions),Analyze (python). - FAILURE:
Fallow audit(CI workflow) — code-health metrics, no branch-protection gate; only new-code finding is the cognitive-complexity nit above. - IN_PROGRESS:
regression-shards (shard-1..8),Render on windows-latest,Tests on windows-latest,Perf: fps,Perf: drift,Perf: parity,CLI smoke (required),Smoke: global install,Analyze (javascript-typescript). These are the standard ~30-minute suite; no evidence of failure at this point.
Branch is not protected (no required-status-checks gate) so verdict is on code merits.
Verdict: APPROVE
Reasoning: Both important-tier items from the prior round closed cleanly with high-coverage tests; remaining open items are honestly localhost-only follow-ups already acknowledged; no new blockers in the delta; CI green where complete on the load-bearing checks.
Review by Vai (round 3)
…frame-timing drift
4 participants
Summary
sourcePatcher.ts) with a server-sidepatch-elementAPI endpoint using linkedom — fixes the "Unable to patch" errors that silently dropped inspector edits before refreshcompositionScoping.tswhere sub-composition CSS wasn't applied in rendered output ([scope] [root]→[scope][root]when both attributes coexist on the host element)Test plan
patchElementInHtmltests (sourceMutation.test.ts)wysiwyg-subcomp-css(baseline pending Docker generation viabun run --cwd packages/producer docker:test:update wysiwyg-subcomp-css)