feat: Introduce persistent user config file#506
Conversation
ENG-641 Add Baseplate user settings file to allow global user configurations
Implement a user settings file for Baseplate that allows global configuration preferences to be stored persistently, replacing the current approach of using environment variables. This will provide a more user-friendly way to configure Baseplate across projects and enable more complex configuration options. |
🦋 Changeset detectedLatest commit: a4bf643 The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
WalkthroughThis change introduces support for persistent user configuration in the Baseplate project builder ecosystem. A new configuration file at Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant UserConfigService
participant Server
participant BuilderServiceManager
participant ProjectBuilderService
User->>CLI: run build or serve command
CLI->>UserConfigService: getUserConfig()
UserConfigService-->>CLI: returns userConfig
CLI->>Server: start/build with userConfig
Server->>BuilderServiceManager: instantiate with userConfig
BuilderServiceManager->>ProjectBuilderService: instantiate with userConfig
ProjectBuilderService->>ProjectBuilderService: buildProject(userConfig)
ProjectBuilderService->>...: downstream functions receive userConfig
sequenceDiagram
participant ProjectBuilderService
participant generateForDirectory
ProjectBuilderService->>generateForDirectory: call with userConfig
generateForDirectory->>generateForDirectory: use userConfig.sync options
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
🪧 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
🔭 Outside diff range comments (1)
packages/project-builder-server/src/sync/index.ts (1)
23-240: 💡 Verification agent🧩 Analysis chain
Consider adding fallback for backward compatibility
The changes migrate from environment variables to a user configuration object without providing fallbacks to the original environment variables. This might cause issues for existing users who haven't migrated to the new configuration system yet.
Consider adding fallbacks to maintain backward compatibility during the transition period:
mergeDriver: userConfig.sync?.customMergeDriver ? { name: 'baseplate-custom-merge-driver', driver: userConfig.sync.customMergeDriver, } : undefined,Let's verify if there are any environment variable fallbacks implemented elsewhere in the codebase:
🏁 Script executed:
#!/bin/bash # Check if environment variables are still used or if fallbacks are implemented # Search for BASEPLATE_CUSTOM_MERGE_DRIVER and BASEPLATE_WRITE_GENERATOR_STEPS_JSON echo "Searching for environment variable references:" rg "BASEPLATE_CUSTOM_MERGE_DRIVER|BASEPLATE_WRITE_GENERATOR_STEPS_JSON" --type ts # Check if there's any migration logic or fallbacks implemented echo "\nChecking for potential migration or fallback logic:" rg "environment.*fallback|fallback.*environment|process\.env" --type ts -A 3 -B 3 packages/project-builder-server/src/user-config/Length of output: 436
Add environment‐variable fallbacks for backward compatibility
The sync code now only reads from
userConfig.sync, but we didn’t preserve the oldprocess.envflags. To avoid breaking existing users, add fallbacks to the original env vars:• File:
packages/project-builder-server/src/sync/index.ts
Location: around themergeDriveroption ingenerateForDirectory
Suggestion:- mergeDriver: userConfig.sync?.customMergeDriver - ? { - name: 'baseplate-custom-merge-driver', - driver: userConfig.sync.customMergeDriver, - } - : undefined, + mergeDriver: + userConfig.sync?.customMergeDriver + ? { + name: 'baseplate-custom-merge-driver', + driver: userConfig.sync.customMergeDriver, + } + : process.env.BASEPLATE_CUSTOM_MERGE_DRIVER + ? { + name: 'baseplate-custom-merge-driver', + driver: process.env.BASEPLATE_CUSTOM_MERGE_DRIVER, + } + : undefined,• File: same—around the
writeGeneratorStepsJsoncheck
Suggestion:- if (userConfig.sync?.writeGeneratorStepsJson && output.metadata) { + const writeSteps = + userConfig.sync?.writeGeneratorStepsJson ?? + (process.env.BASEPLATE_WRITE_GENERATOR_STEPS_JSON === 'true'); + if (writeSteps && output.metadata) {This preserves the old
BASEPLATE_CUSTOM_MERGE_DRIVERandBASEPLATE_WRITE_GENERATOR_STEPS_JSONbehavior until users fully migrate touserConfig.sync.
🧹 Nitpick comments (7)
packages/project-builder-server/src/user-config/user-config-schema.ts (2)
3-16: The schema looks good, but could benefit from more comprehensive documentation.The schema defines the user configuration structure well using Zod. However, consider adding a top-level JSDoc comment that explains the overall purpose of this configuration file, where it's stored, and how it's used by the system.
import { z } from 'zod'; +/** + * Schema for validating user configuration stored in ~/.baseplate/config.json + * This configuration allows users to persist preferences across sessions. + */ export const userConfigSchema = z.object({ sync: z .object({ /** * Whether to write the generator steps JSON file */ writeGeneratorStepsJson: z.boolean().optional().default(false), /** * The custom merge driver to use */ customMergeDriver: z.string().optional(), }) .optional(), });
13-13: Consider adding validation for the customMergeDriver string.The
customMergeDriveris defined as an arbitrary string without constraints. If there are specific formats or values expected for this field, adding validation would help users avoid errors./** * The custom merge driver to use */ - customMergeDriver: z.string().optional(), + customMergeDriver: z.string().min(1).optional(),packages/project-builder-cli/src/services/user-config.ts (2)
11-14: Fixed configuration path may limit flexibility.The configuration path is hardcoded to
.baseplate/config.jsonin the user's home directory. Consider adding support for an environment variable to override this location.function getConfigPath(): string { const homeDir = os.homedir(); - return path.join(homeDir, '.baseplate', 'config.json'); + const configPathOverride = process.env.BASEPLATE_CONFIG_PATH; + return configPathOverride || path.join(homeDir, '.baseplate', 'config.json'); }
21-27: Good error handling for missing configuration files, but consider enhancing error handling.The function gracefully handles missing config files by falling back to an empty object. However, it doesn't handle other potential errors such as JSON parsing errors or permission issues.
export async function getUserConfig(): Promise<BaseplateUserConfig> { const configPath = getConfigPath(); - const config = await readJsonWithSchema(configPath, userConfigSchema).catch( - handleFileNotFoundError, - ); - return config ?? {}; + try { + const config = await readJsonWithSchema(configPath, userConfigSchema); + return config; + } catch (error) { + if (error instanceof Error && 'code' in error && error.code === 'ENOENT') { + // File not found, return empty config + return {}; + } + + // For other errors (like permission issues or malformed JSON), log and return empty config + console.error(`Error loading config from ${configPath}:`, error); + return {}; + } }packages/project-builder-server/src/service/builder-service.ts (1)
28-29: Consider using the imported type from the index file.For consistency, consider importing BaseplateUserConfig from the index file rather than directly from the schema file, especially if other files follow this pattern.
-import type { BaseplateUserConfig } from '../user-config/user-config-schema.js'; +import type { BaseplateUserConfig } from '../user-config/index.js';packages/project-builder-server/src/sync/index.ts (2)
169-173: Custom merge driver configuration properly migratedThe code now correctly uses
userConfig.sync?.customMergeDriverinstead of the previous environment flag. The optional chaining operator is appropriately used to handle cases where thesyncproperty might be undefined.However, consider adding a comment documenting the mapping from the old environment variable name to the new configuration property for future reference.
mergeDriver: userConfig.sync?.customMergeDriver ? { name: 'baseplate-custom-merge-driver', + // Previously controlled by BASEPLATE_CUSTOM_MERGE_DRIVER environment variable driver: userConfig.sync.customMergeDriver, } : undefined,
240-240: Generator steps JSON writing configuration migrated properlyThe conditional check for writing generator steps JSON is now properly using
userConfig.sync?.writeGeneratorStepsJsoninstead of the environment flag.Similarly to the merge driver configuration, consider adding a comment about the previous environment variable:
+ // Previously controlled by BASEPLATE_WRITE_GENERATOR_STEPS_JSON environment variable if (userConfig.sync?.writeGeneratorStepsJson && output.metadata) { await writeGeneratorSteps(output.metadata, projectDirectory); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Lite
📒 Files selected for processing (14)
.changeset/loud-pandas-create.md(1 hunks)packages/project-builder-cli/src/commands/build.ts(2 hunks)packages/project-builder-cli/src/commands/server.ts(2 hunks)packages/project-builder-cli/src/services/user-config.ts(1 hunks)packages/project-builder-server/src/index.ts(1 hunks)packages/project-builder-server/src/runner/index.ts(3 hunks)packages/project-builder-server/src/server/builder-service-manager.ts(3 hunks)packages/project-builder-server/src/service/builder-service.ts(5 hunks)packages/project-builder-server/src/service/environment-flags.ts(0 hunks)packages/project-builder-server/src/sync/index.ts(5 hunks)packages/project-builder-server/src/user-config/index.ts(1 hunks)packages/project-builder-server/src/user-config/user-config-schema.ts(1 hunks)packages/project-builder-test/src/commands/serve.ts(1 hunks)packages/project-builder-test/src/runner/generate.ts(1 hunks)
💤 Files with no reviewable changes (1)
- packages/project-builder-server/src/service/environment-flags.ts
🧰 Additional context used
🧬 Code Graph Analysis (4)
packages/project-builder-server/src/server/builder-service-manager.ts (1)
packages/project-builder-server/src/user-config/user-config-schema.ts (1)
BaseplateUserConfig(18-18)
packages/project-builder-server/src/sync/index.ts (1)
packages/project-builder-server/src/user-config/user-config-schema.ts (1)
BaseplateUserConfig(18-18)
packages/project-builder-server/src/service/builder-service.ts (1)
packages/project-builder-server/src/user-config/user-config-schema.ts (1)
BaseplateUserConfig(18-18)
packages/project-builder-server/src/runner/index.ts (1)
packages/project-builder-server/src/user-config/user-config-schema.ts (1)
BaseplateUserConfig(18-18)
🔇 Additional comments (22)
packages/project-builder-test/src/commands/serve.ts (1)
88-88: Implementation looks good, consider adding a comment for context.The addition of
userConfig: {}maintains compatibility with the new persistent user configuration feature. Since this is test code, an empty object is appropriate as a default value. Consider adding a brief comment to explain why an empty configuration is being used here.builtInPlugins, cliVersion, + // Empty user config for testing purposes userConfig: {}, });packages/project-builder-cli/src/commands/server.ts (3)
13-14: LGTM: Clean import for user config service.The import is correctly placed and follows the project's import ordering pattern.
49-50: LGTM: Proper async loading of user configuration.The user configuration is correctly loaded asynchronously before being needed by the service manager.
55-55: LGTM: Configuration correctly passed to service manager.The user configuration is properly passed to the BuilderServiceManager constructor.
packages/project-builder-server/src/index.ts (1)
8-8: LGTM: Proper export of user configuration module.This export makes the user configuration functionality available to consumers of this package, maintaining consistency with how other modules are exposed.
packages/project-builder-test/src/runner/generate.ts (1)
20-20: Implementation looks good, consider adding a comment for context.The addition of
userConfig: {}maintains compatibility with the new persistent user configuration feature. Consider adding a brief comment to explain why an empty configuration is appropriate in this test context.logger, context: nodeSchemaParserContext, + // Empty user config for testing purposes userConfig: {}, });packages/project-builder-server/src/user-config/index.ts (1)
1-1: LGTM: Clean module re-export patternThis simple re-export pattern is a good practice for organizing and centralizing exports, making it easy to import all user configuration related code from a single entry point.
.changeset/loud-pandas-create.md (1)
2-4: LGTM: Proper patch version bumpsAppropriate patch version updates for both packages, as this adds functionality in a backward compatible way.
packages/project-builder-cli/src/commands/build.ts (3)
6-6: LGTM: Clean import of new user config functionalityGood addition of the getUserConfig import from the new user-config service.
26-26: LGTM: Proper async user config retrievalCorrectly retrieves user configuration asynchronously before passing it to the build function.
31-31: LGTM: Proper integration of user configThe userConfig is correctly passed to the buildProjectForDirectory function, completing the integration of persistent user configuration.
packages/project-builder-server/src/server/builder-service-manager.ts (3)
5-5: LGTM: Clean type importCorrectly imports the BaseplateUserConfig type from the user-config-schema module.
17-17: LGTM: Required userConfig parameter in constructorGood practice to require the userConfig as part of the constructor options object, ensuring it's provided when creating a BuilderServiceManager instance.
37-37: LGTM: Proper propagation of user configThe userConfig is correctly passed down to the ProjectBuilderService, ensuring consistent configuration throughout the system.
packages/project-builder-server/src/runner/index.ts (2)
47-52: Making userConfig a required property ensures consistent configuration throughout the system.The addition of the required userConfig property to the BuildProjectForDirectoryOptions interface is appropriate. It ensures that configuration is consistently passed through the build process.
64-71: Proper propagation of userConfig to downstream functions.The userConfig is correctly passed to the generateForDirectory function for each application, ensuring that user preferences are applied consistently.
packages/project-builder-server/src/service/builder-service.ts (3)
65-71: Adding userConfig as a required option is consistent with the project's design.Making userConfig a required property in ProjectBuilderServiceOptions ensures that it's always provided and propagated through the system.
99-100: Proper storage and initialization of the userConfig property.The userConfig is correctly stored as a private property and initialized in the constructor, following the class's pattern for managing configuration.
Also applies to: 106-107, 117-118
235-241: Correctly passing userConfig to the buildProjectForDirectory function.The userConfig is properly passed to the buildProjectForDirectory function, ensuring that user preferences are applied during the build process.
packages/project-builder-server/src/sync/index.ts (3)
23-23: Well-formed import of BaseplateUserConfig typeThe change introduces the
BaseplateUserConfigtype import to replace the previousenvironmentFlagsimport, aligning with the new persistent configuration approach.
38-38: Interface correctly extended with userConfigThe
GenerateForDirectoryOptionsinterface now properly includes the requireduserConfigproperty of typeBaseplateUserConfig, ensuring type safety for the new configuration pattern.
110-110: Parameter correctly updated in function signatureThe
generateForDirectoryfunction parameter destructuring now includes theuserConfigparameter, consistent with the updated interface definition.
kingston
deleted the
kingston/eng-641-add-baseplate-user-settings-file-to-allow-global-user
branch
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Chores