fix(devtools): remove the correct module in uninstallNuxtModule - #1019
Conversation
uninstallNuxtModule used config.modules.splice(index - 1, 1) inside a forEach, an off-by-one that never removed the requested module and instead deleted a different, innocent one (splice(-1, 1) for a target at index 0 removes the last module). The corrupted config was then persisted to disk. Extract a pure removeNuxtModuleFromCode(source, name) helper that filters the modules array by value, correctly handling both string entries and tuple entries with options, and wire it into uninstallNuxtModule in place of the manual splice. Closes plans/003.
efa0a82 to
3414f1d
Compare
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe uninstall flow now delegates Nuxt config editing to Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
packages/devtools/src/utils/nuxt-config.tsESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox. 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: 1
🧹 Nitpick comments (2)
packages/devtools/test/remove-nuxt-module.test.ts (1)
13-16: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsolidate redundant tests.
This test duplicates the inputs and assertions of the preceding test (lines 7-11). Since the first test already acts as the regression guard for preserving
@nuxt/content(the remaining module), you can safely remove this block or combine their descriptions to avoid redundant execution.♻️ Proposed cleanup
- it('removes the first module without dropping the last', () => { - const out = removeNuxtModuleFromCode(base, '`@nuxt/image`') - expect(out).toContain('`@nuxt/content`') // regression guard for the splice(-1) bug - })🤖 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/devtools/test/remove-nuxt-module.test.ts` around lines 13 - 16, Remove the redundant test block around removeNuxtModuleFromCode, or merge its description and assertion into the preceding test; retain a single regression assertion verifying that removing `@nuxt/image` preserves `@nuxt/content`.packages/devtools/src/server-rpc/npm.ts (1)
147-150: 🩺 Stability & Availability | 🔵 Trivial | 🏗️ Heavy liftSynchronize file modifications and batch dev-server restarts for uninstalls.
The config file is read before the long-running npm process and written back after it completes. Because it currently bypasses the
latestGeneratedandinstallSetbatching logic used byinstallNuxtModule, concurrent uninstalls (or overlapping installs/uninstalls) can cause a race condition (TOCTOU) overwriting each other's config changes, and will trigger multiple overlapping dev-server restarts.Consider extracting a shared concurrency queue to manage
latestGeneratedacross both install and uninstall operations so they safely sequence their config writes.🤖 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/devtools/src/server-rpc/npm.ts` around lines 147 - 150, Update the uninstall flow around magicastGuard and removeNuxtModuleFromCode to use the same shared concurrency queue and latestGenerated/installSet batching state as installNuxtModule. Sequence config reads, transformations, and writes through that shared mechanism so concurrent or overlapping install/uninstall operations preserve each other’s changes and trigger only batched dev-server restarts.
🤖 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/devtools/src/utils/nuxt-config.ts`:
- Around line 16-22: Update the config.modules cleanup logic to mutate the
existing array in place instead of assigning the result of filter(). In the
surrounding module-removal logic, iterate entries in reverse and splice matching
string entries or array entries whose first element equals name, preserving all
nonmatching entries and their existing formatting/comments.
---
Nitpick comments:
In `@packages/devtools/src/server-rpc/npm.ts`:
- Around line 147-150: Update the uninstall flow around magicastGuard and
removeNuxtModuleFromCode to use the same shared concurrency queue and
latestGenerated/installSet batching state as installNuxtModule. Sequence config
reads, transformations, and writes through that shared mechanism so concurrent
or overlapping install/uninstall operations preserve each other’s changes and
trigger only batched dev-server restarts.
In `@packages/devtools/test/remove-nuxt-module.test.ts`:
- Around line 13-16: Remove the redundant test block around
removeNuxtModuleFromCode, or merge its description and assertion into the
preceding test; retain a single regression assertion verifying that removing
`@nuxt/image` preserves `@nuxt/content`.
🪄 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: 59541815-cc50-4fc1-a155-f1a316a00d9e
📒 Files selected for processing (5)
packages/devtools/src/server-rpc/npm.tspackages/devtools/src/utils/nuxt-config.tspackages/devtools/test/remove-nuxt-module.test.tsplans/003-fix-uninstall-nuxt-module.mdplans/README.md
💤 Files with no reviewable changes (1)
- plans/003-fix-uninstall-nuxt-module.md
Reassigning config.modules from filter() makes magicast regenerate the array from scratch, discarding comments/formatting on the entries that remain. Remove matching entries with an in-place, reverse-order splice instead, per review feedback on nuxt#1019.
2 participants
uninstallNuxtModuleedited the user'snuxt.configusingconfig.modules.splice(index - 1, 1)inside aforEachoverObject.values(config.modules). This off-by-one meant the module theuser asked to remove was never deleted — a different, innocent module
was removed instead (for a target at index 0,
splice(-1, 1)removesthe last module). The corrupted config was then written back to
disk, silently corrupting the user's Nuxt config on a headline
"remove module" feature.
This extracts a pure, unit-tested
removeNuxtModuleFromCode(source, name)helper (
packages/devtools/src/utils/nuxt-config.ts) that filters themodulesarray by value, correctly handling both string entries(
'@nuxt/image') and tuple entries with options(
['@nuxt/image', { ... }]), and wires it intouninstallNuxtModulein place of the manual splice.
packages/devtools/src/utils/nuxt-config.ts— new pure helperpackages/devtools/src/server-rpc/npm.ts—uninstallNuxtModulenow delegates to the helperpackages/devtools/test/remove-nuxt-module.test.ts— regression coverage for the splice bug, tuple entries, and the absent-module no-opCreated with the help of an AI agent.