Implement Zod for Robust Configuration Validation #77
Conversation
WalkthroughThe changes involve updates to the project's configuration management and error handling by integrating the Zod library for schema validation. The Changes
Possibly related PRs
Suggested labels
Recent review detailsConfiguration used: CodeRabbit UI Files ignored due to path filters (1)
Files selected for processing (23)
Files not reviewed due to no reviewable changes (3)
Files skipped from review as they are similar to previous changes (20)
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 (
|
yamadashy
changed the title
yamadashy
force-pushed
the
feature/more-message
branch
from
4c85187 to
6546967
Compare
yamadashy
changed the title
yamadashy
force-pushed
the
feature/more-message
branch
from
6546967 to
aec2be6
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Outside diff range comments (3)
src/core/file/fileSearcher.ts (3)
Line range hint
36-39: Include original error message when throwing a new errorIn the
catchblock, the code logs the original error but throws a new generic error message. Including the original error message in the thrown error can provide more context and aid in debugging.Apply this diff to include the original error message:
logger.error('Error filtering files:', error); - throw new Error('Failed to filter files'); + throw new Error(`Failed to filter files: ${error.message}`);
Line range hint
48-58: Remove unnecessary 'async' keyword from 'getIgnoreFilePatterns'The function
getIgnoreFilePatternsis declared asasyncbut does not contain anyawaitexpressions or perform asynchronous operations. This is unnecessary and can be misleading.Apply this diff to remove the
asynckeyword:-export const getIgnoreFilePatterns = async (config: RepopackConfigMerged): Promise<string[]> => { +export const getIgnoreFilePatterns = (config: RepopackConfigMerged): Promise<string[]> => {
Line range hint
60-82: Remove unnecessary 'async' keyword from 'getIgnorePatterns'Similarly, the function
getIgnorePatternsdoes not perform any asynchronous operations and doesn't require theasynckeyword.Apply this diff to remove the
asynckeyword:-export const getIgnorePatterns = async (rootDir: string, config: RepopackConfigMerged): Promise<string[]> => { +export const getIgnorePatterns = (rootDir: string, config: RepopackConfigMerged): Promise<string[]> => {
Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
Files selected for processing (23)
- package.json (2 hunks)
- src/cli/actions/defaultActionRunner.ts (3 hunks)
- src/cli/actions/initActionRunner.ts (1 hunks)
- src/cli/cliPrinter.ts (1 hunks)
- src/cli/cliRunner.ts (1 hunks)
- src/config/configLoader.ts (3 hunks)
- src/config/configSchema.ts (1 hunks)
- src/config/configTypes.ts (0 hunks)
- src/config/configValidator.ts (0 hunks)
- src/config/defaultConfig.ts (2 hunks)
- src/core/file/fileProcessor.ts (1 hunks)
- src/core/file/fileSearcher.ts (1 hunks)
- src/core/output/outputGenerator.ts (1 hunks)
- src/core/output/outputGeneratorTypes.ts (1 hunks)
- src/core/output/outputStyleDecorator.ts (1 hunks)
- src/core/packager.ts (2 hunks)
- src/core/security/securityCheckRunner.ts (2 hunks)
- src/index.ts (1 hunks)
- src/shared/errorHandler.ts (3 hunks)
- tests/config/configLoader.test.ts (4 hunks)
- tests/config/configValidator.test.ts (0 hunks)
- tests/integration-tests/packager.test.ts (1 hunks)
- tests/testing/testUtils.ts (1 hunks)
Files not reviewed due to no reviewable changes (3)
- src/config/configTypes.ts
- src/config/configValidator.ts
- tests/config/configValidator.test.ts
Additional comments not posted (27)
src/index.ts (1)
2-2: Verify the export ofRepopackConfigFilefromconfigSchema.js.The export statement now imports
RepopackConfigFilefrom./config/configSchema.jsinstead of./config/configTypes.jsand re-exports it asRepopackConfig. Ensure thatRepopackConfigFileis correctly exported fromconfigSchema.jsand that all references toRepopackConfigremain valid throughout the codebase.Run the following script to confirm that
RepopackConfigFileis exported fromconfigSchema.js:Additionally, verify that no imports are still referencing
./config/configTypes.js:Verification successful
Verification successful: Export of
RepopackConfigFileis correctThe verification process has confirmed that the changes mentioned in the review comment are correct and properly implemented:
RepopackConfigFileis correctly exported fromsrc/config/configSchema.ts.- There are no remaining references to
configTypes.jsin the codebase.- The export statement in
src/index.tscorrectly re-exportsRepopackConfigFileasRepopackConfigfrom./config/configSchema.js.These changes ensure that all references to
RepopackConfigremain valid throughout the codebase.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that RepopackConfigFile is exported from configSchema.js # Test: Search for the export of RepopackConfigFile. Expect: Export statement is found. rg --type=typescript 'export\s+(interface|type)\s+RepopackConfigFile' ./src/config/configSchema.tsLength of output: 135
Script:
#!/bin/bash # Description: Check for lingering imports from configTypes.js # Test: Search for imports from configTypes.js. Expect: No matches found. rg --type=typescript 'from\s+["'"]\.\/config\/configTypes\.js["'"]'Length of output: 184
Script:
#!/bin/bash # List all .ts and .js files in the src/config directory echo "Listing .ts and .js files in src/config:" fd -e ts -e js . src/config # Check for export of RepopackConfigFile in configSchema.js echo "Checking for export of RepopackConfigFile:" ast-grep --lang typescript --pattern 'export type RepopackConfigFile = $_' src/config/configSchema.js # Check for any remaining imports from configTypes.js echo "Checking for imports from configTypes.js:" ast-grep --lang typescript --pattern 'import $_ from "./config/configTypes.js"' # Check for any usage of RepopackConfig echo "Checking for usage of RepopackConfig:" ast-grep --lang typescript --pattern 'RepopackConfig'Length of output: 948
Script:
#!/bin/bash # Check for export of RepopackConfigFile in configSchema.ts echo "Checking for export of RepopackConfigFile in configSchema.ts:" ast-grep --lang typescript --pattern 'export type RepopackConfigFile = $_' src/config/configSchema.ts # Double-check for any remaining references to configTypes.js echo "Checking for any references to configTypes.js:" rg "configTypes\.js" # Verify the content of src/index.ts echo "Content of src/index.ts:" cat src/index.tsLength of output: 723
src/core/output/outputGeneratorTypes.ts (1)
1-1: Import path correctly updated to reflect new configuration schemaThe import statement for
RepopackConfigMergedhas been appropriately updated to'../../config/configSchema.js', consistent with the integration of Zod for configuration validation. This ensures that theOutputGeneratorContextinterface references the correct type definitions from the updated schema.src/config/defaultConfig.ts (2)
1-1: Import statement correctly includes schema for validationThe addition of
repopackConfigDefaultSchemain the import statement ensures that the schema is available for validating the default configuration.
Line range hint
3-21: Robust default configuration validation implementedUsing
repopackConfigDefaultSchema.parse({...})to parse and validate thedefaultConfigenhances the robustness of your configuration handling. This ensures that any changes to the default configuration are immediately checked against the schema, reducing the risk of runtime errors due to misconfigurations.tests/testing/testUtils.ts (1)
3-3: Verify that all imports ofRepopackConfigMergedare updated toconfigSchema.jsThe type
RepopackConfigMergedis now imported fromconfigSchema.jsinstead ofconfigTypes.js. Please ensure thatRepopackConfigMergedis correctly exported fromconfigSchema.jsand that all other files have updated their import statements accordingly to prevent any type resolution issues.Run the following script to check for any remaining imports from
configTypes.js:Verification successful
Verification successful: All imports of
RepopackConfigMergedare correctly updatedThe changes to import
RepopackConfigMergedfromconfigSchema.jshave been successfully implemented across the codebase. Here's a summary of the findings:
- No remaining imports of
RepopackConfigMergedfromconfigTypes.jswere found.- The type
RepopackConfigMergedis correctly exported fromsrc/config/configSchema.ts.- Multiple files now import
RepopackConfigMergedfromconfigSchema.js, indicating consistent updates throughout the project.The discrepancy between the
.tsfile extension of the actual file and the.jsextension in import statements is normal in TypeScript projects and doesn't cause any issues.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for any import statements of 'RepopackConfigMerged' from 'configTypes.js' # Expected result: No matches should be found, indicating all imports have been updated. rg --type typescript "import.*\{.*RepopackConfigMerged.*\}.*from.*configTypes\.js"Length of output: 119
Script:
#!/bin/bash # Search for any remaining imports of 'RepopackConfigMerged' from 'configTypes.js' echo "Searching for imports from configTypes.js:" rg "import.*\{.*RepopackConfigMerged.*\}.*from.*['\"].*configTypes\.js['\"]" --type-add 'ts:*.{ts,js}' # Check if 'RepopackConfigMerged' is exported from 'configSchema.js' echo -e "\nChecking exports in configSchema.js:" rg "export.*type.*RepopackConfigMerged" src/config/configSchema.js # Search for updated imports of 'RepopackConfigMerged' from 'configSchema.js' echo -e "\nSearching for imports from configSchema.js:" rg "import.*\{.*RepopackConfigMerged.*\}.*from.*['\"].*configSchema\.js['\"]" --type-add 'ts:*.{ts,js}'Length of output: 1637
Script:
#!/bin/bash # Check if 'src/config/configSchema.ts' exists echo "Checking for src/config/configSchema.ts:" if [ -f src/config/configSchema.ts ]; then echo "File exists" # Check if 'RepopackConfigMerged' is exported from 'configSchema.ts' echo -e "\nChecking exports in configSchema.ts:" rg "export.*type.*RepopackConfigMerged" src/config/configSchema.ts else echo "File does not exist" fiLength of output: 400
src/shared/errorHandler.ts (2)
1-1: Importing Zod library for schema validationThe import statement correctly brings in the Zod library, enabling robust configuration validation.
11-16: Addition of custom error class for configuration validationThe
RepopackConfigValidationErrorclass appropriately extendsRepopackErrorand sets the error name for precise error identification.src/core/file/fileProcessor.ts (1)
2-2: Ensure all references toconfigTypes.jsare updatedTo maintain consistency and prevent potential type mismatches, please verify that all imports from
'../../config/configTypes.js'have been updated to'../../config/configSchema.js'throughout the codebase.Run the following script to identify any remaining references to
configTypes.js:Verification successful
To ensure a thorough verification, I'll generate additional shell scripts to search for related terms. This will help us confirm whether all references have been updated correctly.
These scripts will help us get a clearer picture of the current state of the codebase regarding the use of 'configTypes' and 'configSchema'.
All references to
configTypes.jshave been successfully updatedThe verification process has confirmed that all references to 'configTypes.js' have been correctly updated to 'configSchema.js' throughout the codebase. No remaining instances of 'configTypes.js' were found, and 'configSchema.js' is being consistently used across various files.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all files importing from 'configTypes.js' # Test: Search for imports from 'configTypes.js'. Expectation: No matches found. rg --type js --type ts --no-heading 'from\s+.*configTypes\.js'Length of output: 64
Script:
#!/bin/bash # Search for any remaining references to 'configTypes' echo "Searching for 'configTypes':" rg --type js --type ts --no-heading 'configTypes' # Search for usage of 'configSchema' echo "Searching for 'configSchema':" rg --type js --type ts --no-heading 'configSchema' # Broader search for 'config' and 'Types' to catch potential partial matches echo "Searching for 'config' and 'Types':" rg --type js --type ts --no-heading 'config.*Types'Length of output: 2093
src/core/output/outputGenerator.ts (1)
3-3: Import update aligns with Zod integration for configuration validationThe import statement now correctly imports
RepopackConfigMergedfromconfigSchema.js, reflecting the shift to using Zod schemas for robust configuration validation. This change ensures that the type definitions are consistent with the new schema-based validation approach.src/core/output/outputStyleDecorator.ts (1)
1-1: Verify that 'RepopackConfigMerged' is correctly exported from 'configSchema.js'The import statement has been updated to import
RepopackConfigMergedfrom'../../config/configSchema.js'instead of'../../config/configTypes.js'. Please ensure thatRepopackConfigMergedis indeed exported fromconfigSchema.jsand that this change is consistent throughout the codebase.Run the following script to confirm that
RepopackConfigMergedis exported fromconfigSchema.js:Verification successful
'RepopackConfigMerged' is correctly exported from the config schema file
The verification confirms that 'RepopackConfigMerged' is indeed exported from 'src/config/configSchema.ts'. The import statement in 'outputStyleDecorator.ts' correctly references this export, although there's a minor difference in file extension ('.js' vs '.ts') which is typical in TypeScript projects.
- The export is found in 'src/config/configSchema.ts', line 79:
export type RepopackConfigMerged = z.infer<typeof repopackConfigMergedSchema>;Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that 'RepopackConfigMerged' is exported from 'configSchema.js' # Test: Search for the export of 'RepopackConfigMerged' in the codebase. # Expectation: The export should be located in 'configSchema.js'. ast-grep --lang typescript --pattern 'export type RepopackConfigMerged = $_' # Note: This will list all files where 'RepopackConfigMerged' is exported. Ensure it points to 'configSchema.js'.Length of output: 187
package.json (1)
66-67: Addition ofzoddependency is appropriateIncluding
zodin your dependencies supports your goal of enhancing configuration validation.src/core/security/securityCheckRunner.ts (1)
5-5: Verify thatpicocolorsis included in the project dependenciesPlease ensure that
picocolorsis added to your project's dependencies to prevent runtime errors.Run the following script to check if
picocolorsis listed inpackage.json:Verification successful
'picocolors' is correctly included in the project dependencies
The verification process confirms that 'picocolors' is properly listed in the project's dependencies in package.json with version "^1.1.0". This ensures that the package will be installed when setting up the project, preventing potential runtime errors related to missing dependencies.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that 'picocolors' is listed in package.json dependencies or devDependencies. # Test: Check for 'picocolors' in 'package.json'. # Expect: 'picocolors' should be listed under dependencies or devDependencies. jq '.dependencies.picocolors, .devDependencies.picocolors' package.jsonLength of output: 87
src/core/file/fileSearcher.ts (1)
2-2: Verify the export of 'RepopackConfigMerged' from 'configSchema.js'The import path for
RepopackConfigMergedhas been updated to'../../config/configSchema.js'. Ensure thatRepopackConfigMergedis correctly exported fromconfigSchema.jsand that it matches the expected type definitions.Run the following script to verify the export:
Verification successful
To gather more information and address the remaining concerns, I'll run additional scripts to inspect the import statement and the schema definition.
RepopackConfigMerged export verified successfully
The export of 'RepopackConfigMerged' from 'configSchema.js' has been verified. The type is correctly defined and exported, and its usage is consistent with the project's conventions. The import statement in 'fileSearcher.ts' is also correct.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that 'RepopackConfigMerged' is exported from 'configSchema.js' # Test: Search for the export statement of 'RepopackConfigMerged' in 'configSchema.js' # Expect: An export of 'RepopackConfigMerged' type definition. ast-grep --lang typescript --pattern $'export type RepopackConfigMerged = $_' ./src/config/configSchema.tsLength of output: 216
Script:
#!/bin/bash # Description: Verify the import statement and schema definition # Test 1: Check the import statement in fileSearcher.ts echo "Checking import statement:" rg "import.*RepopackConfigMerged" src/core/file/fileSearcher.ts # Test 2: Inspect the definition of repopackConfigMergedSchema echo -e "\nChecking repopackConfigMergedSchema definition:" ast-grep --lang typescript --pattern 'const repopackConfigMergedSchema = $_' src/config/configSchema.ts # Test 3: Check for other usages of z.infer in the project echo -e "\nChecking other usages of z.infer:" rg "z\.infer" --type tsLength of output: 1482
tests/integration-tests/packager.test.ts (1)
7-7: Updated import statement to reflect reorganizationThe import statement now references types from
configSchema.jsinstead ofconfigTypes.js, aligning with the integration of Zod for configuration validation. Ensure thatconfigSchema.jsexportsRepopackConfigFile,RepopackConfigMerged, andRepopackOutputStyleas expected.Run the following script to confirm that the required types are exported:
Verification successful
Import statement is correct and types are properly exported
The import statement in
tests/integration-tests/packager.test.tscorrectly references the required types from../../src/config/configSchema.js. Although the actual definitions are in TypeScript files, the.jsextension in the import is correct due to TypeScript's module resolution:
RepopackConfigFile,RepopackConfigMerged, andRepopackOutputStyleare exported insrc/config/configSchema.tsRepopackConfigFileis also re-exported asRepopackConfiginsrc/index.tsThe review comment's suggestion to update the import statement was accurate, aligning with the integration of Zod for configuration validation.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that 'configSchema.js' exports the required types. # Expected: The types 'RepopackConfigFile', 'RepopackConfigMerged', and 'RepopackOutputStyle' are exported. # Command: rg --type js 'export type.*(RepopackConfigFile|RepopackConfigMerged|RepopackOutputStyle)' -g 'configSchema.js'Length of output: 259
Script:
#!/bin/bash # Search for configSchema.js file echo "Searching for configSchema.js:" fd configSchema.js # If found, display its contents if [ $? -eq 0 ]; then echo -e "\nContents of configSchema.js:" cat $(fd configSchema.js) else echo -e "\nconfigSchema.js not found. Searching for files with the required type definitions:" rg --type ts 'export type.*(RepopackConfigFile|RepopackConfigMerged|RepopackOutputStyle)' fiLength of output: 210
Script:
#!/bin/bash # Display contents of configSchema.js echo "Contents of configSchema.js:" cat $(fd configSchema.js) # If file is empty or doesn't contain expected exports, search for TypeScript files if [ ! -s "$(fd configSchema.js)" ] || ! grep -q "export type" "$(fd configSchema.js)"; then echo -e "\nSearching for TypeScript files with the required type definitions:" rg --type ts 'export type.*(RepopackConfigFile|RepopackConfigMerged|RepopackOutputStyle)' fi # Check the import statement in the original file echo -e "\nChecking import statement in tests/integration-tests/packager.test.ts:" rg "import type.*from '../../src/config/configSchema" tests/integration-tests/packager.test.tsLength of output: 1164
src/cli/cliRunner.ts (1)
4-4: LGTM!The import statement for
RepopackOutputStylehas been correctly updated to reflect the new configuration schema.src/cli/cliPrinter.ts (1)
3-3: Updated import to reflect configuration schema changesThe import statement has been updated to import
RepopackConfigMergedfrom../config/configSchema.jsinstead of../config/configTypes.js. This change aligns with the integration of Zod for configuration validation and suggests that types are now defined inconfigSchema.js.Run the following script to ensure there are no remaining imports from
configTypes.jsin the codebase:Verification successful
To ensure we have accurate information about the remaining imports, I'll generate new shell scripts to verify the ripgrep functionality and use an alternative search method.
Import change successfully implemented
The verification process confirms that there are no remaining imports from '../config/configTypes.js' in the codebase. The change to import
RepopackConfigMergedfrom '../config/configSchema.js' has been consistently applied, aligning with the integration of Zod for configuration validation.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for any remaining imports from '../config/configTypes.js' # Expected Result: No occurrences of imports from '../config/configTypes.js' rg "from ['\"]\.\./config/configTypes\.js['\"]"Length of output: 53
Script:
#!/bin/bash # Test ripgrep functionality with a simple pattern echo "Testing ripgrep functionality:" rg "import" # Use fd to find all TypeScript and JavaScript files, then use grep to search for the import echo "Searching for '../config/configTypes.js' imports:" fd -e ts -e js -x grep -n "from ['\"]../config/configTypes.js['\"]" {}Length of output: 23042
src/cli/actions/defaultActionRunner.ts (2)
3-11: Imports Updated for Configuration ValidationThe added imports for
RepopackConfigtypes,repopackConfigCliSchema, andrethrowValidationErrorIfZodErrorcorrectly support the new configuration validation using Zod. This enhances type safety and validation throughout the module.
34-34: Extraction of CLI Config Parsing intobuildCliConfigFunctionRefactoring the CLI options parsing into the
buildCliConfigfunction improves code readability and maintains separation of concerns. This makes therunDefaultActionfunction cleaner and easier to maintain.src/core/packager.ts (3)
5-5: Correct import of 'RepopackConfigMerged' typeThe import of the type
RepopackConfigMergedfrom'../config/configSchema.js'is appropriate and follows TypeScript best practices.
97-97: Enhanced progress logging with file path formattingIncluding the file path in the progress callback enhances logging clarity, allowing users to see which file is being processed. Using
pc.dim(file.path)to dim the file path improves readability in the console output.
4-4: Ensure 'picocolors' is added to project dependenciesThe module
'picocolors'is being imported, but there's no indication that it has been added to the project's dependencies. Please ensure that'picocolors'is listed in yourpackage.jsonfile underdependenciesto prevent import errors.Run the following script to verify if
'picocolors'is included inpackage.jsondependencies:Verification successful
'picocolors' is correctly listed in project dependencies
The verification process confirms that 'picocolors' is properly included in the project's dependencies. The package.json file lists 'picocolors' with the version "^1.1.0", which allows for compatible updates within the 1.x.x range. No further action is required regarding this dependency.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that 'picocolors' is listed in the project's package.json dependencies. # Test: Check if 'picocolors' is in the dependencies. Expect: 'picocolors' should be listed under 'dependencies' or 'devDependencies'. jq '.dependencies.picocolors // .devDependencies.picocolors' package.jsonLength of output: 84
tests/config/configLoader.test.ts (5)
7-7: Import statements for new types and error classes are correctly addedThe imports for
RepopackConfigCli,RepopackConfigFile, andRepopackConfigValidationErrorare appropriate and necessary for the updated test cases.Also applies to: 9-9
32-34: Mock configuration in test is properly structuredThe mock configuration for the test
'should load and parse a valid local config file'is accurately defined with valid types forfilePath,style, anduseDefaultPatterns.
42-51: Test correctly validates handling of invalid configurationThe test
'should throw RepopackConfigValidationError for invalid config'effectively checks that theloadFileConfigfunction throws an error when provided with an invalid configuration. This ensures robust validation and error handling.
55-57: Global configuration mock includes additional 'style' propertyIncluding the
styleproperty in the global configuration enhances test coverage by verifying that global configurations are loaded with all relevant properties.
91-102: Test verifies correct merging of file and CLI configurationsThe test
'should correctly merge configs'appropriately checks that CLI configurations override file configurations and that properties from both sources are merged as expected.src/cli/actions/initActionRunner.ts (1)
5-5: Import update reflects integration of Zod for configuration validationThe import statement now references
configSchema.jsinstead ofconfigTypes.js, aligning with the introduction of Zod for robust configuration validation as stated in the PR objectives. This change ensures that the configuration types are correctly imported from the updated schema, enhancing type safety and validation throughout the application.
| export const rethrowValidationErrorIfZodError = (error: unknown, message: string): void => { | ||
| if (error instanceof z.ZodError) { | ||
| const zodErrorText = error.errors.map((err) => `${err.path.join('.')}: ${err.message}`).join(', '); | ||
| throw new RepopackConfigValidationError( | ||
| `${message}\n ${zodErrorText}\n Please check the config file and try again.`, | ||
| ); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Consider adding unit tests for the new validation error handling
The rethrowValidationErrorIfZodError function enhances error handling by wrapping Zod validation errors into a custom error class. To ensure its reliability and to catch potential edge cases, consider adding unit tests that cover various validation failure scenarios.
Would you like assistance in creating unit tests for this function, or should I open a GitHub issue to track this task?
| @@ -31,7 +32,7 @@ export const runSecurityCheck = async ( | |||
| }; | |||
| } | |||
|
|
|||
| progressCallback(`Running security check... (${index + 1}/${rawFiles.length})`); | |||
| progressCallback(`Running security check... (${index + 1}/${rawFiles.length}) ${pc.dim(rawFile.path)}`); | |||
There was a problem hiding this comment.
Ensure progressCallback is called regardless of security issues
Currently, the progressCallback is only invoked when no security issues are found in the file. This may lead to inaccurate progress reporting. Consider moving the progressCallback call before the condition to ensure it runs for every file processed.
Apply this diff to move the progressCallback call:
const secretLintResult = await runSecretLint(rawFile.path, rawFile.content, secretLintConfig);
+ progressCallback(`Running security check... (${index + 1}/${rawFiles.length}) ${pc.dim(rawFile.path)}`);
if (secretLintResult.messages.length > 0) {
return {
filePath: rawFile.path,
messages: secretLintResult.messages.map((message) => message.message),
};
}
- progressCallback(`Running security check... (${index + 1}/${rawFiles.length}) ${pc.dim(rawFile.path)}`);
// Sleep for a short time to prevent blocking the event loop
await sleep(1);Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| progressCallback(`Running security check... (${index + 1}/${rawFiles.length}) ${pc.dim(rawFile.path)}`); | |
| const secretLintResult = await runSecretLint(rawFile.path, rawFile.content, secretLintConfig); | |
| progressCallback(`Running security check... (${index + 1}/${rawFiles.length}) ${pc.dim(rawFile.path)}`); | |
| if (secretLintResult.messages.length > 0) { | |
| return { | |
| filePath: rawFile.path, | |
| messages: secretLintResult.messages.map((message) => message.message), | |
| }; | |
| } | |
| // Sleep for a short time to prevent blocking the event loop | |
| await sleep(1); |
|
|
||
| export type RepopackOutputStyle = z.infer<typeof repopackOutputStyleSchema>; | ||
|
|
||
| const repopackConfigBaseSchema = z.object({ |
There was a problem hiding this comment.
Export repopackConfigBaseSchema for Consistency and Reusability
Since repopackConfigBaseSchema serves as the foundation for other schemas and might be useful in other modules or for further extensions, consider exporting it to promote reusability and maintain consistency.
Apply this diff to export the base schema:
-const repopackConfigBaseSchema = z.object({
+export const repopackConfigBaseSchema = z.object({Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const repopackConfigBaseSchema = z.object({ | |
| export const repopackConfigBaseSchema = z.object({ |
| export const repopackConfigDefaultSchema = repopackConfigBaseSchema.and( | ||
| z.object({ | ||
| output: z.object({ |
There was a problem hiding this comment.
Use .extend() Instead of .and() for Object Schema Extension
When extending an object schema, it's more appropriate to use .extend() rather than .and(). The .extend() method is specifically designed for adding new fields to an existing schema and ensures better handling of overlapping keys.
Apply this diff to replace .and() with .extend():
-export const repopackConfigDefaultSchema = repopackConfigBaseSchema.and(
+export const repopackConfigDefaultSchema = repopackConfigBaseSchema.extend(
z.object({
output: z.object({
filePath: z.string(),
style: repopackOutputStyleSchema,
headerText: z.string().optional(),
instructionFilePath: z.string().optional(),
removeComments: z.boolean(),
removeEmptyLines: z.boolean(),
topFilesLength: z.number(),
showLineNumbers: z.boolean(),
}),
include: z.array(z.string()),
ignore: z.object({
useGitignore: z.boolean(),
useDefaultPatterns: z.boolean(),
customPatterns: z.array(z.string()).optional(),
}),
security: z.object({
enableSecurityCheck: z.boolean(),
}),
}),
);Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const repopackConfigDefaultSchema = repopackConfigBaseSchema.and( | |
| z.object({ | |
| output: z.object({ | |
| export const repopackConfigDefaultSchema = repopackConfigBaseSchema.extend( | |
| z.object({ | |
| output: z.object({ |
| import { z } from 'zod'; | ||
| import { RepopackError, rethrowValidationErrorIfZodError } from '../shared/errorHandler.js'; |
There was a problem hiding this comment.
Unused import: 'z' from 'zod'
The imported z from 'zod' is not directly used in this file. Since the schema parsing is handled through imported schemas (repopackConfigFileSchema and repopackConfigMergedSchema), the direct import of z may not be necessary.
Consider removing the unused import to keep the code clean:
-import { z } from 'zod';Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import { z } from 'zod'; | |
| import { RepopackError, rethrowValidationErrorIfZodError } from '../shared/errorHandler.js'; | |
| import { RepopackError, rethrowValidationErrorIfZodError } from '../shared/errorHandler.js'; |
| return repopackConfigMergedSchema.parse(mergedConfig); | ||
| } catch (error) { | ||
| rethrowValidationErrorIfZodError(error, 'Invalid merged config'); | ||
| throw error; |
There was a problem hiding this comment.
Remove redundant error rethrow in merged configuration validation
After calling rethrowValidationErrorIfZodError(error, 'Invalid merged config');, the subsequent throw error; may be unnecessary if the function already throws the error. This redundancy could lead to confusion about which error is ultimately thrown.
Apply this diff to eliminate the redundant throw statement:
} catch (error) {
rethrowValidationErrorIfZodError(error, 'Invalid merged config');
- throw error;
}This change ensures that the error handling is clear and that only the intended error is propagated.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return repopackConfigMergedSchema.parse(mergedConfig); | |
| } catch (error) { | |
| rethrowValidationErrorIfZodError(error, 'Invalid merged config'); | |
| throw error; | |
| return repopackConfigMergedSchema.parse(mergedConfig); | |
| } catch (error) { | |
| rethrowValidationErrorIfZodError(error, 'Invalid merged config'); | |
| } |
| return repopackConfigFileSchema.parse(config); | ||
| } catch (error) { | ||
| if (error instanceof RepopackConfigValidationError) { | ||
| throw new RepopackError(`Invalid configuration in ${filePath}: ${error.message}`); | ||
| } | ||
| rethrowValidationErrorIfZodError(error, 'Invalid config schema'); |
There was a problem hiding this comment.
Ensure consistent error handling in configuration parsing
When parsing the configuration file with repopackConfigFileSchema.parse(config), any validation errors thrown by Zod are handled by rethrowValidationErrorIfZodError(error, 'Invalid config schema');. If this function rethrows the error, the subsequent error handling code might be unreachable. To avoid redundancy and potential confusion, consider simplifying the error handling to ensure that errors are handled appropriately without overlapping.
Apply this diff to streamline the error handling:
} catch (error) {
rethrowValidationErrorIfZodError(error, 'Invalid config schema');
- if (error instanceof SyntaxError) {
- throw new RepopackError(`Invalid JSON in config file ${filePath}: ${error.message}`);
- }
- if (error instanceof Error) {
- throw new RepopackError(`Error loading config from ${filePath}: ${error.message}`);
- }
- throw new RepopackError(`Error loading config from ${filePath}`);
}Ensure that rethrowValidationErrorIfZodError handles all necessary error cases, including JSON parsing errors.
Committable suggestion was skipped due to low confidence.
| ): RepopackConfigMerged => { | ||
| const mergedConfig = { | ||
| cwd, | ||
| output: { | ||
| ...defaultConfig.output, | ||
| ...fileConfig.output, | ||
| ...cliConfig.output, | ||
| }, | ||
| ignore: { | ||
| ...defaultConfig.ignore, | ||
| ...fileConfig.ignore, | ||
| ...cliConfig.ignore, | ||
| customPatterns: [ | ||
| ...(defaultConfig.ignore.customPatterns || []), | ||
| ...(fileConfig.ignore?.customPatterns || []), | ||
| ...(cliConfig.ignore?.customPatterns || []), | ||
| ], | ||
| }, | ||
| include: [...(defaultConfig.include || []), ...(fileConfig.include || []), ...(cliConfig.include || [])], | ||
| security: { | ||
| ...defaultConfig.security, | ||
| ...fileConfig.security, | ||
| ...cliConfig.security, | ||
| }, | ||
| }; | ||
|
|
There was a problem hiding this comment.
Consider deep merging for nested configuration objects
When merging configurations, properties like output, ignore, and security are being shallowly merged. If these properties contain nested objects or arrays, a shallow merge might not combine them as expected, potentially overwriting entire sections of the configuration.
To ensure all nested properties are correctly merged, consider using a deep merge utility like lodash.merge. Here's how you might implement it:
+ import merge from 'lodash.merge';
...
export const mergeConfigs = (
cwd: string,
fileConfig: RepopackConfigFile,
cliConfig: RepopackConfigCli,
): RepopackConfigMerged => {
- const mergedConfig = {
- cwd,
- output: {
- ...defaultConfig.output,
- ...fileConfig.output,
- ...cliConfig.output,
- },
- ignore: {
- ...defaultConfig.ignore,
- ...fileConfig.ignore,
- ...cliConfig.ignore,
- customPatterns: [
- ...(defaultConfig.ignore.customPatterns || []),
- ...(fileConfig.ignore?.customPatterns || []),
- ...(cliConfig.ignore?.customPatterns || []),
- ],
- },
- include: [...(defaultConfig.include || []), ...(fileConfig.include || []), ...(cliConfig.include || [])],
- security: {
- ...defaultConfig.security,
- ...fileConfig.security,
- ...cliConfig.security,
- },
- };
+ const mergedConfig = merge({}, defaultConfig, fileConfig, cliConfig, { cwd });
try {
return repopackConfigMergedSchema.parse(mergedConfig);
} catch (error) {
rethrowValidationErrorIfZodError(error, 'Invalid merged config');
throw error;
}
};This approach provides a more robust merging strategy, ensuring that all configuration values are appropriately combined without unintended overwrites.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ): RepopackConfigMerged => { | |
| const mergedConfig = { | |
| cwd, | |
| output: { | |
| ...defaultConfig.output, | |
| ...fileConfig.output, | |
| ...cliConfig.output, | |
| }, | |
| ignore: { | |
| ...defaultConfig.ignore, | |
| ...fileConfig.ignore, | |
| ...cliConfig.ignore, | |
| customPatterns: [ | |
| ...(defaultConfig.ignore.customPatterns || []), | |
| ...(fileConfig.ignore?.customPatterns || []), | |
| ...(cliConfig.ignore?.customPatterns || []), | |
| ], | |
| }, | |
| include: [...(defaultConfig.include || []), ...(fileConfig.include || []), ...(cliConfig.include || [])], | |
| security: { | |
| ...defaultConfig.security, | |
| ...fileConfig.security, | |
| ...cliConfig.security, | |
| }, | |
| }; | |
| ): RepopackConfigMerged => { | |
| const mergedConfig = merge({}, defaultConfig, fileConfig, cliConfig, { cwd }); | |
| try { | |
| return repopackConfigMergedSchema.parse(mergedConfig); | |
| } catch (error) { | |
| rethrowValidationErrorIfZodError(error, 'Invalid merged config'); | |
| throw error; | |
| } | |
| }; |
| test('should throw RepopackConfigValidationError for invalid merged config', () => { | ||
| const fileConfig: RepopackConfigFile = { | ||
| output: { filePath: 'file-output.txt', style: 'plain' }, | ||
| }; | ||
| const cliConfig: RepopackConfigCli = { | ||
| // @ts-ignore | ||
| output: { style: 'invalid' }, // Invalid style | ||
| }; | ||
|
|
||
| expect(() => mergeConfigs(process.cwd(), fileConfig, cliConfig)).toThrow(RepopackConfigValidationError); | ||
| }); |
There was a problem hiding this comment.
Avoid using '@ts-ignore'; use explicit type casting instead
Using @ts-ignore suppresses TypeScript's type checking and may mask other issues. For better type safety, consider casting the invalid configuration to unknown and then to RepopackConfigCli.
Apply this change to improve type safety:
- const cliConfig: RepopackConfigCli = {
- // @ts-ignore
output: { style: 'invalid' }, // Invalid style
- };
+ const cliConfig = {
+ output: { style: 'invalid' }, // Invalid style
+ } as unknown as RepopackConfigCli;Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| test('should throw RepopackConfigValidationError for invalid merged config', () => { | |
| const fileConfig: RepopackConfigFile = { | |
| output: { filePath: 'file-output.txt', style: 'plain' }, | |
| }; | |
| const cliConfig: RepopackConfigCli = { | |
| // @ts-ignore | |
| output: { style: 'invalid' }, // Invalid style | |
| }; | |
| expect(() => mergeConfigs(process.cwd(), fileConfig, cliConfig)).toThrow(RepopackConfigValidationError); | |
| }); | |
| test('should throw RepopackConfigValidationError for invalid merged config', () => { | |
| const fileConfig: RepopackConfigFile = { | |
| output: { filePath: 'file-output.txt', style: 'plain' }, | |
| }; | |
| const cliConfig = { | |
| output: { style: 'invalid' }, // Invalid style | |
| } as unknown as RepopackConfigCli; | |
| expect(() => mergeConfigs(process.cwd(), fileConfig, cliConfig)).toThrow(RepopackConfigValidationError); | |
| }); |
yamadashy
force-pushed
the
feature/more-message
branch
from
aec2be6 to
b9e39b1
Compare
Summary by CodeRabbit
New Features
Refactor