fix(init): make dependency install recoverable and package-manager aware#1386
Conversation
commit: |
|
Warning Review limit reached
Next review available in: 1 minute Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe CLI now uses shared install utilities for project dependencies and Nuxt modules. Installs resolve package managers, stream output, support cancellation, handle missing managers and ignored build scripts, and return structured results. Module installation now includes required peer dependencies and avoids updating configuration after failure. Initialization tracks recovery actions, forwards package-manager options, conditionally installs modules, and sets a failing exit code when dependencies fail. Unit tests cover the new installation and peer-resolution behavior. Estimated code review effort: 4 (Complex) | ~60 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/nuxt-cli/src/commands/init.ts (1)
486-535: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
--no-installnow also skips git initialization.The git-init block lives inside the
elsebranch of the install check, so--no-install(or a template/package-manager conflict viaskipInstallOnConflict) silently drops the repository initialization the user was just prompted for. Git init should be independent of the install outcome.🐛 Suggested fix
- if (gitInit) { - const gitSpinner = spinner() - ... - } - if (ignoredBuilds.length > 0) {…and move the block after the closing
}of the install branch (before the module handling), so it runs in both paths.🤖 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/nuxt-cli/src/commands/init.ts` around lines 486 - 535, Move the git initialization block identified by the gitInit condition outside the install branch, placing it after the branch closes and before module handling. Preserve its existing success and failure behavior so git init runs independently of --no-install and skipInstallOnConflict.
🧹 Nitpick comments (1)
packages/nuxt-cli/src/commands/module/add.ts (1)
218-220: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRecovery hint hardcodes
pnpm approve-buildswhile naming the resolved package manager.The message reads awkwardly for a non-pnpm manager. Since ignored-build parsing is pnpm-specific, gate the hint on
packageManager.name === 'pnpm'(or usepackageManager.command) —init.tsline 551 already interpolates the selected manager.🤖 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/nuxt-cli/src/commands/module/add.ts` around lines 218 - 220, Update the warning in the ignored-build handling around takeUnreportedIgnoredBuilds so the approval command is shown only when packageManager.name is pnpm (or is derived from packageManager.command); keep the ignored package list and manager name in the warning, while avoiding a hardcoded pnpm recovery hint for other package managers.
🤖 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/nuxt-cli/src/commands/init.ts`:
- Around line 537-553: Restrict ignored-build handling to pnpm: in
packages/nuxt-cli/src/commands/init.ts lines 537-553, gate the block on
selectedPackageManager === 'pnpm' and record the literal recovery command pnpm
approve-builds; in packages/nuxt-cli/src/commands/module/add.ts lines 218-220,
gate the warning on packageManager.name === 'pnpm' so approve-builds guidance is
only shown for pnpm.
- Around line 501-510: Update the packageManager argument in the runInstall call
to use the resolved templatePackageManager descriptor, preserving fields such as
majorVersion and version while selecting the configured install manager as
required. Do not reduce the descriptor to only name and command, so nypm can
generate the correct Yarn Classic or Berry command.
In `@packages/nuxt-cli/src/utils/install.ts`:
- Around line 20-22: Update the install output handling around
OUTPUT_TAIL_LINES, IGNORED_BUILDS_RE, and takeUnreportedIgnoredBuilds so
ignored-build notices are parsed from the complete command output rather than
only the truncated 30-line tail. Preserve tail truncation for returned/display
output, but maintain a separate full or incrementally parsed buffer that init.ts
and module/add.ts can use without losing earlier warnings.
- Around line 196-199: Update the abort handling in execute around the abort
callback and options.signal listener to immediately terminate when the signal is
already aborted, and ensure termination reaches the entire spawned process tree
rather than only the direct child (including corepack-routed package managers).
Preserve the existing one-time listener behavior for signals that abort after
execution starts.
In `@packages/nuxt-cli/test/unit/commands/add.spec.ts`:
- Around line 335-337: Update the failure fixture in the “should not update
nuxt.config when the install fails” test to remain consistent with the selected
npm package manager: either return an npm-specific ENOENT failure from
runInstall or assert that runInstall receives npm before simulating the failure,
while preserving the existing config non-update behavior.
In `@packages/nuxt-cli/test/unit/utils/install.spec.ts`:
- Around line 65-69: Update the “should report a missing package manager instead
of throwing” test to ensure runInstall’s dry-run result includes an executable
command, either by supplying a fixture dependency or mocking the dry-run result.
Verify the nonexistent package-manager command is spawned through the
child-process execution path so ENOENT handling is exercised deterministically.
---
Outside diff comments:
In `@packages/nuxt-cli/src/commands/init.ts`:
- Around line 486-535: Move the git initialization block identified by the
gitInit condition outside the install branch, placing it after the branch closes
and before module handling. Preserve its existing success and failure behavior
so git init runs independently of --no-install and skipInstallOnConflict.
---
Nitpick comments:
In `@packages/nuxt-cli/src/commands/module/add.ts`:
- Around line 218-220: Update the warning in the ignored-build handling around
takeUnreportedIgnoredBuilds so the approval command is shown only when
packageManager.name is pnpm (or is derived from packageManager.command); keep
the ignored package list and manager name in the warning, while avoiding a
hardcoded pnpm recovery hint for other package managers.
🪄 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 Plus
Run ID: 24523a44-5666-445b-a838-0cc14e93f420
📒 Files selected for processing (7)
packages/nuxt-cli/src/commands/init.tspackages/nuxt-cli/src/commands/module/add.tspackages/nuxt-cli/src/utils/install.tspackages/nuxt-cli/test/unit/commands/add.spec.tspackages/nuxt-cli/test/unit/commands/module/add-peers.spec.tspackages/nuxt-cli/test/unit/commands/module/add.spec.tspackages/nuxt-cli/test/unit/utils/install.spec.ts
1 participant
🔗 Linked issue
resolves #1267
resolves #1317
resolves #1198
resolves #1342
resolves #1239
📚 Description
we had a range of issues stemming from asking
nypmto actually run installs - we couldn't surface bugs or issues to the userthis aims to make things much more solid by asking
nypmto give us the command to run, but then taking responsibility for how we do that