Feat/merged tabs with sudo password modal - #14
Merged
Conversation
…pdates - Remove Outdated tab and merge with Installed tab - Create new MergedPackageList component that displays both outdated and installed packages - Add SelectionState component for tracking selected outdated packages - Outdated packages now appear first with checkboxes for selection - Add 'Update Selected' button that's enabled only when packages are selected - Update UI to show 'Installed & Outdated' as single tab - Both lists load simultaneously for better performance - Selection is cleared after successful batch update
- Both LoadInstalled and LoadOutdated now have their own logs Arc - Prevents race conditions and contention on shared logs - Packages now properly load after merge - Each task polls independently from its own logs container
- Add 'Select All' and 'Deselect All' buttons for outdated packages - Increase MAX_LOG_SIZE from 200 to 1000 to retain more logs - Increase default output panel height from 150 to 250 pixels - Fixes auto-disappearing output and improves log visibility
- Add 'verbose-logging' feature to Cargo.toml - TRACE logs only show when verbose-logging feature is enabled - Default debug build now shows DEBUG logs instead of TRACE - Release builds show INFO logs unless verbose-logging is enabled - Usage: cargo run --features verbose-logging
- Add usage section for verbose-logging feature flag - Explain logging levels for different build configurations - Include examples of how to run with verbose logging
- Switch to Installed tab after install, uninstall, update, or update_all succeeds - Automatically reloads and displays updated package list - Provides immediate visual feedback to user that operation completed - List refreshes with new state (installed/outdated status changes)
- Add local state update methods to MergedPackageList: - mark_package_updated: Move package from outdated to installed - remove_from_outdated: Remove package from outdated list - remove_installed_package: Remove from installed list - add_installed_package: Add to installed list - After successful operations, update local state instantly: - Install: Remove from outdated selection - Uninstall: Remove from installed list - Update: Move from outdated to installed - Update All: Move all updated packages from outdated to installed - No more Brew requery after operations = instant UI update! - Provides immediate feedback while staying on current tab
- Clone new_version string before second use - Add missing remove_from_outdated method - All local update methods now compile correctly
- Simplify mark_package_updated to remove from outdated and update installed - After update: package disappears from Outdated table immediately - After update: version in Installed table is refreshed (available_version cleared) - No re-querying Brew, all changes are local and instant - Works perfectly for single and batch updates
- Queue packages for sequential updates instead of all at once - Add PasswordModal component for password input on install/uninstall - Detect password requirement errors and prompt user with modal - Add password-aware install/uninstall methods to BrewCommand - Retry operations with provided password after authentication - Process updates one at a time with progress feedback
- Remove unused imports (SelectionState, Result from anyhow) - Fix password modal response handling with gained_focus() - Prefix unused on_install parameter with underscore
- Remove incorrect if condition on ui.checkbox() call - checkbox() returns Response, not bool - Just call the method directly to update show_password state
- Implement SUDO_ASKPASS mechanism for secure password handling - Fix 'Running Homebrew as root' error on privileged operations - Bump version to 0.5.0 - Add comprehensive documentation for security & design decisions - All password entry now in app modal (no terminal prompts) - Support for privileged cask install/uninstall operations - Merge multiple package lists (formulas + casks) in UI tabs - Proper credential cleanup and error handling
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR implements a unified tabbed interface for installed and outdated packages, adds secure password authentication for privileged operations, and introduces sequential package updates for better stability.
Key Changes
- Merged "Installed" and "Outdated" tabs into a single "Installed & Outdated" view with distinct sections
- Added password modal for secure authentication when operations require administrator privileges
- Implemented sequential package update processing with progress tracking
- Added SUDO_ASKPASS mechanism to prevent terminal password prompts
Reviewed Changes
Copilot reviewed 12 out of 13 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/presentation/ui/app.rs | Core app refactor: merged tabs, added password modal handling, sequential updates, and password error detection |
| src/infrastructure/brew/command.rs | Implemented SUDO_ASKPASS mechanism for secure password handling and privilege escalation detection |
| src/presentation/components/merged_package_list.rs | New component to display both installed and outdated packages in a single unified view |
| src/presentation/components/password_modal.rs | New modal for secure password input with show/hide toggle |
| src/presentation/components/selection_state.rs | New component for managing multi-package selection state |
| src/presentation/components/tab_manager.rs | Removed outdated tab from tab management |
| src/presentation/components/mod.rs | Added exports for new components |
| src/presentation/services/log_capture.rs | Added verbose-logging feature flag and refined log level configuration |
| src/application/use_cases/package_operations.rs | Changed execute signature to take package by reference |
| Cargo.toml | Bumped version to 0.5.0, added verbose-logging feature flag |
| README.md | Updated documentation with new features and usage instructions |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| self.status_message = "Outdated packages loaded".to_string(); | ||
| } | ||
|
|
||
| if self.loading_installed == false && self.loading_outdated == false { |
There was a problem hiding this comment.
Avoid explicit comparisons with boolean literals. Use !self.loading_installed && !self.loading_outdated instead of comparing with false.
Suggested change
| if self.loading_installed == false && self.loading_outdated == false { | |
| if !self.loading_installed && !self.loading_outdated { |
| let script_path = temp_dir.join("brewsty_askpass.sh"); | ||
|
|
||
| let script_content = format!( | ||
| "#!/bin/bash\necho '{}'\n", |
There was a problem hiding this comment.
The password escaping in the askpass script may not be sufficient for all special characters. Consider using base64 encoding or a more robust escaping mechanism to handle passwords with complex special characters (e.g., backticks, dollar signs, newlines) that could cause shell injection issues.
|
|
||
| let output = Command::new("brew") | ||
| .args(args) | ||
| .env("SUDO_ASKPASS", "/nonexistent/askpass") // Force sudo to not use terminal |
There was a problem hiding this comment.
The comment should clarify that this is intentionally a nonexistent path to force sudo to fail when no password is provided, triggering the password modal. The current comment is unclear about the intentional failure mechanism.
Suggested change
| .env("SUDO_ASKPASS", "/nonexistent/askpass") // Force sudo to not use terminal | |
| // Intentionally use a nonexistent path to force sudo to fail when no password is provided, | |
| // which triggers the password modal. This prevents sudo from prompting in the terminal. | |
| .env("SUDO_ASKPASS", "/nonexistent/askpass") |
Comment on lines
+89
to
+91
| if response.gained_focus() { | ||
| response.request_focus(); | ||
| } |
There was a problem hiding this comment.
The condition if response.gained_focus() followed by response.request_focus() is redundant. If the response has already gained focus, requesting focus again is unnecessary. This check should either be removed or the logic should request focus when the modal is first shown.
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.
Changes
What's Fixed
✅ Uninstall/install privileged casks (e.g., docker-desktop) now works
✅ No terminal password prompts (all in app modal)
✅ Secure credential handling (0o700 permissions, ~200ms lifetime)
Testing
✅ Correct password → success
✅ Incorrect password → error + retry
✅ Cancel modal → cancelled cleanly
✅ No terminal prompts
Branch:
feat/merged-tabs-with-sudo-password-modalVersion: 0.5.0