Retry all selectors for combined widgets in ReportError state.

[techanvil]

Feature Description

When the widgets in a widget area are combined due to all being in the ReportError state, it's necessary to individually retry for each of the errors, which is a bit of a confusing user experience as can be seen in the screen capture below.

It would be a UX improvement if the combined Retry button would retry all of the underlying errored selectors.

The Tweak Chrome extension is being used here to simulate an error for all Analytics report requests.

combined-report-error.mp4

---

Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance criteria

• On the DashboardAllTrafficWidgetGA4, if multiple errors occur, all retryable error should be retried, when clicking the "Retry" button on the combined error message.

Implementation Brief

• Update assets/js/modules/analytics/components/dashboard/DashboardAllTrafficWidgetGA4/index.js grouping the retryable errors from each section of the widget:

	const retryableErrors = [
		pieChartError,
		totalUsersError,
		userCountGraphError,
	].filter( Boolean );

• Update assets/js/modules/analytics/components/dashboard/DashboardAllTrafficWidgetGA4/index.js to pass the new grouped errors to the errors prop of the WidgetReportError.

Test Coverage

• No additional coverage.

QA Brief

• Follow the instructions in the ticket description/video and confirm that retrying the Dashboard All Traffic Widget error retries all reports for that widget.
  Note: since the issue was reported the widget has been updated to use the analytics-4 module so the URL pattern in the tweak extension should be updated to: .*/wp-json/google-site-kit/v1/modules/analytics-4/data/report?.*

Changelog entry

• Improve "Retry" behavior on All Traffic Widget.

[tofumatt]

ACs here are good 👍🏻

[bethanylang]

@hussain-t Are you planning to work on this soon? If not, can you please unassign yourself so someone else can pick up? Thank you!

[hussain-t]

@bethanylang, I'll work on it this week. Thanks!

[benbowler]

@hussain-t could I pick this up this week also? I'm looking for a number of IBs while a lot of the EB is blocked?

[hussain-t]

@benbowler, sure 👍

[eugene-manuilov]

@benbowler, I think that suggested approach in IB is slightly off from what we need here. If we dispatch that action, we will invalidate all getReport selectors even those that haven't failed. I think we need a better approach here. I have to ideas of the top of my head to solve this problem, but all of them are not perfect:

1. Implement special selectors for widgets that we can invalidate all together instead of invalidating all getReport selects. For example, we can introduce getAllTrafficWidgetReport( options ) that will call getReport( options ) in its resolver and if we get an error and user clicks on the retry button, then we will invalidate all getAllTrafficWidgetReport calls.
2. Update the ReportErrorActions component to use a new property options that accepts report options that we want to invalidate and invalidate only those reports which options we pass via that property.

cc @techanvil @tofumatt @aaemnnosttv

[benbowler]

@eugene-manuilov as we store all of the errors in the store, how about an action like retryFailedReports that, when dispatched, loops through all of the selectors that failed and invalidates them?

[eugene-manuilov]

@benbowler that will affect all of unrelated reports as well. Let's say there are errors for the All traffic widget and popular pages one. If we go with your retryFailedReports selector, then the failed report for the popular pages will also be automatically retried when a user clicks on the retry button in the All traffic widget, which is not great, right?

[benbowler]

@eugene-manuilov yes I see what you mean. I'll draft a new IB shortly based on either of your approaches.

[techanvil]

Hi @benbowler, just chipping in to say I think it would be preferable to take the approach of passing in options or a set of selector names and args to ReportErrorActions via WidgetReportError / ReportError, rather than the per-widget selector approach, as this would be more generic and reusable. We could then reuse this for the combined error states for the WP Dashboard / Admin Bar that are being discussed on #6377.

[eugene-manuilov]

... just chipping in to say I think it would be preferable to take the approach of passing in options or a set of selector names and args to ReportErrorActions via WidgetReportError / ReportError, rather than the per-widget selector approach, as this would be more generic and reusable.

@techanvil, this basically means that in the All traffic widget when we render the report error component in the pie chart component, we need to pass options that we use for the users graph as well, and for the report error component in the user graph component we need to pass report options that we use for the pie chart component, right? This makes us need to cross reference report options in different components which makes more mess and reduces maintainability. Just imagine if we need to cross reference report options for 3 or more report errors...

[techanvil]

@eugene-manuilov, in practical terms I think it would just mean passing an array of errors into WidgetReportError as ReportErrorActions looks up the selector name and args from the error object(s)...

Actually, looking into this has made me realise we should reconsider this issue a bit.

First off, I should point out that when I initially created the issue, I made a mistake in how I described it. It's not the case that "the widgets in a widget area are combined due to all being in the ReportError state" - that suggests our widget combining logic is in play, when it's not - ReportError is not one of the SPECIAL_WIDGET_STATES that allows it to be combined. Plus - the All Traffic widget is simply that - one widget, not a widget area. Looks like I managed to confuse myself while raising this one! So, sorry for the confusion.

