Fix Link UI displaying out of sync results #57522
Merged
Conversation
This file contains 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
|
Size Change: +102 B (0%) Total Size: 1.69 MB
ℹ️ View Unchanged
|
getdave
added
[Type] Bug
jsnajdr
reviewed
Jan 4, 2024
There was a problem hiding this comment.
Looks good, the component should indeed call updateSuggestions on value change, even when a request for old value is already running.
I think that #45806 merely refactored the already existing isUpdatingSuggestions value, the bug was probably already there. But it's hard to tell after such a long time.
One thing that should be fixed is the onFocus handler.
| @@ -258,7 +250,6 @@ class URLInput extends Component { | |||
| if ( | |||
| value && | |||
| ! disableSuggestions && | |||
| ! this.state.isUpdatingSuggestions && | |||
There was a problem hiding this comment.
This onFocus handler is the only place where state.isUpdatingSuggestions does something useful: it prevents the REST request from being triggered on focus when there is another one already running.
Could be replaced with checking this.suggestionsRequest === null. But first we need to start resetting this.suggestionsRequest back to null when the request is finished. That's not currently done.
There was a problem hiding this comment.
I've used Promise.finally() to reset this.suggestionsRequest to null and then conditionalised the focus logic based on that.
|
I tried everything I could think of to break this - it definitely fixes the issue for me. |
scruffian
approved these changes
Jan 4, 2024
jsnajdr
approved these changes
Jan 8, 2024
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?
Ensures requests to fetch link suggestions are kept in sync with the current value in the
<input>used in the<URLInput>component. Avoids value becoming out of sync with displayed results.This fixes a problem evidenced in the Link UI
<LinkControl>component which may have been introduced in #45806.Fixes #56695
Why?
Currently it is possible to have a value in the Link Control which is out of sync with the results being displayed. This is most obvious when the "Create page" option is displayed as it's possible to have the text of the Page being offered for creation to be out of sync with the value shown in the
<input>.We always want the search results to be reflective of whatever is typed into the input by the user, even if that means more fetch requests are dispatched overall.
How?
Removes unwanted state which has historically been used to "block" additional fetch requests being dispatched when the input value changes if a current request is in progress.
After some investigation I realised that this state is not serving the component well. Currently if you type in the input (the initial search) and a fetch request gets dispatched (note: you have to wait for the
200msdebounce) and then you immediately type something else there is only a request for the initial search query. The updated link value never triggers a fetch for search results because it is blocked by the state conditional.By removing this state, we allow the most recent value of the input to trigger a fetch request for search results. It is then this request which fulfills the search suggestons in the UI.
Race conditions are already handled as the code of
updateSuggestionswill abort in the.then/catchif the current request is not the same as the request stored in the instance property.Testing Instructions
First on
trunkensure you can replicate the Issue.Then verify the fix in this PR:
wordpress isthen as soon as you see the loading spinner display, typeso cool(note the specific text doesn't matter - you just need to ensure you type more text when the loading spinner first shows).Create: wordpress is so cool.trunk.Testing Instructions for Keyboard
Screenshots or screencast
Screen.Capture.on.2024-01-04.at.09-29-33.mp4