refactor: Refactored sync package to be better organized #418
Conversation
ENG-542 Refactor sync engine
We have historical code organization that is hard to follow so we should refactor the sync engine to better reflect better code organization. |
🦋 Changeset detectedLatest commit: b3539b7 The changes in this PR will be included in the next version bump. This PR includes changesets to release 12 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 |
|
Note Reviews pausedUse the following commands to manage reviews:
WalkthroughThis pull request introduces a comprehensive refactoring of the sync package, focusing on improving code organization and modularity. The changes involve restructuring the project's directory structure, updating import paths, and consolidating type definitions across multiple modules. Key modifications include moving core functionalities into more specialized directories like Changes
Sequence DiagramsequenceDiagram
participant Generator
participant GeneratorLoader
participant ProviderManager
participant OutputBuilder
Generator->>GeneratorLoader: Load generator configuration
GeneratorLoader-->>Generator: Return generator config
Generator->>ProviderManager: Resolve dependencies
ProviderManager-->>Generator: Provide dependencies
Generator->>OutputBuilder: Create output builder
OutputBuilder->>Generator: Prepare output
Generator->>OutputBuilder: Write files
OutputBuilder-->>Generator: Confirm output generation
The sequence diagram illustrates the high-level flow of the generator system after the refactoring, showing how generators are loaded, dependencies are resolved, and output is generated through a more modular approach. Finishing Touches
🪧 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: 4
🔭 Outside diff range comments (2)
packages/sync/src/providers/providers.ts (1)
Line range hint
134-188: Ensure correct binding of 'this' in returned methodsThe methods
dependencyandexport, along with their nested methods likeoptional,reference, andoptionalReference, use thethiskeyword. Since these methods are returned as part of an object and may be called independently,thismight not refer to the expected context, leading to potential errors. To ensure thatthisrefers to the correct object, consider converting these methods to arrow functions, which lexically bindthis.Apply this diff to fix the
thisbinding issue:return { type: 'type', name, isReadOnly: options?.isReadOnly, - dependency() { + dependency: () => { return { ...this, type: 'dependency', options: options?.isReadOnly ? { isReadOnly: true } : {}, - optional() { + optional: () => { return toMerged(this, { options: { optional: true } }); }, - reference(exportName) { + reference: (exportName) => { if (this.options.exportName !== undefined) { throw new Error('Cannot overwrite export name on provider type'); } return toMerged(this, { options: { exportName }, }); }, - optionalReference(exportName) { + optionalReference: (exportName) => { if (this.options.exportName !== undefined) { throw new Error('Cannot overwrite export name on provider type'); } return toMerged(this, { options: { exportName: exportName ?? '', optional: true }, }); }, }; }, - export(scope, exportName) { + export: (scope, exportName) => { return { ...this, type: 'export', exports: [{ scope, exportName }], - andExport(scope, exportName) { + andExport: (scope, exportName) => { return toMerged(this, { exports: [...this.exports, { scope, exportName }], }); }, }; }, };packages/sync/src/actions/write-template-action.ts (1)
Line range hint
13-31: Add error handling and input validation.The template action needs better error handling:
- No validation of template existence
- Template path construction could be unsafe
- No error handling for file operations
- Template rendering errors not caught
Consider this improved implementation:
export const writeTemplateAction = createBuilderActionCreator<[Options]>( (options: Options) => async (builder) => { const { destination, template, data, noFormat } = options; + if (!template || !destination) { + throw new Error('Template and destination are required'); + } + const templatePath = path.join( builder.generatorBaseDirectory, 'templates', template, ); - const templateContents = await fs.readFile(templatePath, 'utf8'); + try { + // Verify template exists + await fs.access(templatePath); + + const templateContents = await fs.readFile(templatePath, 'utf8'); - const contents = ejs.render(templateContents, data); + // Render template with error handling + const contents = await new Promise<string>((resolve, reject) => { + try { + const result = ejs.render(templateContents, data); + resolve(result); + } catch (error) { + reject(new Error(`Template rendering failed: ${error.message}`)); + } + }); - builder.writeFile(destination, contents, { shouldFormat: !noFormat }); + builder.writeFile(destination, contents, { shouldFormat: !noFormat }); + } catch (error) { + throw new Error(`Failed to process template ${template}: ${error.message}`); + } }, );
🧹 Nitpick comments (11)
packages/sync/src/utils/fs.ts (1)
9-15: RefactorpathExiststo useasync/awaitsyntax for consistencyThe function
pathExistsuses.then()and.catch()for promise handling, whereas the rest of the code usesasync/await. Refactoring to useasync/awaitwill improve code consistency and readability.Apply this diff to refactor the function:
export async function pathExists(filePath: string): Promise<boolean> { - return fs - .access(filePath) - .then(() => true) - .catch(() => false); + try { + await fs.access(filePath); + return true; + } catch { + return false; + } }packages/sync/src/engine/engine.ts (1)
49-52: Consider strict schema validation instead of.passthrough()When reading the root descriptor, using
baseDescriptorSchema.passthrough()allows extra properties not defined in the schema. If you want to enforce strict schema validation and prevent unexpected properties, consider using.strict()instead of.passthrough().packages/sync/src/generators/loader.ts (1)
98-98: EnsurereplaceAllcompatibility with supported Node.js versionsThe use of
String.prototype.replaceAllmay not be supported in Node.js versions prior to 15.0.0. If you need to support earlier versions, consider usingreplacewith a global regular expression orsplitandjoinas alternatives.packages/sync/src/utils/load-default-export.ts (1)
4-9: Enhance JSDoc with security considerations.The function documentation should include security considerations for dynamic imports.
Add security notes to the JSDoc:
/** * Simple function to load the default export of a module dynamically * * @param modulePath - Path to the module to load * @returns The default export of the module + * @throws {Error} If the module cannot be loaded + * @security This function performs dynamic imports. Ensure that modulePath comes from a trusted source. */packages/sync/src/output/builder-action.ts (1)
23-29: Consider adding error handling.While the implementation is clean, consider adding error handling for potential failures during execution.
export function createBuilderActionCreator<T extends unknown[]>( creator: (...args: T) => BuilderAction['execute'], ): BuilderActionCreator<T> { return (...args) => ({ - execute: (builder) => creator(...args)(builder), + execute: async (builder) => { + try { + return await creator(...args)(builder); + } catch (error) { + throw new Error(`Builder action execution failed: ${error.message}`); + } + }, }); }packages/sync/src/output/formatter.ts (1)
20-33: Consider adding format options parameter.The formatter interface is well-designed, but consider adding an optional
optionsparameter to theformatfunction for future extensibility.export interface GeneratorOutputFormatter { name: string; format: FormatFunction; fileExtensions?: string[]; + options?: Record<string, unknown>; }packages/sync/src/actions/copy-directory-action.ts (1)
Line range hint
31-46: Consider adding error handling for file operations.While the implementation works, it would be beneficial to add proper error handling for file operations.
await Promise.all( files.map(async (file) => { + try { const relativePath = path.relative(templatePath, file); const destinationPath = path.join(destination, relativePath); if (shouldFormat) { const fileContents = await fs.readFile(file, 'utf8'); builder.writeFile(destinationPath, fileContents, { shouldFormat: true, }); } else { const fileContents = await fs.readFile(file); builder.writeFile(destinationPath, fileContents); } + } catch (error) { + throw new Error(`Failed to copy file ${file}: ${error.message}`); + } }), );packages/sync/src/actions/copy-directory-action.unit.test.ts (1)
19-24: Consider using object destructuring for action parametersWhile the functionality is correct, consider using object destructuring for better readability:
- await copyDirectoryAction({ - source: '/', - destination: '/', - }).execute(builder); + const { execute } = copyDirectoryAction({ + source: '/', + destination: '/', + }); + await execute(builder);packages/sync/src/generators/generators.ts (1)
132-160: Consider adding validation examples in documentation.The
GeneratorConfiginterface is well-structured, but it would be helpful to add examples of descriptor validation in the documentation.packages/sync/src/output/generator-task-output.ts (1)
260-273: Consider adding formatter priority mechanism.The current implementation doesn't handle formatter priority, which could be important when multiple formatters could apply to the same file extension.
Consider adding a priority field to the formatter interface and sorting formatters by priority when applying them.
packages/sync/src/utils/create-generator-with-tasks.ts (1)
Line range hint
25-31: Consider adding JSDoc for type parameters.While the interface documentation is good, consider adding JSDoc for the type parameters to improve code maintainability.
Apply this diff to add type parameter documentation:
/** * The output of a simple generator task. + * @template TaskOutput - The type of the task's output */ export interface SimpleGeneratorTaskOutput<TaskOutput = void> { name: string; getOutput: () => TaskOutput; }Also applies to: 38-40
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Lite
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (53)
.changeset/blue-wolves-nail.md(1 hunks).vscode/generator.json.code-snippets(0 hunks)packages/core-generators/src/generators/node/prettier/index.ts(1 hunks)packages/sync/eslint.config.js(1 hunks)packages/sync/package.json(1 hunks)packages/sync/src/actions/copy-directory-action.ts(2 hunks)packages/sync/src/actions/copy-directory-action.unit.test.ts(4 hunks)packages/sync/src/actions/copy-file-action.ts(1 hunks)packages/sync/src/actions/write-formatted-action.ts(1 hunks)packages/sync/src/actions/write-json-action.ts(1 hunks)packages/sync/src/actions/write-template-action.ts(1 hunks)packages/sync/src/core/descriptor.ts(0 hunks)packages/sync/src/core/engine/descriptor-loader.ts(0 hunks)packages/sync/src/core/engine/descriptor-loader.unit.test.ts(0 hunks)packages/sync/src/core/generator-output.ts(0 hunks)packages/sync/src/core/generator.ts(0 hunks)packages/sync/src/core/index.ts(0 hunks)packages/sync/src/core/loader.ts(0 hunks)packages/sync/src/engine/engine.ts(3 hunks)packages/sync/src/generators/entry-builder.ts(4 hunks)packages/sync/src/generators/entry-builder.unit.test.ts(4 hunks)packages/sync/src/generators/generators.ts(1 hunks)packages/sync/src/generators/index.ts(1 hunks)packages/sync/src/generators/loader.ts(1 hunks)packages/sync/src/generators/loader.unit.test.ts(3 hunks)packages/sync/src/index.ts(1 hunks)packages/sync/src/output/builder-action.ts(1 hunks)packages/sync/src/output/formatter.ts(1 hunks)packages/sync/src/output/generator-task-output.ts(1 hunks)packages/sync/src/output/index.ts(1 hunks)packages/sync/src/output/merge-algorithms/index.ts(1 hunks)packages/sync/src/output/write-generator-output.ts(2 hunks)packages/sync/src/output/write-generator-output.unit.test.ts(11 hunks)packages/sync/src/providers/export-scopes.ts(1 hunks)packages/sync/src/providers/index.ts(1 hunks)packages/sync/src/providers/provider.unit.test.ts(1 hunks)packages/sync/src/providers/providers.ts(5 hunks)packages/sync/src/runner/dependency-map.ts(1 hunks)packages/sync/src/runner/dependency-map.unit.test.ts(1 hunks)packages/sync/src/runner/dependency-sort.ts(1 hunks)packages/sync/src/runner/dependency-sort.unit.test.ts(1 hunks)packages/sync/src/runner/generator-runner.ts(5 hunks)packages/sync/src/runner/generator-runner.unit.test.ts(2 hunks)packages/sync/src/runner/index.ts(1 hunks)packages/sync/src/runner/tests/factories.test-helper.ts(2 hunks)packages/sync/src/runner/utils.ts(1 hunks)packages/sync/src/utils/create-generator-types.ts(1 hunks)packages/sync/src/utils/create-generator-with-tasks.ts(2 hunks)packages/sync/src/utils/fs.ts(1 hunks)packages/sync/src/utils/load-default-export.ts(1 hunks)packages/sync/src/utils/require.ts(0 hunks)packages/sync/src/utils/validation.ts(1 hunks)packages/sync/tsconfig.json(1 hunks)
💤 Files with no reviewable changes (9)
- packages/sync/src/core/engine/descriptor-loader.unit.test.ts
- packages/sync/src/core/engine/descriptor-loader.ts
- packages/sync/src/core/descriptor.ts
- .vscode/generator.json.code-snippets
- packages/sync/src/core/index.ts
- packages/sync/src/utils/require.ts
- packages/sync/src/core/loader.ts
- packages/sync/src/core/generator-output.ts
- packages/sync/src/core/generator.ts
✅ Files skipped from review due to trivial changes (14)
- packages/sync/src/providers/index.ts
- packages/sync/eslint.config.js
- packages/sync/src/actions/write-json-action.ts
- packages/sync/src/utils/create-generator-types.ts
- packages/sync/src/runner/dependency-sort.unit.test.ts
- packages/sync/src/runner/dependency-map.unit.test.ts
- .changeset/blue-wolves-nail.md
- packages/sync/src/runner/index.ts
- packages/sync/src/generators/index.ts
- packages/sync/src/output/index.ts
- packages/sync/src/actions/copy-file-action.ts
- packages/sync/src/utils/validation.ts
- packages/sync/src/providers/provider.unit.test.ts
- packages/sync/src/actions/write-formatted-action.ts
🔇 Additional comments (39)
packages/sync/tsconfig.json (1)
12-12: LGTM! Verify test file migration.The updated exclusion pattern aligns with the refactoring objectives to improve code organization. The change reflects the new directory structure where test files have been moved from
src/merge/teststosrc/output/merge-algorithms/tests.Let's verify that all test files have been properly migrated:
✅ Verification successful
Test file migration verified successfully
The test files have been completely migrated to the new location
src/output/merge-algorithms/testswith proper organization into categories (diff3, json, simple-diff). No test files remain in the old location.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify test file migration is complete # Expected: No test files in old location, all test files in new location echo "Checking for any remaining test files in the old location..." fd . "packages/sync/src/merge/tests" echo "Verifying test files exist in the new location..." fd . "packages/sync/src/output/merge-algorithms/tests" echo "Checking for any test files that might have been missed in the migration..." rg -g "*.test.*" -g "*.spec.*" "packages/sync/src/merge"Length of output: 3155
packages/sync/src/output/write-generator-output.unit.test.ts (4)
6-9: LGTM! Clean import restructuring.The import path changes and mock setup follow best practices. The use of explicit node: imports and proper mocking of filesystem modules is a good practice.
102-102: LGTM! Comprehensive test coverage for various scenarios.The test suite thoroughly covers different use cases including:
- Empty outputs
- File merging strategies
- Conflict handling
- Binary files
- Post-write command execution
Also applies to: 135-135, 171-171, 200-200, 214-214, 240-240, 286-286, 318-318
23-23: Address the TODO comment for clean folder tests.The test suite is missing coverage for folder cleaning functionality. This is important for ensuring proper cleanup during file operations.
Would you like me to help implement the missing test cases for clean folder functionality? This would help ensure comprehensive test coverage.
26-26: Verify formatter functionality with the newglobalFormattersparameter.The test correctly validates the formatting functionality with the new parameter name. However, let's ensure we have test coverage for edge cases with multiple formatters.
Run this script to check for other formatter-related tests:
Also applies to: 46-51
✅ Verification successful
The formatter functionality is properly tested with the
globalFormattersparameterThe test suite includes comprehensive coverage for the formatter functionality:
- Mock formatter verification with correct parameters
- File extension filtering
- Integration with other features (merging, binary files)
- Edge cases with empty formatters array
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other formatter-related tests to ensure comprehensive coverage rg -l "globalFormatters.*format.*function" --type tsLength of output: 54
Script:
#!/bin/bash # Search with broader patterns to find formatter-related code echo "=== Searching for formatter-related tests ===" rg -l "globalFormatters" --type ts echo -e "\n=== Searching for formatter implementations ===" ast-grep --pattern 'function $_($$_): $_ { $$$ format($$$) $$$ }' echo -e "\n=== Checking the specific test file content ===" cat packages/sync/src/output/write-generator-output.unit.test.tsLength of output: 9131
packages/sync/src/providers/export-scopes.ts (1)
1-34: Well-structured implementation of ProviderExportScopeThe implementation of
ProviderExportScopeandcreateProviderExportScopeis clear and follows best practices. Input validation and error handling are appropriately handled.packages/sync/src/output/merge-algorithms/index.ts (1)
Line range hint
1-5: LGTM! Well-organized exports.The new export follows the established pattern and maintains consistency with other exports in the file.
packages/sync/src/index.ts (1)
2-5: Verify impact of restructured exports on package consumers.The reorganization from
core/mergetoengine/generators/output/providersimproves code organization. However, since this is the main entry point, we should verify that this change doesn't break existing consumers.Let's check for potential usage patterns that might be affected:
✅ Verification successful
Export reorganization appears safe
The codebase scan shows that all consumers import from the main package entry point
@halfdomelabs/syncrather than from specific subdirectories. The restructuring of internal exports fromcore/mergetoengine/generators/output/providerswon't impact external consumers since they're using the public API through the main entry point.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for import patterns from @coderabbit/sync across the codebase rg "from ['\"]@coderabbit/sync['\"]|from ['\"]@coderabbit/sync/(core|merge)['\"]" -g '!{node_modules,dist}/**'Length of output: 120
Script:
#!/bin/bash # First check package name in package.json cat packages/sync/package.json | jq -r '.name' # Then search for relative imports of core/merge rg "from ['\"]\.\.?/(?:core|merge)['\"]" packages/sync/ # Also search for any imports from this package in tests rg "from ['\"]\.\.?/" packages/sync/src/__tests__/Length of output: 251
Script:
#!/bin/bash # Search for package imports with correct name rg "from ['\"]@halfdomelabs/sync['\"]|from ['\"]@halfdomelabs/sync/(core|merge)['\"]" -g '!{node_modules,dist}/**' # Find and check test files fd "__tests__" packages/sync/ -t f -x rg "from ['\"]\.\.?/" {}Length of output: 23244
packages/sync/src/actions/write-template-action.ts (1)
5-5: LGTM! Import path update aligns with refactoring.The change from relative to absolute import using
@srcalias improves code organization and maintainability.packages/sync/src/output/builder-action.ts (1)
6-8: LGTM! Well-designed interface with async support.The
BuilderActioninterface is cleanly designed with proper TypeScript types and async support.packages/sync/src/output/formatter.ts (1)
11-15: LGTM! Well-typed format function definition.The
FormatFunctiontype is well-defined with proper parameter types and async support.packages/sync/src/runner/utils.ts (1)
3-11: LGTM! Clean import path updates.The import path updates align with the package reorganization effort.
packages/sync/src/actions/copy-directory-action.ts (1)
25-29: LGTM! Improved file discovery with globby.The switch to
globbyprovides more robust file discovery and potential performance improvements.packages/sync/src/runner/tests/factories.test-helper.ts (2)
6-6: LGTM! Import path update aligns with package reorganizationThe import path change reflects the move of generator-related types to a dedicated
generatorsdirectory, improving code organization.
54-54: LGTM! Property addition matches type changesThe addition of
generatorBaseDirectoryproperty aligns with the updatedGeneratorEntryinterface.packages/sync/src/runner/dependency-sort.ts (1)
5-5: LGTM! Import path update maintains consistencyThe import path update aligns with the package's new organizational structure, maintaining consistency with other files.
packages/sync/src/actions/copy-directory-action.unit.test.ts (1)
4-5: LGTM! Output builder standardizationThe switch to
GeneratorTaskOutputBuilderstandardizes the output handling across the codebase.packages/sync/src/runner/generator-runner.unit.test.ts (2)
3-4: LGTM! Comprehensive import reorganizationThe import updates properly organize types and utilities into their respective specialized directories (
generators,providers,output).Also applies to: 7-14
28-28: LGTM! Type update maintains consistencyThe builder parameter type update to
GeneratorTaskOutputBuilderaligns with the standardized output handling.packages/sync/src/generators/loader.unit.test.ts (2)
4-4: LGTM! Import and mock updates align with the refactored structure.The changes correctly reflect the reorganization of the sync package, with updated import paths and mock implementations.
Also applies to: 6-7, 12-12, 14-14
46-51: LGTM! Mock implementations are consistent.The mock implementations correctly use
loadDefaultExportconsistently across all test cases, maintaining the test coverage while adapting to the refactored structure.Also applies to: 102-107
packages/sync/src/runner/generator-runner.ts (3)
69-73: Improved error message clarity for provider validation.The error message now more clearly indicates when a task has exports but no providers.
94-96: LGTM! Updated output builder instantiation.The code correctly uses the new
GeneratorTaskOutputBuilderclass with the appropriate constructor parameter.
120-122: Verify the impact of renaming 'formatters' to 'globalFormatters'.The property has been renamed from 'formatters' to 'globalFormatters'. Ensure all consumers of this output are updated accordingly.
✅ Verification successful
Property rename verification completed successfully
The rename from 'formatters' to 'globalFormatters' has been properly implemented across the codebase. The only occurrence of '.formatters' found is unrelated and belongs to Pino logger configuration in the logger service.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining references to the old 'formatters' property rg -l "\.formatters\b" --type tsLength of output: 106
Script:
#!/bin/bash # Check the content of the file with formatters reference cat packages/fastify-generators/src/generators/core/logger-service/index.ts # Search for type definitions related to generator outputs ast-grep --pattern 'interface $_Output { $$$ formatters$_ $$$ }' # Also search for type definitions with globalFormatters ast-grep --pattern 'interface $_Output { $$$ globalFormatters$_ $$$ }'Length of output: 4591
Script:
#!/bin/bash # Search for generator output related types rg -A 5 "type\s+GeneratorOutput|interface\s+GeneratorOutput" --type tsLength of output: 1331
Script:
#!/bin/bash # Get the full content of the generator task output file cat packages/sync/src/output/generator-task-output.tsLength of output: 6901
packages/sync/src/generators/generators.ts (2)
16-25: LGTM! Well-defined base schema with clear documentation.The base descriptor schema is well-documented and properly defines the core fields required for a generator descriptor.
89-123: Excellent type safety in GeneratorTask interface.The
GeneratorTaskinterface provides strong type safety through:
- Generic type parameters for export and dependency maps
- Conditional type for the run method return value
- Comprehensive documentation
packages/sync/src/generators/entry-builder.ts (2)
92-95: LGTM! Improved JSON validation with schema.The switch to
readJsonWithSchemawithpassthrough()provides better validation while maintaining flexibility for additional fields.
167-169: LGTM! Consistent generator config structure.The return object correctly includes both the generator config and base directory, maintaining consistency with the new structure.
packages/sync/src/generators/entry-builder.unit.test.ts (3)
3-3: LGTM! Import changes align with the refactoring.The imports have been properly updated to reflect the new package structure, with clear separation of concerns between utilities, providers, and generators.
Also applies to: 8-14
184-184: LGTM! Mock expectations properly updated.The mock expectations for
readJsonWithSchemaare correctly updated to include the base descriptor schema.Also applies to: 202-205
64-116: Verify test coverage for all generator configurations.The generator configurations have been restructured to use a
configproperty, which is a good improvement for organization. However, ensure that all possible generator configurations are tested, including edge cases.Run this script to analyze test coverage:
✅ Verification successful
Test coverage for generator configurations is comprehensive and matches production patterns.
The test configurations in entry-builder.unit.test.ts effectively cover:
- Basic generator functionality with dependencies and exports
- Nested generator configurations
- Reference handling and duplicate references
- Descriptor validation
These patterns align well with real-world generator implementations found across the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check test coverage for generator configurations # Look for generator configurations in the codebase and compare with test cases # Find all generator configurations rg -l "createGeneratorWithTasks|createGenerator" # Find corresponding test files rg -l "describe.*generator" --glob "**/*.test.ts"Length of output: 10354
Script:
#!/bin/bash # Search for test files with better patterns echo "=== Test Files ===" rg -l "describe.*[\"'].*generator" --type ts echo -e "\n=== Generator Implementations ===" # Look for actual generator implementations ast-grep --pattern 'createGenerator = () => $$$' ast-grep --pattern 'createGeneratorWithTasks($$$)'Length of output: 66750
packages/sync/src/runner/dependency-map.ts (1)
8-9: LGTM! Import paths updated correctly.The import paths have been properly updated to reflect the new package structure, maintaining clear boundaries between generators and providers.
packages/sync/src/output/generator-task-output.ts (2)
11-132: Well-structured type definitions with clear documentation.The interfaces are well-organized and thoroughly documented, providing a robust type system for generator outputs.
185-201: LGTM! Proper error handling in writeFile method.The method includes appropriate validation:
- Prevents file overwriting
- Validates formatting constraints for Buffer contents
packages/core-generators/src/generators/node/prettier/index.ts (1)
Line range hint
237-241: LGTM! Method updated to match new API.The change from
addFormattertoaddGlobalFormattercorrectly aligns with the newGeneratorTaskOutputBuilderAPI.packages/sync/src/output/write-generator-output.ts (2)
9-12: LGTM!The import paths have been reorganized to improve modularity by moving merge-related types to a dedicated module.
263-263: Verify the impact of the parameter rename.The parameter has been renamed from
formatterstooutput.globalFormatters. This change suggests a broader refactoring of the formatter management system.Run this script to verify the parameter usage across the codebase:
packages/sync/src/utils/create-generator-with-tasks.ts (1)
12-16: LGTM!The import paths have been reorganized to improve code organization, with types now imported from their respective modules.
packages/sync/package.json (1)
40-40: LGTM! Good choice replacing multimatch with globby.The switch to
globbyis a good decision as it's more actively maintained and provides better performance for glob pattern matching.Run this script to verify the removal of
multimatchusage:✅ Verification successful
✓ Verified: multimatch successfully replaced with globby
All instances of
multimatchhave been completely removed and replaced with properglobbyimplementations across the codebase. The migration is thorough and well-executed, with appropriate glob patterns and options being used in each context.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that multimatch has been completely replaced # Search for any remaining multimatch imports or requires echo "Checking for remaining multimatch usage..." rg "(?:require|import).*['\"]multimatch['\"]" # Search for potential glob pattern matching to ensure they're compatible with globby echo "Checking glob pattern usage..." rg "(?:glob|pattern).*['\"]\\*"Length of output: 601
Script:
#!/bin/bash # Check implementation details of files using globby echo "Checking copy-directory-action.ts implementation..." ast-grep --pattern 'const $_ = await globby($$$)' -f packages/sync/src/actions/copy-directory-action.ts echo -e "\nChecking error handling..." rg -B2 -A2 "globby\("Length of output: 3770
| if (!generatorPackagePaths[plugin.name]) { | ||
| const pluginIndex = require.resolve(plugin.name); | ||
| const pluginPath = await packageUp({ | ||
| cwd: path.dirname(pluginIndex), | ||
| }); | ||
| if (pluginPath) { | ||
| generatorPackagePaths[plugin.name] = path.dirname(pluginPath); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling when resolving plugin paths
In the loadGeneratorsForProject function, if require.resolve(plugin.name) fails because the plugin cannot be found, it will throw an error and halt execution. To handle missing or misconfigured plugins gracefully, consider wrapping the require.resolve and packageUp calls in a try/catch block to handle the errors appropriately without crashing the application.
Apply this diff to add error handling:
if (!generatorPackagePaths[plugin.name]) {
- const pluginIndex = require.resolve(plugin.name);
- const pluginPath = await packageUp({
- cwd: path.dirname(pluginIndex),
- });
- if (pluginPath) {
- generatorPackagePaths[plugin.name] = path.dirname(pluginPath);
- }
+ try {
+ const pluginIndex = require.resolve(plugin.name);
+ const pluginPath = await packageUp({
+ cwd: path.dirname(pluginIndex),
+ });
+ if (pluginPath) {
+ generatorPackagePaths[plugin.name] = path.dirname(pluginPath);
+ }
+ } catch (error) {
+ logger.warn(`Failed to resolve plugin "${plugin.name}": ${error.message}`);
+ }
}📝 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.
| if (!generatorPackagePaths[plugin.name]) { | |
| const pluginIndex = require.resolve(plugin.name); | |
| const pluginPath = await packageUp({ | |
| cwd: path.dirname(pluginIndex), | |
| }); | |
| if (pluginPath) { | |
| generatorPackagePaths[plugin.name] = path.dirname(pluginPath); | |
| } | |
| } | |
| if (!generatorPackagePaths[plugin.name]) { | |
| try { | |
| const pluginIndex = require.resolve(plugin.name); | |
| const pluginPath = await packageUp({ | |
| cwd: path.dirname(pluginIndex), | |
| }); | |
| if (pluginPath) { | |
| generatorPackagePaths[plugin.name] = path.dirname(pluginPath); | |
| } | |
| } catch (error) { | |
| logger.warn(`Failed to resolve plugin "${plugin.name}": ${error.message}`); | |
| } | |
| } |
| export async function loadDefaultExport(modulePath: string): Promise<unknown> { | ||
| const fileUrl = modulePath.startsWith('file://') | ||
| ? modulePath | ||
| : pathToFileURL(modulePath).href; | ||
| const fullUrl = | ||
| path.extname(fileUrl) === '.js' ? fileUrl : `${fileUrl}/index.js`; | ||
| const importedModule = (await import(fullUrl)) as | ||
| | { default?: unknown } | ||
| | undefined; | ||
| if (!importedModule?.default) { | ||
| return undefined; | ||
| } | ||
| return importedModule.default; | ||
| } |
There was a problem hiding this comment.
Add error handling and input validation.
The function needs better error handling and input validation:
- Missing try-catch for import failures
- No validation of modulePath
- Path manipulation could be more robust
Consider this improved implementation:
export async function loadDefaultExport(modulePath: string): Promise<unknown> {
+ if (!modulePath) {
+ throw new Error('modulePath is required');
+ }
+
const fileUrl = modulePath.startsWith('file://')
? modulePath
: pathToFileURL(modulePath).href;
+
const fullUrl =
- path.extname(fileUrl) === '.js' ? fileUrl : `${fileUrl}/index.js`;
+ path.extname(fileUrl) === '.js' ? fileUrl : path.join(fileUrl, 'index.js');
+
+ try {
const importedModule = (await import(fullUrl)) as
| { default?: unknown }
| undefined;
if (!importedModule?.default) {
return undefined;
}
return importedModule.default;
+ } catch (error) {
+ throw new Error(`Failed to load module at ${modulePath}: ${error.message}`);
+ }
}Committable suggestion skipped: line range outside the PR's diff.
| const generatorConfigEntry = context.generatorMap[descriptor.generator]; | ||
| if (!generatorConfigEntry) { | ||
| throw new Error( | ||
| `Generator ${descriptor.generator} not found in generator map`, | ||
| ); | ||
| } | ||
| const { config } = generatorConfigEntry; | ||
|
|
||
| const { children = {}, validatedDescriptor } = | ||
| generatorConfig.parseDescriptor(descriptor, { | ||
| generatorMap: context.generatorMap, | ||
| id, | ||
| logger: context.logger, | ||
| }); | ||
| const { children = {}, validatedDescriptor } = config.parseDescriptor( | ||
| descriptor, | ||
| { id, logger: context.logger }, | ||
| ); |
There was a problem hiding this comment.
💡 Codebase verification
Missing test coverage for generator not found error
The error handling for missing generators is implemented but not verified in tests. A test case should be added to entry-builder.unit.test.ts to verify that attempting to use a non-existent generator throws the expected error message.
Example test to add:
it('should throw when generator is not found in generator map', async () => {
await expect(buildGeneratorEntry(
{ generator: 'non-existent' },
'project',
generatorContext,
)).rejects.toThrow('Generator non-existent not found in generator map');
});🔗 Analysis chain
Verify error handling for missing generators.
The error handling for missing generators has been improved. However, we should verify that the error message is propagated correctly to the user.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for error handling patterns in tests
rg -A 5 "Generator.*not found in generator map" --type ts
Length of output: 533
Script:
#!/bin/bash
# Find test files related to entry-builder
fd "entry-builder.test|entry-builder.spec" --type f
# Find imports of entry-builder
rg -l "from.*entry-builder" --type ts
# Look for error handling patterns in test files
rg -B 2 -A 2 "expect.*toThrow|expect.*rejects" --type ts packages/sync/src/
Length of output: 3874
Script:
#!/bin/bash
# Check entry-builder test file content
cat packages/sync/src/generators/entry-builder.unit.test.ts
# Look for any test cases involving generatorMap
rg -B 3 -A 3 "generatorMap" packages/sync/src/generators/entry-builder.unit.test.ts
Length of output: 7193
| resolvePath(relativePath: string): string { | ||
| return ( | ||
| ( | ||
| this.baseDirectory | ||
| ? path.join(this.baseDirectory, relativePath) | ||
| : relativePath | ||
| ) | ||
| // normalize all paths to POSIX style / paths | ||
| .replaceAll(path.sep, path.posix.sep) | ||
| ); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider handling path traversal security risks.
The path resolution logic should be enhanced to prevent path traversal attacks.
Apply this diff to add path validation:
resolvePath(relativePath: string): string {
+ const normalizedPath = path.normalize(relativePath);
+ if (normalizedPath.startsWith('..')) {
+ throw new Error('Path traversal is not allowed');
+ }
return (
(
this.baseDirectory
- ? path.join(this.baseDirectory, relativePath)
+ ? path.join(this.baseDirectory, normalizedPath)
- : relativePath
+ : normalizedPath
)
// normalize all paths to POSIX style / paths
.replaceAll(path.sep, path.posix.sep)
);
}📝 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.
| resolvePath(relativePath: string): string { | |
| return ( | |
| ( | |
| this.baseDirectory | |
| ? path.join(this.baseDirectory, relativePath) | |
| : relativePath | |
| ) | |
| // normalize all paths to POSIX style / paths | |
| .replaceAll(path.sep, path.posix.sep) | |
| ); | |
| } | |
| resolvePath(relativePath: string): string { | |
| const normalizedPath = path.normalize(relativePath); | |
| if (normalizedPath.startsWith('..')) { | |
| throw new Error('Path traversal is not allowed'); | |
| } | |
| return ( | |
| ( | |
| this.baseDirectory | |
| ? path.join(this.baseDirectory, normalizedPath) | |
| : normalizedPath | |
| ) | |
| // normalize all paths to POSIX style / paths | |
| .replaceAll(path.sep, path.posix.sep) | |
| ); | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/sync/src/generators/entry-builder.unit.test.ts (2)
202-205: Enhance mock validation for readJsonWithSchema.The test should explicitly verify that the correct schema is being passed to
readJsonWithSchema.Replace the generic
expect.anything()with a more specific expectation:expect(mockedReadJsonWithSchema).toHaveBeenCalledWith( '/root/child-descriptor.json', - expect.anything() as unknown, + baseDescriptorSchema, );
Line range hint
1-205: Consider adding error case tests.While the happy path tests are comprehensive, consider adding test cases for:
- Invalid descriptor schema validation
- File not found scenarios for reference children
- Malformed generator configurations
Would you like me to provide example test cases for these scenarios?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Lite
📒 Files selected for processing (2)
packages/sync/.prettierignore(1 hunks)packages/sync/src/generators/entry-builder.unit.test.ts(4 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/sync/.prettierignore
🔇 Additional comments (2)
packages/sync/src/generators/entry-builder.unit.test.ts (2)
3-3: LGTM! Well-organized imports.The import statements are properly organized, with a clear separation between function imports and type imports. The use of absolute imports for utilities is a good practice.
Also applies to: 6-6, 8-14
64-76: LGTM! Well-structured generator configurations.The generator configurations are well-organized with a clear separation of concerns between configuration and directory information.
Also applies to: 79-88, 91-95, 98-107, 110-116
| } from './generators.js'; | ||
|
|
||
| vi.mock('./descriptor-loader'); | ||
| vi.mock('../utils/fs.js'); |
There was a problem hiding this comment.
Fix mock path inconsistency.
The mock path ../utils/fs.js doesn't match the import path @src/utils/fs.js. This could lead to issues with the mock not being properly applied.
Apply this diff to fix the mock path:
-vi.mock('../utils/fs.js');
+vi.mock('@src/utils/fs.js');Also applies to: 18-18
|
@coderabbitai pause |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/sync/src/generators/entry-builder.unit.test.ts (1)
15-17:⚠️ Potential issueFix mock path inconsistency.
The mock path
../utils/fs.jsdoesn't match the import path@src/utils/fs.js. This could lead to issues with the mock not being properly applied.Apply this diff to fix the mock path:
-vi.mock('../utils/fs.js'); +vi.mock('@src/utils/fs.js');
🧹 Nitpick comments (1)
packages/sync/src/generators/entry-builder.unit.test.ts (1)
183-204: Add test cases for error scenarios.While the happy path is well tested, consider adding test cases for:
- Invalid JSON file content
- Schema validation failures
- File not found scenarios
Example test case:
it('should handle invalid JSON file', async () => { mockedReadJsonWithSchema.mockRejectedValueOnce(new Error('Invalid JSON')); await expect( buildGeneratorEntry( { generator: 'reference' }, 'project', generatorContext, ) ).rejects.toThrow('Invalid JSON'); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Lite
📒 Files selected for processing (1)
packages/sync/src/generators/entry-builder.unit.test.ts(4 hunks)
🔇 Additional comments (1)
packages/sync/src/generators/entry-builder.unit.test.ts (1)
62-115: Well-structured generator configurations!The new configuration structure with separate
configanddirectoryproperties provides better organization and type safety. The consistent pattern across all generator types improves maintainability.
✅ Actions performedReviews paused. |
Summary by CodeRabbit
Release Notes for Sync Package
New Features
Refactoring
Dependency Updates
globbylibrary for file path matching.multimatchdependency.Performance Improvements
Breaking Changes