feat: make backups real and opt-in instead of a facade - #79
Open
gabrielgstein-dev wants to merge 1 commit into
Open
feat: make backups real and opt-in instead of a facade#79gabrielgstein-dev wants to merge 1 commit into
gabrielgstein-dev wants to merge 1 commit into
Conversation
`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.
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?
Turns
src/utils/backup.tsfrom unreachable code into a working, opt-infeature, and makes
~/.maccleanerrcactually 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.
restoreBackupwas not reachable from the CLI at all, sobackup --listcould only ever print nothing. Separately,loadConfig()wasonly ever called by
config --show— the file written byconfig --inithadno 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:
the failure is reported. Ending with the file in place and an error on screen
beats ending with the file gone and no copy.
freedSpacestays 0 and the size is reportedas
backedUpSize, with the UI saying so before the confirmation and again inthe 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.
supportsBackup = false. The external tooldoes 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
cleansubcommand 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:
backupItemusedpath.replace(homedir(), 'HOME')— replaces the firstoccurrence anywhere in the string and, for a path outside home, returns an
absolute path, making
join(backupDir, '/x')land outside the backupdirectory. It now accepts only paths under home (the one shape
restoreBackupcan undo) and validates viavalidatePathSafety.restoreBackupcheckedstartsWith(BACKUP_DIR)with no trailing slash, so~/.mac-cleaner-cli/backup-anythingpassed the check. Verified refused now.ensureBackupDirnamed the folder with an ISO timestamp (millisecondresolution) and used
mkdir recursive, which accepts an existing directorysilently — 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
Related issue
None — found while tracing what happens to a file after it is selected.
Checklist
bun run lintpassesbun run testpasses (no regressions) — 354 passedbun run buildsucceedsNotes 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 wouldrather 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) andasserted
expect(typeof result).toBe('boolean')— they would pass with thebackup 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.tswas removed from the vitest coverage exclusion list, since it nowruns 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
mkdiron 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 originalpath, and
--restorerefused for a directory outside the backup folder.