Guard receivers against late mutations on detached nodes

[eddiechan]

TL;DR:

• Fixes https://github.com/shop/issues/issues/33295
• Silences noisy TypeError: undefined is not an object (evaluating 'x.properties') / TypeError: Cannot read properties of undefined (reading 'properties') errors that are polluting our logs (Observe link)

What this fixes

• RemoteReceiver and SignalRemoteReceiver now drop insertChild, removeChild, updateProperty, and updateText mutations whose target node is no longer in the receiver's attached map, instead of throwing a TypeError while dereferencing the missing node.
• This race surfaces in production as unhandled promise rejections such as:
  • Safari: TypeError: undefined is not an object (evaluating 'x.properties')
  • V8: TypeError: Cannot read properties of undefined (reading 'properties')
• This happens when a remote sender dispatches a mutation for a node whose host-side state has just been removed. For example, a removeChild for an ancestor was processed earlier in the same batch, or arrived first from a separate batched payload.

Why the existing guard was not sufficient

• PR Make removeChild in receivers less strict #533 (a port of Prevents missing child from causing removal of last node #522) added an if (!removed) return; guard to removeChild. That guard catches the case where the child slot at the given index was empty, but it runs after [...parent.children.peek()], which is the line that throws when parent itself is missing. It also did not touch updateProperty or updateText.
• Issue Unhandled Promise Rejection: TypeError: undefined is not an object (evaluating 'element.eventListeners') #542 reported the same TypeError shape on element.eventListeners and was closed under the assumption that Make removeChild in receivers less strict #533 covered it. As far as I can tell from the patch, it did not because both updateProperty and updateText are still vulnerable. We are seeing this fire several hundred times a week in Shopify checkout, exclusively grouped on the engine-specific error message text.

What this PR does

• It applies the same defensive pattern uniformly to every connection callback that dereferences attached.get(id), in both receivers:

const node = attached.get(id) as TheRightTypeOrUndefined;
if (!node) return;

• Late mutations targeting a detached subtree are by definition no-ops — there is nothing left to mutate. The call callback (the only other entry that takes an id) is unchanged because it already handles the missing-implementation case via an explicit thrown error, which is intentional and surfaces a clear name to consumers.

Tests

Added regression tests in:

• packages/core/source/receivers/tests/RemoteReceiver.test.ts
• packages/signals/source/tests/SignalRemoteReceiver.test.ts

Each suite covers:

1. insertChild after parent detach
2. removeChild after parent detach
3. updateProperty after element detach (all three type branches: PROPERTY, ATTRIBUTE, EVENT_LISTENER, plus the default-arg case that produced the production error)
4. updateText after text-node detach
5. The "same-batch" race: a removeChild and a subsequent updateProperty on a descendant in a single mutate([...]) payload
6. retain / release are not invoked for dropped updateProperty mutations

Verified by reverting the source guards and re-running: every new test fails with exactly TypeError: Cannot read properties of undefined (reading 'properties'), matching the production error.

pnpm vitest run (170 tests) and pnpm type-check are green.

---

Requested by Eddie Chan eddie.chan@shopify.com

[eddiechan]

Note: needed because the new test in packages/signals imports from @remote-dom/core. Without a quiltPackage() plugin in that package's vitest config, vitest tries to resolve @remote-dom/core from the not-yet-built build/ directory and fails. Mirroring packages/core/vite.config.js fixes resolution via quilt:source.

[simontaisne]

Nice one! I like that we're using the same pattern for all callbacks now, and the tests are pretty comprehensive. Good diagnosis too, the #533 guard runs after the line that actually throws when the parent is missing, so this was never covered. A few suggestions inline, but nothing blocking 👍

[simontaisne]

Should we add another test to confirm retain/release are still called when we updateProperty on an attached element?

[simontaisne]

Should we add a test for call() on a detached id to confirm it still throws the explicit “no implementation” error?

[simontaisne]

The new tests use not.toThrow() which confirms the TypeError is gone, but not that the receiver is truly a no-op. After a late insertChild, we could also assert that receiver.root.children.length stays unchanged.

Something like this

// Before the late mutation
const rootChildrenBefore = [...receiver.root.children];

// After
expect(receiver.root.children).toStrictEqual(rootChildrenBefore);
expect(receiver.root.children).toHaveLength(0);

[ncardeli]

Nice job!

[ncardeli]

All four new guards in this file include a short comment explaining the late-mutation rationale, but the pre-existing if (!removed) return; guard from PR #533 sits adjacent to them with no comment. A future reader scanning these stacked early-returns may wonder if one is redundant. A brief comment would parallel the new convention and reduce friction for the next maintainer.

        if (!removed) {
          // The slot at the given index was already empty (e.g. an earlier
          // late removeChild for the same index). Drop the
          // late mutation; nothing to remove.
          return;
        }