Skip to content

fix: detach reused components from their old collection in resetFromString#6775

Open
DavidHarvey wants to merge 3 commits into
GrapesJS:devfrom
DavidHarvey:fix/detach-reused-comps
Open

fix: detach reused components from their old collection in resetFromString#6775
DavidHarvey wants to merge 3 commits into
GrapesJS:devfrom
DavidHarvey:fix/detach-reused-comps

Conversation

@DavidHarvey
Copy link
Copy Markdown
Contributor

Problem

resetFromString reuses existing component models by id (via getComponentsFromDefs) rather than rebuilding them. When a reused model lands under a new parent, it gets added to the new collection but never removed from the old one. Backbone only lets a model belong to one collection, so the move half-happens:

  • model.collection is never reassigned, so parent() still points at the old parent.
  • the old parent's collection still holds the model, so removing that old parent recurses into and tears down a node that's actually still in the tree.

Repro

Wrapper contains:

<div id="i5y6"><p id="i8sd">Insert your text here</p></div>

Delete the <div> so it resets to just <p id="i8sd">…</p>. The <p> is correctly reused and moved up to the body, but p.parent() still returns the deleted <div>. Anything that walks up the tree from there (RTE re-parenting, layers, sorter) ends up on a dead node.

Fix

Detach the reused model from its current container before re-inserting it, the same way Component.append() already does when moving a model:

component.collection?.remove(component, { silent: true } as any);

It has to be silent - a normal remove fires remove on the old collection, which gets forwarded to symbol instances and deletes the wrong thing. The silent remove just clears the old reference; the following reset handles rendering and symbol sync as usual.

Tests

Added a regression test in Component.ts for the case above (reused <p> is the same instance, stays in componentsById, old <div> is gone, and parent()/collection point at the wrapper). Fails on main, passes with the fix.

@artf
Copy link
Copy Markdown
Member

artf commented Jun 2, 2026

Thanks for the PR @DavidHarvey I agree the stale-parent issue seems real, but I’m still a bit uneasy about the fix as written.

From what I understand, the broken case is specifically when resetFromString reuses an existing component by id and that component is supposed to move under a different parent. In that scenario, detaching it from the old collection makes sense.

What gives me pause is that this now detaches every reused component unconditionally, even when it isn’t actually moving. I suspect that’s probably fine in practice, but it feels broader than the bug being fixed.

Would it make sense to only detach when the reused component is being inserted into a different collection/parent?
I’d also feel a bit better with a regression test around symbols for this path, since the reason for silent: true seems to be avoiding incorrect remove-side effects there.

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.

2 participants