🐛 [RUM-6226][rum-react] improve routes wildcard substitution (#3105)

* 🐛 [RUM-6226][rum-react] improve routes wildcard substitution

* 👌 add comments to clarify splats substitution

packages/rum-react/src/domain/reactRouterV6/startReactRouterView.spec.ts

@@ -1,5 +1,6 @@
-import type { RouteMatch } from 'react-router-dom'
+import { type RouteObject, createMemoryRouter, type RouteMatch } from 'react-router-dom'
 import { display } from '@datadog/browser-core'
+import { registerCleanupTask } from '../../../../core/test'
 import { initializeReactPlugin } from '../../../test/initializeReactPlugin'
 import { computeViewName, startReactRouterView } from './startReactRouterView'
 
@@ -42,57 +43,67 @@ describe('computeViewName', () => {
     expect(computeViewName([] as RouteMatch[])).toBe('')
   })
 
-  it('returns the path of the first route match', () => {
-    expect(computeViewName([{ route: { path: '/foo' } }] as RouteMatch[])).toBe('/foo')
-  })
-
-  it('concatenates the paths of all route matches', () => {
-    expect(
-      computeViewName([
-        { route: { path: '/foo' } },
-        { route: { path: 'bar' } },
-        { route: { path: ':id' } },
-      ] as RouteMatch[])
-    ).toBe('/foo/bar/:id')
-  })
-
   it('ignores routes without a path', () => {
     expect(
       computeViewName([{ route: { path: '/foo' } }, { route: {} }, { route: { path: ':id' } }] as RouteMatch[])
     ).toBe('/foo/:id')
   })
 
-  it('handles absolute paths', () => {
-    expect(
-      computeViewName([
-        { route: { path: '/foo' } },
-        { route: { path: '/bar' } },
-        { route: { path: '/:id' } },
-      ] as RouteMatch[])
-    ).toBe('/:id')
-  })
+  // prettier-ignore
+  const cases = [
+      // route paths,         path,           expected view name
 
-  it('removes intermediary trailing slashes', () => {
-    expect(
-      computeViewName([
-        { route: { path: '/foo/' } },
-        { route: { path: 'bar/' } },
-        { route: { path: ':id/' } },
-      ] as RouteMatch[])
-    ).toBe('/foo/bar/:id/')
-  })
+      // Simple paths
+      ['/foo',                '/foo',         '/foo'],
+      ['/foo',                '/bar',         '/foo'], // apparently when the path doesn't match any route, React Router returns the last route as a matching route
+      ['/foo > bar',          '/foo/bar',     '/foo/bar'],
+      ['/foo > bar > :p',     '/foo/bar/1',   '/foo/bar/:p'],
+      [':p',                  '/foo',         '/:p'],
+      ['/foo/:p',             '/foo/bar',     '/foo/:p'],
+      ['/foo > :p',           '/foo/bar',     '/foo/:p'],
+      ['/:a/:b',              '/foo/bar',     '/:a/:b'],
+      ['/:a > :b',            '/foo/bar',     '/:a/:b'],
+      ['/foo-:a',             '/foo-1',       '/foo-:a'],
+      ['/foo/ > bar/ > :id/', '/foo/bar/1/',  '/foo/bar/:id/'],
+      ['/foo > /foo/bar > /foo/bar/:id',
+                              '/foo/bar/1',   '/foo/bar/:id'],
 
-  it('replaces match-all routes with the actual path', () => {
-    expect(
-      computeViewName([
-        { route: { path: '/foo' } },
-        {
-          params: { '*': 'bar' },
-          pathname: '/bar',
-          pathnameBase: '/',
-          route: { path: '*' },
-        },
-      ] as RouteMatch[])
-    ).toBe('/foo/bar')
+      // Splats
+      ['*',                   '/foo/1',       '/foo/1'],
+      ['/foo/*',              '/foo/1',       '/foo/1'],
+      ['/foo > *',            '/foo/1',       '/foo/1'],
+      ['* > *',               '/foo/1',       '/foo/1'],
+      ['/foo/* > *',          '/foo/1',       '/foo/1'],
+      ['* > foo/*',           '/foo/1',       '/foo/1'],
+      ['/foo/* > bar/*',      '/foo/bar/1',   '/foo/bar/1'],
+      ['/foo/* > bar',        '/foo/bar',     '/foo/bar'],
+      ['/foo/:p > *',         '/foo/bar/baz', '/foo/:p/baz'],
+      ['/:p > *',             '/foo/bar/1',   '/:p/bar/1'],
+      ['/foo/* > :p',         '/foo/bar',     '/foo/:p'],
+
+      // Extra edge cases - React Router does not provide the matched path in those case
+      ['/foo/*/bar',          '/foo/1/bar',   '/foo/*/bar'],
+      ['/foo/*-bar',          '/foo/1-bar',   '/foo/*-bar'],
+      ['*/*',                 '/foo/1',       '/*/*'],
+    ] as const
+
+  cases.forEach(([routePaths, path, expectedViewName]) => {
+    it(`compute the right view name for paths ${routePaths}`, () => {
+      // Convert the routePaths representing nested routes paths delimited by ' > ' to an actual
+      // react-router route object. Example: '/foo > bar > :p' would be turned into
+      // { path: '/foo', children: [{ path: 'bar', children: [{ path: ':p' }] }] }
+      const route = routePaths
+        .split(' > ')
+        .reduceRight(
+          (childRoute, path) => ({ path, children: childRoute ? [childRoute] : undefined }),
+          undefined as RouteObject | undefined
+        )!
+
+      const router = createMemoryRouter([route], {
+        initialEntries: [path],
+      })
+      registerCleanupTask(() => router.dispose())
+      expect(computeViewName(router.state.matches)).toEqual(expectedViewName)
+    })
   })
 })

packages/rum-react/src/domain/reactRouterV6/startReactRouterView.ts

@@ -20,11 +20,13 @@ export function computeViewName(routeMatches: RouteMatch[]) {
   let viewName = '/'
 
   for (const routeMatch of routeMatches) {
-    const path = routeMatch.route.path
+    let path = routeMatch.route.path
     if (!path) {
       continue
     }
 
+    path = substitutePathSplats(path, routeMatch.params, routeMatch === routeMatches[routeMatches.length - 1])
+
     if (path.startsWith('/')) {
       // Absolute path, replace the current view name
       viewName = path
@@ -33,13 +35,41 @@ export function computeViewName(routeMatches: RouteMatch[]) {
       if (!viewName.endsWith('/')) {
         viewName += '/'
       }
-      if (path === '*') {
-        viewName += routeMatch.params['*']!
-      } else {
-        viewName += path
-      }
+      viewName += path
     }
   }
 
   return viewName
 }
+
+/**
+ * React-Router allows to define routes with "splats" (or "catchall" or "star") segments[1],
+ * example: /files/*. It has been noticed that keeping those splats in the view name isn't helpful
+ * as it "hides" too much information. This function replaces the splats with the actual URL path
+ * name that they are matching.
+ *
+ * [1]: https://reactrouter.com/en/main/route/route#splats
+ *
+ * @example
+ * substitutePathSplats('/files/*', { '*': 'path/to/file' }, true) // => '/files/path/to/file'
+ */
+function substitutePathSplats(path: string, params: Record<string, string | undefined>, isLastMatchingRoute: boolean) {
+  if (
+    !path.includes('*') ||
+    // In some edge cases, react-router does not provide the `*` parameter, so we don't know what to
+    // replace it with. In this case, we keep the asterisk.
+    !params['*']
+  ) {
+    return path
+  }
+
+  // The `*` parameter is only related to the last matching route path.
+  if (isLastMatchingRoute) {
+    return path.replace(/\*/, params['*'])
+  }
+
+  // Intermediary route paths with a `*` are kind of edge cases, and the `*` parameter is not
+  // relevant for them. We remove it from the path (along with a potential slash preceeding it) to
+  // have a coherent view name once everything is concatenated (see examples in spec file).
+  return path.replace(/\/?\*/, '')
+}