Router: Fix addition and removal of empty classnames

[tyxla]

What?

Fixes an error that occurs when navigating between different blocks in global styles in the site editor.

Why?

Just fixing an error, seems like it was introduced in #67199.

Found while late testing #67199.

How?

We're making sure that before adding or removing a classname during router navigation, there's actually one to be added or removed.

Testing Instructions

• Open /wp-admin/site-editor.php?p=%2Fstyles&section=%2Fblocks
• Navigate to a few blocks.
• Verify you no longer see the error.

Testing Instructions for Keyboard

Same

Screenshots or screencast

[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: tyxla <[email protected]>
Co-authored-by: youknowriad <[email protected]>
Co-authored-by: jsnajdr <[email protected]>

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

[youknowriad]

There's something I'm missing here. When can this happen?

There's a ! options.transition check above that is meant to prevent using a transition entirely no?

[jsnajdr]

There's a ! options.transition check above

But that check doesn't prevent the transition from running! It's:

if ( ! options.transition ) {
  performPush();
}

await new Promise( ... );

There is missing return after performPush(). That's probably the actual bug!

[tyxla]

Agreed, nice find @jsnajdr. Updated in bb9b71f

[youknowriad]

I swear in my mind, there was an early return haha :)

[github-actions]

Flaky tests detected in bb9b71f.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/12066792436
📝 Reported issues:

• [Flaky Test] activate block theme when live previewing from sidebar save button #64737 in /test/e2e/specs/site-editor/activate-theme.spec.js

[jsnajdr]

Yes, this is a perfect fix

[youknowriad]

I think we can probably remove the ?? '' fallback here

[tyxla]

I think you added it mostly to satisfy types there. Without it, TS will not be happy because classname could be undefined and classList.add() can't work with undefined. Happy to keep it.

[youknowriad]

I thought the early return will tell typescript that the classname is not empty there anymore.

[tyxla]

It would have been the case if the calls were synchronous, but this is called in an async function.

[jsnajdr]

Currently TypeScript cannot narrow down the type from string | undefined to undefined because the options.transition usage is inside a nested function (the promise constructor). In such a case it's harder to guarantee in what order code is executed.

But you could solve this by extracting most of the code out of the new Promise callback. The only part that needs to be async is the final transition.finished.finally call. Everything before it is sync.