Repository: Evertras/bubble-table
Pull request #171 (2024-05-03 00:56:38)

Fix bug where selecting a row while filtered would lose all other rows
https://github.com/Evertras/bubble-table/issues/170

Before, we would copy all visible rows over with just the modified row having toggled its select.  However, when filtered, the visible rows did not include all the filtered-out rows, so they would be lost.

Now we keep an internal ID on all rows.  This may also be useful in the future, but it's particularly useful to allow us to find the selected row and toggle its selection directly in the underlying row structure.  This is also quicker and requires less copying, which is a bonus, despite being a simple linear search.


Patch:
diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml
index d7b9553..60da31a 100644
--- a/.github/workflows/lint.yml
+++ b/.github/workflows/lint.yml
@@ -13,9 +13,14 @@ jobs:
     name: lint
     runs-on: ubuntu-latest
     steps:
-      - uses: actions/checkout@v4
-      - name: golangci-lint
-        uses: golangci/golangci-lint-action@v3
+      # For whatever reason, 1.21.9 blows up because it can't
+      # find 'max' in some go lib... pinning to 1.21.4 fixes this
+      - name: Install Go
+        uses: actions/setup-go@v4
         with:
-          version: v1.51.1
+          go-version: 1.21.4
+      - name: Checkout code
+        uses: actions/checkout@v4
+      - name: Lint
+        run: make lint
 
diff --git a/examples/filter/main.go b/examples/filter/main.go
index 5ff19e0..bc836e2 100644
--- a/examples/filter/main.go
+++ b/examples/filter/main.go
@@ -30,6 +30,7 @@ func NewModel() Model {
 			Filtered(true).
 			Focused(true).
 			WithPageSize(10).
+			SelectableRows(true).
 			WithRows([]table.Row{
 				table.NewRow(table.RowData{
 					columnKeyTitle:       "Computer Systems : A Programmer's Perspective",
diff --git a/table/row.go b/table/row.go
index e59fd1c..ab3dd61 100644
--- a/table/row.go
+++ b/table/row.go
@@ -2,6 +2,7 @@ package table
 
 import (
 	"fmt"
+	"sync/atomic"
 
 	"github.com/charmbracelet/lipgloss"
 	"github.com/muesli/reflow/wordwrap"
@@ -21,14 +22,22 @@ type Row struct {
 	Data  RowData
 
 	selected bool
+
+	// id is an internal unique ID to match rows after they're copied
+	id uint32
 }
 
+var lastRowID uint32 = 1
+
 // NewRow creates a new row and copies the given row data.
 func NewRow(data RowData) Row {
 	row := Row{
 		Data: make(map[string]interface{}),
+		id:   lastRowID,
 	}
 
+	atomic.AddUint32(&lastRowID, 1)
+
 	for key, val := range data {
 		// Doesn't deep copy val, but close enough for now...
 		row.Data[key] = val
diff --git a/table/update.go b/table/update.go
index 75998de..198a57d 100644
--- a/table/update.go
+++ b/table/update.go
@@ -30,14 +30,19 @@ func (m *Model) toggleSelect() {
 		return
 	}
 
-	rows := make([]Row, len(m.GetVisibleRows()))
-	copy(rows, m.GetVisibleRows())
+	rows := m.GetVisibleRows()
 
-	currentSelectedState := rows[m.rowCursorIndex].selected
+	rowID := rows[m.rowCursorIndex].id
 
-	rows[m.rowCursorIndex].selected = !currentSelectedState
+	currentSelectedState := false
+
+	for i := range m.rows {
+		if m.rows[i].id == rowID {
+			currentSelectedState = m.rows[i].selected
+			m.rows[i].selected = !m.rows[i].selected
+		}
+	}
 
-	m.rows = rows
 	m.visibleRowCacheUpdated = false
 
 	m.appendUserEvent(UserEventRowSelectToggled{


Test patch:
diff --git a/table/update_test.go b/table/update_test.go
index f42a7a1..87cc5ff 100644
--- a/table/update_test.go
+++ b/table/update_test.go
@@ -330,3 +330,82 @@ func TestFilterWithKeypresses(t *testing.T) {
 
 	assert.Len(t, visible, 2)
 }
+
+// This is a long test with a lot of movement keys pressed, that's okay because
+// it's simply repetitive and tracking the same kind of state change many times
+//
+//nolint:funlen
+func TestSelectOnFilteredTableDoesntLoseRows(t *testing.T) {
+	// Issue: https://github.com/Evertras/bubble-table/issues/170
+	//
+	// Basically, if you filter a table and then select a row, then
+	// clear the filter, then all the other rows should still exist.
+
+	cols := []Column{
+		NewColumn("name", "Name", 10).WithFiltered(true),
+	}
+
+	model := New(cols).WithRows([]Row{
+		NewRow(RowData{"name": "Charmander"}),
+		NewRow(RowData{"name": "Pikachu"}),
+	}).Focused(true).Filtered(true).SelectableRows(true)
+
+	hitKey := func(key rune) {
+		model, _ = model.Update(tea.KeyMsg{
+			Type:  tea.KeyRunes,
+			Runes: []rune{key},
+		})
+	}
+
+	hitEnter := func() {
+		model, _ = model.Update(tea.KeyMsg{Type: tea.KeyEnter})
+	}
+
+	hitEscape := func() {
+		model, _ = model.Update(tea.KeyMsg{Type: tea.KeyEscape})
+	}
+
+	hitSpacebar := func() {
+		model, _ = model.Update(tea.KeyMsg{Type: tea.KeySpace})
+	}
+
+	// First, apply the filter
+	//
+	// Note that we try and filter for the second row, "Pikachu"
+	// so that we can better ensure everything is stably intact
+	visible := model.GetVisibleRows()
+
+	assert.Len(t, visible, 2)
+	hitKey(rune(model.KeyMap().Filter.Keys()[0][0]))
+	assert.Len(t, visible, 2)
+	hitKey('p')
+	hitKey('i')
+	hitKey('k')
+
+	visible = model.GetVisibleRows()
+
+	assert.Len(t, visible, 1)
+
+	hitEnter()
+
+	// Now apply the selection toggle
+	hitSpacebar()
+
+	visible = model.GetVisibleRows()
+	assert.Len(t, visible, 1)
+	assert.True(t, visible[0].selected)
+
+	// Now clear the filter and make sure everything is intact
+	hitEscape()
+
+	visible = model.GetVisibleRows()
+
+	assert.Len(t, visible, 2)
+
+	if t.Failed() {
+		return
+	}
+
+	assert.False(t, visible[0].selected)
+	assert.True(t, visible[1].selected)
+}