Some nodes are not removed correctly after reactivity, wrong references

[AlbertSabate]

Describe the bug
This seems a regression from this bug: #361

To Reproduce
Complicated. It is a non consistent break.
It sometimes happens when navigating between pages. Very rare.

Expected behavior
Page should load all content it is supposed to load

[aralroca]

@AlbertSabate From which version of Brisa does it happen? to know where the regression has occurred

[AlbertSabate]

Happened on 0.2.1-canary.2. I just bumped to canary.3 and I didn't see it for now.
I didn't see this on previous versions, it happens very very rarely. However still with us.

[aralroca]

After this PR of diff-dom-streaming brisa-build/diff-dom-streaming#14 there have been no further changes to the algorithm. It is likely to be improved, but some cases of chunks are still escaping.

Happened on 0.2.1-canary.2. I just bumped to canary.3 and I didn't see it for now. I didn't see this on previous versions, it happens very very rarely. However still with us.

After reproducing it, it happens for a long time

[aralroca]

Nothing to do with diffing algorithm, is a render problem.

I think the problem is in this line

brisa/packages/brisa/src/utils/brisa-element/index.ts

Line 330 in 294eba7

for (const node of lastNodes) node?.remove();

that when it is a document.createTextNode it does not delete well. This leaves visible elements that should have been cleaned up.

[aralroca]

There is nothing to do with the textContent and remove, but try to delete a node that has been modified and isConnected is false while the new version has not been saved, which is the one that should be deleted at this time... I am still investigating...

[aralroca]

It seems that the state change modifies the textNode without noting the new version.

Then the change of the signal of the prop, makes it to be deleted, but it has the old reference that is already disconnected from the DOM and it is not deleted while the new one that has no reference is kept alive without being able to delete it.

[aralroca]

To understand the bug of these non-referenced nodes that cannot be deleted, it is necessary to understand that to control reactivity, each part responsible for maintaining its piece of DOM with reactivity saves the old nodes. The reason for keeping the old nodes is that after reactivity, the old ones are replaced by the new ones, and we need to know these references.

The problem comes when several responsible for controlling the DOM share a node, if one updates it, the reference that the other responsible had saved is not updated. Then, it is converted to a disconnected Node without having the new reference...

This happens when:

• A reactivity Responsible A has the containers where another Responsible B has any of its child nodes.
• The Responsible B deletes the nodes and new ones are added, the Responsible A does not update the reference of the child node, and instead of removing it, keeps this node (origin of this issue)...

[aralroca]

by the way @AlbertSabate, as a workaround meanwhile we not find a solution, you can change the fragment <> by some node like <div>, the problem happens when there is a fragment and different signals control different parts of this fragment

[aralroca]

To understand a little more about the origin of the problem: each reactivity part should be responsible for its nodes, and there SHOULD BE NO SHARED nodes between reactivity parts. For example, this failing test:

brisa/packages/brisa/src/utils/client-build-plugin/integration.test.tsx

Lines 6538 to 6554 in 5f03ee3

	{level2 ? (
	<>
	<span>Level 2 - Active</span>
	<button onClick={() => (showInner.value = !showInner.value)}>
	Toggle Inner
	</button>
	{showInner.value ? (
	<p>Inner Content Visible</p>
	) : (
	<p>Inner Content Hidden</p>
	)}
	</>
	) : (
	<span>Level 2 - Inactive</span>
	)}
	</div>
	</>

branch aralroca/issue-render.

The reactivity part of level2 prop, currently is controlling these nodes:

1)

 <span>Level 2 - Active</span> 

2)

<button onClick={() => (showInner.value = !showInner.value)}>
  Toggle Inner
</button>

3)

 // SHOULD NOT BE RESPONSIBLE OF THS, BUT IT IS
 <p>Inner Content Visible</p>

or

 // SHOULD NOT BE RESPONSIBLE OF THS, BUT IT IS
<p>Inner Content Hidden</p> 

The reactivity part of showInner state, currently is controlling these nodes:

1)

 <p>Inner Content Visible</p>

or

<p>Inner Content Hidden</p> 

If there are 2 responsible parties of a node, this bug can originate, since the reference of only 1 responsible party is updated.

The solution is not to synchronize the internal references, this is only a wrong workaround... The simplest solution would be to ensure that each responsible for reactivity has only its nodes. If there is another function with effect, it would passes to the next responsible.