Forms: Add external admin context support to search params provider#46730
Forms: Add external admin context support to search params provider#46730
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 🤖 🔴 Action required: Please include detailed testing steps, explaining how to test your change, like so: 🔴 Action required: We would recommend that you add a section to the PR description to specify whether this PR includes any changes to data or privacy, like so: Follow this PR Review Process:
If you have questions about anything, reach out in #jetpack-developers for guidance! |
github-actions
Bot
added
the
[Status] Needs Author Reply
There was a problem hiding this comment.
Pull request overview
This PR extends the Forms dashboard search params handling to support external admin contexts where routes are embedded via a ?p= parameter. It introduces a context-based abstraction layer that allows different routing implementations (React Router and WordPress Route) to provide URL search parameter management consistently.
Changes:
- Introduces a new
DashboardSearchParamsContextwith provider and hook for managing URL search parameters - Adds
WpRouteDashboardSearchParamsProviderwith external admin context detection and p-parameter parsing - Adds
ReactRouterDashboardSearchParamsProviderfor standard React Router environments - Updates all components to use the new context instead of directly calling router hooks
- Replaces hardcoded empty trash/spam buttons with proper components in the wp-build dashboard
- Adds inline
isEmptyimplementation to replace lodash dependency
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
src/dashboard/router/dashboard-search-params-context.tsx |
New context definition with provider and hook for search params |
src/dashboard/router/react-router-dashboard-search-params-provider.tsx |
React Router implementation of the search params provider |
src/dashboard/router/wp-route-dashboard-search-params-provider.tsx |
WordPress Route implementation with external admin context support |
src/dashboard/index.tsx |
Wraps dashboard root with React Router provider |
src/dashboard/inbox/stage/index.js |
Updated to use context hook instead of react-router |
src/dashboard/inbox/stage/views.js |
Updated to use context hook instead of react-router |
src/dashboard/forms/views.ts |
Updated to use context hook instead of react-router |
src/dashboard/hooks/use-inbox-data.ts |
Updated to use context hook, adds inline isEmpty implementation |
src/dashboard/components/inbox-status-toggle/index.tsx |
Updated to use context hook instead of react-router |
routes/responses/stage.tsx |
Wraps with WpRoute provider, uses EmptyTrashButton/EmptySpamButton components |
routes/responses/inspector/index.tsx |
Wraps with WpRoute provider, adds URL parsing utilities |
tests/js/dashboard/components/empty-trash-button/index.test.jsx |
Updated tests to provide search params context |
tests/js/dashboard/components/empty-spam-button/index.test.jsx |
Updated tests to provide search params context |
changelog/update-forms-wp-build-dash-ctas |
Changelog for using components in wp-build dashboard |
changelog/fix-forms-external-admin-search-params |
Changelog for external admin context support |
| * Get effective pathname, checking for external admin ?p= parameter. | ||
| * | ||
| * @return The effective pathname. | ||
| */ | ||
| function getEffectivePathname(): string { | ||
| const url = new URL( window.location.href ); | ||
| return url.searchParams.get( 'p' ) || url.pathname; | ||
| } | ||
|
|
||
| /** | ||
| * Extract view parameter from URL pathname. | ||
| * | ||
| * @return The view parameter (inbox, spam, or trash). | ||
| */ | ||
| function getViewFromUrl(): string { | ||
| const pathname = getEffectivePathname(); | ||
| const match = pathname.match( VIEW_PATTERN ); | ||
| return match?.[ 1 ] || 'inbox'; |
There was a problem hiding this comment.
The getEffectivePathname and getViewFromUrl functions duplicate logic already present in parseExternalAdminSearchParams. This creates maintainability concerns if the external admin context detection logic needs to change. Consider extracting the external admin detection and p-parameter parsing into shared utility functions.
| * Get effective pathname, checking for external admin ?p= parameter. | |
| * | |
| * @return The effective pathname. | |
| */ | |
| function getEffectivePathname(): string { | |
| const url = new URL( window.location.href ); | |
| return url.searchParams.get( 'p' ) || url.pathname; | |
| } | |
| /** | |
| * Extract view parameter from URL pathname. | |
| * | |
| * @return The view parameter (inbox, spam, or trash). | |
| */ | |
| function getViewFromUrl(): string { | |
| const pathname = getEffectivePathname(); | |
| const match = pathname.match( VIEW_PATTERN ); | |
| return match?.[ 1 ] || 'inbox'; | |
| * Get effective pathname for a given URL, checking for external admin ?p= parameter. | |
| * | |
| * @param url URL object to extract the effective pathname from. | |
| * @return The effective pathname. | |
| */ | |
| function getEffectivePathnameFromUrl( url: URL ): string { | |
| return url.searchParams.get( 'p' ) || url.pathname; | |
| } | |
| /** | |
| * Extract view parameter from a URL pathname. | |
| * | |
| * @param pathname URL pathname to extract the view from. | |
| * @return The view parameter (inbox, spam, or trash). | |
| */ | |
| function getViewFromPathname( pathname: string ): string { | |
| const match = pathname.match( VIEW_PATTERN ); | |
| return match?.[ 1 ] || 'inbox'; | |
| } | |
| /** | |
| * Get effective pathname, checking for external admin ?p= parameter. | |
| * | |
| * @return The effective pathname. | |
| */ | |
| function getEffectivePathname(): string { | |
| const url = new URL( window.location.href ); | |
| return getEffectivePathnameFromUrl( url ); | |
| } | |
| /** | |
| * Extract view parameter from current window URL. | |
| * | |
| * @return The view parameter (inbox, spam, or trash). | |
| */ | |
| function getViewFromUrl(): string { | |
| const url = new URL( window.location.href ); | |
| const pathname = getEffectivePathnameFromUrl( url ); | |
| return getViewFromPathname( pathname ); |
| const isEmpty = obj => | ||
| [ Object, Array ].includes( ( obj || {} ).constructor ) && ! Object.entries( obj || {} ).length; |
There was a problem hiding this comment.
The isEmpty implementation doesn't correctly check for empty objects. The expression (obj || {}).constructor will return the constructor of an empty object when obj is falsy, making the check unreliable. For example, isEmpty(null) would check if null's constructor is Object or Array, which it isn't, but then checking Object.entries({}).length would return 0 (empty). Consider using a more explicit implementation like: obj == null || (typeof obj === 'object' && Object.keys(obj).length === 0).
| const isEmpty = obj => | |
| [ Object, Array ].includes( ( obj || {} ).constructor ) && ! Object.entries( obj || {} ).length; | |
| const isEmpty = ( obj: unknown ): boolean => | |
| obj == null || ( typeof obj === 'object' && Object.keys( obj ).length === 0 ); |
| ); | ||
| const { updateCountsOptimistically, invalidateCounts } = useDispatch( | ||
| dashboardStore | ||
| ( dashboardStore as unknown as { name: string } ).name |
There was a problem hiding this comment.
The type cast (dashboardStore as unknown as { name: string }).name is incorrect. The useDispatch function expects a store descriptor object, not a string name. The original code dashboardStore was likely correct, and this cast creates a runtime error by extracting a non-existent 'name' property and passing it to useDispatch. This should be reverted to just dashboardStore.
| ( dashboardStore as unknown as { name: string } ).name | |
| dashboardStore |
| /** | ||
| * External dependencies | ||
| */ | ||
| import { useCallback, useEffect, useMemo, useState } from '@wordpress/element'; | ||
| import { useSearch, useNavigate } from '@wordpress/route'; | ||
| /** | ||
| * Internal dependencies | ||
| */ | ||
| import { | ||
| DashboardSearchParamsProvider, | ||
| type DashboardSearchParamsTuple, | ||
| type SetDashboardSearchParams, | ||
| } from './dashboard-search-params-context'; | ||
| /** | ||
| * Types | ||
| */ | ||
| import type { PropsWithChildren } from 'react'; | ||
|
|
||
| type Props = PropsWithChildren< { | ||
| /** | ||
| * Route id to bind `useSearch` / `useNavigate` to. | ||
| * | ||
| * Required because ciab-admin doesn't provide a | ||
| * "nearest match" for `useSearch()` to infer. | ||
| */ | ||
| from: string; | ||
| } >; | ||
|
|
||
| /** | ||
| * Check if we're in an external admin context (e.g., params embedded in ?p= parameter). | ||
| * | ||
| * @return True if in external admin context. | ||
| */ | ||
| function isExternalAdminContext(): boolean { | ||
| const url = new URL( window.location.href ); | ||
| return url.searchParams.has( 'p' ); | ||
| } | ||
|
|
||
| /** | ||
| * Parse search params from the URL for external admin contexts. | ||
| * In these contexts, params may be embedded inside the ?p= value. | ||
| * | ||
| * @return URLSearchParams parsed from the current URL. | ||
| */ | ||
| function parseExternalAdminSearchParams(): URLSearchParams { | ||
| const url = new URL( window.location.href ); | ||
| const result = new URLSearchParams(); | ||
|
|
||
| // First, check direct URL params | ||
| let rawResponseIds = url.searchParams.getAll( 'responseIds' ); | ||
| let search = url.searchParams.get( 'search' ); | ||
|
|
||
| // If not found, check inside the p parameter (external admin context) | ||
| if ( rawResponseIds.length === 0 ) { | ||
| const pValue = url.searchParams.get( 'p' ); | ||
| if ( pValue && pValue.includes( '?' ) ) { | ||
| const pUrl = new URL( pValue, window.location.origin ); | ||
| rawResponseIds = pUrl.searchParams.getAll( 'responseIds' ); | ||
| if ( ! search ) { | ||
| search = pUrl.searchParams.get( 'search' ); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Parse responseIds, handling JSON-encoded arrays from TanStack Router | ||
| for ( const raw of rawResponseIds ) { | ||
| if ( raw.startsWith( '[' ) ) { | ||
| try { | ||
| const parsed = JSON.parse( raw ); | ||
| if ( Array.isArray( parsed ) ) { | ||
| for ( const id of parsed ) { | ||
| result.append( 'responseIds', String( id ) ); | ||
| } | ||
| continue; | ||
| } | ||
| } catch { | ||
| // Not valid JSON, treat as regular value | ||
| } | ||
| } | ||
| result.append( 'responseIds', raw ); | ||
| } | ||
|
|
||
| if ( search ) { | ||
| result.set( 'search', search ); | ||
| } | ||
|
|
||
| return result; | ||
| } | ||
|
|
||
| /** | ||
| * Convert `@wordpress/route` `search` record shape into `URLSearchParams`. | ||
| * | ||
| * @param search - Search record to convert. | ||
| * @return - URL search params. | ||
| */ | ||
| function searchRecordToUrlSearchParams( search: Record< string, unknown > ): URLSearchParams { | ||
| const params = new URLSearchParams(); | ||
|
|
||
| for ( const [ key, value ] of Object.entries( search || {} ) ) { | ||
| if ( value === undefined || value === null ) { | ||
| continue; | ||
| } | ||
|
|
||
| if ( Array.isArray( value ) ) { | ||
| for ( const v of value ) { | ||
| if ( v === undefined || v === null ) { | ||
| continue; | ||
| } | ||
|
|
||
| params.append( key, String( v ) ); | ||
| } | ||
|
|
||
| continue; | ||
| } | ||
|
|
||
| params.set( key, String( value ) ); | ||
| } | ||
|
|
||
| return params; | ||
| } | ||
|
|
||
| /** | ||
| * Convert `URLSearchParams` into the router `search` record shape. | ||
| * | ||
| * Note: some keys (e.g. `responseIds`) are treated as arrays even when only one value is present. | ||
| * | ||
| * @param params - URL search params to convert. | ||
| * @param options - Options. | ||
| * @param options.arrayKeys - Keys that should always be represented as arrays. | ||
| * @return - Record suitable for `@wordpress/route` `search`. | ||
| */ | ||
| function urlSearchParamsToSearchRecord( | ||
| params: URLSearchParams, | ||
| options: { | ||
| /** | ||
| * Keys that should always be represented as arrays. | ||
| * For example: `responseIds` is treated as `string[]` by routes that use selection. | ||
| */ | ||
| arrayKeys?: ReadonlySet< string >; | ||
| } = {} | ||
| ): Record< string, string | string[] > { | ||
| const { arrayKeys = new Set< string >() } = options; | ||
| const out: Record< string, string | string[] > = {}; | ||
|
|
||
| for ( const key of params.keys() ) { | ||
| const values = params.getAll( key ); | ||
|
|
||
| if ( values.length < 1 ) { | ||
| continue; | ||
| } | ||
|
|
||
| if ( arrayKeys.has( key ) ) { | ||
| out[ key ] = values; | ||
| continue; | ||
| } | ||
|
|
||
| // For non-array keys, always coerce to a scalar string. | ||
| // If multiple values exist for a key, preserve the last one. | ||
| out[ key ] = values[ values.length - 1 ]; | ||
| } | ||
|
|
||
| return out; | ||
| } | ||
|
|
||
| /** | ||
| * Provider for the dashboard search params. | ||
| * | ||
| * @param props - Props. | ||
| * @param props.from - Route id to bind `useSearch` / `useNavigate` to. | ||
| * @param props.children - Children. | ||
| * @return - JSX element. | ||
| */ | ||
| export default function WpRouteDashboardSearchParamsProvider( { | ||
| from, | ||
| children, | ||
| }: Props ): JSX.Element { | ||
| const isExternal = isExternalAdminContext(); | ||
|
|
||
| // For external admin contexts, use URL-based parsing with popstate listener | ||
| const [ externalParams, setExternalParams ] = useState( () => | ||
| isExternal ? parseExternalAdminSearchParams() : new URLSearchParams() | ||
| ); | ||
|
|
||
| useEffect( () => { | ||
| if ( ! isExternal ) { | ||
| return; | ||
| } | ||
|
|
||
| /** | ||
| * Updates params state when URL changes via browser navigation. | ||
| */ | ||
| function handleUrlChange() { | ||
| setExternalParams( parseExternalAdminSearchParams() ); | ||
| } | ||
|
|
||
| window.addEventListener( 'popstate', handleUrlChange ); | ||
| return () => window.removeEventListener( 'popstate', handleUrlChange ); | ||
| }, [ isExternal ] ); | ||
|
|
||
| // `useSearch()` re-renders when the router search changes. We'll adapt it to URLSearchParams | ||
| // so shared dashboard code can keep using the URLSearchParams API. | ||
| const search = useSearch( { | ||
| // In this build, the router type isn't registered, so `@wordpress/route` models `from` as `never`. | ||
| // We still want to pass the runtime route id through. | ||
| from: from as unknown as never, | ||
| strict: false, | ||
| } ); | ||
| const navigate = useNavigate(); | ||
| const routerParams = useMemo( () => searchRecordToUrlSearchParams( search ), [ search ] ); | ||
|
|
||
| // Use external params if in external admin context, otherwise use router params | ||
| const searchParams = isExternal ? externalParams : routerParams; | ||
|
|
||
| const setSearchParams: SetDashboardSearchParams = useCallback( | ||
| next => { | ||
| const resolved = typeof next === 'function' ? next( searchParams ) : next; | ||
|
|
||
| if ( isExternal ) { | ||
| // In external admin context, update URL directly via history API | ||
| const url = new URL( window.location.href ); | ||
| const pValue = url.searchParams.get( 'p' ); | ||
|
|
||
| if ( pValue ) { | ||
| // Update params inside the p parameter | ||
| const pUrl = new URL( pValue, window.location.origin ); | ||
| // Clear existing params we manage | ||
| pUrl.searchParams.delete( 'responseIds' ); | ||
| pUrl.searchParams.delete( 'search' ); | ||
| // Set new params | ||
| for ( const [ key, value ] of resolved.entries() ) { | ||
| pUrl.searchParams.append( key, value ); | ||
| } | ||
| url.searchParams.set( 'p', pUrl.pathname + pUrl.search ); | ||
| } else { | ||
| // Update direct URL params | ||
| url.searchParams.delete( 'responseIds' ); | ||
| url.searchParams.delete( 'search' ); | ||
| for ( const [ key, value ] of resolved.entries() ) { | ||
| url.searchParams.append( key, value ); | ||
| } | ||
| } | ||
|
|
||
| window.history.pushState( {}, '', url.toString() ); | ||
| setExternalParams( resolved ); | ||
| return; | ||
| } | ||
|
|
||
| const nextSearch = urlSearchParamsToSearchRecord( resolved, { | ||
| arrayKeys: new Set( [ 'responseIds' ] ), | ||
| } ); | ||
|
|
||
| type NavigateArg = Parameters< typeof navigate >[ 0 ]; | ||
| type SearchOption = NavigateArg extends { search?: infer S } ? S : never; | ||
| type SearchReducer = Exclude< SearchOption, true | undefined >; | ||
|
|
||
| // In `@wordpress/route`, `search` is modeled as a reducer function, but without a registered | ||
| // router type it ends up as a very "loose" signature. We still pass a fully-formed object. | ||
| const replaceSearch = ( () => nextSearch ) as unknown as SearchReducer; | ||
|
|
||
| navigate( { search: replaceSearch } ); | ||
| }, | ||
| [ isExternal, navigate, searchParams ] | ||
| ); | ||
|
|
||
| const value = useMemo( | ||
| () => [ searchParams, setSearchParams ] as DashboardSearchParamsTuple, | ||
| [ searchParams, setSearchParams ] | ||
| ); | ||
|
|
||
| return ( | ||
| <DashboardSearchParamsProvider value={ value }>{ children }</DashboardSearchParamsProvider> | ||
| ); | ||
| } |
There was a problem hiding this comment.
The new WpRouteDashboardSearchParamsProvider component and its helper functions (parseExternalAdminSearchParams, isExternalAdminContext, etc.) lack test coverage. Given the complexity of the external admin context detection and URL parsing logic, especially the JSON array parsing and the p-parameter handling, these functions should have comprehensive unit tests to ensure they work correctly in various scenarios.
| const pUrl = new URL( pValue, window.location.origin ); | ||
| rawResponseIds = pUrl.searchParams.getAll( 'responseIds' ); | ||
| if ( ! search ) { | ||
| search = pUrl.searchParams.get( 'search' ); | ||
| } |
There was a problem hiding this comment.
When constructing a URL from the p parameter value (line 57 and 225), the code assumes the pValue can be converted to a URL using window.location.origin as the base. However, if pValue starts with a protocol (e.g., 'http://'), this could create an incorrect URL. Consider validating that pValue is a relative path before constructing the URL, or handle absolute URLs differently.
| const pUrl = new URL( pValue, window.location.origin ); | |
| rawResponseIds = pUrl.searchParams.getAll( 'responseIds' ); | |
| if ( ! search ) { | |
| search = pUrl.searchParams.get( 'search' ); | |
| } | |
| let pUrl: URL | null = null; | |
| try { | |
| // Treat absolute URLs differently from relative paths. | |
| if ( /^[a-zA-Z][a-zA-Z\d+\-.]*:/.test( pValue ) ) { | |
| pUrl = new URL( pValue ); | |
| } else { | |
| pUrl = new URL( pValue, window.location.origin ); | |
| } | |
| } catch { | |
| pUrl = null; | |
| } | |
| if ( pUrl ) { | |
| rawResponseIds = pUrl.searchParams.getAll( 'responseIds' ); | |
| if ( ! search ) { | |
| search = pUrl.searchParams.get( 'search' ); | |
| } | |
| } |
| // First, check direct URL params | ||
| let rawResponseIds = url.searchParams.getAll( 'responseIds' ); | ||
| let search = url.searchParams.get( 'search' ); | ||
|
|
||
| // If not found, check inside the p parameter (external admin context) | ||
| if ( rawResponseIds.length === 0 ) { | ||
| const pValue = url.searchParams.get( 'p' ); | ||
| if ( pValue && pValue.includes( '?' ) ) { | ||
| const pUrl = new URL( pValue, window.location.origin ); | ||
| rawResponseIds = pUrl.searchParams.getAll( 'responseIds' ); | ||
| if ( ! search ) { | ||
| search = pUrl.searchParams.get( 'search' ); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Parse responseIds, handling JSON-encoded arrays from TanStack Router | ||
| for ( const raw of rawResponseIds ) { | ||
| if ( raw.startsWith( '[' ) ) { | ||
| try { | ||
| const parsed = JSON.parse( raw ); | ||
| if ( Array.isArray( parsed ) ) { | ||
| for ( const id of parsed ) { | ||
| result.append( 'responseIds', String( id ) ); | ||
| } | ||
| continue; | ||
| } | ||
| } catch { | ||
| // Not valid JSON, treat as regular value | ||
| } | ||
| } | ||
| result.append( 'responseIds', raw ); | ||
| } | ||
|
|
||
| if ( search ) { | ||
| result.set( 'search', search ); |
There was a problem hiding this comment.
The parseExternalAdminSearchParams function only handles 'responseIds' and 'search' parameters. This creates tight coupling and means any other search parameters used by the dashboard won't work in external admin contexts. Consider making this more generic by parsing all parameters from the embedded URL, not just these two specific ones.
| // First, check direct URL params | |
| let rawResponseIds = url.searchParams.getAll( 'responseIds' ); | |
| let search = url.searchParams.get( 'search' ); | |
| // If not found, check inside the p parameter (external admin context) | |
| if ( rawResponseIds.length === 0 ) { | |
| const pValue = url.searchParams.get( 'p' ); | |
| if ( pValue && pValue.includes( '?' ) ) { | |
| const pUrl = new URL( pValue, window.location.origin ); | |
| rawResponseIds = pUrl.searchParams.getAll( 'responseIds' ); | |
| if ( ! search ) { | |
| search = pUrl.searchParams.get( 'search' ); | |
| } | |
| } | |
| } | |
| // Parse responseIds, handling JSON-encoded arrays from TanStack Router | |
| for ( const raw of rawResponseIds ) { | |
| if ( raw.startsWith( '[' ) ) { | |
| try { | |
| const parsed = JSON.parse( raw ); | |
| if ( Array.isArray( parsed ) ) { | |
| for ( const id of parsed ) { | |
| result.append( 'responseIds', String( id ) ); | |
| } | |
| continue; | |
| } | |
| } catch { | |
| // Not valid JSON, treat as regular value | |
| } | |
| } | |
| result.append( 'responseIds', raw ); | |
| } | |
| if ( search ) { | |
| result.set( 'search', search ); | |
| /** | |
| * Append a `responseIds` value to the params, handling JSON-encoded arrays | |
| * from TanStack Router (e.g., `["1","2"]`). | |
| * | |
| * @param params - Params object to mutate. | |
| * @param raw - Raw value to append. | |
| */ | |
| const appendResponseIds = ( params: URLSearchParams, raw: string ): void => { | |
| if ( raw.startsWith( '[' ) ) { | |
| try { | |
| const parsed = JSON.parse( raw ); | |
| if ( Array.isArray( parsed ) ) { | |
| for ( const id of parsed ) { | |
| params.append( 'responseIds', String( id ) ); | |
| } | |
| return; | |
| } | |
| } catch { | |
| // Not valid JSON, treat as regular value. | |
| } | |
| } | |
| params.append( 'responseIds', raw ); | |
| }; | |
| // First, copy all direct URL params, except the `p` embedding param. | |
| url.searchParams.forEach( ( value, key ) => { | |
| if ( key === 'p' ) { | |
| return; | |
| } | |
| if ( key === 'responseIds' ) { | |
| appendResponseIds( result, value ); | |
| return; | |
| } | |
| result.append( key, value ); | |
| } ); | |
| // Then, merge params from inside the `p` parameter (external admin context). | |
| const pValue = url.searchParams.get( 'p' ); | |
| if ( pValue && pValue.includes( '?' ) ) { | |
| try { | |
| const pUrl = new URL( pValue, window.location.origin ); | |
| pUrl.searchParams.forEach( ( value, key ) => { | |
| // Avoid re-appending keys that are already present from the top-level URL. | |
| if ( key !== 'responseIds' && result.has( key ) ) { | |
| return; | |
| } | |
| if ( key === 'responseIds' ) { | |
| appendResponseIds( result, value ); | |
| return; | |
| } | |
| result.append( key, value ); | |
| } ); | |
| } catch { | |
| // If `p` is not a valid URL, ignore it and fall back to top-level params only. | |
| } |
| if ( isExternal ) { | ||
| // In external admin context, update URL directly via history API | ||
| const url = new URL( window.location.href ); | ||
| const pValue = url.searchParams.get( 'p' ); | ||
|
|
||
| if ( pValue ) { | ||
| // Update params inside the p parameter | ||
| const pUrl = new URL( pValue, window.location.origin ); | ||
| // Clear existing params we manage | ||
| pUrl.searchParams.delete( 'responseIds' ); | ||
| pUrl.searchParams.delete( 'search' ); | ||
| // Set new params | ||
| for ( const [ key, value ] of resolved.entries() ) { | ||
| pUrl.searchParams.append( key, value ); | ||
| } | ||
| url.searchParams.set( 'p', pUrl.pathname + pUrl.search ); | ||
| } else { | ||
| // Update direct URL params | ||
| url.searchParams.delete( 'responseIds' ); | ||
| url.searchParams.delete( 'search' ); | ||
| for ( const [ key, value ] of resolved.entries() ) { | ||
| url.searchParams.append( key, value ); | ||
| } | ||
| } | ||
|
|
||
| window.history.pushState( {}, '', url.toString() ); | ||
| setExternalParams( resolved ); | ||
| return; |
There was a problem hiding this comment.
The setSearchParams function hardcodes deletion of only 'responseIds' and 'search' parameters when updating the URL. This means if other parameters are present, they will be retained even if they were explicitly removed from the resolved URLSearchParams. Consider deleting all existing managed parameters or iterating through all keys in the current params to properly handle removal.
Code Coverage SummaryCoverage changed in 4 files.
1 file is 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
|
| const newPath = pUrl.pathname.replace( /\/[^/?#]+$/, `/${ newView }` ); | ||
| url.searchParams.set( 'p', newPath + pUrl.search ); | ||
| window.history.pushState( {}, '', url.toString() ); | ||
| window.location.reload(); |
There was a problem hiding this comment.
Forcing a full page reload when changing tabs in external admin context defeats the purpose of a single-page application and creates a poor user experience. Consider dispatching a custom event that updates all provider instances instead of reloading the page, similar to how DASHBOARD_URL_CHANGE_EVENT is used elsewhere in this PR.
| // First, check direct URL params | ||
| let rawResponseIds = url.searchParams.getAll( 'responseIds' ); | ||
| let search = url.searchParams.get( 'search' ); | ||
|
|
||
| // If not found, check inside the p parameter (external admin context) | ||
| if ( rawResponseIds.length === 0 ) { | ||
| const pValue = url.searchParams.get( 'p' ); | ||
| if ( pValue && pValue.includes( '?' ) ) { | ||
| const pUrl = new URL( pValue, window.location.origin ); | ||
| rawResponseIds = pUrl.searchParams.getAll( 'responseIds' ); | ||
| if ( ! search ) { | ||
| search = pUrl.searchParams.get( 'search' ); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Parse responseIds, handling JSON-encoded arrays from TanStack Router | ||
| for ( const raw of rawResponseIds ) { | ||
| if ( raw.startsWith( '[' ) ) { | ||
| try { | ||
| const parsed = JSON.parse( raw ); | ||
| if ( Array.isArray( parsed ) ) { | ||
| for ( const id of parsed ) { | ||
| result.append( 'responseIds', String( id ) ); | ||
| } | ||
| continue; | ||
| } | ||
| } catch { | ||
| // Not valid JSON, treat as regular value | ||
| } | ||
| } | ||
| result.append( 'responseIds', raw ); | ||
| } | ||
|
|
||
| if ( search ) { | ||
| result.set( 'search', search ); | ||
| } | ||
|
|
There was a problem hiding this comment.
The parseExternalAdminSearchParams function is hardcoded to only handle responseIds and search parameters. This limits extensibility if other search parameters need to be supported in the future. Consider making this more generic by parsing all search parameters, not just these two specific ones.
| // First, check direct URL params | |
| let rawResponseIds = url.searchParams.getAll( 'responseIds' ); | |
| let search = url.searchParams.get( 'search' ); | |
| // If not found, check inside the p parameter (external admin context) | |
| if ( rawResponseIds.length === 0 ) { | |
| const pValue = url.searchParams.get( 'p' ); | |
| if ( pValue && pValue.includes( '?' ) ) { | |
| const pUrl = new URL( pValue, window.location.origin ); | |
| rawResponseIds = pUrl.searchParams.getAll( 'responseIds' ); | |
| if ( ! search ) { | |
| search = pUrl.searchParams.get( 'search' ); | |
| } | |
| } | |
| } | |
| // Parse responseIds, handling JSON-encoded arrays from TanStack Router | |
| for ( const raw of rawResponseIds ) { | |
| if ( raw.startsWith( '[' ) ) { | |
| try { | |
| const parsed = JSON.parse( raw ); | |
| if ( Array.isArray( parsed ) ) { | |
| for ( const id of parsed ) { | |
| result.append( 'responseIds', String( id ) ); | |
| } | |
| continue; | |
| } | |
| } catch { | |
| // Not valid JSON, treat as regular value | |
| } | |
| } | |
| result.append( 'responseIds', raw ); | |
| } | |
| if ( search ) { | |
| result.set( 'search', search ); | |
| } | |
| // Track whether we have direct responseIds; this gates whether we inspect ?p=. | |
| const directResponseIds = url.searchParams.getAll( 'responseIds' ); | |
| const hasDirectResponseIds = directResponseIds.length > 0; | |
| // First, copy direct URL params (excluding the container ?p= param). | |
| for ( const [ key, value ] of url.searchParams.entries() ) { | |
| if ( key === 'p' ) { | |
| continue; | |
| } | |
| result.append( key, value ); | |
| } | |
| // If no direct responseIds are present, check inside the p parameter | |
| // for additional params (external admin context). | |
| if ( ! hasDirectResponseIds ) { | |
| const pValue = url.searchParams.get( 'p' ); | |
| if ( pValue && pValue.includes( '?' ) ) { | |
| const pUrl = new URL( pValue, window.location.origin ); | |
| for ( const [ key, value ] of pUrl.searchParams.entries() ) { | |
| // Do not overwrite any params already present from the direct URL. | |
| if ( ! result.has( key ) ) { | |
| result.append( key, value ); | |
| } | |
| } | |
| } | |
| } | |
| // Normalize responseIds, handling JSON-encoded arrays from TanStack Router. | |
| const rawResponseIds = result.getAll( 'responseIds' ); | |
| if ( rawResponseIds.length > 0 ) { | |
| result.delete( 'responseIds' ); | |
| for ( const raw of rawResponseIds ) { | |
| if ( raw.startsWith( '[' ) ) { | |
| try { | |
| const parsed = JSON.parse( raw ); | |
| if ( Array.isArray( parsed ) ) { | |
| for ( const id of parsed ) { | |
| result.append( 'responseIds', String( id ) ); | |
| } | |
| continue; | |
| } | |
| } catch { | |
| // Not valid JSON, treat as regular value. | |
| } | |
| } | |
| result.append( 'responseIds', raw ); | |
| } | |
| } |
| // https://github.com/you-dont-need/You-Dont-Need-Lodash-Underscore?tab=readme-ov-file#_isempty | ||
| const isEmpty = obj => | ||
| [ Object, Array ].includes( ( obj || {} ).constructor ) && ! Object.entries( obj || {} ).length; | ||
|
|
There was a problem hiding this comment.
The custom isEmpty implementation is incomplete and could produce incorrect results. It only checks if Object or Array constructors are used, which fails for objects created with Object.create(null), Map, Set, and other collection types. Additionally, the check !Object.entries(obj || {}).length would incorrectly return true for objects with non-enumerable properties. If removing lodash dependency, consider using a more robust implementation or handle the specific cases needed for this codebase.
| // https://github.com/you-dont-need/You-Dont-Need-Lodash-Underscore?tab=readme-ov-file#_isempty | |
| const isEmpty = obj => | |
| [ Object, Array ].includes( ( obj || {} ).constructor ) && ! Object.entries( obj || {} ).length; | |
| const isEmpty = ( value: unknown ): boolean => { | |
| if ( value == null ) { | |
| return true; | |
| } | |
| if ( typeof value === 'string' ) { | |
| return value.trim().length === 0; | |
| } | |
| if ( Array.isArray( value ) ) { | |
| return value.length === 0; | |
| } | |
| if ( value instanceof Map || value instanceof Set ) { | |
| return value.size === 0; | |
| } | |
| if ( typeof value === 'object' ) { | |
| const obj = value as object; | |
| if ( Object.getOwnPropertyNames( obj ).length > 0 ) { | |
| return false; | |
| } | |
| if ( typeof Object.getOwnPropertySymbols === 'function' && Object.getOwnPropertySymbols( obj ).length > 0 ) { | |
| return false; | |
| } | |
| return true; | |
| } | |
| return false; | |
| }; |
| pUrl.searchParams.delete( 'responseIds' ); | ||
| pUrl.searchParams.delete( 'search' ); | ||
| // Set new params | ||
| for ( const [ key, value ] of resolved.entries() ) { | ||
| pUrl.searchParams.append( key, value ); | ||
| } | ||
| url.searchParams.set( 'p', pUrl.pathname + pUrl.search ); | ||
| } else { | ||
| // Update direct URL params | ||
| url.searchParams.delete( 'responseIds' ); | ||
| url.searchParams.delete( 'search' ); |
There was a problem hiding this comment.
The setSearchParams function in ExternalAdminSearchParamsProvider hardcodes deletion of only responseIds and search parameters at lines 200-201 and 209-210. This means other search parameters that might have been present are preserved when updating, which could lead to stale or conflicting parameter combinations. Consider clearing all managed search parameters or providing a more explicit API for parameter replacement vs. merging.
| children, | ||
| }: Props ): JSX.Element { | ||
| // Check context once at mount - this determines which provider to use | ||
| const [ isExternal ] = useState( isExternalAdminContext ); |
There was a problem hiding this comment.
Using useState to cache the result of isExternalAdminContext() at mount time means the provider cannot adapt if the context changes during the component's lifetime. While this may be intentional for the current use case, if the URL's ?p= parameter can change dynamically (e.g., through browser navigation or programmatic updates), this cached value will become stale and the wrong provider implementation will be used.
| // Update params inside the p parameter | ||
| const pUrl = new URL( pValue, window.location.origin ); | ||
| // Clear existing params we manage | ||
| pUrl.searchParams.delete( 'responseIds' ); | ||
| pUrl.searchParams.delete( 'search' ); | ||
| // Set new params | ||
| for ( const [ key, value ] of resolved.entries() ) { | ||
| pUrl.searchParams.append( key, value ); | ||
| } | ||
| url.searchParams.set( 'p', pUrl.pathname + pUrl.search ); |
There was a problem hiding this comment.
Similar error handling issue: At line 198, constructing a new URL from pValue could throw an error if the value is malformed. Add try-catch error handling to prevent the entire URL update operation from failing due to invalid input.
| // Update params inside the p parameter | |
| const pUrl = new URL( pValue, window.location.origin ); | |
| // Clear existing params we manage | |
| pUrl.searchParams.delete( 'responseIds' ); | |
| pUrl.searchParams.delete( 'search' ); | |
| // Set new params | |
| for ( const [ key, value ] of resolved.entries() ) { | |
| pUrl.searchParams.append( key, value ); | |
| } | |
| url.searchParams.set( 'p', pUrl.pathname + pUrl.search ); | |
| try { | |
| // Update params inside the p parameter | |
| const pUrl = new URL( pValue, window.location.origin ); | |
| // Clear existing params we manage | |
| pUrl.searchParams.delete( 'responseIds' ); | |
| pUrl.searchParams.delete( 'search' ); | |
| // Set new params | |
| for ( const [ key, value ] of resolved.entries() ) { | |
| pUrl.searchParams.append( key, value ); | |
| } | |
| url.searchParams.set( 'p', pUrl.pathname + pUrl.search ); | |
| } catch { | |
| // Fallback: Update direct URL params if pValue is malformed | |
| url.searchParams.delete( 'responseIds' ); | |
| url.searchParams.delete( 'search' ); | |
| for ( const [ key, value ] of resolved.entries() ) { | |
| url.searchParams.append( key, value ); | |
| } | |
| } |
|
|
||
| /** | ||
| * Pattern to extract view from URL pathname. | ||
| */ | ||
| const VIEW_PATTERN = /(?:\/responses\/|\/marketing\/forms\/|\/forms\/)([^/?#]+)/; | ||
|
|
||
| /** | ||
| * Get effective pathname, checking for external admin ?p= parameter. | ||
| * | ||
| * @return The effective pathname. | ||
| */ | ||
| function getEffectivePathname(): string { | ||
| const url = new URL( window.location.href ); | ||
| return url.searchParams.get( 'p' ) || url.pathname; | ||
| } | ||
|
|
||
| /** | ||
| * Extract view parameter from URL pathname. | ||
| * | ||
| * @return The view parameter (inbox, spam, or trash). | ||
| */ | ||
| function getViewFromUrl(): string { | ||
| const pathname = getEffectivePathname(); | ||
| const match = pathname.match( VIEW_PATTERN ); | ||
| return match?.[ 1 ] || 'inbox'; | ||
| } | ||
|
|
There was a problem hiding this comment.
Code duplication detected. The functions getEffectivePathname, getViewFromUrl, and the VIEW_PATTERN constant are duplicated from utils.ts. Import these from the centralized utilities module instead of redefining them here.
| /** | |
| * Pattern to extract view from URL pathname. | |
| */ | |
| const VIEW_PATTERN = /(?:\/responses\/|\/marketing\/forms\/|\/forms\/)([^/?#]+)/; | |
| /** | |
| * Get effective pathname, checking for external admin ?p= parameter. | |
| * | |
| * @return The effective pathname. | |
| */ | |
| function getEffectivePathname(): string { | |
| const url = new URL( window.location.href ); | |
| return url.searchParams.get( 'p' ) || url.pathname; | |
| } | |
| /** | |
| * Extract view parameter from URL pathname. | |
| * | |
| * @return The view parameter (inbox, spam, or trash). | |
| */ | |
| function getViewFromUrl(): string { | |
| const pathname = getEffectivePathname(); | |
| const match = pathname.match( VIEW_PATTERN ); | |
| return match?.[ 1 ] || 'inbox'; | |
| } | |
| import { getViewFromUrl } from '../../../src/dashboard/utils'; |
| // Use JSON array format to match TanStack Router expectations | ||
| nextParams.set( 'responseIds', JSON.stringify( items.map( String ) ) ); |
There was a problem hiding this comment.
Inconsistent handling of responseIds parameter format. At line 258, when setting responseIds, it's stored as a single JSON-stringified array value (e.g., ["1","2"]). However, at line 854-857, when viewing items, multiple responseIds values are appended individually. This inconsistency could lead to parsing issues. The format should be consistent across all usages.
| // Use JSON array format to match TanStack Router expectations | |
| nextParams.set( 'responseIds', JSON.stringify( items.map( String ) ) ); | |
| // Store each selected response ID as its own query parameter value. | |
| items.forEach( item => { | |
| nextParams.append( 'responseIds', String( item ) ); | |
| } ); |
| Significance: minor | ||
| Type: changed | ||
|
|
||
| Use components for empty actions in trash/spam for new wp-build dashboard. |
There was a problem hiding this comment.
The changelog entry should start with a capital letter and end with a period according to the project's coding guidelines. Additionally, it should use imperative mood.
| /** | ||
| * External dependencies | ||
| */ | ||
| import { useCallback, useEffect, useMemo, useState } from '@wordpress/element'; | ||
| import { useSearch, useNavigate } from '@wordpress/route'; | ||
| /** | ||
| * Internal dependencies | ||
| */ | ||
| import { | ||
| DashboardSearchParamsProvider, | ||
| type DashboardSearchParamsTuple, | ||
| type SetDashboardSearchParams, | ||
| } from './dashboard-search-params-context'; | ||
| import { isExternalAdminContext, DASHBOARD_URL_CHANGE_EVENT } from './utils'; | ||
| /** | ||
| * Types | ||
| */ | ||
| import type { PropsWithChildren } from 'react'; | ||
|
|
||
| type Props = PropsWithChildren< { | ||
| /** | ||
| * Route id to bind `useSearch` / `useNavigate` to. | ||
| * | ||
| * Required because ciab-admin doesn't provide a | ||
| * "nearest match" for `useSearch()` to infer. | ||
| */ | ||
| from: string; | ||
| } >; | ||
|
|
||
| /** | ||
| * Parse search params from the URL for external admin contexts. | ||
| * In these contexts, params may be embedded inside the ?p= value. | ||
| * | ||
| * @return URLSearchParams parsed from the current URL. | ||
| */ | ||
| function parseExternalAdminSearchParams(): URLSearchParams { | ||
| const url = new URL( window.location.href ); | ||
| const result = new URLSearchParams(); | ||
|
|
||
| // First, check direct URL params | ||
| let rawResponseIds = url.searchParams.getAll( 'responseIds' ); | ||
| let search = url.searchParams.get( 'search' ); | ||
|
|
||
| // If not found, check inside the p parameter (external admin context) | ||
| if ( rawResponseIds.length === 0 ) { | ||
| const pValue = url.searchParams.get( 'p' ); | ||
| if ( pValue && pValue.includes( '?' ) ) { | ||
| const pUrl = new URL( pValue, window.location.origin ); | ||
| rawResponseIds = pUrl.searchParams.getAll( 'responseIds' ); | ||
| if ( ! search ) { | ||
| search = pUrl.searchParams.get( 'search' ); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Parse responseIds, handling JSON-encoded arrays from TanStack Router | ||
| for ( const raw of rawResponseIds ) { | ||
| if ( raw.startsWith( '[' ) ) { | ||
| try { | ||
| const parsed = JSON.parse( raw ); | ||
| if ( Array.isArray( parsed ) ) { | ||
| for ( const id of parsed ) { | ||
| result.append( 'responseIds', String( id ) ); | ||
| } | ||
| continue; | ||
| } | ||
| } catch { | ||
| // Not valid JSON, treat as regular value | ||
| } | ||
| } | ||
| result.append( 'responseIds', raw ); | ||
| } | ||
|
|
||
| if ( search ) { | ||
| result.set( 'search', search ); | ||
| } | ||
|
|
||
| return result; | ||
| } | ||
|
|
||
| /** | ||
| * Convert `@wordpress/route` `search` record shape into `URLSearchParams`. | ||
| * | ||
| * @param search - Search record to convert. | ||
| * @return - URL search params. | ||
| */ | ||
| function searchRecordToUrlSearchParams( search: Record< string, unknown > ): URLSearchParams { | ||
| const params = new URLSearchParams(); | ||
|
|
||
| for ( const [ key, value ] of Object.entries( search || {} ) ) { | ||
| if ( value === undefined || value === null ) { | ||
| continue; | ||
| } | ||
|
|
||
| if ( Array.isArray( value ) ) { | ||
| for ( const v of value ) { | ||
| if ( v === undefined || v === null ) { | ||
| continue; | ||
| } | ||
|
|
||
| params.append( key, String( v ) ); | ||
| } | ||
|
|
||
| continue; | ||
| } | ||
|
|
||
| params.set( key, String( value ) ); | ||
| } | ||
|
|
||
| return params; | ||
| } | ||
|
|
||
| /** | ||
| * Convert `URLSearchParams` into the router `search` record shape. | ||
| * | ||
| * Note: some keys (e.g. `responseIds`) are treated as arrays even when only one value is present. | ||
| * | ||
| * @param params - URL search params to convert. | ||
| * @param options - Options. | ||
| * @param options.arrayKeys - Keys that should always be represented as arrays. | ||
| * @return - Record suitable for `@wordpress/route` `search`. | ||
| */ | ||
| function urlSearchParamsToSearchRecord( | ||
| params: URLSearchParams, | ||
| options: { | ||
| /** | ||
| * Keys that should always be represented as arrays. | ||
| * For example: `responseIds` is treated as `string[]` by routes that use selection. | ||
| */ | ||
| arrayKeys?: ReadonlySet< string >; | ||
| } = {} | ||
| ): Record< string, string | string[] > { | ||
| const { arrayKeys = new Set< string >() } = options; | ||
| const out: Record< string, string | string[] > = {}; | ||
|
|
||
| for ( const key of params.keys() ) { | ||
| const values = params.getAll( key ); | ||
|
|
||
| if ( values.length < 1 ) { | ||
| continue; | ||
| } | ||
|
|
||
| if ( arrayKeys.has( key ) ) { | ||
| out[ key ] = values; | ||
| continue; | ||
| } | ||
|
|
||
| // For non-array keys, always coerce to a scalar string. | ||
| // If multiple values exist for a key, preserve the last one. | ||
| out[ key ] = values[ values.length - 1 ]; | ||
| } | ||
|
|
||
| return out; | ||
| } | ||
|
|
||
| /** | ||
| * Provider for external admin contexts (no router hooks). | ||
| * Uses direct URL parsing and history API for navigation. | ||
| * | ||
| * @param props - Props. | ||
| * @param props.children - Children. | ||
| * @return - JSX element. | ||
| */ | ||
| function ExternalAdminSearchParamsProvider( { | ||
| children, | ||
| }: { | ||
| children: React.ReactNode; | ||
| } ): JSX.Element { | ||
| const [ searchParams, setSearchParamsState ] = useState( parseExternalAdminSearchParams ); | ||
|
|
||
| useEffect( () => { | ||
| /** | ||
| * Updates params state when URL changes via browser navigation. | ||
| */ | ||
| function handleUrlChange() { | ||
| setSearchParamsState( parseExternalAdminSearchParams() ); | ||
| } | ||
|
|
||
| // Listen for both browser navigation and custom dashboard URL changes | ||
| window.addEventListener( 'popstate', handleUrlChange ); | ||
| window.addEventListener( DASHBOARD_URL_CHANGE_EVENT, handleUrlChange ); | ||
| return () => { | ||
| window.removeEventListener( 'popstate', handleUrlChange ); | ||
| window.removeEventListener( DASHBOARD_URL_CHANGE_EVENT, handleUrlChange ); | ||
| }; | ||
| }, [] ); | ||
|
|
||
| const setSearchParams: SetDashboardSearchParams = useCallback( | ||
| next => { | ||
| const resolved = typeof next === 'function' ? next( searchParams ) : next; | ||
|
|
||
| // Update URL via history API | ||
| const url = new URL( window.location.href ); | ||
| const pValue = url.searchParams.get( 'p' ); | ||
|
|
||
| if ( pValue ) { | ||
| // Update params inside the p parameter | ||
| const pUrl = new URL( pValue, window.location.origin ); | ||
| // Clear existing params we manage | ||
| pUrl.searchParams.delete( 'responseIds' ); | ||
| pUrl.searchParams.delete( 'search' ); | ||
| // Set new params | ||
| for ( const [ key, value ] of resolved.entries() ) { | ||
| pUrl.searchParams.append( key, value ); | ||
| } | ||
| url.searchParams.set( 'p', pUrl.pathname + pUrl.search ); | ||
| } else { | ||
| // Update direct URL params | ||
| url.searchParams.delete( 'responseIds' ); | ||
| url.searchParams.delete( 'search' ); | ||
| for ( const [ key, value ] of resolved.entries() ) { | ||
| url.searchParams.append( key, value ); | ||
| } | ||
| } | ||
|
|
||
| window.history.pushState( {}, '', url.toString() ); | ||
| setSearchParamsState( resolved ); | ||
| // Dispatch custom event so other provider instances can update | ||
| window.dispatchEvent( new CustomEvent( DASHBOARD_URL_CHANGE_EVENT ) ); | ||
| }, | ||
| [ searchParams ] | ||
| ); | ||
|
|
||
| const value = useMemo( | ||
| () => [ searchParams, setSearchParams ] as DashboardSearchParamsTuple, | ||
| [ searchParams, setSearchParams ] | ||
| ); | ||
|
|
||
| return ( | ||
| <DashboardSearchParamsProvider value={ value }>{ children }</DashboardSearchParamsProvider> | ||
| ); | ||
| } | ||
|
|
||
| /** | ||
| * Provider for standard wp-route contexts (uses router hooks). | ||
| * | ||
| * @param props - Props. | ||
| * @param props.from - Route id to bind `useSearch` / `useNavigate` to. | ||
| * @param props.children - Children. | ||
| * @return - JSX element. | ||
| */ | ||
| function WpRouteSearchParamsProvider( { from, children }: Props ): JSX.Element { | ||
| // `useSearch()` re-renders when the router search changes. We'll adapt it to URLSearchParams | ||
| // so shared dashboard code can keep using the URLSearchParams API. | ||
| const search = useSearch( { | ||
| // In this build, the router type isn't registered, so `@wordpress/route` models `from` as `never`. | ||
| // We still want to pass the runtime route id through. | ||
| from: from as unknown as never, | ||
| strict: false, | ||
| } ); | ||
| const navigate = useNavigate(); | ||
| const searchParams = useMemo( () => searchRecordToUrlSearchParams( search ), [ search ] ); | ||
|
|
||
| const setSearchParams: SetDashboardSearchParams = useCallback( | ||
| next => { | ||
| const resolved = typeof next === 'function' ? next( searchParams ) : next; | ||
| const nextSearch = urlSearchParamsToSearchRecord( resolved, { | ||
| arrayKeys: new Set( [ 'responseIds' ] ), | ||
| } ); | ||
|
|
||
| type NavigateArg = Parameters< typeof navigate >[ 0 ]; | ||
| type SearchOption = NavigateArg extends { search?: infer S } ? S : never; | ||
| type SearchReducer = Exclude< SearchOption, true | undefined >; | ||
|
|
||
| // In `@wordpress/route`, `search` is modeled as a reducer function, but without a registered | ||
| // router type it ends up as a very "loose" signature. We still pass a fully-formed object. | ||
| const replaceSearch = ( () => nextSearch ) as unknown as SearchReducer; | ||
|
|
||
| navigate( { search: replaceSearch } ); | ||
| }, | ||
| [ navigate, searchParams ] | ||
| ); | ||
|
|
||
| const value = useMemo( | ||
| () => [ searchParams, setSearchParams ] as DashboardSearchParamsTuple, | ||
| [ searchParams, setSearchParams ] | ||
| ); | ||
|
|
||
| return ( | ||
| <DashboardSearchParamsProvider value={ value }>{ children }</DashboardSearchParamsProvider> | ||
| ); | ||
| } | ||
|
|
||
| /** | ||
| * Provider for the dashboard search params. | ||
| * Automatically detects context and uses appropriate implementation. | ||
| * | ||
| * @param props - Props. | ||
| * @param props.from - Route id to bind `useSearch` / `useNavigate` to. | ||
| * @param props.children - Children. | ||
| * @return - JSX element. | ||
| */ | ||
| export default function WpRouteDashboardSearchParamsProvider( { | ||
| from, | ||
| children, | ||
| }: Props ): JSX.Element { | ||
| // Check context once at mount - this determines which provider to use | ||
| const [ isExternal ] = useState( isExternalAdminContext ); | ||
|
|
||
| if ( isExternal ) { | ||
| return <ExternalAdminSearchParamsProvider>{ children }</ExternalAdminSearchParamsProvider>; | ||
| } | ||
|
|
||
| return <WpRouteSearchParamsProvider from={ from }>{ children }</WpRouteSearchParamsProvider>; | ||
| } |
There was a problem hiding this comment.
The new router infrastructure introduced in this PR lacks test coverage. Key functions like parseExternalAdminSearchParams, searchRecordToUrlSearchParams, urlSearchParamsToSearchRecord, and the provider components themselves have no automated tests. Given the complexity of URL parsing, JSON array handling, and the different code paths for external vs. standard admin contexts, this creates a significant risk of regressions.
This reverts commit 48b43f5.
- Detect external admin context via ?p= parameter presence - Parse search params embedded inside ?p= value - Handle JSON-encoded arrays from TanStack Router - Update Inspector to use search params context - Support URL updates via history API for external contexts
lezama
force-pushed
the
fix/forms-external-admin-search-params
branch
from
3db680a to
0554e41
Compare
| for ( const id of ids ) { | ||
| nextParams.append( 'responseIds', String( id ) ); | ||
| } |
There was a problem hiding this comment.
Mixing JSON-encoded array format (line 261) with individual append calls (lines 857-859) for the same parameter creates an inconsistency in how responseIds are encoded in URLs. The onChangeSelection callback uses JSON format, but the "View" action appends individual values. This inconsistency could lead to confusion and makes the URL handling harder to maintain. Consider standardizing on one approach throughout the codebase.
| for ( const id of ids ) { | |
| nextParams.append( 'responseIds', String( id ) ); | |
| } | |
| nextParams.set( 'responseIds', JSON.stringify( ids ) ); |
| if ( pValue ) { | ||
| // Update params inside the p parameter | ||
| const pUrl = new URL( pValue, window.location.origin ); |
There was a problem hiding this comment.
Similar to line 48, the URL constructor on line 198 can throw an error if pValue contains an invalid URL format. This could cause the search params update to fail silently or crash. Consider wrapping this in a try-catch block to handle potential URL parsing errors gracefully.
| if ( pValue ) { | |
| // Update params inside the p parameter | |
| const pUrl = new URL( pValue, window.location.origin ); | |
| let pUrl: URL | null = null; | |
| if ( pValue ) { | |
| // Update params inside the p parameter | |
| try { | |
| pUrl = new URL( pValue, window.location.origin ); | |
| } catch { | |
| // If pValue is not a valid URL, fall back to updating direct URL params. | |
| pUrl = null; | |
| } | |
| } | |
| if ( pUrl ) { |
| const pUrl = new URL( pValue, window.location.origin ); | ||
| const newPath = pUrl.pathname.replace( /\/[^/?#]+$/, `/${ newView }` ); | ||
| url.searchParams.set( 'p', newPath + pUrl.search ); | ||
| window.history.pushState( {}, '', url.toString() ); | ||
| window.location.reload(); |
There was a problem hiding this comment.
Similar to the URL parsing issues in the search params provider, the URL constructor on line 979 can throw an error if pValue contains an invalid URL format. Consider wrapping this in a try-catch block to prevent potential runtime errors during tab navigation.
| const pUrl = new URL( pValue, window.location.origin ); | |
| const newPath = pUrl.pathname.replace( /\/[^/?#]+$/, `/${ newView }` ); | |
| url.searchParams.set( 'p', newPath + pUrl.search ); | |
| window.history.pushState( {}, '', url.toString() ); | |
| window.location.reload(); | |
| try { | |
| const pUrl = new URL( pValue, window.location.origin ); | |
| const newPath = pUrl.pathname.replace( /\/[^/?#]+$/, `/${ newView }` ); | |
| url.searchParams.set( 'p', newPath + pUrl.search ); | |
| window.history.pushState( {}, '', url.toString() ); | |
| window.location.reload(); | |
| } catch ( error ) { | |
| // Guard against invalid URL formats in the "p" parameter. | |
| } |
| function parseExternalAdminSearchParams(): URLSearchParams { | ||
| const url = new URL( window.location.href ); | ||
| const result = new URLSearchParams(); | ||
|
|
||
| // First, check direct URL params | ||
| let rawResponseIds = url.searchParams.getAll( 'responseIds' ); | ||
| let search = url.searchParams.get( 'search' ); | ||
|
|
||
| // If not found, check inside the p parameter (external admin context) | ||
| if ( rawResponseIds.length === 0 ) { | ||
| const pValue = url.searchParams.get( 'p' ); | ||
| if ( pValue && pValue.includes( '?' ) ) { | ||
| const pUrl = new URL( pValue, window.location.origin ); | ||
| rawResponseIds = pUrl.searchParams.getAll( 'responseIds' ); | ||
| if ( ! search ) { | ||
| search = pUrl.searchParams.get( 'search' ); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Parse responseIds, handling JSON-encoded arrays from TanStack Router | ||
| for ( const raw of rawResponseIds ) { | ||
| if ( raw.startsWith( '[' ) ) { | ||
| try { | ||
| const parsed = JSON.parse( raw ); | ||
| if ( Array.isArray( parsed ) ) { | ||
| for ( const id of parsed ) { | ||
| result.append( 'responseIds', String( id ) ); | ||
| } | ||
| continue; | ||
| } | ||
| } catch { | ||
| // Not valid JSON, treat as regular value | ||
| } | ||
| } | ||
| result.append( 'responseIds', raw ); | ||
| } | ||
|
|
||
| if ( search ) { | ||
| result.set( 'search', search ); | ||
| } | ||
|
|
||
| return result; | ||
| } |
There was a problem hiding this comment.
The new URL parsing logic in parseExternalAdminSearchParams and the external admin provider implementation lack test coverage. Given the complexity of the URL parsing (handling JSON arrays, nested parameters in ?p= value) and the importance of this functionality for external admin contexts, adding tests would help ensure the code works correctly and prevent regressions. The repository has comprehensive testing for similar dashboard components, as seen in tests/js/dashboard/components/.
| Significance: patch | ||
| Type: fixed | ||
| Comment: Add external admin context support to search params provider for compatibility with custom admin shells |
There was a problem hiding this comment.
The changelog entry has the type "fixed" but the comment says "Add external admin context support". According to the custom coding guidelines (CodingGuidelineID: 1000000), changelog entries should "describe the change from a user's perspective". For a feature addition like this, the type should be "added" and it should not be marked as a comment. Consider changing to type "added" or "changed", removing the "Comment:" prefix, and providing a user-facing description.
| // Standard wp-route navigation - need to use window.location for now | ||
| // since we don't have access to the navigate function in external context | ||
| const basePath = getPath(); | ||
| window.location.href = `${ basePath }/${ newView }`; |
There was a problem hiding this comment.
The standard wp-route navigation path on line 989 uses window.location.href which causes a full page reload, similar to the external admin path. The comment on line 986-987 suggests this is a temporary solution, but both code paths result in page reloads for tab changes. Consider whether this is the intended behavior, as it will cause a disruptive user experience and lose any unsaved state.
The "Create a form" button in the wp-build dashboard was not working in external admin contexts because it relied on `window.ajaxurl` (which is not defined outside wp-admin) and used relative URLs like `post-new.php?post_type=jetpack_form`. This fix adds `adminUrl` and `ajaxUrl` to the Forms config endpoint and updates the `useCreateForm` hook to use these config values, falling back to the previous behavior for backwards compatibility.
- Fix isEmpty implementation to handle all empty value types - Add try-catch error handling for URL parsing in search params provider - Remove duplicate utils in inspector - use shared utils.ts - Standardize responseIds format to use individual appends - Use custom event for tab navigation instead of page reload 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
|
|
||
| const response = await fetch( window.ajaxurl, { method: 'POST', body: data } ); | ||
| // Fall back to window.ajaxurl for backwards compatibility. | ||
| const fetchUrl = ajaxUrl || window.ajaxurl; |
There was a problem hiding this comment.
The code references window.ajaxurl, but this property is not typed in the TypeScript declarations. This could lead to type errors in strict TypeScript environments. Consider adding a type declaration for window.ajaxurl in the declarations.d.ts file or casting it appropriately.
| @@ -0,0 +1,3 @@ | |||
| Significance: patch | |||
| Type: fixed | |||
| Comment: Add external admin context support to search params provider for compatibility with custom admin shells | |||
There was a problem hiding this comment.
This changelog entry does not follow the proper format. According to the coding guidelines, changelog entries should start with a capital letter and end with a period. The entry should be rewritten to: "Add external admin context support to search params provider for compatibility with custom admin shells."
| @@ -0,0 +1,3 @@ | |||
| Significance: patch | |||
| Type: fixed | |||
| Comment: Fix "Create a form" button in external admin contexts by using config-provided URLs instead of relying on window.ajaxurl and relative paths. | |||
There was a problem hiding this comment.
This changelog entry does not follow the proper format. According to the coding guidelines, changelog entries should start with a capital letter and end with a period. The entry should be rewritten to: "Fix create form button in external admin contexts by using config-provided URLs instead of relying on window.ajaxurl and relative paths."
| /** | ||
| * Custom event name for URL changes within the dashboard. | ||
| * Uses a custom event to avoid conflicts with browser navigation popstate events. | ||
| */ | ||
| export const DASHBOARD_URL_CHANGE_EVENT = 'jetpack-forms-url-change'; | ||
|
|
||
| /** | ||
| * Pattern to extract view from URL pathname. | ||
| * Matches paths like /responses/inbox, /marketing/forms/spam, /forms/trash | ||
| */ | ||
| export const VIEW_PATTERN = /(?:\/responses\/|\/marketing\/forms\/|\/forms\/)([^/?#]+)/; | ||
|
|
||
| /** | ||
| * Check if we're in an external admin context (e.g., params embedded in ?p= parameter). | ||
| * | ||
| * @return True if in external admin context. | ||
| */ | ||
| export function isExternalAdminContext(): boolean { | ||
| const url = new URL( window.location.href ); | ||
| return url.searchParams.has( 'p' ); | ||
| } | ||
|
|
||
| /** | ||
| * Get effective pathname, checking for external admin ?p= parameter. | ||
| * | ||
| * @return The effective pathname. | ||
| */ | ||
| export function getEffectivePathname(): string { | ||
| const url = new URL( window.location.href ); | ||
| return url.searchParams.get( 'p' ) || url.pathname; | ||
| } | ||
|
|
||
| /** | ||
| * Extract view parameter from URL pathname. | ||
| * | ||
| * @return The view parameter (inbox, spam, or trash). | ||
| */ | ||
| export function getViewFromUrl(): string { | ||
| const pathname = getEffectivePathname(); | ||
| const match = pathname.match( VIEW_PATTERN ); | ||
| return match?.[ 1 ] || 'inbox'; | ||
| } |
There was a problem hiding this comment.
This new utility module introduces several functions (isExternalAdminContext, getEffectivePathname, getViewFromUrl) that handle URL parsing for external admin contexts. The project has test infrastructure for JavaScript/TypeScript code (tests/js/), and similar utility functions in the codebase are tested. Consider adding unit tests for these utility functions to ensure they correctly handle edge cases like malformed URLs, missing parameters, and different URL patterns.
| const basePath = getPath(); | ||
| window.location.href = `${ basePath }/${ newView }`; |
There was a problem hiding this comment.
The getPath function is being called without arguments, but according to its definition in src/dashboard/inbox/utils.js, it expects an item parameter with an entry_permalink property. This will cause a runtime error. It appears the code should use getEffectivePathname from the utils module to get the current pathname, or construct the base path differently. Consider using window.location.pathname to get the base path and then manipulating it to change the view segment.
| * @return {boolean} True if empty, false otherwise. | ||
| */ | ||
| function isEmpty( value: unknown ): boolean { | ||
| if ( value == null ) { |
There was a problem hiding this comment.
This guard always evaluates to false.
lezama
force-pushed
the
fix/forms-external-admin-search-params
branch
from
2aeeced to
01171a1
Compare
|
This PR has been marked as stale. This happened because:
If this PR is still useful, please do a [trunk merge or rebase](https://github.com/Automattic/jetpack/blob/trunk/docs/git-workflow.md#keeping-your-branch-up-to-date) and otherwise make sure it's up to date and has clear testing instructions. You may also want to ping possible reviewers in case they've forgotten about it. Please close this PR if you think it's not valid anymore — if you do, please add a brief explanation. If the PR is not updated (or at least commented on) in another month, it will be automatically closed. |
|
This PR has been automatically closed as it has not been updated in some time. If you want to resume work on the PR, feel free to restore the branch and reopen the PR. |
Summary
Extends Douglas's search params provider (#46695) to work in external admin contexts (e.g., custom admin shells that embed routes via
?p=parameter).Changes:
?p=parameter presence?p=value["61416"])Context
External admin shells may embed routes like:
The standard
@wordpress/routehooks can't parse params from inside the?p=value. This PR adds fallback URL parsing that handles this pattern.Depends on: #46695
Test plan
?p=routing