fix(studio-server): child-scoped patch operations with batch abort

[miguel-heygen]

Part 3/7 of the design-panel fix stack (replaces #1888).

Problem

Editing one line of a multi-line text element serialized the parent's innerHTML into a single text-content operation, which patchElementInHtml applies via textContent = value — escaping the markup into literal <span> text in the user's composition file (source corruption).

Fix (server half; client half is the next PR)

• PatchOperation gains optional childSelector/childIndex, resolved with a scoped querySelectorAll under the already-matched parent — never document-wide.
• patchElementInHtml resolves every operation's target in a pre-pass before mutating; if any child locator misses, the whole batch aborts with matched:false and no partial write, which routes into the client's unresolvable-target error path (toast + revert) instead of a silent no-op.
• Style-declaration parsing extracted to sourceStyleMutation.ts to stay under the repo's file-size cap.
• New ./source-mutation subpath export (mirrors ./finite-mutation) so integration tests can drive the real mutation function.

Testing

sourceMutation.test.ts gains child-locator cases (found, not-found, no-partial-write batch abort); split/wrap/unwrap cases moved to sourceMutationSplitAndGroup.test.ts for the size cap. Full studio-server suite: 218 tests green.

[miguel-heygen]

• test(studio): agent-browser e2e smoke for the design panel #1912
• test(studio): cover persist-failure hook behavior #1911
• fix(studio): surface persist failures with toast and guarded revert #1910
• fix(studio): per-child patch op builders and persist-seam harness #1909
• fix(studio-server): child-scoped patch operations with batch abort #1908 👈 (View in Graphite)
• fix(studio): make canvas selection hit intended elements #1907
• test(studio): add design-panel QA fixture and triage matrix #1906
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

[james-russo-rames-d-jusso]

Reviewed at acf7a748e76925f94588f83339cad85a0fddd41f.
Peer scan: no prior reviews on this PR at review time.
Stack context: this PR against studio-panel-2-selection (part 3/7); paired with #1909 (walked both together for wire coherence). Second-pass lens per James's ask; Via has runtime-interop.

Summary — Adds optional childSelector + childIndex to PatchOperation, plus a pre-pass in patchElementInHtml that resolves every op's target BEFORE any mutation. If any child locator misses, the whole batch aborts (matched:false, html:source), which routes into #1909's toast+revert path instead of the pre-fix "escape parent's innerHTML into <span> literals" corruption. Style-decl helpers extracted to sourceStyleMutation.ts (file-size cap), new ./source-mutation subpath export lets #1909's integration harness drive the real server function. The 297 deletions are the split/wrap/unwrap tests moved to sourceMutationSplitAndGroup.test.ts verbatim + the parseStyleDecls helper relocated — no production semantics lost (I diffed both moved blocks and re-verified case-by-case).

Concerns

• sourceMutation.ts:145-153 — resolveOperationTarget treats childSelector: "" (empty string) as "present" (not undefined), which sends it into parent.querySelectorAll(""), which in linkedom throws → caught → returns null → whole batch aborts. That's a safe failure mode, but it means an empty-string childSelector behaves as "unresolvable target" rather than "no child scope". #1909's builder never emits an empty string today, so this is theoretical — but the type widening leaves the door open (childSelector?: string accepts ""). Consider normalizing !op.childSelector (falsy-guard) at the resolve boundary, or narrow the type to a branded non-empty-string. Not a blocker; call it out for the record.

• sourceMutation.ts:206-209 (text-content branch, post-resolve) — the "single-child unwrap" heuristic (opTarget.children.length === 1 ? opTarget.firstElementChild : opTarget) still fires AFTER opTarget has already been narrowed to a specific child. For a child-scoped text-content op targeting a <span> that itself has one styled <b> inside, this writes text into the <b>, not the <span>. Post-fix source ends up with <span><b>new text</b></span> — which is the same behavior as pre-fix parent-scoped ops, so it's consistent, but it means "same-shape multi-field edit where a child has a single inner wrapper" persists as inner-wrapper text. #1909's buildTextFieldChildOperations returns null unless every field's source === "child" and no text-node sources — so today the paired builder never routes an op to a wrapper-containing child, but the server accepts one if it arrives (e.g. from a future caller). Worth a comment in patchElementInHtml noting the unwrap heuristic applies at the resolved target level, not just parent-level.

• sourceMutation.ts:127-130 — the widened isHTMLElement(el: Node) signature is correct (the caller in splitElementInHtml now feeds cloneNode(true) return which is Node, not Element), but the fallback "style" in el on non-linkedom-window nodes now runs on arbitrary Node types. Text nodes coincidentally don't have style, so this is safe; comment nodes and doctype don't either. Fine as-is — mentioning because the type widening moves this from "always Element" to "any Node", and I want to make sure future callers know the fallback is Node-safe by accident, not by design.

Nits

• sourceMutation.test.ts:88-109 — the "rejects the whole batch when a child-scoped operation cannot resolve" test verifies matched:false and html:source (byte-identical), which is exactly right. Consider also asserting that the FIRST op's inline-style write (op[0] targets a resolvable child) does NOT partially land — same test, one extra assertion (expect(result.html).not.toContain('color: blue')). Belt-and-suspenders against a future refactor that switches from pre-pass-then-apply to inline-abort. (nit)
• sourceStyleMutation.ts:2 — file-level fallow-ignore-next-line complexity on the parse function is honest but the split was motivated by the parent-file size cap, not the parse-function's complexity. Docstring at the top of the new file would explain the split rationale for future maintainers (mirrors the intent of #1909's .fallowrc.jsonc note style). (nit)

