fix: resolve vite via @nuxt/vite-builder under strict isolation#1283
fix: resolve vite via @nuxt/vite-builder under strict isolation#1283
Conversation
commit: |
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 35 minutes and 22 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe PR updates package version resolution and builder detection. banner.ts: Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/nuxi/test/unit/utils/banner.spec.ts (1)
13-16: Add coverage for the@nuxt/vite-builderalias path as well.Both
'vite'and'@nuxt/vite-builder'hit the same switch branch; asserting both reduces regression risk.🧪 Suggested test addition
describe('getBuilder', () => { // regression for banner crash under pnpm globalVirtualStore // where vite is unreachable from cwd — getPkgJSON returns null it('does not throw when vite package.json cannot be resolved', () => { expect(() => getBuilder('/any', 'vite')).not.toThrow() expect(getBuilder('/any', 'vite')).toMatchObject({ name: 'Vite', version: '' }) + expect(() => getBuilder('/any', '@nuxt/vite-builder')).not.toThrow() + expect(getBuilder('/any', '@nuxt/vite-builder')).toMatchObject({ name: 'Vite', version: '' }) }) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/nuxi/test/unit/utils/banner.spec.ts` around lines 13 - 16, Add a second assertion in the existing test case to cover the alias path '@nuxt/vite-builder' as well: call getBuilder('/any', '@nuxt/vite-builder') and assert it does not throw and matches { name: 'Vite', version: '' }, mirroring the existing assertions for getBuilder('/any', 'vite') in the same it block to ensure both branches hitting the Vite case are covered.packages/nuxi/src/utils/banner.ts (1)
21-21: Consider avoiding empty builder versions in returned metadata.Line 21 can return
version: '', which flows intopackages/nuxi/src/commands/info.ts(Line 112) and renders asvite@. Consider either a non-empty sentinel or formatter-side omission of@when version is empty.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/nuxi/src/utils/banner.ts` at line 21, The builder metadata currently returns version: '' when pkgJSON.version is missing, causing consumers to render bare "vite@" strings; update the return object in banner.ts (the object with name and version) to use a non-empty sentinel or omit the property when absent — e.g., set version: pkgJSON?.version ?? undefined or only include version when truthy so downstream code (the info rendering) can skip the "@".
🤖 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/nuxi/src/utils/banner.ts`:
- Line 21: The builder metadata currently returns version: '' when
pkgJSON.version is missing, causing consumers to render bare "vite@" strings;
update the return object in banner.ts (the object with name and version) to use
a non-empty sentinel or omit the property when absent — e.g., set version:
pkgJSON?.version ?? undefined or only include version when truthy so downstream
code (the info rendering) can skip the "@".
In `@packages/nuxi/test/unit/utils/banner.spec.ts`:
- Around line 13-16: Add a second assertion in the existing test case to cover
the alias path '@nuxt/vite-builder' as well: call getBuilder('/any',
'@nuxt/vite-builder') and assert it does not throw and matches { name: 'Vite',
version: '' }, mirroring the existing assertions for getBuilder('/any', 'vite')
in the same it block to ensure both branches hitting the Vite case are covered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3e6e987f-ba2b-4d40-a0ae-4a760362b829
📒 Files selected for processing (2)
packages/nuxi/src/utils/banner.tspackages/nuxi/test/unit/utils/banner.spec.ts
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1283 +/- ##
=======================================
Coverage ? 44.93%
=======================================
Files ? 48
Lines ? 1144
Branches ? 326
=======================================
Hits ? 514
Misses ? 514
Partials ? 116 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
would you be able to go ahead and address the root cause? 🙏 I did some tinkering on the plane just now and I'm pretty sure we need to be able to resolve nuxt -> @nuxt/vite-builder -> vite instead of the current approach of directly trying to resolve vite. |
It was just a stopgap. I felt it was urgent to stop the crash first since it's a pretty bad experience for users. Already looking into the nuxt -> @nuxt/vite-builder -> vite path now. I'll update this PR |
BellYun
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/nuxi/src/utils/versions.ts`:
- Around line 24-38: The search order currently pushes the resolved viaPath
after existing roots so resolution still prefers cwd/Nuxt; change the logic that
handles options?.via in the block that iterates roots so the resolved viaPath
(from resolveModulePath(options.via, { from, try: true })) is placed before the
generic roots — e.g. insert/unshift the viaPath into searchFrom (or build
searchFrom starting with viaPath) and then break — ensuring the via root is
prioritized when the subsequent loop over searchFrom resolves
`${pkg}/package.json`.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 705b7b64-55c5-427a-bd67-cfe58fe7c4f9
📒 Files selected for processing (3)
packages/nuxi/src/utils/banner.tspackages/nuxi/src/utils/versions.tspackages/nuxi/test/unit/utils/banner.spec.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/nuxi/src/utils/banner.ts
- packages/nuxi/test/unit/utils/banner.spec.ts
BellYun
force-pushed
the
fix/banner-null-guard
branch
3 times, most recently
from
d5121fd to
d1d01e3
Compare
BellYun
force-pushed
the
fix/banner-null-guard
branch
from
d1d01e3 to
012d0f6
Compare
3 participants
🔗 Linked issue
Refs nuxt/nuxt#34809 — this covers the CLI side. The devtools side is in nuxt/devtools#986.
📚 Description
Resolves
vitevianuxt -> @nuxt/vite-builder -> viteinstead of resolving it directly from cwd.Under pnpm 11's enableGlobalVirtualStore, vite is not reachable from CWD or nuxt itself. However, since @nuxt/vite-builder declares vite as a direct dependency, the lookup now works correctly through that path.