Fix npm CLI symlink entrypoint detection#19
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 28 minutes and 34 seconds. ⌛ 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. ✨ 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 |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the CLI entry point detection logic by introducing a new isCliEntrypoint function that utilizes realpathSync for more reliable path comparison. A potential issue was identified in the fallback logic where an unhandled exception could occur if path resolution fails; a nested try-catch block was suggested to improve robustness.
| function isCliEntrypoint(entryPoint: string | undefined): boolean { | ||
| if (!entryPoint) { | ||
| return false; | ||
| } | ||
|
|
||
| try { | ||
| return realpathSync(entryPoint) === realpathSync(fileURLToPath(import.meta.url)); | ||
| } catch { | ||
| return import.meta.url === pathToFileURL(entryPoint).href; | ||
| } | ||
| } |
There was a problem hiding this comment.
The fallback logic in the catch block uses pathToFileURL(entryPoint), which can throw a TypeError if entryPoint is a relative path (Node.js requires an absolute path for pathToFileURL). While realpathSync usually resolves relative paths to absolute ones, if it fails for other reasons (e.g., permission issues on a parent directory), the fallback will crash the process with an unhandled exception. Consider wrapping the fallback in a nested try-catch or ensuring the path is absolute before conversion.
| function isCliEntrypoint(entryPoint: string | undefined): boolean { | |
| if (!entryPoint) { | |
| return false; | |
| } | |
| try { | |
| return realpathSync(entryPoint) === realpathSync(fileURLToPath(import.meta.url)); | |
| } catch { | |
| return import.meta.url === pathToFileURL(entryPoint).href; | |
| } | |
| } | |
| function isCliEntrypoint(entryPoint: string | undefined): boolean { | |
| if (!entryPoint) { | |
| return false; | |
| } | |
| try { | |
| return realpathSync(entryPoint) === realpathSync(fileURLToPath(import.meta.url)); | |
| } catch { | |
| try { | |
| return import.meta.url === pathToFileURL(entryPoint).href; | |
| } catch { | |
| return false; | |
| } | |
| } | |
| } |
1 participant
Summary
Validation