Add tab support#3973
Conversation
K9s now supports multiple tabs to allow quick switching between different views and resources. Users can open up to 9 tabs simultaneously, with each tab maintaining its own isolated view stack, command interpreter, and navigation/filter histories. Key bindings have been added for tab management: - `Ctrl-t`: Open a new tab pre-loaded with the current resource. - `Ctrl-x`: Close the active tab (no-op if it's the last tab). - `Ctrl-n`: Switch to the next tab, wrapping around. - `Ctrl-b`: Switch to the previous tab, wrapping around.
When switching contexts, close all tabs except the currently active one to prevent outdated state from persisting.
This change centralizes the logic for switching namespaces within the `App` struct. Previously, individual views directly called `app.Config.SetActiveNamespace`, which has now been replaced by a call to `app.switchNS`. The `TabManager` now also has a `switchNS` method to update all relevant sessions and their content when the namespace changes.
Introduces a unified `dao.Scaler` interface and implementation to handle resource scaling logic, replacing direct type assertions. This simplifies the code and makes it more robust when dealing with different resource types.
SebTardif
left a comment
There was a problem hiding this comment.
PR Review: Add tab support
+551 / -25 across 11 files
The design decomposition (TabSession / TabManager / TabBar) is clean and the overall direction is sound. Four independent review passes (code quality, error handling, type design, simplification) identified several issues worth addressing before merge.
Critical Issues (4)
1. Missing Stop() on outgoing tab — goroutine leak on every tab switch
internal/view/tab_manager.go — activateSession()
When switching tabs, Start() is called on the incoming tab's top component, but Stop() is never called on the outgoing tab's top component. In k9s, Start()/Stop() manage data-watch goroutines, informer subscriptions, and poll tickers. Each tab switch leaks the old tab's goroutines. When the user switches back, Start() creates a second set of watchers on top of the still-running first set. After N round-trip switches, N watcher goroutines are hammering the API server.
Fix: Before detaching listeners, stop the outgoing component:
if oldTop := oldContent.Top(); oldTop != nil {
oldTop.Stop()
}2. Nil-dereference panic in switchNS() on chained method calls
internal/view/tab_manager.go — switchNS(), the else branch
rv.GetTable().GetModel().SetNamespace(ns)If GetTable() or GetModel() returns nil (plausible for stopped views, detail views, or YAML viewers that aren't *Browser), this panics and kills the entire TUI session. This iterates every component in every tab's stack, making the blast radius high.
Fix: Guard the chain:
if tbl := rv.GetTable(); tbl != nil {
if m := tbl.GetModel(); m != nil {
m.SetNamespace(ns)
}
}3. Contradictory concurrency strategy — mutex provides false safety, TOCTOU races throughout
internal/view/tab_manager.go — newTab(), closeActive(), SwitchTo(), NextTab(), PrevTab()
The TabManager uses sync.RWMutex but every method documents "must be called on the tview main goroutine." The code reads state under the lock, releases it, then acts on the stale values:
newTab(): ChecksmaxTabsunder lock, releases, does init work, re-locks to append. Two concurrent calls could both pass the guard.closeActive(): ComputesnextIdxunder lock, releases, callsactivateSession()which re-locks —idxmay be stale.SwitchTo()/NextTab()/PrevTab(): Readcount/curunder RLock, release, then callactivateSession()with potentially out-of-bounds index.switchNS()holds an RLock while mutating session state — semantically incorrect.
Fix: Pick one strategy: either (a) remove the mutex entirely and document that all methods are tview-main-goroutine-only, or (b) hold the lock across the full read-check-act sequences.
4. Initialized PageStack leaked when Command.Init() fails in newTab()
internal/view/tab_manager.go — newTab()
Resources are created sequentially: Content.Init() then cmd.Init(). If cmd.Init() fails, the already-initialized PageStack is never Clear() 'd — it becomes an orphaned, initialized UI primitive.
Fix:
if err := cmd.Init(tm.app.Config.ContextAliasesPath()); err != nil {
sess.Content.Clear()
return fmt.Errorf("init tab command: %w", err)
}Important Issues (6)
5. newTabCmd() — gotoResource() return value silently discarded
internal/view/app.go — newTabCmd()
If newTab() succeeds but gotoResource() fails, the user is left on a fully activated but completely empty tab with no flash message.
6. CloseOtherTabs() doesn't detach listeners before Clear()
internal/view/tab_manager.go — CloseOtherTabs()
Sessions being closed may still have crumbs/menu listeners attached. Clear() fires StackTop() callbacks that propagate to those listeners, potentially corrupting the active display. Call RemoveListener for crumbs and menu on each session before Clear().
7. scale_extender.go behavioral change — risk of accidental scale-to-zero
internal/view/scale_extender.go — makeScaleForm()
The old code returned an error when replica count couldn't be determined. The new code silently logs a warning and renders the form with "0" pre-filled. A user confirming without editing would scale their workload to zero.
8. activateSession() directly swaps 4 App fields — fragile coupling
internal/view/tab_manager.go — activateSession()
TabManager reaches into App and swaps Content, command, cmdHistory, filterHistory by direct assignment. If App ever adds a new per-session field, activateSession must be updated in lockstep with no compile-time guard. Consider grouping these into a session-state struct.
9. Stack.Repopulate() is an exported backdoor bypassing lifecycle invariants
internal/model/stack.go
This new exported method replaces stack contents without calling Stop/Start or notifying listeners. Any StackListener is silently out of sync. Consider making it unexported.
10. Unrelated scale_extender.go refactor bundled into tabs PR
The scale-extender changes are entirely unrelated to tab support. This makes the PR harder to review, revert, and bisect. Consider extracting into a separate PR.
Suggestions
| # | Area | Suggestion |
|---|---|---|
| S1 | Tests | No tests for tab lifecycle (create, switch, close, maxTabs, edge cases). Only change is incrementing action count 14→18. |
| S2 | Theming | TabBar.Render() hardcodes [black:white:b] and [gray:-:-] color tags. These won't respect user themes despite styles being available. |
| S3 | Key conflicts | Ctrl-N/Ctrl-B conflict with tmux default prefix and terminal conventions. Consider making configurable. |
| S4 | Encapsulation | TabManager, TabSession are exported but only used within internal/view. Unexport them. |
| S5 | Clarity | switchNS() contains deep Kubernetes domain logic. Move to TabSession.switchNS() and have TabManager just iterate and delegate. |
| S6 | Listener cleanup | NewTabBar calls styles.AddListener(t) but never unregisters. |
| S7 | Simplification | Repopulate can use slices.Clone(). Duplicated focus-restore blocks in closeActive/CloseOtherTabs should be extracted. |
Strengths
- Clean domain decomposition —
TabSession/TabManager/TabBarare well-scoped. - Auto-hiding tab bar — single-tab users see zero UI impact.
- Context switch closes all tabs — prevents stale cross-cluster tabs.
- Monotonic tab IDs avoid page-name collisions after close/reopen.
- Defensive copy in
Repopulateprevents aliasing bugs. InCmdMode()guard on all tab key handlers.- Wrap-around navigation matches browser UX expectations.
Recommendation: Fix critical issues 1-4 before merge. The goroutine leak (#1) and nil panic (#2) will cause real production problems. The mutex inconsistency (#3) is a ticking time bomb. Add tab lifecycle tests as a follow-up.
K9s now supports multiple tabs to allow quick switching between different views and resources. Users can open up to 9 tabs simultaneously, with each tab maintaining its own isolated view stack, command interpreter, and navigation/filter histories.
Key bindings have been added for tab management:
Ctrl-t: Open a new tab pre-loaded with the current resource.Ctrl-x: Close the active tab (no-op if it's the last tab).Ctrl-n: Switch to the next tab, wrapping around.Ctrl-b: Switch to the previous tab, wrapping around.