refactor: deduplicate column order helpers and fix review findings

mariusvniekerk · claude · mariusvniekerk · commit 6a64a15139f5 · 2026-03-01T07:04:28.000-05:00
- Extract resolveColumnOrder/serializeColumnOrder/lookupDisplayName as
  shared generic functions; task and queue variants now delegate to them
- Restore allHeaders to static array instead of per-render map allocation
- Split down/up into separate cases instead of combined case with re-check
- Use isColumn() guard instead of fragile lastColumnIdx = len - 2
- Move tasks-view no-op guard before the toggle to avoid toggle-then-revert
- Clarify help footer: up/dn navigate vs j/k reorder
- Pre-allocate slices in visibleTaskColumns and syncColumnOrderFromOptions

Co-Authored-By: Claude Opus 4.6 &lt;noreply@anthropic.com&gt;
diff --git a/cmd/roborev/tui/handlers_queue.go b/cmd/roborev/tui/handlers_queue.go
@@ -192,28 +192,29 @@ func (m model) handleColumnOptionsKey() (tea.Model, tea.Cmd) {
 }
 
 func (m model) handleColumnOptionsInput(msg tea.KeyMsg) (tea.Model, tea.Cmd) {
-	lastColumnIdx := len(m.colOptionsList) - 2 // last column item (before borders)
+	isColumn := func(idx int) bool {
+		return idx >= 0 && idx < len(m.colOptionsList) && m.colOptionsList[idx].id != colOptionBorders
+	}
 
 	switch msg.String() {
 	case "esc":
 		m.currentView = m.colOptionsReturnView
 		return m, nil
 	case "ctrl+c":
 		return m, tea.Quit
-	case "down", "up":
-		if msg.String() == "down" {
-			if m.colOptionsIdx < len(m.colOptionsList)-1 {
-				m.colOptionsIdx++
-			}
-		} else {
-			if m.colOptionsIdx > 0 {
-				m.colOptionsIdx--
-			}
+	case "down":
+		if m.colOptionsIdx < len(m.colOptionsList)-1 {
+			m.colOptionsIdx++
+		}
+		return m, nil
+	case "up":
+		if m.colOptionsIdx > 0 {
+			m.colOptionsIdx--
 		}
 		return m, nil
 	case "j":
 		// Move current column down in order
-		if m.colOptionsIdx >= 0 && m.colOptionsIdx < lastColumnIdx {
+		if isColumn(m.colOptionsIdx) && isColumn(m.colOptionsIdx+1) {
 			m.colOptionsList[m.colOptionsIdx], m.colOptionsList[m.colOptionsIdx+1] =
 				m.colOptionsList[m.colOptionsIdx+1], m.colOptionsList[m.colOptionsIdx]
 			m.colOptionsIdx++
@@ -223,7 +224,7 @@ func (m model) handleColumnOptionsInput(msg tea.KeyMsg) (tea.Model, tea.Cmd) {
 		return m, nil
 	case "k":
 		// Move current column up in order
-		if m.colOptionsIdx > 0 && m.colOptionsIdx <= lastColumnIdx {
+		if isColumn(m.colOptionsIdx) && isColumn(m.colOptionsIdx-1) {
 			m.colOptionsList[m.colOptionsIdx], m.colOptionsList[m.colOptionsIdx-1] =
 				m.colOptionsList[m.colOptionsIdx-1], m.colOptionsList[m.colOptionsIdx]
 			m.colOptionsIdx--
@@ -234,16 +235,14 @@ func (m model) handleColumnOptionsInput(msg tea.KeyMsg) (tea.Model, tea.Cmd) {
 	case " ", "enter":
 		if m.colOptionsIdx >= 0 && m.colOptionsIdx < len(m.colOptionsList) {
 			opt := &m.colOptionsList[m.colOptionsIdx]
-			opt.enabled = !opt.enabled
 			if opt.id == colOptionBorders {
-				// Borders toggle
+				opt.enabled = !opt.enabled
 				m.colBordersOn = opt.enabled
+			} else if m.colOptionsReturnView == viewTasks {
+				// Tasks view: no visibility toggle (all columns always shown)
+				return m, nil
 			} else {
-				if m.colOptionsReturnView == viewTasks {
-					// Tasks view: no visibility toggle (all columns always shown)
-					opt.enabled = true
-					return m, nil
-				}
+				opt.enabled = !opt.enabled
 				if opt.enabled {
 					delete(m.hiddenColumns, opt.id)
 				} else {
@@ -263,7 +262,7 @@ func (m model) handleColumnOptionsInput(msg tea.KeyMsg) (tea.Model, tea.Cmd) {
 // syncColumnOrderFromOptions updates m.columnOrder or m.taskColumnOrder
 // from the current colOptionsList (excluding the borders toggle).
 func (m *model) syncColumnOrderFromOptions() {
-	var order []int
+	order := make([]int, 0, len(m.colOptionsList))
 	for _, opt := range m.colOptionsList {
 		if opt.id != colOptionBorders {
 			order = append(order, opt.id)
diff --git a/cmd/roborev/tui/render_queue.go b/cmd/roborev/tui/render_queue.go
@@ -667,14 +667,19 @@ var columnConfigNames = map[int]string{
 	colHandled: "handled",
 }
 
-// columnDisplayName returns the display name for a column constant.
-func columnDisplayName(col int) string {
-	if name, ok := columnNames[col]; ok {
+// lookupDisplayName returns the display name for a column constant from the given map.
+func lookupDisplayName(col int, displayNames map[int]string) string {
+	if name, ok := displayNames[col]; ok {
 		return name
 	}
 	return "?"
 }
 
+// columnDisplayName returns the display name for a queue column constant.
+func columnDisplayName(col int) string {
+	return lookupDisplayName(col, columnNames)
+}
+
 // parseHiddenColumns converts config hidden_columns strings to column ID set.
 func parseHiddenColumns(names []string) map[int]bool {
 	result := map[int]bool{}
@@ -702,16 +707,16 @@ func hiddenColumnsToNames(hidden map[int]bool) []string {
 	return names
 }
 
-// parseColumnOrder converts config names to ordered queue column IDs.
-// Any toggleable columns not in the config list are appended at the end in default order.
-func parseColumnOrder(names []string) []int {
+// resolveColumnOrder converts config names to ordered column IDs using the given
+// configNames map. Any columns from defaults not in names are appended at the end.
+func resolveColumnOrder(names []string, configNames map[int]string, defaults []int) []int {
 	if len(names) == 0 {
-		result := make([]int, len(toggleableColumns))
-		copy(result, toggleableColumns)
+		result := make([]int, len(defaults))
+		copy(result, defaults)
 		return result
 	}
 	lookup := map[string]int{}
-	for id, name := range columnConfigNames {
+	for id, name := range configNames {
 		lookup[name] = id
 	}
 	seen := map[int]bool{}
@@ -722,26 +727,35 @@ func parseColumnOrder(names []string) []int {
 			seen[id] = true
 		}
 	}
-	// Append any missing toggleable columns
-	for _, col := range toggleableColumns {
+	for _, col := range defaults {
 		if !seen[col] {
 			order = append(order, col)
 		}
 	}
 	return order
 }
 
-// columnOrderToNames converts ordered queue column IDs to config names.
-func columnOrderToNames(order []int) []string {
+// serializeColumnOrder converts ordered column IDs to config names.
+func serializeColumnOrder(order []int, configNames map[int]string) []string {
 	names := make([]string, 0, len(order))
 	for _, col := range order {
-		if name, ok := columnConfigNames[col]; ok {
+		if name, ok := configNames[col]; ok {
 			names = append(names, name)
 		}
 	}
 	return names
 }
 
+// parseColumnOrder converts config names to ordered queue column IDs.
+func parseColumnOrder(names []string) []int {
+	return resolveColumnOrder(names, columnConfigNames, toggleableColumns)
+}
+
+// columnOrderToNames converts ordered queue column IDs to config names.
+func columnOrderToNames(order []int) []string {
+	return serializeColumnOrder(order, columnConfigNames)
+}
+
 // visibleColumns returns the ordered list of column indices to display,
 // always including colSel and colJobID, plus any non-hidden toggleable columns.
 func (m model) visibleColumns() []int {
@@ -803,7 +817,7 @@ func (m model) renderColumnOptionsView() string {
 
 	b.WriteString("\n")
 	helpRows := [][]helpItem{
-		{{"j/k", "move"}, {"space", "toggle"}, {"esc", "close"}},
+		{{"↑/↓", "navigate"}, {"j/k", "reorder"}, {"space", "toggle"}, {"esc", "close"}},
 	}
 	b.WriteString(renderHelpTable(helpRows, m.width))
 
diff --git a/cmd/roborev/tui/render_tasks.go b/cmd/roborev/tui/render_tasks.go
@@ -55,55 +55,23 @@ var taskColumnConfigNames = map[int]string{
 
 // taskColumnDisplayName returns the display name for a task column constant.
 func taskColumnDisplayName(col int) string {
-	if name, ok := taskColumnNames[col]; ok {
-		return name
-	}
-	return "?"
+	return lookupDisplayName(col, taskColumnNames)
 }
 
 // parseTaskColumnOrder converts config names to ordered task column IDs.
-// Any columns not in the config list are appended at the end in default order.
 func parseTaskColumnOrder(names []string) []int {
-	if len(names) == 0 {
-		result := make([]int, len(taskToggleableColumns))
-		copy(result, taskToggleableColumns)
-		return result
-	}
-	lookup := map[string]int{}
-	for id, name := range taskColumnConfigNames {
-		lookup[name] = id
-	}
-	seen := map[int]bool{}
-	var order []int
-	for _, n := range names {
-		if id, ok := lookup[strings.ToLower(n)]; ok && !seen[id] {
-			order = append(order, id)
-			seen[id] = true
-		}
-	}
-	// Append any missing toggleable columns
-	for _, col := range taskToggleableColumns {
-		if !seen[col] {
-			order = append(order, col)
-		}
-	}
-	return order
+	return resolveColumnOrder(names, taskColumnConfigNames, taskToggleableColumns)
 }
 
 // taskColumnOrderToNames converts ordered task column IDs to config names.
 func taskColumnOrderToNames(order []int) []string {
-	names := make([]string, 0, len(order))
-	for _, col := range order {
-		if name, ok := taskColumnConfigNames[col]; ok {
-			names = append(names, name)
-		}
-	}
-	return names
+	return serializeColumnOrder(order, taskColumnConfigNames)
 }
 
 // visibleTaskColumns returns the ordered list of task column indices to display.
 func (m model) visibleTaskColumns() []int {
-	cols := []int{tcolSel}
+	cols := make([]int, 0, 1+len(m.taskColumnOrder))
+	cols = append(cols, tcolSel)
 	cols = append(cols, m.taskColumnOrder...)
 	return cols
 }
@@ -206,11 +174,7 @@ func (m model) renderTasksView() string {
 	visCols := m.visibleTaskColumns()
 
 	// Build full row data for ALL fixJobs (stable widths across scroll).
-	allHeaders := make(map[int]string, tcolCount)
-	allHeaders[tcolSel] = ""
-	for col, name := range taskColumnNames {
-		allHeaders[col] = name
-	}
+	allHeaders := [tcolCount]string{tcolSel: "", tcolStatus: "Status", tcolJobID: "Job", tcolParent: "Parent", tcolQueued: "Queued", tcolElapsed: "Elapsed", tcolBranch: "Branch", tcolRepo: "Repo", tcolRefSubject: "Ref/Subject"}
 	allFullRows := make([][]string, len(m.fixJobs))
 	for i, job := range m.fixJobs {
 		cells := m.taskCells(job)
