Skip to content

Commit 0f591a2

Browse files
committed
Fix race condition with shouldRecycle=false during fast scroll
Consolidates component removal, prepend, and append operations into a single layout callback to prevent concurrent DOM manipulation that caused "removeChild" and "Cannot read properties of null" errors. Fixes #296
1 parent 6cbdf36 commit 0f591a2

File tree

1 file changed

+54
-42
lines changed
  • vertical-collection/addon/-private/data-view/radar

1 file changed

+54
-42
lines changed

vertical-collection/addon/-private/data-view/radar/radar.js

Lines changed: 54 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -104,6 +104,7 @@ export default class Radar {
104104
this._componentPool = [];
105105
this._prependComponentPool = [];
106106
this._appendComponentPool = []; // https://github.com/html-next/vertical-collection/issues/296
107+
this._removeComponentPool = []; // Defer removal when shouldRecycle=false to avoid race condition
107108

108109
// Boundaries
109110
this._occludedContentBefore = new OccludedContent(occlusionTagName);
@@ -615,8 +616,11 @@ export default class Radar {
615616
}
616617
}
617618
} else {
618-
virtualComponents.removeObjects(_componentPool);
619+
// When shouldRecycle=false, leave DOM untouched and defer virtualComponents removal
620+
// to layout phase to avoid race condition with Glimmer VM.
621+
this._removeComponentPool.push(..._componentPool);
619622
_componentPool.length = 0;
623+
this._scheduleLayout();
620624
}
621625
}
622626

@@ -640,6 +644,53 @@ export default class Radar {
640644
: '';
641645
}
642646

647+
_scheduleLayout() {
648+
if (this._nextLayout === null) {
649+
this._nextLayout = this.schedule('layout', () => {
650+
this._nextLayout = null;
651+
652+
const {
653+
virtualComponents,
654+
_removeComponentPool,
655+
_appendComponentPool,
656+
_prependComponentPool,
657+
_occludedContentBefore,
658+
_occludedContentAfter,
659+
_itemContainer,
660+
} = this;
661+
662+
// Remove components from virtualComponents in layout phase (safe from race conditions)
663+
if (_removeComponentPool.length > 0) {
664+
virtualComponents.removeObjects(_removeComponentPool);
665+
_removeComponentPool.length = 0;
666+
}
667+
668+
while (_prependComponentPool.length > 0) {
669+
const component = _prependComponentPool.pop();
670+
const relativeNode =
671+
_occludedContentBefore.realLowerBound.nextSibling;
672+
insertRangeBefore(
673+
_itemContainer,
674+
relativeNode,
675+
component.realUpperBound,
676+
component.realLowerBound,
677+
);
678+
}
679+
680+
while (_appendComponentPool.length > 0) {
681+
const component = _appendComponentPool.pop();
682+
const relativeNode = _occludedContentAfter.realUpperBound;
683+
insertRangeBefore(
684+
_itemContainer,
685+
relativeNode,
686+
component.realUpperBound,
687+
component.realLowerBound,
688+
);
689+
}
690+
});
691+
}
692+
}
693+
643694
_appendComponent(component) {
644695
const {
645696
virtualComponents,
@@ -668,26 +719,7 @@ export default class Radar {
668719
// We have to move them _after_ they render, so we schedule that if they exist
669720
if (!shouldRecycle) {
670721
_appendComponentPool.unshift(component);
671-
672-
if (this._nextLayout === null) {
673-
this._nextLayout = this.schedule('layout', () => {
674-
this._nextLayout = null;
675-
676-
while (_appendComponentPool.length > 0) {
677-
const component = _appendComponentPool.pop();
678-
679-
// Changes with each inserted component
680-
const relativeNode = _occludedContentAfter.realUpperBound;
681-
682-
insertRangeBefore(
683-
this._itemContainer,
684-
relativeNode,
685-
component.realUpperBound,
686-
component.realLowerBound,
687-
);
688-
}
689-
});
690-
}
722+
this._scheduleLayout();
691723
}
692724
}
693725
}
@@ -716,27 +748,7 @@ export default class Radar {
716748
// Components that are both new and prepended still need to be rendered at the end because Glimmer.
717749
// We have to move them _after_ they render, so we schedule that if they exist
718750
_prependComponentPool.unshift(component);
719-
720-
if (this._nextLayout === null) {
721-
this._nextLayout = this.schedule('layout', () => {
722-
this._nextLayout = null;
723-
724-
while (_prependComponentPool.length > 0) {
725-
const component = _prependComponentPool.pop();
726-
727-
// Changes with each inserted component
728-
const relativeNode =
729-
_occludedContentBefore.realLowerBound.nextSibling;
730-
731-
insertRangeBefore(
732-
_itemContainer,
733-
relativeNode,
734-
component.realUpperBound,
735-
component.realLowerBound,
736-
);
737-
}
738-
});
739-
}
751+
this._scheduleLayout();
740752
}
741753
}
742754

0 commit comments

Comments
 (0)