Skip to content

fix(editor): stop renderers from reading removed objects and revert the #75 special case - #80

Open
Rn86222 wants to merge 9 commits into
mainfrom
fix/zombie-children
Open

Rn86222 wants to merge 9 commits into
mainfrom
fix/zombie-children

Conversation

@Rn86222

@Rn86222 Rn86222 commented Nov 14, 2026 •

Copy link
Copy Markdown
Contributor

Summary

This PR fixes the "zombie child" problem properly and removes the special case added in #75.

A renderer could be re-rendered by its own subscription before the list it comes from dropped it. #75 hit this when resizing a display dropped a connection:

  • The renderer of the connection is subscribed to the nodes at both ends, so it re-rendered.
  • It looked up a connection that was already gone.
  • The list of connections still held it, because the list was cached until didUnregister arrived. A removal is announced only when its transaction ends.

#75 worked around this by announcing the removal of a connection synchronously, unlike every other store. The transaction delays didUnregister on purpose (63e7069, "guarantee store consistency on did- events"). What was wrong were the caches that relied on that announcement to learn about a change, so this PR fixes the caches instead.

Changes

  • Lists are read from the store on every render (src/store/react/selectors.ts). They used to be cached with memoize-one until didUnregister arrived. The previous array is handed out again while its items are the same, so React still skips re-renders when nothing changed.
  • Renderers render nothing once their object is gone.
    • The hooks reading a single object (useNode, useComponent, useComponentPin) return undefined instead of throwing.
    • The renderers of connections, nodes and interface nodes return null after all their hooks when their object is missing.
    • ensureStoreItem is removed: it checked in a wrapper, which a subscription of the inner component bypasses.
  • The revision counts a change when it is made, not when it is announced.
    • The object-specific stores change their objects only through setObject and deleteObject of the base store, which count the change. The map itself is read-only to them. Moving a node still does not count.
    • The bit width cache is rebuilt when the revision differs from the one it was built at. The node pin and connection stores no longer clear it by hand after a removal, and nothing listens to events just to clear it.
    • The simulation netlist, cached on the same revision, is now exact during a transaction too.
  • Connections announce their removal when the transaction ends, like the other stores (reverts the Announce the removal of connections and node pins right away #75 special case).
  • Context menu:
    • Right-clicking a selected connection keeps the selection, so several connections can be deleted together. This matches the context menu of a node.
    • The menu acts only on selected objects that still exist.
    • Delete now clears the selection; it used to keep the removed connections selected.
  • Cleanup:
    • didUpdate of the component and component pin stores now carries the updated object instead of the one it replaced.
    • Removed the unused isMarkedAsDeleted, useComponentPins and useNodePins.
    • Removed the memoize-one dependency. Only its entries were removed from the lockfile.

Out of scope

"Create a new component" is only offered with nodes selected, and selecting a node clears the selected connections. The connections between the selected nodes are therefore neither copied into the new component nor kept. This is left for a separate change.

Testing

  • npx vitest run: 65 tests pass. New tests cover:

    • the list identity helper
    • the revision counting a removal before it is announced, and counting a config change but not a move
    • didUpdate carrying the updated object
    • a connection announcing its removal after the transaction
    • the bit widths reflecting a removed connection before the announcement

    The revision and bit width tests fail if a removal is not counted right away.

  • npx tsc -p . and npx biome check src pass.

  • Checked by hand in the browser, with no error in the console:

    1. Delete a connection, then drag a node at one of its ends.
    2. Delete a node with connections.
    3. Remove a split aggregate pin that has a connection.
    4. Delete an Input node inside a component used elsewhere, then open the outer component.
    5. Shift-click two connections, right-click one, and delete them, also right after deleting another connection.
    6. Delete the remaining selected connection after one of them was removed along with a pin.

🤖 Generated with Claude Code

Rn86222 and others added 4 commits November 14, 2026 13:35
The hooks returning a list (components, node ids, connection ids) cached
it with memoize-one and only dropped the cache when the event announcing
a change arrived. Some removals are announced only when their
transaction ends, so a component rendered in between got a list that
still held an object that was gone.

Read the list from the store on every render instead, and hand out the
previous array while its items are the same, so that React still skips
re-renders when nothing changed.

Also drop `useComponentPins` and `useNodePins`, which nothing uses, and
the memoize-one dependency.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A renderer can be re-rendered by its own subscription before the list it
comes from drops it. The renderer of a connection, for one, follows the
nodes at both ends, so moving one of them re-rendered it even after the
connection was removed, and it failed to look the connection up.

`ensureStoreItem` did not prevent this: it checked the object in a
wrapper, but the subscription re-renders the inner component alone.
Instead, the hooks reading a single object now return undefined once it
is gone, and the renderers of connections, nodes and interface nodes
call all their hooks first and then render nothing if their object is
missing. `ensureStoreItem` is removed.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… ends

#75 made connections announce their removal right away, unlike the other
stores, because a renderer could look up a removed connection before the
list of connections dropped it. The lists are now read from the store on
every render and the renderers tolerate a removed object, so that
special case is no longer needed. Announce it after the transaction like
the other stores.

The bit widths derived from a connection are stale as soon as it is
gone, so the connection store now clears the cache of the node pin store
right after the removal instead of on the announcement.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… menu

Right-clicking a connection always selected it alone, so several
connections selected with shift-click could not be deleted together.
Keep the selection when the clicked connection is part of it, as the
context menu of a node already does.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@cloudflare-workers-and-pages

cloudflare-workers-and-pages Bot commented Nov 14, 2026 •

Copy link
Copy Markdown

Deploying create-cpu-c8345196-9409-4cce-afc4-389f413ad8ee with  Cloudflare Pages  Cloudflare Pages

Latest commit: e119830
Status: ✅  Deploy successful!
Preview URL: https://c0128919.create-cpu-c8345196-9409-4cce-afc4-389f413ad8ee.pages.dev
Branch Preview URL: https://fix-zombie-children.create-cpu-c8345196-9409-4cce-afc4-389f413ad8ee.pages.dev

View logs

Rn86222 and others added 5 commits November 14, 2026 18:28
…idths on them

The bit width cache was dropped on the events announcing a change. A
removal is announced only when its transaction ends, so the cache was
stale in between, and the node pin and connection stores dropped it by
hand right after a removal to make up for that. The transaction delays
the announcement on purpose; the cache was listening to the wrong
signal.

Count the revision of the store at the moment an object is stored or
removed instead of on the announcement. The object-specific stores now
change their objects only through `setObject` and `deleteObject` of the
base store, which count the change; the map itself is read-only to them.
Moving a node still does not count. The bit width cache is rebuilt when
the revision differs from the one it was built at, so it needs neither
the event listeners nor the manual clearing. The simulation netlist,
cached on the same revision, is now exact during a transaction as well.

Also:
- announce the updated object in `didUpdate` of the component and
  component pin stores, which announced the one it replaced, and
- remove `isMarkedAsDeleted` of the node pin store, which nothing uses.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The selection can keep the ids of objects removed since, e.g. connections
removed along with a node pin, and Delete itself kept the removed
connections selected because it cleared the selection non-exclusively.
Since a right-click now keeps a selection containing the clicked
connection, such ids reached Delete, and removing a connection that was
already gone threw in `unregister` and left the others in place.

Filter the selection by what still exists before acting on it, and clear
the selection exclusively after Delete.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Also test that the component pin store announces an update with the
updated pin.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
# Conflicts:
#	src/store/base.ts
#	src/store/nodePin.ts
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant