Create `useInViewSelect` hook

[tofumatt]

Feature Description

We should add a new hook named useInViewSelect that triggers a useSelect only when the nearest element in an "In-View Context Provider" (likely a WidgetAreaRenderer) is on-screen.

For more context refer to the section "Lazy-load rendering of widget contexts/areas not in view" in the design doc.

This should rely on the useInView hook created in #4120, specifically using useInView( { resetWhenOffScreen: false } ).

---

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

Acceptance criteria

• There should be a useInViewSelect hook created in assets/js/hooks/useInViewSelect.js.
  • This component should have the exact same signature as WordPress Data's/Site Kit Data's useSelect hook.
  • This hook should use the value of the useInView( { resetWhenOffScreen: false } ) hook to either:
    • Pass the arguments to useInViewSelect(...args) through to useSelect(...args) (if useInView() is true)
    • Pass () => undefined to the useSelect hook if useInView() is false
• The WP dashboard widget should use the new useInViewSelect hook from #4096 for all of its API-based selector calls, replacing useSelect for those.
• The result should be that these API requests are only made when the WP dashboard widget is in or close to the viewport. The best way to test it is to hide the widget entirely via screen options. In that case, no API requests should be made.
  • When testing, please ensure to always clear your browser cache, since API requests are only made when nothing is cached yet.

Implementation Brief

useInViewSelect( mapSelect, deps )

• Create hook in path referenced in AC
• Should use the new useInView introduced in #4120 (not to be confused with the current/old hook from react-intersection-observer which should be removed now) which will return a boolean based on the context provider's element (the widget area) is in view
• The given mapSelect function should be wrapped with useCallback using the given deps since we can't change the length of deps passed to useSelect which is essentially used in the same way, we just do it ahead of time here
• Returns useSelect( mapSelect ) (note no deps passed)
  • If inView returns true, pass the given func as mapSelect with the given deps
  • If inView returns false, pass a function that always returns undefined (this should be defined as a constant rather than an a new function every time to avoid triggering the select unnecessarily)

WP Dashboard

