Forms: Optimistically move items from/to inbox/spam/trash #45912
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 2 files.
Full summary · PHP report · JS report If appropriate, add one of these labels to override the failing coverage check:
Covered by non-unit tests
|
There was a problem hiding this comment.
Pull Request Overview
This PR implements optimistic UI updates for form response management in the Jetpack Forms dashboard. When users move items between inbox, spam, and trash, the UI updates immediately without waiting for the server response, providing a more responsive user experience.
Key Changes
- Introduces optimistic status updates using
editEntityRecordbefore API calls complete - Implements automatic rollback of optimistic updates when API calls fail
- Adds filtering logic to show/hide items based on their effective (optimistically updated) status
- Fixes the undo behavior to correctly restore items to their original status
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
projects/packages/forms/src/dashboard/inbox/dataviews/types.tsx |
Refactored type definitions to extract NoticeOptions, DispatchActions, and SelectActions types; added isUndo and targetStatus options to action callbacks |
projects/packages/forms/src/dashboard/inbox/dataviews/actions.tsx |
Added withTimeout and processStatusChange helper functions; updated all action callbacks to support optimistic updates with rollback on failure and proper undo handling |
projects/packages/forms/src/dashboard/hooks/use-inbox-data.ts |
Added filtering logic to exclude items that have been optimistically moved to different status tabs |
projects/packages/forms/changelog/update-forms-optimistic-move-entities |
Added changelog entry documenting the feature |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| itemsUpdated | ||
| .filter( | ||
| ( promise ): promise is PromiseFulfilledResult< { id: number } > => | ||
| promise.status === 'fulfilled' && !! promise.value | ||
| ) | ||
| .map( ( { value } ) => value.id.toString() ), |
There was a problem hiding this comment.
The itemsUpdated array is being filtered and mapped again here, but it's already been filtered in the processStatusChange function (lines 187-189). This creates inconsistency because processStatusChange returns items where promise.value is truthy, but here you filter again with different criteria.
More importantly, itemsUpdated returned from processStatusChange is typed as PromiseSettledResult<unknown>[], not specifically as fulfilled results with { id: number }. The type assertion at line 338 assumes the value has an id property, but the actual API response structure should be validated. Consider either:
- Making
processStatusChangereturn properly typed results, or - Adding runtime validation that the value contains the expected
idproperty
| itemsUpdated | ||
| .filter( | ||
| ( promise ): promise is PromiseFulfilledResult< { id: number } > => | ||
| promise.status === 'fulfilled' && !! promise.value | ||
| ) | ||
| .map( ( { value } ) => value.id.toString() ), |
There was a problem hiding this comment.
Same issue as with markAsSpamAction: itemsUpdated returned from processStatusChange is typed as PromiseSettledResult<unknown>[], but here it's being treated as if it contains { id: number } values. The type assertion doesn't guarantee the runtime value structure matches. Consider adding runtime validation or properly typing the return value of processStatusChange.
| items, | ||
| { registry }, | ||
| // We can trash a spam or inbox item, so we need to restore to the original status | ||
| { isUndo: true, targetStatus: items[ 0 ]?.status } |
There was a problem hiding this comment.
Using items[0]?.status as the targetStatus will not work correctly because at this point the items have already been optimistically updated to 'trash' status. The original status information is lost.
To fix this, you should capture the original status before the optimistic update (e.g., store it in a variable before calling processStatusChange) and use that stored value here instead of items[0]?.status.
| restoreAction.callback( items, { registry } ); | ||
| if ( ! isUndo ) { | ||
| // Reload the items to the store, as they were removed from the store when moved to trash | ||
| receiveEntityRecords( 'postType', 'feedback', items, queryParams, true ); |
There was a problem hiding this comment.
The receiveEntityRecords call should be done before the success notice to ensure the items are available in the store when the undo action is triggered. However, there's a more critical issue: this reloads the items with their current status after they've been optimistically updated to 'trash', which means the undo action will try to restore items from 'trash' to 'trash' (since items[0]?.status will be 'trash' at line 579).
The correct approach would be to capture the original statuses before the optimistic update and pass that information to the undo callback.
| /** | ||
| * Helper function to determine the effective status of a record, | ||
| * considering optimistic edits (status field in edits). | ||
| * | ||
| * @param {FormResponse} record - The record to check. | ||
| * @return {string} The effective status of the record. | ||
| */ | ||
| const getEffectiveStatus = ( record: FormResponse ): string => { | ||
| // getEditedEntityRecord merges edits with the original record, | ||
| // so if status was edited, it will be in the edited record | ||
| return record.status || 'publish'; | ||
| }; |
There was a problem hiding this comment.
[nitpick] The getEffectiveStatus function appears to be redundant. According to the comment, it claims to use getEditedEntityRecord to merge edits with the original record, but the function implementation just returns record.status directly.
Since editedRecords is already the result of calling getEditedEntityRecord on each record (as seen in the earlier code), the status should already include any optimistic edits. This helper function adds no value and the filtering could directly use record.status instead of calling getEffectiveStatus(record).
| records: FormResponse[], | ||
| query?: QueryParams, | ||
| invalidateCache?: boolean | ||
| ) => Promise< void >; |
There was a problem hiding this comment.
The return type annotation for receiveEntityRecords specifies Promise<void>, but based on the WordPress core data APIs, receiveEntityRecords is typically a synchronous action creator that returns an action object, not a Promise. This type definition may cause type mismatches when the function is called.
| ) => Promise< void >; | |
| ) => void; |
| // Failed if rejected OR if fulfilled but result is invalid | ||
| if ( promise.status === 'rejected' || promise.value == null ) { |
There was a problem hiding this comment.
[nitpick] The condition promise.value == null uses loose equality which will match both null and undefined. This is intentional for checking invalid results, but the comment on line 194 says "if fulfilled but result is invalid" - it would be clearer to explicitly check promise.value === null || promise.value === undefined to make the intent more obvious, or add a comment explaining why loose equality is used here.
| // Failed if rejected OR if fulfilled but result is invalid | |
| if ( promise.status === 'rejected' || promise.value == null ) { | |
| // Failed if rejected OR if fulfilled but result is null or undefined | |
| if ( promise.status === 'rejected' || promise.value === null || promise.value === undefined ) { |
vianasw
left a comment
vianasw
left a comment
There was a problem hiding this comment.
Very neat implementation. I tried very hard to break and force race conditions, but I couldn't. I tried with network throttling, breaking the connection, and forcing edge cases. So, despite Copilot's multiple comments that status is not properly handled on undo, it was working fine for me. I left a comment about documenting the optimistic update strategy, but feel free to merge without it.
|
|
||
| /** | ||
| * Helper function to process status changes with optimistic updates and error handling. | ||
| * |
There was a problem hiding this comment.
maybe would be good to explain here the optimistic update strategy. Something like:
/**
* Optimistic Update Strategy:
* 1. Immediately update local state and counts
* 2. Make API call
* 3. On success: invalidate cache to sync with server
* 4. On failure: rollback local changes
* 5. Undo actions must preserve original status for proper restoration
*/
|
Will leave the comments for a follow-up, some things can be simplified or types can be enhanced. |
github-actions
bot
removed
the
[Status] Needs Review
Fixes FORMS-386
Proposed changes:
Other information:
Jetpack product discussion
Does this pull request change what data or activity we track or use?
Testing instructions: