Skip to content

feat: make backups real and opt-in instead of a facade - #79

Open
gabrielgstein-dev wants to merge 1 commit into
guhcostan:mainfrom
gabrielgstein-dev:feat/real-opt-in-backups
Open

feat: make backups real and opt-in instead of a facade#79
gabrielgstein-dev wants to merge 1 commit into
guhcostan:mainfrom
gabrielgstein-dev:feat/real-opt-in-backups

Conversation

@gabrielgstein-dev

Copy link
Copy Markdown

What does this PR do?

Turns src/utils/backup.ts from unreachable code into a working, opt-in
feature, and makes ~/.maccleanerrc actually affect cleaning.

What was happening. The backup module is ~230 lines with a CLI command and
a 7-day retention policy, and it has zero callers. No deletion has ever
produced a backup. restoreBackup was not reachable from the CLI at all, so
backup --list could only ever print nothing. Separately, loadConfig() was
only ever called by config --show — the file written by config --init had
no effect on any scan or clean.

What this does. With {"backupEnabled": true} in ~/.maccleanerrc,
selected items are moved to ~/.mac-cleaner-cli/backup/<timestamp>/
instead of deleted, and come back with the new backup --restore <dir>.
Default stays off, so nothing changes for existing users.

Three contracts, stated because each is a place a backup feature can quietly
betray someone:

  • A failed backup is never turned into a deletion. The item stays put and
    the failure is reported. Ending with the file in place and an error on screen
    beats ending with the file gone and no copy.
  • Moving does not free space. freedSpace stays 0 and the size is reported
    as backedUpSize, with the UI saying so before the confirmation and again in
    the summary. Reporting it as "freed" would trade the old facade for a new
    untruth — and a user seeing "0 B freed" without explanation would file it as
    a bug.
  • Docker and Homebrew declare supportsBackup = false. The external tool
    does the deleting, so there is nothing of ours to move. Both entry points
    warn that those categories WILL be deleted even with backups on.

Both the interactive flow and the clean subcommand honour the setting.
Backing up in one and deleting permanently in the other would be worse than no
backup at all, since the outcome would depend on which command the user typed.

Three bugs in the module, all of which existed only because it had never
run:

  1. backupItem used path.replace(homedir(), 'HOME') — replaces the first
    occurrence anywhere in the string and, for a path outside home, returns an
    absolute path, making join(backupDir, '/x') land outside the backup
    directory. It now accepts only paths under home (the one shape
    restoreBackup can undo) and validates via validatePathSafety.
  2. restoreBackup checked startsWith(BACKUP_DIR) with no trailing slash, so
    ~/.mac-cleaner-cli/backup-anything passed the check. Verified refused now.
  3. ensureBackupDir named the folder with an ISO timestamp (millisecond
    resolution) and used mkdir recursive, which accepts an existing directory
    silently — two runs in the same millisecond would share it.

Benefits. A recoverable mode for users who want one; a config file that
does what it says; and the removal of a feature that promised recovery it could
never deliver.

Type of change

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

Related issue

None — found while tracing what happens to a file after it is selected.

Checklist

  • bun run lint passes
  • bun run test passes (no regressions) — 354 passed
  • bun run build succeeds
  • New tests added for new behavior
  • I've tested this on macOS locally

Notes for reviewer

This is the largest of the set and the one most worth pushing back on — happy
to split it (module fixes / wiring / --restore) or to close it if you would
rather delete the backup module outright. Deleting it is a defensible call:
moving files on the same volume is not a backup against disk failure, and it
does not free space, which is the tool's whole purpose. I went with "make it
real and opt-in" because the CLI already advertises the command.

On the old tests. They used paths under tmpdir() (outside home) and
asserted expect(typeof result).toBe('boolean') — they would pass with the
backup failing 100% of the time, which is exactly the state the module was in.
Replaced with a real round trip: move, assert the original is gone, assert the
content in the copy, restore, assert the content is back at the original path.
backup.ts was removed from the vitest coverage exclusion list, since it now
runs on the deletion path.

Bug 3 above surfaced first as suite flakiness (vitest runs test files in
parallel), roughly one failure in four runs, before it could ever surface as a
production bug. The fix is an incremental suffix plus a non-recursive mkdir
on the last level, which is what makes creation exclusive.

Verified with the built CLI: a real item backed up out of ~/Library/Caches,
backup --list, backup --restore, identical content returned to the original
path, and --restore refused for a directory outside the backup folder.

`src/utils/backup.ts` is 230 lines with a CLI command and a 7-day
retention policy — and zero callers. No deletion has ever produced a
backup. `restoreBackup` was not even reachable from the CLI, so
`backup --list` could only ever show nothing. In the same way,
`loadConfig()` was only called by `config --show`: the file written by
`config --init` had no effect on cleaning at all.

With `{"backupEnabled": true}` in ~/.maccleanerrc, selected items are now
MOVED to ~/.mac-cleaner-cli/backup/<timestamp>/ instead of deleted, and
can be brought back with the new `backup --restore <dir>`.

Three contracts the implementation commits to, stated explicitly because
each one is a place where a "backup" feature can quietly betray a user:

- A failed backup is NOT turned into a deletion. The item stays where it
  is and the failure is reported. Ending with the file in place and an
  error on screen beats ending with the file gone and no copy.
- Moving does not free space. `freedSpace` stays 0 and the size is
  reported as `backedUpSize`, with the UI saying so before the
  confirmation and again in the summary. Reporting it as "freed" would
  trade the old facade for a new untruth.
- Docker and Homebrew declare `supportsBackup = false`: the external tool
  does the deleting, so there is nothing of ours to move. Both the
  interactive flow and the `clean` subcommand warn that those categories
  WILL be deleted even with backups on.

Both entry points honour the setting. Backing up in the interactive flow
while `clean` deleted permanently would be worse than no backup at all,
since the outcome would depend on which command the user typed.

Three bugs in the module, all of which existed only because it had never
run:

1. `backupItem` used `path.replace(homedir(), 'HOME')`, which replaces
   the first occurrence anywhere in the string and, for a path outside
   home, returned an absolute path — making `join()` land outside the
   backup directory. It now accepts only paths under home (the single
   shape `restoreBackup` can undo) and validates via validatePathSafety.
2. `restoreBackup` checked `startsWith(BACKUP_DIR)` with no trailing
   slash, so `~/.mac-cleaner-cli/backup-anything` passed. Verified it is
   refused now.
3. `ensureBackupDir` named the folder with an ISO timestamp (millisecond
   resolution) and used `mkdir recursive`, which accepts an existing
   directory silently — two sessions in the same millisecond would share
   it. This surfaced first as suite flakiness, since vitest runs files in
   parallel.

Tests: the previous ones used paths under tmpdir() (outside home) and
asserted `expect(typeof result).toBe('boolean')` — they would pass with
the backup failing 100% of the time. Replaced with a real round trip:
move, assert the original is gone, assert the content in the copy,
restore, assert the content is back at the original path. backup.ts was
removed from the vitest coverage exclusion list.

Verified with the built CLI: a real item backed up from ~/Library/Caches,
`backup --list`, `backup --restore`, identical content returned, and
`--restore` refused for a path outside the backup folder.
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