From cd9bedba5584874fe9392566112b6db672c25a4f Mon Sep 17 00:00:00 2001 From: fullstackjam Date: Fri, 17 Jul 2026 22:43:45 +0800 Subject: [PATCH] fix(tui): open select screen on what the user actually chose MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Picking a loadout opened the select screen on catCur 0 — whichever catalog category happens to sit first — instead of on the packages the loadout picked. On a machine with everything already installed this left every sidebar row showing a flat grey count with no way to find the selection, and pressing space on the wrong-focused pane toggled a row with no visible cursor. Pin a synthetic "selected" review category first in the sidebar so the screen opens on the answer to its own question. Sidebar counts now separate "selected" from "still to install" instead of collapsing them, and space is a no-op while the sidebar (not the list) holds focus. --- internal/ui/tui/wizard/boot.go | 3 + internal/ui/tui/wizard/select.go | 98 ++++++++++++++++--- internal/ui/tui/wizard/wizard_test.go | 133 ++++++++++++++++++++++++++ 3 files changed, 223 insertions(+), 11 deletions(-) diff --git a/internal/ui/tui/wizard/boot.go b/internal/ui/tui/wizard/boot.go index 8dae8c1..8563a8b 100644 --- a/internal/ui/tui/wizard/boot.go +++ b/internal/ui/tui/wizard/boot.go @@ -226,6 +226,9 @@ func (m Model) pickLoadout(i int) (tea.Model, tea.Cmd) { func (m Model) enterSelect(sel map[string]bool) (tea.Model, tea.Cmd) { m.selected = sel m.screen = scrSelect + // pinReviewCat prepends "selected", so catCur 0 opens on the loadout's own + // picks rather than on whichever category happens to be first. + m.pinReviewCat() m.catCur, m.rowCur, m.scroll = 0, 0, 0 m.query, m.typing = "", false m.hoverRow = -1 diff --git a/internal/ui/tui/wizard/select.go b/internal/ui/tui/wizard/select.go index d4e6004..b18d488 100644 --- a/internal/ui/tui/wizard/select.go +++ b/internal/ui/tui/wizard/select.go @@ -15,6 +15,15 @@ import ( var searchOnline = search.SearchOnline const ( + // reviewCatName is the synthetic sidebar category the select screen opens + // on: the packages the loadout picked, gathered out of every category. The + // screen's question is "install these?" (↵ is bound to install), so the + // cursor has to land on the answer. Opening on a real category instead + // answered with whichever one sits at index 0 — packages the user never + // chose — while the ones they did chose stayed scattered behind nine other + // categories, reachable only by clicking through each in turn. + reviewCatName = "selected" + // onlineCatName is the synthetic sidebar category that holds packages // picked from openboot.dev search results (they aren't in the local // catalog, so without a home they'd vanish when the filter clears). @@ -57,6 +66,59 @@ func categoriesFromConfig(rc *config.RemoteConfig) []config.Category { return cats } +// ── the "selected" review category ── + +// reviewPackages gathers every selected package out of cats, in catalog order. +func reviewPackages(cats []config.Category, selected map[string]bool) []config.Package { + var out []config.Package + for _, c := range cats { + if c.Name == reviewCatName { + continue // it is built from the real categories; don't fold it in + } + for _, p := range c.Packages { + if selected[p.Name] { + out = append(out, p) + } + } + } + return out +} + +// pinReviewCat puts the synthetic "selected" category first in the sidebar, so +// entering the screen with catCur 0 lands on the loadout's picks. +// +// Two cases skip it. Config mode: the sidebar already *is* the config's own +// package lists with everything preselected, so a review category would just +// concatenate all of them. Hand-picking from scratch ("c" on the boot screen): +// there is nothing to review, and an empty pane says less than the catalog. +func (m *Model) pinReviewCat() { + if m.rc != nil || m.selCount() == 0 { + return + } + review := config.Category{Name: reviewCatName, Packages: reviewPackages(m.cats, m.selected)} + m.cats = append([]config.Category{review}, m.cats...) +} + +func (m Model) isReviewCat(i int) bool { + return i >= 0 && i < len(m.cats) && m.cats[i].Name == reviewCatName +} + +// setCat moves the sidebar cursor to category i and resets the list position. +// +// The review category is a snapshot taken on entry, not a live view of the +// selection. Standing in it and unchecking a row leaves the row in place with +// an empty box, so a mis-toggle costs one keypress to undo — and so "a" and +// "x" mean re-check and clear-all rather than acting on a list that empties +// itself out from under them. +func (m Model) setCat(i int) Model { + m.catCur = i + m.rowCur, m.scroll = 0, 0 + if m.isReviewCat(i) { + m.cats[i].Packages = reviewPackages(m.cats, m.selected) + } + return m +} + // ── selection helpers ── func (m Model) isInstalled(name string) bool { return m.installed[name] } @@ -98,6 +160,9 @@ func (m Model) pool() []config.Package { if q != "" { var out []config.Package for _, c := range m.cats { + if c.Name == reviewCatName { + continue // a copy of rows that live in real categories — would double every hit + } for _, p := range c.Packages { if strings.Contains(strings.ToLower(p.Name+" "+p.Description), q) { out = append(out, p) @@ -359,7 +424,11 @@ func (m Model) updateSelect(msg tea.KeyMsg) (tea.Model, tea.Cmd) { //nolint:gocy case "down", "j": m = m.selMoveDown() case " ": - if len(pool) > 0 { + // Space acts on the list cursor, and renderRow draws that cursor only + // while the list holds focus — exactly one pane shows a pointer at a + // time. With the sidebar focused there is therefore no visible target, + // and toggling rowCur anyway flipped a row the user couldn't see. + if m.selFocus == focusList && len(pool) > 0 { m.togglePkg(pool[clamp(m.rowCur, 0, last)]) } case "a": @@ -382,8 +451,7 @@ func (m Model) updateSelect(msg tea.KeyMsg) (tea.Model, tea.Cmd) { //nolint:gocy func (m Model) selMoveUp() Model { if m.selFocus == focusCats && m.query == "" { if m.catCur > 0 { - m.catCur-- - m.rowCur, m.scroll = 0, 0 + m = m.setCat(m.catCur - 1) } return m } @@ -396,8 +464,7 @@ func (m Model) selMoveUp() Model { func (m Model) selMoveDown() Model { if m.selFocus == focusCats && m.query == "" { if m.catCur < len(m.cats)-1 { - m.catCur++ - m.rowCur, m.scroll = 0, 0 + m = m.setCat(m.catCur + 1) } return m } @@ -450,9 +517,9 @@ func (m Model) updateSelectMouse(msg tea.MouseMsg) (tea.Model, tea.Cmd) { } switch kind, idx := m.selectHitTest(msg.X, msg.Y); kind { case hitCat: - m.catCur, m.query, m.typing = idx, "", false + m.query, m.typing = "", false + m = m.setCat(idx) m.selFocus = focusCats - m.rowCur, m.scroll = 0, 0 m.clearOnline() case hitPkg: m.rowCur = idx @@ -547,10 +614,19 @@ func (m Model) selectSidebar(h int) []string { q := strings.TrimSpace(m.query) for i, c := range m.cats { - selN, total := 0, len(c.Packages) + // Two facts, two channels: the count says where your selection is, the + // colour says whether any of it will actually run. Folding them into + // one number (counting only not-yet-installed packages) left a machine + // that already has everything with a flat grey total on every category + // and no way to find its own selection. + selN, todo, total := 0, 0, len(c.Packages) for _, p := range c.Packages { - if m.selected[p.Name] && !m.installed[p.Name] { - selN++ + if !m.selected[p.Name] { + continue + } + selN++ + if !m.installed[p.Name] { + todo++ } } active := i == m.catCur && q == "" @@ -569,7 +645,7 @@ func (m Model) selectSidebar(h int) []string { nameStyle = fg(cText) } } - if selN > 0 { + if todo > 0 { countStyle = fg(cAccentHi) } count := fmt.Sprintf("%d", total) diff --git a/internal/ui/tui/wizard/wizard_test.go b/internal/ui/tui/wizard/wizard_test.go index e92510f..093bb25 100644 --- a/internal/ui/tui/wizard/wizard_test.go +++ b/internal/ui/tui/wizard/wizard_test.go @@ -1,6 +1,7 @@ package wizard import ( + "fmt" "strings" "testing" @@ -572,6 +573,138 @@ func TestHoverUsesReverseVideo(t *testing.T) { assert.False(t, strings.HasSuffix(styled, rev), "the closing reset is not re-opened") } +// Picking a loadout must open the select screen on a "selected" review +// category so the cursor lands on what the user actually chose, not on +// whichever catalog category happens to sit at index 0. +func TestSelectOpensOnReviewCategory(t *testing.T) { + m := finishProbes(sized(96, 30)) + m = send(m, key("2")) // developer loadout + + require.NotEmpty(t, m.cats) + assert.Equal(t, reviewCatName, m.cats[0].Name, "review category is pinned first") + assert.Equal(t, 0, m.catCur, "cursor opens on the review category") + + want := config.GetPackagesForPreset("developer") + pool := m.pool() + assert.Len(t, pool, len(want), "review category holds exactly the loadout's picks") + for _, p := range pool { + assert.True(t, want[p.Name], "%s wasn't part of the loadout", p.Name) + } +} + +// Hand-picking from scratch has nothing to review yet, so it must fall back to +// browsing the catalog rather than opening on an empty pane. +func TestHandPickSkipsReviewCategory(t *testing.T) { + m := finishProbes(sized(96, 30)) + m = send(m, key("c")) + for _, c := range m.cats { + assert.NotEqual(t, reviewCatName, c.Name, "nothing selected yet — no review category") + } +} + +// Config-mode installs already show the config's own package lists with +// everything preselected; a review category on top would just repeat them. +func TestConfigModeSkipsReviewCategory(t *testing.T) { + rc := &config.RemoteConfig{ + Packages: config.PackageEntryList{{Name: "curl"}}, + } + m := sizedConfig(rc) + m = finishProbes(m) + for _, c := range m.cats { + assert.NotEqual(t, reviewCatName, c.Name, "config mode browses the config's own lists") + } +} + +// Space acts on the list cursor, and the pointer glyph is only drawn while the +// list holds focus (TestSelectFocusIsVisuallyIndicated). Toggling on sidebar +// focus flipped a row the user had no way to see was the target. +func TestSpaceIgnoredWhileSidebarFocused(t *testing.T) { + m := finishProbes(sized(96, 30)) + m = send(m, key("c")) // hand-pick, empty selection + m.installed = map[string]bool{} + require.Zero(t, m.selCount()) + + m = send(m, key("left")) // focus the sidebar + require.Equal(t, focusCats, m.selFocus) + m = send(m, key("space")) + assert.Zero(t, m.selCount(), "space with sidebar focused must not toggle an invisible row") + + m = send(m, key("right")) // back to the list + m = send(m, key("space")) + assert.Equal(t, 1, m.selCount(), "space with list focused toggles the cursor row as before") +} + +// The sidebar count must distinguish "selected" from "will actually run": +// folding installed-but-selected packages out of the count left every +// category on a fully up-to-date machine showing a flat, colourless total, +// with no way to tell where the loadout's picks landed. +func TestSidebarCountShowsSelectedEvenWhenAllInstalled(t *testing.T) { + m := finishProbes(sized(96, 30)) + m = send(m, key("2")) // developer loadout + + // Mark every selected package installed, so nothing is left "to do". + installed := map[string]bool{} + for name, on := range m.selected { + if on { + installed[name] = true + } + } + m.installed = installed + + rows := m.selectSidebar(28) + joined := strings.Join(rows, "\n") + assert.Contains(t, joined, fmt.Sprintf("%d/%d", len(installed), len(installed)), + "review category must still report its full selected count, got:\n%s", joined) +} + +// The review category is a snapshot taken on entry; re-entering it after a +// selection change must refresh, not keep showing stale rows. +func TestReviewCategoryRefreshesOnReentry(t *testing.T) { + m := finishProbes(sized(96, 30)) + m = send(m, key("2")) // developer loadout, opens on "selected" + before := len(m.pool()) + require.Positive(t, before) + + // Move off the review category to one with an unselected package — not + // necessarily the very next one, since the developer preset already fills + // some catalog categories completely. + m = send(m, key("left")) // sidebar focus + var extra config.Package + for { + for _, p := range m.pool() { + if !m.selected[p.Name] { + extra = p + break + } + } + if extra.Name != "" { + break + } + before := m.catCur + m = send(m, key("down")) + require.NotEqual(t, before, m.catCur, "ran out of categories without finding an unselected package") + } + require.NotEqual(t, 0, m.catCur, "must have moved off the review category") + m = send(m, key("right")) // back to the list to toggle + pool := m.pool() + m.rowCur = 0 + for i, p := range pool { + if p.Name == extra.Name { + m.rowCur = i + break + } + } + m = send(m, key("space")) + require.True(t, m.selected[extra.Name]) + + // Back to the review category: it must include the new pick. + m = send(m, key("left")) + for m.catCur != 0 { + m = send(m, key("up")) + } + assert.Equal(t, before+1, len(m.pool()), "review category refreshed to include the new pick") +} + // The text ramp must stay terminal-relative: a hex grey is a guess about the // user's background, and the guess is what made pending rows and key hints // invisible on a translucent terminal. Brand hues are exempt — they're bright