fix(cli): regression of CLI plugin resolution#2516
Conversation
|
Claude finished @ymc9's task —— View job PR Review: CLI Plugin Resolution FixTodo List:
🔍 Review SummaryThis PR addresses a regression in CLI plugin resolution with a targeted fix to ensure plugins have valid 📝 Code Quality & Best PracticesStrengths:
Minor concerns:
🐛 Bug AnalysisIssue Fixed: Fix Quality:
⚡ Performance ConsiderationsPositive aspects:
🔒 Security AssessmentNo security concerns identified:
🧪 Test CoverageCurrent state:
Recommendation: Example test case: it('rejects plugin without generate function', async () => {
// Test loading a module that exports something but not a valid CliPlugin
const result = await loadPluginModule('./invalid-plugin.js', '/test/path');
expect(result).toBeUndefined();
});📦 Package Version UpdatesAnalysis:
✅ Overall AssessmentThis is a well-executed fix for a specific regression. The changes are:
The only suggestion is to add a specific test case for the regression scenario to prevent future issues. Recommendation: ✅ APPROVE |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis patch updates many package.json versions from 3.5.0 → 3.5.1 (VSCode extension → 3.5.2) and tightens CLI plugin loading to only return imported modules that expose a Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 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 docstrings
🧪 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/cli/src/actions/action-utils.ts (1)
244-261:⚠️ Potential issue | 🟡 MinorMissing optional chaining causes inconsistent error handling.
If the module exists but has no default export (or default is
undefined), accessingresult.generatethrows aTypeError. This error is caught and re-thrown as aCliErrorwith "Failed to load plugin module", which masks the actual issue and differs from the behavior at line 306 where optional chaining correctly returnsundefinedfor invalid plugins.Proposed fix to add optional chaining for consistency
const importAsEsm = async (spec: string) => { try { const result = (await import(spec)).default as CliPlugin; - return typeof result.generate === 'function' ? result : undefined; + return typeof result?.generate === 'function' ? result : undefined; } catch (err) { throw new CliError(`Failed to load plugin module from ${spec}: ${(err as Error).message}`); } }; const jiti = createJiti(pathToFileURL(basePath).toString()); const importAsTs = async (spec: string) => { try { const result = (await jiti.import(spec, { default: true })) as CliPlugin; - return typeof result.generate === 'function' ? result : undefined; + return typeof result?.generate === 'function' ? result : undefined; } catch (err) { throw new CliError(`Failed to load plugin module from ${spec}: ${(err as Error).message}`); } };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/actions/action-utils.ts` around lines 244 - 261, The plugin loaders importAsEsm and importAsTs can throw when a module has no default export because they access result.generate directly; change the checks to use optional chaining (e.g., verify typeof result?.generate === 'function') so a missing default simply yields undefined instead of causing a TypeError and being wrapped as a CliError; update both importAsEsm and importAsTs (which uses jiti created via createJiti(pathToFileURL(basePath).toString())) to perform the optional-chained property check to match the behavior used elsewhere for CliPlugin validation.
🤖 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/cli/src/actions/action-utils.ts`:
- Around line 294-300: The jiti fallback block currently assumes the imported
value exists and checks typeof result.generate, which can throw if result is
undefined; update the jiti.import handling in the same block to guard the access
(e.g., check result !== undefined/null or use optional chaining like typeof
result?.generate === 'function') before returning the plugin, keeping the same
return behavior (return result if it has a generate function, otherwise
undefined) and reference the result variable, jiti.import call and the
CliPlugin/generate check when making the change.
---
Outside diff comments:
In `@packages/cli/src/actions/action-utils.ts`:
- Around line 244-261: The plugin loaders importAsEsm and importAsTs can throw
when a module has no default export because they access result.generate
directly; change the checks to use optional chaining (e.g., verify typeof
result?.generate === 'function') so a missing default simply yields undefined
instead of causing a TypeError and being wrapped as a CliError; update both
importAsEsm and importAsTs (which uses jiti created via
createJiti(pathToFileURL(basePath).toString())) to perform the optional-chained
property check to match the behavior used elsewhere for CliPlugin validation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a86e637f-0f10-485d-8ff1-764bd4483bbf
📒 Files selected for processing (25)
package.jsonpackages/auth-adapters/better-auth/package.jsonpackages/cli/package.jsonpackages/cli/src/actions/action-utils.tspackages/clients/client-helpers/package.jsonpackages/clients/tanstack-query/package.jsonpackages/common-helpers/package.jsonpackages/config/eslint-config/package.jsonpackages/config/typescript-config/package.jsonpackages/config/vitest-config/package.jsonpackages/create-zenstack/package.jsonpackages/ide/vscode/package.jsonpackages/language/package.jsonpackages/orm/package.jsonpackages/plugins/policy/package.jsonpackages/schema/package.jsonpackages/sdk/package.jsonpackages/server/package.jsonpackages/testtools/package.jsonpackages/zod/package.jsonsamples/orm/package.jsontests/e2e/package.jsontests/regression/package.jsontests/runtimes/bun/package.jsontests/runtimes/edge-runtime/package.json
|
Claude finished @ymc9's task —— View job Claude finished reviewing the pull request PR Review: CLI Plugin Resolution FixTodo List:
🔍 Review SummaryThis PR addresses a regression in CLI plugin resolution with a targeted fix to ensure plugins have valid 📝 Code Quality & Best PracticesStrengths:
Minor concerns:
🐛 Bug AnalysisIssue Fixed: Fix Quality:
CodeRabbit identified a valid concern: Missing optional chaining in lines 247, 257, and 297 could cause ⚡ Performance ConsiderationsPositive aspects:
🔒 Security AssessmentNo security concerns identified:
🧪 Test CoverageCurrent state:
Recommendation: Example test case: it('rejects plugin without generate function', async () => {
// Test loading a module that exports something but not a valid CliPlugin
const result = await loadPluginModule('./invalid-plugin.js', '/test/path');
expect(result).toBeUndefined();
});📦 Package Version UpdatesAnalysis:
✅ Overall AssessmentThis is a well-executed fix for a specific regression. The changes are:
The only suggestions are:
Recommendation: ✅ APPROVE The core fix is solid and addresses the regression effectively. The optional chaining improvement would be nice-to-have but not critical for this release. |
1 participant
Summary by CodeRabbit
Release v3.5.1
Bug Fixes
Chores