render: interactive tab switching with per-surface active-tab state - #50
Conversation
Tabs previously rendered every title but only the first tab's content, with no way to switch. Now the tab bar joins the focus ring: when it holds focus, left/right (or vi-style h/l) switch the active tab, wrapping at either end. The active title renders bold normally and reverse-video while the bar holds focus, consistent with button focus chrome, keeping the monochrome discipline. Active-tab state lives on the Surface keyed by component ID, and — like focus — survives Apply/applyComponents merges; deleteSurface wipes it. An index left out-of-range by an update that shrank the tab list clamps back to the first tab at read time. The focus ring includes only the active tab's descendants, so focus can never land on a component hidden in an inactive tab; switching re-collects the ring and keeps focus on the bar. h/l still type into a focused text field. Fixes #45 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
joestump
left a comment
There was a problem hiding this comment.
Adversarial review of the tab-switching implementation against #45. Verified at head e61d02f with go build ./... && go vet ./... && go test -count=1 ./... all green locally.
Blockers
None. Specifically hunted and cleared:
- Key-routing regressions (
render/render.go:246-260): the new"left","h"/"right","l"cases reproduce the old default-case behavior byte-for-byte when the focused component is not a tab bar —h/lstill type into a focused TextField viakey.Text, arrows stay no-ops there (key.Textis empty), and buttons are unaffected.Standaloneonly interceptsctrl+c/esc/q, so nothing upstream swallows the new keys. - Wrap arithmetic (
render/render.go:408):((activeTab+delta)%n + n) % nis correct for delta ±1 from a clamped base in[0,n). - Panic safety in
switchTab(render/render.go:395-417): the focused tab bar is always reachable from the root via a path that does not traverse its own tabs, so the post-switchcollectFocusablesalways re-collects it and thefocusIdxrestore loop always finds it — no out-of-rangefocusIdx. - State preservation (
render/composite.go:71-94):applyComponentsleavesactiveTabsuntouched (matching the focus-preservation precedent),deleteSurfacewipes it, and the read-time clamp in bothrenderTabsandcollectFocusablesprevents indexing past a shrunken tab list. Focus preservation composes correctly with clamping (a focused button inside a removed tab falls back tofocusIdx = 0). - Focus-ring hygiene (
render/render.go:569-578): only the ACTIVE tab's subtree is walked, so focusables inside hidden tabs leave the ring; empty Tabs stay out of it. - Chrome discipline:
Heading(bold) /ButtonFocused(bold+reverse) only — monochrome, host themes win. - Test quality:
render/tabs_test.goasserts observable behavior (rendered content viaansi.Strip, ring membership, exact chrome bytes), not mere execution, and covers every Done-when item: switching (arrows + h/l), wrap-around, blur guard, Apply survival, out-of-range clamp, delete/re-create reset, and the h/l-into-TextField non-regression.
Ticket compliance: all #45 Done-when items are met. The website/ "Not yet" copy is correctly deferred to the post-merge docs pass per repo policy, and the PR body says so.
Nits (non-blocking)
- Stale active-tab "resurrection" —
render/render.go:423-429(activeTab). The clamp is read-time only and never rewritess.activeTabs, so this sequence: user selects tab index 2 → a merge shrinks the list to 2 tabs (renders tab 0) → a later merge grows it back to 3+ tabs → the view silently jumps back to index 2 with no user input. Arguably a feature (selection survives a transient shrink), but it contradicts the "falls back to the first tab" phrasing in the doc comments. Either clear/rewrite the stored entry when the clamp fires, or document the resurrection as intended. - Stale test name —
render/placeholders_test.go:16still names the testTestTabsRenderTitleBarAndFirstChildwhile its updated comment describes "the default Tabs rendering … the ACTIVE tab".TestTabsRenderTitleBarAndActiveChild(or similar) would match.
Approving-quality change: no verified correctness defects, ticket contract satisfied.
🤖 Posted on behalf of @joestump by Claude.
Union merge of interactive tab switching (#45) with the five feature PRs that landed on main since the branch forked (#48 createSurface no-op, #49 ChildList templates, #51 Modal open/close, #52 editable inputs, #53 InputSubmitted/ChoiceSelected dispatch). Nothing from either side was dropped: - render/render.go: Surface keeps ALL state — activeTabs (branch) plus fieldValues/checkValues/choiceValues/sliderValues/choiceCursor/ openModals/scope (main). isInteractive now also covers non-empty Tabs bars. collectFocusables walks only the ACTIVE tab's subtree, so inputs and modals inside inactive tabs are excluded from the focus ring, and only an open modal's content joins it. Key routing: left/right step a focused slider and switch tabs on a focused tab bar; h/l switch tabs only on the tab bar and stay literal runes on a focused TextField/DateTimeInput; enter/space/esc keep main's button/checkbox/choice/modal/InputSubmitted behavior. - render/composite.go: deleteSurface clears every state map including activeTabs; applyComponents preserves active-tab, focus, modal, and edited-value state across merges, with out-of-range tabs clamping to the first at read time. - render/tabs_test.go: two new tests pin the union — slider-focused vs tab-bar-focused left/right disambiguation, and inputs/modals inside an inactive tab excluded from the focus ring. - README.md / docs/wire-format.md: merged feature descriptions; the 'not yet' lists are now empty (issues #42–#47 all landed). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Merged Conflicted files and how each was resolved
Tests added (
Gate: 🤖 Posted on behalf of |
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
What
Tabs are now interactive. The tab bar joins the focus ring; when it holds focus,
left/right(or vi-styleh/l) switch the active tab and its content renders in place of the old always-first-tab behavior.How
Surface.activeTabsmaps Tabs component ID → active index, lazily initialized. A missing entry means the first tab.collectFocusablescollects the tab bar itself (when it has tabs) and recurses only into the ACTIVE tab's subtree, so focus can never land on a component hidden inside an inactive tab.switchTabre-collects the ring after each switch and keeps focus on the bar.Heading) normally and reverse-video (ButtonFocused) while the bar holds focus — same monochrome discipline as button focus chrome, so host themes win.applyComponentsleavesactiveTabsuntouched (like focus, it's user state the server must not clobber);deleteSurfacewipes it. Out-of-range indices left behind by a merge that shrank the tab list clamp back to the first tab at read time (activeTab), so rendering never indexes past the end.h/l: the new key cases fall through to text-field editing when the focused component isn't a tab bar.Surface.ActiveTab(id)mirrorsFocusables()for hosts/tests.Tests
render/tabs_test.go(all green withgo build ./... && go vet ./... && go test -count=1 ./...):Applymerges (updated active-tab content renders, no reset to first)deleteSurface+ re-create resets to the first tabDocs: updated
README.mdanddocs/wire-format.md"Not yet" lists. Thewebsite/copy of the docs is deliberately untouched — the website docs "Not yet" update is deferred to the post-merge docs pass.Fixes #45
🤖 Posted on behalf of
@joestumpby Claude.