Merge pull request #18723 from backstage/analytics/fix-greedy-home-matching

[Analytics] Avoid erroneously attributing `navigate` events to home plugin

.changeset/analytics-millenial-whoop.md

@@ -0,0 +1,5 @@
+---
+'@backstage/core-app-api': patch
+---
+
+Fixed a bug that could cause `navigate` analytics events to be misattributed to the plugin mounted on the root route (e.g. the `home` plugin at `/`) when the route that was navigated to wasn't associated with a routable extension.

.github/vale/Vocab/Backstage/accept.txt

@@ -213,6 +213,7 @@ middleware
 minikube
 Minikube
 Minio
+misattributed
 misconfiguration
 misconfigured
 mkdocs

packages/core-app-api/src/routing/RouteTracker.test.tsx

@@ -25,30 +25,46 @@ import {
   createPlugin,
   createRouteRef,
 } from '@backstage/core-plugin-api';
+import { MATCH_ALL_ROUTE } from './collectors';
 
 describe('RouteTracker', () => {
+  const routeRef0 = createRouteRef({
+    id: 'home:root',
+  });
   const routeRef1 = createRouteRef({
     id: 'route1',
   });
   const routeRef2 = createRouteRef({
     id: 'route2',
   });
+  const plugin0 = createPlugin({ id: 'home' });
   const plugin1 = createPlugin({ id: 'plugin1' });
+  const plugin2 = createPlugin({ id: 'plugin2' });
 
   const routeObjects: BackstageRouteObject[] = [
+    {
+      path: '',
+      element: <div>home page</div>,
+      routeRefs: new Set([routeRef0]),
+      plugins: new Set([plugin0]),
+      caseSensitive: false,
+      children: [MATCH_ALL_ROUTE],
+    },
     {
       path: '/path/:p1/:p2',
       element: <Link to="/path2/hello">go</Link>,
       routeRefs: new Set([routeRef1]),
       plugins: new Set([plugin1]),
       caseSensitive: false,
+      children: [MATCH_ALL_ROUTE],
     },
     {
       path: '/path2/:param',
       element: <div>hi there</div>,
       routeRefs: new Set([routeRef2]),
-      plugins: new Set(),
+      plugins: new Set([plugin2]),
       caseSensitive: false,
+      children: [MATCH_ALL_ROUTE],
     },
   ];
 
@@ -92,8 +108,8 @@ describe('RouteTracker', () => {
           <RouteTracker routeObjects={routeObjects} />
 
           <Routes>
-            {routeObjects.map(({ routeRefs, ...props }) => (
-              <Route {...props} key={props.path} />
+            {routeObjects.map(({ path, element }) => (
+              <Route key={path} path={path || '/'} element={element} />
             ))}
           </Routes>
         </TestApiProvider>
@@ -109,11 +125,110 @@ describe('RouteTracker', () => {
       },
       context: {
         extension: 'App',
-        pluginId: 'root',
+        pluginId: 'plugin2',
         routeRef: 'route2',
       },
       subject: '/path2/hello',
       value: undefined,
     });
   });
+
+  it('should capture path query and hash', async () => {
+    render(
+      <MemoryRouter initialEntries={['/path/foo/bar?q=1#header-1']}>
+        <TestApiProvider apis={[[analyticsApiRef, mockedAnalytics]]}>
+          <RouteTracker routeObjects={routeObjects} />
+        </TestApiProvider>
+      </MemoryRouter>,
+    );
+
+    expect(mockedAnalytics.captureEvent).toHaveBeenCalledWith({
+      action: 'navigate',
+      attributes: {
+        p1: 'foo',
+        p2: 'bar',
+      },
+      context: {
+        extension: 'App',
+        pluginId: 'plugin1',
+        routeRef: 'route1',
+      },
+      subject: '/path/foo/bar?q=1#header-1',
+      value: undefined,
+    });
+  });
+
+  it('should match the root path and send relevant context', async () => {
+    render(
+      <MemoryRouter initialEntries={['/']}>
+        <TestApiProvider apis={[[analyticsApiRef, mockedAnalytics]]}>
+          <RouteTracker routeObjects={routeObjects} />
+        </TestApiProvider>
+      </MemoryRouter>,
+    );
+
+    expect(mockedAnalytics.captureEvent).toHaveBeenCalledWith({
+      action: 'navigate',
+      attributes: {},
+      context: {
+        extension: 'App',
+        pluginId: 'home',
+        routeRef: 'home:root',
+      },
+      subject: '/',
+      value: undefined,
+    });
+  });
+
+  it('should return default context when it would have otherwise matched on the root path', async () => {
+    render(
+      <MemoryRouter initialEntries={['/not-routable-extension']}>
+        <TestApiProvider apis={[[analyticsApiRef, mockedAnalytics]]}>
+          <RouteTracker routeObjects={routeObjects} />
+          <Routes>
+            <Route
+              path="/not-routable-extension"
+              element={<>Non-extension</>}
+            />
+          </Routes>
+        </TestApiProvider>
+      </MemoryRouter>,
+    );
+
+    expect(mockedAnalytics.captureEvent).toHaveBeenCalledWith({
+      action: 'navigate',
+      attributes: {},
+      context: {
+        extension: 'App',
+        pluginId: 'root',
+        routeRef: 'unknown',
+      },
+      subject: '/not-routable-extension',
+      value: undefined,
+    });
+  });
+
+  it('should return parent route context on navigating to a sub-route', async () => {
+    render(
+      <MemoryRouter initialEntries={['/path2/param-value/sub-route']}>
+        <TestApiProvider apis={[[analyticsApiRef, mockedAnalytics]]}>
+          <RouteTracker routeObjects={routeObjects} />
+        </TestApiProvider>
+      </MemoryRouter>,
+    );
+
+    expect(mockedAnalytics.captureEvent).toHaveBeenCalledWith({
+      action: 'navigate',
+      attributes: {
+        param: 'param-value',
+      },
+      context: {
+        extension: 'App',
+        pluginId: 'plugin2',
+        routeRef: 'route2',
+      },
+      subject: '/path2/param-value/sub-route',
+      value: undefined,
+    });
+  });
 });

packages/core-app-api/src/routing/RouteTracker.tsx

@@ -38,12 +38,11 @@ const getExtensionContext = (
     const matches = matchRoutes(routes, { pathname });
 
     // Of the matching routes, get the last (e.g. most specific) instance of
-    // the BackstageRouteObject.
-
+    // the BackstageRouteObject that contains a routeRef. Filtering by routeRef
+    // ensures subRouteRefs are aligned to their parent routes' context.
     const routeMatch = matches
       ?.filter(match => match?.route.routeRefs?.size > 0)
       .pop();
-
     const routeObject = routeMatch?.route;
 
     // If there is no route object, then allow inheritance of default context.
@@ -51,6 +50,12 @@ const getExtensionContext = (
       return undefined;
     }
 
+    // If the matched route is the root route (no path), and the pathname is
+    // not the path of the homepage, then inherit from the default context.
+    if (routeObject.path === '' && pathname !== '/') {
+      return undefined;
+    }
+
     // If there is a single route ref, use it.
     let routeRef: RouteRef | undefined;
     if (routeObject.routeRefs.size === 1) {
@@ -66,7 +71,7 @@ const getExtensionContext = (
     const params = Object.entries(
       routeMatch?.params || {},
     ).reduce<AnalyticsEventAttributes>((acc, [key, value]) => {
-      if (value !== undefined) {
+      if (value !== undefined && key !== '*') {
         acc[key] = value;
       }
       return acc;