-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
feat(tanstackstart-react): Add file exclude to configure middleware auto-instrumentation #19007
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Conversation
This reverts commit 330d7f8.
2ca2145 to
e0df795
Compare
e0df795 to
e1276d9
Compare
e1276d9 to
864d5b0
Compare
node-overhead report 🧳Note: This is a synthetic benchmark with a minimal express app and does not necessarily reflect the real-world performance impact in an application.
|
…uto-instrumentation
86f5879 to
07f4f30
Compare
Codecov Results 📊Generated by Codecov Action |
| import type { Plugin } from 'vite'; | ||
|
|
||
| type AutoInstrumentMiddlewareOptions = { | ||
| enabled?: boolean; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was an unnecessary option since we just don't add this plugin if it's not enabled, so I removed it
| plugins.push(makeAutoInstrumentMiddlewarePlugin({ enabled: true, debug: options.debug })); | ||
| const autoInstrumentConfig = options.autoInstrumentMiddleware; | ||
| const isDisabled = autoInstrumentConfig === false; | ||
| const excludePatterns = typeof autoInstrumentConfig === 'object' ? autoInstrumentConfig.exclude : undefined; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: The check typeof autoInstrumentConfig === 'object' is insufficient because typeof null is also 'object', which can cause a crash if autoInstrumentConfig is null.
Severity: MEDIUM
Suggested Fix
Update the conditional check to explicitly handle the null case by adding && autoInstrumentConfig !== null. The corrected line should be: const excludePatterns = typeof autoInstrumentConfig === 'object' && autoInstrumentConfig !== null ? autoInstrumentConfig.exclude : undefined;.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: packages/tanstackstart-react/src/vite/sentryTanstackStart.ts#L76
Potential issue: The code checks if `autoInstrumentConfig` is an object using `typeof
autoInstrumentConfig === 'object'`. However, in JavaScript, `typeof null` also evaluates
to `'object'`. If a user provides `null` for the `autoInstrumentMiddleware` option, for
instance through a dynamic JavaScript configuration, this check will pass. The
subsequent attempt to access `autoInstrumentConfig.exclude` will result in a `TypeError:
Cannot read property 'exclude' of null`, causing the Vite build process to crash. This
pattern is inconsistent with other parts of the codebase, which explicitly check for
`null` in similar situations.
Did we get this right? 👍 / 👎 to inform future reviews.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
| }) as PluginWithTransform; | ||
| const result = plugin.transform(createStartFile, '/app/routes/admin/start.ts'); | ||
| expect(result).toBeNull(); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing integration or E2E tests for feat PR
Low Severity · Bugbot Rules
This feat PR adds the file exclude functionality for middleware auto-instrumentation but only includes unit tests. Per the review rules: "When reviewing a feat PR, check if the PR includes at least one integration or E2E test. If neither of the two are present, add a comment, recommending to add one." The existing E2E test suite at dev-packages/e2e-tests/test-applications/tanstackstart-react/ could be extended to verify the exclude patterns work correctly in a real build scenario.
Additional Locations (1)
| transform(code, id) { | ||
| if (!enabled) { | ||
| // Skip if not a TS/JS file | ||
| if (!/\.(ts|tsx|js|jsx|mjs|mts)$/.test(id)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
l: This was here before - still, maybe it makes more sense to have a path.extname(id) instead and check against this, it might be faster than regex or simply an endsWith (might be more effort, since this one only takes a string)
| // file matches exclude patterns, skip | ||
| if (debug) { | ||
| // eslint-disable-next-line no-console | ||
| console.log(`[Sentry] Skipping auto-instrumentation for excluded file: ${id}`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
l: I think this should be rather debug.log, where debug comes from @sentry/core


Depends on (i.e. do not review until the following PR is merged): #19001
This PR extends the autoInstrumentMiddleware option to support fine-grained control over which files are auto-instrumented. Previously, this option only supported a boolean setting. Now you can pass an object with an exclude array to selectively skip specific files from auto-instrumentation.
Closes #18985