Repository navigation
Conversation
size-limit report 📦Compared: 7e1b13b (PR test merge using develop at a01cfca) Against: a01cfca (develop) · base run #62876 ✅ Baseline matches: this diff contains only this PR's changes.
|
|
bugbot run |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
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.
Reviewed by Cursor Bugbot for commit 26e519d. Configure here.
| getBuildLogger(userSentryOptions.silent).warn( | ||
| `[@sentry/nextjs] Found ${misplacedFile}, but Next.js production builds only pick up the instrumentation file next to your ${routerDir} directory. Move it to ${path.join(loadedDir, path.basename(misplacedFile))}, or the Sentry SDK may not be initialized on the server in production.`, | ||
| ); | ||
| } |
There was a problem hiding this comment.
Feat lacks integration or E2E test
Low Severity
This feat is covered only by unit tests that call warnIfMisplacedInstrumentationFile against a temp fixture. This is flagged because the review rules ask feat PRs to include at least one integration or E2E test, for example asserting that next build actually emits the warning for a misplaced instrumentation.ts.
Additional Locations (1)
Triggered by project rule: PR Review Guidelines for Cursor Bot
Reviewed by Cursor Bugbot for commit 26e519d. Configure here.
nicohrubec
left a comment
nicohrubec
left a comment
There was a problem hiding this comment.
there also seems to be a build time log for webpack that checks for missing instrumentation files. we could probably remove this check then and only keep this one so we don't get double warnings?
| const exists = (relativePath: string): boolean => fs.existsSync(path.join(projectDir, relativePath)); | ||
|
|
||
| // Same lookup as Next.js' `findPagesDir`: `pages` before `app`, and `<name>` before `src/<name>`. | ||
| const routerDir = ['pages', 'app'].flatMap(name => [name, path.join('src', name)]).find(exists); |
There was a problem hiding this comment.
| const routerDir = ['pages', 'app'].flatMap(name => [name, path.join('src', name)]).find(exists); | |
| const routerDir = ['pages', path.join('src', 'pages'), 'app', path.join('src', 'app')].find(exists); |
just a suggestion for readability, imo this is a bit more explicit and easier to understand at first glance
| return; | ||
| } | ||
|
|
||
| const loadedDir = path.dirname(routerDir); |
There was a problem hiding this comment.
do we need to show the user in what wrong location the instrumentation file is? as in could we not just simplify this logic to:
- get the path where we expect the instrumentation file to live
- check whether its there
- if not log something like "Could not find a server instrumentation file next to
{router_dir}. Create{router_dir}/instrumentation.{extension}, or move your existing instrumentation file there."
I'd assume a user will know where he placed this file or can easily find out and less logic that might be buggy
This branch has not been deployed
2 participants
With
src/appand a rootinstrumentation.ts(orapp/andsrc/instrumentation.ts),next builddoes not pick up the file. It still runs in dev and innext startwith Turbopack, so local testing passes. But on Vercel and in standalone output, the server SDK never initializes.pages/app, withpageExtensions).This is only a warning because Next.js decides which file it deploys, we can only warn users.
Upstream: vercel/next.js#68740 (other users hit the same cause)
Fixes #17024
Docs: getsentry/sentry-docs#20125