Skip to content

fix(canvas): stop infinite re-render on ContextMenu mount - #1544

Merged
HongmingWang-Rabbit merged 3 commits into
mainfrom
fix/canvas-contextmenu-infinite-loop
Apr 22, 2026
Merged

HongmingWang-Rabbit merged 3 commits into
mainfrom
fix/canvas-contextmenu-infinite-loop

Conversation

@HongmingWang-Rabbit

Copy link
Copy Markdown
Contributor

Summary

ContextMenu selector returned a new filtered array on every render → Zustand's `useSyncExternalStore` saw "snapshot changed" every tick → React scheduler hit the 50-update depth cap → crashes with minified error #185 ("Maximum update depth exceeded").

Visible as "Application error: a client-side exception" on every SaaS tenant once a session cookie resolved. My hongmingwang tenant hit this reliably today.

Root cause

```tsx
// ContextMenu.tsx before:
const children = useCanvasStore((s) =>
contextNodeId ? s.nodes.filter((n) => n.data.parentId === contextNodeId) : []
);
```

`filter()` returns a new array. `useSyncExternalStore` uses `Object.is` to compare snapshots — new arrays always fail that. Store → render → selector → new array → store hook thinks it changed → render again → infinite loop.

Fix

```tsx
// after:
const nodes = useCanvasStore((s) => s.nodes);
const children = useMemo(
() => (contextNodeId ? nodes.filter((n) => n.data.parentId === contextNodeId) : []),
[nodes, contextNodeId],
);
```

`nodes` reference is stable across unrelated store updates (Zustand only changes it when nodes themselves change). `useMemo` derives children without going through Zustand's subscription.

Same pattern to keep in mind elsewhere: never do `.filter` / `.map` / `.slice` / `{...}` inside a Zustand selector unless using `useShallow` or returning a primitive. `ProvisioningTimeout.tsx:39` uses the string-serialize trick for the same reason; worth a codebase-wide audit as follow-up.

Verification

  • Served dev bundle through cloudflared tunnel to the live hongmingwang tenant
  • Dev build surfaced the exact warning `The result of getSnapshot should be cached to avoid an infinite loop at ContextMenu.tsx:26:34` — pinpointed in 30 sec (vs hours of blind debugging on minified prod bundle)
  • After fix: React error fix(security): gate GET /approvals/pending behind AdminAuth (#180) #185 gone; canvas renders; only remaining console errors are expected localhost API 401s from dev server topology

Not in scope

  • Broader Zustand selector audit — separate issue
  • Production source-map enablement (would have caught this without the dev-tunnel workaround) — separate ops task

🤖 Generated with Claude Code

Hongming Wang and others added 3 commits April 21, 2026 19:50
Expanded the rollout section with the exact scripts + env vars
that landed to make Hermes workspace Terminal work on 2026-04-22.
Points at molecule-controlplane#227 (which adds bootstrap script +
EIC_ENDPOINT_SG_ID env var) so operators can reproduce the setup
on a new AWS account in one command.

Also documents the existing-workspace backfill for the instance_id
column — the CP only writes on new provisions, so pre-migration
workspaces need a manual UPDATE before Terminal routes to the
remote path.

Refs: #1528 (resolved)
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Review turned up two issues in the rollout runbook:

1. The tenant env-var list was missing — today's debugging burned 2
   hours on hongmingwang where everything worked infra-side but
   canvas 401'd because MOLECULE_ORG_SLUG and CP_UPSTREAM_URL weren't
   set. Doc without this sends the next operator down the same hole.

   Added a dedicated step-3 table covering CP_UPSTREAM_URL,
   MOLECULE_ORG_SLUG, MOLECULE_ORG_ID, AWS_REGION with the exact
   failure mode each one produces when missing.

2. Backfill loop used tab-separated aws-cli output directly, which
   can concatenate all SG ids into one word and run the loop body
   once with no iteration. Inserted `| tr '\t' '\n'` — no-op on
   well-behaved output, fix on the concatenated case.

Renumbered subsequent sections.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ContextMenu's children selector ran .filter() inside the Zustand
hook, returning a brand-new array reference on every render.
useSyncExternalStore under the hood compares snapshots with
Object.is — a new array always differs, so React kept scheduling
re-renders, hit the 50-update depth cap, and crashed with minified
error #185.

Observed as "Application error: a client-side exception" on every
SaaS tenant once a session cookie resolved. Caught in dev mode
where the build emits the clear warning:

  The result of getSnapshot should be cached to avoid an infinite loop
      at ContextMenu (src/components/ContextMenu.tsx:26:34)

Fix: select the stable nodes array once, derive children via
useMemo outside the store subscription. Same output, no new
reference per render.

Manually verified: dev bundle served through a cloudflared tunnel
to a live tenant, ContextMenu component mounts cleanly, remaining
console errors are all unrelated (localhost API 401s from the dev
server pointing at its own origin).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@HongmingWang-Rabbit
HongmingWang-Rabbit merged commit fc27477 into main Apr 22, 2026
9 checks passed
@HongmingWang-Rabbit
HongmingWang-Rabbit deleted the fix/canvas-contextmenu-infinite-loop branch April 24, 2026 00:10
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