New: Add config migrations mechanism #648
Conversation
WalkthroughThis pull request enhances dependency management and configuration versioning. The changes add a new Zod dependency in Changes
Sequence Diagram(s)sequenceDiagram
participant C as getConfig
participant M as migrateConfig
participant O as isOriginalConfig
participant V as migrateOriginalToV1
participant L as LatestSchema Validator
C->>M: Pass parsed config object
M->>O: Check if config is original format
alt Config is original
O-->>M: true
M->>V: Migrate original to V1
else Not original
O-->>M: false
end
M->>L: Apply migrations & validate config
L-->>M: Validated config (with version)
M-->>C: Return updated config
Poem
✨ 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
🔭 Outside diff range comments (1)
packages/preload/src/commonTypes.ts (1)
17-33:⚠️ Potential issueRemove duplicate Config interface definition.
The file contains two identical definitions of the Config interface. This duplication could lead to maintenance issues and inconsistencies.
Keep the first definition (lines 17-33) and remove the duplicate (lines 221-237).
Also applies to: 221-237
🧹 Nitpick comments (4)
packages/main/src/backend/configManager/configMigration/configMigrator.ts (2)
8-10: Avoid usinganyin themigrationsrecord.
Consider providing a stricter type for both the input and output of each migration function to improve type safety and clarity.-const migrations: Record<number, (config: any) => any> = {}; +import type { ZodTypeAny } from 'zod'; +const migrations: Record<number, (config: unknown) => unknown> = {};
12-26: Consider handling parsing errors more gracefully.
Currently, iflatestConfigSchema.parse()throws, the error will bubble up. This may be acceptable, but wrapping it in user-friendly error logging could aid troubleshooting.packages/main/src/backend/configManager/configMigration/versions/original.ts (2)
31-31: Consider replacinganytype with more specific Zod schemas.The use of
z.any()forcredentialsandloginFieldsfields reduces type safety. Consider defining specific schemas for these fields to ensure proper validation.For the
credentialsfield, consider:- credentials: z.any(), + credentials: z.object({ + access_token: z.string(), + refresh_token: z.string().optional(), + scope: z.string(), + token_type: z.string(), + expiry_date: z.number().optional() + }),For the
loginFieldsfield, consider:- loginFields: z.any(), + loginFields: z.record(z.string(), z.string()),Also applies to: 71-71
85-89: Consider adding a default version field to the original schema.Since this schema represents the original configuration format, it might be helpful to explicitly set a version (e.g., 0) to distinguish it from migrated configurations.
export const originalConfigSchema = z.object({ + version: z.literal(0), outputVendors: outputVendorsSchema, scraping: scrapingSchema, useReactUI: z.boolean().optional(), });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (10)
package.json(1 hunks)packages/main/src/backend/commonTypes.ts(1 hunks)packages/main/src/backend/configManager/configManager.ts(2 hunks)packages/main/src/backend/configManager/configMigration/configMigrator.ts(1 hunks)packages/main/src/backend/configManager/configMigration/versions/original.ts(1 hunks)packages/main/src/backend/configManager/configMigration/versions/v1.ts(1 hunks)packages/main/src/backend/configManager/defaultConfig.ts(1 hunks)packages/preload/src/commonTypes.ts(1 hunks)packages/renderer/src/store/Store.test.tsx(1 hunks)packages/renderer/src/types.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- packages/renderer/src/store/Store.test.tsx
- packages/main/src/backend/configManager/defaultConfig.ts
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build (windows-latest)
- GitHub Check: build (macos-13)
🔇 Additional comments (13)
packages/main/src/backend/configManager/configMigration/configMigrator.ts (2)
1-4: Imports look well-organized.
All imported modules are used consistently and align with the intended functionality.
28-31: Version extraction logic is clear and concise.
Using Zod to validate and parse the version ensures reliable migration sequencing.packages/main/src/backend/configManager/configMigration/versions/v1.ts (3)
1-2: Zod import and reference to the original schema look good.
This ensures schema extensions remain type-safe and well-structured.
4-4: Extending the original schema with a literal version is a robust approach.
This design clearly declares the explicit version of the config.
6-11: Migration function neatly upgrades the config from original to v1.
This is straightforward and aligns with best practices for incremental schema evolution.packages/main/src/backend/configManager/configManager.ts (2)
5-5: ImportingmigrateConfighere ensures migrations are applied consistently.
This central point of use is an appropriate place to manage configuration version evolution.
23-23: ApplyingmigrateConfigbefore returning ensures the user always receives a valid config.
The approach integrates well with the surrounding fallback logic.packages/main/src/backend/configManager/configMigration/versions/original.ts (1)
75-83: LGTM! Well-structured scraping configuration schema.The schema properly defines required and optional fields, with appropriate types for each configuration option.
packages/main/src/backend/commonTypes.ts (1)
14-14: LGTM! Version field properly added to Config interface.The addition of the version field is well-typed and aligns with the PR objectives for configuration versioning.
packages/renderer/src/types.tsx (2)
20-20: LGTM! Version field consistently added across type definitions.The version field addition maintains consistency with the Config interface definitions in other files.
14-14:✅ Verification successful
Verify version field usage across the codebase.
Let's ensure all configuration objects include the new version field.
Also applies to: 18-18, 20-20
🏁 Script executed:
#!/bin/bash # Search for Config type usage and object definitions echo "Searching for Config type usage:" rg -p "Config.*?{" -A 5 echo "Searching for configuration object definitions:" rg -p "const.*Config.*?=.*?{" -A 10Length of output: 433
I'll generate a new shell script that uses corrected regular expressions for Ripgrep to re-run the verification of Config interface and object definitions. This will help ensure that we accurately identify configuration objects that include the new version field.
Please run the following script to collect the necessary output:
Once you verify the output and ensure that all configuration objects include the version field, please update the review comment accordingly.
🏁 Script executed:
#!/bin/bash # Corrected regex search for Config interface definitions including the version field. echo "Searching for Config interface definitions with version field:" rg --color=always -n -e "Config\s*{[^}]*version\s*:" # Corrected regex search for configuration object definitions including the version field. echo "Searching for configuration object definitions with version field:" rg --color=always -n -e "const\s+.*Config\s*=\s*{[^}]*version\s*:"Length of output: 667
Please run the following revised script, which escapes the curly braces correctly. This new version should avoid the regex errors and help gather the necessary output:
After running this, please verify the outputs and ensure that all configuration objects include the version field.
🏁 Script executed:
#!/bin/bash # Corrected regex search for Config interface definitions including the version field. echo "Searching for Config interface definitions with version field:" rg --color=always -n -e 'Config\s*\\{[^}]*version\s*:' # Corrected regex search for configuration object definitions including the version field. echo "Searching for configuration object definitions with version field:" rg --color=always -n -e 'const\s+.*Config\s*=\s*\\{[^}]*version\s*:'Length of output: 679
Please run the following updated verification script. It uses fixed‑string searches with context so you can manually inspect the output and confirm that every Config interface or configuration object includes the new version field:
After running this script, manually verify that each Config block (for example, in file
packages/renderer/src/types.tsxat lines 14, 18, and 20) properly includes the version field.Verify that every configuration interface/object includes the new
versionproperty.
🏁 Script executed:
#!/bin/bash echo "=== Searching for occurrences of 'Config {' with context (possible interface blocks) ===" rg -n -F "Config {" --context 5 . echo "" echo "=== Searching for occurrences of 'version:' with context in files ===" rg -n -F "version:" --context 5 .Length of output: 16505
Version Field Verification Completed
After manually verifying the occurrences across the codebase, all Config interfaces and configuration objects consistently include the new
versionfield. This can be seen in the following files:
- packages/main/src/backend/commonTypes.ts — Config interface has
version: number;- packages/renderer/src/types.tsx — Config interface includes
version: number;- packages/preload/src/commonTypes.ts — Config interface shows
version: number;- Additionally, default configuration objects (e.g., in
defaultConfig.ts) correctly define the version property.No further changes are necessary.
package.json (2)
101-102: Added new dependency: Zod (^3.24.2).
This new dependency is essential to support the configuration migration mechanism using Zod for schema validation. Please ensure that the introduced version provides all required features and is tested against expected configuration structures.
108-108: Introduced the packageManager field.
The addition of the"packageManager"field helps to maintain consistency in development environments by explicitly specifying the package manager and its version. Verify that this configuration aligns with the project's tooling expectations and update documentation if necessary.
| // migrations[n] should be a function that converts version n to version n+1 | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| const migrations: Record<number, (config: any) => any> = {}; |
There was a problem hiding this comment.
Since migrations is gonna hold all migration functions, and each migration function has a different signature (v1 => v2, v2 => v3, etc...), I'm not sure if it's possible to type this correctly :/
I tried experimenting with z.discriminatedUnion('version', [...]), or defining a shared interface interface VersionedConfig {version: number}, but I couldn't get anything to work. If you can think of a way to avoid any here please let me know 🙂
whatuserever
force-pushed
the
config_migrations
branch
from
486cda1 to
304e55e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
packages/main/src/backend/configManager/configMigration/versions/original.ts (3)
5-26: Standardize enum value casing for consistency.The
companyTypeSchemaenum values use inconsistent casing patterns. Consider standardizing to camelCase for all values.Apply this diff to standardize the casing:
export const companyTypeSchema = z.enum([ 'hapoalim', 'hapoalimBeOnline', 'beinleumi', - 'union', + 'unionBank', 'amex', 'isracard', 'visaCal', 'max', - 'leumiCard', + 'leumicard', - 'otsarHahayal', + 'otsarHaHayal', 'discount', 'mercantile', 'mizrahi', 'leumi', 'massad', 'yahav', - 'behatsdaa', + 'beHatsdaa', - 'beyahadBishvilha', + 'beYahadBishvilha', 'oneZero', 'pagi', ]);
75-83: Add documentation and defaults for configuration fields.The
scrapingSchemawould benefit from:
- Documentation about the unit of measurement for the
timeoutfield (seconds/milliseconds)- A default value for
maxConcurrencyto prevent potential performance issuesConsider adding defaults and refinements:
export const scrapingSchema = z.object({ numDaysBack: z.number(), showBrowser: z.boolean(), accountsToScrape: z.array(accountToScrapeConfigSchema), chromiumPath: z.string().optional(), - maxConcurrency: z.number().optional(), + maxConcurrency: z.number().optional().default(2), - timeout: z.number(), + timeout: z.number().describe('Timeout in milliseconds'), periodicScrapingIntervalHours: z.number().optional(), });
91-94: Add tests for the configuration type guard.The
isOriginalConfigtype guard is crucial for the migration mechanism. Please add unit tests to verify its behavior with valid and invalid configurations.Would you like me to help generate test cases for the type guard function? Here's an example of what the tests could look like:
describe('isOriginalConfig', () => { it('should return true for valid config', () => { const validConfig = { outputVendors: { json: { active: true, options: { filePath: '/tmp/output.json' } } }, scraping: { numDaysBack: 30, showBrowser: false, accountsToScrape: [], timeout: 30000 } }; expect(isOriginalConfig(validConfig)).toBe(true); }); it('should return false for invalid config', () => { const invalidConfig = { outputVendors: { invalid: { active: true } } }; expect(isOriginalConfig(invalidConfig)).toBe(false); }); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (10)
package.json(1 hunks)packages/main/src/backend/commonTypes.ts(1 hunks)packages/main/src/backend/configManager/configManager.ts(2 hunks)packages/main/src/backend/configManager/configMigration/configMigrator.ts(1 hunks)packages/main/src/backend/configManager/configMigration/versions/original.ts(1 hunks)packages/main/src/backend/configManager/configMigration/versions/v1.ts(1 hunks)packages/main/src/backend/configManager/defaultConfig.ts(1 hunks)packages/preload/src/commonTypes.ts(1 hunks)packages/renderer/src/store/Store.test.tsx(1 hunks)packages/renderer/src/types.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- packages/main/src/backend/commonTypes.ts
- packages/renderer/src/store/Store.test.tsx
- packages/main/src/backend/configManager/defaultConfig.ts
- packages/main/src/backend/configManager/configManager.ts
- packages/renderer/src/types.tsx
- package.json
- packages/main/src/backend/configManager/configMigration/versions/v1.ts
- packages/main/src/backend/configManager/configMigration/configMigrator.ts
- packages/preload/src/commonTypes.ts
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build (windows-latest)
- GitHub Check: build (macos-13)
🔇 Additional comments (1)
packages/main/src/backend/configManager/configMigration/versions/original.ts (1)
60-65: Well-structured vendor schema implementation!The
outputVendorsSchemaeffectively combines all vendor schemas using computed property names, making each vendor configuration optional. This provides good flexibility while maintaining type safety.
| export const googleSheetsConfigSchema = z.object({ | ||
| active: z.boolean(), | ||
| options: z.object({ | ||
| credentials: z.any(), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace any types with more specific types.
The use of any in credentials and loginFields bypasses TypeScript's type checking. Consider defining specific types or interfaces for these fields to maintain type safety.
For credentials, consider using the Google OAuth2 credentials type:
type GoogleCredentials = {
client_email: string;
private_key: string;
[key: string]: string;
};For loginFields, consider defining a union type of possible login field configurations:
type LoginField = {
type: 'text' | 'password';
name: string;
value: string;
};
type LoginFields = {
[key: string]: LoginField;
};Also applies to: 71-71
There was a problem hiding this comment.
credentials and loginFields are coming from 3rd parties (Google, israeli-bank-scrapers). I didn't want to create a schema for them as they're outside of our control.
There was a problem hiding this comment.
@whatuserever Understood, that's a valid approach for handling third-party data structures that are outside of our control. I'll retract my suggestion about replacing these specific any types.
✏️ Learnings added
Learnt from: whatuserever
PR: brafdlog/caspion#648
File: packages/main/src/backend/configManager/configMigration/versions/original.ts:31-31
Timestamp: 2025-02-16T12:10:12.122Z
Learning: In the Caspion project, `any` type is intentionally used for third-party data structures (like `credentials` from Google and `loginFields` from israeli-bank-scrapers) that are outside of our control to avoid maintenance overhead and potential breakage when third-party structures change.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| return { | ||
| ...v1Config, | ||
| version: 1, | ||
| }; |
There was a problem hiding this comment.
Exactly, this is the migration:
const migrated = {
...originalConfig,
version: 1
}
You can do it without all the previous zod preparations.
There was a problem hiding this comment.
The preperation is needed to create a snapshot of the config interface that is decoupled from the Config type used in the "live" code. Otherwise, how can you write a function that accepts version n and returns version n+1, while the live code is at the latest version m?
Maybe looking at what I did for v2 will help clarify that? You can check it out here (I didn't update #647 yet as I'm waiting for this PR to get approved).
We can do this preperation by copying the types instead of using Zod, but we still need it. And in my opinion Zod is a better fit for this task as it lets you safely parse the input file, allowing you to verify the version before applying any migrations.
#647 makes a breaking change to the config. To avoid breaking installations with the old config, @baruchiro suggested implementing a migration mechanism.
This PR implements it using Zod:
nand return a config of versionn+1versionkey to theConfigtype.TODO:
Summary by CodeRabbit