Improve source map symbolication#1392
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
4990cfb to
3223c4f
Compare
3223c4f to
fdf5f8a
Compare
dannyhw
left a comment
dannyhw
left a comment
There was a problem hiding this comment.
codex review: Posting the main review findings inline. I also did not find targeted tests for the changed symbolicator, open-stack-frame, or Rspack remote source-map fallback behavior, so the current passing checks do not exercise the highest-risk paths.
| private getSourceContent(frame: StackFrame): string | undefined { | ||
| for (const consumer of Object.values(this.sourceMapConsumerCache)) { | ||
| try { | ||
| const source = consumer.sourceContentFor(frame.file, true); |
There was a problem hiding this comment.
codex review: getSourceContent searches every cached source map and returns the first map with frame.file, but processFrame has already discarded which generated bundle/source map produced that frame. In multi-bundle or federated stacks, duplicate logical names like [projectRoot]/src/App.tsx are plausible, so this can render a code frame from the wrong host/remote. Please carry the source-map key/consumer through processFrame and query that exact consumer for sourcesContent.
| fileUrl: frame.file, | ||
| error: (error as Error).message, | ||
| }); | ||
| processedFrames.push({ |
There was a problem hiding this comment.
codex review: This failed-symbolication fallback keeps the raw bundle frame visible with collapse: false. getCodeFrame later tries this raw URL first and returns after the first getSource failure, so one missing map can still suppress a later frame that did symbolicate correctly. Either collapse or skip failed raw frames for code-frame selection, or make getCodeFrame continue to the next frame after a source read miss.
| const filename = url.split(/[?#]/)[0].split('/').pop() ?? ''; | ||
|
|
||
| return ( | ||
| filename.match(/\.([^.]+)\.chunk\.bundle(?:\.map)?$/)?.[1] ?? |
There was a problem hiding this comment.
codex review: This regex does not match Re.Pack's standard [name].chunk.bundle filenames such as remote.chunk.bundle because it requires a dot before the captured name. The container regex also misses the MF v1 default <name>.container.bundle. When direct URL fetch is unavailable, remoteName becomes undefined and the fallback probes all remotes, which can pick a source map from the wrong remote. Please handle the default filename shapes before falling back to all remotes.
| } | ||
| } | ||
|
|
||
| private shouldSkipSourceMap(frame: InputStackFrame): boolean { |
There was a problem hiding this comment.
codex review: This skips purely by generated methodName before resolving the source. A user/app frame named beginWork, dispatchSetState, etc. would be collapsed and never symbolicated. The skip should be tied to source/package context, or applied only after the frame is known to come from React internals.
|
I want to revisit this, I'm not confident we're solving the right thing |
2 participants
WIP based on patches received for resolving errors to editor paths etc, still needs extensive testing
Summary
/open-stack-framehandling by resolving macOS editor commands earlier and mapping feature-app source paths when possibleValidation
node_modules/.bin/biome check --write packages/dev-server/src/plugins/devtools/devtoolsPlugin.ts packages/dev-server/src/plugins/symbolicate/Symbolicator.ts packages/dev-server/src/plugins/symbolicate/sybmolicatePlugin.ts packages/repack/src/commands/rspack/start.tspackages/dev-server/node_modules/.bin/tsc --noEmit -p packages/dev-server/tsconfig.jsonpackages/repack/node_modules/.bin/tsc --noEmit -p packages/repack/tsconfig.jsongit diff --checkNote: the local pre-commit hook attempted to run pnpm through the bundled runtime and stopped before checks with a non-interactive modules purge prompt, so the commit was created after running the relevant checks directly.