Fix file path resolution for config files #6
Conversation
Replaced usage of URL and import.meta.url.pathname with fileURLToPath for consistent file path resolution in Provider and promptLoader modules. This improves compatibility across environments and fixes issues with locating config files.
WalkthroughReplaced URL->pathname conversions with file-system-safe conversions using fileURLToPath(import.meta.url) across multiple modules to resolve current module directories for locating config and prompt files. No public APIs or control flow were changed. Changes
Sequence Diagram(s)(Skipped — changes are local path-resolution adjustments; control flow unchanged.) Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🧹 Nitpick comments (1)
src/providers/base/Provider.js (1)
46-47: LGTM: Improved file path resolution with minor suggestion for robustness.The change to use
fileURLToPath(import.meta.url)withpath.dirnameandpath.resolvecorrectly addresses the cross-platform file path resolution issues. This is consistent with the changes insrc/utils/promptLoader.js.Consider making the path construction more robust by using fewer relative path traversals.
- const currentDir = path.dirname(fileURLToPath(import.meta.url)); - const pricingPath = path.resolve(currentDir, '..', '..', '..', 'config', `${providerName}-pricing.json`); + const currentDir = path.dirname(fileURLToPath(import.meta.url)); + const configDir = path.resolve(currentDir, '../../../config'); + const pricingPath = path.join(configDir, `${providerName}-pricing.json`);This approach separates the config directory resolution from the filename construction, making it clearer and slightly more maintainable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/providers/base/Provider.js(2 hunks)src/utils/promptLoader.js(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
src/providers/base/Provider.js (2)
src/utils/promptLoader.js (1)
currentDir(53-53)src/utils/costTracker.js (1)
currentDir(59-59)
src/utils/promptLoader.js (1)
src/utils/costTracker.js (1)
currentDir(59-59)
🔇 Additional comments (2)
src/utils/promptLoader.js (1)
11-11: LGTM: Proper import added for cross-platform compatibility.The import of
fileURLToPathfrom the 'url' module is correct and necessary for the improved file path resolution.src/providers/base/Provider.js (1)
2-3: LGTM: Proper imports added for improved file path resolution.The addition of
pathandfileURLToPathimports is correct and necessary for the cross-platform file path resolution improvements.
| export function buildSystemPrompt(targetLang, sourceLang = 'English', promptFilePath = null) { | ||
| if (!promptFilePath) { | ||
| const currentDir = path.dirname(new URL(import.meta.url).pathname); | ||
| const currentDir = path.dirname(fileURLToPath(import.meta.url)); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
LGTM: Improved file path resolution for cross-platform compatibility.
The change from new URL(import.meta.url).pathname to fileURLToPath(import.meta.url) is correct and addresses the file path resolution issues mentioned in the PR objectives. This is particularly important for Windows environments where URL pathname handling can be problematic.
However, ensure consistency across the codebase with similar patterns.
🏁 Script executed:
#!/bin/bash
# Description: Check for remaining usage of the old URL pattern that should be updated for consistency
# Expected: Find any remaining instances that might need similar updates
echo "Searching for remaining instances of 'new URL(import.meta.url).pathname' pattern:"
rg -n "new URL\(import\.meta\.url\)\.pathname" --type js
echo -e "\nSearching for any URL-based path resolution patterns:"
rg -n "import\.meta\.url.*pathname" --type jsLength of output: 604
Update remaining file path resolution for cross-platform consistency
The change to fileURLToPath(import.meta.url) correctly addresses Windows pathname issues and should be applied uniformly.
• In src/utils/costTracker.js (line 59), replace the old URL-based pattern:
- const currentDir = path.dirname(new URL(import.meta.url).pathname);
+ const currentDir = path.dirname(fileURLToPath(import.meta.url));Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/utils/costTracker.js at line 59, update the file path resolution to use
path.dirname(fileURLToPath(import.meta.url)) instead of the old URL-based
pattern to ensure cross-platform consistency, especially on Windows. Replace the
existing code with this new pattern to standardize path handling.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/utils/costTracker.js (1)
63-67: Minor: prefer path.join arguments over embedded slashesNot a functional issue (Node handles forward slashes on Windows), but using path.join/readable resolve improves consistency and avoids accidental double-slashes.
Apply:
-const jsonPath = path.resolve(currentDir, `../../config/${providerName}-pricing.json`); +const jsonPath = path.resolve(currentDir, '../../config', `${providerName}-pricing.json`); -const fallbackPath = path.resolve(process.cwd(), `config/${providerName}-pricing.json`); +const fallbackPath = path.join(process.cwd(), 'config', `${providerName}-pricing.json`);tools/ab-prompt-test (3)
160-162: Good: use fileURLToPath(import.meta.url) to locate configThis fixes failures when running from paths with spaces (e.g., LocalWP’s “Local Sites”). Optional DRY improvement below.
To avoid recomputing the same directory multiple times, define a single constant once near the imports and reuse it:
-const currentDir = path.dirname(fileURLToPath(import.meta.url)); -const configPath = path.resolve(currentDir, '../config/openai-pricing.json'); +const configPath = path.resolve(CURRENT_DIR, '../config/openai-pricing.json');Add near the top (outside this hunk):
// At top-level after imports const CURRENT_DIR = path.dirname(fileURLToPath(import.meta.url));
284-286: Good: dictionary directory now resolves correctly on WindowsSame benefit here; consider reusing a shared CURRENT_DIR constant to reduce duplication.
Apply:
-const currentDir = path.dirname(fileURLToPath(import.meta.url)); -const dictionaryDir = currentDir; // Look for dictionary files in tools directory. +const dictionaryDir = CURRENT_DIR; // Look for dictionary files in tools directory.Add once at top-level (if not added already):
const CURRENT_DIR = path.dirname(fileURLToPath(import.meta.url));
623-623: Typo: “sgavings” → “savings”Minor copy fix for console output.
- console.log(chalk.green(` 💰 Total sgavings: $${formatPrice(savings1000)}`)); + console.log(chalk.green(` 💰 Total savings: $${formatPrice(savings1000)}`));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
src/providers/base/Provider.js(2 hunks)src/utils/costTracker.js(2 hunks)tools/ab-prompt-test(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/providers/base/Provider.js
🔇 Additional comments (3)
src/utils/costTracker.js (2)
60-60: Correct: derive module dir via fileURLToPath(import.meta.url)This is the right way to compute the directory across environments; it fixes spaces/Unicode and Windows drive-letter quirks.
3-3: All clear: no URL.pathname usages remain — approved
Verified with ripgrep: zero matches fornew URL(import.meta.url).pathnameorimport.meta.url.pathname. UsingfileURLToPathnow ensures ESM-safe, cross-platform path handling.tools/ab-prompt-test (1)
8-8: Importing fileURLToPath is appropriate for ESM path resolutionThis aligns with the PR objective to resolve config/dictionary files reliably on Windows and other environments.
|
Thanks, @jack-arturo, and sorry it took me some time to review/merge. |
2 participants
Replaced usage of URL and import.meta.url.pathname with fileURLToPath for consistent file path resolution in Provider and promptLoader modules. This improves compatibility across environments and fixes issues with locating config files (i.e. when using LocalWP and running WordPress in
~/Local Sites/)Summary by CodeRabbit
Refactor
Tests