Reliably wait for cart requests to finish before adding new items to the cart #59373
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
Testing GuidelinesHi @ralucaStan @woocommerce/rubik, Apart from reviewing the code changes, please make sure to review the testing instructions (Guide) and verify that relevant tests (E2E, Unit, Integration, etc.) have been added or updated as needed. Reminder: PR reviewers are required to document testing performed. This includes:
|
📝 WalkthroughWalkthroughThe code refactors the mechanism for waiting on cart-related network requests in Playwright-based end-to-end tests. It replaces a method that awaited a single cart response with a new approach that tracks multiple concurrent cart requests using event listeners and a custom polling utility, updating related methods for improved handling of asynchronous cart actions. Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test Code
participant Utils as FrontendUtils
participant Page as Playwright Page
Test->>Utils: addToCart()
Utils->>Page: Start tracking cart-related requests (event listeners)
Utils->>Page: Trigger add-to-cart action
Page-->>Utils: Emit request/response events for /cart, /add_to_cart, /batch
Utils->>Utils: Track pending cart requests
Utils->>Utils: waitForCartRequests() (polls until all pending requests complete)
Utils->>Page: Remove event listeners
Utils-->>Test: addToCart() completes after all cart requests finish
Possibly related PRs
📜 Recent review detailsConfiguration used: .coderabbit.yml 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (2)📓 Common learningsplugins/woocommerce/client/blocks/tests/e2e/utils/frontend/frontend-utils.page.ts (7)⏰ Context from checks skipped due to timeout of 90000ms (18)
🔇 Additional comments (5)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
ralucaStan
requested changes
Jul 7, 2025
plugins/woocommerce/client/blocks/tests/e2e/utils/frontend/frontend-utils.page.ts
Outdated
Show resolved
Hide resolved
| }; | ||
|
|
||
| const responseHandler = ( response: Response ) => { | ||
| pendingRequests.delete( response.url() ); |
There was a problem hiding this comment.
This calls delete for any response that comes in; it should only call it for cart related URLs:
if (
url.includes( '/cart' ) ||
url.includes( '/add_to_cart' ) ||
url.includes( '/batch' )
) {
pendingRequests.delete( response.url() );
}
we could store the URLs for cart calls in a constant for reusability.
There was a problem hiding this comment.
I think it's fine since it won't remove things that are not in the set?
There was a problem hiding this comment.
it won't, it was a small improvement to only call pendingRequests.delete for the requests we added
plugins/woocommerce/client/blocks/tests/e2e/utils/frontend/frontend-utils.page.ts
Outdated
Show resolved
Hide resolved
| if ( error instanceof Error && error.name !== 'TimeoutError' ) { | ||
| throw error; | ||
| private trackCartRequests( timeout = 5000 ) { | ||
| const pendingRequests = new Set< string >(); |
There was a problem hiding this comment.
There is an issue here with using Set, which only holds unique values, because we are only adding http://localhost:8889/wp-json/wc/store/v1/cart/add-item to the Set when a product is added to the cart.
If we add 2 different products, one after the other, the Set will only have 1 value.
We could switch to a counter to keep track of the calls being completed. WDYT?
There was a problem hiding this comment.
Good call, added count tracking
Co-authored-by: Raluca Stan <ralucastn@gmail.com>
d63f6af to
e3c24f2
Compare
ralucaStan
approved these changes
Jul 10, 2025
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.
Submission Review Guidelines:
Changes proposed in this Pull Request:
Follow up to #58947.
I noticed that the above PR will wait for 5s before and after adding to a cart if there are no pending cart requests. I've changed the util to be more explicit in tracking pending requests. We start tracking them before adding items to cart, and then wait for all requests to finish before moving outside the
addToCartutil.Total time before (locally): 19.1s
Total time after (locally): 9.9s
How to test the changes in this Pull Request:
Code review and green CI 🙏
Testing that has already taken place:
Changelog entry
Changelog Entry Details
Significance
Type
Message
Changelog Entry Comment
Comment
E2E test tooling change