fix(joint-react): prevent stale layout:update entries from resurrecting removed cells (dev) - #3495
Open
samuelgja wants to merge 5 commits into
Open
fix(joint-react): prevent stale layout:update entries from resurrecting removed cells (dev)#3495samuelgja wants to merge 5 commits into
samuelgja wants to merge 5 commits into
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The new remove handling can emit incremental “removed” ids for records that never existed in the container (data-less layout:update removals), which can break consumers relying on accurate deltas.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Ports the external-graph undo fix to dev’s staged snapshot container so stale layout:update entries can’t resurrect removed records, and so link records don’t remain stranded/hidden after delete+undo in externally-owned graphs.
Changes:
- Harden
graphProjectionagainst staleadd/change/removeentries by gating writes/removals on current graph membership and sweeping orphaned link records once per removal batch. - Add/adjust tests to reproduce the stale-record + missed-link-removal scenarios and assert container correctness.
- Recheck
pendingLinksafter element-portal React commits so links parked hidden byinsertViewdon’t remain hidden indefinitely.
File summaries
| File | Description |
|---|---|
| packages/joint-react/src/store/graph-projection.ts | Adds graph-membership gating for staged writes/removals and a post-batch orphan-link sweep to prevent stale records from resurfacing. |
| packages/joint-react/src/store/tests/graph-projection-edge-cases.test.ts | Updates edge-case test to simulate a missed link removal and verify the new orphan-link sweep behavior. |
| packages/joint-react/src/hooks/use-create-portal-paper.tsx | Adds a useEffect to re-run checkPendingLinks() after portal content commits. |
| packages/joint-react/src/components/graph/tests/external-graph-undo.test.tsx | New integration-style tests covering delete+undo and stale layout:update races for externally-owned graphs. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…a child-only updates
…ead of re-walking all pending links
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.
Description
Dev-branch port of the externally-owned-graph undo fix (see the companion PR against
master: #3494). The reworked projection ondev(immutable snapshotcontainer with
stageWrite/stageRemovepending buckets) carries the identical defect —verified by running the same failing test against
devbefore the fix.Same three invariants, adapted to the staged container:
add/changeentries are gated on graph membership; a stale entry repairs a lingeringrecord via
stageRemove(only whencurrentRecord(id)exists, so the incremental deltais not polluted with removals of never-present ids).
removeentries only apply when the cell has actually left the graph (paper view-unmountnotifications can name still-mounted cells, e.g. viewport culling).
buckets, keyed on the batch's removed-element set, and skips links the graph still holds.
Plus the same
use-create-portal-paper.tsxrecheck ofpendingLinksafter element-portalcommits, and the same test files.
Note for the future
master→devsync: this PR pre-resolves the merge forgraph-projection.ts— on conflict, take thedevside (it already contains the fix, andthe shared
external-graph-undo.test.tsxverifies the resolution). No changeset here onpurpose: the
masterPR carries the release entry, and a second one would duplicate thechangelog when
devmerges down.Motivation and Context
Same customer report as the
masterPR: delete an element with a link, undo, and therestored element renders empty with its link permanently hidden.
masteris the patch linefor the affected 4.3.5 release; this keeps
dev's state rework from shipping the same bug.Screenshots (if appropriate):