Cross-stack interactions

• With #1909 — this PR's ./source-mutation subpath export is the single reason #1909's persistSeam.integration.test.ts can drive the REAL patchElementInHtml instead of a fixture. That's the load-bearing wire between the two PRs: the integration harness proves the FE builder → BE parser roundtrip against the actual production parser, not a shim. This defuses the "parity-test reflexivity tautology" trap. Solid.
• With #1906 (root fixture PR) — the persist-seam harness in #1909 reads the design-panel-qa fixtures from #1906. #1908's tests do NOT depend on #1906 (they're in-file source strings), so #1908 could theoretically ship without #1906 landing first. Given Graphite stack constraints that's moot, but worth noting the coupling asymmetry.
• With #1910 (persist-failure toast) — this PR's matched:false abort is what #1910's toast+revert consumes. Verified by reading the shape at useDomEditCommits.ts:214-218 (studio side): the server's {matched:false, html:source} produces patchData.changed === false && patchData.matched === false, which fires the unresolvable-target branch. Contract matches; #1910 will exercise it.

Questions

• The pre-pass resolves EVERY op's target before any mutation, but if an op mutates the DOM in a way that changes the resolvability of a LATER op (e.g. op[0] removes a <span> sibling, op[1]'s childIndex:2 was resolved before op[0] fired), the pre-pass holds a stale HTMLElement reference. In the current op set (inline-style, attribute, html-attribute, text-content) none of these should re-parent or remove children, so the reference stays live. Is that assumption documented anywhere? A comment on resolveOperationTarget saying "callers must not add mutation ops that re-parent resolved targets" would future-proof this against a remove-child or insert-child op ever landing.
• Any observability hook for the matched:false abort path? On a batch-abort the server returns 200 with matched:false, html:source, which is indistinguishable from a normal no-op at the top-level metric layer. If the "child locator drifted" case starts firing at scale we'd want to know via a counter. Not a blocker; punt to a follow-up.

What I didn't verify

• The linkedom behavior on empty querySelectorAll("") — I asserted it throws based on the wrapping try/catch, but didn't actually run it. If linkedom instead returns [], the abort is even cleaner (no throw needed). Same net effect; just wanted to flag the untested assumption.
• Cross-repo consumers of studio-server's patch API — I didn't grep pacific / EF / SDK for patch-element route consumers. The route accepts an unchanged {target, operations} shape (optional fields are backwards-compatible for old clients), so I'm reasonably confident nothing breaks; a wire-schema-versioning question for the API team if you disagree.
• Full test suite run — took the PR body's "218 tests green" at face value; CI is green.

— Rames D Jusso

[vanceingalls]

🟢 Clean batch-abort seam; wire contract is coherent

The two-phase resolve-then-apply is well-designed: patchElementInHtml fills a ResolvedPatchOperation[] in a pre-pass, and any child-locator miss returns { matched: false, html: source } before a single setAttribute or textContent = runs. Because the mutation happens on the linkedom in-memory document (never a per-op write), the "no partial write" property holds by construction — the disk write in routes/files.ts is gated on patched !== originalContent, which is only true after a successful full apply. Clean.

Findings (runtime-interop lens)

1. Wire shape is versioned by presence, not by tag — acceptable for a same-repo seam — 🟢

File: packages/studio-server/src/helpers/sourceMutation.ts:132-138

PatchOperation widens by adding two optional fields (childSelector?, childIndex?). Old clients that don't emit them get old behavior (resolveOperationTarget returns parent on childSelector === undefined). New clients that emit them get the new behavior. Since server + client deploy together and the wire is same-repo JSON, no discriminant tag is required — good pragmatic choice. Worth a note in a doc: any future op-type addition should also stay presence-additive (never mutate an existing type value) or the compat window breaks.

