-
-
Notifications
You must be signed in to change notification settings - Fork 12
fix(cli): add a npx/bunx fallback for running prisma commands #409
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughThe Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 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 |
|
Claude finished @ymc9's task —— View job Code Review Complete ✅I've thoroughly reviewed this PR that adds npx/bunx fallback for running Prisma commands. Here's my comprehensive feedback: Todo List
SummaryThis PR improves resilience in the Code Quality & Best Practices ✅
Potential Bugs/Issues ✅
Performance Considerations ✅
Security Concerns ✅
Test Coverage
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds a fallback mechanism for executing Prisma commands when the direct path resolution fails. If resolving the locally installed prisma package fails (e.g., when it's not installed in node_modules), the code now falls back to using npx or bunx to execute Prisma commands.
Key changes:
- Wrapped the
prismapath resolution logic in a try-catch block to handle resolution failures gracefully - Added a fallback to use
execPackage(npx/bunx) when the direct path cannot be resolved - Changed
prismaPathfromstringtostring | undefinedto represent the optional nature of the resolved path
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/cli/src/utils/exec-utils.ts (1)
33-44: Consider adding observability for fallback behavior.The try-catch appropriately handles both ESM and CJS resolution, and the empty catch block allows graceful fallback to npx/bunx. However, silently swallowing all resolution errors might make debugging difficult when issues arise.
Consider logging a debug message when falling back:
} catch { - // ignore and fallback + // ignore and fallback (could add debug logging here if needed) }Or add conditional logging after the try-catch:
} catch { // ignore and fallback } + + if (!prismaPath && process.env.DEBUG) { + console.warn('Could not resolve prisma path, falling back to npx/bunx'); + }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/cli/src/utils/exec-utils.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build-test (20.x, sqlite)
- GitHub Check: build-test (20.x, postgresql)
- GitHub Check: claude-review
🔇 Additional comments (2)
packages/cli/src/utils/exec-utils.ts (2)
46-50: LGTM! Fallback logic is well-implemented.The early return when
prismaPathis undefined properly prevents double execution, and falling back toexecPackage(npx/bunx) ensures the command still runs even when direct resolution fails.
52-52: LGTM! Direct execution path remains unchanged.The execution logic correctly uses the resolved Prisma path when available.
Summary by CodeRabbit