Prevent dashboard from crashing when async SVG fails to load

[jamesozzie]

Bug Description

There have been a couple of users who reported a Loading chunk 33 failed dashboard error, both since Site Kit 1.79.1.

Loading chunk 33 failed.
(error: http://domainname.com/wp-content/plugins/google-site-kit/dist/assets/js/33-a742fa632cd94e4fa105.js)
in Unknown
in LazyContentSVG
in Suspense
in ContentSVG
in div
in div
in div
in Cell
in div
in ForwardRef
in ForwardRef
in ContentAutoUpdate
in div
in ForwardRef
in section
in AdSenseConnectCTA
in div
in div
in Widget
in WithWidgetSlug(Widget)
in AdSenseConnectCTAWidget
in WidgetRenderer
in div
in Cell
in WidgetCellWrapper
in div
in ForwardRef
in div
in div
in ForwardRef
in WidgetAreaRenderer
in div
in WidgetContextRenderer
in DashboardMainApp
in DashboardEntryPoint
in RestoreSnapshots
in ErrorHandler
in StrictMode
in Root

Additional Context

• SK 1.79.1
• No SH info so far
• One site does seem to have mismatching URLs, evident from checking their public REST endpoints
• Awaiting further details

Impacted sites

• https://wordpress.org/support/topic/loading-chunk-33-failed/ | Open | No SH info
• https://wordpress.org/support/topic/site-kit-encountered-an-error-74/ | Open | No SH info
• https://wordpress.org/support/topic/site-kit-encountered-an-error-75/ | Open | SH info

---

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

Acceptance criteria

• Lazy-loaded graphics should be loaded in a common way that prevents a failed import from crashing the dashboard
  • In this case, a suitable light-weight non-lazy-loaded fallback should be rendered in its place. For now this can simply be an error message styled similar to use in other places:

  Error: Failed to load graphic.

  • This should be an edge case so we don't need to make it a polished experience.

Implementation Brief

Test Coverage

QA Brief

Changelog entry

[jamesozzie]

One more report of this today. One similarity to another impacted user is that this user has their site as https, yet their REST endpoints indicate both site references as http. Awaiting further details from correspondence with user.

[jamesozzie]

I've been trying to recreate this based on the reports we have so far, although with no luck at this point. Internal testing task.

We have no Site Health information reports yet, and my testing was based on mixed content, and mismatching URLs. Awaiting further insights from impacted users.

[jamesozzie]

With one users SH info now provided I set up a site with the same plugins and theme active. In my case I wasn't able to recreate.

From reviewing both site URLs which we now have it looks like both are using CDNs, one Cloudflare and the other Amazon's Litesail. Awaiting further insights

[jamesozzie]

One user reported that this issue no longer occurs once both their site URLs are defined as https. With one other impacted user having mismatching URLs I suspect this may also be the cause for that user, who didn't yet respond to our last query.

With one other impacted user making use of a free hosting provider that did result in setup issues based on previous resting at support level we'll keep this open for another few days to check for further reports.

[jamesozzie]

Another user reported the same error this week. As with other impacted users, this seems to come from mismatching URLs defined within their WordPress settings (site address and WordPress address). One may be https, the other http, or a site served a secure, but with the site URLs referencing http only.

Testing performed to try and recreate based on the same scenario - a secure site with their site URLs as http.

For those impacted by this error, check your WordPress general settings, to ensure your site URLs are correct.

Keeping this open for the moment, to discuss or test further.

[mxbclang]

@aaemnnosttv Tagging you in here as we discussed this in today's support/eng sync. Let's see if we can make this a clearer message for users.

[aaemnnosttv]

After taking a look here, I think we can make some small changes to make for a better experience in such a scenario. Moving forward to AC 👍

[eugene-manuilov]

• Using assets/js/components/ErrorHandler/index.js,

  • Add a new optional prop errorMessage which will be rendered if provided in place of the current rendered markup (Notification).
  • The errorMessage should be wrapped within a p tag, with the translated Error: string prepended to errorMessage. It should be styled using the same color as for error messages. (#7a1e00)

• Using assets/js/modules/adsense/components/common/AdSenseConnectCTA/ContentSVG.js,

  • Wrap the existing components within ErrorHandler with errorMessage prop set as per the AC.

@asvinb I think we shouldn't modify the ErrorHandler component because it is intended for another thing. What we need to do is to create a new error boundary specifically for the ContentSVG component that will display the text from AC.

Link to the React documentation: https://reactjs.org/docs/code-splitting.html#error-boundaries

[asvinb]

@eugene-manuilov I initially thought about the same thing but looking at the ErrorHandler component, it's mostly what we need, except that we don't want to render the Notification component but render a custom message. On top of that we already have tracking, what's why i was more inclined to reuse the existing ErrorHandler with the added flexibility of displaying a custom error message. Happy to hear your thoughts about it, but happy to have a separate component as well :)

[eugene-manuilov]

@asvinb, yes, I understand your intention, but I think we should still use a separate component and not change the ErrorHandler component to avoid making even more changes there in the future if we decide to add a fallback image or anything else to the error boundary for svg images.

[asvinb]

Thanks @eugene-manuilov ! IB updated!

[eugene-manuilov]

Thanks, @asvinb. All looks good now, except we don't need tracking for the new error handler. I have removed it from IB.

IB ✔️.

[aaemnnosttv]

⚠️

Looking at the PR that was merged, the solution is incomplete as only one component was updated. There are still other lazy-loaded graphics which should be updated. The ones for TwG will be removed soon so we don't need to touch those. This leaves the one for Idea Hub here: https://github.com/google/site-kit-wp/blob/82474ec5796066b76a2ef281882304a4603d7fef/assets/js/modules/idea-hub/components/common/IdeaHubPromptSVG.js#L28-L30

Secondarily, the idea here was that this would introduce a common component/api for rendering lazy graphics, which would make it easier to make sure this isn't missed in the future. This wasn't very explicit in the requirements about how that should happen so perhaps it may have been a bit too loose 😄 We can iterate on this with a follow-up issue, but for now we will need at least one follow up PR to address the missed usage above.

[hussain-t]

@aaemnnosttv @nfmohit I have created a follow-up PR that wraps the LazyIdeaHubPromptSVG with the MediaErrorHandler component.

[wpdarren]

QA Update: ⚠️

@nfmohit just one question.

While the dashboard loads and the error message in the AC appears, there are a lot of console errors. I am assuming this is expected but did not want to assume. Please could you confirm?

[nfmohit]

@wpdarren This is expected. We are even introducing a new console error for each SVG loading failure.

https://github.com/google/site-kit-wp/blob/585ddc3a885c16550b74d608bce745ec96659cb3/assets/js/components/MediaErrorHandler/index.js#L45

While we want such failures to not crash our application and make it unusable, we would still want relevant console errors to show up to trace such unexpected behaviour.

[wpdarren]

QA Update: ✅

Verified:

Using the development build of Site Kit.

• After making the changes in the QAB:
  • The dashboard doesn't crash.
  • The Adsense Connect CTA widget shows an error according to the ACs for the missing graphics.
  • The Components > MediaErrorHandler Storybook story is correct.
  • The IdeaHub CTA widget shows an error according to the AC for the missing graphics.
  • Confirmed with engineer that console errors are expected.

Screenshots