Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 954e19b34f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return e.path; | ||
| } | ||
|
|
||
| return path.relative(baseDir, e.path).split(path.sep).join('/'); |
There was a problem hiding this comment.
Avoid rewriting shard file paths to relative paths here: downstream Node consumers only fetch sharded data when entries are URLs or absolute file paths, so relative strings from path.relative(...) are treated as plain values and never loaded. That breaks commands like analyze/bundle-diff that call fetchShardingFiles(...) with loadShardingFile(...), causing manifest data keys (e.g. chunkGraph, moduleGraph) to remain string arrays instead of parsed payloads.
Useful? React with 👍 / 👎.
| (file): file is string => | ||
| typeof file === 'string' && file.endsWith('.js'), |
There was a problem hiding this comment.
This filter only keeps files ending in .js, so emitted size is silently undercounted for valid JS chunk outputs like .mjs/.cjs (or other configured JS-like extensions). In those builds, emittedSize and path totals are skewed toward zero, which distorts chunk-group ranking and warning severity in the new report/UI.
Useful? React with 👍 / 👎.
| return Object.fromEntries( | ||
| Object.entries(data).map(([key, value]) => { |
There was a problem hiding this comment.
The new normalization path assumes manifest.data exists and immediately calls Object.entries(data). fetchJSONByUrl still intentionally maps HTML responses to {} (SPA fallback/error page case), so this now throws a TypeError during normalization instead of allowing later fallback handling, turning a recoverable manifest fetch error into an immediate runtime failure.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This PR introduces a new “chunk group graph” report that is produced during the build, exposed via a new server API (/api/graph/chunk-group), and rendered in the Bundle Size UI to help analyze load paths and removable module opportunities.
Changes:
- Add
/api/graph/chunk-groupAPI + SDK/types support for a richer chunk-group graph payload. - Build the chunk-group graph report in
@rsdoctor/core(including path analysis + inherited removable-size deduction) with regression tests. - Add a new Bundle Size UI tab that renders the chunk group graph with search, ranked opportunities, and path details.
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/utils/src/common/data/index.ts | Data loader wiring for the new chunk-group graph API response. |
| packages/types/src/sdk/server/apis/index.ts | Adds the new API enum entry and response typing. |
| packages/types/src/sdk/server/apis/graph.ts | Extends Graph API response typings with chunk-group graph data. |
| packages/types/src/sdk/package.ts | Extends OtherReports to carry the chunk-group graph report. |
| packages/types/src/sdk/instance.ts | Adds reportOtherReports() to the builder SDK instance interface. |
| packages/types/src/sdk/index.ts | Exports the new chunk-group SDK types. |
| packages/types/src/sdk/chunk-group.ts | Defines the chunk group graph payload types. |
| packages/sdk/src/sdk/utils/upload.ts | Writes sharding file paths relative to the output dir for portability. |
| packages/sdk/src/sdk/server/apis/graph.ts | Adds the server route handler for /api/graph/chunk-group. |
| packages/sdk/src/sdk/sdk/index.ts | Stores and exposes otherReports and adds reportOtherReports(). |
| packages/sdk/src/sdk/sdk/core.ts | Uses relative sharding paths when writing the cloud manifest. |
| packages/core/tests/build/utils/chunk-group-report.test.ts | Regression tests for path semantics and removable/inherited module logic. |
| packages/core/src/inner-plugins/plugins/ensureModulesChunkGraph.ts | Generates and reports the chunk-group graph report during build completion. |
| packages/core/src/build-utils/build/chunks/index.ts | Exports the new chunk-group report builder. |
| packages/core/src/build-utils/build/chunks/chunkGroupReport.ts | Implements chunk group graph report generation + path analysis. |
| packages/components/src/utils/request.ts | Normalizes manifest sharding URLs relative to the manifest URL. |
| packages/components/src/utils/data/remote.ts | Falls back to data when cloudData isn’t present in manifests. |
| packages/components/src/pages/BundleSize/components/index.tsx | Adds a new “Chunk Group Graph” tab with lazy rendering. |
| packages/components/src/pages/BundleSize/components/chunk-group.tsx | Implements the chunk group graph visualization + details panel UI. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
|
|
||
| return ( | ||
| <Tooltip title="非必要依赖过多" key={path.id}> |
There was a problem hiding this comment.
The tooltip title string is in Chinese ("非必要依赖过多") while the surrounding UI copy is English. Please translate this to English or route it through the project’s i18n/localization mechanism so the UI language is consistent.
| <Tooltip title="非必要依赖过多" key={path.id}> | |
| <Tooltip title="Too many unnecessary dependencies" key={path.id}> |
| let emittedSize = 0; | ||
| const files = toArray<string>(chunk?.files).filter( | ||
| (file): file is string => | ||
| typeof file === 'string' && file.endsWith('.js'), |
There was a problem hiding this comment.
chunk.files is filtered with file.endsWith('.js'), which will miss other JS-like outputs (e.g. .mjs, .cjs) and make emittedSize / files undercount for those builds. Consider using the existing JS_LIKE_EXTENSIONS set (or a shared helper) to detect JS-like asset extensions instead of hard-coding .js.
| typeof file === 'string' && file.endsWith('.js'), | |
| typeof file === 'string' && | |
| JS_LIKE_EXTENSIONS.has(path.extname(file).toLowerCase()), |
| const { paths: nodePaths, truncated } = findPathsToTarget( | ||
| node.id, | ||
| entryNodeIds, | ||
| forward, | ||
| fallbackNodeId, | ||
| CHUNK_GROUP_PATH_LIMIT, | ||
| ); |
There was a problem hiding this comment.
findPathsToTarget performs a DFS over the chunk-group DAG and is invoked once per node. On large graphs this can become very expensive (potentially exploring a huge number of partial paths before hitting CHUNK_GROUP_PATH_LIMIT). Consider computing paths more incrementally/cached (e.g., dynamic programming from entry nodes, or generating up to N representative paths per node once and reusing them) and/or adding additional guards like a max depth / node-visit budget per target.
| for (const node of localNodes) { | ||
| if (!topologicalOrder.includes(node.id)) { | ||
| topologicalOrder.push(node.id); | ||
| } | ||
| } |
There was a problem hiding this comment.
topologicalOrder.includes(node.id) inside the for (const node of localNodes) loop makes this step O(n²). Since this runs for every build and localNodes can be large, it’s worth switching to a Set for membership checks (or tracking pushed ids) and then appending the missing nodes.
| <Tag color={selectedNode.removableJSSize ? 'gold' : 'default'}> | ||
| {formatSize(selectedNode.removableJSSize)} removable on path | ||
| </Tag> | ||
| <Tag color={selectedNode.localRemovableJSSize ? 'default' : 'default'}> |
There was a problem hiding this comment.
This conditional is redundant: color={selectedNode.localRemovableJSSize ? 'default' : 'default'} always evaluates to 'default'. Either drop the ternary or use a distinct color when localRemovableJSSize is non-zero (to match the other size tags).
| <Tag color={selectedNode.localRemovableJSSize ? 'default' : 'default'}> | |
| <Tag color={selectedNode.localRemovableJSSize ? 'gold' : 'default'}> |
| topUnnecessaryModules: ChunkGroupGraphModuleData[]; | ||
| } | ||
|
|
||
| export interface ChunkGroupGraphNodeData { |
There was a problem hiding this comment.
The chunk group data needs to be reused with the existing chunk graph; otherwise, the data volume will be too large.
| @@ -0,0 +1,117 @@ | |||
| export interface ChunkGroupGraphChunkData { | |||
There was a problem hiding this comment.
Do you need all Chunk group data? Try to streamline them.
|
|
||
| if (_this.options.features.bundle) { | ||
| const chunkGroupGraph = ChunksBuildUtils.buildChunkGroupGraphReport( | ||
| stats.compilation, |
There was a problem hiding this comment.
Try not to get the data from stats. In rspack, we should obtain the data from rspack_rsdoctor_plugin as much as possible. The intention is to abandon all logic that retrieves data from stats.
Summary
/api/graph/chunk-group, including overview stats, ranked group ids, path summaries, import origins, and search text for downstream rendering and analysisTesting
pnpm --filter @rsdoctor/types buildpnpm --filter @rsdoctor/core buildpnpm exec rstest run packages/core/tests/build/utils/chunk-group-report.test.tspnpm --filter @rsdoctor/components buildpnpm --filter @rsdoctor/client build(currently fails in this workspace because@rsdoctor/componentspackage resolution is broken during client bundling, e.g.@rsdoctor/components/utilsand../storage.mjs)