From 9ae4e7e638364b1278179c68775da718b9b00bfc Mon Sep 17 00:00:00 2001
From: Eric Peterson <ericpeterson@spotify.com>
Date: Wed, 19 Jul 2023 13:09:48 +0200
Subject: [PATCH] Fix greedy home matching bug

Signed-off-by: Eric Peterson <ericpeterson@spotify.com>
---
 .changeset/analytics-millenial-whoop.md       |  5 +
 .github/vale/Vocab/Backstage/accept.txt       |  1 +
 .../src/routing/RouteTracker.test.tsx         | 94 ++++++++++++++++++-
 .../core-app-api/src/routing/RouteTracker.tsx |  7 +-
 4 files changed, 100 insertions(+), 7 deletions(-)
 create mode 100644 .changeset/analytics-millenial-whoop.md

diff --git a/.changeset/analytics-millenial-whoop.md b/.changeset/analytics-millenial-whoop.md
new file mode 100644
index 0000000000..265e0072ce
--- /dev/null
+++ b/.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.
diff --git a/.github/vale/Vocab/Backstage/accept.txt b/.github/vale/Vocab/Backstage/accept.txt
index efd6186405..ed09c73c91 100644
--- a/.github/vale/Vocab/Backstage/accept.txt
+++ b/.github/vale/Vocab/Backstage/accept.txt
@@ -213,6 +213,7 @@ middleware
 minikube
 Minikube
 Minio
+misattributed
 misconfiguration
 misconfigured
 mkdocs
diff --git a/packages/core-app-api/src/routing/RouteTracker.test.tsx b/packages/core-app-api/src/routing/RouteTracker.test.tsx
index 6bf58a117e..695e5bbf38 100644
--- a/packages/core-app-api/src/routing/RouteTracker.test.tsx
+++ b/packages/core-app-api/src/routing/RouteTracker.test.tsx
@@ -25,23 +25,37 @@ 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 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',
@@ -49,6 +63,7 @@ describe('RouteTracker', () => {
       routeRefs: new Set([routeRef2]),
       plugins: new Set(),
       caseSensitive: false,
+      children: [MATCH_ALL_ROUTE],
     },
   ];
 
@@ -92,8 +107,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>
@@ -116,4 +131,79 @@ describe('RouteTracker', () => {
       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 no plugin/routeRef is on the route', 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,
+    });
+  });
 });
diff --git a/packages/core-app-api/src/routing/RouteTracker.tsx b/packages/core-app-api/src/routing/RouteTracker.tsx
index f1910728e3..bed72b6134 100644
--- a/packages/core-app-api/src/routing/RouteTracker.tsx
+++ b/packages/core-app-api/src/routing/RouteTracker.tsx
@@ -39,10 +39,7 @@ const getExtensionContext = (
 
     // Of the matching routes, get the last (e.g. most specific) instance of
     // the BackstageRouteObject.
-
-    const routeMatch = matches
-      ?.filter(match => match?.route.routeRefs?.size > 0)
-      .pop();
+    const routeMatch = matches?.pop();
 
     const routeObject = routeMatch?.route;
 
@@ -66,7 +63,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;