-
Notifications
You must be signed in to change notification settings - Fork 851
Forms: Use components for trash/spam header CTAs in wp-build dashboard #46695
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
Conversation
|
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! |
simison
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.
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 introduces a router-agnostic search params abstraction layer for the Forms dashboard to support using shared components in both the React Router build (dashboard) and the WordPress Route (@wordpress/route) build (wp-build). The abstraction allows the Empty Trash and Empty Spam button components to be reused across both builds.
Changes:
- Created a new
DashboardSearchParamsContextwith provider implementations for both React Router and WordPress Route - Replaced all direct
react-routeruseSearchParamsimports with the newuseDashboardSearchParamshook from the context - Replaced lodash's
isEmptywith a local implementation inuse-inbox-data.ts - Updated the wp-build dashboard to use the
EmptyTrashButtonandEmptySpamButtoncomponents instead of inline button definitions
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
dashboard-search-params-context.tsx |
Defines the router-agnostic context and hooks for accessing search params |
wp-route-dashboard-search-params-provider.tsx |
WordPress Route implementation that adapts useSearch/useNavigate to the context API |
react-router-dashboard-search-params-provider.tsx |
React Router implementation that wraps useSearchParams with the context API |
index.tsx |
Wraps the dashboard Layout with ReactRouterDashboardSearchParamsProvider |
stage.tsx |
Wraps the wp-build Page with WpRouteDashboardSearchParamsProvider and uses the new button components |
inbox/stage/views.js |
Updated to use useDashboardSearchParams instead of react-router's useSearchParams |
inbox/stage/index.js |
Updated to use useDashboardSearchParams and removed react-router import |
use-inbox-data.ts |
Updated to use useDashboardSearchParams and replaced lodash isEmpty with local implementation |
forms/views.ts |
Updated to use useDashboardSearchParams instead of react-router's useSearchParams |
inbox-status-toggle/index.tsx |
Updated to use useDashboardSearchParams and removed react-router import |
changelog/update-forms-wp-build-dash-ctas |
Changelog entry for the changes |
a38d3ce to
7683f22
Compare
|
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
Code Coverage SummaryCoverage changed in 2 files.
3 files are newly checked for coverage.
Full summary · PHP report · JS report If appropriate, add one of these labels to override the failing coverage check:
Covered by non-unit tests
|
This reverts commit 48b43f5.
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.
Tests well across all three envs/dashboards.
7683f22 to
b37d53f
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
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
| ); | ||
| const { updateCountsOptimistically, invalidateCounts } = useDispatch( | ||
| dashboardStore | ||
| ( dashboardStore as unknown as { name: string } ).name |
Copilot
AI
Jan 22, 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 store object should be passed directly to useDispatch, not its name property. This change from useDispatch(dashboardStore) to useDispatch(dashboardStore.name) breaks the standard WordPress data store pattern. The useDispatch function expects a store descriptor object, not a string identifier. This will cause runtime errors when trying to access store actions.
| ( dashboardStore as unknown as { name: string } ).name | |
| dashboardStore |
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 was curious about this change as well. Can you elaborate, @dhasilva ?
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.
Good catch, that is a leftover from trying to fix types.
| Significance: minor | ||
| Type: changed | ||
|
|
||
| Use components for empty actions in trash/spam for new wp-build dashboard. |
Copilot
AI
Jan 22, 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 changelog entry uses "actions" instead of "buttons" or "CTAs" which could be confusing since "actions" has a specific meaning in WordPress (data store actions, form actions, etc.). Consider changing to "Use components for empty trash/spam buttons in wp-build dashboard." for clarity.
| Use components for empty actions in trash/spam for new wp-build dashboard. | |
| Use components for empty trash/spam buttons in wp-build dashboard. |
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
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
| }: PropsWithChildren ): JSX.Element { | ||
| const [ searchParams, setReactRouterSearchParams ] = useReactRouterSearchParams(); | ||
|
|
||
| const setSearchParams = setReactRouterSearchParams as unknown as SetDashboardSearchParams; |
Copilot
AI
Jan 22, 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.
Type assertion used to cast React Router's setSearchParams to the custom SetDashboardSearchParams type. While this works at runtime, it would be safer to verify that the signatures are truly compatible or add a wrapper function that ensures type safety.
| Significance: minor | ||
| Type: changed | ||
|
|
||
| Use components for empty actions in trash/spam for new wp-build dashboard. |
Copilot
AI
Jan 22, 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.
Changelog entry should end with a period according to the project's changelog guidelines.
| from: from as unknown as never, | ||
| strict: false, | ||
| } ); |
Copilot
AI
Jan 22, 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.
Type assertion used to work around unregistered router type. While the comment explains the reason, consider whether the router type should be registered properly in the wp-build setup to avoid type safety compromises. The casting to unknown as never bypasses TypeScript's type checking entirely.
| from: from as unknown as never, | |
| strict: false, | |
| } ); | |
| from, | |
| strict: false, | |
| } as unknown ); |
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
Copilot reviewed 12 out of 12 changed files in this pull request and generated 3 comments.
| } | ||
|
|
||
| // For non-array keys, always coerce to a scalar string. | ||
| // If multiple values exist for a key, preserve the last one. |
Copilot
AI
Jan 22, 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 says 'preserve the last one' but URLSearchParams.getAll() returns values in the order they were added. Consider clarifying whether this intentionally takes the last value or if it should take the first value (values[0]) for consistency with typical query parameter handling.
| // If multiple values exist for a key, preserve the last one. | |
| // URLSearchParams.getAll() returns values in insertion (oldest-to-newest) order; | |
| // we intentionally keep the last value so that later duplicates override earlier ones. |
| }; | ||
|
|
||
| // https://github.com/you-dont-need/You-Dont-Need-Lodash-Underscore?tab=readme-ov-file#_isempty | ||
| const isEmpty = obj => |
Copilot
AI
Jan 22, 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 arrow function isEmpty should be defined as a named function declaration or include type annotations for its parameter and return value to maintain consistency with TypeScript best practices in this file.
| const isEmpty = obj => | |
| const isEmpty = ( obj: unknown ): boolean => |
| ( dashboardStore as unknown as { name: string } ).name | ||
| ) as DispatchActions; |
Copilot
AI
Jan 22, 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 type assertion suggests a mismatch between the expected store type and what useDispatch returns. Consider defining a proper type for the store or accessing the store name property in a type-safe way.
| ( dashboardStore as unknown as { name: string } ).name | |
| ) as DispatchActions; | |
| dashboardStore | |
| ) as unknown as DispatchActions; |
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
Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.
| const formatFieldValue = fieldValue => { | ||
| if ( isEmpty( fieldValue ) ) { | ||
| if ( ! fieldValue || isEmpty( fieldValue ) ) { | ||
| return '-'; | ||
| } else if ( Array.isArray( fieldValue ) ) { | ||
| } | ||
| if ( Array.isArray( fieldValue ) ) { |
Copilot
AI
Jan 22, 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.
Switching from lodash/isEmpty to the custom isEmpty helper while also changing the condition to if ( ! fieldValue || isEmpty( fieldValue ) ) alters the previous behavior: values like 0 or false that were previously rendered as "0" / "false" will now be treated as empty and displayed as '-'. If preserving the original semantics is desired, this should rely solely on the structural emptiness check (and/or explicitly handle only the cases that should map to '-'), so that falsy but meaningful values are not hidden from the UI.
| import * as Tabs from '../../src/dashboard/components/tabs'; | ||
| import useCreateForm from '../../src/dashboard/hooks/use-create-form'; | ||
| import { getPath } from '../../src/dashboard/inbox/utils'; | ||
| import WpRouteDashboardSearchParamsProvider from '../../src/dashboard/router/wp-route-dashboard-search-params-provider.tsx'; | ||
| import { store as dashboardStore } from '../../src/dashboard/store'; | ||
| import useConfigValue from '../../src/hooks/use-config-value'; | ||
| import { INTEGRATIONS_STORE, IntegrationsSelectors } from '../../src/store/integrations'; |
Copilot
AI
Jan 22, 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.
EmptyTrashButton / EmptySpamButton rely on useInboxData and the dashboard store’s currentQuery for invalidating core-data entity records after emptying trash/spam, but this route (Stage) never sets currentQuery in that store. In the wp-build context this means the buttons will invalidate only the store’s default query, so the DataViews list driven by this stage’s own useEntityRecords query may not be refreshed correctly after the empty actions; consider wiring this route’s active query into setCurrentQuery (similar to the React dashboard) or providing wp-build-specific invalidation so the list updates immediately after emptying.

Follow-up to FORMS-442
Alternative to #46687
Proposed changes:
import_lodash.isEmpty is not a functionshowing up on tab switching, and that we're fixing simply by not using lodash, which is anyway good change for loading performance.Slack convo about fix p1768928713328179-slack-C052XEUUBL4
Other information:
Jetpack product discussion
Does this pull request change what data or activity we track or use?
Testing instructions: