feat: Delete empty folders when deleting files when syncing #405
Conversation
ENG-516 Delete empty folders when syncing
If we're renaming files or removing folders, we should delete empty folders. |
🦋 Changeset detectedLatest commit: c965caa The changes in this PR will be included in the next version bump. This PR includes changesets to release 12 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including You can disable this status message by setting the WalkthroughThe pull request introduces functionality to automatically delete empty folders when files are removed from the codebase. This is achieved by adding new utility functions for directory management, specifically Changes
Sequence DiagramsequenceDiagram
participant Sync as File Synchronization
participant DirUtils as Directory Utilities
participant FileSystem as File System
Sync->>DirUtils: Call removeEmptyAncestorDirectories
DirUtils->>FileSystem: Check directory contents
FileSystem-->>DirUtils: Return directory status
alt Directory is Empty
DirUtils->>FileSystem: Remove empty directory
DirUtils->>DirUtils: Check parent directory
else Directory is Not Empty
DirUtils->>Sync: Stop removal process
end
The sequence diagram illustrates the process of removing empty ancestor directories, showing how the synchronization process checks and removes empty directories recursively. 🪧 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: 3
🧹 Nitpick comments (4)
packages/project-builder-server/src/utils/directories.unit.test.ts (2)
14-16: Consider a top-level reset check.
vol.reset()is a solid approach. Optionally, confirm if you need to reset additional mock states or loggers at the top level inbeforeAllorafterAllto handle project-level test side effects.
135-154: Ensure error handling is fully tested.The test at line 134+ checks for error handling by mocking
fs.readdir, but consider additional scenarios such as partial directory permissions or read-only file systems to broaden coverage..changeset/odd-drinks-teach.md (1)
Line range hint
1-8: Enhance the changeset description with more details.While the current description is clear, it would be helpful to add more context about:
- The motivation behind this change
- The specific packages affected and their roles
- Any potential impact on existing behavior
Consider expanding the description like this:
-Delete empty folders when deleting files from codebase +Delete empty folders when deleting files from codebase + +When files are deleted during sync operations, their parent directories will now be automatically removed if they become empty. This change: +- Keeps the codebase clean by removing unused directories +- Affects file operations in @halfdomelabs/project-builder-server +- Updates sync behavior in @halfdomelabs/sync +- Maintains consistency in @halfdomelabs/fastify-generatorspackages/project-builder-server/package.json (1)
30-30: Consider enhancing the test script configuration.The current test script is minimal. Consider adding commonly used options for better developer experience.
- "test": "vitest", + "test": "vitest run", + "test:watch": "vitest", + "test:coverage": "vitest run --coverage",
📜 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 (9)
.changeset/odd-drinks-teach.md(1 hunks)packages/fastify-generators/src/generators/core/app-module/index.ts(2 hunks)packages/project-builder-server/package.json(2 hunks)packages/project-builder-server/src/__mocks__/fs.cts(1 hunks)packages/project-builder-server/src/__mocks__/fs/promises.cts(1 hunks)packages/project-builder-server/src/sync/index.ts(2 hunks)packages/project-builder-server/src/utils/directories.ts(1 hunks)packages/project-builder-server/src/utils/directories.unit.test.ts(1 hunks)packages/sync/package.json(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- packages/project-builder-server/src/mocks/fs.cts
- packages/project-builder-server/src/mocks/fs/promises.cts
🔇 Additional comments (6)
packages/project-builder-server/src/utils/directories.ts (1)
4-11: Implement thorough path validation.
Consider validating that dirPath actually points to a directory rather than relying solely on error catching. Suppose the path points to a file or an invalid symlink. The current approach returns false on error, which might mask deeper issues.
packages/fastify-generators/src/generators/core/app-module/index.ts (2)
11-11: Remove unused imports when possible.
If no other code references kebabCase, you might consider removing the import to keep the imports consistent with usage.
35-35: Clarify naming convention.
Using camelCase for the descriptor name and then appending "Module" is logical, but ensure it aligns with any existing naming conventions in the rest of the project for consistency.
packages/project-builder-server/src/utils/directories.unit.test.ts (1)
1-3: Confirm test environment isolation.
In-memory filesystem usage is great for isolation. Ensure any global mocks (like node:fs) are restored properly after all tests to avoid potential side effects in parallel test runs.
packages/project-builder-server/src/sync/index.ts (1)
11-12: Confirm consistent import paths.
Ensure @src/utils/directories.js is consistently referenced across the codebase and no duplicates are introduced (e.g., relative vs. alias paths).
packages/sync/package.json (1)
58-58: Verify memfs version compatibility.
The memfs version has been updated from 4.11.2 to 4.15.1. This is a significant version bump that could include breaking changes.
Let's verify the changes between these versions:
✅ Verification successful
memfs version update is safe to proceed
The version bump from 4.11.2 to 4.15.1 includes several improvements and bug fixes:
- Bug fix for relative symlinks resolution
- New features for FileHandle streams
- Improved permissions support
- No breaking changes reported in the changelog
The verification also shows that memfs is consistently used at version 4.15.1 across packages, maintaining version alignment in the monorepo.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check memfs changelog and verify version consistency across packages
# Check if memfs is used consistently across packages
echo "Checking memfs version consistency across packages..."
fd -t f "package.json" -x grep -l "memfs" {} \; | xargs cat | grep "memfs"
# Fetch memfs releases between versions
gh api repos/streamich/memfs/releases --jq '.[] | select(.tag_name | contains("v4.")) | select(.tag_name >= "v4.11.2" and .tag_name <= "v4.15.1") | {version: .tag_name, body: .body}'
Length of output: 3114
| // Process each directory | ||
| for (const dir of uniqueParentDirs) { | ||
| let currentDir = dir; | ||
|
|
||
| // Continue until we hit the stop directory or root | ||
| while ( | ||
| currentDir !== stopAt && | ||
| currentDir !== path.parse(currentDir).root && | ||
| !parsedDirs.has(currentDir) | ||
| ) { | ||
| parsedDirs.add(currentDir); | ||
| try { | ||
| if (await isDirectoryEmpty(currentDir)) { | ||
| await fs.rmdir(currentDir); | ||
| currentDir = path.dirname(currentDir); | ||
| } else { | ||
| break; // Stop if directory is not empty | ||
| } | ||
| } catch { | ||
| break; // Stop on any error (e.g., permissions, non-existent directory) | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Ensure no partial directory removals on transient failures.
When catching errors at line 41, any partially removed directories remain unreverted. Consider adding a rollback mechanism or differentiating which errors can be retried (e.g., temporary permission issues). This avoids leaving half-removed directory states.
| if (deletedCleanFiles.length > 0) { | ||
| // clean up empty directories | ||
| await removeEmptyAncestorDirectories( | ||
| deletedCleanFiles.map((f) => path.join(projectDirectory, f.filePath)), | ||
| projectDirectory, | ||
| ); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid repeated directory removal calls in loops.
Whenever multiple files in the same directory are deleted, repeated calls to removeEmptyAncestorDirectories might be unnecessary if the function is not idempotent or if it triggers duplicative checks. Consider batching the directory removal in the calling function (which you are partly doing by passing the entire array).
| "inflection": "3.0.0", | ||
| "json-stringify-pretty-compact": "^4.0.0", | ||
| "lodash": "^4.17.21", | ||
| "memfs": "4.15.1", |
There was a problem hiding this comment.
💡 Codebase verification
Move memfs to devDependencies as it's only used in tests
Based on the search results, memfs is exclusively used in test files (.test.ts) and mock files (__mocks__ directory). Specifically:
- Used in test files for volume manipulation:
loader.unit.test.ts,copy-directory-action.unit.test.ts,generator-output-writer.unit.test.ts, anddirectories.unit.test.ts - Used in mock files to mock the filesystem:
fs.ts,fs.cts,promises.ts, andpromises.cts
There are no usages in production code, so this package should be moved to devDependencies.
🔗 Analysis chain
Verify if memfs should be a runtime dependency.
memfs is typically used for testing purposes, but it's added as a runtime dependency. This might indicate:
- It's being used in production code for in-memory operations, or
- It should be moved to devDependencies if it's only used for testing.
Let's verify its usage:
Let me try a different approach to verify the usage of memfs.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check how memfs is being used in the codebase
# Look for memfs imports in source files
echo "Checking memfs usage in source code..."
rg -l "memfs" --type ts --type tsx --glob '!**/*.test.{ts,tsx}' --glob '!**/__mocks__/**'
# Look for memfs imports in test files
echo "Checking memfs usage in test files..."
rg -l "memfs" --type ts --type tsx --glob '**/*.test.{ts,tsx}' --glob '**/__mocks__/**'
Length of output: 405
Script:
#!/bin/bash
# Search for memfs usage in all files, excluding node_modules
echo "Checking memfs usage in source code..."
rg -l "memfs" --type-add 'ts:*.{ts,tsx}' --type ts --glob '!**/*.test.{ts,tsx}' --glob '!**/__mocks__/**' --glob '!**/node_modules/**'
echo -e "\nChecking memfs usage in test files..."
rg -l "memfs" --type-add 'ts:*.{ts,tsx}' --type ts --glob '**/*.test.{ts,tsx}' --glob '**/__mocks__/**' --glob '!**/node_modules/**'
# Let's also check the actual usage context
echo -e "\nChecking the context of memfs usage..."
rg "memfs" --type-add 'ts:*.{ts,tsx}' --type ts --glob '!**/node_modules/**' -C 2
Length of output: 2416
kingston
deleted the
kingston/eng-516-delete-empty-folders-when-syncing
branch
Summary by CodeRabbit
Release Notes
New Features
Enhancements
Bug Fixes
Tests
Chores