From 6e1de28d534268a83b386a54465b9d6856626c6a Mon Sep 17 00:00:00 2001 From: Wes McKinney Date: Mon, 30 Mar 2026 08:38:06 -0500 Subject: [PATCH 1/7] fix(tui): restore column width auto-sizing after requested model fields d2d671f6 introduced colRequestedModel and colRequestedProvider with three bugs that broke auto-sizing for existing users: 1. allHeaders array omitted header text for the new columns (indices 12-13 defaulted to ""), so they rendered with no header 2. Minimum widths (15, 18) were larger than the header text (9, 12), stealing extra space from flex columns even when content was empty 3. migrateColumnConfig did not backfill new default-hidden columns into existing hidden_columns configs, so users with saved column prefs saw two empty columns consuming 33+ chars of table width Fix by adding the missing headers, correcting minimums to match header widths, and backfilling default-hidden columns during config migration. Co-Authored-By: Claude Opus 4.6 (1M context) --- cmd/roborev/tui/queue_test.go | 16 ++++++++++++++-- cmd/roborev/tui/render_queue.go | 25 ++++++++++++++++++++++--- 2 files changed, 36 insertions(+), 5 deletions(-) diff --git a/cmd/roborev/tui/queue_test.go b/cmd/roborev/tui/queue_test.go index 74286010..be96bb3f 100644 --- a/cmd/roborev/tui/queue_test.go +++ b/cmd/roborev/tui/queue_test.go @@ -2210,10 +2210,22 @@ func TestMigrateColumnConfig(t *testing.T) { wantColOrder: []string{"ref", "branch", "repo", "agent", "queued", "elapsed", "status", "pf", "closed"}, }, { - name: "existing hidden_columns preserved without backfill", + name: "existing hidden_columns backfills new defaults", hiddenCols: []string{"branch"}, + wantDirty: true, + wantHidden: []string{"branch", "session_id", "requested_model", "requested_provider"}, + }, + { + name: "hidden_columns already has new defaults", + hiddenCols: []string{"session_id", "requested_model", "requested_provider"}, + wantDirty: false, + wantHidden: []string{"session_id", "requested_model", "requested_provider"}, + }, + { + name: "sentinel preserves show-all intent", + hiddenCols: []string{"_"}, wantDirty: false, - wantHidden: []string{"branch"}, + wantHidden: []string{"_"}, }, } diff --git a/cmd/roborev/tui/render_queue.go b/cmd/roborev/tui/render_queue.go index fee7f2be..3524ed93 100644 --- a/cmd/roborev/tui/render_queue.go +++ b/cmd/roborev/tui/render_queue.go @@ -263,7 +263,7 @@ func (m model) renderQueueView() string { visCols := m.visibleColumns() // Compute per-column max content widths, using cache when data hasn't changed. - allHeaders := [colCount]string{"", "JobID", "Ref", "Branch", "Repo", "Agent", "Queued", "Elapsed", "Status", "P/F", "Closed", "Session"} + allHeaders := [colCount]string{"", "JobID", "Ref", "Branch", "Repo", "Agent", "Queued", "Elapsed", "Status", "P/F", "Closed", "Session", "Req Model", "Req Provider"} allFullRows := make([][]string, len(visibleJobList)) for i, job := range visibleJobList { cells := m.jobCells(job) @@ -320,8 +320,8 @@ func (m model) renderQueueView() string { colHandled: max(contentWidth[colHandled], 6), // "Closed" header = 6 colAgent: min(max(contentWidth[colAgent], 5), 12), // "Agent" header = 5, cap at 12 colSessionID: min(max(contentWidth[colSessionID], 7), 12), // "Session" header = 7, cap at 12 - colRequestedModel: min(max(contentWidth[colRequestedModel], 15), 24), // "Req Model" header = 9 - colRequestedProvider: min(max(contentWidth[colRequestedProvider], 18), 24), // "Req Provider" header = 12 + colRequestedModel: min(max(contentWidth[colRequestedModel], 9), 24), // "Req Model" header = 9 + colRequestedProvider: min(max(contentWidth[colRequestedProvider], 12), 24), // "Req Provider" header = 12 } // Flexible columns absorb excess space @@ -782,6 +782,25 @@ func migrateColumnConfig(cfg *config.Config) bool { break } } + + // Backfill new default-hidden columns into existing configs. + // When hidden_columns is explicitly set (non-nil, non-sentinel), + // columns added after the config was saved won't be in the list + // and will appear visible — stealing width from flex columns. + if len(cfg.HiddenColumns) > 0 && + cfg.HiddenColumns[0] != config.HiddenColumnsNoneSentinel { + for col, hide := range defaultHiddenColumns { + if !hide { + continue + } + name := columnConfigNames[col] + if !slices.Contains(cfg.HiddenColumns, name) { + cfg.HiddenColumns = append(cfg.HiddenColumns, name) + dirty = true + } + } + } + return dirty } From a8104e0ca46ef7eeb2b496c28fd9df937f35e371 Mon Sep 17 00:00:00 2001 From: Wes McKinney Date: Mon, 30 Mar 2026 08:49:24 -0500 Subject: [PATCH 2/7] test(tui): add column metadata smoke tests Add three invariant tests that catch the class of bug introduced in d2d671f6 where new columns were added to the enum but metadata structures were not fully updated: - TestColumnMetadataComplete: every toggleable column must have entries in columnNames and columnConfigNames - TestAllColumnsVisibleHeadersPresent: renders with all columns visible and verifies each header appears (catches missing allHeaders entries) - TestQueueRenderWithStaleHiddenConfig: simulates a pre-upgrade config and verifies migration backfills new default-hidden columns so flex columns retain usable widths All three tests were verified to fail against the pre-fix code and pass after the fix. Co-Authored-By: Claude Opus 4.6 (1M context) --- cmd/roborev/tui/queue_test.go | 115 ++++++++++++++++++++++++++++++++++ 1 file changed, 115 insertions(+) diff --git a/cmd/roborev/tui/queue_test.go b/cmd/roborev/tui/queue_test.go index be96bb3f..ee1f6c22 100644 --- a/cmd/roborev/tui/queue_test.go +++ b/cmd/roborev/tui/queue_test.go @@ -2290,3 +2290,118 @@ func TestDefaultHiddenColumnsIncludeRequestedFields(t *testing.T) { assert.True(t, hidden[colRequestedModel]) assert.True(t, hidden[colRequestedProvider]) } + +// --- Column smoke tests --- +// +// These invariant tests prevent a class of bug where new columns +// are added to the column enum but one or more metadata structures +// (allHeaders, columnNames, columnConfigNames, fixedWidth, +// defaultHiddenColumns, migrateColumnConfig) are not updated. + +// TestColumnMetadataComplete verifies every toggleable column has +// entries in all required metadata maps. +func TestColumnMetadataComplete(t *testing.T) { + assert := assert.New(t) + + // toggleableColumns should cover all columns except colSel + // and colJobID (which are always visible). + assert.Len(toggleableColumns, colCount-2, + "toggleableColumns count must equal colCount-2; "+ + "did you add a column to the enum without adding "+ + "it to toggleableColumns?") + + for _, col := range toggleableColumns { + assert.NotEmpty(columnNames[col], + "column %d missing from columnNames", col) + assert.NotEmpty(columnConfigNames[col], + "column %d missing from columnConfigNames", col) + } +} + +// TestAllColumnsVisibleHeadersPresent renders the queue with every +// column visible and checks that each column header appears in the +// rendered output. Catches missing entries in the allHeaders array +// inside renderQueueView. +func TestAllColumnsVisibleHeadersPresent(t *testing.T) { + m := newModel(localhostEndpoint, withExternalIODisabled()) + m.width = 300 // wide enough for all columns + m.height = 20 + m.hiddenColumns = map[int]bool{} // show all + m.jobs = []storage.ReviewJob{ + makeJob(1, withRef("abc1234"), + withRepoName("repo"), withAgent("test")), + } + m.selectedIdx = 0 + m.selectedJobID = 1 + + output := m.renderQueueView() + + // Find the header line (contains "Status" and "P/F") + var headerLine string + for line := range strings.SplitSeq(output, "\n") { + stripped := stripTestANSI(line) + if strings.Contains(stripped, "Status") && + strings.Contains(stripped, "P/F") { + headerLine = stripped + break + } + } + require.NotEmpty(t, headerLine, "could not find header line") + + for _, col := range toggleableColumns { + name := columnNames[col] + assert.Contains(t, headerLine, name, + "header missing column %q (col=%d)", name, col) + } +} + +// TestQueueRenderWithStaleHiddenConfig simulates an existing user +// whose hidden_columns config predates newly added columns. After +// migration, new default-hidden columns must be hidden and flex +// columns (Ref, Branch, Repo) must retain usable widths. +func TestQueueRenderWithStaleHiddenConfig(t *testing.T) { + // Pre-upgrade config: only session_id hidden (predates new cols) + staleCfg := &config.Config{ + HiddenColumns: []string{"session_id"}, + } + migrateColumnConfig(staleCfg) + migratedHidden := parseHiddenColumns(staleCfg.HiddenColumns) + + // Every default-hidden column must be hidden after migration + for col, hide := range defaultHiddenColumns { + if hide { + assert.True(t, migratedHidden[col], + "column %q should be hidden after migration", + columnConfigNames[col]) + } + } + + // Render with migrated config — flex columns must not be + // starved by phantom visible columns + m := newModel(localhostEndpoint, withExternalIODisabled()) + m.width = 120 + m.height = 20 + m.hiddenColumns = migratedHidden + m.jobs = []storage.ReviewJob{ + makeJob(1, + withRef("abc1234"), + withRepoName("my-project"), + withBranch("feature/branch"), + withAgent("test"), + ), + } + m.selectedIdx = 0 + m.selectedJobID = 1 + + output := m.renderQueueView() + found := false + for line := range strings.SplitSeq(output, "\n") { + if strings.Contains(stripTestANSI(line), "my-project") { + found = true + break + } + } + assert.True(t, found, + "repo name should not be truncated with migrated config "+ + "at width=120") +} From 1d874a904b773466a16639a0910c04a191ff5998 Mon Sep 17 00:00:00 2001 From: Wes McKinney Date: Mon, 30 Mar 2026 08:55:18 -0500 Subject: [PATCH 3/7] fix(tui): use versioned migration for hidden column backfill MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address review findings from #11439 and #11445: - Add ColumnConfigVersion to Config so the backfill migration runs exactly once. A user who later unhides columns won't get them re-hidden on next startup (version >= 1 skips the migration). - Iterate toggleableColumns (a slice) instead of defaultHiddenColumns (a map) for deterministic append order. - Remove the else-if branch that bumped version for configs without explicit hidden_columns — those use parseHiddenColumns(nil) defaults and need no migration. - Add TestQueueRenderPostMigrationUserChoice to verify post-migration configs are not re-migrated. - Add "version 1 config not re-migrated" case to TestMigrateColumnConfig. Co-Authored-By: Claude Opus 4.6 (1M context) --- cmd/roborev/tui/queue_test.go | 51 ++++++++++++++++++++++++++------- cmd/roborev/tui/render_queue.go | 21 ++++++++------ internal/config/config.go | 1 + 3 files changed, 53 insertions(+), 20 deletions(-) diff --git a/cmd/roborev/tui/queue_test.go b/cmd/roborev/tui/queue_test.go index ee1f6c22..0fe17a75 100644 --- a/cmd/roborev/tui/queue_test.go +++ b/cmd/roborev/tui/queue_test.go @@ -2163,9 +2163,11 @@ func TestMigrateColumnConfig(t *testing.T) { name string columnOrder []string hiddenCols []string + version int wantDirty bool wantColOrder []string wantHidden []string + wantVersion int }{ { name: "nil config unchanged", @@ -2210,16 +2212,18 @@ func TestMigrateColumnConfig(t *testing.T) { wantColOrder: []string{"ref", "branch", "repo", "agent", "queued", "elapsed", "status", "pf", "closed"}, }, { - name: "existing hidden_columns backfills new defaults", - hiddenCols: []string{"branch"}, - wantDirty: true, - wantHidden: []string{"branch", "session_id", "requested_model", "requested_provider"}, + name: "stale hidden_columns backfills new defaults", + hiddenCols: []string{"branch"}, + wantDirty: true, + wantHidden: []string{"branch", "session_id", "requested_model", "requested_provider"}, + wantVersion: 1, }, { - name: "hidden_columns already has new defaults", - hiddenCols: []string{"session_id", "requested_model", "requested_provider"}, - wantDirty: false, - wantHidden: []string{"session_id", "requested_model", "requested_provider"}, + name: "hidden_columns already has new defaults", + hiddenCols: []string{"session_id", "requested_model", "requested_provider"}, + wantDirty: true, + wantHidden: []string{"session_id", "requested_model", "requested_provider"}, + wantVersion: 1, }, { name: "sentinel preserves show-all intent", @@ -2227,18 +2231,28 @@ func TestMigrateColumnConfig(t *testing.T) { wantDirty: false, wantHidden: []string{"_"}, }, + { + name: "version 1 config not re-migrated", + hiddenCols: []string{"branch"}, + version: 1, + wantDirty: false, + wantHidden: []string{"branch"}, + wantVersion: 1, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { cfg := &config.Config{ - ColumnOrder: slices.Clone(tt.columnOrder), - HiddenColumns: slices.Clone(tt.hiddenCols), + ColumnOrder: slices.Clone(tt.columnOrder), + HiddenColumns: slices.Clone(tt.hiddenCols), + ColumnConfigVersion: tt.version, } dirty := migrateColumnConfig(cfg) assert.Equal(t, tt.wantDirty, dirty) assert.True(t, slices.Equal(cfg.ColumnOrder, tt.wantColOrder)) assert.True(t, slices.Equal(cfg.HiddenColumns, tt.wantHidden)) + assert.Equal(t, tt.wantVersion, cfg.ColumnConfigVersion) }) } } @@ -2360,7 +2374,7 @@ func TestAllColumnsVisibleHeadersPresent(t *testing.T) { // migration, new default-hidden columns must be hidden and flex // columns (Ref, Branch, Repo) must retain usable widths. func TestQueueRenderWithStaleHiddenConfig(t *testing.T) { - // Pre-upgrade config: only session_id hidden (predates new cols) + // Pre-upgrade config (version 0): only session_id hidden staleCfg := &config.Config{ HiddenColumns: []string{"session_id"}, } @@ -2405,3 +2419,18 @@ func TestQueueRenderWithStaleHiddenConfig(t *testing.T) { "repo name should not be truncated with migrated config "+ "at width=120") } + +// TestQueueRenderPostMigrationUserChoice verifies that after the +// version-1 migration, a user who explicitly unhides new columns +// keeps their choice on subsequent startups. +func TestQueueRenderPostMigrationUserChoice(t *testing.T) { + // Post-migration config: user unhid all default-hidden columns + postCfg := &config.Config{ + HiddenColumns: []string{"branch"}, + ColumnConfigVersion: 1, + } + dirty := migrateColumnConfig(postCfg) + assert.False(t, dirty, "version-1 config should not be re-migrated") + assert.Equal(t, []string{"branch"}, postCfg.HiddenColumns, + "user's explicit column choices must be preserved") +} diff --git a/cmd/roborev/tui/render_queue.go b/cmd/roborev/tui/render_queue.go index 3524ed93..e077651f 100644 --- a/cmd/roborev/tui/render_queue.go +++ b/cmd/roborev/tui/render_queue.go @@ -783,22 +783,25 @@ func migrateColumnConfig(cfg *config.Config) bool { } } - // Backfill new default-hidden columns into existing configs. - // When hidden_columns is explicitly set (non-nil, non-sentinel), - // columns added after the config was saved won't be in the list - // and will appear visible — stealing width from flex columns. - if len(cfg.HiddenColumns) > 0 && + // Version 1: backfill requested-model/provider columns into + // existing hidden_columns configs. Only runs once — the version + // marker prevents re-hiding columns a user later unhides. + if cfg.ColumnConfigVersion < 1 && + len(cfg.HiddenColumns) > 0 && cfg.HiddenColumns[0] != config.HiddenColumnsNoneSentinel { - for col, hide := range defaultHiddenColumns { - if !hide { + for _, col := range toggleableColumns { + if !defaultHiddenColumns[col] { continue } name := columnConfigNames[col] if !slices.Contains(cfg.HiddenColumns, name) { - cfg.HiddenColumns = append(cfg.HiddenColumns, name) - dirty = true + cfg.HiddenColumns = append( + cfg.HiddenColumns, name, + ) } } + cfg.ColumnConfigVersion = 1 + dirty = true } return dirty diff --git a/internal/config/config.go b/internal/config/config.go index ee56d4c8..fea4e898 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -176,6 +176,7 @@ type Config struct { ColumnBorders bool `toml:"column_borders" comment:"Show column borders in the TUI queue."` // Show ▕ separators between columns ColumnOrder []string `toml:"column_order" comment:"Custom queue column order in the TUI."` // Custom queue column display order TaskColumnOrder []string `toml:"task_column_order" comment:"Custom Tasks column order in the TUI."` // Custom task column display order + ColumnConfigVersion int `toml:"column_config_version"` // Tracks column migration version to avoid re-running one-shot migrations // Advanced feature flags Advanced AdvancedConfig `toml:"advanced"` From 6dc03a041413b57bc5fcdb2eacb904f7d0ef157d Mon Sep 17 00:00:00 2001 From: Wes McKinney Date: Mon, 30 Mar 2026 09:00:59 -0500 Subject: [PATCH 4/7] fix(tui): stamp ColumnConfigVersion on save to prevent re-migration MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit saveColumnOptions now writes ColumnConfigVersion = 1 alongside the user's column preferences. Without this, a user starting from a version-0 config (sentinel or nil hidden_columns) could save custom column choices, but the next startup would treat the saved config as stale and backfill default-hidden columns — undoing the user's choice. Add TestSaveColumnOptionStampsVersion to verify the save-then-reload cycle preserves preferences. Co-Authored-By: Claude Opus 4.6 (1M context) --- cmd/roborev/tui/queue_test.go | 33 +++++++++++++++++++++++++++++++++ cmd/roborev/tui/render_queue.go | 1 + 2 files changed, 34 insertions(+) diff --git a/cmd/roborev/tui/queue_test.go b/cmd/roborev/tui/queue_test.go index 0fe17a75..f8296545 100644 --- a/cmd/roborev/tui/queue_test.go +++ b/cmd/roborev/tui/queue_test.go @@ -2434,3 +2434,36 @@ func TestQueueRenderPostMigrationUserChoice(t *testing.T) { assert.Equal(t, []string{"branch"}, postCfg.HiddenColumns, "user's explicit column choices must be preserved") } + +// TestSaveColumnOptionStampsVersion simulates the save-then-reload +// cycle: a user starts from a version-0 config (e.g., sentinel or +// nil hidden_columns), customizes column visibility through the TUI +// options modal, and saves. On next startup, the saved config must +// not be re-migrated — saveColumnOptions stamps ColumnConfigVersion. +func TestSaveColumnOptionStampsVersion(t *testing.T) { + // Simulate what saveColumnOptions writes: the user chose to hide + // only "branch", showing all default-hidden columns. + // saveColumnOptions now stamps ColumnConfigVersion = 1. + savedCfg := &config.Config{ + HiddenColumns: []string{"branch"}, + ColumnConfigVersion: 1, // stamped by saveColumnOptions + } + + // Next startup: migrateColumnConfig must not backfill + dirty := migrateColumnConfig(savedCfg) + assert.False(t, dirty) + assert.Equal(t, []string{"branch"}, savedCfg.HiddenColumns, + "saved column preferences must survive reload") + assert.Equal(t, 1, savedCfg.ColumnConfigVersion) + + // Contrast: without the version stamp, migration would backfill + unstampedCfg := &config.Config{ + HiddenColumns: []string{"branch"}, + ColumnConfigVersion: 0, // missing stamp (the bug) + } + dirty = migrateColumnConfig(unstampedCfg) + assert.True(t, dirty, + "version-0 config with explicit hidden_columns must "+ + "trigger migration") + assert.Equal(t, 1, unstampedCfg.ColumnConfigVersion) +} diff --git a/cmd/roborev/tui/render_queue.go b/cmd/roborev/tui/render_queue.go index e077651f..83674997 100644 --- a/cmd/roborev/tui/render_queue.go +++ b/cmd/roborev/tui/render_queue.go @@ -1016,6 +1016,7 @@ func (m model) saveColumnOptions() tea.Cmd { cfg.ColumnOrder = colOrd cfg.TaskColumnOrder = taskColOrd cfg.Advanced.TasksEnabled = tasksEnabled + cfg.ColumnConfigVersion = 1 if err := config.SaveGlobal(cfg); err != nil { return configSaveErrMsg{err: fmt.Errorf("save config: %w", err)} } From 6a1e52c42b5cedb4633668d3796ac1e1aa308bcf Mon Sep 17 00:00:00 2001 From: Wes McKinney Date: Mon, 30 Mar 2026 10:42:02 -0500 Subject: [PATCH 5/7] test(tui): add end-to-end save path and pre-v1 migration tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - TestSaveColumnOptionsWritesVersion: exercises the real saveColumnOptions → LoadGlobal/SaveGlobal path via ROBOREV_DATA_DIR, verifying ColumnConfigVersion = 1 is persisted and survives reload - TestMigratePreV1ConfigWithVisibleNewColumns: documents that the v1 migration backfills default-hidden columns for unversioned configs, and verifies the version stamp prevents re-migration on second run Co-Authored-By: Claude Opus 4.6 (1M context) --- cmd/roborev/tui/queue_test.go | 63 +++++++++++++++++++++++++++++++++++ 1 file changed, 63 insertions(+) diff --git a/cmd/roborev/tui/queue_test.go b/cmd/roborev/tui/queue_test.go index f8296545..0b09c769 100644 --- a/cmd/roborev/tui/queue_test.go +++ b/cmd/roborev/tui/queue_test.go @@ -2467,3 +2467,66 @@ func TestSaveColumnOptionStampsVersion(t *testing.T) { "trigger migration") assert.Equal(t, 1, unstampedCfg.ColumnConfigVersion) } + +// TestSaveColumnOptionsWritesVersion exercises the real +// saveColumnOptions → config.LoadGlobal/SaveGlobal path and +// verifies ColumnConfigVersion is persisted. +func TestSaveColumnOptionsWritesVersion(t *testing.T) { + tmpDir := t.TempDir() + t.Setenv("ROBOREV_DATA_DIR", tmpDir) + + // Seed a version-0 config with sentinel hidden_columns + seedCfg := &config.Config{ + HiddenColumns: []string{config.HiddenColumnsNoneSentinel}, + } + require.NoError(t, config.SaveGlobal(seedCfg)) + + // Build a model that will save column preferences + m := newModel(localhostEndpoint, withExternalIODisabled()) + m.hiddenColumns = map[int]bool{colBranch: true} + + // Execute the save command + cmd := m.saveColumnOptions() + msg := cmd() + assert.Nil(t, msg, "saveColumnOptions should succeed") + + // Reload and verify version was stamped + loaded, err := config.LoadGlobal() + require.NoError(t, err) + assert.Equal(t, 1, loaded.ColumnConfigVersion, + "ColumnConfigVersion must be persisted by saveColumnOptions") + + // Verify the saved config survives migration without changes + dirty := migrateColumnConfig(loaded) + assert.False(t, dirty, + "freshly saved config must not trigger migration") +} + +// TestMigratePreV1ConfigWithVisibleNewColumns documents the +// migration behavior for the edge case where a user was briefly on +// the buggy version (d2d671f6) and explicitly saved preferences +// showing the new columns before ColumnConfigVersion existed. +// +// The migration cannot distinguish this from a stale config, so it +// backfills. This is the correct tradeoff: the buggy window was +// brief and the columns being visible caused broken layouts. +func TestMigratePreV1ConfigWithVisibleNewColumns(t *testing.T) { + // User saved ["branch"] during buggy window, intending to show + // requested_model and requested_provider. No version stamp. + cfg := &config.Config{ + HiddenColumns: []string{"branch"}, + } + + dirty := migrateColumnConfig(cfg) + assert.True(t, dirty) + assert.Equal(t, 1, cfg.ColumnConfigVersion) + + // Migration backfills — user must re-unhide via options modal + hidden := parseHiddenColumns(cfg.HiddenColumns) + assert.True(t, hidden[colRequestedModel]) + assert.True(t, hidden[colRequestedProvider]) + + // Second run: version stamp prevents re-migration + dirty = migrateColumnConfig(cfg) + assert.False(t, dirty) +} From 535820a00ba72a54d2b2e3e51e4c07b2200f1bdb Mon Sep 17 00:00:00 2001 From: Wes McKinney Date: Mon, 30 Mar 2026 12:47:42 -0500 Subject: [PATCH 6/7] fix(tui): scope v1 migration to only newly introduced columns MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The backfill loop was iterating all defaultHiddenColumns, which includes session_id — a column that existed before d2d671f6. A user with hidden_columns = ["branch"] may have intentionally shown session_id; the migration should not override that choice. Scope the backfill to only colRequestedModel and colRequestedProvider (the columns actually introduced in d2d671f6). Add a test case verifying session_id visibility is preserved during migration. Co-Authored-By: Claude Opus 4.6 (1M context) --- cmd/roborev/tui/queue_test.go | 22 +++++++++++++--------- cmd/roborev/tui/render_queue.go | 12 +++++------- 2 files changed, 18 insertions(+), 16 deletions(-) diff --git a/cmd/roborev/tui/queue_test.go b/cmd/roborev/tui/queue_test.go index 0b09c769..8fd2ddb6 100644 --- a/cmd/roborev/tui/queue_test.go +++ b/cmd/roborev/tui/queue_test.go @@ -2212,9 +2212,16 @@ func TestMigrateColumnConfig(t *testing.T) { wantColOrder: []string{"ref", "branch", "repo", "agent", "queued", "elapsed", "status", "pf", "closed"}, }, { - name: "stale hidden_columns backfills new defaults", + name: "stale hidden_columns backfills only new columns", hiddenCols: []string{"branch"}, wantDirty: true, + wantHidden: []string{"branch", "requested_model", "requested_provider"}, + wantVersion: 1, + }, + { + name: "visible session_id preserved during backfill", + hiddenCols: []string{"branch", "session_id"}, + wantDirty: true, wantHidden: []string{"branch", "session_id", "requested_model", "requested_provider"}, wantVersion: 1, }, @@ -2381,14 +2388,11 @@ func TestQueueRenderWithStaleHiddenConfig(t *testing.T) { migrateColumnConfig(staleCfg) migratedHidden := parseHiddenColumns(staleCfg.HiddenColumns) - // Every default-hidden column must be hidden after migration - for col, hide := range defaultHiddenColumns { - if hide { - assert.True(t, migratedHidden[col], - "column %q should be hidden after migration", - columnConfigNames[col]) - } - } + // New columns must be hidden after migration + assert.True(t, migratedHidden[colRequestedModel], + "requested_model should be hidden after migration") + assert.True(t, migratedHidden[colRequestedProvider], + "requested_provider should be hidden after migration") // Render with migrated config — flex columns must not be // starved by phantom visible columns diff --git a/cmd/roborev/tui/render_queue.go b/cmd/roborev/tui/render_queue.go index 83674997..2883d3fb 100644 --- a/cmd/roborev/tui/render_queue.go +++ b/cmd/roborev/tui/render_queue.go @@ -783,16 +783,14 @@ func migrateColumnConfig(cfg *config.Config) bool { } } - // Version 1: backfill requested-model/provider columns into - // existing hidden_columns configs. Only runs once — the version - // marker prevents re-hiding columns a user later unhides. + // Version 1: backfill only the columns introduced in d2d671f6. + // Do not touch session_id — it was already a default-hidden + // column, so its absence from the list is a deliberate choice. if cfg.ColumnConfigVersion < 1 && len(cfg.HiddenColumns) > 0 && cfg.HiddenColumns[0] != config.HiddenColumnsNoneSentinel { - for _, col := range toggleableColumns { - if !defaultHiddenColumns[col] { - continue - } + v1NewColumns := []int{colRequestedModel, colRequestedProvider} + for _, col := range v1NewColumns { name := columnConfigNames[col] if !slices.Contains(cfg.HiddenColumns, name) { cfg.HiddenColumns = append( From e6b91ae7fecdff32e7e1de872fdbc6f3c76df291 Mon Sep 17 00:00:00 2001 From: Wes McKinney Date: Mon, 30 Mar 2026 13:32:52 -0500 Subject: [PATCH 7/7] test(tui): verify persisted HiddenColumns in save-path test Assert the reloaded HiddenColumns value matches the user's selection, not just the version stamp. Co-Authored-By: Claude Opus 4.6 (1M context) --- cmd/roborev/tui/queue_test.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/cmd/roborev/tui/queue_test.go b/cmd/roborev/tui/queue_test.go index 8fd2ddb6..a9d2da18 100644 --- a/cmd/roborev/tui/queue_test.go +++ b/cmd/roborev/tui/queue_test.go @@ -2494,11 +2494,13 @@ func TestSaveColumnOptionsWritesVersion(t *testing.T) { msg := cmd() assert.Nil(t, msg, "saveColumnOptions should succeed") - // Reload and verify version was stamped + // Reload and verify both version and hidden columns were persisted loaded, err := config.LoadGlobal() require.NoError(t, err) assert.Equal(t, 1, loaded.ColumnConfigVersion, "ColumnConfigVersion must be persisted by saveColumnOptions") + assert.Equal(t, []string{"branch"}, loaded.HiddenColumns, + "HiddenColumns must match the user's selection") // Verify the saved config survives migration without changes dirty := migrateColumnConfig(loaded)