Skip to content

feat: add dry-run and risk warnings to the interactive flow - #78

Open
gabrielgstein-dev wants to merge 1 commit into
guhcostan:mainfrom
gabrielgstein-dev:fix/dry-run-and-risk-warnings-in-interactive-flow
Open

feat: add dry-run and risk warnings to the interactive flow#78
gabrielgstein-dev wants to merge 1 commit into
guhcostan:mainfrom
gabrielgstein-dev:fix/dry-run-and-risk-warnings-in-interactive-flow

Conversation

@gabrielgstein-dev

Copy link
Copy Markdown

What does this PR do?

Brings dry-run and risk information to the flow npx mac-cleaner-cli actually
runs, and fixes a dry run that reported success for paths the real run refuses.

What was happening. -d, --dry-run existed only on the clean subcommand.
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 of
a category's safetyLevel. Both fields have been in types.ts all along; the
screen where the decision is made just never displayed them.

Three changes:

  1. -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.

  2. Safety icon (green/yellow/red) plus a legend in the picker, and the risky
    category's safetyNote inline, on the row where the choice happens.

  3. 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. removeItemWithError returned early on dryRun before
validating the path:

if (dryRun) return null;              // reported as success
const safetyError = validatePathSafety(path);   // never reached

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 in PROTECTED_PATHS. The dry run reported the full
total; 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.

  • Users can rehearse a deletion before committing to it, on the command they
    actually run.
  • The safety level and its explanation reach the screen where the decision is
    made, instead of living only in the source.
  • The dry run's numbers can be trusted: what it says it would free is what the
    real run would free.

Type of change

  • Bug fix
  • New feature (new scanner, command, or option)
  • Improvement to existing feature
  • Chore / dependency update
  • Documentation

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 lint passes
  • bun run test passes (no regressions) — 350 passed
  • bun run build succeeds
  • New tests added for new behavior
  • I've tested this on macOS locally

Notes for reviewer

This PR changes an existing test's expectation — please look at this one.

should report no failures on dry run asserted:

const result = await removeItems(
  [{ path: '/System/Library', size: 0, name: 'Library', isDirectory: true }],
  true
);
expect(result.success).toBe(1);
expect(result.failed).toBe(0);

That expectation encoded the bug: /System/Library is protected, so the real
run 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/Caches was untouched
(176 entries before and after).

`-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).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants