Error state for Key Metric Widget / Tiles

[jimmymadon]

Feature Description

Key Metric widget tiles are much smaller than our usual widgets and hence require a new error component to be designed as per the Figma mocks.

---

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

Acceptance criteria

• If any of the Key Metric widget tiles throws an error, an error component should be displayed as per this Figma mock.
  • Clicking on the "Retry" button should behave like our existing ReportErrorActions component invalidating the resolution of the selectors that threw an error thereby attempting to resolve those selectors again.
  • Clicking on the "Get Help" link should also behave like our existing ReportErrorActions component and use the getErrorTroubleshootingLinkURL selector.
• If all the Key Metric Widget tiles are throwing an error, then a single larger component should be rendered as per this Figma mock instead of multiple smaller tile errors.
• Any Key Metric widget tiles implemented already should have this functionality added to them. Tiles implemented after this feature is executed will reuse the components created as part of this issue.

(DRAFT) Implementation Brief

Handling Individual Key Metrics Widget Errors

While the existing <WidgetReportError> could in theory be utilised and will correctly render an error message, error description and correct "Retry" handler, it will not display and format the contents in a means that satisfies the Figma design for a single key metric widget error state.

Looking into the <WidgetReportError> component, and ultimately the <ReportError> component (used by the <WidgetReportError> component), the following components should be utilised when creating a new error component for Key Metrics Widgets: <CTA> and <ReportErrorActions>. These two components can be combined within said new error component, via a new wrapper, to be styled as per the Figma design.

1. Create a new <KeyMetricsWidgetError> component, borrowing logic from the <WidgetReportError> component, ultimately making use of <CTA> and <ReportErrorActions> components to render the error title, the retry button and the troubleshooting URL/CTA.

   • Said component should accept only the following props given that no error message needs to be surfaced within the UI:
     • moduleSlug required for the <ReportErrorActions> component to re-attempt data fetching correctly.
   • This component can likely reside at assets/js/googlesitekit/widgets/components/KeyMetricsWidgetError.js
   • Style the components according to the Figma design](https://www.figma.com/file/CdtrHB0HAjfGRWTnTMMQI0/Key-Metrics?type=design&node-id=5409-80974&mode=design&t=NDZafi0d8jEUSTko-0). Use the existing SASS files (and add as necessary) located at assets/sass/components/key-metrics

2. Add a mens to check for error states within each of the Key Metrics Widget components. This can be achieved via the getErrorForSelector method within a useSelect implementation using the MODULES_ANALYTICS_4 module, i.e:

	const error = useSelect( ( select ) =>
		select( MODULES_ANALYTICS_4 ).getErrorForSelector( 'getReport', [
			reportOptions,
		] )
	);

4. Render the <KeyMetricsWidgetError> component as opposed to the <MetricTileNumeric> component if an error is encountered. This will involve conditional rendering within the return statement of the <LoyalVisitorsWidget> component, an shorthand example to demonstrate may be as follows:

return ! error ? (
    <MetricTileNumeric .../>
) : (
    <KeyMetricsWidgetError .../>
);

Handling All Key Metrics Widgets Erroring

TBC

Test Coverage

QA Brief

Changelog entry

[eugene-manuilov]

AC ✔️

[jimmymadon]

@10upsimon Please note the change in the design of the error state for the KMW tile.

[10upsimon]

@jimmymadon I am not sure where I am supposed to reference the new design URL from, as the one I had prior no longer works, instead it takes me to the overall Figma view of all key metrics design components.

The most obvious one I could find was https://www.figma.com/file/CdtrHB0HAjfGRWTnTMMQI0/Key-Metrics?type=design&node-id=5409-80974&mode=design&t=NDZafi0d8jEUSTko-0 and this seems to match what is in the issue, so I assume this is correct?

[10upsimon]

@jimmymadon (cc @aaemnnosttv) I have temporarily renamed "Implementation Brief" to "(DRAFT) Implementation Brief" in the issue as I would like feedback, but I do not consider it complete given that I still need to flesh out the scenario for when all widgets are erroring.

I would, however, appreciate some feedback on this so long, and whether it is too verbose, not verbose enough, too instructions-driven etc. Thank you!

--

The handling of all widgets erroring (point 2 in the AC) has me a little stumped though, and the following points are sticking points for me currently:

• How do we monitor multiple data stores/states for errors? Could this be done by using multiple error checks via multiple uses of getErrorForSelector() within the parent component housing all key metrics widgets?
• Would there be any complications with this given that the amount of key metrics widgets could be dynamic based on the users selection of widgets to show?
• I'm aware that the <ReportErrorActions> component seems to almost magically know what to resolve based on the component in which it is rendered, how could it be instructed to re-fetch data fetching for a myriad of components, or would it simply know this based on the useInViewSelect implementations where the same module is assigned to each that is ultimately assigned to the moduleSlug prop of <ReportErrorActions>?
• Could we potentially update some local state contained within the parent component housing all key metrics widgets each time an error occurs and use this as a means to index errors by available key metrics slugs? This could be a simple means of tracking total metric tiles vs which have errored.
  • I did consider whether the getErrorForSelector within useSelect component could be used within the parent component, but I also think the reportOptions would then need to be housed outside of each key metrics widget component and that seems unnecessarily complicated.
• I have some concerns around the UX/UI implications of the "all widgets erroring" scenario, it would be quite visually jarring to go form 1, 2 or maybe even 3 rows of key metrics widget components to all of a sudden having a single error component that occupies considerably less UI space, do you think this needs to be raised with UX?
• Given that loading states can be resolved at differing times, this too could be a UX consideration, having multiple widgets enter into individual error states, only to then be replaced with the single one as above.

Apologies if these seem more like "thoughts on paper", they mostly are me thinking aloud while considering the potential means of implementing some points in the AC.

[10upsimon]

@jimmymadon @aaemnnosttv I've moved this to IB Review in a bid to get some early feedback on my approach to the IB as a whole. I did not assign either of you directly as I am unsure if that would be considered acceptable in this instance, please do let me know.

[techanvil]

Hi @10upsimon, nice work so far getting stuck in here.

With regard to your approach to the IB, I would say the general level of verbosity is good, however we usually avoid the detail of listing code examples in the IB itself, preferring to keep it at a slightly higher level - although we do take a pragmatic approach where sometimes it's appropriate to refer to a PoC PR if we've put one together while investigating the issue. We tend to try to provide a good level of detail in the IB, highlighting things that will be useful and giving a useful amount of direction to the implementer, without essentially writing a transliteration of the code that will be written.

The rest of this reply doesn't directly follow the structure of your questions, but I felt a more holistic overview of things would be useful while hopefully providing enough information to answer your q's. That said, please let me know if there's anything you would like me to answer more directly.

---

As a bit of background, it's worth me pointing out that at present we do support the concept of combining all widgets within a widget area into a single widget when they all share the same state - "state" here referring to them rendering a generic component for certain scenarios. Currently these states are the ReportZero and RecoverableModules, so e.g. if all widgets in widget area render the ReportZero component (by way of the WidgetReportZero wrapper which is necessary for tracking this state metadata), they will be combined to a single widget rendering ReportZero. The logic for this can be seen in WidgetAreaRenderer and combineWidgets() / shouldCombineAllWidgets() with the list of combinable widget states in SPECIAL_WIDGET_STATES. Further areas of relevance include useWidgetStateEffect() which is used by WidgetReportZero and WidgetRecoverableModules (and some others which relate to widget states which may similarly be combined if they are on the same row) to track the metadata, and the underlying Redux plumbing.

So, this info can help to answer a couple of questions.

• For one, from a UX perspective, this sort of widget-combining is not a new concept for Site Kit, so it doesn't seem contentious in that regard. While on the subject I'd also say, imagine we're in this situation where all the Key Metric tiles are erroring - that would mean twelve four identical visual error states, each with a Retry button which would mean the user would need to click on twelve four buttons to recover the UI. Despite the layout shift going from a single to multiple widgets, it seems to me a better UX to present this error info in a unified manner with just a single Retry button to press. However, the other UX aspects you've raised - potentially going from multiple rows of widgets to a single one, and the related async loading state resolution - could use a bit of clarification. We could manage it by having a single, combined loading state that is visible while any widget is still loading, but this would be problematic in itself with the layout then shifting when the widgets do load, as well as conflicting with the plan for each widget to have an individual loading state as per Loading State for Key Metric Widget Tiles #7158. I'd suggest looping @sigal-teller in here and asking for her feedback from a UX perspective. My feeling is that "all widgets failing" is the edge case and so having this unlikely, unhappy path scenario to be the case where the layout shifts is the right compromise here. I will however touch on how we could implement a combined loading state below.
• Secondly, and more briefly, it essentially gives a solid pointer as to how we can solve the problem of combining the Key Metric widgets when they all are in an error state.

I'll now move onto the more specific details relating to the implementation of this issue.

---

Your initial draft, speccing out the KeyMetricsWidgetError component is heading in the right direction. However, it's not correct to say that it only needs the moduleSlug prop. In order to be able to retry the error(s) for a given widget, at a bare minimum the selector names and args that relate to the errors need to be passed in, in order to invalidate the resolution for the selectors, which will trigger a retry of them; I'd suggest that in fact the error objects as retrieved via getErrorForSelector() should probably be passed in, as these objects include the selector name and args, as well as the error itself which we might still need (note how the error data is checked within ReportErrorActions).

Furthermore, as we're going to need KeyMetricsWidgetError to be one of the SPECIAL_WIDGET_STATES in order to be combinable, and seeing as the moduleSlug will differ across Key Metrics widgets (with data coming from adsense, analytics-4, and search-console according to the Design Doc), we should in fact pass in a prop which is a map of moduleSlug to errors, making this component usable in both the singular and combined case (let's call this moduleErrorMap for the sake of referring to it below).

It's also evident we can't use ReportErrorActions as it stands, as it expects errors for a single moduleSlug. Maybe we could update it to instead take the moduleErrorMap prop, or it might be more straightforward to create a new, similar component for our requirements here.

One more detail is the naming of this error component - we'll need to provide a wrapper to it, so it would be more streamlined to call this something like KeyMetricsReportError, so the wrapper can be WidgetKeyMetricsReportError.

Moving onto this wrapper, it will need to use useWidgetStateEffect() to track the metadata for the widget. One detail that stands out here is that we do need to track a moduleSlug in the metadata as this is used to match the widgets in shouldCombineAllWidgets(). However as mentioned the "real" module slugs will differ across the individual widgets, so I'd suggest here we need to provide a fixed value for moduleSlug for all of the Key Metric widgets, this could just be 'keyMetrics'. Maybe the WidgetKeyMetricsReportError prop for this could be named metaModuleSlug to make it clear it's not a regular module slug. It also occurs to me, we'll need a way to combine the data in moduleErrorMap for all of the widgets so we can pass the full list of error/selector data into the combined widget so it can retry all of them. An initial thought here is we could add a reduceMetadata arg to useWidgetStateEffect() which could be a reducer-style callback we then invoke in combineWidgets() to merge the metadata for the "combine all widgets" case.

Briefly considering the suggestion of having a single, combined loading state for all widgets, one way to achieve this would be to add another "special widget state", say with a new KeyMetricsLoading / WidgetKeyMetricsLoading combination that similarly uses useWidgetStateEffect() to set the metadata, but in shouldCombineAllWidgets() we return true if any of the widgets are in this state. So we could rename the existing SPECIAL_WIDGET_STATES to SPECIAL_WIDGET_STATES_ALL and add a new list, SPECIAL_WIDGET_STATES_ANY to which we add KeyMetricsLoading. Then in every KM widget we render this new WidgetKeyMetricsLoading while it's loading, with the result being if any widget is still loading we see the combined loading widget.

---

A final few thoughts:

It does occur to me I've gone quite far into a fairly specific approach for implementation - we could consider alternatives, for example we might want to manage the list of errors across all KM widgets via some Redux state, instead of via the metadata approach, but I think managing this as the errors change state would add a degree of complexity that would be a bit excessive for the requirements here. I don't really think there's a sensible alternative to using the existing "combine widgets" approach either. At least not without refactoring the widgets to lift the data fetching out of them so as to be manageable in a centralized manner, but again I think think this would probably add more complexity than it's worth and would break the encapsulation we have with each widget managing its own data requirements. We couldn't really do it in the way you've suggested with a sort of container widget while still leveraging our system as it stands where each widget is rendered by a WidgetRenderer, within a generic widget area and context.

That all said - please do feel free to consider other approaches too! If a more straightforward approach to anything I've suggested occurs to you, we should certainly give it a good look.

As mentioned at the top, I hope this answers your questions but I realise it's a lot to unpack so please let me know if anything is unclear and/or you'd like to jump on a call to discuss this.

[10upsimon]

@techanvil thank you so incredibly much for taking the time to explain aspects of the approach in so much detail. Albeit a lot to digest, it gives invaluable context.

I'm going to need to take quite some time digesting what you've said and making sense of what and where certain aspects even are, especially before I am in any position to make any suggestions of an alternative approach.

--

@techanvil @aaemnnosttv I will state that a lot of what is in the reply is out of my wheel house at the moment, and it's going to come with what is likely to be a healthy sum of hours on my end making sense of it in the context of the code base. If either of you feel that this is going to be detrimental to getting this ticket ready for execution in sprint 106, I'd be totally understanding even you suggest we move the IB phase of the issue to someone who is guaranteed to get it ready for timely execution.

[techanvil]

Thanks @10upsimon, very good point to raise. @aaemnnosttv if you land here before Slack, we have a thread there discussing whether Simon should indeed hand the IB over which it would be great to get your input on.

[aaemnnosttv]

Thanks for all the context and good ideas @techanvil and @10upsimon ! The widget API is a bit of a beast to say the least.

I realize AC has already been set here, but I'd suggest that we reduce the scope to the first point only – implementing error states for individual tiles as designed without getting into combining widgets.

It could be that the effort needed to achieve the monolithic error state is more than it would be worth (maybe we should revisit this state with UX?). Considering this state is likely an edge case, I'm not sure it makes sense to invest a large amount of time on it right now.

Generally speaking, I feel any substantial changes to the Widget API should happen in a dedicated issue. Adapting KMW to use it should be the easy part.

Apologies for such a brief reply to your very thoughtful and articulate ideas. It seems we may be trying to do a bit too much in this issue. 30 doesn't seem big enough for this 😄

[10upsimon]

Thank you so much @aaemnnosttv, this certainly sounds like a more sensible approach to me. In terms of updating the AC to be reflective of your comment above, should that be done by the original AC author., @jimmymadon in this case?

[techanvil]

Thanks @aaemnnosttv, this sounds like a good way forward to me too - with a slight twist, my thinking is we should create two new issues, one with the reduced AC, and one with the remaining aspects (the combined error state) to be clarified later. That way we avoid future readers of the cut down version having to wade through all the detail above, much of which would no longer be relevant. We should however link to this issue from the new ones for the historical context.

We've discussed this on Slack, and @jimmymadon is going ahead to create these new issues.

[techanvil]

Closing in favour of #7310 and #7311.