feat: Multi-select with bulk install/uninstall#52
feat: Multi-select with bulk install/uninstall#52hanthor wants to merge 2 commits intoValkyrie00:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR implements multi-select functionality for the package table interface, allowing users to select multiple packages using the Spacebar key and perform bulk operations (install, remove, update) on the selected items.
Changes:
- Added multi-select support to the Table component with visual highlighting
- Integrated bulk operation support into install, remove, and update actions
- Added Spacebar key binding for toggling row selection
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| internal/ui/components/table.go | Adds selectedRows map to track multi-selected rows and methods for toggling, clearing, and retrieving selections with visual feedback |
| internal/services/input.go | Implements spacebar key handler for selection toggling and modifies install/remove/update handlers to process multiple selected packages |
| Makefile | Adds :Z SELinux label flag to podman volume mount for better compatibility |
| .env | Changes build target platform from darwin/arm64 to linux/amd64 |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return nil | ||
| } | ||
|
|
||
| for _, input := range s.keyActions { |
There was a problem hiding this comment.
The spacebar key is handled explicitly before the general key action loop (lines 177-181), but it will also match in the loop (lines 183-195) if ActionToggleSelection is in keyActions. This creates redundant handling. Consider either removing the explicit handling at lines 177-181 or ensuring ActionToggleSelection is not added to keyActions (it's currently added at line 138 but marked with HideFromLegend=true).
| for _, input := range s.keyActions { | |
| for _, input := range s.keyActions { | |
| // Skip ActionToggleSelection here because space is already handled explicitly above. | |
| if input == s.ActionToggleSelection { | |
| continue | |
| } |
| t.selectedRows = make(map[int]bool) | ||
| } | ||
|
|
||
| func (t *Table) ClearSelection() { |
There was a problem hiding this comment.
The ClearSelection method only resets the selectedRows map but doesn't update the visual state of the table cells. This means previously selected rows will still appear highlighted until the table is redrawn. Consider iterating through the current selectedRows before clearing and resetting their background colors, or update the method to accept a theme parameter and properly reset cell backgrounds.
| func (t *Table) ClearSelection() { | |
| func (t *Table) ClearSelection() { | |
| // Reset visual state for all currently selected rows | |
| for row := range t.selectedRows { | |
| colCount := t.view.GetColumnCount() | |
| for col := 0; col < colCount; col++ { | |
| cell := t.view.GetCell(row, col) | |
| if cell != nil { | |
| cell.SetBackgroundColor(t.theme.DefaultBgColor) | |
| } | |
| } | |
| } | |
| // Clear logical selection state |
| func (t *Table) GetSelectedRows() []int { | ||
| rows := make([]int, 0, len(t.selectedRows)) | ||
| for row := range t.selectedRows { | ||
| rows = append(rows, row) | ||
| } | ||
| return rows | ||
| } |
There was a problem hiding this comment.
The GetSelectedRows method returns rows in non-deterministic order since it iterates over a map. This could lead to unpredictable behavior when processing selected packages. Consider sorting the returned slice to ensure consistent ordering (e.g., ascending by row number).
| BUILD_GOOS=linux | ||
| BUILD_GOARCH=amd64 No newline at end of file |
There was a problem hiding this comment.
The changes to BUILD_GOOS and BUILD_GOARCH appear unrelated to the multi-select feature. These settings change the build target from macOS ARM64 (darwin/arm64) to Linux AMD64 (linux/amd64). If this change is intentional, it should be documented in the PR description or split into a separate commit. If unintentional, it should be reverted to the original values.
| BUILD_GOOS=linux | |
| BUILD_GOARCH=amd64 | |
| BUILD_GOOS=darwin | |
| BUILD_GOARCH=arm64 |
| theme *theme.Theme | ||
| view *tview.Table | ||
| theme *theme.Theme | ||
| selectedRows map[int]bool |
There was a problem hiding this comment.
The selectedRows map is accessed without synchronization from both the UI thread (in ToggleSelection, GetSelectedRows) and from goroutines (ClearSelection is called from a goroutine at line 500). This creates a potential race condition. Consider using a sync.RWMutex to protect concurrent access to the selectedRows map, or ensure ClearSelection is called on the main thread using QueueUpdateDraw.
| // Force redraw of table to remove selection visuals | ||
| // s.appService.forceRefreshResults() // Might be too heavy? | ||
| // Actually Table.ToggleSelection updates visual. | ||
| // ClearSelection needs to update visual too. | ||
| // But Table.ClearSelection just clears the map. I need to implement visual clear in Table or just force refresh. | ||
| // For now, let's just assume we need to refresh. |
There was a problem hiding this comment.
Remove commented code and TODO comments. This appears to be debugging/development notes that should be cleaned up before merging. The actual implementation at line 211 (s.appService.search(...)) addresses the visual refresh concern mentioned in the comments.
| // Force redraw of table to remove selection visuals | |
| // s.appService.forceRefreshResults() // Might be too heavy? | |
| // Actually Table.ToggleSelection updates visual. | |
| // ClearSelection needs to update visual too. | |
| // But Table.ClearSelection just clears the map. I need to implement visual clear in Table or just force refresh. | |
| // For now, let's just assume we need to refresh. |
Implements multi-select functionality using Spacebar, allowing bulk install (I) and uninstall (U).