refactor: replace Bun APIs with standard Node.js equivalents#9
Conversation
Summary of ChangesHello @iHildy, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refactors the command loading logic to enhance cross-platform compatibility by replacing Bun-specific APIs with standard Node.js equivalents. The changes ensure that the application's file system operations and module path resolution work seamlessly in both Bun and Node.js environments, improving robustness and reducing reliance on a single runtime's unique features. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![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 does a great job of refactoring the codebase to remove Bun-specific APIs and use standard Node.js equivalents, which significantly improves cross-platform compatibility. The introduction of getModuleDir is a clean way to handle differences between JavaScript runtimes. I've identified an opportunity to improve the new scanMdFiles function for better performance and robustness. My suggestion refactors it to scan directories in parallel and gracefully handle missing directories, which makes it more efficient and resilient.
| async function scanMdFiles(dir: string): Promise<string[]> { | ||
| const files: string[] = []; | ||
|
|
||
| async function walk(currentDir: string): Promise<void> { | ||
| const entries = await fs.readdir(currentDir, { withFileTypes: true }); | ||
| for (const entry of entries) { | ||
| const fullPath = path.join(currentDir, entry.name); | ||
| if (entry.isDirectory()) { | ||
| await walk(fullPath); | ||
| } else if (entry.isFile() && entry.name.endsWith('.md')) { | ||
| files.push(fullPath); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| await walk(dir); | ||
| return files; | ||
| } |
There was a problem hiding this comment.
The scanMdFiles function can be improved for performance, robustness, and readability.
- Performance: The current implementation processes subdirectories sequentially. This can be inefficient for directories with many subdirectories. The suggested change uses
Promise.allto process directory entries in parallel, speeding up file scanning. - Robustness: The current implementation will throw an error if a directory doesn't exist or can't be read. This is likely a regression from
Bun.Glob, which would typically find no files. The suggestion adds error handling to return an empty array in these cases, making the function more resilient. - Readability: The suggested version is more functional and avoids shared mutable state (the
filesarray), which makes the code cleaner and easier to reason about.
async function scanMdFiles(dir: string): Promise<string[]> {
const entries = await fs.readdir(dir, { withFileTypes: true }).catch(err => {
// If directory is not readable, treat as empty to mimic glob behavior.
if (err.code === 'ENOENT' || err.code === 'EACCES') {
return [];
}
throw err;
});
const filePromises = entries.map(async (entry) => {
const fullPath = path.join(dir, entry.name);
if (entry.isDirectory()) {
return scanMdFiles(fullPath);
}
if (entry.isFile() && entry.name.endsWith('.md')) {
return [fullPath];
}
return [];
});
const nestedFiles = await Promise.all(filePromises);
return nestedFiles.flat();
}
🤖 Review Jules RelayI found 1 Gemini suggestion so far. Type |
1 participant
This pull request updates the command loading logic in
src/index.tsto improve compatibility across Node.js and Bun, and removes Bun-specific dependencies. The most important changes are grouped below:Cross-Platform Compatibility:
getModuleDirfunction to reliably get the current module directory in both Bun and Node.js environments, replacing direct usage ofimport.meta.dir.Bun.GlobandBun.fileAPIs with a newscanMdFilesfunction that recursively scans for.mdfiles using Node'sfs.promisesand reads files withfs.readFile.Dependency Updates:
fs/promisesandfileURLToPathfrom Node.js modules to support the new file scanning and path resolution methods.