Query Loop: Fix 'undo trap' and improve debouncing for 'Keyword' control #69845
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
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 If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
Size Change: -13 B (0%) Total Size: 1.84 MB
ℹ️ View Unchanged
|
Mamaduka
commented
Apr 7, 2025
| @@ -51,7 +51,7 @@ function ParentControl( { parents, postType, onChange } ) { | |||
| ), | |||
| }; | |||
| }, | |||
| [ search, parents ] | |||
| [ search, postType, parents ] | |||
There was a problem hiding this comment.
Just another ESLInt warning fix.
Mamaduka
commented
Apr 7, 2025
| @@ -120,7 +120,7 @@ function TaxonomyItem( { taxonomy, termIds, onChange } ) { | |||
| ), | |||
| }; | |||
| }, | |||
| [ search, termIds ] | |||
| [ search, taxonomy.slug, termIds ] | |||
There was a problem hiding this comment.
Just another ESLInt warning fix.
stokesman
approved these changes
Apr 7, 2025
There was a problem hiding this comment.
This works as expected in my tests and the code looks good to me.
While testing I think I discovered a bug in trunk this fixes. If you have entered a value in the keyword control and then try undo things go awry.
query-loop-undo-after-keyword-specified-trunk.mp4
After these changes undo works as expected. Yet a small gripe I can make is that the “Keyword” input value doesn’t update as you undo/redo. Looks like that’d be due to the local querySearch state. I don’t think we need to mess with that in this PR though.
Nice work!
packages/block-library/src/query/edit/inspector-controls/index.js
Outdated
Show resolved
Hide resolved
|
Thanks for the testing and review, @stokesman! I definitely missed the "undo trap" bug; I'm glad this fixes something real besides my squabbles with ESLint warnings 😄
That's an interesting challenge. I would hate to introduce yet another effect just to sync this state. Taking a personal note and will experiment with the fix in a follow-up. |
Yep, it would be great if we could avoid adding any effect. I did experiment with it though. I liked the idea of getting rid of the local state and making the input uncontrolled. It still needs a sync effect and I tried a ref callback based one.diff --git a/packages/block-library/src/query/edit/inspector-controls/index.js b/packages/block-library/src/query/edit/inspector-controls/index.js
index 0fe7fadc89..eb653184d5 100644
--- a/packages/block-library/src/query/edit/inspector-controls/index.js
+++ b/packages/block-library/src/query/edit/inspector-controls/index.js
@@ -15,7 +15,7 @@ import { useSelect } from '@wordpress/data';
import { store as coreStore } from '@wordpress/core-data';
import { __ } from '@wordpress/i18n';
import { debounce } from '@wordpress/compose';
-import { useState, useMemo } from '@wordpress/element';
+import { useCallback, useMemo } from '@wordpress/element';
/**
* Internal dependencies
@@ -98,12 +98,19 @@ export default function QueryInspectorControls( props ) {
setQuery( updateQuery );
};
- const [ querySearch, setQuerySearch ] = useState( query.search );
const debouncedQuerySearch = useMemo( () => {
return debounce( ( newQuerySearch ) => {
setQuery( { search: newQuerySearch } );
}, 250 );
}, [ setQuery ] );
+ const syncKeywordInput = useCallback(
+ ( node ) => {
+ if ( node && node.value !== query.search ) {
+ node.value = query.search;
+ }
+ },
+ [ query.search ]
+ );
const orderByOptions = useOrderByOptions( postType );
const showInheritControl = isControlAllowed( allowedControls, 'inherit' );
@@ -376,7 +383,6 @@ export default function QueryInspectorControls( props ) {
taxQuery: null,
format: [],
} );
- setQuerySearch( '' );
} }
dropdownMenuProps={ dropdownMenuProps }
>
@@ -410,19 +416,16 @@ export default function QueryInspectorControls( props ) {
) }
{ showSearchControl && (
<ToolsPanelItem
- hasValue={ () => !! querySearch }
+ hasValue={ () => !! query.search }
label={ __( 'Keyword' ) }
- onDeselect={ () => setQuerySearch( '' ) }
>
<TextControl
__nextHasNoMarginBottom
__next40pxDefaultSize
label={ __( 'Keyword' ) }
- value={ querySearch }
- onChange={ ( newQuerySearch ) => {
- debouncedQuerySearch( newQuerySearch );
- setQuerySearch( newQuerySearch );
- } }
+ ref={ syncKeywordInput }
+ defaultValue={ query.search }
+ onChange={ debouncedQuerySearch }
/>
</ToolsPanelItem>
) }
That seemed to work but I didn’t test too extensively. |
|
That could also work, @stokesman! I've another idea to experiment with. I'm going to merge this and will try to fix the "out of sync state" in a follow-up. UpdateMy other idea for keeping things in sync was an uncontrolled component with the Additionally, I realized that all custom syncing solutions and debounced updates will have the same problem. Based on typing speed, if you hit that "sweet spot" when previous changes are dispatched, and you continue writing, there's a chance that the attribute state will overwrite your local input. I know this would be very rare, but it's definitely a more annoying bug. I don't have any other ideas for resolving the remaining problem. But if it becomes a big frustration for users, we can always remove debouncing and use Uncontrolled with key
diff --git packages/block-library/src/query/edit/inspector-controls/index.js packages/block-library/src/query/edit/inspector-controls/index.js
index 3c82827d84a..6cb53fa4b6e 100644
--- packages/block-library/src/query/edit/inspector-controls/index.js
+++ packages/block-library/src/query/edit/inspector-controls/index.js
@@ -421,11 +421,9 @@ export default function QueryInspectorControls( props ) {
__nextHasNoMarginBottom
__next40pxDefaultSize
label={ __( 'Keyword' ) }
- value={ querySearch }
- onChange={ ( newQuerySearch ) => {
- debouncedQuerySearch( newQuerySearch );
- setQuerySearch( newQuerySearch );
- } }
+ key={ query.search }
+ defaultValue={ query.search }
+ onChange={ debouncedQuerySearch }
/>
</ToolsPanelItem>
) } |
chriszarate
pushed a commit
to chriszarate/gutenberg
that referenced
this pull request
Jul 1, 2025
…rol (WordPress#69845) Co-authored-by: Mamaduka <mamaduka@git.wordpress.org> Co-authored-by: stokesman <presstoke@git.wordpress.org>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
I started this branch to fix some annoying ESLint warnings for the Query Loop block's controls, then realized that I could get rid of
useEffect, which syncs local state and thequery.searchattribute.Side note: I wonder if
updateQueryoptimization from 693f886 could be handy in the future. This is the downside of attribute objects; they will be marked as changed even if all values are the same. Related: #27323.How
Directly call
debouncedQuerySearchwhen changing the Keyword control value.Testing Instructions
Testing Instructions for Keyboard
Same.
Screenshots or screencast