feat: add dry-run and risk warnings to the interactive flow - #78
Open
gabrielgstein-dev wants to merge 1 commit into
Open
Conversation
`-d, --dry-run` existed only on the `clean` subcommand. The default command — the `npx mac-cleaner-cli` the README leads with — had no way to preview a deletion, never printed `safetyNote`, and gave no visual sign of a category's safety level. The user chose blind and could not rehearse. Three changes: - `-d, --dry-run` on the default command. The flag is passed down to `scanner.clean()` rather than simulated in the command, because Docker and Homebrew override `clean()` with external commands and have their own dry-run branch; simulating from the outside would produce a report that does not match what the real command would do. - Safety icon (green/yellow/red) plus a legend in the file picker, and the risky category's safetyNote inline, next to the row where the decision is actually made. The level has been in types.ts all along and this screen never showed it. - The summary before the confirmation now lists the selection category by category, repeats the warnings for the selected risky ones, and states that deletion is permanent and does not use the Trash. It also fixes a dry run that lied. `removeItemWithError` returned early on `dryRun` BEFORE validating the path, so the preview reported success for paths the real run refuses. On my machine the logs scanner offers 64 items / 129 MB of which 40 live under /var/log, which is in PROTECTED_PATHS: the dry run promised the full total and the real run returned a failure for each of those 40. Note for review: this changes an existing test's expectation. "should report no failures on dry run" asserted success and zero failures for '/System/Library' on a dry run — an expectation that encoded the bug, since that path is protected and the real run refuses it. It is now split into two tests: protected paths report a failure (like the real run), and a deletable path reports success while still being left untouched on disk. Verified with the built CLI in a real pty: icons and warnings render, the prompt reads "Simulate cleaning?", the report reads "[DRY RUN] Nothing was deleted", and ~/Library/Caches was untouched (176 entries before and after).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Brings dry-run and risk information to the flow
npx mac-cleaner-cliactuallyruns, and fixes a dry run that reported success for paths the real run refuses.
What was happening.
-d, --dry-runexisted only on thecleansubcommand.The default command — the one the README leads with — had no way to preview a
deletion. It also never printed
safetyNote, and the picker showed no sign ofa category's
safetyLevel. Both fields have been intypes.tsall along; thescreen where the decision is made just never displayed them.
Three changes:
-d, --dry-runon the default command. The flag is passed down toscanner.clean()rather than simulated in the command, becauseDockerandHomebrewoverrideclean()with external commands and have their owndry-run branch. Simulating from the outside would produce a report that does
not match what the real command would do.
Safety icon (green/yellow/red) plus a legend in the picker, and the risky
category's
safetyNoteinline, on the row where the choice happens.The summary before the confirmation lists the selection category by
category, repeats the warnings for the selected risky ones, and states that
deletion is permanent and does not use the Trash.
The dry-run bug.
removeItemWithErrorreturned early ondryRunbeforevalidating the path:
So the preview promised space that the real run would never free. Concrete
case on my machine: the logs scanner offers 64 items / 129 MB, of which 40 live
under
/var/log— which is inPROTECTED_PATHS. The dry run reported the fulltotal; the real run returned a failure for each of those 40. Moving the check
above the early return makes the preview agree with reality.
Benefits.
actually run.
made, instead of living only in the source.
real run would free.
Type of change
Related issue
None directly, though the honest-preview part is adjacent to #61
("Failed to remove items"), where protected/EPERM paths surfaced as failures
after the fact.
Checklist
bun run lintpassesbun run testpasses (no regressions) — 350 passedbun run buildsucceedsNotes for reviewer
This PR changes an existing test's expectation — please look at this one.
should report no failures on dry runasserted:That expectation encoded the bug:
/System/Libraryis protected, so the realrun refuses it, and the test asserted the dry run would claim otherwise. It is
now split in two — a protected path reports a failure (matching the real run),
and a deletable path reports success while still being left untouched on disk.
If you would rather keep the old assertion, the honest-preview part of this PR
is the piece to drop, and I can split it out.
Verified with the built CLI in a real pty: icons and warnings render, the
prompt reads
Simulate cleaning?, the report reads[DRY RUN] Nothing was deleted, and~/Library/Cacheswas untouched(176 entries before and after).