2. Route-handler distinguishes matched:false from changed:false correctly — 🟢

File: packages/studio-server/src/routes/files.ts:1752-1772

The POST /file-mutations/patch-element/* handler returns ok: true, changed: false, matched: false (HTTP 200) on batch-abort, keeping matched in the body so the client can distinguish "nothing to write" from "one child locator missed." The client's useDomEditCommits.ts already reads matched === false (analytics event save_skipped_unresolvable), so the wire contract is coherent. The user-visible toast+revert that lets a user see the failure is #1910's job — the PR body reads as if it lands here ("routes into the client's unresolvable-target error path (toast + revert)"), but at this SHA the toast doesn't fire. Suggest tightening the PR description or leaving a follow-up marker; not a blocker.

3. isHTMLElement widening Element → Node — safe under audit — 🟢

File: packages/studio-server/src/helpers/sourceMutation.ts:127-130

Widening to Node is required because cloneNode (line 165 in the split path) returns Node, and passing an unnarrowed clone into the old signature would have been a type error. The added el.nodeType === 1 gate on the cross-realm fallback correctly restricts to element nodes when defaultView.HTMLElement is unavailable. resolveOperationTarget's only call passes elements from querySelectorAll, so no non-element leaks in. OK.

4. Idempotency on retry — pragmatically fine — 🟢

File: packages/studio-server/src/helpers/sourceMutation.ts:156-219

If a client retries a batch after a transient network failure, the second run reads the freshly-written originalContent and re-applies the same ops. Style/attribute/text-content ops are all idempotent under repeat (setting color: red twice = color: red), and matched is deterministic. No stateful accumulator, no counter drift. Reasonable.

5. Batch abort discards all operations, including safe ones — by design — 🟢

File: packages/studio-server/src/helpers/sourceMutation.ts:166-171

If the batch has 3 ops and only op #2 has a bad child locator, all 3 are dropped. That's the correct "all-or-nothing" contract given the client builds a whole-selection commit as a single logical edit. Confirms the design intent — worth stating explicitly in the exported function's docstring so a future contributor doesn't "fix" it into partial apply.

6. -297 line drop is a helper extraction, not a compat break — 🟢

File: packages/studio-server/src/helpers/sourceStyleMutation.ts:1-58

parseStyleDecls / serializeStyleDecls / patchStyleAttrString move verbatim from sourceMutation.ts to sourceStyleMutation.ts (repo file-size cap). Same behavior, same output. No wire-shape or public-surface change. parseStyleDecls is now exported (for #1909's harness); the export doesn't leak internal state. Clean extraction.

Cross-PR interop (#1908 ↔ #1909)

Server PatchOperation (this PR, sourceMutation.ts:132) and client PatchOperation (#1909, sourcePatcher.ts:90) are two structurally-identical but separately-declared interfaces. That's the ordinary consequence of the monorepo package boundary — TypeScript won't catch drift if server adds a new op type. #1909 handles this by driving the real server function through real client builders in persistSeam.integration.test.ts; that's the right defense, and the harness would catch any wire-shape divergence in future PRs. Server-side test coverage of the wire contract is only via the unit tests in sourceMutation.test.ts — those don't exercise the client-builder call site. Consider adding a lightweight round-trip fixture on the server side too, so the seam stays covered from both directions if either side changes independently.

Batch abort round-trip (server aborts → client route returns matched:false → client hook logs analytics) is exercised on the server side (sourceMutation.test.ts's "no partial write" case) and on the client seam (persistSeam.integration.test.ts runtime-node matched:false case). End-to-end toast+revert path exists only after #1910 lands.

---

Review by Via (runtime-interop lens)

[james-russo-rames-d-jusso]

R2 verification — reviewed at acf7a748e76925f94588f83339cad85a0fddd41f (unchanged from R1; Miguel deferred both R1 🟠 as non-blocking-today rather than pushing new commits).

Peer scan since R1: Via (@vanceingalls) posted a runtime-interop lens review (id 4627877870) at 2026-07-03T20:07:19Z — six 🟢 findings covering wire-shape widening, matched:false vs changed:false distinction, isHTMLElement narrowing, idempotency on retry, batch-abort all-or-nothing contract, and the sourceStyleMutation.ts extraction. No dissent from R1; the additional signal (batch abort discards even the safe ops in a batch — by design) is a good clarification worth reading. Layered here, not restated.

Verification of R1 findings

🟠 F1 — empty-string childSelector falls into try/catch — resolveOperationTarget at sourceMutation.ts:145-153 unchanged.
Line 146 still guards on op.childSelector === undefined (strict), so an empty-string childSelector still bypasses the guard and runs parent.querySelectorAll("") inside the try/catch. That .querySelectorAll("") throws a SyntaxError under linkedom (as under DOM), the catch returns null, and the batch aborts with matched:false — so today's behavior is safe-abort, not silent-wrong-write. Verified at HEAD that #1909's buildTextFieldChildLocator (domEditingLayers.ts:188-208) either returns a proper :scope > <tag> string or null, never an empty string. So F1 stays as-recorded: non-blocking today, guarded by builder shape rather than by resolveOperationTarget itself. Worth revisiting if a future op-builder ever emits childSelector: "" (test the boundary explicitly).

🟠 F2 — text-content unwrap heuristic still fires post-resolve — sourceMutation.ts:205-211 unchanged.
The textTarget = opTarget.firstElementChild if opTarget.children.length === 1 heuristic runs after resolution. If a client op supplies childSelector pointing at a child element that itself has a single inner wrapper, the textContent write descends one level further than intended. Non-blocking today because #1909's client only builds text-content ops for editable-leaf children (isEditableTextLeaf(el) in domEditingLayers.ts:35-37 requires el.children.length === 0), so post-resolve there is nothing to unwrap into. As with F1, the safety property lives at the builder, not at resolveOperationTarget — so a future client emitting text-content against a non-leaf resolved child would silently target the inner wrapper. Suggest a small op.childSelector === undefined gate on the unwrap branch so the resolved-child path is authoritative; happy for it to land in a follow-up.

New at HEAD

🟢 Miguel's deferral rationale is coherent.
Read the "verdict summary" comment posted 2026-07-03T20:24:47Z — both R1 🟠 explicitly deferred with the reasoning that #1909's actual builder emissions don't exercise the edges. Cross-checked against domEditingLayers.ts + domEditTextFieldCommitOps.ts at HEAD; the deferral is grounded (builders neither emit empty-string childSelector nor text-content against non-leaf resolved children). Accepting as-recorded per the open-item-alternative-resolution rubric — the operational goal (no source corruption via child-scoped ops) is met by the current shape.

Cross-stack recheck

• ./source-mutation subpath export (this PR) is consumed by #1909's persistSeam.integration.test.ts — verified at packages/studio/src/hooks/persistSeam.integration.test.ts at #1909 HEAD; harness drives the real server function through real client builders.
• matched:false return contract (this PR) is what #1910's toast+revert consumes at the client hook seam — per Miguel's summary, #1910 has landed on that end.
• No new cross-stack surprises since R1.

CI

All required checks green at acf7a748. Graphite mergeability still in-progress (stack-order pending), non-blocking.

Verdict

🟢 approve pending stack — the two 🟠 stay open-but-recorded as fence-not-guard concerns; both are one-line fixes at sourceMutation.ts if #1908's server contract ever gets a caller outside #1909's builder. Not blocking merge given the current stack, but I'd take the follow-up if you agree the fence-vs-guard framing is fair.

— Rames D Jusso

[vanceingalls]

🟢 R3 verification — restack-only, R2 verdict stands

R3 verification — reviewed at 1119d024a2b511dba3a25f88b68cead4800004f2 (R2 was acf7a748). New tip carries the same commit message fix(studio-server): child-scoped patch operations with batch abort; SHA change is a Graphite restack onto the same 8 new base commits picked up by #1906 + #1907. File list is identical to R2 (6 files under packages/studio-server/ — same scope, same shape).

No behavior delta on #1908 at R3. The two R2 🟠 findings I dispositioned as non-blocking-today (both hinge on #1909's builder not emitting the shapes that would expose them) are unchanged. Since #1909 is also restack-only at R3, the coupling still lives at #1910's wiring — same disposition as R2. 🟢 stands.

---

R3 by Via (runtime-interop lens)

[jrusso1020]

APPROVE — part of the 7-PR studio-panel fix stack (#1906–#1912), stamped as a batch after independent verification.

Verified:

• Dual-reviewer convergence at R4: RDJ (correctness/architecture lens) + Via (runtime-interop lens) both green on all 7, cross-reconciled with matching file:line citations across four rounds. This PR's head is exactly its R4-cited SHA — no drift since the verdict.
• CI green: all test/lint/build checks SUCCESS; the single pending check is Graphite's mergeability_check (a stack-merge-readiness computation, not a code gate).
• Spot-checked the coupled residual (#1910): the persist-hook triangle is coherently closed — StudioSaveHttpError.alreadyToasted (single-toast invariant), prepareContent write-fail preserves the already-persisted server finalContent, and text-commit shouldRevert widened to any persist failure. #1911's new cells exercise these paths.

Non-author stamp clearing the review gate; merge via the normal Graphite path (bottom-up, no admin-merge). Layered on RDJ + Via.

— Rames Jusso

The base branch was changed.