Repository navigation
feat(browser): React Router integration - #1130
JeremyFunk wants to merge 2 commits into
Conversation
@maple-dev/browser/react-router: data-mode and framework-mode client instrumentations, traceNavigations, useMaplePageload and reportRouteError. @maple-dev/browser/react-router/server: the server instrumentation (request span named after the route, loader and action spans) and handleError.
Data mode no longer ends the page load at a state change before the first load finishes. Framework mode ends a click abandoned by back or forward as interrupted, ends the page load at the first Layout mount only, and leaves sanitized server loader errors to the server's span.
Maple reviewConfidence 3/5 · needs attention Warning This review ended early; what follows is what it established. Adds
What was checked
Observability coverage: 4 of 4 changes observable
Files not reviewed (3)The review ended before it read these diffs, so nothing above vouches for them.
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| // `navigate(-1)` is a history navigation, like the back button, and hash links run nothing | ||
| if (typeof to === "number" || to.startsWith("#")) return |
There was a problem hiding this comment.
🔴 Hash and history navigation silently blocked
For hash links or numeric history targets, frameworkInstrumentation returns without calling navigate(). Those navigations never execute, leaving the page at its current location.
| // `navigate(-1)` is a history navigation, like the back button, and hash links run nothing | |
| if (typeof to === "number" || to.startsWith("#")) return | |
| // Skip tracing hash links and history navigation, but still execute them | |
| if (typeof to === "number" || to.startsWith("#")) { | |
| await navigate() | |
| return | |
| } |
Was this helpful? React with 👍 or 👎 to provide feedback.
| // Fetcher loads, revalidations and hash changes aren't navigations | ||
| if (!loading && state.location.pathname === pathname) return | ||
| // A route without loaders goes straight to the new location | ||
| if (!loading) startNavigation(state.location.pathname) |
There was a problem hiding this comment.
🟡 Search-only navigations lose their spans
On routes without loaders, traceNavigations skips completed query changes because their pathname stays unchanged. The navigation never gets a span.
Learn more
Data-mode navigations without loaders can publish only an idle state at their new location. The current pathname comparison treats a query-only navigation as a fetcher or hash update. The subscriber therefore skips a real navigation, despite tracing query changes when loaders run.
Example: Starting at /about, where no loader exists, navigate to /about?tab=team. The router changes location without entering a loading state; the callback sees the same pathname and records no navigate /about span.
Recommended fix: Track the committed pathname and search string together, and compare both at the idle-state gate. Ignore changes to hash alone; retain the existing loading path for loader-backed navigations.
Was this helpful? React with 👍 or 👎 to provide feedback.
| export const matchedPattern = (paths: readonly (string | undefined)[]): string => | ||
| routePattern(paths.filter(Boolean).join("/")) |
There was a problem hiding this comment.
🟡 Absolute child routes get duplicated trace names
For nested absolute routes, matchedPattern concatenates the parent's path with the complete child path. A /projects/:id child under /projects becomes /projects/projects/:id in navigation names.
Learn more
React Router supports a nested route whose absolute path starts with its parent's path. Both data-mode matching and framework page-load matching pass each matched route path into matchedPattern. Concatenating those paths repeats the parent segment for an absolute child.
Example: A parent with path: "/projects" and child with path: "/projects/:id" matches /projects/42, but the recorded span is named navigate /projects/projects/:id.
Recommended fix: Build the pattern incrementally, replacing the accumulated prefix when a route path starts with /, while retaining the current behavior for relative children and pathless layouts. Add a test with an absolute nested child.
Was this helpful? React with 👍 or 👎 to provide feedback.
This branch has not been deployed
1 participant
Summary
A React Router integration for
@maple-dev/browser, replacing thetracing.ts/router-tracing.ts/entry.server.tsxglue the React Router frontend guide had customers copy. Stacked on #1128 (/server,serverTiming(), the Next.js entries).@maple-dev/browser/react-router→dataRouterInstrumentation: ClientInstrumentationcreateBrowserRouter'sinstrumentations. Starts the page load before the first loaders run; a span per loader and action@maple-dev/browser/react-router→traceNavigations(router): () => voidcreateBrowserRouter. Ends each navigation, named after the matched route pattern@maple-dev/browser/react-router→frameworkInstrumentation: ClientInstrumentationHydratedRouter. Page load start, thenavigatehook (latest-click guard, pathname-onlyurl.path, hash links skipped), loader and action spans@maple-dev/browser/react-router→useMaplePageload(): voidLayout. Ends the page load once the page hydrates, named fromuseMatches()@maple-dev/browser/react-router→reportRouteError: ClientOnErrorFunctiononErrorforRouterProvider/HydratedRouter: skips thrown responses and errors a loader span recorded@maple-dev/browser/react-router/server→serverInstrumentation: ServerInstrumentationentry.server.tsxinstrumentations: a request span renamed toGET /projects/:id, with the status code, and server loader/action spans@maple-dev/browser/react-router/server→handleError: HandleErrorFunctionhandleRequestjoins the page load withserverTiming()from@maple-dev/browser/server(two lines, in the docs)./react-router/serverdepends on@opentelemetry/apiandreact-routeronly.react-routeris an optional peer (*);react-router@^8.4.0is a dev dependency. The main entry is unchanged: eager 37.42 kB, first-party 14.30 kB, same as #1128.Behavior
Reproduces the guide's verified behavior:
url.pathkeeps the first path); routes without loaders get zero-length spans; back/forward and query changes are navigations; hash changes, revalidations and fetchers aren't; a URL no route matches isnavigate /.app.navigation.interrupted; redirected navigations are named after the clicked route; back/forward aren't traced; every route gets aloaderspan (module loading).Errors fail a loader span (React Router reportsredirect(),data()and Responses as success). One error, one issue: loader errors are on their span andreportRouteError/handleErrorskip them.Differences from the guide code, each found in review and covered by a test:
HydrateFallback): it waits forstate.initialized.popstatelistener). Before, the abortednavigate()ended it as a completednavigate /slow, and the back navigation's loaders ran under it.useMaplePageloadends the page load once: React Router mounts a separateLayoutaround the page, theErrorBoundaryand theHydrateFallback, so a remount could end an in-flight navigation.Error("Unexpected Server Error")with no stack. The browser loader span andreportRouteErrorskip it, so it's recorded once, on the server's loader span (the Next.js entry skipsdigesterrors for the same reason).handleErrorlogs what React Router's default logs (error.errorfor a no-match 404) and records with the shared dedupe, without changing the request span's status (that follows the response code).Tests
packages/browser: typecheck, 173 tests (node + Chromium), build, size.route.test.ts: pattern joining, loader/action instrumentation never throws.index.browser.test.ts: the realcreateMemoryRouter+RouterProviderin Chromium, for both modes: naming, loader parenting, query/hash/revalidation/fetcher, history, redirect, merged and interrupted clicks, popstate, not-found, loader/render/server errors, page load on hydration and on Layout remount.server.test.ts: the realcreateRequestHandlerwith a build whose server entry exports the two server pieces: request span under the HTTP span, loader span under it,Server-Timingequals the request span,.datarequests, loader/render errors recorded once, 404 logged and not recorded, aborted requests, no server OTel.End to end
The React Router 8.4 test apps instrumented with the old guide code (framework mode with SSR via
react-router-serve+ Node SDK, and a data-mode SPA) were switched to the packed tarball, production builds, and driven with the same Playwright script. Span trees match the old run in every scenario, apart from the order of the two parallelfetchspans and the status message now on the failed loader span.GET→GET /projects/:id→pageload /projects/:id,server-timingon HTML, new trace on reload → samepageload /projects/:id+ loader + fetches → samenavigate /projects/:id> loaders > fetches > API → samenavigate /old+ both loaders → samenavigate /projects/:id,url.path=/old→ samenavigateINTERRUPTED +navigate /projects/:id→ samenavigate /projects/:id(404 data),pageload /+ 404 server span → samereact_router.erroronce; SSR: request span 500 + exception → samereact_router.erroronce → samebrowser.uncaught_error; API 500 → sametraceparent, none to third party → samenavigate /slow→navigateINTERRUPTED at the popstateUnexpected Server Error) → server loader span onlyKnown limits
metato the instrumentation results. On 7.15 to 8.0 those spans keep plain names (navigate,GET); data mode and the page load are unaffected.url.pathresolves a relativeto(edit,.) against the URL, React Router against the route; the span name is right. With abasename, the page load'surl.pathincludes it and navigations' don't.window.location.pathname, so a hash router's page loadurl.pathis/..datarequests in their own trace,clientLoader.hydrateloads after the page load ends.Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.