fix(start-plugin-core): stop eclipsing user's rsbuild output.assetPrefix#7828
fix(start-plugin-core): stop eclipsing user's rsbuild output.assetPrefix#7828alexandre-marchina wants to merge 2 commits into
Conversation
📝 WalkthroughWalkthroughThe Rsbuild environment planner now accepts an optional asset prefix, preserves explicit user configuration, avoids writing a default client override, and keeps the SSR asset prefix unset. Tests cover default, path, automatic, and URL values. ChangesAsset prefix handling
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/start-plugin-core/tests/rsbuild/output-directory.test.ts (1)
200-207: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAssert that
assetPrefixis absent for all server environments.The Rsbuild environment plan statically adds the base
serverenvironment and dynamically adds thessr(provider) environment based on the configuration. You can optionally strengthen this test by asserting that neither of the server environments receives anassetPrefix.♻️ Proposed refactor
test('server environment never gets an assetPrefix', () => { const plan = createRsbuildEnvironmentPlan({ ...baseOptions, assetPrefix: 'https://cdn.example.com/', }) expect(plan.environments.ssr!.output?.assetPrefix).toBeUndefined() + expect(plan.environments.server!.output?.assetPrefix).toBeUndefined() })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/start-plugin-core/tests/rsbuild/output-directory.test.ts` around lines 200 - 207, Strengthen the server asset-prefix test around createRsbuildEnvironmentPlan by asserting that assetPrefix is undefined for both the statically added server environment and the dynamically configured ssr environment. Keep the existing CDN input and assertions for non-server behavior unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/start-plugin-core/tests/rsbuild/output-directory.test.ts`:
- Around line 200-207: Strengthen the server asset-prefix test around
createRsbuildEnvironmentPlan by asserting that assetPrefix is undefined for both
the statically added server environment and the dynamically configured ssr
environment. Keep the existing CDN input and assertions for non-server behavior
unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f94d03c3-2333-4b1f-8697-838aa3be5fab
📒 Files selected for processing (3)
packages/start-plugin-core/src/rsbuild/planning.tspackages/start-plugin-core/src/rsbuild/plugin.tspackages/start-plugin-core/tests/rsbuild/output-directory.test.ts
|
We only review implementation PRs after the underlying issue has been reproduced, triaged, and the scope has been agreed with a maintainer. Please include a minimal reproduction in the PR and wait for maintainer confirmation before submitting a fix. I’m closing this PR for now; we can revisit it once the issue is confirmed |
Fixes #7827.
Summary
This PR uses
api.getRsbuildConfig('original')to detect real user intent, and only writes to the client environment when the user setoutput.assetPrefixorserver.base. Theserver.baseprecedence path is preserved.Summary by CodeRabbit