test(joint-react): add repro for useCell throw on cell removal - #3442
Open
kumilingus wants to merge 2 commits into
Open
test(joint-react): add repro for useCell throw on cell removal#3442kumilingus wants to merge 2 commits into
kumilingus wants to merge 2 commits into
Conversation
Removing a cell from a controlled `cells` array throws
`useCell(): no cell with id "…"` and unmounts the paper.
The trigger is a `renderElement` subtree that subscribes to its own
cell. When the cell leaves the store React re-runs that subtree's
selector before the parent reconciles the removal, so the selector
looks up a cell that is already gone:
Error: useCell(): no cell with id "b"
at use-cell.ts:31
at computeNext (use-cells.ts:51)
Renaming a node is the everyday way in, since an id is often authored
data and editing it is a remove plus an add. `useMeasureElement` reaches
the same path via `useCell(selectElementSize)`, so a content-sized node
hits this without naming `useCell`.
Three stories: the failing case, a narrowing case whose `renderElement`
reads only its `data` argument and survives, and a workaround that keys
the provider on the set of ids.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
kumilingus
added a commit
to clientIO/joint-demos
that referenced
this pull request
Aug 3, 2026
`isMeasured` was documented as a guard against laying out unmeasured boxes. It is really a decision about which of the two triggers owns a given run, and the three cases were measured rather than assumed: direction alone leaves every size untouched, so no measured callback ever arrives and the effect is the only trigger; a changed id remounts and resets sizes to 0x0, so the effect stands down; a label alone leaves sizes stale but non-zero, so both run. The old wording is what led to a reasonable-looking attempt to delete the function. Two neighbouring comments were wrong in the same direction. The effect claimed editing never re-frames, when a rename does — `pendingFit` is fresh on every mount and the remount rebuilds the scroller. And the graph key said nothing about being temporary, though it exists solely to work around clientIO/joint#3442 and takes the 0x0 pass, the camera reset and the teardown-per-keystroke with it when that lands. Comments only; no behaviour change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR documents and addresses a crash in @joint/react where useCell() can throw during controlled cell removal (notably when an element’s renderElement subtree subscribes to its own cell), and adds coverage to prevent regressions.
Changes:
- Add a Storybook reproduction under
Bugs/Controlled cells: removing a celldemonstrating the crash and two non-crashing variants. - Update
useCellto tolerate the “removed-mid-render” window by returning the last resolved value for the same id instead of throwing when the cell is temporarily missing. - Add a regression test covering controlled id change (remove+add) with a subscribing
renderElementsubtree.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/joint-react/stories/bugs/controlled-cells-id-change.stories.tsx | Adds a minimal Storybook repro and narrowing/workaround stories for the controlled removal crash. |
| packages/joint-react/src/hooks/use-cell.ts | Adjusts useCell selector behavior to avoid throwing when a subscribed cell disappears mid-render. |
| packages/joint-react/src/hooks/tests/use-cell-controlled-removal.test.tsx | Adds a regression test ensuring controlled removal (via id rename) does not crash a subscribing subtree. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+1
to
4
| import { useContext, useMemo, useRef } from 'react'; | ||
| import { CellIdContext } from '../context'; | ||
| import { useCells } from './use-cells'; | ||
| import type { AnyCellRecord, CellId, CellRecord, Computed } from '../types/cell.types'; |
Comment on lines
+127
to
+130
| // Remembers the last value this hook resolved and the id it belonged to, so the | ||
| // selector can tolerate the removed-mid-render window below. One ref mutated in | ||
| // place (never reassigned) — no per-render allocation on the hot path. Tagging | ||
| // by id means a changed id never returns a stale value from a different cell. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds a minimal Storybook reproduction under
Bugs/Controlled cells: removing a cell. No library code is touched.The bug
Removing a cell from a controlled
cellsarray throws and takes the whole paper down:The trigger is a
renderElementsubtree that subscribes to its own cell. When the cell leaves the store, React re-runs that subtree's selector before the parent reconciles the removal, so the selector looks up a cell that is already gone.Renaming a node is the everyday way in: an id is often authored data — a node name in a text source — so editing that name is a remove plus an add. The repro came out of a Mermaid-to-JointJS renderer where every keystroke in a node id crashed the canvas.
useCellis called directly in the story to keep it small, butuseMeasureElementreaches the same line viauseCell(selectElementSize), which is how a content-sized node hits this without ever naminguseCell.Stories
Press the button in each; measured with Playwright against
storybook dev:BrokenuseCell(): no cell with id "b"WithoutSubscriptionWorkaroundRemountOnIdChangeWithoutSubscriptionis the narrowing result: identical cell updates, butrenderElementreads only itsdataargument. It survives — so the subscription is the necessary ingredient, not the removal on its own.WorkaroundRemountOnIdChangekeys the provider on the set of ids, which unmounts the subscribed subtrees before the new cells are applied. It works, at the cost of a full graph rebuild on every id change.Notes
4.3.1(this tree) and the published4.3.2—use-cell.tshas the same unguarded throw in both.🤖 Generated with Claude Code