Repository navigation
fix(runtime): the editor keeps waking during a drag on a preview it did not build - #3850
Conversation
…id not build The manual-edit gesture watch tested mutation targets with `instanceof Element`. The composition body is adopted into the preview frame, so its nodes answer to another realm's Element and the check is false for every one of them: the watch never sees a gesture, and the paused transport it gates does not wake while the user drags. Routes the check through the runtime's structural predicate, which is what the preview-guard lint added alongside it now requires. Main is red on that lint for this line, so this also unbreaks it. Test adopts an element from a second realm and asserts the marker is seen and cleared; it fails with the identity check restored.
terencecho
left a comment
There was a problem hiding this comment.
APPROVE at 02743c710a57483c4e2bc53c2573043a1d259d14 — unbreaks main's Test: runtime contract lint by fixing #3845's instanceof Element at manualEditGestureWatch.ts:56 via the structural predicate #3848 shipped, and closes a real cross-realm correctness gap (the gesture watch has been silently skipping every mutation record from adopted preview nodes since #3845 landed).
Author + scope. miguel-heygen (trust-listed). Head 02743c710. Base main. +28/-1 across 2 files (manualEditGestureWatch.ts + manualEditGestureWatch.test.ts). mergeStateStatus BLOCKED (missing approval — first at-head review, zero others). No CI failures at snapshot; some checks still queued.
The one-line prod change
packages/core/src/runtime/manualEditGestureWatch.ts:56 swaps if (!(target instanceof Element)) continue; for if (!isElementNode(target)) continue;, importing isElementNode from ./domRealm (line 2 — the module #3848 introduced). Structural check is nodeType === 1 per domRealm.ts:50 — cross-realm safe because Node.ELEMENT_NODE === 1 is a numeric constant identical across realms. Same tag-identity as the OLD instanceof Element on same-realm nodes; strictly wider on cross-realm nodes, which is the point.
Why this is both lint AND correctness
- Lint:
#3848'spackages/core/scripts/lint-runtime-preview-guards.tsbansinstanceof Elementinsrc/runtimevia/\binstanceof\s+(?:Element|Node|Text|Document|DocumentFragment|ShadowRoot|HTML[A-Za-z]*Element|SVG[A-Za-z]*Element)\b/.manualEditGestureWatch.ts:56in #3845 predates that rule but landed on main after; each was green alone, they collide on the merge — classic same-cause independent-branch collision. - Correctness: the composition body is
document.adoptNode-d into the preview frame, so its element nodes carry the SOURCE realm's Element prototype.target instanceof Elementcompares against the PREVIEW realm's Element → false for every mutation record from the composition body. The gesture watch'singestloop skipped 100% of those records, soisActive()stayed false for every drag whose marker was set on an adopted node, and the parked runtime transport (init.ts:3410'scanParkTransport) never woke on drag start. Same class of bug #3848 was built to end — this just retires the one instance that snuck into the same runtime module in a sibling PR.
Test (the invariant Miguel's fix pins)
manualEditGestureWatch.test.ts:37-58 adds sees a gesture on an element adopted from another realm:
- Builds a foreign JSDOM realm, creates a
divin it,document.adoptNode-s it into the test-doc'sbody. - Line 49 explicit pin:
expect(el instanceof Element).toBe(false)— proves the test exercises the bug scenario (adoption doesn't rewrite the prototype chain; the element'sElementstill refers to the foreign realm). - Sets the gesture marker →
watch.isActive()returns true (mutation reached ingest, added tomarkedSet). - Removes the marker →
watch.isActive()returns false (mutation reached ingest, cleared frommarkedSet).
Reverting the swap to instanceof Element flips the first expect(watch.isActive()).toBe(true) red — the ingest loop skips the record, marked stays empty. Reverting only the CLEAR-path handling (e.g., skipping the marked.delete(target) branch) flips the second .toBe(false) red. Both directions of the predicate matter and both are pinned.
Test file itself contains el instanceof Element — that's fine, the lint's isScannableSource filter skips *.test.ts, so the assertion doesn't re-red the guard.
No orthogonal changes, no scope creep
Diff is exactly two files, exactly the two mechanisms above. No shared-decorator hooks touched, no adjacent predicate migrations, no shape changes.
Miguel's callouts acknowledged
- #3846 retarget note: understood — you retargeted #3846 to main at
d9081f03d(unchanged head), CI fails the same lint through its base, will go green after this merges. I'll stamp #3846 atd9081f03das soon as this lands + CI turns green on it. - #3845 follow-ups noted post-merge: the explicit park-then-event interleaving test AND the
init.ts:3499double-arm timer overwrite — both accepted as post-merge follow-ups. Fine.
— Review by tai (pr-review)
mainis red. The preview-guard lint added in #3848 bans realm-bound DOMinstanceofundersrc/runtime, and the gesture watch merged in #3845 has one atmanualEditGestureWatch.ts:56. Each PR was green alone; they collide only once both are on main.It is not only a lint failure. The composition body is adopted into the preview frame, so its nodes carry another realm's prototypes and
target instanceof Elementis false for every mutation record. The watch therefore never reports a gesture, and the transport it gates does not wake while the user drags a layer.Change
The record target goes through
isElementNodefromdomRealm.ts, with a comment naming why an identity check cannot work here.Verification
manualEditGestureWatch.test.tsinstanceof Elementrestoredtest:hyperframe-runtime-citypecheck plus preview-guard lint plus runtime testsThe new test adopts an element from a second realm, asserts
el instanceof Elementis false, then asserts the watch sees the marker appear and clear.test:runtime-coveragefails on this machine for thirty-odd untouched files with a module-mocking error, identically with and without this change, so it is not from this diff.