useToolsPanel: calculate menuItems in layout effect to avoid painting intermediate state

[jsnajdr]

Fixes a visual glitch when displaying a tools panel in style editor. Go to global styles editor for individual blocks and in the list of blocks, click on Paragraph. At first, toolbar items with no content will appear:

only after a moment the full UI is rendered:

The cause of this is how the state of the panel is updated, calculating derived state in a cascade of effects:

const [ a, setA ] = useState();
const [ b, setB ] = useState();
const [ c, setC ] = useState();

useEffect( () => {
  setB( ... );
}, [ a ] );

useEffect( () => {
  setC( ... );
}, [ b ] );

This needs several rerenders and paints until the final state is achieved.

In our case, the state involved is the menuItems object and the isShown value derived from it.

This PR fixes that by using layout effect, so that all the updates happen without an intermediate paint that reveals the non-final UI state. See also #56536 which applied exactly the same fix at another place.

Only the first commit is needed to actually fix the glitch. The rest is using useLayoutEffect also for other state-updating effects in the same file. And removing some unneeded dependencies from useCallback and useEffect calls, namely state setters.

The next step could be a bigger refactoring to make the state updates more streamlined. We don't need effects: they only trigger rerenders. We could use useMemo or, for state updates that need to see the previous state, a useReducer.

[ciampo]

The changes make sense to me, although they seem to cause errors in unit tests. I guess we'd need to understand if we can improve the performance and timing of the updates, while keeping the expected behavior.

Only the first commit is needed to actually fix the glitch. The rest is using useLayoutEffect also for other state-updating effects in the same file. And removing some unneeded dependencies from useCallback and useEffect calls, namely state setters.

Maybe removing the second commit would fix the unit tests?

[github-actions]

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: jsnajdr <[email protected]>
Co-authored-by: ciampo <[email protected]>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

[jsnajdr]

they seem to cause errors in unit tests.

I fixed that by using layout effects also in ToolsPanelItem, so that the order of execution stays the same. The tool panels are a maze of fragile mutual updates, the code deserves a thorough review and refactor.

[ciampo]

The tool panels are a maze of fragile mutual updates, the code deserves a thorough review and refactor.

Yeah, I got to the same conclusion while reviewing the PR. I think that moving the logic to a reducer could be a way to centralise state and logic a bit more.

[ciampo]

LGTM 🚀

Kapture.2024-09-20.at.11.37.20.mp4

Notes / follow-ups:

• as mentioned earlier, ToolsPanel could benefit from an internal refactor. The logic is currently fragmented in a lot of places, making it fragile and hard to parse;
• I noticed a considerable delay when navigating to the block screen. We should look into how to take away that delay, either by improving loading performance of the screen, or by making it non-blocking (maybe show a skeleton UI while the real data is loading and a suspense boundary?)

[github-actions]

I just cherry-picked this PR to the wp/6.7 branch to get it included in the next release: 2cce429

[youknowriad]

Hello folks, this is causing a big error for me, when trying to edit an empty page in the site editor (just typing within an empty post content block). I think it's causing some kind of infinite loop. Reverting this PR solves the issue.

[youknowriad]

Also worth noting this PR caused an increase in "block select" metric numbers.

[jsnajdr]

There are infinite loops when the tools panel is used in combination with slots and fills. I tested only in Site Editor, in one specific panel that is not extensible. I missed the fatal errors this causes elsewhere.

Also worth noting this PR caused an increase in "block select" metric numbers.

Interesting. That metric is measuring how long a focus or focusin event handlers take to execute. And this PR makes the tools panels render more "synchronously". Instead of rendering an empty placeholder initially and being populated with real content only after a cascade of useEffects, the render is now immediate.

When preparing this PR for a second try, we should try to render the tools panel as a "low priority" update.

[stokesman]

The next step could be a bigger refactoring to make the state updates more streamlined. We don't need effects:

Maybe we could take it one effect at a time. I happened to have #61694 which removes one and tests well for me.

[ciampo]

I personally like the approach in #65564 as it's guaranteeing that all updates happen atomically, but happy to hear everyone's thoughts.

[stokesman]

I personally like the approach in #65564

Me too. I hadn’t seen it when I made my prior comment and wish to add that the PR I mentioned is complementary and not an alternative.