diff --git a/packages/remix/src/client/remixRouteParameterization.ts b/packages/remix/src/client/remixRouteParameterization.ts index 5e389c2e87ad..eba1fedff620 100644 --- a/packages/remix/src/client/remixRouteParameterization.ts +++ b/packages/remix/src/client/remixRouteParameterization.ts @@ -12,28 +12,56 @@ let cachedManifestString: string | undefined = undefined; const compiledRegexCache: Map = new Map(); const routeResultCache: Map = new Map(); +// Specificity ranks for a single route segment, from most to least specific. `END` is the rank of +// the position just past the last segment of a route, so that a route which stops is compared +// against whatever the longer route continues with. +const SEGMENT_STATIC = 0; +const SEGMENT_DYNAMIC = 1; +const SEGMENT_END = 2; +const SEGMENT_SPLAT = 3; + /** - * Calculate specificity score for route matching. Lower scores = more specific routes. + * Calculate the specificity rank for a single route segment. Lower ranks = more specific segments. */ -function getRouteSpecificity(routePath: string): number { - const segments = routePath.split('/').filter(Boolean); - let score = 0; - - for (const segment of segments) { - if (segment.startsWith(':')) { - const paramName = segment.substring(1); - if (paramName.endsWith('*')) { - // Splat/catchall routes are least specific - score += 100; - } else { - // Dynamic segments are more specific than splats - score += 10; - } +function getSegmentSpecificity(segment: string | undefined): number { + if (segment === undefined) { + // The route has no more segments + return SEGMENT_END; + } + if (!segment.startsWith(':')) { + // Static segments are the most specific + return SEGMENT_STATIC; + } + + // Splat/catchall routes are the least specific + return segment.substring(1).endsWith('*') ? SEGMENT_SPLAT : SEGMENT_DYNAMIC; +} + +/** + * Compare two route paths by specificity, ordering the most specific route first. + * + * Routes are compared segment by segment, with the first segment they disagree on deciding the + * winner. Comparing aggregate scores instead would rank a short splat like '/:lang/:*' above a + * longer but strictly narrower route like '/:lang/guides/:category/:*', because the longer route + * accumulates more score simply by having more segments. + * + * Routes of differing lengths are compared one segment past the shorter one, where `SEGMENT_END` + * decides whether continuing narrows the route or widens it: '/:lang/foo' is more specific than + * '/:lang', but '/:lang' is more specific than '/:lang/:*'. + */ +function compareRouteSpecificity(routePathA: string, routePathB: string): number { + const segmentsA = routePathA.split('/').filter(Boolean); + const segmentsB = routePathB.split('/').filter(Boolean); + + const comparedSegmentCount = Math.min(segmentsA.length, segmentsB.length) + 1; + for (let i = 0; i < comparedSegmentCount; i++) { + const difference = getSegmentSpecificity(segmentsA[i]) - getSegmentSpecificity(segmentsB[i]); + if (difference !== 0) { + return difference; } - // Static segments add 0 (most specific) } - return score; + return 0; } /** @@ -157,7 +185,7 @@ export const maybeParameterizeRemixRoute = (route: string): string | undefined = const { staticRoutes, dynamicRoutes } = manifest; const matches = findMatchingRoutes(route, staticRoutes, dynamicRoutes); - const result = matches.sort((a, b) => getRouteSpecificity(a) - getRouteSpecificity(b))[0]; + const result = matches.sort(compareRouteSpecificity)[0]; routeResultCache.set(route, result); diff --git a/packages/remix/test/client/remixRouteParameterization.test.ts b/packages/remix/test/client/remixRouteParameterization.test.ts index 128d50a363aa..39ce3630b34c 100644 --- a/packages/remix/test/client/remixRouteParameterization.test.ts +++ b/packages/remix/test/client/remixRouteParameterization.test.ts @@ -358,6 +358,30 @@ describe('maybeParameterizeRemixRoute', () => { // Unmatched patterns should fall back to catch-all expect(maybeParameterizeRemixRoute('/some/other/path')).toBe('/:*'); }); + + it('should prefer a longer route over a shorter splat that also matches', () => { + const manifest: RouteManifest = { + staticRoutes: [], + dynamicRoutes: [ + { + path: '/:lang/:*', + regex: '^/([^/]+)/(.+)$', + paramNames: ['lang', '*'], + }, + { + path: '/:lang/guides/:category/:*', + regex: '^/([^/]+)/guides/([^/]+)/(.+)$', + paramNames: ['lang', 'category', '*'], + }, + ], + }; + globalWithInjectedManifest._sentryRemixRouteManifest = JSON.stringify(manifest); + + expect(maybeParameterizeRemixRoute('/fr/guides/renting/foo')).toBe('/:lang/guides/:category/:*'); + + // The splat still wins where nothing narrower matches + expect(maybeParameterizeRemixRoute('/fr/anything/else')).toBe('/:lang/:*'); + }); }); describe('caching behavior', () => {