Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -31,9 +31,10 @@
"typecheck": "react-router typegen && tsc",
"clean": "npx rimraf node_modules pnpm-lock.yaml",
"test:build": "pnpm install && pnpm build",
"test:assert": "pnpm test:ts && pnpm test:playwright",
"test:assert": "pnpm test:ts && pnpm test:prod && pnpm test:dev",
"test:ts": "pnpm typecheck",
"test:playwright": "playwright test"
"test:prod": "TEST_ENV=production playwright test",
"test:dev": "TEST_ENV=development playwright test"
},
"eslintConfig": {
"extends": [
Expand Down
Original file line number Diff line number Diff line change
@@ -1,9 +1,15 @@
import { getPlaywrightConfig } from '@sentry-internal/test-utils';
import { fileURLToPath } from 'url';

// Run the same tests against both the production server (`react-router-serve`) and the dev server
// (`react-router dev`), selected via TEST_ENV. This ensures server build capture (used for middleware
// name resolution) works in both modes. `react-router dev` runs the Vite dev server, which ignores
// PORT, so the port is passed on the command instead.
const startCommand = process.env.TEST_ENV === 'development' ? `pnpm dev --port 3030` : `PORT=3030 pnpm start`;

const config = getPlaywrightConfig(
{
startCommand: `PORT=3030 pnpm start`,
startCommand,
port: 3030,
},
// Boot Redis before the tests run, outside the webServer startup-timeout window.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,13 @@ test.describe('server - instrumentation API error capture', () => {
expect(error.contexts?.trace?.trace_id).toBe(transaction.contexts?.trace?.trace_id);
});

// Skipped in dev: the action error is sometimes captured via the client instrumentation path
// (mechanism `react_router.client_action`, client-side transaction name) rather than the server
// `react_router.action` / `POST ...` asserted here, making this flaky in dev. Server-mechanism
// error capture in dev is still covered by the loader/middleware error tests above.
test('should capture action errors with instrumentation API mechanism', async ({ page }) => {
test.skip(process.env.TEST_ENV === 'development', 'Action error capture races the client path in dev');

const errorPromise = waitForError(APP_NAME, async errorEvent => {
return errorEvent.exception?.values?.[0]?.value === 'Action error for testing';
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -219,9 +219,10 @@ test.describe('client - hybrid navigation (instrumentation API span + legacy par
'sentry.origin': 'auto.navigation.react_router.instrumentation_api',
'navigation.type': 'router.back',
'url.template': '/performance',
// react-router-serve 301-redirects the bare index route to a trailing slash
'url.path': '/performance/',
'url.full': expect.stringMatching(/^https?:\/\/localhost:\d+\/performance\/$/),
// react-router-serve 301-redirects the bare index route to a trailing slash in prod, while
// the dev server serves it without - accept both.
'url.path': expect.stringMatching(/^\/performance\/?$/),
'url.full': expect.stringMatching(/^https?:\/\/localhost:\d+\/performance\/?$/),
},
},
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,10 @@ test.describe('client - instrumentation API pageload', () => {
op: 'pageload',
data: {
'url.template': '/performance',
// react-router-serve 301-redirects the bare index route to a trailing slash
'url.path': '/performance/',
'url.full': expect.stringMatching(/^https?:\/\/localhost:\d+\/performance\/$/),
// react-router-serve 301-redirects the bare index route to a trailing slash in prod, while
// the dev server serves it without - accept both.
'url.path': expect.stringMatching(/^\/performance\/?$/),
'url.full': expect.stringMatching(/^https?:\/\/localhost:\d+\/performance\/?$/),
},
},
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,14 @@ test.describe('server - instrumentation API performance', () => {
});
});

// Prod-only: the dev server (Vite) serves source modules (`/@vite/client`, `/app/*`) as separate
// requests, each producing its own http.server transaction, so "exactly one" only holds in prod.
test('sends exactly one http.server transaction per request (no double-instrumentation)', async ({ page }) => {
test.skip(
process.env.TEST_ENV === 'development',
'Dev server emits extra http.server transactions for module requests',
);

const httpServerTransactions: Array<string | undefined> = [];
void waitForTransaction(APP_NAME, async transactionEvent => {
if (transactionEvent.contexts?.trace?.op === 'http.server') {
Expand Down
16 changes: 5 additions & 11 deletions packages/react-router/src/vite/makeServerBuildCapturePlugin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,21 +7,15 @@ const SERVER_BUILD_MODULE_ID = 'virtual:react-router/server-build';
* A Sentry plugin for React Router to capture the server build for middleware name resolution.
*/
export function makeServerBuildCapturePlugin(): Plugin {
let isSsrBuild = false;

return {
name: 'sentry-react-router-server-build-capture',
enforce: 'post',

configResolved(config) {
isSsrBuild = !!config.build.ssr;
},

transform(code, id) {
// TODO: This only captures the server build for production SSR builds. Dev mode
// (`react-router dev`) is not covered yet, so middleware names may be missing there - this
// should be handled for dev too.
if (!isSsrBuild) {
transform(code, id, options) {
// Only inject into the server build module when transforming for the SSR environment.
// `options.ssr` is set for the SSR module in both dev (`react-router dev`) and production
// builds, so this covers both - unlike `config.build.ssr`, which is only true during a build.
if (!options?.ssr) {
return null;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,16 @@ describe('makeServerBuildCapturePlugin', () => {
expect(plugin.enforce).toBe('post');
});

it('should return null for non-SSR builds', () => {
it('should return null for non-SSR transforms', () => {
const plugin = makeServerBuildCapturePlugin();

const result = (plugin as any).transform(SERVER_BUILD_CODE, SERVER_BUILD_MODULE_ID, { ssr: false });

expect(result).toBeNull();
});

it('should return null when no options are passed', () => {
const plugin = makeServerBuildCapturePlugin();
(plugin as any).configResolved({ build: { ssr: false } });

const result = (plugin as any).transform(SERVER_BUILD_CODE, SERVER_BUILD_MODULE_ID);

Expand All @@ -22,18 +29,18 @@ describe('makeServerBuildCapturePlugin', () => {

it('should return null for non-server-build modules in SSR mode', () => {
const plugin = makeServerBuildCapturePlugin();
(plugin as any).configResolved({ build: { ssr: true } });

const result = (plugin as any).transform('export function helper() {}', 'src/utils.ts');
const result = (plugin as any).transform('export function helper() {}', 'src/utils.ts', { ssr: true });

expect(result).toBeNull();
});

it('should inject capture snippet into the server build module in SSR mode', () => {
// `options.ssr` is true for the SSR module in both dev (`react-router dev`) and production builds,
// so a single assertion covers both cases - the plugin cannot (and need not) distinguish them.
it('should inject capture snippet into the server build module for SSR transforms', () => {
const plugin = makeServerBuildCapturePlugin();
(plugin as any).configResolved({ build: { ssr: true } });

const result = (plugin as any).transform(SERVER_BUILD_CODE, SERVER_BUILD_MODULE_ID);
const result = (plugin as any).transform(SERVER_BUILD_CODE, SERVER_BUILD_MODULE_ID, { ssr: true });

expect(result).not.toBeNull();
expect(result.code).toContain(SERVER_BUILD_CODE);
Expand Down
Loading