feat: Refactor output file writing and support file IDs to allow rename operations #428
Conversation
ENG-384 Support renaming files and merging changes in
Right now, if you rename a model that triggers renames in the generated files, it will attempt to delete the old file and create a new file. However, this is less ideal since it means that any changes in the old file will not be ported over to the new file. Therefore, we need a way to track renames during generation of files. I have a proposed approach for doing so:
|
🦋 Changeset detectedLatest commit: f490199 The changes in this PR will be included in the next version bump. This PR includes changesets to release 13 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
WalkthroughThis pull request introduces a comprehensive refactoring of the file generation and synchronization process across multiple packages. The changes focus on enhancing the flexibility of file handling, introducing more robust merge algorithms, and improving the overall structure of file writing operations. Key modifications include restructuring file writing mechanisms, updating merge algorithm types, and introducing new utility functions for file and generator management. Changes
Sequence DiagramsequenceDiagram
participant Generator
participant PrepareFiles
participant WriteFiles
participant PostWriteCommands
Generator->>PrepareFiles: Prepare files for generation
PrepareFiles-->>PrepareFiles: Apply merge algorithms
PrepareFiles->>WriteFiles: Generate file operations
WriteFiles->>WriteFiles: Write files to output
WriteFiles-->>PostWriteCommands: Collect modified paths
PostWriteCommands->>PostWriteCommands: Filter and sort commands
PostWriteCommands->>PostWriteCommands: Execute commands
This diagram illustrates the enhanced file generation process, highlighting the new steps of file preparation, writing, and post-write command execution with improved merge and conflict resolution capabilities. 🪧 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: 5
🧹 Nitpick comments (51)
packages/sync/src/output/write-generator-output.unit.test.ts (4)
27-71: Consider enhancing test description and assertions.While the test covers the basic functionality well, consider:
- Making the test description more specific about what's being tested (e.g., "should write files to correct paths and execute post-write commands successfully")
- Adding assertions for file contents
- Adding edge cases for empty files or special characters in paths
74-106: Strengthen conflict handling test.Consider adding assertions for:
- The actual file contents after conflict (whether original content is preserved)
- The exact reason for command failure
- Edge cases where only some files conflict while others succeed
140-168: Consider additional formatter error scenarios.While the basic formatter error case is covered, consider adding tests for:
- Multiple formatters where only one fails
- Different types of formatting errors
- Partial success scenarios where some files are formatted successfully
20-168: Consider adding tests for edge cases.The test suite is well-structured, but consider adding tests for:
- Empty file lists
- Invalid/duplicate file IDs
- Concurrent write operations
- Large file lists performance
Example test structure for empty file list:
it('should handle empty file list gracefully', async () => { const output: GeneratorOutput = { files: new Map(), globalFormatters: [], postWriteCommands: [] }; const result = await writeGeneratorOutput(output, outputDirectory, { logger }); expect(result.fileIdToRelativePathMap.size).toBe(0); expect(result.relativePathsWithConflicts).toHaveLength(0); });packages/sync/src/output/string-merge-algorithms/composite-merge.ts (1)
3-16: Consider adding error handling and logging.The implementation is clean and follows good practices. However, consider these improvements:
- Add error handling for individual algorithm failures
- Include logging for debugging purposes
- Consider adding a timeout mechanism for long-running algorithms
Here's a suggested enhancement:
export function buildCompositeMergeAlgorithm( mergeAlgorithms: StringMergeAlgorithm[], ): StringMergeAlgorithm { return async (input) => { + const errors: Error[] = []; // try merge algorithms in order until one works for (const algorithm of mergeAlgorithms) { try { const result = await algorithm(input); if (result) { return result; } + } catch (error) { + errors.push(error); + console.warn(`Merge algorithm failed:`, error); + continue; } } + if (errors.length > 0) { + console.error(`All merge algorithms failed:`, errors); + } return null; }; }packages/sync/src/output/string-merge-algorithms/diff3.ts (1)
12-29: Add error handling and improve type safety.While the implementation is solid, consider these improvements:
- Add try-catch block for merge operations
- Consider validating input strings before merge
- Type assertion could be improved
Here's a suggested enhancement:
export const diff3MergeAlgorithm: StringMergeAlgorithm = (input) => { + // Validate inputs + if (!input.previousWorkingText || !input.previousGeneratedText || !input.currentGeneratedText) { + return null; + } + + try { const mergeResult = merge( input.previousWorkingText, input.previousGeneratedText, input.currentGeneratedText, { // monkey patching types of diff3 lib (https://github.com/bhousel/node-diff3/blob/main/index.mjs#L434) ...({ label: { a: 'existing', b: 'baseplate' }, } as Record<string, unknown>), stringSeparator: '\n', }, ); + + if (!mergeResult?.result) { + return null; + } + return { mergedText: mergeResult.result.join('\n'), hasConflict: mergeResult.conflict, }; + } catch (error) { + console.error('Diff3 merge failed:', error); + return null; + } };packages/sync/src/output/string-merge-algorithms/json.ts (1)
Line range hint
8-44: Improve type safety and error handling for JSON operations.The implementation handles basic errors well, but could be enhanced in several areas:
- Add specific error handling for patch operations
- Improve type safety for JSON structures
- Consider adding validation for JSON schema
Here's a suggested enhancement:
+interface JsonObject { + [key: string]: unknown; +} + export const jsonMergeAlgorithm: StringMergeAlgorithm = (input) => { try { - const originalJson = JSON.parse(input.previousGeneratedText) as Record< - string, - unknown - >; - const newJson = JSON.parse(input.currentGeneratedText) as Record< - string, - unknown - >; - const existingJson = JSON.parse(input.previousWorkingText) as Record< - string, - unknown - >; + const originalJson = JSON.parse(input.previousGeneratedText) as JsonObject; + const newJson = JSON.parse(input.currentGeneratedText) as JsonObject; + const existingJson = JSON.parse(input.previousWorkingText) as JsonObject; + + // Validate JSON objects + if (!isValidJsonObject(originalJson) || !isValidJsonObject(newJson) || !isValidJsonObject(existingJson)) { + return null; + } const diff = jsonPatch.compare(originalJson, newJson, true); if (diff.length === 0) { return { mergedText: input.previousWorkingText, hasConflict: false, }; } + let patchResult; + try { + patchResult = jsonPatch.applyPatch(existingJson, diff, true, false); + } catch (error) { + console.error('JSON patch application failed:', error); + return null; + } const result = JSON.stringify( - jsonPatch.applyPatch(existingJson, diff, true, false).newDocument, + patchResult.newDocument, null, 2, ); return { mergedText: result, hasConflict: false, }; - } catch { + } catch (error) { // default to merge strings method if patching fails + console.error('JSON merge failed:', error); return null; } }; + +function isValidJsonObject(obj: unknown): obj is JsonObject { + return obj !== null && typeof obj === 'object' && !Array.isArray(obj); +}packages/sync/src/output/string-merge-algorithms/types.ts (1)
4-14: LGTM! Well-structured type definitions with clear documentation.The types are well-designed with:
- Clear separation of concerns between input and output types
- Support for both successful and failed merge scenarios
- Flexibility for both sync and async implementations
Consider adding examples in the JSDoc comments to illustrate typical usage scenarios, especially for the
StringMergeAlgorithmInputinterface.Also applies to: 19-34, 43-45
packages/sync/src/output/string-merge-algorithms/simple-diff.unit.test.ts (1)
11-14: LGTM! Tests updated correctly for new function signature.The test cases have been properly adapted to use the new object parameter structure while maintaining the same test coverage.
Consider adding test cases for:
- Mixed line endings (CRLF vs LF)
- Unicode characters
- Very large text inputs
- Malformed inputs (undefined, null)
Also applies to: 30-33, 40-43
packages/sync/src/output/string-merge-algorithms/composite-merge.test.ts (1)
25-29: LGTM! Test cases consistently use new input structure.All test cases have been updated to provide the required input fields.
Consider adding test cases for:
- Async algorithms with varying completion times
- Error handling in algorithms
- Mixed sync/async algorithm chains
Also applies to: 43-47, 55-59
packages/sync/src/output/string-merge-algorithms/tests/merge.test-helper.ts (1)
62-66: Update error message to match new variable names.The error message still refers to "user or new files" while the variables have been renamed to more descriptive terms.
- throw new Error(`Missing required user or new files in ${casePath}`); + throw new Error(`Missing required previous working text or current generated text files in ${casePath}`);packages/project-builder-server/src/sync/index.ts (2)
25-43: Remove commented-outrenameCleanDirectoryIfExistsfunctionThe entire
renameCleanDirectoryIfExistsfunction is commented out. If it's no longer needed, consider removing it to keep the codebase clean. If you plan to use it in the future, add aTODOcomment explaining its intended purpose.
50-63: Handle JSON parsing errors ingetPreviousGeneratedFileIdMapIf
file-id-map.jsonexists but contains malformed JSON,fs.readJsonwill throw a parsing error that's currently unhandled. Consider catching JSON parsing errors to provide clearer error messages and prevent the application from crashing unexpectedly.packages/sync/src/utils/concurrency.ts (1)
1-11: LGTM! Consider adding upper bound for very large systems.The implementation follows best practices for I/O-bound concurrency management. The minimum threshold of 10 ensures good parallelism even on low-core systems.
Consider adding an upper bound (e.g., 32) to prevent excessive resource usage on systems with many cores:
export function getGenerationConcurrencyLimit(): number { const cpus = os.cpus().length; - return Math.max(cpus * 2, 10); + return Math.min(Math.max(cpus * 2, 10), 32); }packages/sync/src/output/write-generator-file/errors.ts (1)
1-11: LGTM! Consider preserving full error details.The error class implementation is robust and user-friendly. The limit of 10 errors in the message is good for readability.
Consider adding a method to access all errors, even if the message only shows 10:
export class WriteGeneratorFilesError extends Error { constructor(public errors: { relativePath: string; error: unknown }[]) { super( `Error writing generator files (showing first 10): ${errors .slice(0, 10) .map(({ relativePath, error }) => `${relativePath}: ${String(error)}`) .join('\n')}`, ); this.name = 'WriteGeneratorFilesError'; } + + public getAllErrors(): string[] { + return this.errors.map( + ({ relativePath, error }) => `${relativePath}: ${String(error)}` + ); + } }packages/sync/src/utils/find-duplicates.unit.test.ts (1)
5-13: Add test cases for edge scenarios.While the basic test cases are good, consider adding tests for:
- Empty array input
- Multiple duplicates (e.g.,
['a', 'b', 'a', 'b'])- Case sensitivity (if relevant)
- Special characters
it('should handle empty array', () => { expect(findDuplicates([])).toEqual([]); }); it('should find multiple duplicates', () => { expect(findDuplicates(['a', 'b', 'a', 'b'])).toEqual(['a', 'b']); }); it('should handle case sensitivity', () => { expect(findDuplicates(['a', 'A'])).toEqual([]); // or ['a'] depending on requirements });packages/sync/src/utils/find-duplicates.ts (1)
1-20: LGTM! Consider adding input validation.The implementation is efficient using Set for O(n) lookup. The function maintains the order of duplicates as they are encountered, which is a good design choice.
Consider adding input validation:
export function findDuplicates(strings: string[]): string[] { + if (!Array.isArray(strings)) { + throw new TypeError('Input must be an array'); + } const stringSet = new Set<string>();packages/sync/src/actions/write-formatted-action.ts (1)
6-6: LGTM! Consider making the options interface more explicit.The changes improve clarity by using more explicit naming and structured parameters. The addition of file ID support aligns well with the PR objectives.
Consider making the options interface more explicit about file operations:
interface Options { destination: string; contents: string; - shouldNeverOverwrite?: boolean; + fileOptions?: { + shouldNeverOverwrite?: boolean; + }; }Also applies to: 11-20
packages/sync/src/actions/write-json-action.ts (1)
4-4: LGTM! Consider enhancing type safety for contents.The changes effectively support file IDs while maintaining backward compatibility. The nullish coalescing operator for ID fallback is a good choice.
Consider enhancing type safety for the contents:
interface Options { id?: string; destination: string; - contents: unknown; + contents: Record<string, unknown>; noFormat?: boolean; }Also applies to: 12-22
packages/sync/src/output/post-write-commands/sort-commands.ts (1)
11-24: Consider enhancing type safety with type guards.The implementation is clean and efficient, but could benefit from stronger typing.
Consider adding type guards:
+type PriorityString = keyof typeof POST_WRITE_COMMAND_PRIORITY; + +function isPriorityString(value: unknown): value is PriorityString { + return typeof value === 'string' && value in POST_WRITE_COMMAND_PRIORITY; +} + export function sortPostWriteCommands( commands: PostWriteCommand[], ): PostWriteCommand[] { const sortedCommands = sortBy(commands, [ (command) => { const priority = command.options?.priority ?? 'DEFAULT'; - return typeof priority === 'number' + return typeof priority === 'number' ? priority - : POST_WRITE_COMMAND_PRIORITY[priority]; + : isPriorityString(priority) + ? POST_WRITE_COMMAND_PRIORITY[priority] + : POST_WRITE_COMMAND_PRIORITY.DEFAULT; }, ]); return sortedCommands; }packages/sync/src/tests/logger.test-helper.ts (1)
9-11: Simplify array initialization.The array initialization can be simplified for better readability.
- let [errorOutput, warnOutput, infoOutput, debugOutput] = Array.from({ - length: 4, - }).fill('') as string[]; + let [errorOutput, warnOutput, infoOutput, debugOutput] = ['', '', '', ''];packages/sync/src/output/prepare-generator-files/errors.ts (1)
20-30: Consider adding total error count in the message.The implementation nicely limits the displayed errors to 10, but it would be helpful to know the total count of errors when there are more than 10.
constructor(public errors: { relativePath: string; error: unknown }[]) { super( - `Error preparing generator files (showing first 10): ${errors + `Error preparing generator files (showing first 10${errors.length > 10 ? ` of ${errors.length}` : ''}): ${errors .slice(0, 10) .map(({ relativePath, error }) => `${relativePath}: ${String(error)}`) .join('\n')}`, );packages/sync/src/actions/copy-directory-action.ts (1)
38-44: Consider deduplicating id and filePath.The
idandfilePathproperties currently hold the same value (destinationPath). Consider:
- Using a helper function to derive one from the other
- Documenting why both are needed if they serve different purposes
- builder.writeFile({ - id: destinationPath, - filePath: destinationPath, + const fileConfig = { + id: builder.generateFileId(destinationPath), + filePath: destinationPath, contents: fileContents, - options: { - shouldFormat: true, - }, + options: shouldFormat ? { shouldFormat: true } : undefined, - }); + }; + builder.writeFile(fileConfig);Also applies to: 48-52
packages/sync/src/output/post-write-commands/run-commands.ts (1)
42-50: Consider implementing a retry mechanism for failed commands.Some commands might fail due to temporary issues (e.g., network connectivity). A retry mechanism with exponential backoff could improve reliability.
} catch (error) { + const maxRetries = 3; + const retryDelay = 1000; // Start with 1 second delay + let retryCount = 0; + + while (retryCount < maxRetries) { + try { + retryCount++; + await new Promise(resolve => setTimeout(resolve, retryDelay * Math.pow(2, retryCount - 1))); + await executeCommand(commandString, { + cwd: path.join(outputDirectory, workingDirectory), + timeout: command.options?.timeout ?? COMMAND_TIMEOUT_MILLIS, + }); + return; // Success, exit retry loop + } catch (retryError) { + if (retryCount === maxRetries) { logger.error(chalk.red(`Unable to run ${commandString}`)); if (error instanceof ExecaError) { logger.error(error.stderr); } else { logger.error(String(error)); } failedCommands.push(command.command); + } + } + } }packages/utils/src/fs/find-nearest-package-json.ts (1)
39-49: Consider adding symlink resolution and caching.The current implementation might have performance implications when called frequently and doesn't handle symlinks explicitly.
Consider:
- Adding a caching mechanism for frequently accessed paths
- Adding explicit symlink resolution using
fs.realpathfor (;;) { try { + const realCurrentDir = await fs.promises.realpath(currentDir); // Check for package.json - const packageJsonPath = path.join(currentDir, 'package.json'); + const packageJsonPath = path.join(realCurrentDir, 'package.json'); const stat = await fs.promises.stat(packageJsonPath); if (stat.isFile()) { return packageJsonPath; } } catch { - // Ignore errors and continue searching + // Log specific error types for debugging + if (error.code !== 'ENOENT') { + logger?.debug(`Error accessing ${currentDir}: ${error.code}`); + } }packages/sync/src/output/write-generator-file/write-generator-files.ts (2)
37-54: Consider adding progress tracking and partial success handling.The parallel file writing implementation could benefit from progress tracking and better error handling.
Consider:
- Adding progress tracking for long-running operations
- Implementing partial success handling
- Adding a rollback mechanism for failed operations
+ const successfulWrites: string[] = []; await Promise.all( fileOperations.map((fileOperation) => writeLimit(async () => { try { await writeGeneratorFile({ fileOperation, outputDirectory, generatedContentsDirectory, }); + successfulWrites.push(fileOperation.relativePath); } catch (error: unknown) { + // Attempt rollback for successful writes if needed + if (successfulWrites.length > 0) { + await rollbackWrites(successfulWrites, outputDirectory); + } writeErrors.push({ relativePath: fileOperation.relativePath, error, }); } }), ), );
33-33: Consider dynamic concurrency limits based on file sizes.The current implementation uses a fixed concurrency limit without considering file sizes.
Consider adjusting the concurrency limit based on file sizes to prevent memory issues with large files:
- const writeLimit = pLimit(getGenerationConcurrencyLimit()); + const concurrencyLimit = await calculateOptimalConcurrency(fileOperations); + const writeLimit = pLimit(concurrencyLimit);packages/sync/src/output/post-write-commands/sort-commands.unit.test.ts (1)
6-52: Add test cases for edge cases.The test suite covers basic priority sorting scenarios well. Consider adding test cases for:
- Invalid priority values
- Mixed priority types (string and numeric in the same array)
Example test case:
it('should handle mixed priority types', () => { const commands: PostWriteCommand[] = [ { command: 'numeric priority', options: { priority: 100 } }, { command: 'string priority', options: { priority: 'DEPENDENCIES' } }, { command: 'no priority' }, ]; const sorted = sortPostWriteCommands(commands); expect(sorted.map((c) => c.command)).toEqual([ 'string priority', 'numeric priority', 'no priority', ]); });packages/sync/src/generators/find-generator-package-name.ts (2)
32-35: Consider using a custom error class for better error handling.Create a specific error class for missing package.json to allow consumers to handle this case differently.
export class MissingPackageJsonError extends Error { constructor(directory: string) { super(`No package.json found for generator at ${directory}`); this.name = 'MissingPackageJsonError'; } } // Usage throw new MissingPackageJsonError(generatorDirectory);
40-43: Replace type assertion with runtime validation.The type assertion
as PackageJsondoesn't provide runtime validation. Consider usingzodfor safer parsing.import { z } from 'zod'; const packageJsonSchema = z.object({ name: z.string(), }).passthrough(); const packageJson = packageJsonSchema.parse(JSON.parse(packageJsonContent)); const packageName = packageJson.name;packages/fastify-generators/src/generators/core/readme/index.ts (1)
Line range hint
19-51: Consider extracting the README template to a separate file.The hardcoded README template makes the code less maintainable. Consider moving it to a separate markdown file.
Example implementation:
import { readFile } from 'node:fs/promises'; import path from 'node:path'; // Load template from file const templatePath = path.join(import.meta.dirname, 'templates/README.md'); const template = await readFile(templatePath, 'utf8'); const readmeContent = template.replace('${projectName}', projectName);packages/sync/src/output/codebase-file-reader.ts (2)
41-50: Improve error type checking with a type guard.The error type checking could be more robust using a type guard function.
interface NodeError extends Error { code: string; } function isNodeError(error: unknown): error is NodeError { return error instanceof Error && 'code' in error; } // Usage .catch((error: unknown): undefined => { if (isNodeError(error) && error.code === 'ENOENT') { return; } throw error; });
36-38: Normalize paths for cross-platform compatibility.Consider normalizing paths to ensure consistent behavior across different platforms.
fileExists: (relativePath: string) => fileExists(path.join(directory, path.normalize(relativePath))),packages/project-builder-server/src/runner/index.ts (1)
17-17: Consider using package imports instead of relative paths.Replace the relative import with a package import for better maintainability:
-import { generateForDirectory } from '../sync/index.js'; +import { generateForDirectory } from '@halfdomelabs/sync';packages/utils/src/fs/find-nearest-package-json.unit.test.ts (1)
8-93: Comprehensive test coverage with room for enhancement.The test suite effectively covers the main scenarios. Consider adding tests for:
- Error cases (e.g., permission denied)
- Symbolic links
- Invalid package.json files
Example test case:
it('handles permission errors gracefully', async () => { vol.fromJSON({ '/project/package.json': '{}', }, '/'); // Mock fs.access to simulate permission denied vi.spyOn(vol, 'access').mockRejectedValueOnce(new Error('EACCES')); const result = await findNearestPackageJson({ cwd: '/project', }); expect(result).toBeUndefined(); });packages/sync/src/output/post-write-commands/run-commands.unit.test.ts (1)
43-45: Consider extracting the default timeout as a constant.The default timeout of 5 minutes (300,000ms) appears as a magic number. Consider extracting this to a named constant for better maintainability.
+const DEFAULT_COMMAND_TIMEOUT_MS = 300_000; // 5 minutes expect(executeCommandMock).toHaveBeenCalledWith('echo "test"', { cwd: '/root', - timeout: 300_000, // 5 minutes in milliseconds + timeout: DEFAULT_COMMAND_TIMEOUT_MS, });packages/sync/src/generators/find-generator-package-name.unit.test.ts (1)
67-78: Consider adding specific JSON parse error test cases.While the test covers invalid JSON, it might be valuable to test specific JSON parse error scenarios.
it('throws error if package.json is invalid', async () => { const testFs = { - '/test/package.json': 'invalid json', + '/test/package.json': '{ "name": "test-package" ', // Missing closing brace '/test/generators/my-generator/index.ts': '', }; vol.fromJSON(testFs, '/'); const cache: PackageNameCache = new Map(); await expect( findGeneratorPackageName('/test/generators/my-generator', cache), ).rejects.toThrow('Failed to read or parse package.json'); }); +it('throws error if package.json contains invalid name type', async () => { + const testFs = { + '/test/package.json': '{ "name": 123 }', // Name should be string + '/test/generators/my-generator/index.ts': '', + }; + vol.fromJSON(testFs, '/'); + + const cache: PackageNameCache = new Map(); + await expect( + findGeneratorPackageName('/test/generators/my-generator', cache), + ).rejects.toThrow('Invalid package name'); +});packages/core-generators/src/generators/docker/docker-compose/index.ts (1)
102-106: Consider adding format option for consistency.While the implementation is correct, consider adding
shouldFormat: trueto the .env file for consistency with the docker-compose.yml file.builder.writeFile({ id: 'docker-env', filePath: dockerEnvPath, contents: `COMPOSE_PROJECT_NAME=${projectName}-dev\n`, + options: { + shouldFormat: true, + }, });packages/sync/src/output/write-generator-file/write-generator-files.unit.test.ts (2)
19-44: Consider adding verification for file permissions.While the test covers basic parallel writing, it should also verify that the created files have appropriate permissions.
expect(vol.toJSON()).toEqual({ '/root/file1.txt': 'content 1', '/root/nested/file2.txt': 'content 2', }); + + // Verify file permissions + const stats1 = await fs.promises.stat('/root/file1.txt'); + const stats2 = await fs.promises.stat('/root/nested/file2.txt'); + expect(stats1.mode & 0o666).toBe(0o666); + expect(stats2.mode & 0o666).toBe(0o666);
55-91: Add test cases for specific error types.The error collection test could be more specific about the types of errors that can occur.
Consider adding test cases for:
- Permission denied errors
- Disk full scenarios
- Invalid path characters
- Path length exceeded
packages/sync/src/output/write-generator-file/write-generator-file.unit.test.ts (2)
37-55: Add a comment explaining the different content in each directory.While the test is well-structured, it would be helpful to document why
merged contentandgenerated contentdiffer, as this might not be immediately obvious to other developers.
79-97: Consider adding file permission checks.While the test validates the creation of conflict files, it would be beneficial to also verify that the files are created with appropriate permissions, especially since conflict files might need different access levels.
packages/sync/src/output/write-generator-file/write-generator-file.ts (2)
26-44: Consider adding specific error handling for common filesystem errors.While the function handles the basic error cases, it would be beneficial to catch and handle specific filesystem errors (e.g., ENOENT, EACCES) to provide more informative error messages.
async function renameFileIfNeeded( previousPath: string, newPath: string, outputDirectory: string, ): Promise<void> { if (previousPath === newPath) return; const fullPreviousPath = path.join(outputDirectory, previousPath); const fullNewPath = path.join(outputDirectory, newPath); if (await pathExists(fullNewPath)) { throw new Error( `Cannot rename ${fullPreviousPath} to ${fullNewPath} as target path already exists`, ); } await ensureDir(path.dirname(fullNewPath)); - await fs.rename(fullPreviousPath, fullNewPath); + try { + await fs.rename(fullPreviousPath, fullNewPath); + } catch (error) { + if (error instanceof Error) { + if ('code' in error) { + switch (error.code) { + case 'ENOENT': + throw new Error(`Source file ${fullPreviousPath} does not exist`); + case 'EACCES': + throw new Error(`Permission denied while renaming ${fullPreviousPath} to ${fullNewPath}`); + default: + throw error; + } + } + } + throw error; + } }
49-55: Add error handling for file writing operations.Similar to the rename function, consider adding specific error handling for common file writing errors.
async function writeFileContents( contents: Buffer, filePath: string, ): Promise<void> { await ensureDir(path.dirname(filePath)); - await fs.writeFile(filePath, contents); + try { + await fs.writeFile(filePath, contents); + } catch (error) { + if (error instanceof Error && 'code' in error) { + switch (error.code) { + case 'ENOSPC': + throw new Error(`No space left on device while writing to ${filePath}`); + case 'EACCES': + throw new Error(`Permission denied while writing to ${filePath}`); + default: + throw error; + } + } + throw error; + } }packages/sync/src/output/prepare-generator-files/prepare-generator-files.unit.test.ts (1)
59-82: Add more specific error assertions.Consider adding assertions to verify:
- The exact error message
- The error details for the failed file
- That the valid file was processed correctly before the error
packages/sync/src/output/clean-deleted-files.unit.test.ts (1)
18-136: Consider extracting test data setup into helper functions.The test cases contain repetitive setup code for the virtual file system and payload objects. Consider extracting these into helper functions to improve maintainability and reduce duplication.
Example refactor:
function setupTestFiles(files: Record<string, string>) { vol.fromJSON(files); } function createPreviousGeneratedPayload(directory: string, idToPathMap: [string, string][]) { return { fileReader: createCodebaseFileReaderFromDirectory(directory), fileIdToRelativePathMap: new Map<string, string>(idToPathMap), }; }packages/sync/src/generators/build-generator-entry.unit.test.ts (1)
29-36: Consider extracting package.json setup into a helper function.The package.json setup is duplicated across multiple test cases. This could be simplified with a helper function.
Example refactor:
function setupPackageJson(path: string, packageName: string) { vol.fromJSON({ [`${path}/package.json`]: JSON.stringify({ name: packageName, }), }, '/'); }Also applies to: 76-86, 153-160, 186-193
packages/sync/src/output/prepare-generator-files/prepare-generator-file.unit.test.ts (1)
105-125: Consider adding specific formatter error cases.While the basic formatter error test is good, consider adding specific cases:
- Syntax errors
- Invalid formatting rules
- Partial formatting failures
Would you like me to propose additional test cases?
packages/react-generators/src/generators/apollo/react-apollo/index.ts (1)
18-18: LGTM! Consider documenting priority relationships.Good use of
POST_WRITE_COMMAND_PRIORITY.CODEGEN + 1to ensure GraphQL code generation runs after Prisma generate. The comment clearly explains the intention.Consider adding a comment in the constants file documenting the relationship between different code generation priorities to help maintain the correct execution order.
Also applies to: 457-459
packages/core-generators/src/generators/node/eslint/index.ts (1)
103-110: LGTM! Consider minor improvements to the implementation.The structured approach with
id,filePath,contents, andoptionsis clear and maintainable. Consider these optional improvements:
- Make the id more specific (e.g., 'node-eslint-ignore') to avoid potential conflicts
- Add input validation for
config.eslintIgnorearraybuilder.writeFile({ - id: 'eslint-ignore', + id: 'node-eslint-ignore', filePath: '.eslintignore', - contents: `${config.eslintIgnore.join('\n')}\n`, + contents: `${(config.eslintIgnore || []).join('\n')}\n`, options: { shouldFormat: true, }, });packages/core-generators/src/generators/node/prettier/index.ts (1)
269-276: LGTM! Refactored .prettierignore file writing with formatting support.The change follows the new pattern for file writing operations and adds proper formatting support. However, consider adding a comment explaining why formatting is needed for the ignore file.
builder.writeFile({ id: 'prettier-ignore', filePath: '.prettierignore', contents: `${prettierIgnoreSorted.join('\n')}\n`, options: { + // Format the ignore file to ensure consistent line endings and trailing newline shouldFormat: true, }, });
📜 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 (98)
.changeset/four-tigers-care.md(1 hunks).changeset/tender-trains-chew.md(1 hunks)knip.jsonc(1 hunks)packages/core-generators/src/actions/copy-typescript-file-action.ts(3 hunks)packages/core-generators/src/actions/copy-typescript-files-action.ts(2 hunks)packages/core-generators/src/generators/docker/docker-compose/index.ts(1 hunks)packages/core-generators/src/generators/node/eslint/index.ts(1 hunks)packages/core-generators/src/generators/node/node/index.ts(2 hunks)packages/core-generators/src/generators/node/prettier/index.ts(1 hunks)packages/core-generators/src/writers/typescript/source-file.ts(1 hunks)packages/fastify-generators/src/generators/core/config-service/index.ts(1 hunks)packages/fastify-generators/src/generators/core/readme/index.ts(1 hunks)packages/fastify-generators/src/generators/pothos/pothos/index.ts(2 hunks)packages/fastify-generators/src/generators/prisma/prisma/index.ts(3 hunks)packages/project-builder-cli/src/index.ts(0 hunks)packages/project-builder-server/package.json(0 hunks)packages/project-builder-server/src/runner/index.ts(1 hunks)packages/project-builder-server/src/sync/index.ts(6 hunks)packages/react-generators/src/generators/apollo/react-apollo/index.ts(2 hunks)packages/react-generators/src/generators/core/react-config/index.ts(1 hunks)packages/react-generators/src/generators/core/react/index.ts(1 hunks)packages/sync/.prettierignore(1 hunks)packages/sync/eslint.config.js(1 hunks)packages/sync/package.json(1 hunks)packages/sync/src/__mocks__/fs.cts(1 hunks)packages/sync/src/__mocks__/fs.ts(0 hunks)packages/sync/src/__mocks__/fs/promises.cts(1 hunks)packages/sync/src/__mocks__/fs/promises.ts(0 hunks)packages/sync/src/actions/copy-directory-action.ts(1 hunks)packages/sync/src/actions/copy-directory-action.unit.test.ts(5 hunks)packages/sync/src/actions/copy-file-action.ts(3 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(2 hunks)packages/sync/src/engine/engine.ts(3 hunks)packages/sync/src/generators/build-generator-entry.ts(4 hunks)packages/sync/src/generators/build-generator-entry.unit.test.ts(10 hunks)packages/sync/src/generators/find-generator-package-name.ts(1 hunks)packages/sync/src/generators/find-generator-package-name.unit.test.ts(1 hunks)packages/sync/src/output/clean-deleted-files.ts(1 hunks)packages/sync/src/output/clean-deleted-files.unit.test.ts(1 hunks)packages/sync/src/output/codebase-file-reader.ts(1 hunks)packages/sync/src/output/generator-task-output.ts(6 hunks)packages/sync/src/output/index.ts(1 hunks)packages/sync/src/output/merge-algorithms/composite-merge.ts(0 hunks)packages/sync/src/output/merge-algorithms/diff3.ts(0 hunks)packages/sync/src/output/merge-algorithms/types.ts(0 hunks)packages/sync/src/output/post-write-commands/filter-commands.ts(1 hunks)packages/sync/src/output/post-write-commands/filter-commands.unit.test.ts(1 hunks)packages/sync/src/output/post-write-commands/index.ts(1 hunks)packages/sync/src/output/post-write-commands/run-commands.ts(1 hunks)packages/sync/src/output/post-write-commands/run-commands.unit.test.ts(1 hunks)packages/sync/src/output/post-write-commands/sort-commands.ts(1 hunks)packages/sync/src/output/post-write-commands/sort-commands.unit.test.ts(1 hunks)packages/sync/src/output/post-write-commands/types.ts(1 hunks)packages/sync/src/output/prepare-generator-files/errors.ts(1 hunks)packages/sync/src/output/prepare-generator-files/index.ts(1 hunks)packages/sync/src/output/prepare-generator-files/prepare-generator-file.ts(1 hunks)packages/sync/src/output/prepare-generator-files/prepare-generator-file.unit.test.ts(1 hunks)packages/sync/src/output/prepare-generator-files/prepare-generator-files.ts(1 hunks)packages/sync/src/output/prepare-generator-files/prepare-generator-files.unit.test.ts(1 hunks)packages/sync/src/output/prepare-generator-files/types.ts(1 hunks)packages/sync/src/output/string-merge-algorithms/composite-merge.test.ts(3 hunks)packages/sync/src/output/string-merge-algorithms/composite-merge.ts(1 hunks)packages/sync/src/output/string-merge-algorithms/diff3.ts(1 hunks)packages/sync/src/output/string-merge-algorithms/json.ts(2 hunks)packages/sync/src/output/string-merge-algorithms/json.unit.test.ts(1 hunks)packages/sync/src/output/string-merge-algorithms/simple-diff.ts(2 hunks)packages/sync/src/output/string-merge-algorithms/simple-diff.unit.test.ts(2 hunks)packages/sync/src/output/string-merge-algorithms/tests/merge.test-helper.ts(4 hunks)packages/sync/src/output/string-merge-algorithms/types.ts(1 hunks)packages/sync/src/output/write-generator-file/errors.ts(1 hunks)packages/sync/src/output/write-generator-file/index.ts(1 hunks)packages/sync/src/output/write-generator-file/write-generator-file.ts(1 hunks)packages/sync/src/output/write-generator-file/write-generator-file.unit.test.ts(1 hunks)packages/sync/src/output/write-generator-file/write-generator-files.ts(1 hunks)packages/sync/src/output/write-generator-file/write-generator-files.unit.test.ts(1 hunks)packages/sync/src/output/write-generator-output.ts(1 hunks)packages/sync/src/output/write-generator-output.unit.test.ts(2 hunks)packages/sync/src/runner/generator-runner.ts(3 hunks)packages/sync/src/runner/generator-runner.unit.test.ts(5 hunks)packages/sync/src/runner/tests/factories.test-helper.ts(1 hunks)packages/sync/src/tests/logger.test-helper.ts(1 hunks)packages/sync/src/utils/concurrency.ts(1 hunks)packages/sync/src/utils/create-generator.unit.test.ts(1 hunks)packages/sync/src/utils/find-duplicates.ts(1 hunks)packages/sync/src/utils/find-duplicates.unit.test.ts(1 hunks)packages/sync/src/utils/fs.ts(1 hunks)packages/sync/tsconfig.json(1 hunks)packages/utils/package.json(2 hunks)packages/utils/src/__mocks__/fs.cts(1 hunks)packages/utils/src/__mocks__/fs/promises.cts(1 hunks)packages/utils/src/fs/find-nearest-package-json.ts(1 hunks)packages/utils/src/fs/find-nearest-package-json.unit.test.ts(1 hunks)packages/utils/src/fs/index.ts(1 hunks)packages/utils/src/node.ts(1 hunks)packages/utils/tsconfig.json(1 hunks)turbo.json(1 hunks)
💤 Files with no reviewable changes (7)
- packages/sync/src/mocks/fs/promises.ts
- packages/project-builder-server/package.json
- packages/sync/src/mocks/fs.ts
- packages/project-builder-cli/src/index.ts
- packages/sync/src/output/merge-algorithms/diff3.ts
- packages/sync/src/output/merge-algorithms/composite-merge.ts
- packages/sync/src/output/merge-algorithms/types.ts
✅ Files skipped from review due to trivial changes (12)
- .changeset/tender-trains-chew.md
- packages/utils/src/fs/index.ts
- packages/sync/src/mocks/fs/promises.cts
- packages/utils/src/mocks/fs/promises.cts
- packages/core-generators/src/writers/typescript/source-file.ts
- packages/utils/src/mocks/fs.cts
- packages/sync/src/mocks/fs.cts
- packages/utils/src/node.ts
- packages/sync/src/output/post-write-commands/index.ts
- packages/sync/.prettierignore
- packages/sync/src/output/write-generator-file/index.ts
- packages/sync/src/output/prepare-generator-files/index.ts
🔇 Additional comments (108)
packages/sync/src/output/write-generator-output.unit.test.ts (2)
4-9: LGTM! Well-structured test setup.The test setup is well-organized with proper mocking, centralized logger creation, and consistent output directory usage.
Also applies to: 20-21
109-137: LGTM! Good multi-directory support test.The test effectively verifies file writing to multiple directories with proper content validation.
packages/sync/src/engine/engine.ts (3)
11-12: LGTM! Type names are now more consistent and descriptive.The renamed types
WriteGeneratorOutputOptionsandWriteGeneratorOutputResultbetter align with the module's functionality and improve code clarity.
31-31: Verify the impact of making loadProject asynchronous.The change to return
Promise<GeneratorEntry>correctly reflects the asynchronous nature ofbuildGeneratorEntry. However, this is a breaking change that could affect existing consumers.Let's verify the impact:
✅ Verification successful
✓ Making loadProject asynchronous aligns with existing usage
The change is safe as the only consumer in
packages/project-builder-server/src/sync/index.tsalready handlesloadProjectasynchronously with properawaitusage.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for direct calls to loadProject that might need updating ast-grep --pattern 'loadProject($$$)' # Search for any potential .then() or await usage with loadProject rg -A 2 'loadProject\('Length of output: 572
53-55: Verify the impact of logger parameter removal and approve interface simplification.The method signature has been simplified by removing the
loggerparameter while maintaining core functionality. This suggests logging is now handled internally bywriteGeneratorOutput.Let's verify the logger parameter removal impact:
✅ Verification successful
Logger parameter change is safe and improves API design
The logger parameter wasn't removed but rather moved to the options object with the same default behavior (console). This is a positive refactoring that maintains all existing functionality while making the API more consistent.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if writeGeneratorOutput now handles logging internally rg -A 5 'writeGeneratorOutput.*logger' # Search for calls to writeOutput that pass a logger parameter ast-grep --pattern 'writeOutput($$$, $$$, $$$, $_)'Length of output: 464
Script:
#!/bin/bash # Find writeGeneratorOutput implementation ast-grep --pattern 'export $_ function writeGeneratorOutput($_) { $$$ }' # Find all calls to writeGeneratorOutput rg -A 2 'writeGeneratorOutput\('Length of output: 1719
Script:
#!/bin/bash # Get the implementation file content cat packages/sync/src/output/write-generator-output.ts # Check for any other files that might define the options type rg -l 'WriteGeneratorOutputOptions'Length of output: 4646
packages/sync/src/output/string-merge-algorithms/json.unit.test.ts (1)
4-8: LGTM! Test structure has been improved.The refactored test setup is more explicit and maintains the same functionality while aligning with the new
StringMergeAlgorithmtype.Let's verify the test coverage:
✅ Verification successful
✓ Test coverage is comprehensive and well-structured
The JSON merge algorithm is thoroughly tested through the shared test framework which:
- Verifies the core merge functionality
- Validates error handling
- Ensures proper conflict detection
- Confirms correct output formatting
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check test coverage for json merge algorithm fd -e ts -e js "json\.(ts|js)$" | grep -v "test" | xargs -I {} rg -l "{}" -g "*test*"Length of output: 87
Script:
#!/bin/bash # Find JSON merge algorithm implementation and its test coverage echo "=== Finding JSON merge algorithm implementation ===" fd -e ts -e js "json" --type f echo -e "\n=== Checking implementation contents ===" ast-grep --pattern 'export $$$jsonMergeAlgorithm$$$' echo -e "\n=== Finding test files referencing the implementation ===" rg -l "jsonMergeAlgorithm" -g "*test*"Length of output: 1146
Script:
#!/bin/bash echo "=== Implementation file contents ===" cat packages/sync/src/output/string-merge-algorithms/json.ts echo -e "\n=== Test file contents ===" cat packages/sync/src/output/string-merge-algorithms/json.unit.test.tsLength of output: 1553
Script:
#!/bin/bash echo "=== Test Helper Contents ===" cat packages/sync/src/output/string-merge-algorithms/tests/merge.test-helper.jsLength of output: 256
Script:
#!/bin/bash echo "=== Finding Test Helper ===" fd "merge.test-helper" --type f echo -e "\n=== Test Helper Contents ===" fd "merge.test-helper" --type f --exec cat {}Length of output: 3457
packages/sync/src/output/string-merge-algorithms/simple-diff.ts (1)
21-27: LGTM! Improved function signature with better type safety.The refactored signature using object destructuring enhances code clarity and maintainability.
Consider adding input validation for edge cases, such as when the input strings contain different line ending styles (CRLF vs LF). This could affect the diff result.
packages/sync/src/output/string-merge-algorithms/composite-merge.test.ts (1)
9-14: LGTM! Mock algorithms correctly implement new type structure.The mock implementations properly handle the new input structure and return types.
packages/sync/src/output/string-merge-algorithms/tests/merge.test-helper.ts (4)
5-6: LGTM! Clean type import.The type-only import for
StringMergeAlgorithmis well-structured and follows best practices.
44-46: LGTM! Clear parameter updates.The signature changes improve type safety with
StringMergeAlgorithmand add flexibility with the optionalformatContentsparameter.
71-75: LGTM! Clean object parameter pattern.The refactored merge algorithm call using an object parameter improves readability and maintainability.
85-88: LGTM! Well-implemented format handling.The format handling is robustly implemented with proper optional chaining and maintains backward compatibility.
packages/sync/src/output/prepare-generator-files/prepare-generator-files.ts (4)
1-13: Well-organized imports and type definitionsThe import statements and type definitions are well-structured, enhancing the readability and maintainability of the code.
17-26: Clear interface definition forPrepareGeneratorFilesInputThe
PrepareGeneratorFilesInputinterface is clearly defined, specifying the required input properties.
28-40: Well-definedPrepareGeneratorFilesResultinterfaceThe
PrepareGeneratorFilesResultinterface effectively outlines the expected output, improving type safety.
48-91: Efficient concurrency handling and error management inprepareGeneratorFilesThe function
prepareGeneratorFilescorrectly utilizesp-limitto manage concurrency and handles errors gracefully by collecting and throwing them as aPrepareGeneratorFilesError.packages/sync/src/generators/build-generator-entry.ts (4)
77-90: Asynchronous update and caching mechanism inbuildGeneratorEntryRecursiveConverting
buildGeneratorEntryRecursiveto anasyncfunction and incorporatingfindGeneratorPackageNamewith caching improves efficiency by reducing redundant filesystem accesses.
105-134: Proper asynchronous recursion over child generator entriesThe implementation correctly handles asynchronous recursion for child entries, ensuring all generator entries are built as expected.
145-155: Initialization ofpackageNameCacheand async update tobuildGeneratorEntryUpdating
buildGeneratorEntryto be asynchronous and initializingpackageNameCacheenhances performance and maintains consistency in package name retrieval.
Line range hint
77-155: Ensure all callers handle the updated asynchronous functionsSince
buildGeneratorEntryRecursiveandbuildGeneratorEntryare now asynchronous, please verify that all callers have been updated to await these functions appropriately.Run the following script to identify usages that may need updates:
✅ Verification successful
All callers properly handle the asynchronous functions
Note: The
buildGeneratorEntryfunction ingenerator-runner.unit.test.tsis a different test helper implementation and not related to the async changes.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all calls to `buildGeneratorEntryRecursive` and `buildGeneratorEntry` that need to handle async changes. # Search for calls to `buildGeneratorEntryRecursive` rg 'buildGeneratorEntryRecursive\(' -A 2 # Search for calls to `buildGeneratorEntry` rg 'buildGeneratorEntry\(' -A 2Length of output: 3828
packages/sync/src/output/write-generator-output.ts (7)
25-42: StructuredWriteGeneratorOutputOptionsinterfaceThe new
WriteGeneratorOutputOptionsinterface organizes the function options effectively, improving code clarity.
47-65: Definition ofWriteGeneratorOutputResultinterface enhances type safetyDefining the result interface clarifies the expected output and enhances type checking.
77-81: Async function update aligns with modern practicesUpdating
writeGeneratorOutputto an asynchronous function ensures non-blocking operations and better handling of asynchronous tasks.
88-96: Proper setup of file writer contextThe initialization of
fileWriterContextwith necessary parameters is well-implemented, setting the stage for file operations.
98-107: Effective modularization usingprepareGeneratorFilesandwriteGeneratorFilesBreaking down the file writing process into preparation and execution improves code modularity and readability.
130-140: Appropriate handling of conflicts before executing post-write commandsThe logic correctly skips running post-write commands when there are file conflicts, preventing potential issues.
Line range hint
175-181: Comprehensive error handling with informative loggingCatching
FormatterErrorand logging detailed information aids in debugging and maintains robustness.packages/project-builder-server/src/sync/index.ts (2)
143-146: EnsurefailedCommandsis always defined before accessinglengthAccessing
failedCommands.lengthassumes thatfailedCommandsis always defined. To prevent potential runtime errors, ensure thatfailedCommandsis initialized to an empty array if there are no failed commands.Also applies to: 167-183
211-211: Confirm Node.js version compatibility forfs.rmThe
fs.rmmethod is available in Node.js v14.14.0 and above. If your project supports earlier Node.js versions, consider usingfs.rmdiror updating the minimum required Node.js version.packages/sync/src/output/prepare-generator-files/prepare-generator-file.ts (1)
1-373: Well-structured implementation with robust error handlingThe
prepare-generator-file.tsmodule effectively handles file content preparation and merging with comprehensive error handling and conflict detection. The use of utility functions enhances readability and maintainability.packages/sync/eslint.config.js (1)
6-6: Update ignore path to reflect directory changesThe updated ignore path
src/output/string-merge-algorithms/tests/**/*correctly reflects the new directory structure, ensuring ESLint ignores the intended test files.packages/sync/src/output/index.ts (1)
4-6: LGTM! Export changes align with refactoring objectives.The restructured exports properly separate types and implementations, supporting the new file writing and merge algorithm capabilities.
packages/sync/src/utils/fs.ts (2)
3-13: LGTM! Efficient implementation of path existence check.The new
pathExistsfunction usesfs.accesswhich is more performant thanfs.statfor simple existence checks.
20-24: LGTM! Good separation of concerns between path and file checks.The renamed
fileExistsfunction now clearly indicates its specific purpose of checking for file existence, improving code clarity.packages/sync/src/tests/logger.test-helper.ts (1)
13-29: LGTM! Well-structured test logger implementation.The implementation properly captures log messages and provides clear getter methods for verification in tests.
packages/sync/src/actions/write-template-action.ts (2)
8-8: LGTM! Good addition of optional ID field.The optional
idproperty allows for better file tracking while maintaining backward compatibility.
29-36: LGTM! Well-structured writeFile call with proper fallback.The new object parameter structure improves readability, and the fallback to destination for ID is a good practice.
packages/sync/src/output/prepare-generator-files/errors.ts (2)
1-8: LGTM! Clear conflict error implementation.The error message clearly indicates the conflict location and provides guidance for resolution.
10-18: LGTM! Well-structured formatter error with debugging context.Good practice to store both the original error and file contents for debugging purposes.
packages/sync/src/output/post-write-commands/filter-commands.ts (1)
12-34: LGTM! Well-structured implementation with robust error handling.The function effectively filters commands based on modified paths and rerun commands, with proper handling of edge cases:
- Safe access to optional properties using
??and optional chaining- Consistent array handling for
onlyIfChanged- Clear boolean conditions
packages/sync/src/output/post-write-commands/types.ts (2)
6-19: LGTM! Well-structured priority constants with room for expansion.The priority values (100, 200, 300) provide good spacing for potential future priority levels.
24-43: Consider adding validation for the timeout value.While the interface is well-defined, the timeout property could benefit from runtime validation to ensure it's a positive number.
Would you like me to provide an implementation for timeout validation?
packages/core-generators/src/actions/copy-typescript-files-action.ts (1)
17-17: LGTM! Improved property naming for better clarity.The rename from
neverOverwritetoshouldNeverOverwritemakes the boolean property's purpose more explicit.Also applies to: 43-43
packages/sync/src/output/post-write-commands/run-commands.ts (1)
31-41: Consider validating command strings before execution.The command string is directly executed without validation, which could pose security risks if the commands are user-provided.
Consider adding validation to ensure the command string meets security requirements.
packages/fastify-generators/src/generators/core/readme/index.ts (1)
52-59: LGTM! The structured parameters improve clarity.The new approach with
id,filePath, andoptionsparameters makes the file writing operation more explicit and configurable.packages/sync/src/output/prepare-generator-files/types.ts (1)
9-75: Well-designed interfaces for file operations!The new interfaces provide a robust foundation for file ID tracking and rename operations. The readonly modifiers and clear separation of concerns enhance type safety and maintainability.
packages/project-builder-server/src/runner/index.ts (1)
Line range hint
1-93: Verify the impact of removing clean folder functionality.The removal of
buildToCleanFolderfunctionality might affect existing consumers. Please ensure all consumers are updated accordingly.packages/core-generators/src/actions/copy-typescript-file-action.ts (2)
12-12: Consider validating the file ID format.The optional
idparameter should be validated to ensure it follows a consistent format (e.g., no special characters that could cause issues in file systems).
18-18: Great improvements to API clarity!The renaming of
neverOverwritetoshouldNeverOverwriteand the restructuredwriteFileparameters enhance readability and maintainability.Also applies to: 73-80
packages/sync/src/actions/copy-directory-action.unit.test.ts (2)
19-22: LGTM! Consistent builder initialization pattern.The refactoring of
GeneratorTaskOutputBuilderinitialization to use a structured object parameter improves clarity and maintainability.Also applies to: 39-42, 66-69
51-51: Verify the file ID format across the codebase.The new file ID format (
${generatorName}:${destinationPath}) is consistently used in the tests. Let's verify this pattern is followed throughout the codebase.Also applies to: 55-55, 79-79
✅ Verification successful
File ID format is consistent across the codebase
The format
${generatorName}:${destinationPath}is consistently used throughout the codebase for file IDs in copy operations and file writes. The test case's usage oftest:/dest/test1.txtfollows this established pattern.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for file ID assignments to ensure consistent format rg -g '*.{ts,js}' -A 1 'id:.*:.*' --no-filenameLength of output: 5618
Script:
#!/bin/bash # Search for file operations and their ID patterns ast-grep --pattern 'id: `$_:$_`' # Also search for specific file operation contexts rg -g '*.{ts,js}' -B 2 -A 2 'contents.*Buffer.from|writeFile|readFile.*id:'Length of output: 25793
packages/sync/src/output/post-write-commands/run-commands.unit.test.ts (1)
52-62: LGTM! Robust error handling test.The error handling test properly verifies both the command failure tracking and error logging.
packages/sync/src/generators/find-generator-package-name.unit.test.ts (1)
16-33: LGTM! Effective cache implementation test.The test effectively verifies both the package name resolution and cache update.
packages/sync/src/output/post-write-commands/filter-commands.unit.test.ts (1)
6-93: LGTM! Comprehensive test coverage for command filtering.The test suite thoroughly covers all aspects of command filtering:
- Commands without conditions
- Path-based filtering
- Rerun command handling
- Multiple path scenarios
- Edge cases
packages/core-generators/src/generators/docker/docker-compose/index.ts (1)
92-100: LGTM! Well-structured file writing implementation.The new structured approach with file IDs enhances maintainability and enables rename operations.
packages/sync/src/output/clean-deleted-files.ts (2)
92-98: LGTM! Good practice cleaning up empty directories.The implementation correctly handles cleanup of empty ancestor directories after file deletion.
73-89: Consider adding atomic file operations.There's a potential race condition between reading the file content and deleting it. If the file is modified between these operations, we might delete a file that was actually modified by the user.
Consider wrapping the read and delete operations in a transaction-like pattern:
- const currentContent = await fs.readFile(fullPath); - const previousGeneratedContent = - await previousGeneratedPayload.fileReader.readFile(previousRelativePath); - - if (!previousGeneratedContent) { - continue; - } - - if (currentContent.equals(previousGeneratedContent)) { - // File is identical to previous generated version, delete it - await fs.unlink(fullPath); - deletedRelativePaths.push(previousRelativePath); - } else { - // File was modified, mark for pending deletion - relativePathsPendingDelete.push(previousRelativePath); - } + try { + const [currentContent, previousGeneratedContent] = await Promise.all([ + fs.readFile(fullPath), + previousGeneratedPayload.fileReader.readFile(previousRelativePath), + ]); + + if (!previousGeneratedContent) { + continue; + } + + if (currentContent.equals(previousGeneratedContent)) { + // File is identical to previous generated version, delete it + await fs.unlink(fullPath).catch((error) => { + if (error.code !== 'ENOENT') { + throw error; + } + }); + deletedRelativePaths.push(previousRelativePath); + } else { + // File was modified, mark for pending deletion + relativePathsPendingDelete.push(previousRelativePath); + } + } catch (error) { + if (error.code !== 'ENOENT') { + throw error; + } + }knip.jsonc (1)
83-85: LGTM! Configuration updates align with project structure.The changes properly configure the new utils package workspace and update test ignore patterns to match the new directory structure.
Also applies to: 92-92
packages/sync/src/output/write-generator-file/write-generator-files.unit.test.ts (1)
93-113: LGTM! Good coverage of generated contents directory.The test properly verifies that both merged and generated contents are written to their respective directories.
packages/sync/src/output/write-generator-file/write-generator-file.unit.test.ts (4)
1-13: LGTM! Well-structured test setup.The test setup is clean and follows best practices:
- Uses
memfsfor in-memory file system operations- Properly mocks
fsmodules- Resets the virtual file system before each test
19-35: LGTM! Clear and focused test case.The test effectively validates the basic file writing functionality with proper assertions.
57-77: LGTM! Comprehensive rename test.The test effectively validates the rename operation by checking both the removal of the old file and the creation of the new file.
99-124: LGTM! Robust error handling test.The test thoroughly validates the error case by:
- Checking the error type and message
- Verifying that files remain unchanged after the error
packages/sync/src/output/write-generator-file/write-generator-file.ts (5)
8-21: LGTM! Well-documented interface.The
WriteGeneratorFileInputinterface is clearly defined with comprehensive JSDoc comments for each property.
60-69: LGTM! Clean and focused implementation.The function properly handles the optional nature of merged contents and reuses the
writeFileContentshelper.
74-81: LGTM! Consistent implementation.The function follows the same pattern as other helpers and reuses the
writeFileContentshelper.
86-95: LGTM! Well-structured implementation.The function properly handles the optional nature of conflict paths and reuses the
writeFileContentshelper.
100-140: LGTM! Well-orchestrated file operations.The function effectively coordinates all file operations in a logical sequence with clear comments explaining each step.
packages/sync/src/output/prepare-generator-files/prepare-generator-files.unit.test.ts (4)
12-34: LGTM! Well-structured test helpers.The helper functions
createMockContextandcreateMockFileDataprovide clean and reusable test data setup.
37-57: LGTM! Comprehensive success case test.The test effectively validates the successful preparation of multiple files with proper assertions.
84-92: LGTM! Good edge case coverage.The test properly validates the handling of empty input.
94-122: LGTM! Thorough options testing.The test effectively validates the handling of different file options, particularly the
shouldNeverOverwriteoption.packages/react-generators/src/generators/core/react-config/index.ts (1)
142-146: LGTM! Clean migration to new file writing format.The change properly adopts the new object format for
builder.writeFilewhile maintaining the existing functionality.packages/sync/src/utils/create-generator.unit.test.ts (1)
28-32: LGTM! Improved parameter structure forwriteFile.The refactoring to use an object parameter with named properties (
id,filePath,contents) enhances code clarity and maintainability.packages/sync/src/output/clean-deleted-files.unit.test.ts (1)
1-16: LGTM! Well-structured test setup.The test suite is properly set up with mocks and cleanup, ensuring isolated test execution.
packages/sync/src/runner/generator-runner.ts (3)
5-5: LGTM! Added duplicate detection utility.The import of
findDuplicatessupports the new file ID verification feature.
99-102: LGTM! Enhanced context for GeneratorTaskOutputBuilder.The constructor now receives a structured object with both directory and name, improving clarity and extensibility.
142-147: LGTM! Added validation to prevent duplicate file IDs.This check prevents potential conflicts and overrides by ensuring unique file IDs across the entire build output.
packages/sync/src/generators/build-generator-entry.unit.test.ts (1)
1-2: LGTM! Improved test isolation with proper mocking.The addition of fs mocking and cleanup in beforeEach ensures reliable test execution.
Also applies to: 15-18, 23-26
packages/react-generators/src/generators/core/react/index.ts (1)
136-136: LGTM! Parameter rename improves clarity.The rename from
neverOverwritetoshouldNeverOverwritefollows better naming conventions by making the boolean parameter's purpose more explicit.packages/sync/src/runner/generator-runner.unit.test.ts (3)
Line range hint
5-32: LGTM! Enhanced type imports and interface updates.The addition of
POST_WRITE_COMMAND_PRIORITYimport and the newgeneratorNameparameter inbuildGeneratorEntrysupport the file ID feature.
81-106: LGTM! Updated test case with new file writing structure.The test case now properly demonstrates the new object-based file writing API with explicit file IDs and command priorities.
119-190: LGTM! Comprehensive nested entry test updates.The nested entry test case thoroughly validates:
- File ID generation with generator name prefixes
- Command priority handling
- Options preservation (shouldFormat)
packages/sync/src/output/generator-task-output.ts (4)
16-42: LGTM! Enhanced file management with IDs and alternate IDs.The addition of
idandalternateFullIdsproperties enables robust file tracking and migration between generators. The rename ofneverOverwritetoshouldNeverOverwriteimproves clarity.
90-100: LGTM! Well-structured builder context.The new
GeneratorTaskOutputBuilderContextinterface properly encapsulates the builder's initialization parameters.
Line range hint
153-178: LGTM! Improved file writing API.The new object-based
writeFilemethod signature provides better clarity and type safety. The file ID generation using generator name prefix ensures uniqueness.
203-203: LGTM! Simplified command handling.The post-write command structure is simplified while maintaining all necessary functionality through the options object.
packages/fastify-generators/src/generators/core/config-service/index.ts (1)
182-196: LGTM! Updated env file writing to use new API.The changes properly implement the new object-based writeFile API with appropriate file IDs and options for both
.env.exampleand.envfiles.packages/fastify-generators/src/generators/prisma/prisma/index.ts (3)
14-18: LGTM! Clean import restructuring.The imports are well-organized and the addition of POST_WRITE_COMMAND_PRIORITY enhances command execution control.
179-183: LGTM! Improved file writing structure.The new object format for writeFile improves clarity and maintainability.
185-188: LGTM! Enhanced command priority handling.Good use of POST_WRITE_COMMAND_PRIORITY.CODEGEN to ensure proper execution order of the prisma generate command.
packages/sync/src/output/prepare-generator-files/prepare-generator-file.unit.test.ts (4)
40-56: LGTM! Good basic file handling test.The test properly verifies new file creation without previous versions.
58-79: LGTM! Good overwrite protection test.Properly verifies the shouldNeverOverwrite option behavior.
81-103: LGTM! Thorough conflict detection test.Good test for merge conflict markers detection.
214-248: LGTM! Comprehensive file renaming test.Good coverage of file renaming with alternate IDs.
packages/fastify-generators/src/generators/pothos/pothos/index.ts (1)
22-22: LGTM! Verify command execution order.Good addition of the
POST_WRITE_COMMAND_PRIORITY.CODEGENconstant for standardizing the schema generation priority. This aligns with the broader effort to organize command execution order.Let's verify if there are any other commands that might need to run before or after schema generation:
Also applies to: 333-334
✅ Verification successful
Command priorities are correctly ordered
The post-write commands follow a logical execution order:
- Base operations (install, formatting)
- Schema generation (prisma, pothos) at
CODEGENpriority- Code generation (react-apollo) at
CODEGEN + 1to run after schema is available🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other post-write commands to verify execution order rg -A 2 "addPostWriteCommand.*priority:" .Length of output: 44
Script:
#!/bin/bash # Search with broader patterns echo "=== Searching for addPostWriteCommand usage ===" rg "addPostWriteCommand" -A 3 echo -e "\n=== Searching for POST_WRITE_COMMAND_PRIORITY usage ===" rg "POST_WRITE_COMMAND_PRIORITY" -A 2 echo -e "\n=== Looking for builder configuration ===" fd "builder.ts|builder.js" --exec cat {}Length of output: 46560
packages/utils/tsconfig.json (1)
11-11: Verify impact of removing test exclusions.The removal of the exclude pattern will now include previously excluded test files in the compilation. While this aligns with the project restructuring, verify that this doesn't significantly impact compilation time or cause any unintended side effects.
Let's check for potential impacts:
packages/sync/tsconfig.json (1)
12-12: Verify path update consistency.The exclude pattern has been updated to reflect the new string-merge-algorithms path. This aligns with the broader refactoring effort.
Let's verify the path update is consistent across the codebase:
✅ Verification successful
Path update is valid and consistent across configurations
The string-merge-algorithms/tests directory exists at the correct location and all configuration files are properly aligned with this structure.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for any remaining references to the old path rg -g '*.{json,js,ts}' "merge-algorithms/tests" # Verify the new path exists fd -t d "string-merge-algorithms/tests"Length of output: 273
Script:
#!/bin/bash # Check if the directory exists in the correct location fd -t d "string-merge-algorithms/tests" packages/sync/src/output/ # Find any other potential configuration files that might need updates fd -e json -e js -e yaml -e yml "config|rc" packages/sync/Length of output: 252
Script:
#!/bin/bash # Check content of other config files cat packages/sync/tsconfig.build.json cat packages/sync/prettier.config.js # Look for any test files that might be related fd -t f "test" packages/sync/src/output/Length of output: 1416
.changeset/four-tigers-care.md (1)
1-10: LGTM! Version bumps and description are appropriate.The version bumps follow semantic versioning correctly - minor for new functionality in sync package and patches for adaptations in dependent packages.
turbo.json (1)
25-25: Verify if disabling typecheck cache is intentional.Disabling the cache for the typecheck task could impact build performance. Please confirm if this change is intentional and related to the file writing refactor.
packages/utils/package.json (2)
15-18: LGTM! Well-structured exports configuration.The explicit exports configuration provides better control over package entry points.
45-45: Verify memfs version compatibility.The addition of memfs for in-memory filesystem operations aligns with the refactoring goals.
✅ Verification successful
Current memfs version is stable and compatible
The installed version (4.15.1) is recent and stable. While newer versions (up to 4.17.0) are available, there are no critical issues that require immediate attention. Consider updating to the latest version in a future maintenance cycle.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for any known issues with memfs 4.15.1 npm view [email protected] bugs.url --json | jq . # Check if this is the latest compatible version npm view memfs versions --json | jq 'map(select(startswith("4."))) | last'Length of output: 176
Script:
#!/bin/bash # Fetch the release notes for relevant versions echo "Release info for 4.16.0:" npm view [email protected] time.4.16.0 description --json | jq -r . echo -e "\nRelease info for 4.17.0:" npm view [email protected] time.4.17.0 description --json | jq -r . # Check engines requirement npm view [email protected] engines --json | jq .Length of output: 408
Script:
#!/bin/bash # Fetch GitHub releases between 4.15.1 and 4.17.0 gh release list --repo streamich/memfs --limit 10 | grep -E "4.1[567]"Length of output: 382
packages/sync/src/runner/tests/factories.test-helper.ts (1)
61-61: LGTM! Enhanced flexibility in test helper.Good use of nullish coalescing to support custom generator names while maintaining backward compatibility.
packages/sync/package.json (1)
35-35: LGTM! Appropriate dependency addition.The addition of @halfdomelabs/utils as a workspace dependency aligns with the refactoring goals and ensures consistent versioning.
packages/core-generators/src/generators/node/node/index.ts (4)
5-5: LGTM! Import of POST_WRITE_COMMAND_PRIORITY enhances command execution control.The addition of this constant will help standardize post-write command priorities across the codebase.
216-219: LGTM! Added file ID for package.json.The addition of the 'package-json' ID aligns with the PR's objective to support file rename operations.
223-227: LGTM! Refactored .npmrc file writing to use the new object structure.The change follows the new pattern for file writing operations, providing better support for file rename operations through the 'npmrc' ID.
236-236: LGTM! Added explicit priority for dependency installation.Setting POST_WRITE_COMMAND_PRIORITY.DEPENDENCIES ensures that package installation occurs at the appropriate time in the build process.
| } catch { | ||
| // Ignore errors and continue searching | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling granularity.
The catch block silently ignores all errors, which might hide important issues like permission problems.
Consider distinguishing between different types of errors:
- } catch {
- // Ignore errors and continue searching
+ } catch (error) {
+ if (error.code === 'ENOENT') {
+ // File doesn't exist, continue searching
+ continue;
+ }
+ if (error.code === 'EACCES') {
+ // Permission denied, might want to log this
+ continue;
+ }
+ // Unexpected error, might want to throw
+ throw error;
}Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (9)
packages/sync/src/output/post-write-commands/sort-commands.ts (2)
5-10: Enhance JSDoc documentation with more details.While the documentation is clear, it would be helpful to add more details about:
- The priority system (number vs string priorities)
- The validation behavior for invalid priorities
- The sorting order (ascending/descending)
/** * Sort post-write commands by priority * * @param commands - The commands to sort + * @param commands[].options.priority - Optional priority (number or string from POST_WRITE_COMMAND_PRIORITY) + * @throws {Error} When a string priority is not found in POST_WRITE_COMMAND_PRIORITY * @returns The sorted commands in ascending order by priority */
11-30: Consider making the function pure and improving error handling.The implementation looks good but could be enhanced in a few ways:
export function sortPostWriteCommands( commands: PostWriteCommand[], ): PostWriteCommand[] { - const sortedCommands = sortBy(commands, [ + const sortedCommands = sortBy([...commands], [ (command) => { const priority = command.options?.priority ?? 'DEFAULT'; if ( typeof priority === 'string' && !(priority in POST_WRITE_COMMAND_PRIORITY) ) { - throw new Error(`Invalid priority: ${priority}`); + throw new Error( + `Invalid priority: "${priority}". Valid priorities are: ${Object.keys(POST_WRITE_COMMAND_PRIORITY).join(', ')}` + ); } return typeof priority === 'number' ? priority : POST_WRITE_COMMAND_PRIORITY[priority]; }, ]); return sortedCommands; }Consider extracting the priority validation to a separate function:
function validatePriority(priority: string | number): void { if (typeof priority === 'string' && !(priority in POST_WRITE_COMMAND_PRIORITY)) { throw new Error( `Invalid priority: "${priority}". Valid priorities are: ${Object.keys(POST_WRITE_COMMAND_PRIORITY).join(', ')}` ); } }packages/project-builder-server/src/sync/index.ts (6)
27-44: Remove commented out code.The commented out
renameCleanDirectoryIfExistsfunction appears to be legacy code that's no longer needed. Since the migration to the new directory structure is complete, this commented code should be removed to improve maintainability.
46-46: Update constant name to match new convention.The constant
GENERATED_DIRECTORYstill uses the old.cleanpath despite the migration to a new directory structure. Consider updating this tobaseplate/.generatedor similar to match the new conventions mentioned in the comments.
129-139: Consider extracting output writing configuration.The configuration object passed to
writeOutputcontains multiple related parameters. Consider extracting this into a dedicated interface/type for better maintainability and documentation.interface WriteOutputConfig { previousGeneratedPayload?: PreviousGeneratedPayload; generatedContentsDirectory: string; rerunCommands?: string[]; logger: Logger; }
172-189: Extract conflict handling logic to a separate function.The conflict handling logic is complex and would be more maintainable as a separate function. This would also make the main function easier to read and test.
async function handleConflicts( logger: Logger, relativePathsWithConflicts: string[], failedCommands: string[] ): Promise<void> { if (relativePathsWithConflicts.length === 0) return; logger.warn( chalk.red( `Conflicts occurred while writing files:\n${relativePathsWithConflicts.join('\n')}` ) ); if (failedCommands.length > 0) { logger.warn('\nOnce resolved, please re-run the generator or run the following commands:'); for (const command of failedCommands) { logger.warn(` ${command}`); } } }
191-198: Extract pending delete handling to a separate function.Similar to the conflict handling, the pending delete logic would be more maintainable as a separate function.
123-125: Consider extracting path constants.The path
'baseplate/build/generated_tmp'is hardcoded. Consider moving this to a constant alongside other path-related constants at the top of the file.const GENERATED_TMP_DIRECTORY = 'baseplate/build/generated_tmp';packages/sync/src/actions/copy-file-action.ts (1)
41-45: Add error handling for file operations.The file reading operation could fail for various reasons (file not found, permissions, etc.). Consider wrapping it in a try-catch block with appropriate error handling.
- const fileContents = await fs.readFile(templatePath, 'utf8'); - const replacedFileContents = applyReplacements( - fileContents, - replacements ?? {}, - ); + try { + const fileContents = await fs.readFile(templatePath, 'utf8'); + const replacedFileContents = applyReplacements( + fileContents, + replacements ?? {}, + ); + } catch (error) { + throw new Error( + `Failed to read template file ${templatePath}: ${error.message}` + ); + }
📜 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 (5)
packages/core-generators/src/generators/node/eslint/index.ts(1 hunks)packages/project-builder-server/src/sync/index.ts(6 hunks)packages/sync/src/actions/copy-file-action.ts(2 hunks)packages/sync/src/output/index.ts(1 hunks)packages/sync/src/output/post-write-commands/sort-commands.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/core-generators/src/generators/node/eslint/index.ts
- packages/sync/src/output/index.ts
🧰 Additional context used
📓 Learnings (1)
packages/project-builder-server/src/sync/index.ts (1)
Learnt from: kingston
PR: halfdomelabs/baseplate#428
File: packages/project-builder-server/src/sync/index.ts:5-5
Timestamp: 2025-01-23T09:12:46.178Z
Learning: Avoid importing directly from 'dist' directories. Instead, expose functionality through the package's public API and import from the main package entry point.
🔇 Additional comments (8)
packages/sync/src/output/post-write-commands/sort-commands.ts (1)
1-3: LGTM! Clean and focused imports.The imports are well-organized, using named imports and proper ESM file extensions.
packages/project-builder-server/src/sync/index.ts (4)
5-5: Import from package's main entry point instead of 'dist'.Based on previous feedback, we should avoid importing directly from 'dist' directories. Instead, import
createCodebaseFileReaderFromDirectoryfrom the package's main entry point.
49-67: LGTM! Well-implemented file ID map retrieval.The implementation includes proper error handling, type validation using zod, and graceful handling of missing files.
69-85: LGTM! Clean implementation of previous payload retrieval.The function is well-structured with clear error handling and proper type safety.
Line range hint
1-220: Overall implementation LGTM with minor suggestions for improvement.The file implements a robust file generation and synchronization system with proper error handling, type safety, and conflict resolution. While there are opportunities for refactoring to improve maintainability, the current implementation is solid and functional.
packages/sync/src/actions/copy-file-action.ts (3)
10-10: LGTM! Improved property naming.The rename from
neverOverwritetoshouldNeverOverwriteenhances clarity and maintains consistency with theshouldFormatnaming pattern.
27-33: LGTM! Clean destructuring pattern.The destructuring is well-organized and includes all necessary properties from the Options interface.
47-48: Verify the file ID strategy for rename operations.Using the destination path as the file ID might not be sufficient for supporting rename operations, as the ID would change when the file is renamed. Consider using a more stable identifier that persists across renames.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/utils/src/fs/find-nearest-package-json.ts (1)
31-41: 🛠️ Refactor suggestionImprove error handling granularity.
The catch block silently ignores all errors, which might hide important issues like permission problems.
🧹 Nitpick comments (7)
packages/project-builder-server/src/plugins/plugin-discovery.unit.test.ts (3)
11-18: Consider grouping related mocks together.The mocks are currently spread across multiple
vi.mock()calls. Consider grouping them into a single mock setup block for better organization and maintainability.-// Mock the required modules -vi.mock('node:fs/promises'); -vi.mock('@halfdomelabs/project-builder-lib/plugin-tools', () => ({ - loadPluginsInPackage: vi.fn(), -})); -vi.mock('node:module', () => ({ - createRequire: vi.fn(), -})); +// Mock external dependencies +vi.mock('node:fs/promises'); +vi.mock('node:module', () => ({ + createRequire: vi.fn(), +})); + +// Mock internal dependencies +vi.mock('@halfdomelabs/project-builder-lib/plugin-tools', () => ({ + loadPluginsInPackage: vi.fn(), +}));
42-99: Consider enhancing test readability and coverage.The test is well-structured but could benefit from:
- Testing multiple plugin discoveries instead of just one
- Verifying logger calls
- Using test data constants for better maintainability
+ // Test data constants + const TEST_PLUGINS = { + 'baseplate-plugin-test': { + path: '/project/node_modules/baseplate-plugin-test', + metadata: { + id: 'test-plugin', + packageName: 'baseplate-plugin-test', + // ... other metadata + }, + }, + '@org/baseplate-plugin-other': { + path: '/project/node_modules/@org/baseplate-plugin-other', + metadata: { + // ... metadata for second plugin + }, + }, + }; it('should discover plugins from package.json dependencies', async () => { // ... existing setup ... // Additional assertions + expect(mockLogger.info).toHaveBeenCalledWith( + expect.stringContaining('Discovered 1 plugin(s)') + ); });
101-149: Consider adding more error scenarios.While the current error cases are good, consider adding tests for:
- Empty dependencies in package.json
- Network errors during plugin resolution
- Invalid plugin metadata
- Circular dependencies
Example test case:
it('should handle empty dependencies gracefully', async () => { const projectDir = '/project'; vol.fromJSON({ '/project/package.json': JSON.stringify({ dependencies: {}, }), }); const result = await discoverPlugins(projectDir, mockLogger); expect(result).toEqual([]); expect(mockLogger.info).toHaveBeenCalledWith( expect.stringContaining('No plugin dependencies found') ); });packages/utils/src/fs/read-json-with-schema.ts (1)
15-20: Consider adding a size limit for file reading.To prevent potential memory issues when reading large files, consider adding a size limit check.
try { + const stats = await fs.stat(filePath); + if (stats.size > 10_000_000) { // 10MB limit + throw new TypeError(`File too large: ${filePath}`); + } const fileContent = await fs.readFile(filePath, 'utf8'); const parsedData = JSON.parse(fileContent) as unknown;packages/utils/src/fs/read-json-with-schema.unit.test.ts (1)
13-67: Consider adding edge case tests.While the current test coverage is good, consider adding tests for these edge cases:
- Empty file
- File with only whitespace
- File with BOM (Byte Order Mark)
it('should handle empty file', async () => { const filePath = '/empty.json'; vol.fromJSON({ [filePath]: '' }); await expect(readJsonWithSchema(filePath, schema)).rejects.toThrow('Invalid JSON'); }); it('should handle whitespace-only file', async () => { const filePath = '/whitespace.json'; vol.fromJSON({ [filePath]: ' \n\t ' }); await expect(readJsonWithSchema(filePath, schema)).rejects.toThrow('Invalid JSON'); }); it('should handle file with BOM', async () => { const filePath = '/bom.json'; vol.fromJSON({ [filePath]: '\uFEFF{"name": "John Doe", "age": 30}' }); const result = await readJsonWithSchema(filePath, schema); expect(result).toEqual({ name: 'John Doe', age: 30 }); });packages/project-builder-server/src/plugins/plugin-discovery.ts (1)
35-41: Consider enhancing package.json schema validation.The current schema only validates the presence of dependencies. Consider validating other required fields like
nameandversion.const packageJson = await readJsonWithSchema( packageJsonPath, z.object({ + name: z.string(), + version: z.string(), dependencies: z.record(z.string()).optional(), devDependencies: z.record(z.string()).optional(), }),packages/utils/src/fs/find-nearest-package-json.ts (1)
47-49: Consider adding debug logging.Add debug logging when stopping at node_modules to help with troubleshooting.
if (stopAtNodeModules && path.basename(currentDir) === 'node_modules') { + console.debug(`Stopping search at node_modules: ${currentDir}`); return undefined; }
📜 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 (11)
packages/project-builder-server/package.json(0 hunks)packages/project-builder-server/src/plugins/plugin-discovery.ts(4 hunks)packages/project-builder-server/src/plugins/plugin-discovery.unit.test.ts(1 hunks)packages/project-builder-server/src/server/plugin.ts(0 hunks)packages/sync/src/generators/find-generator-package-name.ts(1 hunks)packages/utils/package.json(2 hunks)packages/utils/src/fs/find-nearest-package-json.ts(1 hunks)packages/utils/src/fs/find-nearest-package-json.unit.test.ts(1 hunks)packages/utils/src/fs/index.ts(1 hunks)packages/utils/src/fs/read-json-with-schema.ts(1 hunks)packages/utils/src/fs/read-json-with-schema.unit.test.ts(1 hunks)
💤 Files with no reviewable changes (2)
- packages/project-builder-server/src/server/plugin.ts
- packages/project-builder-server/package.json
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/utils/src/fs/index.ts
- packages/sync/src/generators/find-generator-package-name.ts
- packages/utils/src/fs/find-nearest-package-json.unit.test.ts
- packages/utils/package.json
🔇 Additional comments (7)
packages/project-builder-server/src/plugins/plugin-discovery.unit.test.ts (2)
20-40: Well-structured test setup!The test suite setup is thorough with proper mock initialization and cleanup in beforeEach. Good practice using vol.reset() and vi.clearAllMocks() to ensure test isolation.
1-150: Overall excellent test coverage and structure!The test file demonstrates good testing practices with:
- Proper test isolation
- Comprehensive mocking
- Good mix of success and error scenarios
- Clear arrange-act-assert pattern
While some improvements were suggested, the overall implementation is solid and maintainable.
packages/utils/src/fs/read-json-with-schema.ts (2)
11-14: LGTM! Well-designed generic function signature.The function signature is well-designed with proper generic type constraints and clear parameter types.
20-30: LGTM! Comprehensive error handling.The error handling is well-structured:
- Distinguishes between validation and syntax errors
- Provides clear error messages
- Preserves stack traces for unknown errors
packages/utils/src/fs/read-json-with-schema.unit.test.ts (1)
1-12: LGTM! Well-structured test setup.Good use of memfs for isolation and proper cleanup in beforeEach.
packages/project-builder-server/src/plugins/plugin-discovery.ts (1)
5-11: LGTM! Clean dependency management.Good refactoring to use the new utility functions and remove fs-extra dependency.
packages/utils/src/fs/find-nearest-package-json.ts (1)
4-16: LGTM! Well-documented interface.The interface is clearly documented with proper JSDoc comments.
| const pluginPackageJsonPath = await findNearestPackageJson({ | ||
| cwd: packagePath, | ||
| stopAtNodeModules: true, | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add schema validation for plugin package.json.
Consider validating the plugin's package.json structure to ensure it contains required plugin metadata.
const pluginPackageJsonPath = await findNearestPackageJson({
cwd: packagePath,
stopAtNodeModules: true,
});
if (!pluginPackageJsonPath) {
logger.error(
`Could not find package.json file for the plugin ${packageName}.`,
);
return;
}
+const pluginSchema = z.object({
+ name: z.string(),
+ version: z.string(),
+ main: z.string(),
+ // Add any other required plugin metadata fields
+});
+
+const pluginPackageJson = await readJsonWithSchema(
+ pluginPackageJsonPath,
+ pluginSchema,
+).catch((error) => {
+ logger.error(
+ `Invalid package.json for plugin ${packageName}: ${error.message}`,
+ );
+ return;
+});
+
+if (!pluginPackageJson) return;
return loadPluginsInPackage(Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (2)
packages/project-builder-test/src/utils/version.ts (2)
Line range hint
12-16: Consider using a more robust path resolution strategyThe relative path '../project-builder-cli/package.json' is brittle and may break if the package structure changes.
Consider using a workspace-aware solution or environment variables to locate the CLI package.
Line range hint
17-19: Add error handling for JSON parsingThe JSON.parse call could throw for invalid JSON content.
- const fileContent = await fs.readFile(cliPackageJson, 'utf8'); - const packageJson = JSON.parse(fileContent) as { version: string }; - return packageJson.version; + try { + const fileContent = await fs.readFile(cliPackageJson, 'utf8'); + const packageJson = JSON.parse(fileContent) as { version: string }; + if (typeof packageJson.version !== 'string') { + throw new Error('Invalid version field in package.json'); + } + return packageJson.version; + } catch (error) { + throw new Error(`Failed to read CLI version: ${error instanceof Error ? error.message : 'Unknown error'}`); + }
🧹 Nitpick comments (21)
packages/utils/src/fs/index.ts (2)
1-7: Well-organized barrel file structure.The index file provides a clean, centralized export point for file system utilities. The organization is logical, and the explicit
.jsextensions align with ES modules best practices.However, consider adding JSDoc comments to document the purpose of this barrel file and list the available utilities for better developer experience.
+/** + * File system utility functions for handling directory and file operations. + * + * @module + * Exports: + * - dirExists: Check if directory exists + * - ensureDir: Create directory if it doesn't exist + * - fileExists: Check if file exists + * - findNearestPackageJson: Locate nearest package.json file + * - handleNotFoundError: Handle file not found errors + * - readJsonWithSchema: Read and validate JSON with schema + * - writeJson: Write JSON to file + */ export * from './dir-exists.js'; export * from './ensure-dir.js';
1-7: Consider grouping exports by functionality.While the current organization is clean, consider grouping the exports by functionality (e.g., read operations, write operations, utility functions) using comments for better code organization.
+// Directory operations export * from './dir-exists.js'; export * from './ensure-dir.js'; + +// File operations export * from './file-exists.js'; export * from './find-nearest-package-json.js'; export * from './handle-not-found-error.js'; + +// JSON operations export * from './read-json-with-schema.js'; export * from './write-json.js';packages/utils/src/fs/ensure-dir.ts (2)
7-9: Add error handling for common edge cases.While the implementation is correct, consider adding error handling for common scenarios:
- Permission denied errors
- Path contains invalid characters
- Path points to an existing file instead of directory
export async function ensureDir(directoryPath: string): Promise<void> { - await mkdir(directoryPath, { recursive: true }); + try { + await mkdir(directoryPath, { recursive: true }); + } catch (error) { + if (error instanceof Error) { + throw new Error(`Failed to ensure directory exists at ${directoryPath}: ${error.message}`); + } + throw error; + } }
3-6: Enhance documentation with examples and error cases.Consider adding usage examples and possible error scenarios to the JSDoc documentation.
/** * Ensures a directory exists, creating it recursively if it doesn't * @param directoryPath - The path to the directory + * @throws {Error} When directory creation fails due to permissions or invalid path + * @example + * ```ts + * // Create a nested directory structure + * await ensureDir('./path/to/nested/directory'); + * ``` */packages/utils/src/fs/dir-exists.ts (2)
8-12: Improve parameter naming consistency.The parameter name
filePathsuggests a file path, but the function checks for directory existence. Consider renaming for clarity.-export async function dirExists(filePath: string): Promise<boolean> { - return stat(filePath) +export async function dirExists(dirPath: string): Promise<boolean> { + return stat(dirPath) .then((file) => file.isDirectory()) .catch(() => false); }
3-7: Enhance documentation with specific error cases.The documentation should clarify what specific conditions lead to a false return value.
/** * Checks if a directory exists - * @param filePath - The path to the directory + * @param dirPath - The path to the directory * @returns True if the directory exists, false otherwise + * @remarks Returns false in the following cases: + * - Path doesn't exist + * - Path exists but is not a directory + * - Permission errors when accessing the path */packages/utils/src/fs/write-json.ts (1)
3-7: Enhance documentation with examples and error cases.Add examples and document potential error scenarios in the JSDoc.
/** * Writes a JSON file with 2 spaces indentation. * @param filePath - The path to the file. * @param data - The data to write to the file. + * @throws {Error} When data cannot be serialized to JSON + * @throws {Error} When file writing fails + * @example + * ```ts + * await writeJson('./config.json', { key: 'value' }); + * ``` */packages/utils/src/fs/file-exists.ts (2)
8-12: Consider logging specific error types.While silently returning false is appropriate for the API, consider logging specific error types for debugging purposes.
export async function fileExists(filePath: string): Promise<boolean> { return stat(filePath) .then((file) => file.isFile()) - .catch(() => false); + .catch((error) => { + if (error.code !== 'ENOENT') { + console.debug(`Unexpected error checking file existence: ${error.message}`); + } + return false; + }); }
3-7: Enhance documentation with specific error cases.The documentation should clarify what specific conditions lead to a false return value.
/** * Checks if a file exists and is a file * @param filePath - The path to the file * @returns True if the file exists and is a file, false otherwise + * @remarks Returns false in the following cases: + * - Path doesn't exist + * - Path exists but is not a regular file + * - Permission errors when accessing the path */packages/utils/src/fs/handle-not-found-error.ts (1)
8-16: Consider using type predicate for better type safetyThe current implementation uses type casting. Consider using a type predicate for better type safety and maintainability.
-export function handleFileNotFoundError(error: unknown): undefined { +export function handleFileNotFoundError(error: unknown): undefined { + function isErrnoException(error: unknown): error is NodeJS.ErrnoException { + return error instanceof Error && 'code' in error; + } + if ( - error instanceof Error && - (error as NodeJS.ErrnoException).code === 'ENOENT' + isErrnoException(error) && error.code === 'ENOENT' ) { return undefined; } throw error; }packages/utils/src/fs/ensure-dir.unit.test.ts (1)
12-28: Add test cases for error scenarios and nested directoriesThe test suite should include additional scenarios:
- Nested directory creation
- Permission errors
- Path validation
Example test case for nested directories:
it('should create nested directories recursively', async () => { const nestedDir = '/tmp/parent/child/grandchild'; await ensureDir(nestedDir); expect(vol.existsSync(nestedDir)).toBe(true); }); it('should handle invalid paths', async () => { const invalidPath = ''; await expect(ensureDir(invalidPath)) .rejects .toThrow(); });packages/project-builder-cli/src/utils/version.ts (1)
1-1: Consider consolidating duplicate version retrieval logic.This implementation is identical to
packages/create-project/src/version.ts. Consider moving this common functionality to a shared utility to avoid duplication.-import { findNearestPackageJson } from '@halfdomelabs/utils/node'; -import { promises as fs } from 'node:fs'; -import { fileURLToPath } from 'node:url'; +import { getPackageVersion } from '@halfdomelabs/utils/node'; -let cachedVersion: string | undefined | null; - -export async function getPackageVersion(): Promise<string | null> { - if (cachedVersion === undefined) { - // Construct the path to the package.json file. - const packageJsonPath = await findNearestPackageJson({ - cwd: fileURLToPath(import.meta.url), - }); - - if (packageJsonPath) { - // Read the package.json file. - const fileContent = await fs.readFile(packageJsonPath, 'utf8'); - const packageJson = JSON.parse(fileContent) as { version: string }; - - // Return the version. - cachedVersion = packageJson.version || null; - } else { - cachedVersion = null; - } - } - - return cachedVersion; -} +export { getPackageVersion };Also applies to: 10-12
packages/project-builder-test/src/utils/directories.ts (1)
Line range hint
5-11: Improve error message clarity.The error message "Could not find package root" could be more descriptive by including the search path.
- throw new Error('Could not find package root'); + throw new Error(`Could not find package.json in or above ${import.meta.dirname}`);packages/utils/src/fs/handle-not-found-error.unit.test.ts (1)
12-13: Document the reason for ESLint disable.The ESLint disable comment should include a more detailed explanation of why the rule needs to be disabled.
- // eslint-disable-next-line @typescript-eslint/no-confusing-void-expression -- we want to test the return value + // eslint-disable-next-line @typescript-eslint/no-confusing-void-expression -- handleFileNotFoundError returns undefined for ENOENT errors, which we need to explicitly testpackages/project-builder-server/src/service/builder-service.ts (2)
128-145: Consider adding error handling for file operations.While the initialization logic is sound, it would benefit from explicit error handling for the file write operation.
- await writeFile( - this.projectJsonPath, - JSON.stringify({ - name: starterName, - cliVersion: this.cliVersion, - portOffset: 5000, - schemaVersion: getLatestMigrationVersion(), - } satisfies ProjectDefinitionInput), - ); + try { + await writeFile( + this.projectJsonPath, + JSON.stringify({ + name: starterName, + cliVersion: this.cliVersion, + portOffset: 5000, + schemaVersion: getLatestMigrationVersion(), + } satisfies ProjectDefinitionInput), + ); + } catch (error) { + this.logger.error(`Failed to create project-definition.json: ${error}`); + throw error; + }
182-190: Consider adding JSON validation before write.The writeConfig method should validate that the contents are valid JSON before writing to prevent corruption.
+ try { + JSON.parse(payload.contents); // Validate JSON + } catch (error) { + throw new Error('Invalid JSON content provided'); + } await writeFile(this.projectJsonPath, payload.contents, 'utf8');packages/project-builder-server/src/sync/index.ts (5)
35-52: Remove commented-out code.The commented-out
renameCleanDirectoryIfExistsfunction appears to be legacy code that's no longer needed. Consider removing it to improve code maintainability.
54-54: Update directory name to match new convention.The
GENERATED_DIRECTORYconstant still uses the old.cleanpath despite the codebase moving away from this naming convention. Consider updating it to a more appropriate name likebaseplate/.generatedfor consistency.
71-74: Enhance error message clarity.The error message could be more descriptive by including the actual error code:
- `Failed to get previous generated file id map (${generatedFileIdMapPath}): ${String(err)}`, + `Failed to get previous generated file id map (${generatedFileIdMapPath}): [${(err as {code?: string}).code || 'UNKNOWN'}] ${String(err)}`,
222-224: Improve error handling in cleanup.The current implementation silently ignores cleanup errors. Consider logging these errors at debug level:
- await rm(generatedTemporaryDirectory, { recursive: true }).catch(() => { - /* ignore errors */ - }); + await rm(generatedTemporaryDirectory, { recursive: true }).catch((err) => { + logger.debug(`Failed to clean up temporary directory: ${err.message}`); + });
177-194: Structure conflict messages for better readability.Consider formatting the conflict messages in a more structured way:
- `Conflicts occurred while writing files:\n${relativePathsWithConflicts.join( - '\n', - )}`, + 'Conflicts occurred while writing files:\n' + + relativePathsWithConflicts + .map((path) => ` - ${path}`) + .join('\n'),
📜 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 (27)
packages/core-generators/package.json(1 hunks)packages/core-generators/src/actions/copy-typescript-file-action.ts(4 hunks)packages/core-generators/src/generators/node/prettier/index.ts(3 hunks)packages/create-project/package.json(1 hunks)packages/create-project/src/version.ts(1 hunks)packages/project-builder-cli/package.json(1 hunks)packages/project-builder-cli/src/utils/version.ts(1 hunks)packages/project-builder-server/package.json(0 hunks)packages/project-builder-server/src/runner/index.ts(1 hunks)packages/project-builder-server/src/service/builder-service.ts(4 hunks)packages/project-builder-server/src/sync/index.ts(5 hunks)packages/project-builder-test/package.json(1 hunks)packages/project-builder-test/src/utils/directories.ts(2 hunks)packages/project-builder-test/src/utils/version.ts(1 hunks)packages/project-builder-web/baseplate/project.json(0 hunks)packages/utils/src/fs/dir-exists.ts(1 hunks)packages/utils/src/fs/dir-exists.unit.test.ts(1 hunks)packages/utils/src/fs/ensure-dir.ts(1 hunks)packages/utils/src/fs/ensure-dir.unit.test.ts(1 hunks)packages/utils/src/fs/file-exists.ts(1 hunks)packages/utils/src/fs/file-exists.unit.test.ts(1 hunks)packages/utils/src/fs/handle-not-found-error.ts(1 hunks)packages/utils/src/fs/handle-not-found-error.unit.test.ts(1 hunks)packages/utils/src/fs/index.ts(1 hunks)packages/utils/src/fs/write-json.ts(1 hunks)packages/utils/src/fs/write-json.unit.test.ts(1 hunks)turbo.json(2 hunks)
💤 Files with no reviewable changes (2)
- packages/project-builder-server/package.json
- packages/project-builder-web/baseplate/project.json
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/core-generators/src/generators/node/prettier/index.ts
- packages/project-builder-server/src/runner/index.ts
🧰 Additional context used
📓 Learnings (1)
packages/project-builder-server/src/sync/index.ts (1)
Learnt from: kingston
PR: halfdomelabs/baseplate#428
File: packages/project-builder-server/src/sync/index.ts:5-5
Timestamp: 2025-01-23T09:12:46.178Z
Learning: Avoid importing directly from 'dist' directories. Instead, expose functionality through the package's public API and import from the main package entry point.
🔇 Additional comments (22)
packages/utils/src/fs/index.ts (1)
1-7: Verify the presence and implementation of all referenced modules.Let's ensure all the referenced modules exist and are properly implemented.
✅ Verification successful
All referenced modules are present and properly implemented
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the existence and implementation of all referenced modules # Check if all referenced files exist echo "Checking for existence of all referenced files..." for file in dir-exists ensure-dir file-exists find-nearest-package-json handle-not-found-error read-json-with-schema write-json; do if ! fd -e ts "^${file}.ts$" "packages/utils/src/fs"; then echo "Warning: ${file}.ts not found" fi done # Check implementation of key functions echo -e "\nChecking implementation of key functions..." ast-grep --pattern 'export function $func($_) { $$$ }'Length of output: 1867
Script:
#!/bin/bash # Check exports and implementations in each file echo "Checking dir-exists.ts..." ast-grep --pattern 'export function dirExists($_) { $$$ }' packages/utils/src/fs/dir-exists.ts echo -e "\nChecking ensure-dir.ts..." ast-grep --pattern 'export function ensureDir($_) { $$$ }' packages/utils/src/fs/ensure-dir.ts echo -e "\nChecking file-exists.ts..." ast-grep --pattern 'export function fileExists($_) { $$$ }' packages/utils/src/fs/file-exists.ts echo -e "\nChecking find-nearest-package-json.ts..." ast-grep --pattern 'export function findNearestPackageJson($_) { $$$ }' packages/utils/src/fs/find-nearest-package-json.ts echo -e "\nChecking handle-not-found-error.ts..." ast-grep --pattern 'export function handleNotFoundError($_) { $$$ }' packages/utils/src/fs/handle-not-found-error.ts echo -e "\nChecking read-json-with-schema.ts..." ast-grep --pattern 'export function readJsonWithSchema($_) { $$$ }' packages/utils/src/fs/read-json-with-schema.ts echo -e "\nChecking write-json.ts..." ast-grep --pattern 'export function writeJson($_) { $$$ }' packages/utils/src/fs/write-json.tsLength of output: 1305
Script:
#!/bin/bash # Check for any kind of exports in each file for file in dir-exists ensure-dir file-exists find-nearest-package-json handle-not-found-error read-json-with-schema write-json; do echo "=== Checking ${file}.ts ===" echo "File content:" rg --no-line-number "export" "packages/utils/src/fs/${file}.ts" echo -e "\nAST pattern matching:" ast-grep --pattern 'export $_' "packages/utils/src/fs/${file}.ts" echo -e "\n" doneLength of output: 3715
packages/create-project/src/version.ts (1)
1-1: LGTM! Good refactoring to use internal utility.The replacement of
packageUpwithfindNearestPackageJsonfrom internal utils is a good move for better maintainability and consistency across the codebase.Also applies to: 10-12
packages/project-builder-test/src/utils/directories.ts (1)
Line range hint
15-26: Verify path traversal logic and improve error handling.The nested package root lookup using path.dirname twice could be fragile. Consider:
- Adding path validation
- Making error messages more descriptive
- Using a more robust way to find the workspace root
✅ Verification successful
Path traversal implementation is robust and reliable
The current implementation uses a well-tested
findNearestPackageJsonutility that properly handles edge cases and is consistently used across the codebase. The error messages clearly identify which lookup failed, and the two-level directory traversal correctly matches the monorepo structure where packages are located atpackages/*/package.json.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the directory structure assumptions # Test: Check if the expected directory structure exists echo "Checking directory structure..." fd -t d "tests$" -x echo "Found tests directory: {}" fd -t d "src/tests$" -x echo "Found src/tests directory: {}" # Test: Check for package.json files and their relative positions echo -e "\nChecking package.json locations..." fd "package.json$" -x echo "Found package.json: {}"Length of output: 2030
Script:
#!/bin/bash # Check the implementation of findNearestPackageJson ast-grep --pattern 'function findNearestPackageJson' -A 10 rg "findNearestPackageJson" -A 5Length of output: 13302
packages/utils/src/fs/handle-not-found-error.unit.test.ts (1)
1-38: LGTM! Well-structured tests with good coverage.The test suite is well-organized using the Arrange-Act-Assert pattern and covers all important cases:
- ENOENT errors returning undefined
- Non-ENOENT errors being rethrown
- Invalid error objects being handled
packages/utils/src/fs/dir-exists.unit.test.ts (3)
1-7: LGTM! Good test setup with proper mocking.The test setup is well-structured with appropriate imports and mocking of the fs/promises module using memfs for isolation.
8-10: LGTM! Clean test state management.Good practice using beforeEach to reset the virtual filesystem before each test.
12-51: LGTM! Comprehensive test coverage.The test suite effectively covers all key scenarios:
- Existing directory
- Non-existent path
- Path pointing to a file
Well-structured tests following the Arrange-Act-Assert pattern with clear comments.
packages/utils/src/fs/file-exists.unit.test.ts (2)
1-7: LGTM! Consistent test setup with dir-exists tests.The test setup maintains consistency with the dir-exists tests, using the same testing infrastructure and mocking approach.
12-51: LGTM! Comprehensive test coverage with parallel structure.The test suite maintains symmetry with dir-exists tests while testing the inverse scenarios:
- Existing file
- Non-existent file
- Path pointing to a directory
Well-organized tests with clear separation of concerns.
packages/project-builder-server/src/service/builder-service.ts (2)
11-11: LGTM! Good modernization of imports.Clean transition from fs-extra to native fs/promises, with proper organization of imports.
Also applies to: 14-14
171-176: LGTM! Efficient parallel file operations.Good use of Promise.all for concurrent file operations when reading config.
turbo.json (2)
7-8: LGTM! Clear build output configuration.Explicit output paths and errors-only logging will help keep the build logs focused and relevant.
19-19: Verify impact of cache changes on build performance.The changes to disable caching for typecheck and modify test output logging might affect build performance.
Also applies to: 26-26
✅ Verification successful
Cache and logging configurations are intentionally optimized
The current configuration follows best practices:
- Disabling cache for
typecheckensures reliable type checking- "errors-only" logging for tests reduces I/O overhead while preserving error visibility
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for existing performance metrics rg -A 5 "typecheck.*duration|test.*duration" .turbo/turbo-*.logLength of output: 126
Script:
#!/bin/bash # Check git history for turbo.json changes git log -p turbo.json | head -n 50 # Look for performance-related documentation fd -t f "README|CONTRIBUTING|PERFORMANCE" -E "node_modules" --exec cat {} # Show current turbo.json content cat turbo.jsonLength of output: 13341
packages/create-project/package.json (1)
28-34: LGTM: Dependency changes align with the refactoring goals.The replacement of
package-upwith@halfdomelabs/utilsis consistent with the broader refactoring effort to consolidate utility functions internally.packages/project-builder-test/package.json (1)
34-34: LGTM: Consistent dependency update.The addition of
@halfdomelabs/utilsaligns with the dependency changes across other packages.packages/project-builder-cli/package.json (1)
40-40: LGTM: Consistent dependency management.The dependency changes follow the same pattern as other packages, maintaining consistency across the project.
packages/core-generators/package.json (1)
57-57: Verify replacement of fs-extra functionality.The removal of
fs-extrasuggests a switch to native Node.jsfs/promises. Please ensure all fs-extra functionality has been properly migrated.✅ Verification successful
✅ fs-extra successfully replaced with native fs/promises
The codebase has been fully migrated to use the native Node.js
fs/promisesAPI with no remainingfs-extradependencies.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining fs-extra usage rg "fs-extra" --type ts # Search for native fs usage rg "node:fs/promises" --type tsLength of output: 3878
packages/core-generators/src/actions/copy-typescript-file-action.ts (4)
11-13: LGTM: Added file ID support for rename operations.The addition of the optional
idfield toCopyTypescriptFileOptionsenables tracking files for rename operations, which is a key objective of this PR.
17-19: LGTM: Improved naming clarity.Renaming
neverOverwritetoshouldNeverOverwritemakes the boolean flag's purpose more explicit.
73-80: LGTM: Enhanced file writing interface.The refactored
writeFilecall with a structured object parameter improves code organization and maintainability. The separation of core parameters (id,filePath,contents) from options (shouldFormat,shouldNeverOverwrite) is clean and extensible.
2-2: LGTM: Using native fs/promises API.The switch to
node:fs/promisesfrom fs-extra aligns with modern Node.js practices and reduces external dependencies.packages/project-builder-server/src/sync/index.ts (1)
2-27: LGTM! Clean imports and well-structured type definitions.The code follows best practices:
- Properly imports from package entry points
- Uses native Node.js promises-based fs operations
- Implements strong typing with zod schema
| export async function writeJson( | ||
| filePath: string, | ||
| data: unknown, | ||
| ): Promise<void> { | ||
| await writeFile(filePath, `${JSON.stringify(data, null, 2)}\n`); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling and type validation.
The current implementation lacks error handling for JSON serialization and file writing operations. Also, consider adding type safety for the data parameter.
+import { z } from 'zod';
+
+/**
+ * Validates that a value can be serialized to JSON
+ */
+const JsonSerializable = z.unknown().refine(
+ (data) => {
+ try {
+ JSON.stringify(data);
+ return true;
+ } catch {
+ return false;
+ }
+ },
+ { message: 'Value cannot be serialized to JSON' }
+);
+
export async function writeJson(
filePath: string,
- data: unknown,
+ data: z.infer<typeof JsonSerializable>,
): Promise<void> {
- await writeFile(filePath, `${JSON.stringify(data, null, 2)}\n`);
+ try {
+ const jsonString = JSON.stringify(data, null, 2);
+ await writeFile(filePath, `${jsonString}\n`);
+ } catch (error) {
+ if (error instanceof Error) {
+ throw new Error(`Failed to write JSON to ${filePath}: ${error.message}`);
+ }
+ throw error;
+ }
}📝 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 async function writeJson( | |
| filePath: string, | |
| data: unknown, | |
| ): Promise<void> { | |
| await writeFile(filePath, `${JSON.stringify(data, null, 2)}\n`); | |
| } | |
| import { z } from 'zod'; | |
| /** | |
| * Validates that a value can be serialized to JSON | |
| */ | |
| const JsonSerializable = z.unknown().refine( | |
| (data) => { | |
| try { | |
| JSON.stringify(data); | |
| return true; | |
| } catch { | |
| return false; | |
| } | |
| }, | |
| { message: 'Value cannot be serialized to JSON' } | |
| ); | |
| export async function writeJson( | |
| filePath: string, | |
| data: z.infer<typeof JsonSerializable>, | |
| ): Promise<void> { | |
| try { | |
| const jsonString = JSON.stringify(data, null, 2); | |
| await writeFile(filePath, `${jsonString}\n`); | |
| } catch (error) { | |
| if (error instanceof Error) { | |
| throw new Error(`Failed to write JSON to ${filePath}: ${error.message}`); | |
| } | |
| throw error; | |
| } | |
| } |
| describe('writeJson', () => { | ||
| it('should write a valid JSON file', async () => { | ||
| // Arrange | ||
| const filePath = '/valid.json'; | ||
| const data = { name: 'John Doe', age: 30 }; | ||
|
|
||
| // Act | ||
| await writeJson(filePath, data); | ||
|
|
||
| // Assert | ||
| expect(vol.readFileSync(filePath, 'utf8')).toEqual( | ||
| `${JSON.stringify(data, null, 2)}\n`, | ||
| ); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance test coverage with additional test cases
The current test suite only covers the happy path. Consider adding the following test cases:
- Invalid JSON data
- File system permission errors
- Directory doesn't exist
- Empty data
Example test case for invalid data:
it('should throw error for circular references', async () => {
// Arrange
const filePath = '/invalid.json';
const circular: any = { };
circular.self = circular;
// Act & Assert
await expect(writeJson(filePath, circular))
.rejects
.toThrow('Converting circular structure to JSON');
});
kingston
deleted the
kingston/eng-384-support-renaming-files-and-merging-changes-in
branch
Summary by CodeRabbit
Based on the comprehensive summary of changes, here are the concise release notes:
New Features
Library Updates
Refactoring
Performance