Skip to content

Fix for #296: Insert the virtual component bound back to DOM also for the case shouldRecycle=false#479

Merged
runspired merged 1 commit intohtml-next:mainfrom
johanrd:fix/296-remove-component-pool
Jan 24, 2026
Merged

Fix for #296: Insert the virtual component bound back to DOM also for the case shouldRecycle=false#479
runspired merged 1 commit intohtml-next:mainfrom
johanrd:fix/296-remove-component-pool

Conversation

@johanrd
Copy link
Contributor

@johanrd johanrd commented Oct 4, 2025

Fix for #296 race condition with shouldRecycle=false during fast scroll.

The issue was that we had 'forgotten' to apply the fix 'Insert the virtual component bound back before removal to make sure Glimmer not confused about the state of the DOM.' also to the case shouldRecycle=true.

I was able to reproduce with scrolling fast up and down with the following component:

// @ts-expect-error VerticalCollection is not typed yet
import VerticalCollection from '@html-next/vertical-collection/components/vertical-collection/component'

const projectList = [{id:'dd3gd'},{id:'yd3u5'},{id:'3d34'},{id:'hd3zx'},{id:'3d3h'},{id:'hd3ss'},{id:'3d3z'},{id:'zd319'},{id:'id371'},{id:'9d33x'},{id:'9d3b1'},{id:'3d35'},{id:'zd38d'},{id:'id3l9'},{id:'wd3g1'},{id:'hd3st'},{id:'9d3p9'},{id:'9d3i5'},{id:'id3e5'},{id:'id3sd'},{id:'id3zh'},{id:'jd3kt'},{id:'9d3wd'},{id:'hd3zw'},{id:'jd36l'},{id:'jd3dp'},{id:'zd3fh'},{id:'id370'},{id:'ad33h'},{id:'jd3rx'},{id:'wd3n5'},{id:'dd3ng'},{id:'dd3uk'},{id:'ed31o'},{id:'ed38s'},{id:'yd30x'},{id:'3d3v'},{id:'zd3tp'},{id:'zd3ml'},{id:'jd3z1'},{id:'xd38h'},{id:'id3e4'},{id:'3d3w'},{id:'kd3d9'},{id:'kd3kd'},{id:'kd365'},{id:'0d37x'},{id:'0d3f1'},{id:'ad3al'},{id:'0d3m5'},{id:'0d30t'}];

<template>
  <nav id='project-menu-nav' style='flex: 1; overflow: auto; height: 100%'>
    <VerticalCollection
      @items={{projectList}}
      @containerSelector='#project-menu-nav'
      @estimateHeight={{121}}
      @staticHeight={{true}}
      @shouldRecycle={{false}}
      @key='id'
      as |project|
    >
      <div style='width: 100%; height: 121px; border-bottom: 1px solid lightgray; box-sizing: border-box;'>
        {{project.id}}
      </div>
    </VerticalCollection>
  </nav>
</template>

Here the scroll window was 350 px height, it is more reproducible when it is small, and scrolling up and down very quickly:

Screen.Recording.2025-10-03.at.22.55.31.mov

@johanrd johanrd changed the title Proposed fix for #296: remove component pool Fix for #296: remove component pool Oct 6, 2025
@johanrd johanrd changed the title Fix for #296: remove component pool Fix for #296: Insert the virtual component bound back to DOM also for the case shouldRecycle=true Oct 6, 2025
@johanrd
Copy link
Contributor Author

johanrd commented Oct 9, 2025

I think this should be quite uncontroversial. Pinging the ones I know have touched upon this issue before: @ahamedalthaf @Atrue @runspired @mixonic, as this seems to have had some history😅 please chip in if you see something concerning, or if you think it is ready for merge, thanks!

@johanrd johanrd changed the title Fix for #296: Insert the virtual component bound back to DOM also for the case shouldRecycle=true Fix for #296: Insert the virtual component bound back to DOM also for the case shouldRecycle=false Nov 3, 2025
for (let i = _componentPool.length - 1; i >= 0; i--) {
const component = _componentPool[i];
const item = objectAt(items, component.index);
if (shouldRecycle === true && item) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the last PR that worked on fixing this I'd left this comment for future follow up:

I think we should follow up with a check for whether we are in the document-fragment or still in the DOM, if we are not in the fragment there's no reason to re-insert in this way.

I suspect that might still be true but I'd need to sit down and reason through the code. The worst that could happen merging this as is I believe is that we just move some DOM "in-place" which isn't too big a deal.

@NullVoxPopuli
Copy link
Contributor

@johanrd would you be willing to rebase this PR since the repo is now a v2 addon? <3

@johanrd johanrd force-pushed the fix/296-remove-component-pool branch from 5fa1db9 to 8cd0514 Compare January 23, 2026 23:37
@johanrd johanrd force-pushed the fix/296-remove-component-pool branch from 0c38fd4 to e73f799 Compare January 23, 2026 23:39
@johanrd
Copy link
Contributor Author

johanrd commented Jan 23, 2026

@NullVoxPopuli thanks! done👍

@runspired runspired merged commit 6716813 into html-next:main Jan 24, 2026
11 checks passed
@github-actions github-actions bot mentioned this pull request Jan 24, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants