Conversation
commit: |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1306 +/- ##
=======================================
Coverage ? 55.42%
=======================================
Files ? 50
Lines ? 1373
Branches ? 398
=======================================
Hits ? 761
Misses ? 502
Partials ? 110 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Warning Rate limit exceeded
To continue reviewing without waiting, purchase usage credits in the billing tab. ⌛ 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 (2)
📝 WalkthroughWalkthroughThis PR implements the Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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.
Actionable comments posted: 3
🧹 Nitpick comments (2)
packages/nuxi/test/unit/commands/module/remove.spec.ts (2)
27-53: 💤 Low value
runCommandandfetchModulesspies are set at describe scope and never clearedBoth spies accumulate call counts across tests, and
fetchModuleswon't re-read its mock implementation if it needs to change per-test. Since all four tests currently rely on the same stubs this doesn't cause failures, but it's fragile. Move the spy setup intobeforeEach(or call.mockClear()/.mockReset()there alongsideupdateConfigandremoveDependency).♻️ Proposed fix
-describe('module remove', () => { - vi.spyOn(runCommands, 'runCommand').mockImplementation(vi.fn()) - vi.spyOn(utils, 'fetchModules').mockResolvedValue([...]) - - beforeEach(() => { - updateConfig.mockClear() - removeDependency.mockClear() - }) +describe('module remove', () => { + beforeEach(() => { + updateConfig.mockClear() + removeDependency.mockClear() + vi.spyOn(runCommands, 'runCommand').mockImplementation(vi.fn()) + vi.spyOn(utils, 'fetchModules').mockResolvedValue([...]) + })🤖 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/nuxi/test/unit/commands/module/remove.spec.ts` around lines 27 - 53, The spies for runCommands.runCommand and utils.fetchModules are created at describe scope and retain state between tests; move their setup into a beforeEach (or call .mockClear()/.mockReset() there) so each test gets a fresh spy and fetchModules can be re-mocked per-test; also ensure you clear mocks for related functions like updateConfig and removeDependency in the same beforeEach to avoid accumulated call counts and stale implementations when tests change behavior.
7-7: 🏗️ Heavy lift
updateConfigmock bypassesonUpdate— the config-removal logic is entirely untestedThe mock resolves immediately without ever calling the
onUpdatecallback. As a result,removedFromConfigstays empty in every test,readModuleName, the reverse-splice loop (lines 159-166 ofremove.ts), and themultiselectpicker path are never exercised. The tests only validate the uninstall-from-disk flow.To test the config-update logic, the mock should capture and invoke the callback:
♻️ Proposed fix
-const updateConfig = vi.fn(() => Promise.resolve()) +const updateConfig = vi.fn(async (opts: { onUpdate?: (config: any) => Promise<void>, onCreate?: () => boolean | void }) => { + if (opts.onUpdate) { + await opts.onUpdate({ modules: ['@nuxt/content'] }) + } +})Also applies to: 17-17
🤖 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/nuxi/test/unit/commands/module/remove.spec.ts` at line 7, The test's updateConfig mock currently resolves without invoking the module removal callback, so the config-removal path (onUpdate callback) is never exercised; change the vi.fn mock for updateConfig to accept the same args as the real function and call the provided onUpdate callback with a simulated config and return its result so removedFromConfig gets populated; specifically, update the mock referenced as updateConfig in the spec to call the onUpdate param (triggering readModuleName, the reverse-splice logic in remove.ts, and the multiselect picker path) and return the resulting Promise so the tests exercise both config-update and disk-uninstall flows.
🤖 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.
Inline comments:
In `@packages/nuxi/src/commands/module/remove.ts`:
- Around line 127-166: The code assumes config.modules is always an array
causing a TypeError when it's undefined; ensure safe handling by normalizing
config.modules to an array before reading or mutating it (e.g., if
(!Array.isArray(config.modules)) config.modules = []), then build the present
list with readModuleName from config.modules, use modules.length / multiselect
logic as-is, and safely iterate backwards over config.modules to splice and push
into removedFromConfig; this keeps the rest of the flow (multiselect, isCancel,
toRemove, readModuleName, removedFromConfig) unchanged while preventing
undefined access.
- Around line 153-166: The removal logic compares resolved npm names in toRemove
to raw config entries from readModuleName, so alias entries like 'content' won't
match and remain in nuxt.config after uninstall; update removeModules to compare
resolved names by either (A) resolving each config entry with resolveModule (or
using modulesDB/installedNames) before checking (i.e., call resolveModule on the
value returned by readModuleName and compare that resolved name to toRemove) or
(B) populate toRemove with both the original and resolved package names so the
existing loop that uses readModuleName finds matches; update references to
toRemove, readModuleName, resolveModule, removeModules, and
modulesDB/installedNames accordingly.
- Around line 235-244: The removeDependency rejection is currently only logged
and the function returns true, causing the caller to still run prepare; change
the error handling in the removeDependency call so that after logging you either
re-throw the error or return false to propagate failure to the caller (so caller
honor prepare skip when ctx.args.skipUninstall is false), and update the calling
code that checks the boolean result to skip calling prepare when false; also
confirm nypm.removeDependency's signature — if it does not accept string[] then
iterate over allToRemove and call removeDependency(name, options) for each entry
(or use Promise.all on per-name calls) instead of passing the array. Ensure
references to removeDependency, prepare, ctx.args.skipUninstall and allToRemove
are updated accordingly.
---
Nitpick comments:
In `@packages/nuxi/test/unit/commands/module/remove.spec.ts`:
- Around line 27-53: The spies for runCommands.runCommand and utils.fetchModules
are created at describe scope and retain state between tests; move their setup
into a beforeEach (or call .mockClear()/.mockReset() there) so each test gets a
fresh spy and fetchModules can be re-mocked per-test; also ensure you clear
mocks for related functions like updateConfig and removeDependency in the same
beforeEach to avoid accumulated call counts and stale implementations when tests
change behavior.
- Line 7: The test's updateConfig mock currently resolves without invoking the
module removal callback, so the config-removal path (onUpdate callback) is never
exercised; change the vi.fn mock for updateConfig to accept the same args as the
real function and call the provided onUpdate callback with a simulated config
and return its result so removedFromConfig gets populated; specifically, update
the mock referenced as updateConfig in the spec to call the onUpdate param
(triggering readModuleName, the reverse-splice logic in remove.ts, and the
multiselect picker path) and return the resulting Promise so the tests exercise
both config-update and disk-uninstall flows.
🪄 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: 69293820-fddb-4912-b24a-6ce800fe37f7
📒 Files selected for processing (3)
packages/nuxi/src/commands/module/index.tspackages/nuxi/src/commands/module/remove.tspackages/nuxi/test/unit/commands/module/remove.spec.ts
| toRemove = new Set(picked as string[]) | ||
| } | ||
| else { | ||
| toRemove = new Set(modules) | ||
| } | ||
|
|
||
| for (let i = config.modules.length - 1; i >= 0; i--) { | ||
| const name = readModuleName(config.modules[i]) | ||
| if (name && toRemove.has(name)) { | ||
| logger.info(`Removing ${colors.cyan(name)} from the ${colors.cyan('modules')}`) | ||
| config.modules.splice(i, 1) | ||
| removedFromConfig.push(name) | ||
| } | ||
| } |
There was a problem hiding this comment.
Resolved npm name in toRemove won't match alias-form config entries — leaves the project in a broken state
modules passed to removeModules are already resolved npm package names (e.g., @nuxt/content), so toRemove at line 156 contains only npm names. readModuleName reads config entries as literal strings. If a user's nuxt.config.ts contains the short alias ('content' instead of '@nuxt/content'), the comparison fails silently: the package gets uninstalled but the config entry is left intact, referencing a package that no longer exists.
The fix is to resolve config entry names through the same resolveModule logic before comparing, or to pass both original inputs and their resolved forms through to toRemove. One clean approach: pass modulesDB and installedNames into removeModules and resolve the config entry name inline:
- if (name && toRemove.has(name)) {
+ if (name && (toRemove.has(name) || toRemove.has(resolveModule(name, modulesDB, installed)))) {🤖 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/nuxi/src/commands/module/remove.ts` around lines 153 - 166, The
removal logic compares resolved npm names in toRemove to raw config entries from
readModuleName, so alias entries like 'content' won't match and remain in
nuxt.config after uninstall; update removeModules to compare resolved names by
either (A) resolving each config entry with resolveModule (or using
modulesDB/installedNames) before checking (i.e., call resolveModule on the value
returned by readModuleName and compare that resolved name to toRemove) or (B)
populate toRemove with both the original and resolved package names so the
existing loop that uses readModuleName finds matches; update references to
toRemove, readModuleName, resolveModule, removeModules, and
modulesDB/installedNames accordingly.
There was a problem hiding this comment.
Not valid.. Nuxt does not resolve aliases inside config.modules
There was a problem hiding this comment.
@Flo0806, you're right — since Nuxt requires full npm package names (or file paths) in config.modules and does not resolve short aliases there, readModuleName will always return the canonical npm name. The comparison with toRemove is therefore correct, and my concern was a false positive. Apologies for the noise!
✏️ Learnings added
Learnt from: Flo0806
Repo: nuxt/cli PR: 1306
File: packages/nuxi/src/commands/module/remove.ts:153-166
Timestamp: 2026-05-07T12:55:55.836Z
Learning: In Nuxt's `nuxt.config.ts`, the `modules` array does not support short aliases (e.g., `'content'`). Only full npm package names (e.g., `'nuxt/content'`) or file paths are valid entries. Therefore, when reading module names from the config via `readModuleName` in `packages/nuxi/src/commands/module/remove.ts`, the values will always be canonical npm names and can be directly compared to resolved npm package names without any additional alias resolution.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| await removeDependency(allToRemove, { | ||
| cwd, | ||
| packageManager, | ||
| workspace: packageManager?.name === 'pnpm' && existsSync(resolve(cwd, 'pnpm-workspace.yaml')), | ||
| }).catch((error) => { | ||
| logger.error(String(error)) | ||
| }) | ||
| } | ||
|
|
||
| return true |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check nypm's removeDependency type signature for array support
# Expected: function signature should show string | string[] as first param
# Find the nypm type declaration file
fd -i 'index.d.ts' --search-path node_modules/nypm | head -5 | xargs grep -A 5 'removeDependency' 2>/dev/null || \
fd -i 'nypm' --type f --extension ts | xargs grep -A 5 'removeDependency' 2>/dev/null | head -30Repository: nuxt/cli
Length of output: 157
🏁 Script executed:
wc -l packages/nuxi/src/commands/module/remove.tsRepository: nuxt/cli
Length of output: 100
🏁 Script executed:
cat -n packages/nuxi/src/commands/module/remove.ts | sed -n '90,250p'Repository: nuxt/cli
Length of output: 6359
🌐 Web query:
nypm removeDependency function signature documentation
💡 Result:
nypm removeDependency function signature (documented) The nypm package documents the API method as: removeDependency(name, options) — “Removes dependency from the project.” [1] Notes on how to use it (from the same docs page): - CLI form: npx nypm remove [1] - API usage section lists the “removeDependency(name, options)” signature under “API Usage”. [1] Sources: [1] https://www.npmjs.com/package/nypm
Citations:
removeDependency failure is silently swallowed; prepare still executes
When removeDependency rejects (line 239), the error is only logged and the function returns true (line 244). The caller then unconditionally runs prepare (lines 101-105) if !ctx.args.skipUninstall, which may fail or produce confusing results if node_modules is in an inconsistent state. Return false or re-throw after logging so the caller can skip prepare.
Additionally, verify that nypm's removeDependency accepts string[] as its first argument — the public docs only document removeDependency(name, options) with singular examples, not array support.
🤖 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/nuxi/src/commands/module/remove.ts` around lines 235 - 244, The
removeDependency rejection is currently only logged and the function returns
true, causing the caller to still run prepare; change the error handling in the
removeDependency call so that after logging you either re-throw the error or
return false to propagate failure to the caller (so caller honor prepare skip
when ctx.args.skipUninstall is false), and update the calling code that checks
the boolean result to skip calling prepare when false; also confirm
nypm.removeDependency's signature — if it does not accept string[] then iterate
over allToRemove and call removeDependency(name, options) for each entry (or use
Promise.all on per-name calls) instead of passing the array. Ensure references
to removeDependency, prepare, ctx.args.skipUninstall and allToRemove are updated
accordingly.
2 participants
🔗 Linked issue
Resolves: #1184
📚 Description
Added a
module removecommand to remove modules from Nuxtsmodulesarray, including uninstallation from package.json (along with peer dependencies, provided they are not required by other packages).Possible params:
skipConfig,skipUninstallTests
Added tests , similar to add command