Anyhow, rather than combining anything, we are simply rendering a WidgetReportError at the top level of each of the widgets mentioned in the AC when we see the first error that we want to handle.

site-kit-wp/assets/js/modules/analytics/components/dashboard/DashboardAllTrafficWidgetGA4/index.js

Lines 301 to 310 in ab9d76b

	if ( pieChartError ) {
	return (
	<Widget>
	<WidgetReportError
	moduleSlug="analytics-4"
	error={ pieChartError }
	/>
	</Widget>
	);
	}

site-kit-wp/assets/js/modules/search-console/components/dashboard/SearchFunnelWidgetGA4/index.js

Lines 309 to 318 in ab9d76b

	if ( searchConsoleError ) {
	return (
	<Widget Header={ Header } Footer={ WidgetFooter }>
	<WidgetReportError
	moduleSlug="search-console"
	error={ searchConsoleError }
	/>
	</Widget>
	);
	}

So, with the widget errors clearly not being in a combined state, if we take the approach of retrying all of the widget's report errors while only showing one of the errors, we can end up not showing some of the errors to the user, which is not really the intention here.

Note that ReportError is already designed to handle multiple errors, deduplicating them when duplicates are present:

site-kit-wp/assets/js/components/ReportError.js

Lines 92 to 101 in ab9d76b

	const uniqueErrors = uniqWith(
	errors.map( ( err ) => ( {
	...err,
	message: getMessage( err ),
	reconnectURL: err.data?.reconnectURL,
	} ) ),
	( errorA, errorB ) =>
	errorA.message === errorB.message &&
	errorA.reconnectURL === errorB.reconnectURL
	);

Its child ReportErrorActions will retry multiple errors:

site-kit-wp/assets/js/components/ReportErrorActions.js

Lines 103 to 113 in ab9d76b

	const handleRetry = useCallback( () => {
	retryableErrors.forEach( ( err ) => {
	const { selectorData } = err;
	dispatch( selectorData.storeName ).invalidateResolution(
	selectorData.name,
	selectorData.args
	);
	} );
	onRetry?.();
	}, [ dispatch, retryableErrors, onRetry ] );

With that in mind, I think the thing to do here for the All Traffic widget is to pass all of the errors into WidgetReportError at that top level render. This will allow ReportError to show and retry multiple errors as currently designed, deduping as necessary.

diff --git a/assets/js/modules/analytics/components/dashboard/DashboardAllTrafficWidgetGA4/index.js b/assets/js/modules/analytics/components/dashboard/DashboardAllTrafficWidgetGA4/index.js
index 7e20a3d3c9..8e7338cb6f 100644
--- a/assets/js/modules/analytics/components/dashboard/DashboardAllTrafficWidgetGA4/index.js
+++ b/assets/js/modules/analytics/components/dashboard/DashboardAllTrafficWidgetGA4/index.js
@@ -298,12 +298,18 @@ function DashboardAllTrafficWidgetGA4( props ) {
 		setValue,
 	] );
 
+	const retryableErrors = [
+		pieChartError,
+		totalUsersError,
+		userCountGraphError,
+	].filter( Boolean );
+
 	if ( pieChartError ) {
 		return (
 			<Widget>
 				<WidgetReportError
 					moduleSlug="analytics-4"
-					error={ pieChartError }
+					error={ retryableErrors }
 				/>
 			</Widget>
 		);

As for the Search Funnel widget, I don't think we should do anything here at this point, because the top level error is for the Search Console module, while the error shown in the Overview is for the Analytics module, and we don't currently have a design or pattern for showing errors for multiple modules at the same time. Maybe this could be something to address as a separate issue.

site-kit-wp/assets/js/modules/search-console/components/dashboard/SearchFunnelWidgetGA4/Overview/OptionalCells.js

Lines 85 to 92 in ab9d76b

	error && (
	<Cell { ...halfCellProps }>
	<WidgetReportError
	moduleSlug="analytics-4"
	error={ error }
	/>
	</Cell>
	) }

What do you think?

[eugene-manuilov]

Thanks for explaining this, @techanvil. Yes, I think we can go with what you suggested. @benbowler, could you please update your IB?

[benbowler]

IB updated.

[techanvil]

Thanks @benbowler, the IB LGTM ✅

[mohitwp]

QA Update ✅

• Tested on dev environment.
• Verified that On the DashboardAllTrafficWidgetGA4, if multiple errors occur, all retryable error gets retried, when clicking the "Retry" button on the combined error message.

I noticed the another issue and created a separate ticket for that #8434

Recording.837.mp4