🐛 Fix config for default data dir #4535
Conversation
actual-github-bot
bot
changed the title
✅ Deploy Preview for actualbudget ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
lelemm
changed the title
WalkthroughThe changes modify the configuration loading process in the application. The logic determining the default data directory now first checks for the environment variable 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (6)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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: 1
🧹 Nitpick comments (1)
packages/sync-server/src/load-config.js (1)
14-18: Consider platform compatibility for the hardcoded/datapath.The hardcoded
/datapath might cause issues on Windows systems where absolute paths are formatted differently. Consider using a platform-agnostic approach or path.join with appropriate components for cross-platform compatibility.const defaultDataDir = process.env.ACTUAL_DATA_DIR ? process.env.ACTUAL_DATA_DIR - : fs.existsSync('/data') - ? '/data' + : fs.existsSync(path.resolve('/data')) + ? path.resolve('/data') : projectRoot;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
upcoming-release-notes/4535.mdis excluded by!**/*.md
📒 Files selected for processing (1)
packages/sync-server/src/load-config.js(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: build (macos-latest)
- GitHub Check: build (windows-latest)
- GitHub Check: build (ubuntu-latest)
- GitHub Check: Build Docker image (alpine)
- GitHub Check: Build Docker image (ubuntu)
🔇 Additional comments (3)
packages/sync-server/src/load-config.js (3)
272-274: Consistent use of the new default data directory for config.json.This existing code correctly uses the
defaultDataDirfor locating the config.json file if it's not found in the project root. This aligns with the changes made to thedefaultDataDirvariable and ensures consistency across the application.
14-18:✅ Verification successful
Improved configuration hierarchy for default data directory.
The new logic provides a clearer hierarchy for determining the default data directory:
- First, use the
ACTUAL_DATA_DIRenvironment variable if set- Check if
/datadirectory exists and use it as fallback- Ultimately default to
projectRootif neither option is availableThis enhancement increases flexibility while maintaining backward compatibility.
🏁 Script executed:
#!/bin/bash # Description: Check if these changes properly address the issue of missing default values # Verify that this module is used wherever configuration is loaded rg -A 3 "load-config|defaultDataDir" --type js # Check if the ACTUAL_DATA_DIR env var is referenced elsewhere rg "ACTUAL_DATA_DIR" --type jsLength of output: 4905
Default Data Directory Configuration Verified
The new logic in
packages/sync-server/src/load-config.js(lines 14–18) correctly implements the intended hierarchy:
- Use the
ACTUAL_DATA_DIRenvironment variable if available- Fall back to
/dataif it exists- Default to
projectRoototherwiseSearch results confirm that the configuration is referenced consistently across the codebase, ensuring backward compatibility and flexibility.
82-82:✅ Verification successful
Fixed configuration paths to use the correct data directory.
This change properly directs server and user files to the determined data directory rather than always using the project root. This ensures consistent file storage location based on the configured data directory.
Also applies to: 91-91
🏁 Script executed:
#!/bin/bash # Description: Verify the impact of these path changes on the application # Check where serverFiles and userFiles are accessed rg "serverFiles|userFiles" --type js # Verify if there are any hardcoded paths that might need to be updated rg -A 1 "server-files|user-files" --type jsLength of output: 2400
Configuration Paths Update Confirmed
The changes in
packages/sync-server/src/load-config.jsfor both lines 82 and 91 now correctly usedefaultDataDirrather than falling back to the project root. The search results confirm that all references toserverFilesanduserFilesacross the codebase correctly use the new configuration values, ensuring that file operations (including those in migrations and utility functions) point to the intended data directory.
- Files verified:
packages/sync-server/src/load-config.js(both server and user file paths)- Related usages in migrations and utility functions
The update properly addresses the hardcoded paths and aligns with our goal for consistent file storage based on the configured data directory.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/sync-server/src/load-config.js (1)
14-18: Consider formatting the ternary operators for better readability.The nested ternary operators could benefit from consistent indentation as flagged by the linter.
const defaultDataDir = process.env.ACTUAL_DATA_DIR ? process.env.ACTUAL_DATA_DIR - : fs.existsSync('/data') - ? '/data' - : projectRoot; + : fs.existsSync('/data') + ? '/data' + : projectRoot;🧰 Tools
🪛 ESLint
[error] 17-17: Insert
··(prettier/prettier)
[error] 18-18: Insert
··(prettier/prettier)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/sync-server/src/load-config.js(4 hunks)
🧰 Additional context used
🪛 ESLint
packages/sync-server/src/load-config.js
[error] 17-17: Insert ··
(prettier/prettier)
[error] 18-18: Insert ··
(prettier/prettier)
🔇 Additional comments (5)
packages/sync-server/src/load-config.js (5)
14-18: Enhanced default data directory logic with environment variable support.The updated approach for determining the default data directory now follows a clear priority order:
- Use
ACTUAL_DATA_DIRenvironment variable if set- Use
/datadirectory if it exists- Fall back to
projectRootif neither of the above is availableThis change improves configurability while maintaining backward compatibility.
Consider adding write permission verification to ensure the selected directory is writable:
const defaultDataDir = process.env.ACTUAL_DATA_DIR ? process.env.ACTUAL_DATA_DIR : fs.existsSync('/data') ? '/data' : projectRoot; + +// Verify the directory is writable +try { + fs.accessSync(defaultDataDir, fs.constants.W_OK); + debug(`Verified write access to data directory: ${defaultDataDir}`); +} catch (err) { + console.warn(`Warning: Selected data directory "${defaultDataDir}" is not writable. This may cause problems when writing data.`); +}This addresses the previous review comment about directory write permission verification.
🧰 Tools
🪛 ESLint
[error] 17-17: Insert
··(prettier/prettier)
[error] 18-18: Insert
··(prettier/prettier)
61-61: Updated default data directory for configuration consistency.The config schema now correctly uses the newly defined
defaultDataDirwhen not in test mode, ensuring consistency with the data directory logic defined earlier.
82-82: Updated server files path to use the same data directory.Server files are now correctly stored in the configured data directory instead of the project root, which aligns with the fix for data directory defaults.
91-91: Updated user files path to use the same data directory.User files are now correctly stored in the configured data directory instead of the project root, which aligns with the fix for data directory defaults.
14-18:❓ Verification inconclusive
Verify config file path resolution works correctly with the new data directory logic.
The code now checks for a config file in
defaultDataDirif not found in the project root. Let's ensure this works as expected with the updateddefaultDataDirdetermination logic.Also applies to: 272-274
🏁 Script executed:
#!/bin/bash # Description: Verify config file path resolution with the new data directory logic # 1. Check for any other places in the codebase that might use the config path echo "Looking for other locations that use config paths:" rg "config\.json|configPath" --type js # 2. Check if there are any tests that might need to be updated echo "Checking tests that might need to be updated:" rg "config\.json|configPath" --type js --glob "**/test/**" # 3. Look for places where directories are created to ensure the data directory exists echo "Checking if data directories are created elsewhere:" rg "mkdir|mkdirSync" --type js | grep -E "serverFiles|userFiles|dataDir"Length of output: 1451
Action Required: Manually Verify Config File Resolution Logic
The updated config resolution in
packages/sync-server/src/load-config.jsappears to use the newdefaultDataDirlogic correctly (i.e. usingprocess.env.ACTUAL_DATA_DIR, then/dataif available, otherwise defaulting toprojectRoot). However, note that our automated test search returned no matching test files and no evidence of data directory creation elsewhere. Please manually verify that:
- The logic in lines 14–18 (and the similar logic in lines 272–274) correctly falls back to loading the config file from
defaultDataDirwhen it’s not found inprojectRoot.- Existing tests (if any) or manual test scenarios cover this fallback behavior, especially when switching between the various configuration scenarios.
- The absence of test file matches from the automated search is expected and does not hide needed adjustments in test cases.
🧰 Tools
🪛 ESLint
[error] 17-17: Insert
··(prettier/prettier)
[error] 18-18: Insert
··(prettier/prettier)
When transitioning from old to new load-config we lost some default values for user data. This fix it