🔨 Refactoring load-config.js #4440
Conversation
✅ Deploy Preview for actualbudget ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
…emm/actual into maintenance/config_refactory
WalkthroughThe changes update the configuration management throughout the repository by replacing direct property accesses with calls to the Suggested labels
Suggested reviewers
✨ Finishing Touches
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: 0
🧹 Nitpick comments (5)
packages/sync-server/src/scripts/enable-openid.js (1)
24-24: Use config alias to avoid parameter naming conflicts.The change to use
config.getProperties()is good, but based on the project's conventions, whenconfigis used as a function parameter, the imported configuration should use the aliasfinalConfigto avoid naming conflicts.- import { config } from '../load-config.js'; + import { config as finalConfig } from '../load-config.js'; // Then update line 24 to use finalConfig - const { error } = (await enableOpenID(config.getProperties())) || {}; + const { error } = (await enableOpenID(finalConfig.getProperties())) || {};packages/sync-server/src/scripts/health-check.js (1)
5-8: Improved protocol handling with explicit key and cert checksThe protocol determination logic has been updated to specifically check for both
https.keyandhttps.certrather than a simple boolean flag. This provides more explicit validation that all requirements for HTTPS are present before using it.packages/sync-server/src/app.js (1)
108-117: Mixed usage ofconfig.httpsandconfig.get('https.key')/config.get('https.cert').
While callingconfig.get('https.key')andconfig.get('https.cert')is correct, consider also replacing...config.httpswith an object destructured fromconfig.get('https')for full consistency.packages/sync-server/src/load-config.js (1)
189-211: Validation and sensitive output masking.
Good job validating with{ allowed: 'strict' }and obscuring HTTPS secrets in debug logs. For further security, consider secrets management to avoid storing them on-disk in plain text.packages/sync-server/src/accounts/openid.js (1)
5-5: Consider renaming the imported config to avoid naming conflicts.Based on project conventions, the imported
configshould be renamed tofinalConfigwhenconfigis used as a function parameter to avoid naming conflicts. This practice is established elsewhere in the codebase.-import { config } from '../load-config.js'; +import { config as finalConfig } from '../load-config.js';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
upcoming-release-notes/4440.mdis excluded by!**/*.mdyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (13)
packages/sync-server/migrations/1694360000000-create-folders.js(1 hunks)packages/sync-server/package.json(2 hunks)packages/sync-server/src/account-db.js(3 hunks)packages/sync-server/src/accounts/openid.js(6 hunks)packages/sync-server/src/accounts/password.js(1 hunks)packages/sync-server/src/app.js(5 hunks)packages/sync-server/src/config-types.ts(0 hunks)packages/sync-server/src/load-config.js(1 hunks)packages/sync-server/src/migrations.js(1 hunks)packages/sync-server/src/scripts/enable-openid.js(1 hunks)packages/sync-server/src/scripts/health-check.js(1 hunks)packages/sync-server/src/util/paths.js(1 hunks)packages/sync-server/src/util/validate-user.js(1 hunks)
💤 Files with no reviewable changes (1)
- packages/sync-server/src/config-types.ts
🧰 Additional context used
🧠 Learnings (2)
packages/sync-server/src/scripts/enable-openid.js (1)
Learnt from: matt-fidd
PR: actualbudget/actual#4363
File: packages/sync-server/src/scripts/enable-openid.js:6-6
Timestamp: 2025-02-11T23:39:06.219Z
Learning: In the Actual codebase, when `config` is used as a function parameter, the imported configuration should use the alias `finalConfig` to avoid naming conflicts. Example: `import { config as finalConfig } from '../load-config.js'`.
packages/sync-server/src/accounts/openid.js (1)
Learnt from: matt-fidd
PR: actualbudget/actual#4363
File: packages/sync-server/src/scripts/enable-openid.js:6-6
Timestamp: 2025-02-11T23:39:06.219Z
Learning: In the Actual codebase, when `config` is used as a function parameter, the imported configuration should use the alias `finalConfig` to avoid naming conflicts. Example: `import { config as finalConfig } from '../load-config.js'`.
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: Visual regression
- GitHub Check: Functional
- GitHub Check: build (windows-latest)
- GitHub Check: Build Docker image (alpine)
- GitHub Check: build (ubuntu-latest)
- GitHub Check: Build Docker image (ubuntu)
🔇 Additional comments (27)
packages/sync-server/package.json (1)
50-50: Added dependencies for configuration management with convict.The addition of
convictand its TypeScript type definitions supports the refactoring of configuration management throughout the codebase, moving from direct property access to a schema-based approach.Also applies to: 60-60
packages/sync-server/src/util/validate-user.js (1)
49-50: Configuration access standardized with config.get().Good change to maintain consistency with the rest of the codebase by replacing direct property access with the
config.get()method while preserving the fallback logic.packages/sync-server/src/migrations.js (1)
15-18: Standardized configuration access using config.get().Good implementation of the refactoring strategy, consistently using
config.get()for all configuration values rather than direct property access.packages/sync-server/src/accounts/password.js (1)
88-94: Standardized configuration access withconfig.get()The change from direct property access to the
config.get()method aligns with the PR's refactoring goal of standardizing configuration access throughout the codebase.packages/sync-server/src/util/paths.js (2)
7-7: Standardized configuration access in getPathForUserFileUsing
config.get('userFiles')instead of direct property access aligns with the PR's refactoring goal.
12-12: Standardized configuration access in getPathForGroupFileUsing
config.get('userFiles')instead of direct property access aligns with the PR's refactoring goal.packages/sync-server/migrations/1694360000000-create-folders.js (2)
18-19: Standardized configuration access in migration up functionThe change to use
config.get()instead of direct property access for bothserverFilesanduserFilesis consistent with the PR's refactoring goal.
23-24: Standardized configuration access in migration down functionThe change to use
config.get()instead of direct property access for bothserverFilesanduserFilesis consistent with the PR's refactoring goal.packages/sync-server/src/app.js (7)
26-26: Consistent use ofconfig.get('trustedProxies').
This change aligns with the new schema-based configuration approach, ensuring all config values are retrieved uniformly.
38-40: Switched toconfig.get('upload.fileSizeLimitMB').
The new usage simplifies retrieval of this limit. Confirm thatfileSizeLimitMBis suitably validated in the schema.
44-45: Use ofconfig.get('upload.fileSizeSyncLimitMB').
The transition away from direct property access is consistent with the rest of the refactor.
50-51: Adoption ofconfig.get('upload.syncEncryptedFileSizeLimitMB').
Good job maintaining consistency. Ensure this limit adequately protects against large uploads.
64-64: Replaced direct property access withconfig.get('mode').
This is consistent with the new schema pattern. No issues are apparent here.
94-97: Switched to serving static files fromconfig.get('webRoot').
This maintains uniform usage of config values. Confirm that the configured path is correct in production.
119-124: Adoption ofconfig.get('port')andconfig.get('hostname').
These changes unify server listening configuration with the updated schema approach.packages/sync-server/src/account-db.js (4)
14-14: Use ofconfig.get('serverFiles').
Efficiently retrieves the path for the account database. This helps maintain a single source of truth from the convict schema.
32-34: Incorporation ofconfig.get('enforceOpenId').
Ensures that OpenID enforcement is taken from the schema. The conditional logic is correct given the new config approach.
60-61: Use ofconfig.get('allowedLoginMethods').
Good shift to the standardizedgetmethod. Confirm edge cases whereallowedLoginMethodsmight be empty or missing are handled gracefully in the schema.
65-66: Fallback toconfig.get('loginMethod').
Using the convict-based approach for login method retrieval improves consistency and maintainability.packages/sync-server/src/load-config.js (4)
6-6: Introduction ofconvict.
Adoptingconvictis a solid move for declaring and validating configuration in a structured manner.
24-32: Implementation of customtokenExpirationformat.
This is a valid custom convict format. It handles both numeric and explicit string values (like'never'). No issues found.
34-185: Schema definitions partially omit'production'frommode.
Your schema allowsenv: ['production', 'development', 'test']butmodeonly includes['test', 'development']. Verify that excluding'production'frommodeis intentional.
187-188:configSchema.loadFile('config.json')usage.
This is the standard convict pattern. Ensure that a missing or invalidconfig.jsonis handled gracefully.packages/sync-server/src/accounts/openid.js (4)
12-15: Parameter renaming improves clarity, but check validation logic.The parameter rename from
configtoconfigParameteris good for clarity. The added check fordiscoveryURLimproves validation, but consider how this relates to line 52 where it appears only one ofissuerordiscoveryURLis required.
141-149: Good variable rename for clarity.Renaming
configtoconfigFromDbclearly indicates this is configuration data retrieved from the database, reducing confusion with the importedconfigmodule.
266-273: Consistent use of config.get() improves configuration management.The change to use
config.get('token_expiration')instead of direct property access aligns with the refactoring objective of standardizing configuration access across the codebase.However, there might be confusion about why direct property access is used for
configFromDbwhileconfig.get()is used for token expiration. Consider adding a comment explaining the different sources of configuration.
51-59:✅ Verification successful
Verify that the issuer determination logic is consistent with previous behavior.
The changes to the issuer discovery process now allow for either
discoveryURLor an explicit issuer configuration. Ensure this maintains backward compatibility with existing configurations.
🏁 Script executed:
#!/bin/bash # Check other places in the codebase where OpenID clients are created to ensure consistent behavior rg -A 3 "new Issuer" --glob "*.js"Length of output: 418
Issuer determination logic verified: No issues found.
The updated code correctly maintains the prior behavior by choosing between issuer discovery via
discoveryURLand using an explicit issuer configuration. Our search confirmed that the explicit issuer instantiation remains consistent with earlier implementations, with no further instances or conflicts detected.
lelemm
changed the title
lelemm
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/sync-server/src/load-config.js (1)
269-269: Consider providing a helper method for accessing nested configuration.The export renames
configSchematoconfig, which is good for backward compatibility. For improved developer experience when accessing deeply nested properties, consider adding utility methods to make access more concise.- export { configSchema as config }; + const config = configSchema; + + // Helper for cleaner nested property access + config.getPath = function(path) { + return this.get(path); + }; + + export { config };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/sync-server/src/load-config.js(1 hunks)
🔇 Additional comments (6)
packages/sync-server/src/load-config.js (6)
6-6: Good choice of configuration library.Using
convictis a solid approach for configuration management as it provides schema validation, format checking, and centralized configuration management.
24-32: Well-implemented custom format validation.The custom format for token expiration is thorough, handling multiple valid use cases ('never', 'openid-provider', or a non-negative number) and providing clear error messages.
35-239: Well-structured configuration schema with comprehensive documentation.The schema is well-organized with appropriate documentation, default values, environment variable mappings, and format validations for each property. The nested objects for complex configurations like HTTPS and OpenID follow good practices.
241-243: Good defensive programming with config file loading.The code checks for the existence of the config file before attempting to load it, preventing potential errors when the file doesn't exist.
245-245: Strict validation ensures configuration integrity.Using
{ allowed: 'strict' }for validation prevents unknown properties in the configuration, which helps catch typos and configuration errors early.
257-267: Good security practice for sensitive information logging.The code properly masks sensitive information in regular debug logs while still providing access to the full values in sensitive debug logs for troubleshooting when needed.
Refactoring load-config.js