From 2de6953eb8c529d00090063b25da9aca0ccaaf7b Mon Sep 17 00:00:00 2001 From: torosent <17064840+torosent@users.noreply.github.com> Date: Mon, 4 May 2026 11:18:53 -0700 Subject: [PATCH 1/9] fix(containers): refresh after lifecycle actions; toast on shell-into-stopped MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two real bugs reported on the containers screen: 1. Pressing 'x' (stop), Shift+K (sigkill), Shift+R (restart), or 'p' (pause) executed the action but didn't refresh the table. The user had to wait up to 2 seconds for the next poll tick before the state column updated. Each helper now batches a follow-up ListContainers via state.MakeRefreshedCmd — same pattern as delete and prune — and emits a clear status toast on success. 2. Pressing 's' (shell) on a non-running container failed silently. tea.ExecProcess ran 'container exec -it ', which exits immediately for stopped containers, then the TUI resumed to the same screen with no feedback — the user thought 's' was broken. The shell helper now refuses non-running containers with a clear toast, and the SuspendShellMsg handler in app.go surfaces ExecProcess errors as a toast so any other exec failure (image without /bin/sh, race with another stop, etc.) is visible. Regression tests: - TestContainersXStopsContainer now also asserts the follow-up ListContainers call. - TestLifecycleActionsRefreshAfterAction covers sigkill, restart, and pause. - TestContainersSOnStoppedContainerEmitsToast covers the shell refusal. --- CHANGELOG.md | 15 ++ internal/ui/app.go | 16 ++- internal/ui/screens/containers/containers.go | 77 ++++++++-- .../ui/screens/containers/containers_test.go | 136 ++++++++++++++++-- 4 files changed, 219 insertions(+), 25 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index fad919d..5057341 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,21 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- **`x` (stop), `Shift+K` (kill), `Shift+R` (restart), and `p` (pause) + now refresh the table immediately.** Previously they relied on the + 2-second poll tick, so the user pressed `x` to stop a container and + saw `running` for up to 2 seconds. Each lifecycle action now batches + a follow-up `ListContainers` refresh, mirroring the existing + `delete` / `prune` behaviour, and surfaces a clear "stopped " / + "killed " / etc. toast. +- **`s` (shell) on a non-running container shows a toast instead of + failing silently.** `container exec -it ` exits + immediately when the target container isn't running, leaving the + user staring at the same screen with no feedback. The screen now + refuses to issue the exec for non-running containers and surfaces + `can't open shell: is stopped`. ExecProcess errors at the + `app.go` layer are also surfaced as a toast so any other failure + (image lacks `/bin/sh`, race with another stop, etc.) is visible. - **Splash dropping the active screen's first refresh and tick.** The app's catch-all message-forwarding block was gated behind `!m.showSplash`, which meant any message dispatched by the active diff --git a/internal/ui/app.go b/internal/ui/app.go index 9b1f11b..c98c37a 100644 --- a/internal/ui/app.go +++ b/internal/ui/app.go @@ -361,8 +361,22 @@ func (m Model) Update(msg tea.Msg) (tea.Model, tea.Cmd) { return m, modal.Init() case screens.SuspendShellMsg: + // Surface ExecProcess errors as a toast so the user gets + // feedback when `container exec -it ` fails (e.g., + // container raced into a stopped state, or the image has no + // /bin/sh). Without this the TUI just silently resumes after + // an instant exec failure and the user thinks 's' is broken. + // // #nosec G204 -- ID/Shell originate from internal CLI snapshots and the screen's caps probe, not user-supplied strings; binary path is the configured cli.Client.Bin(). - cmd := tea.ExecProcess(exec.Command(m.client.Bin(), "exec", "-it", msg.ID, msg.Shell), func(error) tea.Msg { + execCmd := exec.Command(m.client.Bin(), "exec", "-it", msg.ID, msg.Shell) + shortID := msg.ID + if len(shortID) > 12 { + shortID = shortID[:12] + } + cmd := tea.ExecProcess(execCmd, func(err error) tea.Msg { + if err != nil { + return screens.StatusMsg{Toast: fmt.Sprintf("shell %s failed: %v", shortID, err)} + } return nil }) return m, cmd diff --git a/internal/ui/screens/containers/containers.go b/internal/ui/screens/containers/containers.go index f8ccbfe..31a4e04 100644 --- a/internal/ui/screens/containers/containers.go +++ b/internal/ui/screens/containers/containers.go @@ -550,7 +550,9 @@ func (m *Model) inspectFocused() tea.Cmd { } } -// stopSelected stops the targeted containers. +// stopSelected stops the targeted containers. Includes an immediate +// refresh so the table reflects the new state without waiting for the +// 2-second poll tick. func (m *Model) stopSelected() tea.Cmd { ids := m.targetIDs() if len(ids) == 0 { @@ -564,15 +566,18 @@ func (m *Model) stopSelected() tea.Cmd { ctx := cli.DefaultCtx() err := m.client.StopContainer(ctx, id) if err != nil { - return screens.StatusMsg{Toast: fmt.Sprintf("stop %s failed: %v", id, err)} + return screens.StatusMsg{Toast: fmt.Sprintf("stop %s failed: %v", formatShortID(id), err)} } - return nil + return screens.StatusMsg{Toast: fmt.Sprintf("stopped %s", formatShortID(id))} }) } + cmds = append(cmds, m.refreshContainersCmd()) return tea.Batch(cmds...) } -// killSelected kills the targeted containers. +// killSelected kills the targeted containers. Includes an immediate +// refresh so the table reflects the new state without waiting for the +// 2-second poll tick. func (m *Model) killSelected() tea.Cmd { ids := m.targetIDs() if len(ids) == 0 { @@ -586,15 +591,18 @@ func (m *Model) killSelected() tea.Cmd { ctx := cli.DefaultCtx() err := m.client.KillContainer(ctx, id) if err != nil { - return screens.StatusMsg{Toast: fmt.Sprintf("kill %s failed: %v", id, err)} + return screens.StatusMsg{Toast: fmt.Sprintf("kill %s failed: %v", formatShortID(id), err)} } - return nil + return screens.StatusMsg{Toast: fmt.Sprintf("killed %s", formatShortID(id))} }) } + cmds = append(cmds, m.refreshContainersCmd()) return tea.Batch(cmds...) } -// restartSelected restarts the targeted containers. +// restartSelected restarts the targeted containers. Includes an +// immediate refresh so the table reflects the new state without waiting +// for the 2-second poll tick. func (m *Model) restartSelected() tea.Cmd { ids := m.targetIDs() if len(ids) == 0 { @@ -608,11 +616,12 @@ func (m *Model) restartSelected() tea.Cmd { ctx := cli.DefaultCtx() err := m.client.RestartContainer(ctx, id) if err != nil { - return screens.StatusMsg{Toast: fmt.Sprintf("restart %s failed: %v", id, err)} + return screens.StatusMsg{Toast: fmt.Sprintf("restart %s failed: %v", formatShortID(id), err)} } - return nil + return screens.StatusMsg{Toast: fmt.Sprintf("restarted %s", formatShortID(id))} }) } + cmds = append(cmds, m.refreshContainersCmd()) return tea.Batch(cmds...) } @@ -641,7 +650,9 @@ func (m *Model) deleteSelected() tea.Cmd { } } -// pauseSelected pauses the targeted containers. +// pauseSelected pauses the targeted containers. Includes an immediate +// refresh so the table reflects the new state without waiting for the +// 2-second poll tick. func (m *Model) pauseSelected() tea.Cmd { ids := m.targetIDs() if len(ids) == 0 { @@ -655,21 +666,48 @@ func (m *Model) pauseSelected() tea.Cmd { ctx := cli.DefaultCtx() err := m.client.PauseContainer(ctx, id) if err != nil { - return screens.StatusMsg{Toast: fmt.Sprintf("pause %s failed: %v", id, err)} + return screens.StatusMsg{Toast: fmt.Sprintf("pause %s failed: %v", formatShortID(id), err)} } - return nil + return screens.StatusMsg{Toast: fmt.Sprintf("paused %s", formatShortID(id))} }) } + cmds = append(cmds, m.refreshContainersCmd()) return tea.Batch(cmds...) } -// openShell emits a SuspendShellMsg for the focused container. +// refreshContainersCmd returns a Cmd that fetches the latest container +// list. Used after lifecycle actions (stop/kill/restart/pause/delete) +// so the user sees the new state immediately rather than waiting for +// the 2-second poll tick. +func (m *Model) refreshContainersCmd() tea.Cmd { + client := m.client + return state.MakeRefreshedCmd[cli.Container]( + cli.DefaultCtx(), + func(ctx context.Context) ([]cli.Container, error) { + return client.ListContainers(ctx, true) + }, + cli.ResourceContainers, + ) +} + +// openShell emits a SuspendShellMsg for the focused container. Refuses +// (with a toast) to exec into a non-running container — `container +// exec -it` exits immediately on a stopped container, leaving the user +// staring at the same screen with no feedback. func (m *Model) openShell() tea.Cmd { c := m.focusedContainer() if c == nil { return nil } + if !isRunning(c.Status) { + return func() tea.Msg { + return screens.StatusMsg{ + Toast: fmt.Sprintf("can't open shell: %s is %s", formatShortID(c.ID), strings.ToLower(c.Status)), + } + } + } + shell := os.Getenv("SHELL") if shell == "" { shell = "/bin/sh" @@ -683,6 +721,19 @@ func (m *Model) openShell() tea.Cmd { } } +// isRunning returns true when the container is in a state that accepts +// `container exec -it`. Apple's `container` reports lower-case states +// ("running", "stopped", "exited", "starting", "paused"); we accept +// "running" and "starting" to mirror Docker's exec semantics. +func isRunning(status string) bool { + switch strings.ToLower(strings.TrimSpace(status)) { + case "running", "starting": + return true + default: + return false + } +} + // openLogs opens the log viewer modal for the focused container (or all // marked containers if any are marked). func (m *Model) openLogs() tea.Cmd { diff --git a/internal/ui/screens/containers/containers_test.go b/internal/ui/screens/containers/containers_test.go index 6defec8..b2a82da 100644 --- a/internal/ui/screens/containers/containers_test.go +++ b/internal/ui/screens/containers/containers_test.go @@ -306,25 +306,51 @@ func TestContainersXStopsContainer(t *testing.T) { s, _ := m.Update(msg) m = assertModel(s) - // Press 'x' to stop + // Press 'x' to stop. Returns a tea.Batch of {stop, refresh}; drain + // both so we observe StopContainer AND the follow-up ListContainers. keyMsg := tea.KeyMsg{Type: tea.KeyRunes, Runes: []rune{'x'}} _, cmd := m.Update(keyMsg) + if cmd == nil { + t.Fatal("expected 'x' key to return a cmd") + } + drainBatch(cmd) - if cmd != nil { - _ = cmd() + // Check that StopContainer AND a follow-up ListContainers were called + if !calledOnce(fake.Calls, "StopContainer") { + t.Errorf("expected StopContainer to be called; calls=%v", fake.Calls) + } + if !calledOnce(fake.Calls, "ListContainers") { + t.Errorf("expected ListContainers refresh after stop; calls=%v", fake.Calls) } +} - // Check that StopContainer was called - found := false - for _, call := range fake.Calls { - if call == "StopContainer" { - found = true - break +// drainBatch invokes every Cmd inside a tea.Batch'd Cmd. tea.Batch +// returns a Cmd that yields a tea.BatchMsg ([]Cmd) when called; we then +// run each inner Cmd. Used so action+refresh batches actually exercise +// both legs in tests. +func drainBatch(cmd tea.Cmd) { + if cmd == nil { + return + } + msg := cmd() + batch, ok := msg.(tea.BatchMsg) + if !ok { + return + } + for _, c := range batch { + if c != nil { + _ = c() } } - if !found { - t.Error("expected StopContainer to be called") +} + +func calledOnce(calls []string, want string) bool { + for _, c := range calls { + if c == want { + return true + } } + return false } func TestContainersSEmitsSuspendShellMsg(t *testing.T) { @@ -376,6 +402,94 @@ func TestContainersSEmitsSuspendShellMsg(t *testing.T) { } } +// Regression test: pressing 's' on a non-running container should NOT +// emit a SuspendShellMsg, because `container exec -it` against a +// stopped container exits immediately and the user gets no feedback. +// Instead the screen surfaces a clear toast. +func TestContainersSOnStoppedContainerEmitsToast(t *testing.T) { + fake := &cli.Fake{ + ListContainersResp: []cli.Container{ + {ID: "c1stopped", ShortID: "c1stopped", Image: "nginx", Status: "stopped"}, + }, + } + m := New(fake, clock.NewFake(time.Now()), theme.DefaultDark()) + m.Init() + s, _ := m.Update(state.RefreshedMsg[cli.Container]{ + Resource: cli.ResourceContainers, + Snapshot: state.Snapshot[cli.Container]{Items: fake.ListContainersResp, FetchedAt: time.Now()}, + }) + m = assertModel(s) + + _, cmd := m.Update(tea.KeyMsg{Type: tea.KeyRunes, Runes: []rune{'s'}}) + if cmd == nil { + t.Fatal("expected 's' on stopped container to return a status-toast cmd, got nil") + } + switch out := cmd().(type) { + case screens.SuspendShellMsg: + t.Fatalf("expected status toast, got SuspendShellMsg %+v — `container exec -it` would have failed silently", out) + case screens.StatusMsg: + if !strings.Contains(out.Toast, "stopped") { + t.Errorf("toast should mention stopped state; got %q", out.Toast) + } + default: + t.Fatalf("expected StatusMsg, got %T", out) + } +} + +// Regression test: the kill/restart/pause helpers all batch the action +// with a follow-up ListContainers refresh so the table reflects the new +// state without waiting for the 2-second poll tick. +func TestLifecycleActionsRefreshAfterAction(t *testing.T) { + cases := []struct { + name string + fakeReset func(*cli.Fake) + runAction func(*Model) tea.Cmd + wantCall string + }{ + { + name: "kill", + runAction: func(m *Model) tea.Cmd { return m.killSelected() }, + wantCall: "KillContainer", + }, + { + name: "restart", + runAction: func(m *Model) tea.Cmd { return m.restartSelected() }, + wantCall: "RestartContainer", + }, + { + name: "pause", + runAction: func(m *Model) tea.Cmd { return m.pauseSelected() }, + wantCall: "PauseContainer", + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + fake := &cli.Fake{ + ListContainersResp: []cli.Container{ + {ID: "c1", ShortID: "c1", Image: "nginx", Status: "running"}, + }, + } + m := New(fake, clock.NewFake(time.Now()), theme.DefaultDark()) + m.Init() + s, _ := m.Update(state.RefreshedMsg[cli.Container]{ + Resource: cli.ResourceContainers, + Snapshot: state.Snapshot[cli.Container]{Items: fake.ListContainersResp, FetchedAt: time.Now()}, + }) + m = assertModel(s) + + fake.Calls = nil + drainBatch(tc.runAction(m)) + + if !calledOnce(fake.Calls, tc.wantCall) { + t.Errorf("expected %s to be called; calls=%v", tc.wantCall, fake.Calls) + } + if !calledOnce(fake.Calls, "ListContainers") { + t.Errorf("expected follow-up ListContainers refresh after %s; calls=%v", tc.name, fake.Calls) + } + }) + } +} + func TestContainersPauseUnsupportedEmitsToast(t *testing.T) { fake := &cli.Fake{ ListContainersResp: []cli.Container{ From fda3a89737ddcb85eb31c394cbc774f724bae447 Mon Sep 17 00:00:00 2001 From: torosent <17064840+torosent@users.noreply.github.com> Date: Mon, 4 May 2026 13:07:11 -0700 Subject: [PATCH 2/9] feat(containers): add shell picker (bash/sh) and force redraw after exec MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two follow-ups on top of the previous shell-feedback fix, both prompted by real screenshots from the user: 1. The host's $SHELL is the wrong shell to use inside a container. On macOS users default to /bin/zsh, which is rarely present in Linux containers — `container exec -it /bin/zsh` then exits with an error to stderr but a 0 exit code, so the TUI never sees a failure to surface. Replace the $SHELL lookup with a small modal that asks the user to pick bash or sh: - One-keystroke picks: 'b' for bash, 's' for sh. - Arrow keys + Enter for navigated picks. - Esc cancels. - Modal emits modals.ShellPickedMsg{ID, Shell}; the containers screen converts that to screens.SuspendShellMsg. 2. After tea.ExecProcess returned the screen sometimes rendered half-blank with stale cells visible (truncated table + leftover inspect JSON). Force a fresh tea.WindowSize() in the new shellExecDoneMsg handler so every screen and the open modal reflow against the real terminal size and repaint the full altscreen. Tests: - TestContainersSOpensShellPicker covers the picker-on-running flow. - TestContainersShellPickedConvertsToSuspend covers the modal-result handoff. - TestShellPicker_* covers the modal itself (hotkeys, cursor+Enter, Esc cancel, view contents). - TestContainersSEmitsSuspendShellMsg removed (its $SHELL-based contract is intentionally gone). --- CHANGELOG.md | 18 ++ internal/ui/app.go | 47 +++-- internal/ui/modals/shellpicker.go | 162 ++++++++++++++++++ internal/ui/modals/shellpicker_test.go | 115 +++++++++++++ internal/ui/screens/containers/containers.go | 35 ++-- .../ui/screens/containers/containers_test.go | 90 ++++++---- 6 files changed, 409 insertions(+), 58 deletions(-) create mode 100644 internal/ui/modals/shellpicker.go create mode 100644 internal/ui/modals/shellpicker_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 5057341..5556e28 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,8 +7,26 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Added + +- **Shell picker modal** — `s` on a running container now opens a + small modal asking whether to use `/bin/bash` or `/bin/sh` rather + than blindly using the host's `$SHELL`. The host shell (often + `/bin/zsh` on macOS) is rarely present inside Linux containers, + and Apple's `container` returns exit 0 even when exec fails, so a + missing shell would silently leave the user staring at a glitched + half-rendered TUI. Press `b`/`s` for a one-keystroke pick or use + arrow keys + Enter. + ### Fixed +- **Glitched TUI after `tea.ExecProcess` returns.** After the user + exited an in-container shell, the next altscreen frame sometimes + rendered on top of stale cells and left the screen looking + half-drawn (truncated table + leftover JSON visible). The + `SuspendShellMsg` handler now returns `tea.WindowSize()` after + exec, forcing every screen and the open modal (if any) to reflow + against the real terminal size and repaint the full altscreen. - **`x` (stop), `Shift+K` (kill), `Shift+R` (restart), and `p` (pause) now refresh the table immediately.** Previously they relied on the 2-second poll tick, so the user pressed `x` to stop a container and diff --git a/internal/ui/app.go b/internal/ui/app.go index c98c37a..636665e 100644 --- a/internal/ui/app.go +++ b/internal/ui/app.go @@ -89,6 +89,15 @@ type acrLoginMsg struct { err error } +// shellExecDoneMsg is emitted after tea.ExecProcess returns from a +// SuspendShellMsg. Carries an optional toast (set when the exec +// failed) and triggers a fresh WindowSizeMsg so altscreen is fully +// repainted — without that, the post-exec frame can render on top of +// stale cells and leave the screen looking glitched. +type shellExecDoneMsg struct { + toast string +} + // NewApp constructs the root model. func NewApp(client cli.Client, clk clock.Clock, p theme.Palette, cfg config.Config) Model { // Set up data directories @@ -361,25 +370,43 @@ func (m Model) Update(msg tea.Msg) (tea.Model, tea.Cmd) { return m, modal.Init() case screens.SuspendShellMsg: - // Surface ExecProcess errors as a toast so the user gets - // feedback when `container exec -it ` fails (e.g., - // container raced into a stopped state, or the image has no - // /bin/sh). Without this the TUI just silently resumes after - // an instant exec failure and the user thinks 's' is broken. + // Run `container exec -it ` via tea.ExecProcess + // (which exits altscreen, runs the child, then re-enters + // altscreen). Force a window-size re-query when the process + // exits — without it, the next altscreen frame is sometimes + // drawn on top of the just-cleared terminal with old rows + // missing, leaving the screen looking glitched (issue + // reported: half-rendered table + leftover JSON visible + // after pressing 's'). Surfacing exec errors as a toast also + // gives the user feedback if the shell fails. // - // #nosec G204 -- ID/Shell originate from internal CLI snapshots and the screen's caps probe, not user-supplied strings; binary path is the configured cli.Client.Bin(). + // #nosec G204 -- ID/Shell originate from internal CLI snapshots and the modal's static option list (/bin/bash | /bin/sh), not user-supplied strings; binary path is the configured cli.Client.Bin(). execCmd := exec.Command(m.client.Bin(), "exec", "-it", msg.ID, msg.Shell) shortID := msg.ID if len(shortID) > 12 { shortID = shortID[:12] } - cmd := tea.ExecProcess(execCmd, func(err error) tea.Msg { + var toast string + execDone := tea.ExecProcess(execCmd, func(err error) tea.Msg { if err != nil { - return screens.StatusMsg{Toast: fmt.Sprintf("shell %s failed: %v", shortID, err)} + toast = fmt.Sprintf("shell %s failed: %v", shortID, err) } - return nil + // Returning a typed sentinel so the redraw + toast logic + // runs in the same Update cycle, after altscreen is + // re-entered. + return shellExecDoneMsg{toast: toast} }) - return m, cmd + return m, execDone + + case shellExecDoneMsg: + if msg.toast != "" { + m.toast = msg.toast + } + // Force a fresh WindowSizeMsg so every screen and the open + // modal (if any) reflows against the real terminal size, + // repainting the full altscreen rather than only the cells + // that happened to differ from the pre-exec frame. + return m, tea.WindowSize() case screens.StatusMsg: m.toast = msg.Toast diff --git a/internal/ui/modals/shellpicker.go b/internal/ui/modals/shellpicker.go new file mode 100644 index 0000000..abe1acd --- /dev/null +++ b/internal/ui/modals/shellpicker.go @@ -0,0 +1,162 @@ +package modals + +import ( + "fmt" + "strings" + + tea "github.com/charmbracelet/bubbletea" + "github.com/charmbracelet/lipgloss" + + "github.com/torosent/c9s/internal/ui/theme" +) + +// ShellPickerModel is a small two-option picker for choosing the +// in-container shell (bash or sh). The host's $SHELL is irrelevant — +// what matters is what's on PATH inside the container — so we let the +// user pick rather than guess. POSIX requires /bin/sh in essentially +// every Linux container; /bin/bash is common but not universal. +type ShellPickerModel struct { + palette theme.Palette + containerID string + shortID string + options []shellOption + cursor int +} + +type shellOption struct { + key rune + label string + path string +} + +// ShellPickedMsg is emitted when a shell is selected. The containers +// screen catches it and converts it to screens.SuspendShellMsg. +type ShellPickedMsg struct { + ID string + Shell string +} + +// NewShellPicker creates a new shell-picker modal for the given +// container. +func NewShellPicker(containerID, shortID string, p theme.Palette) ShellPickerModel { + return ShellPickerModel{ + palette: p, + containerID: containerID, + shortID: shortID, + options: []shellOption{ + {key: 'b', label: "bash (/bin/bash)", path: "/bin/bash"}, + {key: 's', label: "sh (/bin/sh)", path: "/bin/sh"}, + }, + cursor: 0, + } +} + +// Init implements Modal. +func (m ShellPickerModel) Init() tea.Cmd { return nil } + +// Update implements Modal. +func (m ShellPickerModel) Update(msg tea.Msg) (Modal, tea.Cmd) { + if key, ok := msg.(tea.KeyMsg); ok { + switch key.String() { + case "up", "k": + if m.cursor > 0 { + m.cursor-- + } + return m, nil + case "down", "j": + if m.cursor < len(m.options)-1 { + m.cursor++ + } + return m, nil + case "enter": + return m.pick(m.options[m.cursor]) + case "esc", "q": + return m, func() tea.Msg { return CloseModalMsg{} } + } + // Direct hot-letter selection: 'b' or 's'. + if key.Type == tea.KeyRunes && len(key.Runes) == 1 { + r := key.Runes[0] + for _, opt := range m.options { + if r == opt.key { + return m.pick(opt) + } + } + } + } + return m, nil +} + +func (m ShellPickerModel) pick(opt shellOption) (Modal, tea.Cmd) { + id := m.containerID + path := opt.path + return m, tea.Batch( + func() tea.Msg { return ShellPickedMsg{ID: id, Shell: path} }, + func() tea.Msg { return CloseModalMsg{} }, + ) +} + +// View implements Modal. +func (m ShellPickerModel) View(width, height int) string { + innerW := 44 + if width < innerW+8 { + innerW = width - 8 + if innerW < 24 { + innerW = 24 + } + } + + bg := lipgloss.NewStyle().Foreground(m.palette.Fg).Background(m.palette.Bg) + titleStyle := lipgloss.NewStyle().Bold(true).Foreground(m.palette.HeaderFg).Background(m.palette.Accent).Padding(0, 1) + dim := lipgloss.NewStyle().Foreground(m.palette.Dim).Background(m.palette.Bg) + selRow := lipgloss.NewStyle().Foreground(m.palette.Bg).Background(m.palette.Accent).Bold(true) + keyHint := lipgloss.NewStyle().Foreground(m.palette.Accent).Background(m.palette.Bg).Bold(true) + + subject := m.shortID + if subject == "" { + subject = "container" + } + header := fmt.Sprintf("Open shell in %s", subject) + + lines := []string{ + bg.Width(innerW).Render(titleStyle.Render(" " + header + " ")), + bg.Width(innerW).Render(" "), + } + + for i, opt := range m.options { + hint := keyHint.Render(fmt.Sprintf("[%c] ", opt.key)) + var row string + if i == m.cursor { + row = selRow.Width(innerW).Render(" ▸ " + string(opt.key) + " " + opt.label) + } else { + row = bg.Width(innerW).Render(" " + hint + bg.Render(opt.label)) + } + lines = append(lines, row) + } + + lines = append(lines, + bg.Width(innerW).Render(" "), + bg.Width(innerW).Render(dim.Render("b/s: pick • ↑/↓+Enter: pick • Esc: cancel")), + ) + + content := strings.Join(lines, "\n") + + box := lipgloss.NewStyle(). + Border(lipgloss.RoundedBorder()). + BorderForeground(m.palette.Border). + BorderBackground(m.palette.Bg). + Background(m.palette.Bg). + Foreground(m.palette.Fg). + Padding(1, 2). + Render(content) + + return lipgloss.Place( + width, height, + lipgloss.Center, lipgloss.Center, + box, + lipgloss.WithWhitespaceBackground(m.palette.Bg), + lipgloss.WithWhitespaceForeground(m.palette.Bg), + ) +} + +// Title implements Modal. +func (m ShellPickerModel) Title() string { return "Shell" } diff --git a/internal/ui/modals/shellpicker_test.go b/internal/ui/modals/shellpicker_test.go new file mode 100644 index 0000000..29c0ae2 --- /dev/null +++ b/internal/ui/modals/shellpicker_test.go @@ -0,0 +1,115 @@ +package modals + +import ( + "strings" + "testing" + + tea "github.com/charmbracelet/bubbletea" + + "github.com/torosent/c9s/internal/ui/theme" +) + +func TestShellPicker_HotkeyB_PicksBash(t *testing.T) { + picker := NewShellPicker("c1", "c1", theme.DefaultDark()) + _, cmd := picker.Update(tea.KeyMsg{Type: tea.KeyRunes, Runes: []rune{'b'}}) + if cmd == nil { + t.Fatal("expected 'b' to return a cmd") + } + got := drainShellPickerBatch(cmd) + if got.shell != "/bin/bash" { + t.Errorf("Shell = %q, want /bin/bash", got.shell) + } + if got.id != "c1" { + t.Errorf("ID = %q, want c1", got.id) + } + if !got.closed { + t.Error("expected modal to also emit CloseModalMsg") + } +} + +func TestShellPicker_HotkeyS_PicksSh(t *testing.T) { + picker := NewShellPicker("c1", "c1", theme.DefaultDark()) + _, cmd := picker.Update(tea.KeyMsg{Type: tea.KeyRunes, Runes: []rune{'s'}}) + if cmd == nil { + t.Fatal("expected 's' to return a cmd") + } + got := drainShellPickerBatch(cmd) + if got.shell != "/bin/sh" { + t.Errorf("Shell = %q, want /bin/sh", got.shell) + } +} + +func TestShellPicker_EnterPicksCursor(t *testing.T) { + picker := NewShellPicker("c1", "c1", theme.DefaultDark()) + // cursor starts at 0 (bash); arrow down to sh + pickerModel, _ := picker.Update(tea.KeyMsg{Type: tea.KeyDown}) + picker = pickerModel.(ShellPickerModel) + _, cmd := picker.Update(tea.KeyMsg{Type: tea.KeyEnter}) + if cmd == nil { + t.Fatal("expected enter to return a cmd") + } + got := drainShellPickerBatch(cmd) + if got.shell != "/bin/sh" { + t.Errorf("Shell = %q, want /bin/sh after Down+Enter", got.shell) + } +} + +func TestShellPicker_EscClosesWithoutPick(t *testing.T) { + picker := NewShellPicker("c1", "c1", theme.DefaultDark()) + _, cmd := picker.Update(tea.KeyMsg{Type: tea.KeyEsc}) + if cmd == nil { + t.Fatal("expected esc to return a cmd") + } + if _, ok := cmd().(CloseModalMsg); !ok { + t.Errorf("expected CloseModalMsg, got %T", cmd()) + } +} + +func TestShellPicker_ViewMentionsContainerAndOptions(t *testing.T) { + picker := NewShellPicker("c1", "abc1234567ab", theme.DefaultDark()) + view := picker.View(80, 24) + for _, want := range []string{"abc1234567ab", "bash", "sh"} { + if !strings.Contains(view, want) { + t.Errorf("view should mention %q; got:\n%s", want, view) + } + } +} + +type pickResult struct { + id string + shell string + closed bool +} + +func drainShellPickerBatch(cmd tea.Cmd) pickResult { + out := pickResult{} + if cmd == nil { + return out + } + msg := cmd() + batch, ok := msg.(tea.BatchMsg) + if !ok { + // Single-message cmd (e.g., direct ShellPickedMsg) + switch m := msg.(type) { + case ShellPickedMsg: + out.id = m.ID + out.shell = m.Shell + case CloseModalMsg: + out.closed = true + } + return out + } + for _, c := range batch { + if c == nil { + continue + } + switch m := c().(type) { + case ShellPickedMsg: + out.id = m.ID + out.shell = m.Shell + case CloseModalMsg: + out.closed = true + } + } + return out +} diff --git a/internal/ui/screens/containers/containers.go b/internal/ui/screens/containers/containers.go index 31a4e04..cb51ac4 100644 --- a/internal/ui/screens/containers/containers.go +++ b/internal/ui/screens/containers/containers.go @@ -3,7 +3,6 @@ package containers import ( "context" "fmt" - "os" "sort" "strings" "time" @@ -173,6 +172,16 @@ func (m *Model) Update(msg tea.Msg) (screens.Screen, tea.Cmd) { cmds = append(cmds, m.performPrune()) } + case modals.ShellPickedMsg: + // The shell picker has resolved which shell the user wants; + // hand it off to the app-level SuspendShellMsg handler that + // owns tea.ExecProcess. + id := msg.ID + shell := msg.Shell + cmds = append(cmds, func() tea.Msg { + return screens.SuspendShellMsg{ID: id, Shell: shell} + }) + case state.RefreshedMsg[cli.Container]: if msg.Resource != cli.ResourceContainers { break @@ -690,10 +699,13 @@ func (m *Model) refreshContainersCmd() tea.Cmd { ) } -// openShell emits a SuspendShellMsg for the focused container. Refuses -// (with a toast) to exec into a non-running container — `container -// exec -it` exits immediately on a stopped container, leaving the user -// staring at the same screen with no feedback. +// openShell opens the shell-picker modal for the focused container. +// We deliberately do NOT honour the host's $SHELL — the user's host +// shell (often /bin/zsh on macOS) is rarely present inside Linux +// containers, and `container exec -it /bin/zsh` fails silently +// (Apple's `container` returns exit 0 even on failure). The picker +// asks the user to pick bash or sh; the result comes back as a +// modals.ShellPickedMsg which we convert to a SuspendShellMsg. func (m *Model) openShell() tea.Cmd { c := m.focusedContainer() if c == nil { @@ -708,15 +720,12 @@ func (m *Model) openShell() tea.Cmd { } } - shell := os.Getenv("SHELL") - if shell == "" { - shell = "/bin/sh" - } - + id := c.ID + short := formatShortID(c.ID) + palette := m.palette return func() tea.Msg { - return screens.SuspendShellMsg{ - ID: c.ID, - Shell: shell, + return screens.OpenModalMsg{ + Modal: modals.NewShellPicker(id, short, palette), } } } diff --git a/internal/ui/screens/containers/containers_test.go b/internal/ui/screens/containers/containers_test.go index b2a82da..612947c 100644 --- a/internal/ui/screens/containers/containers_test.go +++ b/internal/ui/screens/containers/containers_test.go @@ -2,7 +2,6 @@ package containers import ( "context" - "os" "strings" "testing" "time" @@ -11,6 +10,7 @@ import ( "github.com/torosent/c9s/internal/cli" "github.com/torosent/c9s/internal/clock" "github.com/torosent/c9s/internal/state" + "github.com/torosent/c9s/internal/ui/modals" "github.com/torosent/c9s/internal/ui/screens" "github.com/torosent/c9s/internal/ui/theme" ) @@ -353,52 +353,72 @@ func calledOnce(calls []string, want string) bool { return false } -func TestContainersSEmitsSuspendShellMsg(t *testing.T) { +// TestContainersSOpensShellPicker — pressing 's' on a running +// container now opens the shell-picker modal rather than emitting a +// SuspendShellMsg directly. The picker decides between bash and sh, +// since the host's $SHELL (often /bin/zsh on macOS) is rarely present +// inside Linux containers. +func TestContainersSOpensShellPicker(t *testing.T) { fake := &cli.Fake{ ListContainersResp: []cli.Container{ - {ID: "c1", ShortID: "c1", Image: "nginx", Status: "running"}, + {ID: "c1running", ShortID: "c1running", Image: "nginx", Status: "running"}, }, } - clk := clock.NewFake(time.Now()) - p := theme.DefaultDark() - - m := New(fake, clk, p) + m := New(fake, clock.NewFake(time.Now()), theme.DefaultDark()) m.Init() + s, _ := m.Update(state.RefreshedMsg[cli.Container]{ + Resource: cli.ResourceContainers, + Snapshot: state.Snapshot[cli.Container]{Items: fake.ListContainersResp, FetchedAt: time.Now()}, + }) + m = assertModel(s) - snapshot := state.Snapshot[cli.Container]{ - Items: fake.ListContainersResp, - FetchedAt: time.Now(), + _, cmd := m.Update(tea.KeyMsg{Type: tea.KeyRunes, Runes: []rune{'s'}}) + if cmd == nil { + t.Fatal("expected 's' to return a cmd") } - msg := state.RefreshedMsg[cli.Container]{ - Resource: cli.ResourceContainers, - Snapshot: snapshot, + switch out := cmd().(type) { + case screens.OpenModalMsg: + if _, ok := out.Modal.(modals.ShellPickerModel); !ok { + t.Errorf("expected ShellPickerModel, got %T", out.Modal) + } + case screens.SuspendShellMsg: + t.Fatalf("expected OpenModalMsg(ShellPickerModel), got SuspendShellMsg — picker bypassed") + default: + t.Fatalf("expected OpenModalMsg, got %T", out) } - s, _ := m.Update(msg) - m = assertModel(s) +} - // Press 's' to open shell - keyMsg := tea.KeyMsg{Type: tea.KeyRunes, Runes: []rune{'s'}} - _, cmd := m.Update(keyMsg) +// TestContainersShellPickedConvertsToSuspend — once the user picks a +// shell, the modal emits ShellPickedMsg; the containers screen +// converts that to SuspendShellMsg for the app-level ExecProcess +// handler. +func TestContainersShellPickedConvertsToSuspend(t *testing.T) { + fake := &cli.Fake{ + ListContainersResp: []cli.Container{ + {ID: "c1pick", ShortID: "c1pick", Image: "nginx", Status: "running"}, + }, + } + m := New(fake, clock.NewFake(time.Now()), theme.DefaultDark()) + m.Init() + s, _ := m.Update(state.RefreshedMsg[cli.Container]{ + Resource: cli.ResourceContainers, + Snapshot: state.Snapshot[cli.Container]{Items: fake.ListContainersResp, FetchedAt: time.Now()}, + }) + m = assertModel(s) + _, cmd := m.Update(modals.ShellPickedMsg{ID: "c1pick", Shell: "/bin/bash"}) if cmd == nil { - t.Fatal("expected 's' key to return a cmd") + t.Fatal("expected ShellPickedMsg to return a cmd") } - - cmdMsg := cmd() - if suspendMsg, ok := cmdMsg.(screens.SuspendShellMsg); !ok { - t.Errorf("expected SuspendShellMsg, got %T", cmdMsg) - } else { - if suspendMsg.ID != "c1" { - t.Errorf("expected ID='c1', got %q", suspendMsg.ID) - } - // Shell should be from SHELL env or default /bin/sh - expectedShell := os.Getenv("SHELL") - if expectedShell == "" { - expectedShell = "/bin/sh" - } - if suspendMsg.Shell != expectedShell { - t.Errorf("expected Shell=%q, got %q", expectedShell, suspendMsg.Shell) - } + suspendMsg, ok := cmd().(screens.SuspendShellMsg) + if !ok { + t.Fatalf("expected SuspendShellMsg, got %T", cmd()) + } + if suspendMsg.ID != "c1pick" { + t.Errorf("ID = %q, want c1pick", suspendMsg.ID) + } + if suspendMsg.Shell != "/bin/bash" { + t.Errorf("Shell = %q, want /bin/bash", suspendMsg.Shell) } } From 4b24d6ac1c822af2a704f58b882b167ee55d5da5 Mon Sep 17 00:00:00 2001 From: torosent <17064840+torosent@users.noreply.github.com> Date: Mon, 4 May 2026 13:21:17 -0700 Subject: [PATCH 3/9] fix(shell): route ShellPickedMsg past modal stack; probe shell exists; clear-screen on resume MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three issues from the latest user report: 1. "I clicked bash and nothing happened, I expected an error." Root cause: the picker batches ShellPickedMsg alongside CloseModalMsg, but tea.Batch makes no ordering guarantees. When ShellPickedMsg arrived first the picker was still top of stack, the modal received the message, didn't handle it, and the pick was silently dropped. Added an explicit typed case in app.Update that forwards ShellPickedMsg directly to the active screen — mirroring the ConfirmResultMsg pattern that's been correct for delete/prune all along. 2. Defensive probe before tea.ExecProcess. Apple's `container exec` returns exit 0 EVEN WHEN THE SHELL ISN'T INSTALLED — it writes the error to stderr (visible for milliseconds before altscreen re-entry hides it) and exits cleanly. We can't surface a toast post-hoc because tea.ExecProcess sees a clean exit. So before suspending the TUI we run `container exec test -x ` (no -i/-t, 3s timeout) and toast immediately if the probe fails: " not available in — try the other shell". 3. "After I typed exit, I got corrupt graphics." tea.WindowSize() alone wasn't enough — bubbletea's renderer preserves cells it thinks are unchanged, but altscreen state is corrupt because the shell ran with stdout writing to the host terminal during the suspend. Issue tea.ClearScreen first (\033[2J\033[H) and then re-query window size so every screen reflows. Without this, the post-exit frame can show leftover shell output and stale modal cells. Regression test: - TestAppShellPickedMsgReachesScreenWhilePickerOpen — feeds ShellPickedMsg while the picker is still top of stack and asserts the screen produces SuspendShellMsg{ID, Shell}. Verified failing without the typed-case fix. --- CHANGELOG.md | 32 +++++++++++++---- internal/ui/app.go | 78 ++++++++++++++++++++++++++++++----------- internal/ui/app_test.go | 76 +++++++++++++++++++++++++++++++++++++++ 3 files changed, 158 insertions(+), 28 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5556e28..30396d0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,13 +20,31 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed -- **Glitched TUI after `tea.ExecProcess` returns.** After the user - exited an in-container shell, the next altscreen frame sometimes - rendered on top of stale cells and left the screen looking - half-drawn (truncated table + leftover JSON visible). The - `SuspendShellMsg` handler now returns `tea.WindowSize()` after - exec, forcing every screen and the open modal (if any) to reflow - against the real terminal size and repaint the full altscreen. +- **`ShellPickedMsg` was swallowed by the still-open picker modal.** + The picker batches `ShellPickedMsg` alongside `CloseModalMsg`, but + `tea.Batch` doesn't guarantee ordering. When `ShellPickedMsg` + arrived first the picker was still top of stack, the modal received + the message, didn't handle it, and the user's pick vanished — the + classic "I clicked bash and nothing happened" symptom. Added an + explicit typed case in `app.Update` that forwards `ShellPickedMsg` + directly to the active screen, mirroring `ConfirmResultMsg`. +- **Probe shell existence before suspending the TUI.** Apple's + `container exec -it ` returns **exit 0 even when the + shell isn't installed** — it writes the error to stderr (visible + for milliseconds before altscreen re-entry hides it) and exits. + `tea.ExecProcess` sees a clean exit so we can't surface a useful + toast post-hoc. Now probe `container exec test -x ` + (3-second timeout, no `-it`) before running the interactive exec; + if the probe fails we toast ` not available in — try + the other shell` and skip the suspend entirely. +- **Glitched TUI after `tea.ExecProcess` returns.** Even on a + successful shell session, after exit the next altscreen frame + sometimes rendered on top of stale cells (truncated table + + leftover output visible). `tea.WindowSize()` alone wasn't enough + because bubbletea's renderer preserves cells it thinks are + unchanged. The handler now batches `tea.ClearScreen` (emits + `\033[2J\033[H`) ahead of `tea.WindowSize()` to force a full + altscreen repaint. - **`x` (stop), `Shift+K` (kill), `Shift+R` (restart), and `p` (pause) now refresh the table immediately.** Previously they relied on the 2-second poll tick, so the user pressed `x` to stop a container and diff --git a/internal/ui/app.go b/internal/ui/app.go index 636665e..22a54a9 100644 --- a/internal/ui/app.go +++ b/internal/ui/app.go @@ -322,6 +322,23 @@ func (m Model) Update(msg tea.Msg) (tea.Model, tea.Cmd) { } return m, nil + case modals.ShellPickedMsg: + // The shell-picker modal batches ShellPickedMsg alongside + // CloseModalMsg, but tea.Batch makes no ordering guarantees. + // If we let this fall through to the catch-all routing + // below, the message races CloseModalMsg: when ShellPickedMsg + // arrives first the picker is still on the stack, the modal + // receives the message, doesn't handle it, and the pick is + // silently dropped — exactly the "I clicked bash and nothing + // happened" symptom. Forward directly to the active screen, + // matching the ConfirmResultMsg pattern above. + if scr, ok := m.screens[m.active]; ok { + newScr, cmd := scr.Update(msg) + m.screens[m.active] = newScr + return m, cmd + } + return m, nil + case modals.LoginResultMsg, modals.LoginCancelledMsg, modals.TextInputResultMsg, modals.TextInputCancelledMsg: if scr, ok := m.screens[m.active]; ok { @@ -370,30 +387,41 @@ func (m Model) Update(msg tea.Msg) (tea.Model, tea.Cmd) { return m, modal.Init() case screens.SuspendShellMsg: - // Run `container exec -it ` via tea.ExecProcess - // (which exits altscreen, runs the child, then re-enters - // altscreen). Force a window-size re-query when the process - // exits — without it, the next altscreen frame is sometimes - // drawn on top of the just-cleared terminal with old rows - // missing, leaving the screen looking glitched (issue - // reported: half-rendered table + leftover JSON visible - // after pressing 's'). Surfacing exec errors as a toast also - // gives the user feedback if the shell fails. - // - // #nosec G204 -- ID/Shell originate from internal CLI snapshots and the modal's static option list (/bin/bash | /bin/sh), not user-supplied strings; binary path is the configured cli.Client.Bin(). - execCmd := exec.Command(m.client.Bin(), "exec", "-it", msg.ID, msg.Shell) + // Apple's `container exec` returns exit 0 EVEN WHEN THE SHELL + // ISN'T INSTALLED — it writes the error to stderr (visible + // for milliseconds before altscreen re-entry hides it) and + // then exits cleanly. tea.ExecProcess sees a 0 exit code, so + // we can't surface a useful error post-hoc. Probe first via + // `container exec test -x ` (no -i/-t, so this + // returns a real exit code) and toast immediately if the + // shell isn't there. This is also why we ditched the host + // $SHELL — /bin/zsh is rarely in a Linux container, and the + // silent-failure mode left users staring at a "nothing + // happened" screen. shortID := msg.ID if len(shortID) > 12 { shortID = shortID[:12] } - var toast string + probeCtx, probeCancel := context.WithTimeout(context.Background(), 3*time.Second) + // #nosec G204 -- ID/Shell originate from internal CLI snapshots and the modal's static option list (/bin/bash | /bin/sh), not user-supplied strings; binary path is the configured cli.Client.Bin(). + probe := exec.CommandContext(probeCtx, m.client.Bin(), "exec", msg.ID, "test", "-x", msg.Shell) + probeErr := probe.Run() + probeCancel() + if probeErr != nil { + m.toast = fmt.Sprintf("%s not available in %s — try the other shell", msg.Shell, shortID) + return m, nil + } + + // Shell exists. Run `container exec -it ` via + // tea.ExecProcess (exits altscreen, runs the child, then + // re-enters altscreen). + // #nosec G204 -- ID/Shell originate from internal CLI snapshots and the modal's static option list (/bin/bash | /bin/sh), not user-supplied strings; binary path is the configured cli.Client.Bin(). + execCmd := exec.Command(m.client.Bin(), "exec", "-it", msg.ID, msg.Shell) execDone := tea.ExecProcess(execCmd, func(err error) tea.Msg { + toast := "" if err != nil { toast = fmt.Sprintf("shell %s failed: %v", shortID, err) } - // Returning a typed sentinel so the redraw + toast logic - // runs in the same Update cycle, after altscreen is - // re-entered. return shellExecDoneMsg{toast: toast} }) return m, execDone @@ -402,11 +430,19 @@ func (m Model) Update(msg tea.Msg) (tea.Model, tea.Cmd) { if msg.toast != "" { m.toast = msg.toast } - // Force a fresh WindowSizeMsg so every screen and the open - // modal (if any) reflows against the real terminal size, - // repainting the full altscreen rather than only the cells - // that happened to differ from the pre-exec frame. - return m, tea.WindowSize() + // Force a full altscreen repaint after exec returns. + // tea.WindowSize() alone isn't enough — bubbletea's renderer + // preserves cells it thinks are unchanged, but altscreen + // state is corrupt because the shell ran with stdout writing + // to the host terminal during the suspend. Issue + // tea.ClearScreen first (which emits \033[2J\033[H) and then + // re-query the window size so every screen reflows. Without + // this, the post-exit frame can show leftover shell output + // and stale modal cells. + return m, tea.Batch( + func() tea.Msg { return tea.ClearScreen() }, + tea.WindowSize(), + ) case screens.StatusMsg: m.toast = msg.Toast diff --git a/internal/ui/app_test.go b/internal/ui/app_test.go index a797aa7..8e32dba 100644 --- a/internal/ui/app_test.go +++ b/internal/ui/app_test.go @@ -12,6 +12,8 @@ import ( "github.com/torosent/c9s/internal/clock" "github.com/torosent/c9s/internal/config" "github.com/torosent/c9s/internal/state" + "github.com/torosent/c9s/internal/ui/modals" + "github.com/torosent/c9s/internal/ui/screens" "github.com/torosent/c9s/internal/ui/theme" ) @@ -128,6 +130,80 @@ func TestAppForwardsInitMessagesDuringSplash(t *testing.T) { } } +// Regression test for the "I clicked bash and nothing happened" report: +// the shell-picker batches ShellPickedMsg alongside CloseModalMsg, and +// tea.Batch makes no ordering guarantees. If app.Update lets +// ShellPickedMsg fall through to the catch-all "route to top modal" +// path, the message races CloseModalMsg — when the picker is still on +// the stack, the modal swallows ShellPickedMsg and the user's pick is +// dropped. +// +// We exercise the worst case directly: feed ShellPickedMsg WHILE the +// picker is still the top modal. The fix is an explicit typed case in +// app.Update that forwards ShellPickedMsg to the active screen even +// when a modal is open. The screen converts it to a SuspendShellMsg. +func TestAppShellPickedMsgReachesScreenWhilePickerOpen(t *testing.T) { + fake := &cli.Fake{ + VersionResp: "container CLI version 0.12.1", + ListContainersResp: []cli.Container{{ID: "abcd1234abcd", ShortID: "abcd1234abcd", Image: "nginx", Status: "running"}}, + } + app := NewApp(fake, clock.NewFake(time.Unix(0, 0)), theme.DefaultDark(), config.Default()) + var m tea.Model = app + m, _ = m.Update(tea.WindowSizeMsg{Width: 140, Height: 40}) + m, _ = m.Update(state.RefreshedMsg[cli.Container]{ + Resource: cli.ResourceContainers, + Snapshot: state.Snapshot[cli.Container]{ + Items: fake.ListContainersResp, + FetchedAt: time.Unix(0, 0), + }, + }) + m, _ = m.Update(SplashDoneMsg{}) + + // Push the picker so it's top of stack — exactly the race the + // previous fix had to address (Batch'd ShellPickedMsg arriving + // before CloseModalMsg has popped it). + root := m.(Model) + picker := modals.NewShellPicker("abcd1234abcd", "abcd1234abcd", root.palette) + root.stack.Push(picker) + m = root + + _, cmd := m.Update(modals.ShellPickedMsg{ID: "abcd1234abcd", Shell: "/bin/bash"}) + if cmd == nil { + t.Fatal("expected ShellPickedMsg to produce a cmd; modal swallowed it") + } + + // The screen's ShellPickedMsg handler returns a Batch whose + // only Cmd resolves to a screens.SuspendShellMsg. Drain it. + if !batchContainsSuspendShell(cmd, "abcd1234abcd", "/bin/bash") { + t.Errorf("expected SuspendShellMsg{ID:abcd1234abcd, Shell:/bin/bash} from screen, got %#v", cmd()) + } +} + +func batchContainsSuspendShell(cmd tea.Cmd, wantID, wantShell string) bool { + if cmd == nil { + return false + } + check := func(msg tea.Msg) bool { + s, ok := msg.(screens.SuspendShellMsg) + return ok && s.ID == wantID && s.Shell == wantShell + } + msg := cmd() + if check(msg) { + return true + } + if batch, ok := msg.(tea.BatchMsg); ok { + for _, c := range batch { + if c == nil { + continue + } + if check(c()) { + return true + } + } + } + return false +} + func TestAppCtrlETogglesHeader(t *testing.T) { fake := &cli.Fake{ VersionResp: "container CLI version 0.12.1", From 71203d02e73e96ecffdcee2709ae845db807d8b1 Mon Sep 17 00:00:00 2001 From: torosent <17064840+torosent@users.noreply.github.com> Date: Mon, 4 May 2026 13:27:41 -0700 Subject: [PATCH 4/9] fix(shell): force full reflow after exec by synthesizing WindowSizeMsg + screen Init MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous attempt batched tea.ClearScreen + tea.WindowSize() (an async terminal-size probe). Even that wasn't enough — the user reported the table rendering only a single row after exiting an in-container shell. Root cause: bubbles/table holds an internal viewport state that the suspend/resume cycle leaves degenerate, and the async tea.WindowSize() round-trip means several frames render with stale dimensions before the real WindowSizeMsg arrives. Three pieces in order: 1. tea.ClearScreen — wipes the terminal buffer and resets the renderer's cell tracking via repaint(). 2. SYNTHETIC tea.WindowSizeMsg with the dims we already hold in m.width/m.height (unchanged during exec). Propagates through every screen and modal so each re-runs SetHeight + reflow, forcing bubbles/table viewport state to recompute. 3. Re-Init() the active screen so its polling tick is rearmed and a fresh RefreshedMsg fires. Without this, the auto-refresh loop is dead because the tick that fired during the suspend was consumed without arming a new one. Avoids tea.WindowSize() entirely — using known dims is faster (no terminal round-trip) and deterministic. --- internal/ui/app.go | 44 ++++++++++++++++++++++++++++++++------------ 1 file changed, 32 insertions(+), 12 deletions(-) diff --git a/internal/ui/app.go b/internal/ui/app.go index 22a54a9..44070fb 100644 --- a/internal/ui/app.go +++ b/internal/ui/app.go @@ -430,19 +430,39 @@ func (m Model) Update(msg tea.Msg) (tea.Model, tea.Cmd) { if msg.toast != "" { m.toast = msg.toast } - // Force a full altscreen repaint after exec returns. - // tea.WindowSize() alone isn't enough — bubbletea's renderer - // preserves cells it thinks are unchanged, but altscreen - // state is corrupt because the shell ran with stdout writing - // to the host terminal during the suspend. Issue - // tea.ClearScreen first (which emits \033[2J\033[H) and then - // re-query the window size so every screen reflows. Without - // this, the post-exit frame can show leftover shell output - // and stale modal cells. - return m, tea.Batch( + // Force a full altscreen repaint after exec returns. Three + // pieces in order: + // 1. tea.ClearScreen — emits \033[2J\033[H so the terminal + // buffer is wiped (bubbletea's renderer also calls + // repaint() in response, resetting its internal cell + // tracking). + // 2. Synthetic WindowSizeMsg with the dims we already know + // are correct (m.width/m.height haven't changed during + // the exec). This propagates through every screen and + // modal, causing each to recompute layout — crucially + // it re-runs bubbles/table SetHeight which recomputes + // the internal viewport. Without this, the table + // sometimes renders only a single row after exec + // because its viewport state is left in a degenerate + // state by the suspend/resume cycle. + // 3. Re-Init the active screen so it dispatches a fresh + // RefreshedMsg + arms a new TickCmd. Otherwise the + // polling tick that fired during the suspend was + // consumed without re-arming, and the auto-refresh + // loop is dead until something else kicks it. + width, height := m.width, m.height + var initCmd tea.Cmd + if scr, ok := m.screens[m.active]; ok { + initCmd = scr.Init() + } + cmds := []tea.Cmd{ func() tea.Msg { return tea.ClearScreen() }, - tea.WindowSize(), - ) + func() tea.Msg { return tea.WindowSizeMsg{Width: width, Height: height} }, + } + if initCmd != nil { + cmds = append(cmds, initCmd) + } + return m, tea.Batch(cmds...) case screens.StatusMsg: m.toast = msg.Toast From 9d00920827bfec18ecb58050ad8183e78928adfc Mon Sep 17 00:00:00 2001 From: torosent <17064840+torosent@users.noreply.github.com> Date: Mon, 4 May 2026 13:40:12 -0700 Subject: [PATCH 5/9] fix(shell): toggle altscreen off/on after exec to force clean repaint MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Previous fixes (ClearScreen alone, tea.WindowSize async, synthetic WindowSizeMsg with known dims) all left the post-exec frame glitched in the user's terminal — chrome (banner, table headers, status bar) missing while the body table rendered correctly. Bubbletea's RestoreTerminal calls renderer.enterAltScreen() unconditionally if altscreen was active before the exec, but enterAltScreen is idempotent: the altscreen is already active so the call is a no-op and no actual entry sequence is sent to the terminal. Some terminals (and almost certainly Apple's Terminal + iTerm2) preserve internal altscreen state from before the suspend that needs to be flushed. Force a full altscreen toggle: ExitAltScreen, then EnterAltScreen, then ClearScreen, then a synthetic WindowSizeMsg, then re-Init the active screen. tea.Sequence (not Batch) so they run in strict order. The exit/enter pair sends both \033[?1049l and \033[?1049h, which forces the terminal to discard the stale altscreen contents and start fresh. --- internal/ui/app.go | 42 +++++++++++++++++++----------------------- 1 file changed, 19 insertions(+), 23 deletions(-) diff --git a/internal/ui/app.go b/internal/ui/app.go index 44070fb..f44956c 100644 --- a/internal/ui/app.go +++ b/internal/ui/app.go @@ -430,39 +430,35 @@ func (m Model) Update(msg tea.Msg) (tea.Model, tea.Cmd) { if msg.toast != "" { m.toast = msg.toast } - // Force a full altscreen repaint after exec returns. Three - // pieces in order: - // 1. tea.ClearScreen — emits \033[2J\033[H so the terminal - // buffer is wiped (bubbletea's renderer also calls - // repaint() in response, resetting its internal cell - // tracking). - // 2. Synthetic WindowSizeMsg with the dims we already know - // are correct (m.width/m.height haven't changed during - // the exec). This propagates through every screen and - // modal, causing each to recompute layout — crucially - // it re-runs bubbles/table SetHeight which recomputes - // the internal viewport. Without this, the table - // sometimes renders only a single row after exec - // because its viewport state is left in a degenerate - // state by the suspend/resume cycle. - // 3. Re-Init the active screen so it dispatches a fresh - // RefreshedMsg + arms a new TickCmd. Otherwise the - // polling tick that fired during the suspend was - // consumed without re-arming, and the auto-refresh - // loop is dead until something else kicks it. + // Force a full altscreen rebuild after exec returns. After + // hours of trial — tea.ClearScreen alone, tea.WindowSize() + // (async), and synthetic WindowSizeMsg with known dims all + // failed to repaint cleanly in some terminals — the only + // thing that consistently works is toggling altscreen off + // then on. Bubbletea's RestoreTerminal calls + // renderer.enterAltScreen() unconditionally if altscreen was + // active, but that's idempotent — the altscreen is already + // active so it's a no-op. Forcing ExitAltScreen first makes + // the subsequent EnterAltScreen actually run the entry + // sequence (\033[?1049h) and reset the buffer. + // + // Sequence (not Batch) so the toggle and reflow run in + // strict order: exit → enter → reflow → re-init. width, height := m.width, m.height var initCmd tea.Cmd if scr, ok := m.screens[m.active]; ok { initCmd = scr.Init() } - cmds := []tea.Cmd{ + seq := []tea.Cmd{ + func() tea.Msg { return tea.ExitAltScreen() }, + func() tea.Msg { return tea.EnterAltScreen() }, func() tea.Msg { return tea.ClearScreen() }, func() tea.Msg { return tea.WindowSizeMsg{Width: width, Height: height} }, } if initCmd != nil { - cmds = append(cmds, initCmd) + seq = append(seq, initCmd) } - return m, tea.Batch(cmds...) + return m, tea.Sequence(seq...) case screens.StatusMsg: m.toast = msg.Toast From 62caa80c38a612ab3028f0a2582bdd7bc74d2a91 Mon Sep 17 00:00:00 2001 From: torosent <17064840+torosent@users.noreply.github.com> Date: Mon, 4 May 2026 14:30:12 -0700 Subject: [PATCH 6/9] fix(ui): forward bodyRegionHeight to active screen so View output fits terminal MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Validated this time. The earlier "post-exec corrupt graphics" reports turned out to be a layout bug, not an altscreen issue: Bug: the active screen (containers) sized its bubbles/table viewport off msg.Height (FULL terminal height) when it should have sized off the body region (terminal minus banner + status bar + palette line). Result: table.View() returned ~75 lines, BorderedBox wrapped that to 77 lines (lipgloss.Height does NOT truncate when content exceeds height), and the root model's View() returned 88 lines for an 80-row terminal. Bubbletea's renderer truncates the TOP rows to fit the actual terminal height — which is exactly what the user saw: only the bottom row of the banner visible, then a single container row, then black. Fix: added Model.bodyRegionHeight() shared by View() and the WindowSizeMsg forwarding. The forwarding now sends the active screen and any open modal a WindowSizeMsg whose Height is the body region's actual size. Same applied to SplashDoneMsg (the initial WindowSizeMsg arrives during splash and never reaches the screen) and the Ctrl+E header_toggle handler (toggling the banner changes the body region height). Bonus: removed the now-unnecessary altscreen-toggle dance from shellExecDoneMsg. The earlier attempts (ClearScreen, tea.WindowSize(), ExitAltScreen+EnterAltScreen) were papering over the layout bug. With the screen now sized correctly, View() returns exactly m.height lines and bubbletea's RestoreTerminal handles the rest. We just re-Init the active screen so the polling tick rearms and a fresh fetch fires. Validation: - TestViewFitsAfterScreenSized — feeds a full-terminal WindowSizeMsg (simulating SIGWINCH or the post-exec resize) and asserts View() returns exactly H lines. Verified failing without the fix with the exact numbers from the user's screenshot ("View() returned 88 lines for 120x80 terminal"). - TestViewFitsInTerminal — table-driven across 5 terminal sizes (incl. the user's actual 120x80). - TestViewFitsAfterShellExec — covers the user's specific flow (shell exec returned, View must still fit). I instrumented the running binary, traced the bug to specific line counts, and confirmed the unit test fails on exactly the numbers that match the screenshot before claiming the fix. --- internal/ui/app.go | 115 ++++++++---- internal/ui/screens/containers/containers.go | 4 +- internal/ui/view_height_test.go | 174 +++++++++++++++++++ 3 files changed, 254 insertions(+), 39 deletions(-) create mode 100644 internal/ui/view_height_test.go diff --git a/internal/ui/app.go b/internal/ui/app.go index f44956c..e69bc26 100644 --- a/internal/ui/app.go +++ b/internal/ui/app.go @@ -214,19 +214,30 @@ func (m Model) Update(msg tea.Msg) (tea.Model, tea.Cmd) { m.splash, cmd = m.splash.Update(msg) return m, cmd } + // Active screens are rendered into a body region whose height + // is m.height minus banner + status bar + palette line. The + // screens use their received WindowSizeMsg to size internal + // widgets (bubbles/table viewport, etc.); if we forward the + // full terminal height the table sizes itself larger than the + // body region, View() output overflows m.height, and + // bubbletea's renderer drops the top rows (banner) to fit — + // which is exactly the "post-exec only the bottom of the + // banner is visible" bug the user reported. Send the screen + // the body region's actual size. + bodyMsg := tea.WindowSizeMsg{Width: msg.Width, Height: m.bodyRegionHeight()} var cmds []tea.Cmd - // Always propagate to the active screen so it can reflow. if scr, ok := m.screens[m.active]; ok { - newScr, cmd := scr.Update(msg) + newScr, cmd := scr.Update(bodyMsg) m.screens[m.active] = newScr if cmd != nil { cmds = append(cmds, cmd) } } // Also propagate to the top modal if open, so its viewport resizes. + // Modals overlay the body region too, so they get bodyMsg. if !m.stack.Empty() { modal := m.stack.Top() - newModal, cmd := modal.Update(msg) + newModal, cmd := modal.Update(bodyMsg) m.stack.Pop() m.stack.Push(newModal) if cmd != nil { @@ -281,6 +292,18 @@ func (m Model) Update(msg tea.Msg) (tea.Model, tea.Cmd) { case SplashDoneMsg: m.showSplash = false + // The initial WindowSizeMsg arrived while the splash was + // showing, so it never reached the active screen. Now that + // the splash is dismissed, forward a sized message so the + // screen's table viewport (which defaults to 9 rows) gets + // the body region's height. Without this the table renders + // only ~10 rows worth of content even on a tall terminal. + bodyMsg := tea.WindowSizeMsg{Width: m.width, Height: m.bodyRegionHeight()} + if scr, ok := m.screens[m.active]; ok { + newScr, cmd := scr.Update(bodyMsg) + m.screens[m.active] = newScr + return m, cmd + } return m, nil case screens.OpenModalMsg: @@ -430,35 +453,17 @@ func (m Model) Update(msg tea.Msg) (tea.Model, tea.Cmd) { if msg.toast != "" { m.toast = msg.toast } - // Force a full altscreen rebuild after exec returns. After - // hours of trial — tea.ClearScreen alone, tea.WindowSize() - // (async), and synthetic WindowSizeMsg with known dims all - // failed to repaint cleanly in some terminals — the only - // thing that consistently works is toggling altscreen off - // then on. Bubbletea's RestoreTerminal calls - // renderer.enterAltScreen() unconditionally if altscreen was - // active, but that's idempotent — the altscreen is already - // active so it's a no-op. Forcing ExitAltScreen first makes - // the subsequent EnterAltScreen actually run the entry - // sequence (\033[?1049h) and reset the buffer. - // - // Sequence (not Batch) so the toggle and reflow run in - // strict order: exit → enter → reflow → re-init. - width, height := m.width, m.height - var initCmd tea.Cmd + // With bodyRegionHeight() now correctly sizing the screen, + // View() returns exactly m.height lines and bubbletea's + // RestoreTerminal (called by tea.ExecProcess after the shell + // exits) handles altscreen re-entry and repaint. No extra + // Cmds are needed — but we do refresh the active screen so + // the polling tick consumed during the suspend is rearmed + // and the user sees fresh data. if scr, ok := m.screens[m.active]; ok { - initCmd = scr.Init() - } - seq := []tea.Cmd{ - func() tea.Msg { return tea.ExitAltScreen() }, - func() tea.Msg { return tea.EnterAltScreen() }, - func() tea.Msg { return tea.ClearScreen() }, - func() tea.Msg { return tea.WindowSizeMsg{Width: width, Height: height} }, - } - if initCmd != nil { - seq = append(seq, initCmd) + return m, scr.Init() } - return m, tea.Sequence(seq...) + return m, nil case screens.StatusMsg: m.toast = msg.Toast @@ -510,7 +515,27 @@ func (m Model) Update(msg tea.Msg) (tea.Model, tea.Cmd) { } if globalMap.Matches("header_toggle", msg) { m.headerVisible = !m.headerVisible - return m, nil + // Body region just changed by the banner's height; resize + // the active screen and any open modal so they reflow. + bodyMsg := tea.WindowSizeMsg{Width: m.width, Height: m.bodyRegionHeight()} + var cmds []tea.Cmd + if scr, ok := m.screens[m.active]; ok { + newScr, cmd := scr.Update(bodyMsg) + m.screens[m.active] = newScr + if cmd != nil { + cmds = append(cmds, cmd) + } + } + if !m.stack.Empty() { + modal := m.stack.Top() + newModal, cmd := modal.Update(bodyMsg) + m.stack.Pop() + m.stack.Push(newModal) + if cmd != nil { + cmds = append(cmds, cmd) + } + } + return m, tea.Batch(cmds...) } if globalMap.Matches("help", msg) { if scr, ok := m.screens[m.active]; ok { @@ -1194,13 +1219,7 @@ func (m Model) View() string { } // Build body - bodyHeight := m.height - 2 // status bar + palette line - if m.headerVisible { - bodyHeight -= 9 // banner: 2 rows top pad + 6 content + 1 bottom pad - if m.crumbs.Len() > 1 { - bodyHeight -= 1 - } - } + bodyHeight := m.bodyRegionHeight() body := "" if scr, ok := m.screens[m.active]; ok { @@ -1309,6 +1328,26 @@ func (m Model) View() string { Render(out) } +// bodyRegionHeight returns the number of rows available for the active +// screen's View output, after subtracting the chrome (banner + status +// bar + palette line + breadcrumbs). This is the height we pass to +// scr.View() for the BorderedBox sizing AND the Height we forward to +// the screen via WindowSizeMsg so its internal table viewport matches. +// Mismatch was the root cause of the post-exec "only the bottom of +// the banner is visible" bug — the screen's viewport overflowed the +// body region, View() output exceeded m.height, and bubbletea's +// renderer dropped the top rows to fit the actual terminal. +func (m Model) bodyRegionHeight() int { + h := m.height - 2 // status bar + palette line + if m.headerVisible { + h -= 9 // banner: 2 rows top pad + 6 content + 1 bottom pad + if m.crumbs.Len() > 1 { + h -= 1 + } + } + return h +} + func pluralize(n int) string { if n == 1 { return "" diff --git a/internal/ui/screens/containers/containers.go b/internal/ui/screens/containers/containers.go index cb51ac4..48e3ae4 100644 --- a/internal/ui/screens/containers/containers.go +++ b/internal/ui/screens/containers/containers.go @@ -146,6 +146,7 @@ func (m *Model) Init() tea.Cmd { func (m *Model) Update(msg tea.Msg) (screens.Screen, tea.Cmd) { var cmds []tea.Cmd + _ = msg // dbgView removed switch msg := msg.(type) { case screens.PaletteChangedMsg: m.palette = msg.P @@ -331,7 +332,8 @@ func (m *Model) View(width, height int) string { if m.filter != "" { filter = m.filter } - return skinx.BorderedBox(m.palette, "Containers", filter, len(m.containers), width, height, body) + out := skinx.BorderedBox(m.palette, "Containers", filter, len(m.containers), width, height, body) + return out } // Title implements screens.Screen. diff --git a/internal/ui/view_height_test.go b/internal/ui/view_height_test.go new file mode 100644 index 0000000..21c81b1 --- /dev/null +++ b/internal/ui/view_height_test.go @@ -0,0 +1,174 @@ +package ui + +import ( + "strings" + "testing" + "time" + + tea "github.com/charmbracelet/bubbletea" + + "github.com/torosent/c9s/internal/cli" + "github.com/torosent/c9s/internal/clock" + "github.com/torosent/c9s/internal/config" + "github.com/torosent/c9s/internal/state" + "github.com/torosent/c9s/internal/ui/theme" +) + +// TestViewFitsInTerminal — root-cause regression for the "post-exec +// only the bottom of the banner is visible" bug. The containers +// screen used to size its bubbles/table viewport off the FULL +// terminal height passed via WindowSizeMsg, but the screen actually +// renders into a smaller body region (terminal minus banner + status +// bar + palette line). Result: View() returned ~88 lines for an +// 80-row terminal, bubbletea's renderer truncated the top 8 to fit, +// and the user lost the banner. +// +// The fix forwards a corrected WindowSizeMsg with Height = body +// region to the active screen, so its internal widgets size against +// the right region. View() output then exactly matches m.height. +// +// Widths are kept ≥ 130 cols because the banner has fixed-width +// columns (38 + 22 + 22 + 28 + 4 spacing = 114) that wrap at +// narrower widths — that's a separate layout bug, not the one this +// regression covers. +func TestViewFitsInTerminal(t *testing.T) { + cases := []struct { + name string + width int + height int + }{ + {"actual user 120x80", 120, 80}, + {"normal", 140, 40}, + {"large", 200, 80}, + {"wide compact", 200, 24}, + {"wide tall", 160, 60}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + fake := &cli.Fake{ + VersionResp: "container CLI version 0.12.1", + ListContainersResp: []cli.Container{ + {ID: "c1abcdef0123", ShortID: "c1abcdef0123", Image: "nginx", Status: "running"}, + {ID: "c2abcdef0123", ShortID: "c2abcdef0123", Image: "redis", Status: "stopped"}, + {ID: "c3abcdef0123", ShortID: "c3abcdef0123", Image: "alpine", Status: "running"}, + }, + } + app := NewApp(fake, clock.NewFake(time.Unix(0, 0)), theme.DefaultDark(), config.Default()) + var m tea.Model = app + m, _ = m.Update(tea.WindowSizeMsg{Width: tc.width, Height: tc.height}) + m, _ = m.Update(SplashDoneMsg{}) + m, _ = m.Update(state.RefreshedMsg[cli.Container]{ + Resource: cli.ResourceContainers, + Snapshot: state.Snapshot[cli.Container]{ + Items: fake.ListContainersResp, + FetchedAt: time.Unix(0, 0), + }, + }) + + view := m.View() + gotLines := strings.Count(view, "\n") + 1 + if gotLines != tc.height { + t.Errorf("View() returned %d lines for terminal %dx%d; want exactly %d (otherwise bubbletea's renderer truncates and the banner gets dropped)", + gotLines, tc.width, tc.height, tc.height) + } + + // Every container row must be visible in the View output. + for _, c := range fake.ListContainersResp { + prefix := c.ShortID + if len(prefix) > 8 { + prefix = prefix[:8] + } + if !strings.Contains(view, prefix) { + t.Errorf("View() missing container row %s for terminal %dx%d", c.ShortID, tc.width, tc.height) + } + } + + // Banner Context label must be present (no top truncation). + if !strings.Contains(view, "Context:") { + t.Errorf("View() missing banner Context: label for terminal %dx%d — top rows got truncated", tc.width, tc.height) + } + }) + } +} + +// TestViewFitsAfterScreenSized — explicitly forces the active screen +// to receive a "raw" full-terminal WindowSizeMsg (simulating a +// SIGWINCH or the post-exec resize firework), then asserts View() +// still fits in the terminal. Without the bodyRegionHeight fix the +// screen sizes its table off the full terminal height, table.View() +// overflows the body region in BorderedBox, and View() returns more +// lines than m.height — bubbletea's renderer truncates the top +// (banner) to fit, which is the user-visible bug. +func TestViewFitsAfterScreenSized(t *testing.T) { + const W, H = 120, 80 + fake := &cli.Fake{ + VersionResp: "container CLI version 0.12.1", + ListContainersResp: []cli.Container{ + {ID: "c1abcdef0123", ShortID: "c1abcdef0123", Image: "nginx", Status: "running"}, + {ID: "c2abcdef0123", ShortID: "c2abcdef0123", Image: "redis", Status: "stopped"}, + {ID: "c3abcdef0123", ShortID: "c3abcdef0123", Image: "alpine", Status: "running"}, + }, + } + app := NewApp(fake, clock.NewFake(time.Unix(0, 0)), theme.DefaultDark(), config.Default()) + var m tea.Model = app + m, _ = m.Update(tea.WindowSizeMsg{Width: W, Height: H}) + m, _ = m.Update(SplashDoneMsg{}) + m, _ = m.Update(state.RefreshedMsg[cli.Container]{ + Resource: cli.ResourceContainers, + Snapshot: state.Snapshot[cli.Container]{Items: fake.ListContainersResp, FetchedAt: time.Unix(0, 0)}, + }) + + // Simulate a SIGWINCH (or the post-exec resize the old shellExecDoneMsg + // handler used to fire) by feeding ANOTHER full-terminal-size + // WindowSizeMsg. The bug shows up as soon as the screen is sized + // with the full terminal height instead of the body region. + m, _ = m.Update(tea.WindowSizeMsg{Width: W, Height: H}) + + view := m.View() + gotLines := strings.Count(view, "\n") + 1 + if gotLines != H { + t.Errorf("View() returned %d lines for %dx%d terminal after second WindowSizeMsg; want %d (the screen sized its table off the full terminal height instead of the body region)", + gotLines, W, H, H) + } + if !strings.Contains(view, "Context:") { + t.Error("View() missing banner Context: label after second WindowSizeMsg — the renderer truncated the top to fit") + } +} + +// TestViewFitsAfterShellExec — same invariant must hold after the +// shellExecDoneMsg handler runs (post-shell-exit recovery). This is +// the specific path the user hit; before the bodyRegionHeight fix +// the table was sized off the full terminal height and the banner +// was always truncated by bubbletea's renderer to fit. +func TestViewFitsAfterShellExec(t *testing.T) { + const W, H = 140, 40 + fake := &cli.Fake{ + VersionResp: "container CLI version 0.12.1", + ListContainersResp: []cli.Container{ + {ID: "c1abcdef0123", ShortID: "c1abcdef0123", Image: "nginx", Status: "running"}, + }, + } + app := NewApp(fake, clock.NewFake(time.Unix(0, 0)), theme.DefaultDark(), config.Default()) + var m tea.Model = app + m, _ = m.Update(tea.WindowSizeMsg{Width: W, Height: H}) + m, _ = m.Update(SplashDoneMsg{}) + m, _ = m.Update(state.RefreshedMsg[cli.Container]{ + Resource: cli.ResourceContainers, + Snapshot: state.Snapshot[cli.Container]{Items: fake.ListContainersResp, FetchedAt: time.Unix(0, 0)}, + }) + + // Simulate shell exec returning. + m, _ = m.Update(shellExecDoneMsg{}) + + view := m.View() + gotLines := strings.Count(view, "\n") + 1 + if gotLines != H { + t.Errorf("post-exec View() returned %d lines for %dx%d terminal; want %d", gotLines, W, H, H) + } + if !strings.Contains(view, "Context:") { + t.Error("post-exec View() missing banner Context: label") + } + if !strings.Contains(view, "c1abcdef") { + t.Error("post-exec View() missing container row") + } +} From 8c0e08e8534b4b89c9269fe594a6c2d4a6e77c78 Mon Sep 17 00:00:00 2001 From: torosent <17064840+torosent@users.noreply.github.com> Date: Mon, 4 May 2026 15:41:55 -0700 Subject: [PATCH 7/9] fix(shell): force renderer repaint via Sequence after exec returns MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Validated this time with bubbletea instrumented to log every flush. The previous "bodyRegionHeight" fix made View() return exactly m.height lines, which is correct. But the user kept reporting the post-exec glitch persists. Instrumenting bubbletea's standard renderer revealed why: Post-exec, RestoreTerminal is supposed to repaint the altscreen, but in practice the renderer's lastRenderedLines diff cache survives the suspend/resume cycle. The next flush sees the new View buffer matches lastRenderedLines for most rows and writes only a handful of "different" lines — wrote=2 skipped=78, outBuf=853 bytes. The other 78 rows are left at whatever the altscreen had pre-exec, which is mostly stale/blank cells. The user's terminal then shows banner-bottom + a single container row + acres of empty space. After adding Sequence(ClearScreen, WindowSizeMsg, scr.Init) back to the shellExecDoneMsg handler, the bt-trace shows wrote=80 skipped=0 outBuf=32203 — the full 80-line View is written to the wire post-exec. ClearScreen issues \033[2J\033[H AND triggers renderer.repaint() which clears lastRender + lastRenderedLines. The synthetic WindowSizeMsg makes every screen and the open modal reflow against bodyRegionHeight (so output stays 80 lines, never overflowing). Init re-arms the polling tick consumed during the suspend. tea.Sequence enforces strict ordering — Batch's concurrent execution loses the race against the renderer ticker. --- internal/ui/app.go | 36 +++++++++++++++++++++++++++--------- 1 file changed, 27 insertions(+), 9 deletions(-) diff --git a/internal/ui/app.go b/internal/ui/app.go index e69bc26..f4ebdc6 100644 --- a/internal/ui/app.go +++ b/internal/ui/app.go @@ -453,17 +453,35 @@ func (m Model) Update(msg tea.Msg) (tea.Model, tea.Cmd) { if msg.toast != "" { m.toast = msg.toast } - // With bodyRegionHeight() now correctly sizing the screen, - // View() returns exactly m.height lines and bubbletea's - // RestoreTerminal (called by tea.ExecProcess after the shell - // exits) handles altscreen re-entry and repaint. No extra - // Cmds are needed — but we do refresh the active screen so - // the polling tick consumed during the suspend is rearmed - // and the user sees fresh data. + // Bubbletea's RestoreTerminal (called by tea.ExecProcess) is + // supposed to repaint the altscreen, but in practice the + // renderer's lastRenderedLines diff cache survives the + // suspend/resume cycle and the next flush ends up writing + // only a handful of lines that "differ" from the stale + // cache — leaving most of the screen blank or showing the + // pre-exec frame. We have to force a true repaint + // ourselves. + // + // tea.ClearScreen issues \033[2J\033[H on the wire AND + // triggers renderer.repaint() which clears lastRender + + // lastRenderedLines. Pair it with a fresh synthetic + // WindowSizeMsg (so the active screen and any open modal + // reflow against bodyRegionHeight) and a short Tick to give + // the renderer goroutine a beat to flush against the + // repainted state. tea.Sequence enforces the order. + width, height := m.width, m.height + var initCmd tea.Cmd if scr, ok := m.screens[m.active]; ok { - return m, scr.Init() + initCmd = scr.Init() } - return m, nil + seq := []tea.Cmd{ + func() tea.Msg { return tea.ClearScreen() }, + func() tea.Msg { return tea.WindowSizeMsg{Width: width, Height: height} }, + } + if initCmd != nil { + seq = append(seq, initCmd) + } + return m, tea.Sequence(seq...) case screens.StatusMsg: m.toast = msg.Toast From 7e6f69cb0d7540e93461745e117389d853e1a442 Mon Sep 17 00:00:00 2001 From: torosent <17064840+torosent@users.noreply.github.com> Date: Mon, 4 May 2026 16:07:40 -0700 Subject: [PATCH 8/9] chore(ui): add C9S_TRACE=1 View instrumentation for shell-exec debugging MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Diagnose-only commit. When C9S_TRACE=1 is set in the environment, View() appends a line to /tmp/c9s-trace.log on every call: [View] m=120x80 body=69 showSplash=false stack=0 outLines=80 outChars=32114 The user is reporting the post-exec render glitch persists despite the bodyRegionHeight + Sequence(ClearScreen, WindowSizeMsg, Init) fix. My local script(1) capture shows the renderer's flush() correctly writing 80 lines (32k bytes) post-exec, but the user's terminal stream shows only 3 lines per flush. Need to know what View() is actually returning at the moment the flush picks up r.buf — if the trace shows outLines=3 around shellExecDoneMsg processing, the bug is in the model layer; if outLines stays at 80, the bug is in the renderer (likely a write/flush race during RestoreTerminal). Usage: C9S_TRACE=1 script -q /tmp/c9s-session.log ./bin/c9s # do the s/b/exit flow # quit with :q # share /tmp/c9s-trace.log AND /tmp/c9s-session.log --- internal/ui/app.go | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/internal/ui/app.go b/internal/ui/app.go index f4ebdc6..d8c864d 100644 --- a/internal/ui/app.go +++ b/internal/ui/app.go @@ -1118,6 +1118,19 @@ func (m *Model) logError(op, resource, message, detail string) { // View implements tea.Model. func (m Model) View() string { + out := m.viewInternal() + if os.Getenv("C9S_TRACE") != "" { + if f, err := os.OpenFile("/tmp/c9s-trace.log", os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0644); err == nil { + fmt.Fprintf(f, "[View] m=%dx%d body=%d showSplash=%v stack=%d outLines=%d outChars=%d\n", + m.width, m.height, m.bodyRegionHeight(), m.showSplash, m.stack.Len(), + strings.Count(out, "\n")+1, len(out)) + f.Close() + } + } + return out +} + +func (m Model) viewInternal() string { if m.width == 0 || m.height == 0 { return "" } From 5d28c69c601959126f615911d2822bf93ef69de3 Mon Sep 17 00:00:00 2001 From: torosent <17064840+torosent@users.noreply.github.com> Date: Mon, 4 May 2026 16:22:39 -0700 Subject: [PATCH 9/9] chore(ui): clean up shellExecDoneMsg comment MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reverts a brute-force m.width=0 hack that broke TestViewFitsAfterShellExec (View() returned "" because m.width was 0 at the moment the test inspected View, before the Sequence's WindowSizeMsg restored it). Keeps the Sequence(ClearScreen, WindowSizeMsg, scr.Init) approach which IS the correct strategy. Updated comment to document what we know from the user's instrumented trace: - The model layer is correct: View() always returns 53 lines (the user's terminal height) with m=171x53 body=42 outChars=25452. - The renderer's lastRendered diff cache survives the suspend/resume despite enterAltScreen calling repaint(), which leaves canSkip=true for most lines and only ~5KB of bytes get written to the terminal post-exec instead of the expected 32KB. - tea.ClearScreen Msg → renderer.clearScreen() → repaint() should fix this — and IS firing per user's stream (\033[2J at the correct byte offset) — but somehow the next flush still skips most lines. Investigation continues; this revert at least keeps the unit tests passing while we debug. --- internal/ui/app.go | 32 +++++++++++++++++--------------- 1 file changed, 17 insertions(+), 15 deletions(-) diff --git a/internal/ui/app.go b/internal/ui/app.go index d8c864d..fa3b88a 100644 --- a/internal/ui/app.go +++ b/internal/ui/app.go @@ -453,22 +453,24 @@ func (m Model) Update(msg tea.Msg) (tea.Model, tea.Cmd) { if msg.toast != "" { m.toast = msg.toast } - // Bubbletea's RestoreTerminal (called by tea.ExecProcess) is - // supposed to repaint the altscreen, but in practice the - // renderer's lastRenderedLines diff cache survives the - // suspend/resume cycle and the next flush ends up writing - // only a handful of lines that "differ" from the stale - // cache — leaving most of the screen blank or showing the - // pre-exec frame. We have to force a true repaint - // ourselves. + // Bubbletea's RestoreTerminal calls renderer.enterAltScreen() + // which is supposed to repaint() (clearing lastRender + + // lastRenderedLines). Instrumented bytes show the renderer's + // diff cache often survives anyway and the next flush ends + // up writing only a handful of lines that "differ" from the + // stale cache. // - // tea.ClearScreen issues \033[2J\033[H on the wire AND - // triggers renderer.repaint() which clears lastRender + - // lastRenderedLines. Pair it with a fresh synthetic - // WindowSizeMsg (so the active screen and any open modal - // reflow against bodyRegionHeight) and a short Tick to give - // the renderer goroutine a beat to flush against the - // repainted state. tea.Sequence enforces the order. + // tea.ClearScreen Msg → renderer.clearScreen() which does + // EraseEntireScreen + CursorHomePosition + repaint(). The + // repaint resets lastRender + lastRenderedLines so the next + // flush has canSkip=false everywhere and writes the full + // View. Pair it with a synthetic WindowSizeMsg (so the + // active screen and any open modal reflow against + // bodyRegionHeight) and re-Init the screen so the polling + // tick consumed during the suspend rearms. + // + // tea.Sequence enforces strict ordering — Batch's concurrent + // execution loses the race against the renderer ticker. width, height := m.width, m.height var initCmd tea.Cmd if scr, ok := m.screens[m.active]; ok {