• Update WPDashboardApp to work similar to the changes in WidgetAreaRenderer made in #4120, essentially using useIntersection, and wrapping the WP dashboard widget with an InViewProvider and setting the ref on the div.googlesitekit-wp-dashboard
• Update useSelect hooks in WPDashboardWidgets to use useInViewSelect instead
  • Add a condition to return null if selected values return undefined to prevent nested components from rendering
  • Ensure widget selectors/resolvers only run when widget is in view (see #4001)

Test Coverage

• Add Jest unit tests for useInViewSelect

QA Brief

• Open the developer console in your browser and observe the network requests (specifically XHR/Fetch requests, probably in the "Network" tab). Ensure Analytics is connected.
• Visit the WordPress Dashboard (eg /wp-admin on a site that has Site Kit setup
• Note that no requests to Site Kit's REST API routes (eg. matching /google-site-kit/v1/ are made when at the top of the page
• Scroll down to the bottom of the page
• Note the widgets for Site Kit's box start to show a loading/preview block, then load in their contents. There should also now be XHR/Fetch requests for /google-site-kit/v1/.

Changelog entry

• Add a new useInViewSelect hook that allows to call a specific selector only when in view.

[felixarntz]

@tofumatt Maybe a good idea to actually test-drive this while implementing (rather than just implementing without using it anywhere initially) would be the WP dashboard widget - see https://github.com/google/site-kit-wp/issues/4001#issuecomment-923421368. I wonder if we should just merge these two tickets.

The main load is really implementing this hook, using it in WP dashboard widget would just be a matter of replacing useSelects, and by doing that here we would be able to know that it actually works.

[tofumatt]

Fair enough, we can test-drive it there for sure. I'd be happy to include replacing the usage inside the WP Dashboard as part of this issue though 👍🏻

[felixarntz]

@tofumatt Let's do that. Once that's part of the ACs here, you can close #4001 with a reference to here. We should then explicitly include that bit in the changelog here as well, maybe even change the issue title to "Create useInViewSelect hook and use it in WP dashboard widget".

[aaemnnosttv]

@tofumatt I think we need to update the ACs here which don't include the relevant changes to @felixarntz 's https://github.com/google/site-kit-wp/issues/4096#issuecomment-926885002 above (I've included in the IB though).

The hook should return either true or false—false if the parent component/context has NEVER been on-screen, but true as soon as it has been visible even once.

• If the component appears on-screen, then is scrolled out-of-screen, the useInView hook should still return true.

I don't recall why we wanted this but I think we should avoid introducing it as it makes things quite a bit more complicated. The part I remember is that we needed a reset to be avoid the mass select that would be triggered on the dashboard (if everything had been in view once) when changing the date range. However, if we leave it as-is and don't implement in-view means "was ever in view", then this is no longer a concern because any useInViewSelect uses which were not in view would not trigger selects again. These would likely be back in their loading state though which shouldn't really matter if they're out of view as long as it doesn't result in a layout shift. As soon as they came back into view, they should of course render as before, or load/request new data if needed.

I've added the current IB without this which seems like a good place to start. If needed, I suggest we explore enhancing the behavior in a follow-up issue.

Back to @tofumatt for revising the ACs but otherwise I think this is ready for IBR 👍

[felixarntz]

@aaemnnosttv

I don't recall why we wanted this but I think we should avoid introducing it as it makes things quite a bit more complicated. The part I remember is that we needed a reset to be avoid the mass select that would be triggered on the dashboard (if everything had been in view once) when changing the date range.

The reason is that it will likely result in some clunky UX, I see specifically the following two problems if we don't do it like that:

• If the user scrolls very fast, it is unavoidable to sometimes still run into a loading state because the scroll threshold cannot account for any scroll behavior. This is to be expected on page load, or when data changes, but not once everything has loaded. Imagine this:
  • User goes to the dashboard.
  • User scrolls super fast to a widget further down. They see some loading states, which is fine, and eventually all widgets up to the point where they're scrolled to are loaded.
  • They now scroll back up super fast. They will now briefly (as in almost just "flashing") see loading states again, which is weird because there is actually nothing to load.
• If we visually "unload" widgets that were already loaded just because they are scrolled out of sight, in addition to the above problem it could lead to a notably worse impact of any layout shifts. While we should avoid them in general, it is sometimes hard in practice, and while a layout shift on load is a common problem, it would become a lot worse if we "unload" widgets again that are scrolled past.

[tofumatt]

@felixarntz covered it exactly—we don't want to be removing fully-rendered and loaded widgets/elements from the DOM entirely when they're scrolled out-of-view.

In the case of Google Charts like the Analytics ones, for instance, it's actually quite expensive to create those charts—they have a lot of event handlers, SVG/canvas computations/drawing, etc. Once they're rendered they don't consume too many resources, but loading/unloading them is quite expensive.

Doing this anytime they scroll out-of-view would be wasteful and result in a lot of needless DOM manipulation/Google Charts work.

So we want to include that "once in-view-once always return true" to this hook—I think it's a vital part of how it works.

[tofumatt]

I've kept the ACs as-discussed, and fleshed out the "reset" mechanism a bit more. Originally @aaemnnosttv and I discussed it and I wasn't sure about storing that "has been in view" state in the datastore, but upon further consideration I think it's the best, most flexible, and most straightforward approach. I've raised the estimate to account for this more fleshed-out implementation of the reset and its tests.

I think it's best to include those reset actions in this issue so we can test/build it more holistically. Happy to take this one on as I have an idea of how to approach it already 🙂

[aaemnnosttv]

Thanks @tofumatt!

• There should be a mechanism to "reset" the useInViewSelect state for any hook that has had its useInView hook return true at least once. The best way to implement this would be to have each useInViewSelect hook create its own instanceID and store the "useInView() has returned true at least once since reset" flag for its instance ID. All of these instance IDs that have returned true once should be stored in an array in the CORE_UI datastore. This way we can easily reset all hooks by clearing this array when the UI should be "reset".

I'm not sure we need to build additional datastore infra to accommodate the reset functionality we want. Also, I don't think the has-been-in-view state should be unique to each useInViewSelect because the in-view state is provided by the higher context, so it would make more sense to manage this in a more centralized way.

I think we can encapsulate this in the WidgetAreaRenderer building on the foundation in #4120 like so:

• Store the inView value that we would provide via context and store it in a state variable
• Use an effect that handles the mutation of setting inView state to true (as in ever in view once) only if true, using the inView intersection observer entry value as a dependency of that effect (that way when it later became false, the state would remain true)
• Pass the inView from state as the in view context value
• Now, we can simply add another useEffect that's based on the current date range as its only dependency – when that changes, it should set the state-managed inView value to the real current value from the I.O. entry (essentially resetting it, but without forcing a re-render if the value hasn't changed)
  • If we really wanted to, we could have an additional value as a dependency of this effect to force an update, which could be a single simple value managed by CORE_UI or something else (e.g. forceUpdate = () => setValue( inViewBust, num + 1 ) where num would be a simple integer to bust the cache with)

I think that would solve all our needs without really requiring any changes to useInViewSelect itself while still allowing for a global reset via the datastore as an escape hatch if we really needed it.

Another benefit of this approach would be room for adding throttle/debounce-like functionality in the updating of the state-managed value which will likely be needed at some point as well.

Let me know what you think!

[tofumatt]

I had considered the "increment a value to 'cache-bust' the inViewSelect boolean" approach, and I think that part works okay, but the issue with your approach is that it ends up coupling the WidgetAreaRenderer and the hook to a degree. I wanted a system where the hook was independent, in case we wanted to use it in either finer-grained or broader <div>s in the future. It means it's easier to scaffold—it also means it's easier to understand externally.

Your proposed solution doesn't immediately strike me as more performant, but I feel like it's a bit more complicated. Maybe I'm missing something?

We could probably combine the approaches though, with only a reset data store action that incremented the "cache-busting" variable. But I thought this functionality warranted a dedicated selector as it's not "arbitrary" data and I thought it was a bit easier to understand...

The throttling/debouncing option with setState state-managed variables though is fair—keeping that out of the data store is handy.

I think we can combine the two approaches for something that combines the best of both worlds 🙂

[aaemnnosttv]

Per https://github.com/google/site-kit-wp/issues/4120#issuecomment-947788625 that option name might change, but otherwise this LGTM 👍

IB ✅

[felixarntz]

Per https://github.com/google/site-kit-wp/issues/4096#issuecomment-924477283, I've updated the ACs here to also encompass what was previously the ACs of #4001. The IB already accounts for that, so execution here can proceed as is.

[wpdarren]

QA Update: ✅

Verified:

• No requests to Site Kit's REST API routes are made when at the top of the page
• When I scroll down to the bottom of the page the widgets for Site Kit's box start to show a loading/preview block then load in their data.
• On loading, there are also XHR/Fetch requests for /google-site-kit/v1/.

[felixarntz]

Approval ❌

@tofumatt Per what we discussed yesterday, the implementation in #4347 isn't really accurate. It changes a couple of useSelects to useInViewSelect that don't trigger API calls, while it doesn't actually change the useSelects that do trigger API calls. Specifically, the WPDashboardWidgets shouldn't need to be modified in this issue at all, but the sub-components (which are the ones triggering the API calls) like WPDashboardClicks, WPDashboardImpressions etc. should.

Can you open a follow-up PR for main that reverts the changes in WPDashboardWidgets and instead adds them for where API calls are triggered (getReport and isGatheringData selector calls in any WPDashboard* components)?

cc @eugene-manuilov @aaemnnosttv

[aaemnnosttv]

@felixarntz back to you for another pass 👍

[felixarntz]

Approval ❌

@tofumatt @aaemnnosttv The fix PR is mostly good, but one API request is still being made from the WP dashboard widget even when it is hidden, likely because the isGatheringData call in WPDashboardPopularPages is still using useSelect.

[aaemnnosttv]

@felixarntz agh, I must have missed that in my testing because Analytics wasn't connected. Submitting a PR to fix it now 👍

[felixarntz]

Approval ✅