Inserter: patterns refresh when selecting different blocks

[annezazu]

Using 6.7 beta 1, I noticed that if I have the patterns tab open for the Inserter, it seems to be regularly re-rendering the list of patterns whenever I am clicking around the page. This sometimes leads to a "no results" similar to what's described here https://github.com/WordPress/gutenberg/issues/65833

https://github.com/user-attachments/assets/6ebb5004-4098-4808-afe6-4ff769567180

I am noting this as an enhancement as something to smooth out but it feels like a bug in many ways :)

[andrewserong]

but it feels like a bug in many ways

Same here. Just in case, I've added it to the 6.7 board, but feel free to remove if anyone thinks it's more of an enhancement idea.

[ndiego]

Let's reclassify it as a bug. All enhancements should be removed from the 6.7 board at this point, and I agree that it feels very "buggy".

[colorful-tones]

I feel like this may be a duplicate of #65833? 🤔

[ndiego]

I feel like this may be a duplicate of https://github.com/WordPress/gutenberg/issues/65833? 🤔

Definitely related, but I do not feel it's a duplicate. I added more testing feedback in that ticket.

[kevin940726]

I might be missing something obvious, but this might be the expected behavior. When clicking around the page, we also move the focus to different blocks. Some blocks don't allow inserting patterns, thus it shows "no results." I think this becomes more visible because we no longer close the inserter when moving focus. I agree that the UI and the wording are confusing and feel "buggy," but I'm not sure what the expected behavior should be 🤔. Someone more familiar with the recent changes might be able to verify whether this is indeed a bug. 🙏

[ndiego]

I am not sure how to achieve this from a technical standpoint, but the pattern panel should only visibly update if the available selection of patterns needs to change. Right now, it visually refreshes every time you click on something, even if the patterns that are available to the user do not actually change.

[ndiego]

For example, in the video below, IMO the pattern panel should not visually refresh when clicking on the two blocks since the patterns that are available to the user do not change.

https://github.com/user-attachments/assets/f89c8929-e6d4-4414-8d14-1146ec966d51

[ramonjd]

I was just looking at https://github.com/WordPress/gutenberg/pull/66053 and I'm also not sure how to do this. Memoizing the incoming block and shown patterns, or abstracting the list into a separate component doesn't seem to mitigate the refresh.

Somehow I think it's related to the AsyncList, passed to the component as shownPatterns.

Notice how its value changes between identical blocks. The patterns end up being the same, but not before they're not 😄

https://github.com/user-attachments/assets/518da03a-b67a-428b-8934-366137c099a8

For example, the flash completely goes away without it. E.g.,

                    <BlockPatternsList
                        ref={ scrollContainerRef }
/*
                        shownPatterns={ pagingProps.categoryPatternsAsyncList }
*/
                        shownPatterns={ pagingProps.categoryPatterns }
                        blockPatterns={ pagingProps.categoryPatterns }

I'm not saying we remove it, only trying to identify the cause of the rerender trigger. 🤔

[kevin940726]

I think a part of the reason why we introduced AsyncList here is because rendering patterns is too slow. I hope that someday we can revive https://github.com/WordPress/gutenberg/issues/54999 and make them fast enough that we don't need this kind of hack. 🤔

[madhusudhand]

The PR #66053 addresses the flickering problem.

I think a part of the reason why we introduced AsyncList here is because rendering patterns is too slow.

Part of the fix is to render only the async list instead of entire pattern list, which is fixing the problem. I agree, removing the async list and fixing the block preview rendering gives better user experience.

[kevin940726]

I also believe AsyncList was first introduced when we were not using any pagination, so that might not be needed now.

[madhusudhand]

I also believe AsyncList was first introduced when we were not using any pagination, so that might not be needed now.

Created #66114 which removes async list and render performance is much better without it.

[getdave]

@WordPress/gutenberg-core Anyone with good experience of asyncList who could help here as this bug if going to be very noticeable for Zoom Out in WordPress 6.7.

Specifically

• can we safely drop asyncList usage now we have pagination? Rendering 20 patterns upfront still seems high to me.
• why does async list seem to return inconsistent results with the initial result sometimes higher than subsequent results?
• other suggestions for avoiding the Patterns re-rendering as we move between blocks.

[youknowriad]

can we safely drop asyncList usage now we have pagination? Rendering 20 patterns upfront still seems high to me.

Probably if we limit to 20 patterns. We should consider the Async component that is used in the site editor as a replacement.

[jsnajdr]

The useAsyncList component ensures that we render only 5 patterns at a time. Then it schedules another 5 to be rendered a bit later and so on. This was useful when the component was limiting "unlimited" renders to 5, but maybe it's not so useful for limiting only 20 to 5.

When the component gets an identical list (but a different array) on a render update, it explicitly tries to find out if some part of the list is already rendered, see the getFirstItemsPresentInState function. So, if we click between two blocks that have the same list of patterns to select from, then it's a different array (filtered based on different criteria), but the result is the same. The hook should be able to detect that, it should not rerender the whole list with a "flash of empty content". If this doesn't happen, it's worth debugging why.

[jsnajdr]

If the useAsyncList hook is presented with a new list that contains the same patterns, it will rerender them all from scratch anyway because the pattern objects are not really identical. The __experimentalGetAllowedPatterns selector does a mapping like this:

patterns.map( ( pattern ) => ( {
  ...pattern,
  get blocks() {
    return getParsedPattern( pattern ).blocks;
  },
} ) );

where even identical patterns get transformed to non-identical objects.

It could help to memoize these mapped patterns with a weakmap: WeakMap<RawPattern, EnhancedPattern>.

Then the flashes should go away. I can try it out in the next 24 hours.

[jsnajdr]

66159 is fixing this for me. The patterns list is stable and doesn't rerender when switching between selected blocks.

But locally I see another strange bug, I wonder if anyone else sees it, too:

There is a really big vertical space between the patterns in the pattern list, each item occupies the entire height of the viewport. It's caused by the .block-editor-block-patterns-list__item element having a height: 100% style. The styles are old, there's no recent change. It seems like some change at another place changed the element against which the relative 100% value is calculated.