Skip to content

Fix cleaning safety guards#30

Closed
edufalcao wants to merge 1 commit intomomenbasel:mainfrom
edufalcao:security/cleaning-safety-fixes
Closed

Fix cleaning safety guards#30
edufalcao wants to merge 1 commit intomomenbasel:mainfrom
edufalcao:security/cleaning-safety-fixes

Conversation

@edufalcao
Copy link
Copy Markdown

Summary

  • Fix item selection state so items that scan as not selected, especially large and old files, remain deselected unless the user explicitly opts them in.
  • Add a destructive confirmation gate for manual cleaning and snapshot the exact selected items at confirmation request time.
  • Restrict scheduled auto-cleaning to cache-like categories and keep personal-file/purgeable categories out of unattended cleanup.
  • Add XCTest coverage for selection semantics, scheduled-clean category policy, and manual cleaning confirmation behavior.

Safety notes

  • This change was prepared by an LLM coding agent, Codex, with human oversight.
  • Because this application can permanently delete local files, this PR should be reviewed and tested carefully before merging.
  • The cleaner still uses permanent deletion through FileManager.removeItem; this PR reduces accidental deletion risk but does not change deletion to Trash-based recovery.

Verification

  • xcodebuild test -project PureMac.xcodeproj -scheme PureMac -destination 'platform=macOS'
    • 9 tests executed, 0 failures, TEST SUCCEEDED.
  • xcodebuild build -project PureMac.xcodeproj -scheme PureMac -configuration Release -destination 'platform=macOS'
    • BUILD SUCCEEDED.
  • git diff --check
    • No whitespace errors.
  • Static searches checked for stale deselectedItems state, telemetry/network clients, credential handling, and destructive/process execution surfaces.

@momenbasel
Copy link
Copy Markdown
Owner

Hey @edufalcao, this is a really thoughtful PR - the safety guards you've proposed are exactly the kind of thing that separates a reliable tool from a dangerous one. Especially love:

  • Confirmation dialog before destructive operations
  • Restricting auto-clean to cache-like categories (keeping personal files and purgeable space out of unattended cleanup)
  • The XCTest coverage - that's how it should be done

We shipped a v2.0 rewrite that changed the architecture significantly, so this can't merge directly. But we've adopted your confirmation dialog approach - the clean buttons now show a .confirmationDialog before proceeding. The auto-clean safety restriction is a great idea we should add too.

If you're interested in contributing to v2, adding test coverage for the new AppState would be incredibly valuable. The codebase is much cleaner now and easier to test.

Really appreciate the careful approach with the safety notes too. Thank you!

momenbasel added a commit that referenced this pull request Apr 14, 2026
Inspired by community PRs #29, #30, #32, #33:

- Onboarding: 3-page welcome flow (Welcome, FDA setup, Ready)
  Shows only on first launch via @AppStorage
  Polls FDA status every second with live UI update
- Search: .searchable() in CategoryDetailView for filtering files
- Confirmation: .confirmationDialog before all destructive operations
- Protection: 27 Apple system apps excluded from uninstaller
  (Safari, Finder, Mail, Calendar, Photos, etc.)
  Also skips /System paths entirely
@momenbasel
Copy link
Copy Markdown
Owner

Hey @edufalcao! Closing this one - we adopted your confirmation dialog approach and system app safety ideas in v2.0. Clean buttons now show .confirmationDialog before proceeding, and we added system app protection to the uninstaller.

Sorry for closing PRs - we rewrote the entire codebase to use native SwiftUI (NavigationSplitView, Form, Toggle, etc.) for v2.0, so the v1 code structure doesn't exist anymore. Your safety-first thinking was really valuable though and directly influenced the v2 implementation.

The test coverage idea is still something we need - if you'd like to take a look at the new AppState and write some XCTests for it, that would be incredible. Would love your eyes on the new codebase! Thanks for the thoughtful contribution!

@momenbasel momenbasel closed this Apr 14, 2026
@momenbasel
Copy link
Copy Markdown
Owner

Hey @edufalcao! You're credited in the v2.0 README under the Acknowledgments section for the cleaning safety guards PR. Your confirmation dialog and auto-clean safety ideas are now in production. Thank you!

@edufalcao
Copy link
Copy Markdown
Author

Thank you for the thoughtful response and for taking the time to review this so carefully.

I appreciate the kind words, and I’m glad some of the ideas were useful for v2. It’s great to know the confirmation dialog and auto-clean safety concepts helped influence the new implementation.

Also, thank you for the credit in the README — I really appreciate it.

I’d be happy to take a look at the new AppState/codebase and see whether I can help with test coverage there as well.

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