Update CLI cleanLogs to use rmdir #2810
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2810 +/- ##
==========================================
+ Coverage 71.83% 71.85% +0.02%
==========================================
Files 134 134
Lines 7331 7340 +9
Branches 1622 1539 -83
==========================================
+ Hits 5266 5274 +8
- Misses 1937 2020 +83
+ Partials 128 46 -82 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR changes how the CLI cleans up the _markbind directory after a failed build/serve so that it uses non-recursive directory removal and preserves directory structure when files already exist, and extends unit tests to cover these scenarios.
Changes:
- Updated
cleanupFailedMarkbindBuildto delete_markbind/logsand_markbindusingfs.rmdirSync, ignoring errors and only removing empty directories. - Added tests to verify that
_markbind/logsand_markbindare preserved when they contain files, and that an emptylogsdirectory is removed while a non-empty_markbindis kept.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| packages/cli/src/util/cliUtil.js | Adjusts cleanupFailedMarkbindBuild to attempt non-recursive deletion of _markbind/logs and then _markbind, thereby only removing them when empty. |
| packages/cli/test/unit/cliUtil.test.js | Extends unit tests to cover preservation of _markbind and logs when they contain files, and removal of an empty logs directory while keeping a non-empty _markbind. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| cleanupFailedMarkbindBuild: () => { | ||
| const markbindDir = path.join(process.cwd(), '_markbind'); | ||
| if (fs.pathExistsSync(markbindDir)) { | ||
| // delete _markbind/ folder and contents | ||
| fs.rmSync(markbindDir, { recursive: true, force: true }); | ||
| } | ||
| const logsDir = path.join(markbindDir, 'logs'); | ||
|
|
||
| tryDeleteFolder(logsDir); | ||
| tryDeleteFolder(markbindDir); |
There was a problem hiding this comment.
With the new non-recursive tryDeleteFolder implementation, cleanupFailedMarkbindBuild will never successfully delete a non-empty _markbind directory: fs.rmdirSync(markbindDir) will always throw ENOTEMPTY when _markbind contains any files or subdirectories, and that error is intentionally swallowed. This means that in common failure scenarios where _markbind already has contents (not just logs), no cleanup actually happens, which contradicts the intent of this helper and the existing tests. Consider either retaining recursive deletion when you know _markbind was created by this failed run, or otherwise tightening the heuristic (for example, only attempting to clean when the directory tree is known/verified to contain only MarkBind-created artifacts) so that we still remove transient build artifacts while preserving pre-existing user content.
There was a problem hiding this comment.
This is intended behavior. I don't think there's quite a mechanism for us to find out whether the folder was just-created in this run? If there is LMK. But I think for now this is OK?
cliUtil tests use nested JSON structure to pass intended mocked structure to memfs' fromJSON method. This method does not parse nested JSON properly and carries on silently, resulting in misleading test results. Update test suite to not use nested JSON where possible, and where necessary use .fromNestedJSON instead.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 3 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
1 participant
What is the purpose of this pull request?
Resolves #2767
Overview of changes:
Update CLI clean functionality to use
rmdirinstead of a force delete. This preserves the folder structure in the situation where files already exist in them. Added test cases to account for this case.Anything you'd like to highlight/discuss:
Testing instructions:
Proposed commit message: (wrap lines at 72 characters)
Make cleaning in CLI respect existing files
Checklist: ☑️
Reviewer checklist:
Indicate the SEMVER impact of the PR:
At the end of the review, please label the PR with the appropriate label:
r.Major,r.Minor,r.Patch.Breaking change release note preparation (if applicable):