fix(studio): extend resolver-shadow runtime-node filter to timing/GSAP/reorder chokepoints

[vanceingalls]

Summary

Live telemetry (dashboard 1732039, studio:sdk_resolver_shadow) showed that after the v0.7.22 runtime-node filter shipped (PR #1795), the entire residual element_not_found noise moved to the timing/GSAP chokepoints — the one filtered path (DOM-edit) went quiet, but recordResolverParity (used by setTiming/addGsapTween/removeElement/reorderElements) had no such filter. This extends the same source-presence filter there.

• recordResolverParity is now async with an optional readSource?: () => Promise<string | undefined>. The cheap resolve-check still runs first, so the source read only happens on a real divergence. When present, it suppresses runtime-generated nodes (mirrors the existing DOM-edit filter) and emits sourceHfIdCount for triage.
• Wired into 3 of 4 call sites in sdkCutover.ts (setTiming, addGsapTween, removeElement) plus the 4th (reorderElements in useDomEditSession.ts) after a follow-up review.
• A pure, verified-byte-identical extraction (sdkCutoverEligibility.ts) was needed mid-branch to stay under the packages/studio 600-line filesize cap.

Review history

Built via subagent-driven development (2 planned tasks + 1 unplanned extraction task, each independently reviewed and approved), then a whole-branch review, then a max-effort independent review that found 4 additional issues — all fixed in the final commit:

1. Race condition (real bug): the fire-and-forget async call let the caller's synchronous session mutation run before sessionElementCount was read, so the field could reflect post-edit state instead of the pre-edit state it's meant to diagnose. Fixed by capturing the count before the await.
2. A pre-existing test call wasn't updated to await when the function became async — fixed, plus verified no other test relies on the old sync behavior.
3. Coverage gap: the reorderElements chokepoint was left unfiltered (initially a deliberate YAGNI call, since live telemetry showed 100% of the residual coming from the other two paths) — extended anyway per follow-up, reusing an already-passed-but-unused readProjectFile prop. New test renders the real hook (with sub-hooks mocked) and asserts the reader is now wired.
4. A comment falsely claimed an invariant that doesn't hold given the suppression check's intentionally loose substring match vs. the count's strict attribute match — corrected in 3 places, no logic change.

Every fix was independently re-verified against the diff (not just the implementer's report) before merge.

Test plan

• sdkResolverShadow.test.ts: 37/37 (5 new cases: suppression, present-but-unresolved, duplicate-id, no-reader, fail-open)
• sdkCutover.test.ts: 54/54 (unaffected by the eligibility extraction — verified byte-identical)
• useDomEditSession.test.tsx: 6/6 (1 new: reorder chokepoint wiring)
• Full packages/studio suite: 1216/1216, no regressions
• typecheck + oxlint + oxfmt clean throughout

🤖 Generated with Claude Code

[miga-heygen]

Review — fix(studio): extend resolver-shadow runtime-node filter to timing/GSAP/reorder chokepoints

Verdict: LGTM ✅

What this does

After PR #1795 filtered out runtime-generated nodes from the DOM-edit shadow path, all the residual element_not_found noise moved to the three remaining chokepoints (setTiming, addGsapTween, removeElement) plus reorderElements. This PR extends the same source-presence filter to all four, with a well-designed async reader pattern.

Correctness

Race condition fix — the headline change. Making recordResolverParity async means the caller's synchronous session mutation runs before readSource resolves. Pre-capturing sessionElementCount before the await is critical — without it, the telemetry field would reflect post-edit state instead of the pre-edit state it exists to diagnose. Verified: the capture sits between resolveSnapshot() (sync, step 1) and await readSource() (step 2), so it always reflects pre-mutation state.

Fire-and-forget semantics. All four call sites use void recordResolverParity(...). The outer try/catch (preserved from the original function body) prevents unhandled promise rejections. The inner try/catch around readSource() provides fail-open semantics — a read error sets source = undefined, which skips suppression and emits the event. Correct: observability should never silently drop a genuine divergence because of an unrelated I/O failure.

Source reader wiring per call site:

• sdkTimingPersist: () => readProjectFile(targetPath) — reads the active file. ✅
• sdkGsapTweenPersist: same pattern, only for op.kind === "add" (non-add ops use recordAnimationResolverParity, which is unaffected). ✅
• sdkDeletePersist: () => Promise.resolve(originalContent) — the content is already available in memory, so no I/O needed. Clever and correct. ✅
• onReorderShadow (useDomEditSession): () => readProjectFile(activeCompPath) via the existing prop. The _readProjectFile → readProjectFile rename correctly drops the void-discard line. ✅

Loose vs. strict match semantics. The suppression check (source.includes(hfId)) is intentionally loose (substring match, biased toward keeping signal). The sourceHfIdCount field uses strict data-hf-id="..." attribute matching via countHfIdInSource. This means an emitted event can legitimately carry sourceHfIdCount: 0 — the comments at L759-763 and L773-780 document exactly when and why. Accurate and well-explained.

Extraction (sdkCutoverEligibility.ts)

Pure code motion to stay under the 600-line file cap. All constants, types, and functions are identical. The shouldUseSdkCutover is both imported locally (for use in persist functions) and re-exported (for external consumers like tests and useDomEditSession). No circular dependency risk — sdkCutoverEligibility.ts depends only on sourcePatcher types and htmlAttrSafety, neither of which depends on sdkCutover.ts.

Tests

Strong coverage — 5 new behavioral test cases in sdkResolverShadow.test.ts:

1. Suppression: runtime node (hfId absent from source) → no emit
2. Present-but-unresolved: hfId IS in source but missing from session → emit with sourceHfIdCount: 1
3. Duplicate-id ambiguity: two data-hf-id matches → sourceHfIdCount: 2
4. No reader (backward compat): no readSource → emit without sourceHfIdCount
5. Fail-open: readSource throws → emit without sourceHfIdCount

All existing tests updated to await the now-async function. The useDomEditSession.test.tsx (277 lines, 167 of which are mock stubs) is heavy but structurally required for testing a React hook that composes 9 sub-hooks — the single test case verifies that recordResolverParity receives the source reader as its 4th argument when sdkSession and activeCompPath are set.

Ponytail lens

sdkCutoverEligibility.ts:L1-116: Pure extraction, no new abstraction. Forced by file cap. ✅
useDomEditSession.test.tsx: 167 lines of mock surface for 1 test. The alternative (extracting the callback factory to a standalone function) would change architecture for testability — keeping it as-is is the right call for now, but worth noting the maintenance cost if this hook adds a 10th sub-hook.

Lean already. Ship.

Nit (non-blocking)

sdkCutover.ts:L213-217 — the } else clause after a braced if body uses statement form (recordAnimationResolverParity(...)) without braces. Cosmetic only; won't block.

— Miga

[james-russo-rames-d-jusso]

Reviewed at 676e094021b9c66ba71e7031a84ddeb7c9aac3f0. Solid extension of the runtime-node filter to setTiming/addGsapTween/removeElement/reorderElements; the "capture sessionElementCount before the await" fix is a real bug caught, and the fail-open on read error is the right posture for telemetry.

🟠 Must-fix

• AI-authorship trailer (repo convention). PR body has the 🤖 Generated with [Claude Code] footer, and commit f25d6dd162 has a Co-Authored-By: Claude trailer. HF enforces stripping these before merge — either amend to remove the trailer + drop the footer from the body, or (if the repo now permits them) confirm the convention has changed. Flagging per current rubric; not a code-quality concern, purely convention.

🟡 Concerns

• useDomEditSession.ts:297-299 — N-target readProjectFile fan-out. For a reorder that produces N failing-to-resolve targets, the loop invokes void recordResolverParity(sdkSession, target, "reorderElements", reorderSrc) N times, and each one independently awaits reorderSrc() → readProjectFile(activeCompPath), which is an HTTP fetch("/api/projects/<pid>/files/<activeCompPath>") (useFileManager.ts:69-77). Same file, N concurrent GETs. Practically small (only fires on the failure branch, which was rare enough that a filter is being added), but worth single-flighting since the source is stable across all targets in one reorder:

  onReorderShadow: sdkSession
    ? (targets: string[]) => {
        // Hoist the read: one promise shared by every target in this batch.
        const readOnce = activeCompPath
          ? (() => { let p: Promise<string> | undefined; return () => (p ??= readProjectFile(activeCompPath)); })()
          : undefined;
        for (const target of targets)
          void recordResolverParity(sdkSession, target, "reorderElements", readOnce);
      }
    : undefined,

  Same suppression semantics, one fetch per reorder instead of N. (Not blocking — the source-read only fires when resolveSnapshot misses, so cost is bounded by the failure population the filter is meant to suppress. Worth doing while we're here.)

• sdkResolverShadow.ts:336 + runResolverShadow:274-288 comment — loose-vs-strict asymmetry surfaces on the dashboard as sourceHfIdCount: 0. The suppression check uses source.includes(hfId) (loose substring) while countHfIdInSource requires the exact data-hf-id="<id>" attribute. That means the emitted-with-sourceHfIdCount=0 cohort mixes two very different failure modes: (a) hfId in source only as plain text (class, comment, string literal — treat as "not a real attribute occurrence" per the comment), and (b) any future case where the strict counter diverges from the loose check for a real reason. Once this hits Datadog/PostHog we'll have to bisect that by grepping mismatches text — expensive at scale. Suggest tagging the two apart at emit time:

  const strictCount = source !== undefined ? countHfIdInSource(source, hfId) : undefined;
  trackStudioEvent("sdk_resolver_shadow", {
    hfId, opLabel, sessionElementCount,
    ...(strictCount !== undefined ? { sourceHfIdCount: strictCount } : {}),
    // Loose match kept, strict did not — hfId appeared as text but not as data-hf-id="…"
    ...(strictCount === 0 ? { sourceLooseMatchOnly: true } : {}),
    …
  });

  Dashboard can then filter (a) out with a single tag predicate instead of inferring "0 == not-a-real-attribute" from the comment. Same shape as the existing sessionElementCount === 0 vs > 0 distinction — tagging the class up front > mining it out later. Same suggestion for runResolverShadow.

🟢 What looks right

• Fire-and-forget void recordResolverParity(...) at every production call site + the captured sessionElementCount BEFORE the await — that's the real bug the PR body calls out, and it's implemented + doc-commented correctly at sdkResolverShadow.ts:325.
• Fail-open on readSource() error (sdkResolverShadow.ts:329-333) — telemetry never drops a real divergence because of an I/O flake. Tested at sdkResolverShadow.test.ts:417-424.
• Delete-path (sdkCutover.ts:486-488) supplies originalContent synchronously via Promise.resolve — no extra HTTP round-trip, correct lock-in-time source (mirrors the exact bytes the delete sees).
• Test coverage across (suppression / present-but-unresolved / duplicate-id / no-reader / fail-open) is genuinely thorough; the new useDomEditSession.test.tsx renders the real hook with sub-hooks mocked and asserts the 4th-arg wiring — solid check for the reorder chokepoint being live.
• sdkCutoverEligibility.ts extraction reads byte-identical to the moved block (verified against the diff), and the 507-line result stays comfortably under the 600-line cap. sdkCutover.test.ts unaffected because it imports shouldUseSdkCutover via the re-export at sdkCutover.ts:481.

What I didn't verify

• Live PostHog studio:sdk_resolver_shadow schema — whether sourceHfIdCount: 0 events already exist in prod and whether the dashboard has a filter for them. Suggestion above assumes we want to prevent that class from being needed; if the dashboard is already grepping mismatches text, my suggestion adds a tag for future use rather than fixing a live gap.
• The claim that the 5 preserved shouldUseSdkCutover tests in the new .tsx are byte-identical to the deleted .ts — scanned by eye, they look identical, but didn't diff them file-vs-file. Low risk given the 6/6 pass in the PR body.

Review by Rames D Jusso

[miguel-heygen]

Code review LGTM; holding on the repo-convention envelope only.

Strengths: packages/studio/src/utils/sdkResolverShadow.ts:320 correctly captures sessionElementCount before the first await, preserving the pre-mutation diagnostic value despite the fire-and-forget call sites. packages/studio/src/utils/sdkResolverShadow.ts:328 fail-opens on source-read errors, so observability never suppresses a real resolver divergence because file I/O failed. The source readers are wired at the right chokepoints: timing/GSAP add/delete/reorder all pass source content without changing the user-visible persist path.

Blocker: PR metadata still carries AI-attribution envelope that this repo has been stripping before merge: PR body line 28 has the Generated with Claude Code footer, and commit f25d6dd1 has Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>. Strip the footer and amend/rebase the commit trailer away, then this is stampable.

Notes: CI is green. I attempted the three focused test files locally, but the isolated worktree has no installed deps (react/jsx-dev-runtime / @hyperframes/sdk unresolved); relying on the green CI run for execution.

Verdict: REQUEST CHANGES
Reasoning: The implementation matches the intended async source-reader design and I found no code blocker, but the metadata convention gate is still present.

— Magi

[miguel-heygen]

Approving to supersede my earlier trailer-only hold. The implementation itself is sound at 676e0940: recordResolverParity captures sessionElementCount before the first await, source-read failures fail open, and the timing / GSAP add / delete / reorder chokepoints all pass the relevant source content without changing the user-visible persist path.

RDJ's two telemetry concerns are valid follow-ups rather than blockers from my read: reorder can fan out readProjectFile per unresolved target, and sourceHfIdCount: 0 mixes loose-match-only cases with not-attempted/strict-zero ambiguity. Neither breaks the filter or persistence path, and CI is green.

Verdict: APPROVE
Reasoning: Code path is correct, test coverage covers suppression / fail-open / duplicate count / no-reader behavior, and the remaining observations are observability polish rather than merge blockers.

— Magi