-
Notifications
You must be signed in to change notification settings - Fork 851
Forms: refactor actions for wp build dashboard #46635
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
| page: targetPage, | ||
| }; | ||
|
|
||
| setCurrentQuery( updatedQuery ); |
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.
TODO: use navigate() from new router
|
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! |
| const markAsSpamAction: Action< FormResponse > = { | ||
| id: 'mark-as-spam', | ||
| isPrimary: true, | ||
| icon: <Icon icon={ spam } />, |
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.
Not sure we need icons anymore with latest DataViews? TODO: check if bulk actions need them
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.
The footer bulk actions used to need them, but not anymore.
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.
Code Coverage SummaryThis PR did not change code coverage! That could be good or bad, depending on the situation. Everything covered before, and still is? Great! Nothing was covered before? Not so great. 🤷 |
648f1ce to
9ee44c0
Compare
--------- Co-authored-by: Christian Gastrell <cgastrell@gmail.com>
9ee44c0 to
4a40787
Compare
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.
Pull request overview
This PR refactors form response actions from inline implementations in stage.tsx to a separate actions.tsx file to improve code organization for the WordPress build dashboard.
Changes:
- Extracted action handlers from
stage.tsxinto a newactions.tsxfile - Created a
getActions()function that returns actions based on the current view - Removed unused imports and type dependencies from
stage.tsx
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 11 comments.
| File | Description |
|---|---|
| projects/packages/forms/routes/responses/stage.tsx | Removed inline action implementations and replaced with call to getActions() function; cleaned up unused imports |
| projects/packages/forms/routes/responses/actions.tsx | New file containing all action implementations with optimistic updates, error handling, and undo functionality |
| projects/packages/forms/changelog/update-forms-wp-build-dashboard-actions | Added changelog entry for the refactoring |
| navigate, | ||
| searchParams, | ||
| view: params.view, | ||
| getItemId, |
Copilot
AI
Jan 20, 2026
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.
The parameter getItemId is passed to the getActions function but is not used in the implementation. The function parameter should be removed from both the call site and the function signature to avoid confusion.
| getItemId, |
| ); | ||
|
|
||
| // Check for both rejected promises and fulfilled promises with undefined/invalid results | ||
| // const itemsUpdated: PromiseFulfilledResult< { id: number } >[] = []; |
Copilot
AI
Jan 20, 2026
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.
This commented-out code should be removed. The active implementation on line 250 is the correct one.
| // const itemsUpdated: PromiseFulfilledResult< { id: number } >[] = []; |
| let status = 'inbox'; | ||
| if ( items[ 0 ]?.status === 'trash' ) { | ||
| status = 'trash'; | ||
| } |
Copilot
AI
Jan 20, 2026
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.
The status detection logic assumes all items have the same status by checking only items[0]?.status. In bulk operations, items could potentially have different statuses. Consider checking if all items have the 'trash' status, or handling mixed statuses more robustly.
| let status = 'inbox'; | |
| if ( items[ 0 ]?.status === 'trash' ) { | |
| status = 'trash'; | |
| } | |
| // Treat the view as "trash" only if all successfully updated items were in trash. | |
| const allItemsFromTrash = itemsUpdated.every( | |
| ( item: FormResponse ) => item.status === 'trash' | |
| ); | |
| const status = allItemsFromTrash ? 'trash' : 'inbox'; |
| items.forEach( () => { | ||
| updateCountsOptimistically( 'trash', 'deleted', 1, queryParams ); | ||
| } ); | ||
|
|
||
| const promises = await Promise.allSettled( | ||
| items.map( ( { id } ) => | ||
| deleteEntityRecord( 'postType', 'feedback', id, { force: true }, { throwOnError: true } ) | ||
| ) | ||
| ); | ||
|
|
||
| const itemsUpdated = promises.filter( ( { status } ) => status === 'fulfilled' ); | ||
|
|
||
| // If there is at least one successful update, invalidate the cache for filters. | ||
| if ( itemsUpdated.length ) { | ||
| invalidateFilters(); | ||
| invalidateCacheAndNavigate( registry, getCurrentQuery(), queryParams, 'trash' ); | ||
| } | ||
|
|
||
| if ( itemsUpdated.length === items.length ) { | ||
| // Every request was successful. | ||
| const successMessage = | ||
| items.length === 1 | ||
| ? __( 'Response deleted permanently.', 'jetpack-forms' ) | ||
| : sprintf( | ||
| /* translators: %d: the number of responses. */ | ||
| _n( | ||
| '%d response deleted permanently.', | ||
| '%d responses deleted permanently.', | ||
| items.length, | ||
| 'jetpack-forms' | ||
| ), | ||
| items.length | ||
| ); | ||
|
|
||
| createSuccessNotice( successMessage, { type: 'snackbar', id: 'move-to-trash-action' } ); | ||
|
|
||
| // Update the URL to remove references to deleted items. | ||
| // Parse the hash to extract just the query params (e.g., #/responses?r=1,2,3) | ||
| const hash = window.location.hash; | ||
| const hashQueryIndex = hash.indexOf( '?' ); | ||
| const hashBase = hashQueryIndex > 0 ? hash.substring( 0, hashQueryIndex ) : hash; | ||
| const hashQuery = hashQueryIndex > 0 ? hash.substring( hashQueryIndex + 1 ) : ''; | ||
|
|
||
| const hashParams = new URLSearchParams( hashQuery ); | ||
| const currentSelection = hashParams.get( 'r' )?.split( ',' ) || []; | ||
| const deletedIds = items.map( ( { id } ) => id.toString() ); | ||
| const newSelection = currentSelection.filter( id => ! deletedIds.includes( id ) ); | ||
|
|
||
| if ( newSelection.length ) { | ||
| hashParams.set( 'r', newSelection.join( ',' ) ); | ||
| } else { | ||
| hashParams.delete( 'r' ); | ||
| } | ||
|
|
||
| const hashString = hashParams.toString(); | ||
| window.location.hash = hashString ? `${ hashBase }?${ hashString }` : hashBase; | ||
|
|
||
| return; | ||
| } | ||
| // There is at least one failure. | ||
| const numberOfErrors = promises.filter( ( { status } ) => status === 'rejected' ).length; | ||
| const errorMessage = getGenericErrorMessage( numberOfErrors ); | ||
|
|
||
| createErrorNotice( errorMessage, { type: 'snackbar' } ); |
Copilot
AI
Jan 20, 2026
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.
The deleteAction performs optimistic count updates on line 920-922 for all items but does not revert these counts if some deletions fail. If some items fail to delete (checked on line 938), the counts will be incorrect. Consider reverting the optimistic count updates for failed items.
| if ( promises.every( ( { status } ) => status === 'fulfilled' ) ) { | ||
| // Every request was successful. | ||
| const successMessage = | ||
| items.length === 1 | ||
| ? __( 'Response marked as read.', 'jetpack-forms' ) | ||
| : sprintf( | ||
| /* translators: %d: the number of responses. */ | ||
| _n( | ||
| '%d response marked as read.', | ||
| '%d responses marked as read.', | ||
| items.length, | ||
| 'jetpack-forms' | ||
| ), | ||
| items.length | ||
| ); | ||
|
|
||
| createSuccessNotice( successMessage, { | ||
| type: 'snackbar', | ||
| id: 'mark-as-read-action', | ||
| actions: [ | ||
| { | ||
| label: __( 'Undo', 'jetpack-forms' ), | ||
| onClick: () => { | ||
| markAsUnreadAction.callback( items, { registry } ); | ||
| }, | ||
| }, | ||
| ], | ||
| } ); |
Copilot
AI
Jan 20, 2026
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.
The success notice is shown when all promises are fulfilled (line 1062), but the undo action on line 1085 attempts to undo all items, including those that may have failed. If some items failed to be marked as read, undoing all items would incorrectly attempt to mark the failed items as unread. Consider only including successfully updated items in the undo callback.
| queryParams, | ||
| } ), | ||
| [ navigate, searchParams, params.view, queryParams ] |
Copilot
AI
Jan 20, 2026
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.
The parameter queryParams is passed to the getActions function but is never used in the implementation. Each action obtains queryParams dynamically by calling getCountQueryParams(getCurrentQuery()) inside the callback. This unused parameter should be removed from both the call site and the function signature.
| queryParams, | |
| } ), | |
| [ navigate, searchParams, params.view, queryParams ] | |
| } ), | |
| [ navigate, searchParams, params.view ] |
| // If there is at least one successful update, invalidate the cache and navigate if needed | ||
| if ( itemsUpdated.length ) { | ||
| let status = 'inbox'; | ||
| if ( items[ 0 ]?.status === 'trash' ) { |
Copilot
AI
Jan 20, 2026
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.
The status detection logic assumes all items have the same status by checking only items[0]?.status. In bulk operations, items could potentially have different statuses. Consider checking if all items have the 'trash' status, or handling mixed statuses more robustly.
| if ( items[ 0 ]?.status === 'trash' ) { | |
| const allItemsInTrash = itemsUpdated.every( | |
| item => item.status === 'trash' | |
| ); | |
| if ( allItemsInTrash ) { |
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.
There is no need, all items are guaranteed to have the same status, at least for now.
| const { getCurrentQuery } = registry.select( dashboardStore ); | ||
|
|
||
| const queryParams = getCountQueryParams( getCurrentQuery() ); | ||
| const previousStatus = items[ 0 ]?.status; // All items have the same status |
Copilot
AI
Jan 20, 2026
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.
The comment "All items have the same status" assumes a constraint that may not be enforced. While the isEligible check on line 764 ensures individual items are not 'trash', in a bulk operation items could still have different statuses (e.g., 'publish', 'spam', 'draft'). This could lead to incorrect undo behavior on line 869 where previousStatus is used for all items.
| const previousStatus = items[ 0 ]?.status; // All items have the same status | |
| const firstItemStatus = items[ 0 ]?.status; | |
| const hasMixedStatus = items.some( item => item.status !== firstItemStatus ); | |
| if ( hasMixedStatus ) { | |
| createErrorNotice( | |
| __( | |
| 'Unable to move responses with different statuses to trash at once.', | |
| 'jetpack-forms' | |
| ), | |
| { type: 'snackbar' } | |
| ); | |
| removeNotice( 'move-to-trash-action' ); | |
| return; | |
| } | |
| const previousStatus = firstItemStatus; |
| items.length | ||
| ); | ||
|
|
||
| createSuccessNotice( successMessage, { type: 'snackbar', id: 'move-to-trash-action' } ); |
Copilot
AI
Jan 20, 2026
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.
The notice ID 'move-to-trash-action' is used for the deleteAction, but this ID is already used by the moveToTrashAction. This could cause notice conflicts. Consider using a unique ID like 'delete-action' or 'delete-permanently-action'.
| createSuccessNotice( successMessage, { type: 'snackbar', id: 'move-to-trash-action' } ); | |
| createSuccessNotice( successMessage, { | |
| type: 'snackbar', | |
| id: 'delete-permanently-action', | |
| } ); |
| if ( promises.every( ( { status } ) => status === 'fulfilled' ) ) { | ||
| // Invalidate counts cache to ensure counts are refetched and stay accurate | ||
| invalidateCounts(); | ||
| // Mark successfully updated records as invalid instead of removing from view | ||
| const updatedIds = items.map( item => item.id ); |
Copilot
AI
Jan 20, 2026
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.
There's an inconsistency in when counts are invalidated. The markAsReadAction invalidates counts if any items succeed (line 1051), but markAsUnreadAction only invalidates counts if all items succeed (line 1165-1167). For consistency and correctness, both should invalidate counts whenever at least one item succeeds.
| if ( promises.every( ( { status } ) => status === 'fulfilled' ) ) { | |
| // Invalidate counts cache to ensure counts are refetched and stay accurate | |
| invalidateCounts(); | |
| // Mark successfully updated records as invalid instead of removing from view | |
| const updatedIds = items.map( item => item.id ); | |
| const updatedIds = promises.reduce( ( acc, { status }, index ) => { | |
| if ( status === 'fulfilled' ) { | |
| acc.push( items[ index ].id ); | |
| } | |
| return acc; | |
| }, [] ); | |
| if ( updatedIds.length > 0 ) { | |
| // Invalidate counts cache to ensure counts are refetched and stay accurate | |
| invalidateCounts(); | |
| // Mark successfully updated records as invalid instead of removing from view |

Follow up to FORMS-442
Proposed changes:
Other information:
Jetpack product discussion
Does this pull request change what data or activity we track or use?
Testing instructions:
navigate()from router