chore: Resolve DATA_CACHE_DIR in .env.development
#306
Conversation
WalkthroughThe updates modify environment variable settings in Changes
Poem
Tip ⚡️ Faster reviews with caching
Enjoy the performance boost—your workflow just got faster. ✨ Finishing Touches
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. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
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: 0
🧹 Nitpick comments (2)
.env.development (2)
26-26: Ensure environment variable expansion
Most.envloaders (e.g.,dotenv) do not expand shell variables like${PWD}by default. Verify that the project usesdotenv-expandor a similar mechanism so thatDATA_CACHE_DIRresolves correctly at runtime instead of remaining the literal string.
27-27: Clarify theCONFIG_PATHplaceholder
The commented-outCONFIG_PATHcan be a helpful hint, but consider adding a brief inline note about when to enable it or moving detailed instructions to documentation to reduce clutter in the env file.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.env.development(1 hunks)package.json(0 hunks)packages/backend/package.json(1 hunks)packages/backend/src/index.ts(0 hunks)
💤 Files with no reviewable changes (2)
- packages/backend/src/index.ts
- package.json
🔇 Additional comments (1)
packages/backend/package.json (1)
8-8: Remove redundant--cacheDirargument
Centralizing the cache directory viaDATA_CACHE_DIRis on point. Please confirm that the backend startup sequence sources the.env.developmentfile (or equivalent) so thatprocess.env.DATA_CACHE_DIRis correctly picked up in development.
|
/review |
![sourcebot-review-agent-dev[bot]](https://avatars.githubusercontent.com/u/135396723?s=60&v=4){kind=small}
| DATA_CACHE_DIR=${PWD}/.sourcebot # Path to the sourcebot cache dir (ex. ~/sourcebot/.sourcebot) | ||
| # CONFIG_PATH=${PWD}/config.json # Path to the sourcebot config file (if one exists) |
There was a problem hiding this comment.
Hardcoding DATA_CACHE_DIR to ${PWD}/.sourcebot may cause issues in environments where PWD is not set or differs from the expected working directory. Consider allowing this to be overridden externally or document the assumption clearly. Also, if this file is sourced in contexts where PWD is not defined, it could lead to unexpected path resolution failures.
| @@ -12,7 +12,6 @@ | |||
| "dev:zoekt": "yarn with-env zoekt-webserver -index .sourcebot/index -rpc", | |||
| "dev:backend": "yarn with-env yarn workspace @sourcebot/backend dev:watch", | |||
| "dev:web": "yarn with-env yarn workspace @sourcebot/web dev", | |||
| "dev:review-agent": "yarn with-env yarn workspace @sourcebot/review-agent dev", | |||
| "watch:mcp": "yarn workspace @sourcebot/mcp build:watch", | |||
There was a problem hiding this comment.
The removal of the "dev:review-agent" script line should be accompanied by verification that no other scripts or documentation reference this script. If references exist, they need to be updated or removed to prevent confusion or build failures.
| @@ -5,7 +5,7 @@ | |||
| "main": "index.js", | |||
| "type": "module", | |||
| "scripts": { | |||
| "dev:watch": "tsc-watch --preserveWatchOutput --onSuccess \"yarn dev --cacheDir ../../.sourcebot\"", | |||
| "dev:watch": "tsc-watch --preserveWatchOutput --onSuccess \"yarn dev\"", | |||
There was a problem hiding this comment.
Removing the --cacheDir option from the 'dev:watch' script may break functionality if 'dev' expects the cacheDir argument. Verify that the 'dev' script or the application can operate correctly without the explicit cacheDir setting before removing it.
| @@ -1,7 +1,6 @@ | |||
| import "./instrument.js"; | |||
|
|
|||
| import * as Sentry from "@sentry/node"; | |||
| import { ArgumentParser } from "argparse"; | |||
| import { existsSync } from 'fs'; | |||
There was a problem hiding this comment.
The import statement for 'ArgumentParser' from 'argparse' was removed. Ensure this import is not used elsewhere in this file or the removal may cause runtime errors.
| @@ -37,8 +36,6 @@ process.on('unhandledRejection', (reason, promise) => { | |||
| process.exit(1); | |||
| }); | |||
|
|
|||
| console.log(process.cwd()); | |||
|
|
|||
| const cacheDir = env.DATA_CACHE_DIR; | |||
| const reposPath = path.join(cacheDir, 'repos'); | |||
There was a problem hiding this comment.
The removed console.log call on process.cwd() is useful for debugging the current working directory during application startup. Confirm this removal is intentional as no alternative logging has been added.
|
/review |
| DATA_CACHE_DIR=${PWD}/.sourcebot # Path to the sourcebot cache dir (ex. ~/sourcebot/.sourcebot) | ||
| # CONFIG_PATH=${PWD}/config.json # Path to the sourcebot config file (if one exists) |
There was a problem hiding this comment.
Avoid setting DATA_CACHE_DIR and CONFIG_PATH using PWD directly in the .env file as this can lead to unexpected paths if the working directory changes. Instead, set these variables dynamically in the application or in a startup script to ensure they resolve to the intended absolute path.
| DATA_CACHE_DIR=${PWD}/.sourcebot # Path to the sourcebot cache dir (ex. ~/sourcebot/.sourcebot) | ||
| # CONFIG_PATH=${PWD}/config.json # Path to the sourcebot config file (if one exists) |
There was a problem hiding this comment.
Using ${PWD} for DATA_CACHE_DIR and CONFIG_PATH hardcodes the directory to the current working directory at load time, which may cause issues if the environment file is sourced from different locations or scripts run with different working directories. Consider using absolute paths or allowing these variables to be overridden externally with default fallbacks, for example: DATA_CACHE_DIR=${DATA_CACHE_DIR:-${PWD}/.sourcebot}.
| @@ -12,7 +12,6 @@ | |||
| "dev:zoekt": "yarn with-env zoekt-webserver -index .sourcebot/index -rpc", | |||
| "dev:backend": "yarn with-env yarn workspace @sourcebot/backend dev:watch", | |||
| "dev:web": "yarn with-env yarn workspace @sourcebot/web dev", | |||
| "dev:review-agent": "yarn with-env yarn workspace @sourcebot/review-agent dev", | |||
| "watch:mcp": "yarn workspace @sourcebot/mcp build:watch", | |||
There was a problem hiding this comment.
The removal of the 'dev:review-agent' script should be verified against any automation or developers' workflows that might depend on it to avoid breaking development processes.
| @@ -12,7 +12,6 @@ | |||
| "dev:zoekt": "yarn with-env zoekt-webserver -index .sourcebot/index -rpc", | |||
| "dev:backend": "yarn with-env yarn workspace @sourcebot/backend dev:watch", | |||
| "dev:web": "yarn with-env yarn workspace @sourcebot/web dev", | |||
| "dev:review-agent": "yarn with-env yarn workspace @sourcebot/review-agent dev", | |||
| "watch:mcp": "yarn workspace @sourcebot/mcp build:watch", | |||
There was a problem hiding this comment.
The script 'dev:review-agent' was removed. Verify that this script is no longer required for development to avoid broken workflows.
| @@ -5,7 +5,7 @@ | |||
| "main": "index.js", | |||
| "type": "module", | |||
| "scripts": { | |||
| "dev:watch": "tsc-watch --preserveWatchOutput --onSuccess \"yarn dev --cacheDir ../../.sourcebot\"", | |||
| "dev:watch": "tsc-watch --preserveWatchOutput --onSuccess \"yarn dev\"", | |||
There was a problem hiding this comment.
Removing the --cacheDir ../../.sourcebot argument from the dev:watch script may cause the development command to not use the intended cache directory as per the PR description about DATA_CACHE_DIR. Please verify that the environment variable or other configuration correctly sets this cache directory elsewhere, or consider retaining this argument if it is necessary for correct cache directory resolution.
| @@ -5,7 +5,7 @@ | |||
| "main": "index.js", | |||
| "type": "module", | |||
| "scripts": { | |||
| "dev:watch": "tsc-watch --preserveWatchOutput --onSuccess \"yarn dev --cacheDir ../../.sourcebot\"", | |||
| "dev:watch": "tsc-watch --preserveWatchOutput --onSuccess \"yarn dev\"", | |||
There was a problem hiding this comment.
Removing the '--cacheDir ../../.sourcebot' argument from the 'dev:watch' script could lead to missing necessary cache directory settings required by the application. Confirm that the application no longer needs this argument or that it is set elsewhere in the environment or codebase. Otherwise, this change could cause runtime errors due to missing cache directory configuration.
| @@ -1,7 +1,6 @@ | |||
| import "./instrument.js"; | |||
|
|
|||
| import * as Sentry from "@sentry/node"; | |||
| import { ArgumentParser } from "argparse"; | |||
| import { existsSync } from 'fs'; | |||
There was a problem hiding this comment.
Remove the unused import statement 'import { ArgumentParser } from "argparse";' instead of just deleting the line to avoid confusion or future errors.
| @@ -1,7 +1,6 @@ | |||
| import "./instrument.js"; | |||
|
|
|||
| import * as Sentry from "@sentry/node"; | |||
| import { ArgumentParser } from "argparse"; | |||
| import { existsSync } from 'fs'; | |||
There was a problem hiding this comment.
Remove the unused import statement for ArgumentParser since it has been deleted from the imports, to avoid confusion or clutter in the import section.
| @@ -37,8 +36,6 @@ process.on('unhandledRejection', (reason, promise) => { | |||
| process.exit(1); | |||
| }); | |||
|
|
|||
| console.log(process.cwd()); | |||
|
|
|||
| const cacheDir = env.DATA_CACHE_DIR; | |||
| const reposPath = path.join(cacheDir, 'repos'); | |||
There was a problem hiding this comment.
Remove the commented-out console.log statement or add a comment explaining why it was removed, to keep the code clean and maintainable.
| @@ -37,8 +36,6 @@ process.on('unhandledRejection', (reason, promise) => { | |||
| process.exit(1); | |||
| }); | |||
|
|
|||
| console.log(process.cwd()); | |||
|
|
|||
| const cacheDir = env.DATA_CACHE_DIR; | |||
| const reposPath = path.join(cacheDir, 'repos'); | |||
There was a problem hiding this comment.
Removed console.log(process.cwd()) statement which may have been used for debugging. Please confirm this removal is intentional and will not impact debugging or logging requirements.
This PR adds a default value for the required variable
DATA_CACHE_DIRin.env.development.Summary by CodeRabbit