feat: simplify reactlynx external bundle configuration#2370
Merged
Conversation
🦋 Changeset detectedLatest commit: bab7eb6 The changes in this PR will be included in the next version bump. This PR includes changesets to release 5 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds an extensible externals-preset system (built-in Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
❌ 1 Tests Failed:
View the full list of 2 ❄️ flaky test(s)
To view more test analytics, go to the Test Analytics Dashboard |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/rspeedy/lynx-bundle-rslib-config/src/externalBundleRslibConfig.ts (1)
71-73: ExportExternalsPresetstype from the public API.The
ExternalsPresetsinterface is used byOutputConfig.externalsPresets(line 112) but is not re-exported frompackages/rspeedy/lynx-bundle-rslib-config/src/index.ts. Consumers cannot reference this type without importing from an internal path.Add the export to
index.ts:export type { EncodeOptions, ExternalsPresets } from './externalBundleRslibConfig.js'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rspeedy/lynx-bundle-rslib-config/src/externalBundleRslibConfig.ts` around lines 71 - 73, Add ExternalsPresets to the package public exports so consumers can reference the type used by OutputConfig.externalsPresets; update the module's public entry (the index export list) to export the type ExternalsPresets (and match the existing pattern used for EncodeOptions) by re-exporting it from externalBundleRslibConfig (the file that declares ExternalsPresets) so external callers can import the type from the package root rather than an internal path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/rspeedy/lynx-bundle-rslib-config/src/externalBundleRslibConfig.ts`:
- Around line 71-73: Add ExternalsPresets to the package public exports so
consumers can reference the type used by OutputConfig.externalsPresets; update
the module's public entry (the index export list) to export the type
ExternalsPresets (and match the existing pattern used for EncodeOptions) by
re-exporting it from externalBundleRslibConfig (the file that declares
ExternalsPresets) so external callers can import the type from the package root
rather than an internal path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 82f69c14-9d0a-4fc9-bd2a-60c4878a7b91
📒 Files selected for processing (4)
.changeset/four-forks-watch.mdexamples/react-externals/rslib-comp-lib.config.tspackages/rspeedy/lynx-bundle-rslib-config/src/externalBundleRslibConfig.tspackages/rspeedy/lynx-bundle-rslib-config/test/external-bundle.test.ts
Merging this PR will improve performance by 26.82%
|
| Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|
| ⚡ | 002-hello-reactLynx-destroyBackground |
861.5 µs | 679.3 µs | +26.82% |
| ⚡ | transform 1000 view elements |
47.3 ms | 43.6 ms | +8.54% |
Comparing feat/externals-preset (bab7eb6) with main (9ad2dc3)
Footnotes
-
21 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩
Web Explorer#8499 Bundle Size — 728.6KiB (0%).bab7eb6(current) vs 9ad2dc3 main#8494(baseline) Bundle metrics
|
| Current #8499 |
Baseline #8494 |
|
|---|---|---|
 Initial JS |
43.3KiB |
43.3KiB |
| Initial CSS |
2.16KiB |
2.16KiB |
| Cache Invalidation |
0% |
0% |
| Chunks |
8 |
8 |
| Assets |
10 |
10 |
| Modules |
149 |
149 |
| Duplicate Modules |
11 |
11 |
| Duplicate Code |
34.68% |
34.68% |
| Packages |
3 |
3 |
| Duplicate Packages |
0 |
0 |
Bundle size by type no changes
| Current #8499 |
Baseline #8494 |
|
|---|---|---|
| Other |
384.4KiB |
384.4KiB |
| JS |
342.04KiB |
342.04KiB |
| CSS |
2.16KiB |
2.16KiB |
Bundle analysis report Branch feat/externals-preset Project dashboard
Generated by RelativeCI Documentation Report issue
HuJean
previously approved these changes
Mar 25, 2026
upupming
force-pushed
the
feat/externals-preset
branch
7 times, most recently
from
8702054 to
1e4d139
Compare
upupming
force-pushed
the
feat/externals-preset
branch
3 times, most recently
from
eecfa75 to
8f0bb56
Compare
upupming
force-pushed
the
feat/externals-preset
branch
from
8f0bb56 to
a2e1c21
Compare
upupming
force-pushed
the
feat/externals-preset
branch
3 times, most recently
from
72c3341 to
7466da1
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
packages/rspeedy/plugin-external-bundle/src/index.ts (2)
636-662: Prune managed assets against the final merged externals map.
managedBundleAssetsstarts from every preset-provided bundle and then adds explicit ones, but it never removes preset assets that the mergedexternalsobject no longer references. Fullurloverrides can still emit or serve an unused local bundle. Consider collecting the referencedbundlePaths from finalexternalsand filtering the asset map before wiring the middleware/plugin.Also applies to: 835-846
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rspeedy/plugin-external-bundle/src/index.ts` around lines 636 - 662, getManagedBundleAssets currently seeds assets from presetManagedAssets and adds explicit externals but never removes preset entries that the final merged externals no longer reference; collect the final set of referenced bundlePath values by iterating options.externals (use normalizePluginExternal and skip externals with external.url), then filter the initial assets Map to only keep keys present in that referenced set (use normalizeBundlePath for key comparisons) before returning the Map; apply the same pruning change to the other identical asset-building block that uses the same externals/normalizePluginExternal logic.
34-107: Avoid maintaining the ReactLynx request map in two places.This template duplicates the request list and
libraryNamesegments already defined inpackages/rspeedy/lynx-bundle-rslib-config/src/externalBundleRslibConfig.ts. A future add/remove on only one side will desynchronize external-bundle production from runtime loading. Consider extracting a shared request→library-name constant and deriving both shapes from it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rspeedy/plugin-external-bundle/src/index.ts` around lines 34 - 107, Extract the duplicated module→libraryName/section data into a single exported constant (e.g. REACT_LYNX_MODULE_MAP) and have both reactLynxExternalTemplate and the data in externalBundleRslibConfig derive their shapes from that constant; specifically, replace the hard-coded entries in reactLynxExternalTemplate with a map/transform over the shared REACT_LYNX_MODULE_MAP to attach background, mainThread, async and any url fields, and update the other module (externalBundleRslibConfig) to import and consume the same REACT_LYNX_MODULE_MAP so additions/removals are made in one place.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/react-umd/README.md`:
- Around line 149-150: The README incorrectly claims the consumer API is "the
same extension pattern"; update the wording so it clarifies that
pluginExternalBundle uses the public ExternalsPresetDefinition shape
(ExternalsPresetDefinition) which expects resolveExternals and
resolveManagedAssets functions rather than the producer's inline externals
object. Replace the phrase "the same extension pattern" and add a short note
that consumers must implement resolveExternals/resolveManagedAssets (not the
inline externals shape) to ensure the config will type-check with
pluginExternalBundle.
In `@packages/rspeedy/plugin-external-bundle/src/index.ts`:
- Around line 339-340: normalizeBundlePath currently only strips leading slashes
which allows path traversal and leading-slash bypasses; update
normalizeBundlePath to (1) trim leading slashes, (2) run a POSIX normalize to
collapse "."/".." segments (e.g., path.posix.normalize), and (3) reject or throw
if the normalized result begins with "../" or equals ".." (ensuring no escape
from externalBundleRoot). Then ensure normalizePluginExternal (and any code that
returns object-type externals) calls normalizeBundlePath for the bundlePath
field so all explicit externals are validated before being used with
path.resolve/externalBundleRoot.
In `@packages/rspeedy/plugin-external-bundle/test/index.test.ts`:
- Around line 13-16: Define __dirname at the top of the ESM test file before any
use as a cwd: import file URL utilities and derive a filesystem directory from
import.meta.url (use fileURLToPath + path.dirname) and then use that __dirname
wherever cwd: __dirname is passed (e.g., in the test calls around lines
referenced). Ensure you add the imports needed for fileURLToPath (from
'node:url') and create the const __dirname =
path.dirname(fileURLToPath(import.meta.url)) so all 15 cwd: __dirname usages
succeed in this ESM module.
In `@packages/webpack/externals-loading-webpack-plugin/src/index.ts`:
- Around line 459-463: The current runtime expression that builds fetchExpr
concatenates compiler.webpack.RuntimeGlobals.publicPath and
normalizedExternal.bundlePath raw, which can produce missing slashes (e.g.,
"...assetsreact..."); update the fetch expression so it normalizes the separator
at runtime—ensure exactly one "/" between publicPath and bundlePath (trim
trailing slashes from RuntimeGlobals.publicPath and leading slashes from
normalizedExternal.bundlePath, then join with a single "/") when
normalizedExternal.url is falsy; modify the logic around
fetchExpr/normalizedExternal to perform this normalized join using the same
slash-normalization approach as the build-time asset join.
---
Nitpick comments:
In `@packages/rspeedy/plugin-external-bundle/src/index.ts`:
- Around line 636-662: getManagedBundleAssets currently seeds assets from
presetManagedAssets and adds explicit externals but never removes preset entries
that the final merged externals no longer reference; collect the final set of
referenced bundlePath values by iterating options.externals (use
normalizePluginExternal and skip externals with external.url), then filter the
initial assets Map to only keep keys present in that referenced set (use
normalizeBundlePath for key comparisons) before returning the Map; apply the
same pruning change to the other identical asset-building block that uses the
same externals/normalizePluginExternal logic.
- Around line 34-107: Extract the duplicated module→libraryName/section data
into a single exported constant (e.g. REACT_LYNX_MODULE_MAP) and have both
reactLynxExternalTemplate and the data in externalBundleRslibConfig derive their
shapes from that constant; specifically, replace the hard-coded entries in
reactLynxExternalTemplate with a map/transform over the shared
REACT_LYNX_MODULE_MAP to attach background, mainThread, async and any url
fields, and update the other module (externalBundleRslibConfig) to import and
consume the same REACT_LYNX_MODULE_MAP so additions/removals are made in one
place.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ab64345d-bc17-49be-9151-e73ce91d2258
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (31)
.changeset/blue-emus-brake.md.github/externals-loading-webpack-plugin.instructions.md.github/lynx-bundle-rslib-config.instructions.md.github/plugin-external-bundle.instructions.mdexamples/react-externals/.gitignoreexamples/react-externals/README.mdexamples/react-externals/lynx.config.tsexamples/react-externals/package.jsonexamples/react-externals/rslib-comp-lib.config.tsexamples/react-externals/turbo.jsonpackages/react-umd/README.mdpackages/react-umd/package.jsonpackages/react-umd/rslib.config.tspackages/react-umd/turbo.jsonpackages/rspeedy/lynx-bundle-rslib-config/README.mdpackages/rspeedy/lynx-bundle-rslib-config/etc/lynx-bundle-rslib-config.api.mdpackages/rspeedy/lynx-bundle-rslib-config/src/externalBundleRslibConfig.tspackages/rspeedy/lynx-bundle-rslib-config/src/index.tspackages/rspeedy/lynx-bundle-rslib-config/test/external-bundle.test.tspackages/rspeedy/plugin-external-bundle/README.mdpackages/rspeedy/plugin-external-bundle/etc/external-bundle-rsbuild-plugin.api.mdpackages/rspeedy/plugin-external-bundle/package.jsonpackages/rspeedy/plugin-external-bundle/src/index.tspackages/rspeedy/plugin-external-bundle/test/index.test.tspackages/rspeedy/plugin-external-bundle/test/resolve-root-path.test.tspackages/rspeedy/plugin-external-bundle/tsconfig.build.jsonpackages/rspeedy/plugin-external-bundle/tsconfig.jsonpackages/rspeedy/plugin-external-bundle/turbo.jsonpackages/webpack/externals-loading-webpack-plugin/README.mdpackages/webpack/externals-loading-webpack-plugin/etc/externals-loading-webpack-plugin.api.mdpackages/webpack/externals-loading-webpack-plugin/src/index.ts
✅ Files skipped from review due to trivial changes (18)
- examples/react-externals/.gitignore
- packages/webpack/externals-loading-webpack-plugin/etc/externals-loading-webpack-plugin.api.md
- examples/react-externals/turbo.json
- packages/rspeedy/plugin-external-bundle/package.json
- packages/react-umd/turbo.json
- packages/rspeedy/plugin-external-bundle/tsconfig.build.json
- packages/rspeedy/plugin-external-bundle/turbo.json
- .github/lynx-bundle-rslib-config.instructions.md
- packages/rspeedy/plugin-external-bundle/tsconfig.json
- .github/externals-loading-webpack-plugin.instructions.md
- packages/react-umd/rslib.config.ts
- packages/react-umd/package.json
- packages/rspeedy/lynx-bundle-rslib-config/README.md
- packages/webpack/externals-loading-webpack-plugin/README.md
- packages/rspeedy/plugin-external-bundle/README.md
- .github/plugin-external-bundle.instructions.md
- .changeset/blue-emus-brake.md
- examples/react-externals/README.md
🚧 Files skipped from review as they are similar to previous changes (1)
- examples/react-externals/rslib-comp-lib.config.ts
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/rspeedy/plugin-external-bundle/test/index.test.ts (1)
5-24:⚠️ Potential issue | 🔴 CriticalMissing
__dirnamedefinition in ESM module.This file uses
cwd: __dirnamethroughout (e.g., lines 90, 149, 195, etc.) but never defines__dirname. Since this is an ESM module ("type": "module"in package.json),__dirnameis not available globally and will cause aReferenceErrorat runtime.🐛 Proposed fix
import { existsSync, mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync, } from 'node:fs' import type { IncomingMessage, ServerResponse } from 'node:http' import { tmpdir } from 'node:os' import path from 'node:path' import { Writable } from 'node:stream' +import { fileURLToPath } from 'node:url' import { createRsbuild } from '@rsbuild/core' import { describe, expect, test } from 'vitest' import type { ExternalsLoadingPluginOptions } from '@lynx-js/externals-loading-webpack-plugin' import { ExternalsLoadingPlugin } from '@lynx-js/externals-loading-webpack-plugin' import { pluginStubLayers } from './stub-layers.plugin.js' + +const __filename = fileURLToPath(import.meta.url) +const __dirname = path.dirname(__filename)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rspeedy/plugin-external-bundle/test/index.test.ts` around lines 5 - 24, This test file uses cwd: __dirname but is an ESM module where __dirname is undefined; fix by defining __dirname at the top of the module by importing fileURLToPath from 'node:url' (and using path.dirname from 'node:path') to derive __dirname from import.meta.url, then leave the rest of the tests (the places using cwd: __dirname) unchanged so they run without ReferenceError.
🧹 Nitpick comments (1)
packages/rspeedy/lynx-bundle-rslib-config/test/external-bundle.test.ts (1)
29-54: Potential runtime error when externals returns an empty object.The
resolveExternalhelper castsoutput.externalsto a function type (lines 33-38), but pertransformExternalsin the source file, when no externals are configured, it returns{}(an empty object) rather than a function. Since{}is truthy, the check at line 41 passes, but calling it as a function at line 46 would throwTypeError: externalsResolver is not a function.This is only an issue if a test uses
resolveExternalwithout configuring any externals or presets. The current tests appear to always configure externals, but this edge case could cause confusing errors in future tests.🛡️ Defensive type check suggestion
function resolveExternal( rslibConfig: RslibConfig, request: string, ) { const externalsResolver = rslibConfig.lib[0]?.output?.externals as | (( data: { request?: string }, callback: (error?: Error, result?: unknown) => void, ) => void) | undefined + | Record<string, unknown> return new Promise<unknown>((resolve, reject) => { - if (!externalsResolver) { + if (!externalsResolver || typeof externalsResolver !== 'function') { reject(new Error('Expected output.externals to be configured')) return }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rspeedy/lynx-bundle-rslib-config/test/external-bundle.test.ts` around lines 29 - 54, The helper resolveExternal currently assumes rslibConfig.lib[0]?.output?.externals is callable; update resolveExternal to verify externalsResolver is actually a function (use typeof externalsResolver === 'function') before invoking it, and if it is not a function (e.g., an empty object), reject the Promise with a clear Error (or handle the non-function case explicitly) so calling externalsResolver({ request }, ...) cannot throw; refer to the resolveExternal function and the externalsResolver local variable to locate the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/rspeedy/plugin-external-bundle/test/index.test.ts`:
- Around line 5-24: This test file uses cwd: __dirname but is an ESM module
where __dirname is undefined; fix by defining __dirname at the top of the module
by importing fileURLToPath from 'node:url' (and using path.dirname from
'node:path') to derive __dirname from import.meta.url, then leave the rest of
the tests (the places using cwd: __dirname) unchanged so they run without
ReferenceError.
---
Nitpick comments:
In `@packages/rspeedy/lynx-bundle-rslib-config/test/external-bundle.test.ts`:
- Around line 29-54: The helper resolveExternal currently assumes
rslibConfig.lib[0]?.output?.externals is callable; update resolveExternal to
verify externalsResolver is actually a function (use typeof externalsResolver
=== 'function') before invoking it, and if it is not a function (e.g., an empty
object), reject the Promise with a clear Error (or handle the non-function case
explicitly) so calling externalsResolver({ request }, ...) cannot throw; refer
to the resolveExternal function and the externalsResolver local variable to
locate the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5208d218-8f20-4f60-9e63-21539124a3da
📒 Files selected for processing (5)
packages/react-umd/rslib.config.tspackages/rspeedy/lynx-bundle-rslib-config/src/externalBundleRslibConfig.tspackages/rspeedy/lynx-bundle-rslib-config/test/external-bundle.test.tspackages/rspeedy/plugin-external-bundle/src/index.tspackages/rspeedy/plugin-external-bundle/test/index.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/react-umd/rslib.config.ts
- packages/rspeedy/plugin-external-bundle/src/index.ts
upupming
force-pushed
the
feat/externals-preset
branch
from
7466da1 to
fe08f65
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/rspeedy/plugin-external-bundle/src/index.ts (1)
341-343:⚠️ Potential issue | 🟡 MinorPath traversal vectors remain unaddressed in
normalizeBundlePath().This was flagged in a previous review. The function only strips leading slashes but does not prevent
../traversal sequences:export function normalizeBundlePath(bundlePath: string): string { return bundlePath.replace(/^\/+/, '') // '../foo' passes through unchanged }Additionally,
normalizePluginExternal()(lines 715-721) returns object-type externals unchanged without applying any normalization to theirbundlePathfield.While the practical risk is reduced because these values come from developer configuration rather than user input, defense-in-depth is advisable for a public API that accepts arbitrary paths.
Suggested fix
export function normalizeBundlePath(bundlePath: string): string { - return bundlePath.replace(/^\/+/, '') + const normalized = path.posix.normalize( + bundlePath.replace(/\\/g, '/').replace(/^\/+/, ''), + ) + + if (normalized.startsWith('../') || normalized === '..') { + throw new Error( + `bundlePath must not traverse outside the bundle root, got "${bundlePath}".`, + ) + } + + return normalized }And apply it to object externals:
function normalizePluginExternal( request: string, external: PluginExternalConfig, ): PluginExternalValue { if (typeof external !== 'string') { - return external + return external.bundlePath + ? { ...external, bundlePath: normalizeBundlePath(external.bundlePath) } + : external }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rspeedy/plugin-external-bundle/src/index.ts` around lines 341 - 343, normalizeBundlePath currently only strips leading slashes and allows "../" traversal; update normalizeBundlePath to (1) normalize separators (convert backslashes to forward slashes), (2) split and resolve path segments removing "." and properly handling ".." so it cannot escape the root (either by ignoring ".." that would go above root or by throwing on attempted traversal), and (3) trim any remaining leading slashes; then ensure normalizePluginExternal applies this same normalization to the bundlePath field of object-type externals (references: normalizeBundlePath and normalizePluginExternal) so all externals have safe, traversal-free bundlePath values.
🧹 Nitpick comments (1)
packages/rspeedy/plugin-external-bundle/src/index.ts (1)
797-820: Consider adding error handling for the file stream.The dev server middleware creates a read stream but doesn't handle potential errors after streaming begins. If the file becomes unavailable mid-stream, the response may hang.
Suggested improvement
if (bundlePath && existsSync(bundlePath)) { res.setHeader( 'Content-Type', 'application/octet-stream', ) res.setHeader('Access-Control-Allow-Origin', '*') - createReadStream(bundlePath).pipe(res) + createReadStream(bundlePath) + .on('error', () => { + if (!res.headersSent) { + res.statusCode = 500 + } + res.end() + }) + .pipe(res) return }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rspeedy/plugin-external-bundle/src/index.ts` around lines 797 - 820, The middleware in setupMiddlewares creates a ReadStream via createReadStream(bundlePath) but doesn't attach error or close handlers, so if the file becomes unavailable mid-stream the response can hang; fix by assigning the stream to a variable, attach stream.on('error', (err) => { set res.statusCode (e.g. 500), set an error Content-Type, log the error and call res.end() } ) and stream.on('close'|'end', () => { /* cleanup if needed */ }), and also listen for res.on('close', () => stream.destroy()) to ensure the stream is destroyed if the client disconnects; update the middleware around createReadStream(bundlePath) in the setupMiddlewares function that uses localBundleAssets and existsSync to perform these handlers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/rspeedy/plugin-external-bundle/src/index.ts`:
- Around line 341-343: normalizeBundlePath currently only strips leading slashes
and allows "../" traversal; update normalizeBundlePath to (1) normalize
separators (convert backslashes to forward slashes), (2) split and resolve path
segments removing "." and properly handling ".." so it cannot escape the root
(either by ignoring ".." that would go above root or by throwing on attempted
traversal), and (3) trim any remaining leading slashes; then ensure
normalizePluginExternal applies this same normalization to the bundlePath field
of object-type externals (references: normalizeBundlePath and
normalizePluginExternal) so all externals have safe, traversal-free bundlePath
values.
---
Nitpick comments:
In `@packages/rspeedy/plugin-external-bundle/src/index.ts`:
- Around line 797-820: The middleware in setupMiddlewares creates a ReadStream
via createReadStream(bundlePath) but doesn't attach error or close handlers, so
if the file becomes unavailable mid-stream the response can hang; fix by
assigning the stream to a variable, attach stream.on('error', (err) => { set
res.statusCode (e.g. 500), set an error Content-Type, log the error and call
res.end() } ) and stream.on('close'|'end', () => { /* cleanup if needed */ }),
and also listen for res.on('close', () => stream.destroy()) to ensure the stream
is destroyed if the client disconnects; update the middleware around
createReadStream(bundlePath) in the setupMiddlewares function that uses
localBundleAssets and existsSync to perform these handlers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9f19dca6-31be-4310-8278-06f538759db3
📒 Files selected for processing (6)
.github/plugin-external-bundle.instructions.mdpackages/react-umd/rslib.config.tspackages/rspeedy/lynx-bundle-rslib-config/src/externalBundleRslibConfig.tspackages/rspeedy/lynx-bundle-rslib-config/test/external-bundle.test.tspackages/rspeedy/plugin-external-bundle/src/index.tspackages/rspeedy/plugin-external-bundle/test/index.test.ts
✅ Files skipped from review due to trivial changes (3)
- .github/plugin-external-bundle.instructions.md
- packages/rspeedy/lynx-bundle-rslib-config/test/external-bundle.test.ts
- packages/rspeedy/plugin-external-bundle/test/index.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/react-umd/rslib.config.ts
upupming
force-pushed
the
feat/externals-preset
branch
from
fe08f65 to
ed4b977
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
packages/rspeedy/plugin-external-bundle/src/index.ts (1)
341-342:⚠️ Potential issue | 🟠 MajorReject non-relative
bundlePathvalues before resolving them.Right now
normalizeBundlePath()only strips leading/, so../fooor..\\foostill survives into Line 656'spath.resolve(...).normalizePluginExternal()also forwards rawbundlePathvalues for both string shorthand and object-form externals, so leading slashes/traversal segments can still bypass the intendedpublicPath + bundlePathbehavior at runtime.🛡️ Suggested fix
export function normalizeBundlePath(bundlePath: string): string { - return bundlePath.replace(/^\/+/, '') + const normalized = path.posix.normalize( + bundlePath.replace(/\\/g, '/').replace(/^\/+/, ''), + ) + + if ( + normalized === '' || + normalized === '.' || + normalized === '..' || + normalized.startsWith('../') || + /^[a-z][a-z\d+\-.]*:/i.test(normalized) + ) { + throw new Error( + `bundlePath must be a non-empty relative path inside the external bundle root, got "${bundlePath}".`, + ) + } + + return normalized } @@ function normalizePluginExternal( request: string, external: PluginExternalConfig, ): PluginExternalValue { if (typeof external !== 'string') { - return external + return external.bundlePath + ? { ...external, bundlePath: normalizeBundlePath(external.bundlePath) } + : external } return { - bundlePath: external, + bundlePath: normalizeBundlePath(external), libraryName: request,Also applies to: 654-659, 715-724
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rspeedy/plugin-external-bundle/src/index.ts` around lines 341 - 342, normalizeBundlePath currently only strips leading slashes but allows path traversal like "../foo" or backslash variants to reach path.resolve and bypass intended publicPath concatenation; update normalizeBundlePath to reject or sanitize any non-relative-safe inputs by validating the normalized bundlePath does not contain ".." segments or backslashes and does not start with "/" or "\" (e.g., return an empty string or throw for invalid values), and update normalizePluginExternal to use this stricter validation before forwarding bundlePath (both for string shorthand and object-form externals) so that path.resolve(...) and publicPath + bundlePath behavior cannot be bypassed at runtime.packages/rspeedy/plugin-external-bundle/test/index.test.ts (1)
5-24:⚠️ Potential issue | 🔴 CriticalDefine
__dirnamebefore using it ascwd.This file passes
cwd: __dirnamein multiple tests, but nothing defines__dirnamehere. In this package's ESM setup that throws before any of the new assertions run.🩹 Suggested fix
import { tmpdir } from 'node:os' import path from 'node:path' import { Writable } from 'node:stream' +import { fileURLToPath } from 'node:url' @@ import { ExternalsLoadingPlugin } from '@lynx-js/externals-loading-webpack-plugin' import { pluginStubLayers } from './stub-layers.plugin.js' + +const __dirname = path.dirname(fileURLToPath(import.meta.url))Expected result: the search finds several
__dirnameusages, no local definition, and the package metadata shows ESM mode.#!/bin/bash set -euo pipefail echo '=== __dirname usage ===' rg -n '\bcwd:\s*__dirname\b|\b__dirname\b' packages/rspeedy/plugin-external-bundle/test/index.test.ts echo echo '=== local __dirname definition ===' rg -n 'const __dirname\b|fileURLToPath\(import\.meta\.url\)' packages/rspeedy/plugin-external-bundle/test/index.test.ts echo echo '=== package module type ===' fd '^package\.json$' packages/rspeedy/plugin-external-bundle --exec sh -c 'echo "--- $1"; sed -n "1,40p" "$1"' sh {}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rspeedy/plugin-external-bundle/test/index.test.ts` around lines 5 - 24, Define __dirname at the top of the test file before any cwd: __dirname usages by converting import.meta.url to a filename and deriving its directory; add the initialization (using fileURLToPath and path.dirname) above the tests so calls that pass cwd: __dirname (e.g., inside tests that call createRsbuild or reference pluginStubLayers) run in ESM mode without throwing. Ensure the import for fileURLToPath (from 'node:url') is added alongside existing node imports and placed before any code that uses __dirname.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/rspeedy/lynx-bundle-rslib-config/src/externalBundleRslibConfig.ts`:
- Around line 445-448: The error thrown when LAYERS is missing incorrectly
references "external-bundle-rsbuild-plugin"; update the throw inside
defineExternalBundleRslibConfig to mention this package (the current package)
instead of that plugin. Locate the check for LAYERS in
defineExternalBundleRslibConfig and change the Error message to reference the
current package name and that a DSL plugin (e.g., pluginReactLynx) must be
installed so users are pointed to the correct entry point.
---
Duplicate comments:
In `@packages/rspeedy/plugin-external-bundle/src/index.ts`:
- Around line 341-342: normalizeBundlePath currently only strips leading slashes
but allows path traversal like "../foo" or backslash variants to reach
path.resolve and bypass intended publicPath concatenation; update
normalizeBundlePath to reject or sanitize any non-relative-safe inputs by
validating the normalized bundlePath does not contain ".." segments or
backslashes and does not start with "/" or "\" (e.g., return an empty string or
throw for invalid values), and update normalizePluginExternal to use this
stricter validation before forwarding bundlePath (both for string shorthand and
object-form externals) so that path.resolve(...) and publicPath + bundlePath
behavior cannot be bypassed at runtime.
In `@packages/rspeedy/plugin-external-bundle/test/index.test.ts`:
- Around line 5-24: Define __dirname at the top of the test file before any cwd:
__dirname usages by converting import.meta.url to a filename and deriving its
directory; add the initialization (using fileURLToPath and path.dirname) above
the tests so calls that pass cwd: __dirname (e.g., inside tests that call
createRsbuild or reference pluginStubLayers) run in ESM mode without throwing.
Ensure the import for fileURLToPath (from 'node:url') is added alongside
existing node imports and placed before any code that uses __dirname.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4bdc04b3-c096-4dcb-8710-10968317ad47
📒 Files selected for processing (8)
.changeset/blue-emus-brake.md.changeset/four-forks-watch.md.github/plugin-external-bundle.instructions.mdpackages/react-umd/rslib.config.tspackages/rspeedy/lynx-bundle-rslib-config/src/externalBundleRslibConfig.tspackages/rspeedy/lynx-bundle-rslib-config/test/external-bundle.test.tspackages/rspeedy/plugin-external-bundle/src/index.tspackages/rspeedy/plugin-external-bundle/test/index.test.ts
✅ Files skipped from review due to trivial changes (2)
- packages/react-umd/rslib.config.ts
- .github/plugin-external-bundle.instructions.md
🚧 Files skipped from review as they are similar to previous changes (2)
- .changeset/blue-emus-brake.md
- packages/rspeedy/lynx-bundle-rslib-config/test/external-bundle.test.ts
packages/rspeedy/lynx-bundle-rslib-config/src/externalBundleRslibConfig.ts
Show resolved
Hide resolved
upupming
force-pushed
the
feat/externals-preset
branch
from
b77f5ea to
804a808
Compare
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: Yiming Li <yimingli.cs@gmail.com>
luhc228
reviewed
Mar 27, 2026
luhc228
previously approved these changes
Mar 27, 2026
… github.com:lynx-family/lynx-stack into feat/externals-preset
React Example#6921 Bundle Size — 237.89KiB (0%).bab7eb6(current) vs 9ad2dc3 main#6916(baseline) Bundle metrics
|
| Current #6921 |
Baseline #6916 |
|
|---|---|---|
| Initial JS |
0B |
0B |
| Initial CSS |
0B |
0B |
| Cache Invalidation |
0% |
0% |
| Chunks |
0 |
0 |
| Assets |
4 |
4 |
| Modules |
180 |
180 |
| Duplicate Modules |
71 |
71 |
| Duplicate Code |
46.4% |
46.4% |
| Packages |
2 |
2 |
| Duplicate Packages |
0 |
0 |
Bundle size by type no changes
| Current #6921 |
Baseline #6916 |
|
|---|---|---|
| IMG |
145.76KiB |
145.76KiB |
| Other |
92.13KiB |
92.13KiB |
Bundle analysis report Branch feat/externals-preset Project dashboard
Generated by RelativeCI Documentation Report issue
React MTF Example#55 Bundle Size — 207.47KiB (0%).bab7eb6(current) vs 9ad2dc3 main#50(baseline) Bundle metrics
|
| Current #55 |
Baseline #50 |
|
|---|---|---|
| Initial JS |
0B |
0B |
| Initial CSS |
0B |
0B |
| Cache Invalidation |
0% |
0% |
| Chunks |
0 |
0 |
| Assets |
3 |
3 |
| Modules |
174 |
174 |
| Duplicate Modules |
68 |
68 |
| Duplicate Code |
46.09% |
46.09% |
| Packages |
2 |
2 |
| Duplicate Packages |
0 |
0 |
Bundle size by type no changes
| Current #55 |
Baseline #50 |
|
|---|---|---|
| IMG |
111.23KiB |
111.23KiB |
| Other |
96.24KiB |
96.24KiB |
Bundle analysis report Branch feat/externals-preset Project dashboard
Generated by RelativeCI Documentation Report issue
colinaaa
approved these changes
Mar 28, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
reactlynxexternals presets to bothdefineExternalBundleRslibConfigandpluginExternalBundlebundlePath-based external loading so runtime code resolves externals frompublicPath + bundlePathinstead of hard-coded absolute URLs@lynx-js/react-umddev/prod export entries so external-bundle tooling can resolve packaged ReactLynx runtimes explicitlyreact-externalsexample to use preset-based configuration, dedicated external bundle output roots, and emitted bundle assetsTesting
pnpm --filter @lynx-js/lynx-bundle-rslib-config test -- test/external-bundle.test.tspnpm --filter @lynx-js/external-bundle-rsbuild-plugin testpnpm --filter @lynx-js/externals-loading-webpack-plugin testpnpm turbo api-extractor --filter=@lynx-js/lynx-bundle-rslib-config --filter=@lynx-js/external-bundle-rsbuild-plugin --filter=@lynx-js/externals-loading-webpack-plugin -- --localSummary by CodeRabbit
New Features
Documentation
Chores