-
Notifications
You must be signed in to change notification settings - Fork 843
Forms: Add notice after optimistic response status update #46056
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: trunk
Are you sure you want to change the base?
Conversation
|
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
|
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 Follow this PR Review Process:
If you have questions about anything, reach out in #jetpack-developers for guidance! |
Code Coverage SummaryCoverage changed in 6 files. Only the first 5 are listed here.
Full summary · PHP report · JS report Coverage check overridden by
I don't care about code coverage for this PR
|
2a3daaf to
a5dd0d3
Compare
| invalidateCacheAndNavigate( registry, getCurrentQuery(), queryParams, 'trash' ); | ||
| } | ||
|
|
||
| requestAnimationFrame( () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think requestAnimationFrame doesn't guarantee cache invalidation is complete. It just schedules for the next paint. If cache invalidation takes longer, you might remove the pending action before data refetches, causing a brief flash of empty/old data.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I had to use getEntityRecords to ensure the data is loaded before setting the pending action as resolved. Good catch. I was testing with the trash action, which reloaded faster.
771e74f to
27825be
Compare
vianasw
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cache invalidation is a really hard problem! I tested it again, and the flickering still persists when I undo an action (mark as spam, send to trash). Upon investigation, here is what I discovered:
- Mark spam: API call → response arrives → receiveEntityRecords(spam) auto-called by WordPress
- Undo clicked: optimistic update → editEntityRecord(publish)
- First API's receiveEntityRecords(spam) arrives → overwrites the undo's optimistic edit → flicker!
- Undo API completes → receiveEntityRecords(publish)
I think I have a fix that involves waiting for the original action to complete, but adds a slight delay. I'll follow up on Slack.
Fixes a race condition where clicking undo immediately after marking a response as spam/trash would cause the UI to flicker. The item would briefly disappear and reappear, and checkboxes would flash on all entries. Root cause: When undo was clicked, the original action's API response would arrive after the undo's optimistic update, overwriting it with stale data. This created a sequence where the UI would update to the wrong state (spam) before correcting itself (publish). Solution: Make undo wait for the original action's refetch to complete before starting. Implemented by: - Tracking pending refetch promises in a Map by actionId - Making undo onClick async and awaiting the original refetch - Using undoTriggered flag to skip refetch if undo clicked quickly - Removing original pending action state before starting undo This ensures only one refetch result is applied, eliminating the flicker while keeping undo responsive.
Follow-up to #45912
Proposed changes:
Other information:
Jetpack product discussion
p1763124260359199/1763124228.578279-slack-C052XEUUBL4
Does this pull request change what data or activity we track or use?
Testing instructions: