Skip to content

chore: Resolve DATA_CACHE_DIR in .env.development #306

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
May 12, 2025
Merged

chore: Resolve DATA_CACHE_DIR in .env.development #306

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion .env.development
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,8 @@ AUTH_URL="http://localhost:3000"
# AUTH_GOOGLE_CLIENT_ID=""
# AUTH_GOOGLE_CLIENT_SECRET=""

#DATA_CACHE_DIR="" # Path to the sourcebot cache dir (ex. ~/sourcebot/.sourcebot)
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)
Comment on lines +26 to +27
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +26 to +27
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +26 to +27
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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}.


# Email
# EMAIL_FROM_ADDRESS="" # The from address for transactional emails.
Expand Down
1 change: 0 additions & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The script 'dev:review-agent' was removed. Verify that this script is no longer required for development to avoid broken workflows.

"watch:schemas": "yarn workspace @sourcebot/schemas watch",
"dev:prisma:migrate:dev": "yarn with-env yarn workspace @sourcebot/db prisma:migrate:dev",
Expand Down
2 changes: 1 addition & 1 deletion packages/backend/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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\"",
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

"dev": "node ./dist/index.js",
"build": "tsc",
"test": "cross-env SKIP_ENV_VALIDATION=1 vitest --config ./vitest.config.ts"
Expand Down
3 changes: 0 additions & 3 deletions packages/backend/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
import "./instrument.js";

import * as Sentry from "@sentry/node";
import { ArgumentParser } from "argparse";
import { existsSync } from 'fs';
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove the unused import statement 'import { ArgumentParser } from "argparse";' instead of just deleting the line to avoid confusion or future errors.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove the unused import statement for ArgumentParser since it has been deleted from the imports, to avoid confusion or clutter in the import section.

import { mkdir } from 'fs/promises';
import path from 'path';
Expand Down Expand Up @@ -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');
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove the commented-out console.log statement or add a comment explaining why it was removed, to keep the code clean and maintainable.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

const indexPath = path.join(cacheDir, 'index');
Expand Down