SlotFill: fix a bug with storing stale fillProps #67000

jsnajdr · 2024-11-14T12:38:25Z

Fixes a SlotFill bug where a slot is sometimes rendered with old stale fillProps. It's described and discussed here: #65907 (comment). In short, the updateSlot method needs to check a ref value to determine if it's still the current registered slot. When an old slot is unregistered and new slot is registered, there is a short period of time where both are semi-registered and special care is needed.

~~That's the first commit in this PR. In addition, I'm doing a series of refactorings of SlotFill in order to put it in a better shape:~~

github-actions · 2024-11-14T12:38:49Z

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

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

github-actions · 2024-11-14T13:00:33Z

Flaky tests detected in 4c78204.
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/11854163018
📝 Reported issues:

[Flaky Test] submenu click on the arrow interactions #61884 in /test/e2e/specs/editor/blocks/navigation-frontend-interactivity.spec.js

youknowriad · 2024-11-14T17:39:26Z

packages/components/src/slot-fill/bubbles-virtually/use-slot.ts

@@ -21,8 +21,10 @@ export default function useSlot( name: SlotKey ) {

 	const api = useMemo(
 		() => ( {
-			updateSlot: ( fillProps: FillProps ) =>
-				registry.updateSlot( name, fillProps ),
+			updateSlot: (


Where is this one used. or is it not used at all? I see the function arguments modified but I don't see where the calling of the function is updated.

The updateSlot method is used in the Slot component. Called on every render, in a effect without dependencies, to make sure that the latest fillProps is always synced promptly to the registry.

The bug being fixed is triggered when, for any reason, the Slot is rerendered one more time before unmounting. Then updateSlot would store a stale fillProps value at a time when a new Slot is already being mounted. The unregisterSlot method already has exactly the same ref check.

But this one updateSlot that is returned from useSlot, is not used right?

Oh yes, this is a good observation. The updateSlot and unregisterSlot functions returned from useSlot are not really used. And I think I could get rid of the entire api object and return only the slot. I'll do that in a followup PR.

youknowriad · 2024-11-14T17:40:16Z

packages/components/src/slot-fill/bubbles-virtually/slot.tsx

+	const fillPropsRef = useRef( fillProps );
+	useLayoutEffect( () => {
+		fillPropsRef.current = fillProps;
+	}, [ fillProps ] );


In general this is an indication that something should have been defined using the new useEvent maybe.

Unfortunately useEvent is specialized to support only callbacks. Here we have a value that is a regular object.

What I was thinking is that we could do something like

const registerSlotEffect = useEvent(() => { registerSlot( name, ref, fillProps ); } ); useEffect(() => { registerSlotEffect(); }, [ name ] );

Yes, if we really wanted to use useEvent, it's possible, but it doesn't really improve the code IMO. Also the fillPropsRef.current value is used in two effect hooks, which makes useEvent even less attractive.

youknowriad

Left some random comments but to be honest, this one is hard to review. I think I'd just to trust the tests and your confidence in this PR :)

jsnajdr · 2024-11-15T10:03:53Z

this one is hard to review.

OK, I updated the PR to be smaller and more focused. I'll do the bigger refactors separately.

jsnajdr · 2024-11-15T10:08:26Z

packages/components/src/slot-fill/bubbles-virtually/slot-fill-provider.tsx

 			}
+
+			slots.delete( name );


Here I'm just tidying up the code, no behavior change. unregisterSlot and updateSlot now do the same ref code and the code looks also the same.

jsnajdr · 2024-11-15T10:09:37Z

packages/components/src/slot-fill/bubbles-virtually/slot-fill-provider.tsx

-		if ( ! fillsForName ) {
-			return;
-		}
+	const unregisterFill: SlotFillBubblesVirtuallyContext[ 'unregisterFill' ] =


Here I'm fixing a typo in the type: changing registerFill to unregisterFill. Both functions have the same signature, so the bug didn't have any observable consequences, but worth fixing anyway.

jsnajdr · 2024-11-15T10:10:31Z

packages/components/src/slot-fill/bubbles-virtually/slot.tsx

 	useLayoutEffect( () => {
-		registry.updateSlot( name, fillProps );
+		registry.updateSlot( name, ref, fillPropsRef.current );


This is a critical line in the patch, adding a ref param to the updateSlot call.

jsnajdr · 2024-11-15T10:10:56Z

packages/components/src/slot-fill/bubbles-virtually/slot-fill-provider.tsx

 		fillProps
 	) => {
 		const slot = slots.get( name );
 		if ( ! slot ) {
 			return;
 		}

+		if ( slot.ref !== ref ) {
+			return;
+		}


This is second critical line of the patch, adding a check if ref matches.

Mamaduka

Thank you, @jsnajdr!

P.S. Inline comments were very helpful!

jsnajdr added the [Type] Bug An existing feature does not function as intended label Nov 14, 2024

jsnajdr requested review from Mamaduka, youknowriad, mirka, ciampo and ellatrix November 14, 2024 12:38

jsnajdr self-assigned this Nov 14, 2024

jsnajdr requested a review from ajitbohra as a code owner November 14, 2024 12:38

Mamaduka added the [Package] Components /packages/components label Nov 14, 2024

youknowriad reviewed Nov 14, 2024

View reviewed changes

jsnajdr added 3 commits November 15, 2024 11:00

SlotFill: updateSlot should check the ref

23d09b2

Slot: store fillProps to a ref to fix effect deps

7bd8715

SlotFillProvider: tidy up API code

4c78204

jsnajdr force-pushed the fix/slot-fill-update-slot branch from 01c1142 to 4c78204 Compare November 15, 2024 10:00

jsnajdr changed the title ~~SlotFill: fix a bug with storing stale fillProps + several refactors~~ SlotFill: fix a bug with storing stale fillProps Nov 15, 2024

jsnajdr commented Nov 15, 2024

View reviewed changes

youknowriad approved these changes Nov 15, 2024

View reviewed changes

Add changelog entry

7fa987d

Mamaduka approved these changes Nov 15, 2024

View reviewed changes

jsnajdr enabled auto-merge (squash) November 15, 2024 11:11

jsnajdr merged commit c3a9c1a into trunk Nov 15, 2024
64 checks passed

jsnajdr deleted the fix/slot-fill-update-slot branch November 15, 2024 11:34

github-actions bot added this to the Gutenberg 19.8 milestone Nov 15, 2024

This was referenced Nov 18, 2024

SlotFill: remove registration API from useSlot result #67070

Merged

SlotFill: rewrite base Slot to functional, unify rerenderable refs #67153

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SlotFill: fix a bug with storing stale fillProps #67000

SlotFill: fix a bug with storing stale fillProps #67000

jsnajdr commented Nov 14, 2024 •

edited

Loading

github-actions bot commented Nov 14, 2024 •

edited

Loading

github-actions bot commented Nov 14, 2024 •

edited

Loading

youknowriad Nov 14, 2024

jsnajdr Nov 15, 2024

youknowriad Nov 15, 2024

jsnajdr Nov 15, 2024

youknowriad Nov 14, 2024

jsnajdr Nov 15, 2024

youknowriad Nov 15, 2024

jsnajdr Nov 15, 2024

youknowriad left a comment

jsnajdr commented Nov 15, 2024

jsnajdr Nov 15, 2024

jsnajdr Nov 15, 2024

jsnajdr Nov 15, 2024

jsnajdr Nov 15, 2024

Mamaduka left a comment •

edited

Loading

SlotFill: fix a bug with storing stale fillProps #67000

SlotFill: fix a bug with storing stale fillProps #67000

Conversation

jsnajdr commented Nov 14, 2024 • edited Loading

github-actions bot commented Nov 14, 2024 • edited Loading

github-actions bot commented Nov 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youknowriad left a comment

Choose a reason for hiding this comment

jsnajdr commented Nov 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mamaduka left a comment • edited Loading

Choose a reason for hiding this comment

jsnajdr commented Nov 14, 2024 •

edited

Loading

github-actions bot commented Nov 14, 2024 •

edited

Loading

github-actions bot commented Nov 14, 2024 •

edited

Loading

Mamaduka left a comment •

edited

Loading