Fix: Race condition when adding multiple tags rapidly#74235
Fix: Race condition when adding multiple tags rapidly#74235UmeraGhori wants to merge 5 commits intoWordPress:trunkfrom
Conversation
|
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. |
|
👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @UmeraGhori! In case you missed it, we'd love to have you join us in our Slack community. If you want to learn more about WordPress development in general, check out the Core Handbook full of helpful information. |
9d6fb32 to
6008eea
Compare
|
It seems the E2E tests are failing specifically on the reload check ( |
|
Thanks for the review, @SirLouen! |
There was a problem hiding this comment.
@UmeraGhori here are a couple of things you need to review:
First, there is something wrong with those test conditions (10s timeout is a lot so it should not be happening)
TimeoutError: page.waitForResponse: Timeout 10000ms exceeded while waiting for event "response"
It appears that the condition is never reached on time.
My best guess was that the tags are never actually being 201 created.
So I manually tested the debounce solution, and despite the idea being good, it is not working: the tags are not saved as I thought. Try to add a bunch, save, and return to the post, and you will see they are not there.
If you are planning to add some unit testing, maybe you could add a test adding 3 tags in rapid-fire succession with a similar pattern and check if all 3 with the debounce have been queried.
Also, I don't understand why the hasAssignAction should not be returned earlier.
Finally, you don't need to overcomment the code. If the code is not self-explanatory, it is generally not good.
|
Thanks for the review, @SirLouen! You're spot on—the debounce logic was cancelling the save requests. I'll replace it with a proper fix, move hasAssignAction for early return, and add the rapid-fire test case you suggested. Updating shortly. |
|
@SirLouen Thanks for the reproduction report! You were right about the API collapsing with 400 errors. I have refactored the solution to handle this race condition:
Verified that tags now persist correctly even during rapid entry. |
|
@UmeraGhori, please check the conversation in the issue. I think the fix here is getting complex for a behavior we most likely remove in the next release. |
Thanks @Mamaduka for the heads-up! I wasn't aware that this behavior might be removed in the next release. I will check the issue discussion as you suggested. Should I close this PR for now, or wait for a final decision on the removal? |
|
A whole new approach is going to be required for this ticket. I'm closing this PR. |
What?
I have added a debounce to the
onChangehandler inFlatTermSelector.Why?
When users add multiple tags rapidly, API requests were racing against each other, causing
400 Bad Requesterrors and tags disappearing. This fixes issue #73782.How?
useDebouncehook (500ms delay).setValues) immediately so the user experience remains fast.Testing Instructions
tag1, tag2, tag3) hitting enter/comma quickly.Fixes #73782