Chore/track analytics consent screen view and selection rates #2784

[edu-stx]

Try out this version of the Hiro Wallet - download extension builds.

Segment is always on. Segment can only be turned on, it can’t be turned off once it’s turned on. Therefore, in order to be able to capture the analytics acceptance rate (and then nothing else should the user opt out of analytics), Segment needs to be turned on as soon as the app loads. The user’s consent response is respected by modifying the behavior of the methods we use to interact with Segment: if the user accepts, they work as expected and capture analytics, otherwise, they’re converted into no-ops and calling those functions won’t capture any analytics.

Segment only captures consent page load and consent response by default. These are the only two analytics that Segment will capture by default. Should the user opt out of analytics, Segment will not capture anything further.

Users can’t manually navigate to pages other than the consent screen without having provided their consent response. Users may try to navigate to any part of the app by typing the address in the URL bar. However, given Segment is on by default, until they provide a response to the analytics consent, they will always be redirected to the consent page and aren’t allowed to view other pages or interact with the app in any way (and therefore avoid data collection).

Sentry code has been commented removed given it was disabled to avoid interfering with this PR, will be taken care of as part of #2822.

FYI @fbwoolf, using settings slice as recommended in comment.

Closes #2784

[edu-stx]

@markmhx issues with page views not being reported correctly are now fixed, feel free to try it out.

[kyranjamie]

Nice approach Edu. Appreciate the time spent trying different solutions 👍🏼

[markmhendrickson]

The data is now showing up for me during testing in the Segment debugger 🙏

I did try loading another route (/buy) upon installing the build and before responding to either consent option, and the view was tracked even though the page redirected properly back to the /request_diagnostics screen.

Could we make this airtight in that this buy page view wouldn't get reported in this scenario in addition to the subsequent request-diagnostics one?

[fbwoolf]

Nice work @edu-stx! 👍

[edu-stx]

@kyranjamie @markmhx the page views are being tracked by this line whenever the path changes,

https://github.com/hirosystems/stacks-wallet-web/blob/8414602e1954bf1d7678be866acc9a39149a79cf/src/app/routes/app-routes.tsx#L51

Even if a user goes to /a-page-that-doesnt-exist, it will still get picked up. To make it airtight while adhering to the conventions currently in place, I've wrapped the app in a component solely dedicated to the consent screen. Looks roughly like,

function AppRoutesAfterAnalyticsConsentAnswered() {
  // All routes as they were, unchanged
}

function AppRoutes() {
  if (!hasResponded) return <AllowDiagnosticsPage />

  return <AppRoutesAfterAnalyticsConsentAnswered />
}

[kyranjamie]

Great that you've changed the page tracking events, this is an improvement. Have a search for .page( though, as in some places we still call this manually.

[kyranjamie]

What happened to using the route guard?

If we do do it this way: Wouldn't it make more sense to flip it: creating a separate component for the RequestDiagnosticsPage, and leave the existing routes here?

if (!hasResponded) return <AppRoutesBeforeAnalyticsConsentAnswered />;

return <AllTheRoutesAsTheyWereBefore>...

The exception is the Request Diagnostics routes, so I'd make a component for that, not the rest of the entire app's routing.

---

More generally, if this pattern has advantages for this case, I'm okay with it. But for example, why don't we do the same for creating an account?

if (!hasCreatedAccount) {
  return <Routes><OnlyRoutesForMakingAnAccount /><Routes/>
}

It's more common in my exp. to define all the routes statically, and implement route guards to prevent navigation to restricted pages, than it is conditionally creating a route config.

[edu-stx]

Great that you've changed the page tracking events, this is an improvement. Have a search for .page( though, as in some places we still call this manually.

Thank you :) Do you mean in general in the codebase or as part of this PR? Code unrelated to this feature seems out of scope. This PR has a .page() call for the analytics consent page, given the “catch all” routes’ .page() seen below is not yet loaded when the consent screen loads.

https://github.com/hirosystems/stacks-wallet-web/blob/8414602e1954bf1d7678be866acc9a39149a79cf/src/app/routes/app-routes.tsx#L51

What happened to using the route guard?

Using the route guard caused page views to still be picked up by the above before users answered the analytics consent. We would have ended up collecting any routes the users manually navigated to before they responded to the analytics consent, meaning we would not be making the data collection “airtight”, i.e. strictly contained to the analytics page & response prior to user acceptance.

If we do do it this way: Wouldn't it make more sense to flip it: creating a separate component for the RequestDiagnosticsPage, and leave the existing routes here?

This is the approach taken in an earlier commit, albeit rendering the elements directly. It seemed from an earlier discussion that we should not follow this pattern, and in any case it doesn’t work with the above catch all .page() hook.

More generally, if this pattern has advantages for this case [...]

It not so much it having advantages, as it is fulfilling the objectives at hand,

• From the feature: strictly ensure only views and responses to consent are collected prior to acceptance
• From earlier feedback: keep as close to the existing patterns as possible

I’m sure there are better ways of doing this, although I can’t see how we’d go about it without, for example, refactoring how we collect page views to collect only what is expected. Incidentally, the current implementation will collect not just page views, but any random string the user types in the URL.

But for example, why don't we [...]

Seems that it’s a matter of convention: what conventions do we have and follow, and to what extent do the conventions we adopt allow us to implement the features we want? In this PR, I tried my best to follow existing conventions while implementing the feature 😇

[kyranjamie]

Sorry if this wasn't clear.

useEffect(() => void analytics.page('view', `${pathname}`), [analytics, pathname]);

This effect improves the page tracking, but in some places page tracking is handled manually, which means this addition will double log page views. As this PR introduces this behaviour, I think it should be fixed before merging. Edit: Edu pointed out this is already in dev
Example 1. Example 2. All that's required is removing these page tracking events.

The other point was just a small one on component organisation, rather than anything relating to a previous conversation.
As a developer, when I come to <AppRoutes /> I expect to see the app's routes. But now, the main responsibility of the component is handling the edge case regarding the request diagnostics page. It makes more sense to create an abstraction around the edge case, and move that to its own component, rather than the entire app's routes. Here's a clearer example:

function AppRoutesBeforeUserHasConsented() {
  return (
    <Routes>
      <Route path={RouteUrls.RequestDiagnostics} element={<AllowDiagnosticsPage />} />
      <Route path="*" element={<Navigate replace to={RouteUrls.RequestDiagnostics} />} />
    </Routes>
  );
}
export function AppRoutes() {
  const hasStateRehydrated = useHasStateRehydrated();
  const hasResponded = useHasUserRespondedToAnalyticsConsent();

  if (!hasStateRehydrated) return <LoadingSpinner />;

  if (!hasResponded) {
    return <AppRoutesBeforeUserHasConsented />
  }

  return <AllTheAppsRoutesAsComponentNameSuggests />;
}

[edu-stx]

Ok, I'll re-arrange app routes