From 93a0a5a0d1261428398329e285557e79f657fd6c Mon Sep 17 00:00:00 2001 From: Hai Huang Date: Tue, 22 Sep 2026 15:06:28 -0400 Subject: [PATCH 1/4] feat(abctl): make the [?] overlay a map of the panes, not a key dump The overlay answered "what does this key do" and never "what screens are there, and how do I reach them". Four things made that so: - OTHER PANES compacted each pane to its bare keys. `USAGE m w b s esc` told a reader the pane has five keys and nothing about what any of them do, or what the pane is. - No pane was ever described. Nothing said usage draws charts or that the pipeline is the plugin chain, so choosing a pane meant visiting it. - No topology. namespaces -> pods -> sessions -> events -> detail is a spine with usage/pipeline/catalog hanging off it; that shape, and the fact that Esc walks the spine back, appeared nowhere. - GLOBAL was a bin: the overlay's own scroll keys, the keys that OPEN other panes, the spend drawer's `a`/`w` (inert unless it is already up) and a caveat with an empty key column, under one heading claiming they all worked everywhere. Three of those groups did not. Restructured into: the active pane (purpose + keys), GO TO ANOTHER PANE, THE DRILL PATH, ANYWHERE, INSIDE THE SPEND DRAWER, then EVERY PANE in full. Ordered by what a lost reader asks first. The jump section is per pane and lists only keys that work there, which matters because the three do not share an allowlist: `u`/`P` open from the session views, `C` from anything past the pickers, `$` follows the drawer's host rule. The old group rendered identically everywhere, so on the namespaces picker it advertised three dead keys and on usage a fourth. jumpsFrom() owns that, and the parity test drives the real handlers for all nine panes rather than restating their switches. `P`, `C` and `u` are no longer advertised twice. They lived in globalKeys AND inside the pane groups, two copies free to disagree; they are now only in jumpTargets, whose labels derive from paneName so a row cannot name a pane something its own title does not. The body also wraps now. syncHelpViewport's comment claimed it re-wrapped on resize while helpBodyLines took no width at all, so the one long line the old overlay had -- the spend-scope caveat, ~100 columns -- lost its second half on an 80-column terminal and said nothing about it. Cost: the body grows from ~33 to ~75 lines, so the scroll affordance is now up at nearly every size. `g`/`G` already worked and the close hint is already pinned; completeness belongs on the one surface with no width or height budget to defend. Signed-off-by: Hai Huang Assisted-By: Claude (Anthropic AI) Signed-off-by: Hai Huang --- authbridge/cmd/abctl/README.md | 51 +- authbridge/cmd/abctl/tui/app.go | 9 +- authbridge/cmd/abctl/tui/help_overlay.go | 490 +++++++++++++++--- authbridge/cmd/abctl/tui/help_overlay_test.go | 26 +- .../cmd/abctl/tui/help_pane_map_test.go | 257 +++++++++ authbridge/cmd/abctl/tui/pipeline_key_test.go | 95 ++-- authbridge/cmd/abctl/tui/spend_drawer.go | 11 +- 7 files changed, 812 insertions(+), 127 deletions(-) create mode 100644 authbridge/cmd/abctl/tui/help_pane_map_test.go diff --git a/authbridge/cmd/abctl/README.md b/authbridge/cmd/abctl/README.md index 72b9ce517..b99475a1d 100644 --- a/authbridge/cmd/abctl/README.md +++ b/authbridge/cmd/abctl/README.md @@ -755,24 +755,51 @@ abctl is for, and the other three are surfaces you visit and leave. Layered on top of all of them: -- **Key help**: a modal overlay listing every keybinding, opened by `?` - from anywhere (picker included). The current pane's bindings come - first and are highlighted; the global keys and a one-line summary of - every other pane follow. While it's up it owns the keyboard — `?`, - `Esc`, or `q` closes it (`q` closes the overlay rather than quitting - abctl). This is the discoverable home for keys the single-line footer - has no room for, `P` among them. Two exceptions: while a pipeline edit - is in flight that overlay is already modal and owns `y`/`N`, and while - the filter input is focused `?` is a character you're typing (session - IDs and hosts can contain one). In both cases `?` is inert until the - keyboard is released. +- **Key help**: a modal overlay opened by `?` from anywhere (picker + included). It is a map of the panes as much as a key list, ordered by + what a lost reader asks first: + + 1. the current pane, highlighted — what it shows, then its own keys; + 2. **GO TO ANOTHER PANE** — the keys that leave, each naming the pane + it opens and what is on it. Rendered per pane and only where the + key actually works: all four (`u`, `P`, `C`, `$`) from the session + views, `C` alone on Usage, and on the two pickers a line saying + they open once you're connected rather than four dead keys; + 3. **THE DRILL PATH** — `namespaces → pods → sessions → events → event + detail` on one line, with where you are in brackets, since that + spine is also what `Esc` walks back; + 4. **ANYWHERE**, then **INSIDE THE SPEND DRAWER** (`a`/`w` are live + only while it is open, so they are not mixed in with the keys that + always work — and the section is omitted on the panes where `$` + is refused); + 5. **EVERY PANE** — the other eight in full, purpose and every + binding's description. Not compacted to bare keys: `USAGE m w b s + esc` said the pane has five keys and nothing about what any of them + do. + + While it's up it owns the keyboard — `?`, `Esc`, or `q` closes it + (`q` closes the overlay rather than quitting abctl). This is the + discoverable home for keys the single-line footer has no room for, + `P` among them. Two exceptions: while a pipeline edit is in flight + that overlay is already modal and owns `y`/`N`, and while the filter + input is focused `?` is a character you're typing (session IDs and + hosts can contain one). In both cases `?` is inert until the keyboard + is released. The body scrolls, so the full reference is reachable on a short terminal: `↑↓`/`jk` by line, `b`/`f` or PgUp/PgDn by page, `u`/`d` by half page, `g`/`G` to the ends. A `[↑↓] scroll %` affordance appears in the overlay's footer only when the content overflows; the close hint stays pinned there at every scroll position. Resizing the - terminal re-ranges the body without losing your place. + terminal re-ranges AND re-wraps the body without losing your place — + prose wraps to the panel width rather than being clipped at the right + edge, so the descriptions and the scope note survive a narrow + terminal. + + Spelling out all nine panes costs roughly three screens at 24 rows, + which `g`/`G` and the pinned close hint are what make affordable. The + overlay is the one surface with no width or height budget to defend, + so it is where completeness belongs. ## Keybindings diff --git a/authbridge/cmd/abctl/tui/app.go b/authbridge/cmd/abctl/tui/app.go index 9b3ac1888..7f6932985 100644 --- a/authbridge/cmd/abctl/tui/app.go +++ b/authbridge/cmd/abctl/tui/app.go @@ -821,8 +821,15 @@ func (m *model) backToPodsPane() { // every resize while it's open, so the body re-wraps and the scroll range // stays correct. resetScroll is true only on open — a resize should keep // the reader where they were. +// +// THE BODY ACTUALLY RE-WRAPS NOW. This comment claimed it did while +// helpBodyLines took no width at all — it built one width-blind string and the +// viewport clipped whatever overran, so the overlay's longest line lost its +// second half on an 80-column terminal and said nothing about it. The wrap +// budget is the terminal minus the frame the panel draws around the viewport. func (m *model) syncHelpViewport(resetScroll bool) { - body := helpBodyLines(m.pane) + frameW := styleBorder.GetHorizontalBorderSize() + helpPadX*2 + body := helpBodyLines(m.pane, m.width-frameW) w, h := helpViewportSize(m.width, m.height, helpBodyWidth(body)) m.helpVp.Width = w m.helpVp.Height = h diff --git a/authbridge/cmd/abctl/tui/help_overlay.go b/authbridge/cmd/abctl/tui/help_overlay.go index e491f1aa0..db8d6b163 100644 --- a/authbridge/cmd/abctl/tui/help_overlay.go +++ b/authbridge/cmd/abctl/tui/help_overlay.go @@ -16,57 +16,212 @@ type keyBinding struct { } // keyGroup is a titled block of bindings in the help overlay. +// +// purpose says what the group IS — for a pane, what it shows. It is the field +// the pane-focused rewrite added, and it exists because the overlay used to name +// nine panes and describe none of them: "what can I look at, and why" was +// unanswerable from the one surface that exists to answer it. +// +// notes carry prose that belongs to the group rather than to any single key. +// They exist so a caveat stops being smuggled into the key column as a binding +// with an empty `keys` field, which is how the spend-scope note used to ship — a +// sentence wearing a keybinding's clothes. type keyGroup struct { title string + purpose string bindings []keyBinding + notes []string } -// globalKeys are the bindings that work in (nearly) every pane. `C` lives -// here because it's the discoverability problem this overlay exists to -// solve: it works from every session-view pane but was never shown in any -// footer. The picker panes are the documented exception — noted in the -// group title rather than duplicated per-pane. +// Section titles. Named constants because the jump section is conditional and +// two others are asserted on, so a literal would drift. +const ( + jumpSectionTitle = "GO TO ANOTHER PANE" + drillSectionTitle = "THE DRILL PATH" + anywhereTitle = "ANYWHERE" + spendDrawerTitle = "INSIDE THE SPEND DRAWER" + everyPaneTitle = "EVERY PANE" + + // thisPaneSuffix marks the active pane's group. Carried in the stored title + // rather than added at render time so paneName can strip it and the two forms + // can never disagree. + thisPaneSuffix = " (this pane)" +) + +// anywhereKeys are the bindings that work on every pane regardless of what is on +// screen. +// +// WHAT IS NO LONGER HERE IS THE POINT. This was `globalKeys`, and it had become a +// bin: the overlay's own scroll keys, the keys that OPEN other panes (`P`, `C`), +// the spend drawer's `a`/`w` — inert unless the drawer is already up — and a +// caveat with an empty key column, all in one flat list under one heading that +// claimed they worked everywhere. Three of those groups did not. // -// `C` WAS `P` UNTIL THE PIPELINE TOOK THAT LETTER. The sentence above is why -// that swap was cheap: a binding no footer ever advertised has close to no -// muscle memory behind it, and "C for Catalog" needs less explaining than a `P` -// borrowed from the word "Plugin". -var globalKeys = keyGroup{ - title: "GLOBAL", +// The pane-opening keys moved to jumpTargets, which renders them per pane and +// only where they work. The drawer's keys moved to spendDrawerKeys, with the +// caveat as a note. +var anywhereKeys = keyGroup{ + title: anywhereTitle, bindings: []keyBinding{ {"?", "this help"}, {"↑↓ / jk", "scroll this help"}, - {"P", "pipeline (session views)"}, - {"C", "plugin catalog (session views)"}, - {"p", "pause/resume stream"}, + {"p", "pause / resume the stream"}, {"g / G", "jump to top / bottom"}, {"b / f", "page up / down"}, - // The strip is global, so its expansion is too — and this overlay advertises itself as - // listing every binding, which is the only reason a reader would find `$` at all. The - // drawer's own hint line only helps someone who already pressed it. - {"$", "expand the spend band into tiers and a breakdown (not on usage)"}, - // The scope, stated where there is always room for it — and THIS IS NOW THE ONLY PLACE - // IT IS SPELLED OUT. The sessions footer used to carry it as a notice, and the pane - // title after that as " · lifetime totals"; both are gone, because a note that has to - // fit in a title could only name a span, and the table has no single span to name (see - // paneView's sessions case). Here there is room for the two facts that actually matter. - // - // "resets on proxy restart" rather than "lifetime", which is the correction that - // motivated dropping the title note: the store is in memory, so a session's figures - // cover only as far back as the current proxy process. A reader comparing the table - // against the band's day figure and finding it smaller is seeing that, not a bug. - {"", "every band cell names its own span; the table is per session, and resets on proxy restart"}, - {"a · w", "while it is open: cycle the axis · the span (the band's four)"}, {"q · ctrl+c", "quit"}, }, } -// paneKeys maps each pane to its own bindings, rendered first (and -// emphasized) when the overlay opens over that pane. Panes absent from -// this map fall back to the global group alone. +// spendDrawerKeys are live only while the spend drawer is open. Their own +// section, because listing `a` and `w` beside the keys that work everywhere +// taught two bindings that do nothing most of the time. +var spendDrawerKeys = keyGroup{ + title: spendDrawerTitle, + purpose: "opened with $ over the spend band; a and w are live only while it is up", + bindings: []keyBinding{ + {"a", "cycle the axis"}, + {"w", "cycle the span (the band's four)"}, + {"$ · esc", "close"}, + }, + // The scope, stated where there is always room for it — and THIS IS STILL THE ONLY PLACE + // IT IS SPELLED OUT. The sessions footer used to carry it as a notice, and the pane title + // after that as " · lifetime totals"; both are gone, because a note that has to fit in a + // title could only name a span, and the table has no single span to name (see paneView's + // sessions case). + // + // "resets on proxy restart" rather than "lifetime", which is the correction that motivated + // dropping the title note: the store is in memory, so a session's figures cover only as far + // back as the current proxy process. A reader comparing the table against the band's day + // figure and finding it smaller is seeing that, not a bug. + // + // A NOTE NOW, not a binding with keys:"". The same sentence, no longer pretending to be a key. + notes: []string{ + "Every band cell names its own span. The sessions table is per session and resets " + + "on proxy restart, so its figures can read smaller than the band's.", + }, +} + +// helpGlobalGroups returns the groups that are not panes and that apply on pane, +// in render order. +// +// A FUNCTION, AND THE ONE helpBodyLines RENDERS FROM. As a plain slice it was a +// registry only the tests read, so a group added to the body and not to the slice +// would have escaped every invariant check — the exact drift those checks exist to +// catch. The drawer's group is conditional for the reason the jump section drops +// `$`: on the pickers and on usage the key is refused, and a titled block +// explaining what `a` and `w` do inside a surface that cannot be opened is three +// keys of pure noise. +func helpGlobalGroups(pane paneID) []keyGroup { + groups := []keyGroup{anywhereKeys} + if ok, _ := spendDrawerHostPane(pane); ok { + groups = append(groups, spendDrawerKeys) + } + return groups +} + +// drillPath is the spine: the panes you reach by drilling in, in order, each a +// step deeper than the last. Rendered as a single line because the SHAPE is the +// information — the old overlay listed all nine panes flat, so nothing said that +// events sits under sessions, or that esc walks back up rather than out. +var drillPath = []paneID{paneNamespaces, panePods, paneSessions, paneEvents, paneDetail} + +// jumpTarget is a key that opens a surface from somewhere else, as opposed to one +// that acts within a pane. +// +// pane is paneNone for `$`: it opens a drawer over the current pane rather than a +// pane, so its availability comes from spendDrawerHostPane instead of from a +// handler changing m.pane, and label names it. For real panes the label is +// derived from paneName, so it cannot drift from the group title. +type jumpTarget struct { + key string + pane paneID + label string + desc string +} + +// name is how the jump row labels the target. +func (jt jumpTarget) name() string { + if jt.pane == paneNone { + return jt.label + } + return strings.ToLower(paneName(jt.pane)) +} + +// jumpTargets are the four keys that leave the pane you are on. +// +// `C` WAS `P` UNTIL THE PIPELINE TOOK THAT LETTER, and this section is the reason +// that swap was cheap: `P`-for-catalog was never shown in any footer, so there +// was close to no muscle memory behind it — and the discoverability problem the +// overlay existed to solve for it is now a titled section rather than one line in +// a group of ten. +var jumpTargets = []jumpTarget{ + {key: "u", pane: paneUsage, + desc: "charts over the events the proxy has seen — every session from the " + + "sessions table, the selected one from events"}, + {key: "P", pane: panePipeline, + desc: "the plugin chain this proxy runs, editable in $EDITOR"}, + {key: "C", pane: paneCatalog, + desc: "every plugin the proxy offers, from /v1/plugins"}, + {key: "$", pane: paneNone, label: "spend", + desc: "a drawer over the spend band: tiers and a breakdown"}, +} + +// jumpsFrom returns the jump targets that actually work from p. +// +// THE KEYS DO NOT SHARE ONE ALLOWLIST, which is why this is a switch and not a +// single membership test: `u` and `P` open only from the session views, `C` opens +// from anything past the pickers, and `$` follows the drawer's own host rule. The +// old overlay papered over that by writing "(session views)" beside two keys and +// "(not on usage)" beside a third, in a group that rendered identically on every +// pane — so on the namespaces picker it advertised three keys that do nothing and +// on usage it advertised a fourth. +// +// Kept honest by TestHelpBody_JumpSectionMatchesTheKeysThatActuallyWork, which +// drives the real handlers for every pane rather than re-stating these sets. +func jumpsFrom(p paneID) []jumpTarget { + var out []jumpTarget + for _, jt := range jumpTargets { + // Never offer a jump to the pane the reader is already standing on. + if jt.pane == p { + continue + } + switch jt.key { + case "u", "P": + switch p { + case paneSessions, paneEvents, paneDetail: + default: + continue + } + case "C": + switch p { + case paneNamespaces, panePods: + continue + } + case "$": + if ok, _ := spendDrawerHostPane(p); !ok { + continue + } + } + out = append(out, jt) + } + return out +} + +// paneKeys maps each pane to its purpose and its own bindings, rendered first +// (and emphasized) when the overlay opens over that pane, and again in full under +// everyPaneTitle for the panes the reader is not on. +// +// EVERY GROUP CARRIES A PURPOSE, enforced by TestHelpPurpose_EveryPaneHasOne. A +// pane the overlay names but does not describe is a pane the reader has to visit +// to find out whether it was the one they wanted. +// +// NO GROUP REPEATS A JUMP KEY. `P` and `u` used to appear here and in globalKeys +// both; they now live only in jumpTargets, which renders directly beneath this +// block and knows which of them work from where. var paneKeys = map[paneID]keyGroup{ paneNamespaces: { - title: "NAMESPACES (this pane)", + title: "NAMESPACES (this pane)", + purpose: "agents grouped by namespace; where abctl starts", bindings: []keyBinding{ {"↑↓ / jk", "navigate"}, {"↵", "open namespace"}, @@ -76,7 +231,8 @@ var paneKeys = map[paneID]keyGroup{ }, }, panePods: { - title: "PODS (this pane)", + title: "PODS (this pane)", + purpose: "the agent pods in that namespace, each with a proxy to connect to", bindings: []keyBinding{ {"↑↓ / jk", "navigate"}, {"↵", "port-forward + connect"}, @@ -86,17 +242,18 @@ var paneKeys = map[paneID]keyGroup{ }, paneSessions: { title: "SESSIONS (this pane)", + purpose: "one row per agent session, with its tokens, cost and context. " + + "Figures are per session and reset when the proxy restarts.", bindings: []keyBinding{ {"↑↓ / jk", "navigate"}, {"↵ / → / l", "drill into session"}, - {"P", "pipeline"}, - {"u", "usage charts (all sessions)"}, {"/", "filter"}, {"esc", "back to pods picker"}, }, }, paneEvents: { - title: "EVENTS (this pane)", + title: "EVENTS (this pane)", + purpose: "the live request stream for one session, newest last", bindings: []keyBinding{ {"↑↓ / jk", "navigate"}, {"↵ / → / l", "event detail"}, @@ -106,21 +263,21 @@ var paneKeys = map[paneID]keyGroup{ {"c then s", "sort by a column: desc → asc → chronological"}, {"o", "load the page before the oldest event shown"}, {"t", "back to the live tail (resumes updates)"}, - {"u", "usage charts (this session)"}, {"esc / ← / h", "back to sessions"}, }, }, paneDetail: { - title: "EVENT DETAIL (this pane)", + title: "EVENT DETAIL (this pane)", + purpose: "one event in full, as the proxy recorded it", bindings: []keyBinding{ {"↑↓", "scroll"}, {"y", "yank event JSON to ~/.cortex/abctl-events"}, - {"u", "usage charts (this session)"}, {"esc / ← / h", "back to events"}, }, }, panePipeline: { - title: "PIPELINE (this pane)", + title: "PIPELINE (this pane)", + purpose: "the plugin chain this proxy runs, in the order it runs them", bindings: []keyBinding{ {"↑↓ / jk", "navigate"}, {"↵ / → / l", "plugin detail"}, @@ -129,14 +286,16 @@ var paneKeys = map[paneID]keyGroup{ }, }, panePluginDetail: { - title: "PLUGIN DETAIL (this pane)", + title: "PLUGIN DETAIL (this pane)", + purpose: "one plugin's resolved config, as the proxy loaded it", bindings: []keyBinding{ {"↑↓", "scroll"}, {"esc / ← / h", "back"}, }, }, paneUsage: { - title: "USAGE (this pane)", + title: "USAGE (this pane)", + purpose: "stacked-bar charts over the events the proxy has seen", bindings: []keyBinding{ {"m", "cycle metric (tokens/requests/errors/latency/cost)"}, {"w", "cycle window (10m/1h/6h)"}, @@ -146,7 +305,8 @@ var paneKeys = map[paneID]keyGroup{ }, }, paneCatalog: { - title: "PLUGIN CATALOG (this pane)", + title: "PLUGIN CATALOG (this pane)", + purpose: "every plugin the proxy offers, whether or not the pipeline runs it", bindings: []keyBinding{ {"↑↓ / jk", "navigate"}, {"↵ / → / l", "plugin detail"}, @@ -167,45 +327,228 @@ var otherPaneOrder = []paneID{ // align into a readable second column across every group. const helpKeyColWidth = 12 -// renderKeyGroup renders one titled group. emphasize bolds the title and -// the key column — used for the pane the overlay was opened over. -func renderKeyGroup(g keyGroup, emphasize bool) string { +// helpMinProseWidth is the narrowest budget helpBodyLines will wrap to. Below it +// wrapping produces a column of single words, which is less readable than letting +// the viewport clip — and a terminal that narrow has already lost the key columns. +const helpMinProseWidth = 40 + +// paneName is a pane's display name without the active-pane marker: "EVENT +// DETAIL", never "EVENT DETAIL (this pane)". One derivation, so the drill path, +// the jump rows and the everyPaneTitle section can never disagree with the title +// the pane's own group renders. +func paneName(p paneID) string { + return strings.TrimSuffix(paneKeys[p].title, thisPaneSuffix) +} + +// wrapWords breaks s into lines of at most width display columns, splitting on +// spaces only. Words longer than width are left over-long rather than broken: the +// only such words here are paths and URLs, and a broken path is unusable while a +// clipped one is still recognizable. +func wrapWords(s string, width int) []string { + if width < 1 { + return []string{s} + } + var ( + lines []string + cur string + ) + for _, word := range strings.Fields(s) { + switch { + case cur == "": + cur = word + case lipgloss.Width(cur)+1+lipgloss.Width(word) <= width: + cur += " " + word + default: + lines = append(lines, cur) + cur = word + } + } + if cur != "" { + lines = append(lines, cur) + } + if len(lines) == 0 { + return []string{""} + } + return lines +} + +// proseBlock renders prefix followed by text, wrapped to width with a hanging +// indent under the prefix, so a pane's purpose reads as one paragraph attached to +// its name rather than as a loose sentence between the keys. +// +// A LONG PREFIX BREAKS INSTEAD OF HANGING. Past a third of the width the hanging +// indent costs more columns than the paragraph has left — "INSIDE THE SPEND +// DRAWER · " is 26 of 76 — so the text drops to its own indented lines. Pane +// titles are all under the threshold and stay inline, which is the common case. +func proseBlock(prefix, text string, width int, prefixStyle lipgloss.Style) string { + indent := lipgloss.Width(prefix) + if indent > width/3 { + lines := wrapWords(text, width-4) + var b strings.Builder + b.WriteString(prefixStyle.Render(strings.TrimSuffix(prefix, " · "))) + for _, ln := range lines { + b.WriteString("\n " + styleHint.Render(ln)) + } + return b.String() + } + + body := width - indent + if body < 1 { + body = 1 + } + lines := wrapWords(text, body) + var b strings.Builder + b.WriteString(prefixStyle.Render(prefix) + styleHint.Render(lines[0])) + for _, ln := range lines[1:] { + b.WriteString("\n" + strings.Repeat(" ", indent) + styleHint.Render(ln)) + } + return b.String() +} + +// renderKeyGroup renders one titled group: its title and purpose as a paragraph, +// then its bindings, then any notes. emphasize bolds the title and the key column +// — used for the pane the overlay was opened over. indent shifts the whole block +// right, which is how the everyPaneTitle section nests nine of them. +// +// Descriptions wrap to the remaining width rather than running off the edge. They +// did run off it: the body was built width-blind and the viewport clipped +// whatever did not fit, so the one long line the old overlay had (the spend-scope +// caveat, ~100 columns) was simply cut in half on an 80-column terminal. +func renderKeyGroup(g keyGroup, emphasize bool, width, indent int) string { titleStyle := styleHint keyStyle := styleMuted if emphasize { titleStyle = styleTitle keyStyle = styleOK } - b.WriteString(titleStyle.Render(g.title)) + pad := strings.Repeat(" ", indent) + + var b strings.Builder + if g.purpose == "" { + b.WriteString(pad + titleStyle.Render(g.title)) + } else { + b.WriteString(proseBlock(pad+g.title+" · ", g.purpose, width, titleStyle)) + } + + // The key column widens to fit the group rather than clipping at + // helpKeyColWidth, because the jump section puts "C plugin catalog" in it — + // the key AND the pane it opens, so the target names line up in a column of + // their own instead of running into the descriptions behind an em dash. + keyCol := helpKeyColWidth + for _, kb := range g.bindings { + if w := lipgloss.Width(kb.keys); w > keyCol { + keyCol = w + } + } + + // A TWO-COLUMN GUTTER, not one. With a single space the group's widest key — + // the one that sets keyCol and so gets no padding — ran straight into its + // description: "C plugin catalog every plugin the proxy offers". + keyIndent := indent + 2 + descCol := keyIndent + keyCol + 2 for _, kb := range g.bindings { - keys := kb.keys - if w := lipgloss.Width(keys); w < helpKeyColWidth { - keys += strings.Repeat(" ", helpKeyColWidth-w) + keys := padRight(kb.keys, keyCol) + b.WriteString("\n" + strings.Repeat(" ", keyIndent) + keyStyle.Render(keys) + " ") + for i, ln := range wrapWords(kb.desc, width-descCol) { + if i > 0 { + b.WriteString("\n" + strings.Repeat(" ", descCol)) + } + b.WriteString(styleHint.Render(ln)) + } + } + for _, note := range g.notes { + b.WriteString("\n") + for i, ln := range wrapWords(note, width-keyIndent) { + if i > 0 { + b.WriteString("\n") + } + b.WriteString(strings.Repeat(" ", keyIndent) + styleHint.Render(ln)) } - b.WriteString("\n " + keyStyle.Render(keys) + " " + styleHint.Render(kb.desc)) } return b.String() } -// helpBodyLines builds the scrollable body of the key-help overlay: the -// active pane's group (emphasized) first, then the global keys, then a -// one-line summary of every other pane. Returned as a single string so a -// viewport can page through it. +// renderJumpSection renders the keys that leave the pane the reader is on, or — +// when none of them work there — one line saying why. +// +// THE "WHY" LINE IS NOT DECORATION. On the two picker panes every jump key is +// inert, and an overlay that simply omitted the section would read as "there is +// nothing else to look at" on the very pane a new reader opens first. +func renderJumpSection(pane paneID, width int) string { + jumps := jumpsFrom(pane) + if len(jumps) == 0 { + return proseBlock(" ", "usage, pipeline and the plugin catalog open once you are "+ + "connected to an agent.", width, styleHint) + } + + // The pane each key opens goes in the KEY column, beside the key, so the four + // target names align. Putting it at the head of the description instead left + // "usage", "pipeline", "plugin catalog" and "spend" starting at four different + // columns, which is the opposite of what a section listing places should do. + g := keyGroup{title: jumpSectionTitle, bindings: make([]keyBinding, 0, len(jumps))} + for _, jt := range jumps { + g.bindings = append(g.bindings, keyBinding{ + keys: jt.key + " " + jt.name(), + desc: jt.desc, + }) + } + return renderKeyGroup(g, false, width, 0) +} + +// renderDrillPath renders the spine as one line, with the pane the reader is on +// marked. The marker is what makes it a map rather than a list: "you are here" +// plus "esc goes left" answers the two questions a lost reader has. +func renderDrillPath(pane paneID, width int) string { + names := make([]string, 0, len(drillPath)) + for _, p := range drillPath { + name := strings.ToLower(paneName(p)) + if p == pane { + name = "[" + name + "]" + } + names = append(names, name) + } + return styleHint.Render(drillSectionTitle) + "\n" + + proseBlock(" ", strings.Join(names, " → "), width, styleHint) +} + +// helpBodyLines builds the scrollable body of the key-help overlay, wrapped to +// width: the active pane in full (emphasized), then where it can go, then the +// spine, then the keys that work anywhere, then the spend drawer's own, then every +// other pane in full. Returned as a single string so a viewport can page it. +// +// ORDERED BY WHAT A LOST READER ASKS FIRST: where am I, where can I go, how do I +// get back, and only then the complete reference. The old body ran active pane → +// GLOBAL → a glyph wall, which answered the last question badly and the first +// three not at all. // -// The close hint is deliberately NOT included — it lives in the overlay's -// fixed footer so it can't be scrolled out of reach. -func helpBodyLines(pane paneID) string { +// The other panes are rendered IN FULL, descriptions and all, rather than +// compacted to their bare keys. `USAGE m w b s esc` told a reader that the +// pane has five keys and nothing about what any of them do, which is the defect +// this rewrite exists to fix. It costs roughly forty lines of scroll; the close +// hint and g/G are the reason that is affordable. +// +// The close hint is deliberately NOT included — it lives in the overlay's fixed +// footer so it can't be scrolled out of reach. +func helpBodyLines(pane paneID, width int) string { + if width < helpMinProseWidth { + width = helpMinProseWidth + } var sections []string if g, ok := paneKeys[pane]; ok { - sections = append(sections, renderKeyGroup(g, true)) + sections = append(sections, renderKeyGroup(g, true, width, 0)) + } + sections = append(sections, + renderJumpSection(pane, width), + renderDrillPath(pane, width), + ) + for _, g := range helpGlobalGroups(pane) { + sections = append(sections, renderKeyGroup(g, false, width, 0)) } - sections = append(sections, renderKeyGroup(globalKeys, false)) - // Remaining panes, compacted to one line each so the overlay stays - // scannable. The active pane is already rendered in full above. - var others []string + // Every other pane, in full. The active pane is already rendered above. + others := []string{styleHint.Render(everyPaneTitle)} for _, p := range otherPaneOrder { if p == pane { continue @@ -214,18 +557,11 @@ func helpBodyLines(pane paneID) string { if !ok { continue } - keys := make([]string, 0, len(g.bindings)) - for _, kb := range g.bindings { - keys = append(keys, kb.keys) - } - // Strip the "(this pane)" suffix the active-pane title carries. - name := strings.TrimSuffix(g.title, " (this pane)") - others = append(others, " "+styleMuted.Render(padRight(name, 16))+" "+ - styleHint.Render(strings.Join(keys, " "))) - } - if len(others) > 0 { - sections = append(sections, - styleHint.Render("OTHER PANES")+"\n"+strings.Join(others, "\n")) + g.title = paneName(p) + others = append(others, renderKeyGroup(g, false, width, 2)) + } + if len(others) > 1 { + sections = append(sections, strings.Join(others, "\n\n")) } return strings.Join(sections, "\n\n") diff --git a/authbridge/cmd/abctl/tui/help_overlay_test.go b/authbridge/cmd/abctl/tui/help_overlay_test.go index ca80a99a4..8069407b2 100644 --- a/authbridge/cmd/abctl/tui/help_overlay_test.go +++ b/authbridge/cmd/abctl/tui/help_overlay_test.go @@ -95,13 +95,27 @@ func TestHelpOverlayRendersAndEmphasizesCurrentPane(t *testing.T) { if !strings.Contains(view, "NAMESPACES (this pane)") { t.Fatalf("overlay should emphasize the active pane's group:\n%s", view) } - // P is the binding this overlay exists to make discoverable. + // The pane's PURPOSE, not just its keys. The overlay named nine panes and + // described none of them before the pane-focused rewrite. + if !strings.Contains(view, paneKeys[paneNamespaces].purpose) { + t.Fatalf("overlay should say what the active pane shows:\n%s", view) + } + // `C` is the binding this overlay exists to make discoverable. On the picker + // it does not work yet, so the overlay names the catalog and says when it opens + // rather than advertising a dead key. if !strings.Contains(view, "plugin catalog") { - t.Fatalf("overlay should document the P / plugin catalog binding:\n%s", view) - } - // Other panes are summarized, not omitted. - if !strings.Contains(view, "OTHER PANES") { - t.Fatalf("overlay should list other panes:\n%s", view) + t.Fatalf("overlay should name the plugin catalog:\n%s", view) + } + if strings.Contains(view, jumpSectionTitle) { + t.Fatalf("no jump key works on the picker; overlay should not offer them:\n%s", view) + } + // Other panes are documented, not omitted. That they are documented IN FULL is + // asserted on the body instead (see + // TestHelpBody_EveryPaneSectionSpellsOutEveryDescription): the section runs + // past the fold on any ordinary terminal, so View() legitimately shows only its + // first rows. + if !strings.Contains(view, everyPaneTitle) { + t.Fatalf("overlay should list every other pane:\n%s", view) } if !strings.Contains(view, "close") { t.Fatalf("overlay should say how to close itself:\n%s", view) diff --git a/authbridge/cmd/abctl/tui/help_pane_map_test.go b/authbridge/cmd/abctl/tui/help_pane_map_test.go new file mode 100644 index 000000000..1f732e954 --- /dev/null +++ b/authbridge/cmd/abctl/tui/help_pane_map_test.go @@ -0,0 +1,257 @@ +package tui + +import ( + "strings" + "testing" + + "github.com/charmbracelet/lipgloss" +) + +// helpWide is a wrap budget wider than any line the overlay builds, so tests +// that assert on content are not reading a truncated body. +const helpWide = 200 + +// allHelpGroups is every group the overlay can render anywhere: the non-pane +// groups unioned over every pane (they are pane-conditional, so asking one pane +// would miss any group that applies only elsewhere) plus each pane's own. +func allHelpGroups() []keyGroup { + var groups []keyGroup + seen := map[string]bool{} + for p := paneNamespaces; p <= lastPaneID; p++ { + for _, g := range helpGlobalGroups(p) { + if !seen[g.title] { + seen[g.title] = true + groups = append(groups, g) + } + } + groups = append(groups, paneKeys[p]) + } + return groups +} + +// nonPaneGroups is allHelpGroups without the panes — the surfaces that used to be +// the single `globalKeys` list. +func nonPaneGroups() []keyGroup { + var groups []keyGroup + seen := map[string]bool{} + for p := paneNamespaces; p <= lastPaneID; p++ { + for _, g := range helpGlobalGroups(p) { + if !seen[g.title] { + seen[g.title] = true + groups = append(groups, g) + } + } + } + return groups +} + +// Every pane must say what it IS, not just which keys it takes. The overlay +// used to name nine panes and describe none of them, which made "what can I +// look at" unanswerable from the one surface that exists to answer it. +func TestHelpPurpose_EveryPaneHasOne(t *testing.T) { + for p := paneNamespaces; p <= lastPaneID; p++ { + g, ok := paneKeys[p] + if !ok { + t.Errorf("pane %v has no paneKeys entry", p) + continue + } + if strings.TrimSpace(g.purpose) == "" { + t.Errorf("pane %v (%q) has no purpose line — the overlay would name it "+ + "without saying what it shows", p, g.title) + } + } +} + +// THE GLYPH WALL IS THE BUG THIS LOCKS OUT. The old "OTHER PANES" section +// compacted each pane to its bare keys — `USAGE m w b s esc` — so the +// overlay told a reader that usage has four keys and nothing about what any of +// them do. Every binding's description must survive into the body. +func TestHelpBody_EveryPaneSectionSpellsOutEveryDescription(t *testing.T) { + body := helpBodyLines(paneSessions, helpWide) + + if !strings.Contains(body, "EVERY PANE") { + t.Fatalf("body has no EVERY PANE section:\n%s", body) + } + for p := paneNamespaces; p <= lastPaneID; p++ { + g := paneKeys[p] + name := paneName(p) + if !strings.Contains(body, name) { + t.Errorf("pane %v (%q) is not named anywhere in the body", p, name) + } + if !strings.Contains(body, g.purpose) { + t.Errorf("pane %v purpose %q missing from the body", p, g.purpose) + } + for _, kb := range g.bindings { + if !strings.Contains(body, kb.desc) { + t.Errorf("pane %v binds %q to %q, and the description is not in the "+ + "body — that is the glyph wall coming back", p, kb.keys, kb.desc) + } + } + } +} + +// The jump section must list exactly the keys that actually change panes from +// where the reader is standing. Driven against the real handlers rather than a +// second copy of their allowlists: `u`, `P` and `C` do NOT share one (P is +// sessions/events/detail, C is anything past the pickers), so a hardcoded list +// here would encode today's accident and go stale silently. +func TestHelpBody_JumpSectionMatchesTheKeysThatActuallyWork(t *testing.T) { + for p := paneNamespaces; p <= lastPaneID; p++ { + listed := map[string]bool{} + for _, jt := range jumpsFrom(p) { + listed[jt.key] = true + } + + for _, jt := range jumpTargets { + var works bool + switch jt.pane { + case paneNone: + // `$` opens no pane — the drawer's own host check is the authority. + works, _ = (&model{pane: p}).spendDrawerHost() + case p: + // Never advertise a jump to the pane the reader is already on. + works = false + default: + m := &model{ + pane: p, + client: deadClient(), + selectedSess: "s1", // `u` needs one on events/detail. + previousPane: paneNone, + pipelineReturnPane: paneNone, + } + m.handleKey(keyRune(rune(jt.key[0]))) + works = m.pane == jt.pane + } + + if works && !listed[jt.key] { + t.Errorf("%q works from pane %v but the jump section does not list it", + jt.key, p) + } + if !works && listed[jt.key] { + t.Errorf("the jump section offers %q from pane %v, where it does nothing", + jt.key, p) + } + } + } +} + +// The pickers run before a connection exists, so none of the jump keys work +// there. Saying nothing would read as "this pane has no way out"; the overlay +// says why instead. +func TestHelpBody_PickerPanesExplainWhyThereIsNoJumpSection(t *testing.T) { + for _, p := range []paneID{paneNamespaces, panePods} { + body := helpBodyLines(p, helpWide) + if strings.Contains(body, jumpSectionTitle) { + t.Errorf("pane %v renders %q, but no jump key works there", + p, jumpSectionTitle) + } + if !strings.Contains(body, "connected") { + t.Errorf("pane %v neither offers the jump keys nor explains why:\n%s", p, body) + } + } +} + +// The spine, in order, on one line. This is the structure the old overlay never +// showed: a reader could not tell that events sits under sessions, or that esc +// walks back up rather than quitting. +func TestHelpBody_DrillPathIsInOrder(t *testing.T) { + body := helpBodyLines(paneSessions, helpWide) + if !strings.Contains(body, "THE DRILL PATH") { + t.Fatalf("body has no drill path section:\n%s", body) + } + + // The spine is the line under the section title, so read that line rather than + // the whole body — an ordering assertion over the body would also be satisfied + // by the EVERY PANE section further down. Found by the title and not by the + // arrow: `↵ / → / l` is a binding on most panes and comes first. + var line string + lines := strings.Split(body, "\n") + for i, ln := range lines { + if strings.Contains(ln, drillSectionTitle) && i+1 < len(lines) { + line = lines[i+1] + break + } + } + if line == "" { + t.Fatalf("no drill path line under %q:\n%s", drillSectionTitle, body) + } + + at := -1 + for _, p := range drillPath { + name := strings.ToLower(paneName(p)) + i := strings.Index(line, name) + if i < 0 { + t.Fatalf("drill path does not name %q: %q", name, line) + } + if i <= at { + t.Errorf("drill path lists %q out of order: %q", name, line) + } + at = i + } + + // "You are here" is what makes the line a map rather than a list. + if want := "[" + strings.ToLower(paneName(paneSessions)) + "]"; !strings.Contains(line, want) { + t.Errorf("drill path does not mark the active pane with %q: %q", want, line) + } +} + +// `P`, `C`, `u` and `$` are advertised once, in the jump section. They used to +// appear in globalKeys AND be repeated inside the pane groups, so the overlay +// carried two copies of each that could disagree. +func TestHelpPaneKeys_DoNotRepeatTheJumpKeys(t *testing.T) { + jump := map[string]bool{} + for _, jt := range jumpTargets { + jump[jt.key] = true + } + for p := paneNamespaces; p <= lastPaneID; p++ { + for _, kb := range paneKeys[p].bindings { + if jump[kb.keys] { + t.Errorf("paneKeys[%v] repeats the jump key %q (%q); the jump "+ + "section is where it belongs", p, kb.keys, kb.desc) + } + } + } +} + +// `a` and `w` are live only while the drawer is open, so listing them beside +// the keys that work everywhere taught a binding that mostly does nothing. +func TestHelpBody_SpendDrawerKeysAreTheirOwnSection(t *testing.T) { + body := helpBodyLines(paneSessions, helpWide) + if !strings.Contains(body, "INSIDE THE SPEND DRAWER") { + t.Fatalf("drawer-only keys have no section of their own:\n%s", body) + } + for _, k := range []string{"a", "w"} { + for _, kb := range anywhereKeys.bindings { + if kb.keys == k { + t.Errorf("anywhereKeys claims %q (%q), which only works while the "+ + "spend drawer is open", k, kb.desc) + } + } + } +} + +// Prose in the key column was how the old overlay smuggled a caveat into a key +// table: a binding with keys:"" and a sentence for a description. Notes now +// belong to a group, so no binding needs an empty key. +func TestHelpBindings_NeverHaveAnEmptyKeyColumn(t *testing.T) { + for _, g := range allHelpGroups() { + for _, kb := range g.bindings { + if strings.TrimSpace(kb.keys) == "" { + t.Errorf("group %q carries a binding with no key: %q", g.title, kb.desc) + } + } + } +} + +// Long prose must wrap to the terminal, not run off it. syncHelpViewport's own +// doc comment claimed the body "re-wraps" on resize while helpBodyLines took no +// width at all, so every purpose line was simply clipped at narrow widths. +func TestHelpBody_WrapsProseToTheGivenWidth(t *testing.T) { + const width = 56 + body := helpBodyLines(paneSessions, width) + for _, ln := range strings.Split(body, "\n") { + if w := lipgloss.Width(ln); w > width { + t.Errorf("line is %d columns, over the %d budget: %q", w, width, ln) + } + } +} diff --git a/authbridge/cmd/abctl/tui/pipeline_key_test.go b/authbridge/cmd/abctl/tui/pipeline_key_test.go index 175308efc..f5cd6f52d 100644 --- a/authbridge/cmd/abctl/tui/pipeline_key_test.go +++ b/authbridge/cmd/abctl/tui/pipeline_key_test.go @@ -299,18 +299,29 @@ func TestPipelineFooter_DropsTabAndNamesWhereEscGoes(t *testing.T) { // both remapped keys must be correct. A stale entry here is worse than no // entry: it sends the reader to the wrong pane. func TestHelpOverlay_DocumentsTheRemappedKeys(t *testing.T) { - desc := map[string]string{} - for _, kb := range globalKeys.bindings { - desc[kb.keys] = strings.ToLower(kb.desc) + // `P` and `C` moved out of the flat global group into jumpTargets, which is + // the section that renders them per pane. Their MEANING is asserted against + // the pane each one names rather than against a description string: the row's + // label is derived from that pane, so naming the pane is naming the key. + target := map[string]paneID{} + for _, jt := range jumpTargets { + target[jt.key] = jt.pane } - if d, ok := desc["P"]; !ok || !strings.Contains(d, "pipeline") { - t.Errorf("globalKeys does not name `P` as the pipeline key (got %q)", d) + if got := target["P"]; got != panePipeline { + t.Errorf("jumpTargets does not point `P` at the pipeline (got pane %v)", got) } - if d, ok := desc["C"]; !ok || !strings.Contains(d, "catalog") { - t.Errorf("globalKeys does not name `C` as the catalog key (got %q)", d) + if got := target["C"]; got != paneCatalog { + t.Errorf("jumpTargets does not point `C` at the plugin catalog (got pane %v)", got) + } + + desc := map[string]string{} + for _, g := range nonPaneGroups() { + for _, kb := range g.bindings { + desc[kb.keys] = strings.ToLower(kb.desc) + } } if d := desc["p"]; !strings.Contains(d, "pause") { - t.Errorf("globalKeys lost `p` as pause (got %q)", d) + t.Errorf("the global groups lost `p` as pause (got %q)", d) } for _, pane := range []paneID{paneSessions, panePipeline} { @@ -325,18 +336,27 @@ func TestHelpOverlay_DocumentsTheRemappedKeys(t *testing.T) { // Two bindings claiming one key is the defect this whole change corrects: `P` // was the catalog's and "pipeline" had no key at all. The overlay is where such // a collision is visible, so assert it cannot come back silently. +// Now spans every non-pane group AND the jump targets, because the single +// globalKeys list this checked has been split into three surfaces — and a key +// claimed by two of THEM is the same defect one step out. The keys:"" exemption +// is gone with the prose that needed it; see +// TestHelpBindings_NeverHaveAnEmptyKeyColumn. func TestGlobalKeys_AdvertiseNoKeyTwice(t *testing.T) { seen := map[string]string{} - for _, kb := range globalKeys.bindings { - // The scope notes carry no key; they are prose in the key column's place. - if kb.keys == "" { - continue + claim := func(key, what string) { + if prev, dup := seen[key]; dup { + t.Errorf("the overlay advertises %q for two different things: %q and %q", + key, prev, what) } - if prev, dup := seen[kb.keys]; dup { - t.Errorf("globalKeys advertises %q for two different things: %q and %q", - kb.keys, prev, kb.desc) + seen[key] = what + } + for _, g := range nonPaneGroups() { + for _, kb := range g.bindings { + claim(kb.keys, kb.desc) } - seen[kb.keys] = kb.desc + } + for _, jt := range jumpTargets { + claim(jt.key, jt.name()) } } @@ -356,27 +376,42 @@ func TestGlobalKeys_AdvertiseNoKeyTwice(t *testing.T) { func TestRemappedKeys_MeanTheSameThingInEveryGroup(t *testing.T) { want := map[string]string{"P": "pipeline", "C": "catalog"} + // jumpTargets is now the single definition, so the check that used to compare + // copies becomes a check that there is exactly one. A pane group is no longer + // free to repeat these keys at all — see TestHelpPaneKeys_DoNotRepeatTheJumpKeys + // — so any occurrence here is a REDEFINITION, which is what sent readers to the + // wrong surface. for _, pane := range otherPaneOrder { for _, kb := range paneKeys[pane].bindings { - substr, watched := want[kb.keys] - if !watched { - continue - } - if !strings.Contains(strings.ToLower(kb.desc), substr) { - t.Errorf("paneKeys[%v] binds %q to %q, which is not the %s — "+ - "the key means one thing and every group must say so", - pane, kb.keys, kb.desc, substr) + if substr, watched := want[kb.keys]; watched { + t.Errorf("paneKeys[%v] binds %q to %q; %q belongs to jumpTargets "+ + "(the %s) and must not be redefined per pane", + pane, kb.keys, kb.desc, kb.keys, substr) } } } - // And the global group itself, which is the definition the pane groups defer to. - for _, kb := range globalKeys.bindings { - if substr, watched := want[kb.keys]; watched { - if !strings.Contains(strings.ToLower(kb.desc), substr) { - t.Errorf("globalKeys binds %q to %q, want it to name the %s", - kb.keys, kb.desc, substr) + // And the definition itself, which the jump rows render from. + for _, jt := range jumpTargets { + if substr, watched := want[jt.key]; watched { + if !strings.Contains(strings.ToLower(jt.name()), substr) { + t.Errorf("jumpTargets labels %q %q, want it to name the %s", + jt.key, jt.name(), substr) } } } + + // The labels are derived from paneName, and the jump rows are the only place a + // reader learns which pane a key opens. Assert the derivation rather than + // trusting it: a label that drifts from the pane's own title would have the + // overlay calling one surface two names. + for _, jt := range jumpTargets { + if jt.pane == paneNone { + continue + } + if wantName := strings.ToLower(paneName(jt.pane)); jt.name() != wantName { + t.Errorf("jumpTargets labels %q %q but its pane is titled %q", + jt.key, jt.name(), wantName) + } + } } diff --git a/authbridge/cmd/abctl/tui/spend_drawer.go b/authbridge/cmd/abctl/tui/spend_drawer.go index 1172dfd99..c2a4f16d9 100644 --- a/authbridge/cmd/abctl/tui/spend_drawer.go +++ b/authbridge/cmd/abctl/tui/spend_drawer.go @@ -218,7 +218,16 @@ func wrapIndex(i, n int) int { // the strip on a terminal this short", which is a height complaint about a pane condition. A // reader resizes, nothing changes, and the key looks broken. func (m *model) spendDrawerHost() (bool, string) { - switch m.pane { + return spendDrawerHostPane(m.pane) +} + +// spendDrawerHostPane is the pane rule on its own, so the [?] overlay can ask it +// without a model. The overlay only lists `$` where `$` works, and it used to +// hardcode "(not on usage)" beside the key instead — a second copy of this switch, +// in prose, on a line that rendered on every pane including the two where the +// drawer is refused for an entirely different reason. +func spendDrawerHostPane(pane paneID) (bool, string) { + switch pane { case paneNamespaces, panePods: // The strip itself does not draw here: these run before a connection exists, so there // is no spend to summarise, let alone break down. From 945ccff3466e86a50a5bc624e04bd78214a4e352 Mon Sep 17 00:00:00 2001 From: Hai Huang Date: Tue, 22 Sep 2026 15:36:10 -0400 Subject: [PATCH 2/4] fix(abctl): wrap the overlay's enum lists, and locate every pane on the map Review on #1100. Four findings, all confirmed by measurement first. wrapWords split on spaces only, and its comment justified that by claiming the only over-long words here were paths and URLs. Wrong: the offenders are the usage pane's own enum lists -- "(tokens/requests/errors/latency/ cost)" at 37 columns and "(none/status/method/plugin/host;" at 31. Under EVERY PANE the description column starts at 18, so both overran anything below 61 columns and the viewport clipped them. Measured 53 overflowing lines across the nine panes at widths 40/41/48; at 50 columns a reader saw "(tokens/requests/erro". That is the defect this PR exists to remove, reintroduced one layer down. Now breaks after `/` when a single word will not fit, joining the pieces with no separator so no invented space appears mid-token. The guard for that checked ONE pane at ONE width -- paneSessions at 56, the combination where the invariant happened to hold. It now runs all nine panes across 40/41/48/50/56/61/80/120, which is zero overflow. renderDrillPath marked only panes on the spine, so usage, pipeline, plugin detail and the catalog rendered a bare list with no "you are here" at all -- the four panes whose reader most needs locating, and the case the doc comment and the README both claimed was covered. They are key-opened surfaces rather than steps, so there is no position to bracket; they now get a line naming that and naming where esc returns them. Guarded for every pane, not one. The no-jump line on the pickers was headerless prose at the same indent and style as everything above it, so it read as a trailing row of the pane block rather than as the answer to "where can I go". It keeps the GO TO ANOTHER PANE heading now, carried as the group's purpose: the section holds its place in the overlay's shape on every pane while still listing no key that does nothing. Signed-off-by: Hai Huang Assisted-By: Claude (Anthropic AI) Signed-off-by: Hai Huang --- authbridge/cmd/abctl/README.md | 11 +- authbridge/cmd/abctl/tui/help_overlay.go | 101 +++++++++++++++--- authbridge/cmd/abctl/tui/help_overlay_test.go | 12 ++- .../cmd/abctl/tui/help_pane_map_test.go | 74 +++++++++++-- 4 files changed, 169 insertions(+), 29 deletions(-) diff --git a/authbridge/cmd/abctl/README.md b/authbridge/cmd/abctl/README.md index b99475a1d..afd0e216f 100644 --- a/authbridge/cmd/abctl/README.md +++ b/authbridge/cmd/abctl/README.md @@ -763,11 +763,14 @@ Layered on top of all of them: 2. **GO TO ANOTHER PANE** — the keys that leave, each naming the pane it opens and what is on it. Rendered per pane and only where the key actually works: all four (`u`, `P`, `C`, `$`) from the session - views, `C` alone on Usage, and on the two pickers a line saying - they open once you're connected rather than four dead keys; + views, `C` alone on Usage, and on the two pickers the heading kept + with a line saying they open once you're connected, rather than four + dead keys; 3. **THE DRILL PATH** — `namespaces → pods → sessions → events → event - detail` on one line, with where you are in brackets, since that - spine is also what `Esc` walks back; + detail` on one line, since that spine is also what `Esc` walks back. + If you're on one of its five panes it's in brackets; the four + key-opened panes are not steps on it, so they get a line naming that + and naming where `Esc` returns them; 4. **ANYWHERE**, then **INSIDE THE SPEND DRAWER** (`a`/`w` are live only while it is open, so they are not mixed in with the keys that always work — and the section is omitted on the panes where `$` diff --git a/authbridge/cmd/abctl/tui/help_overlay.go b/authbridge/cmd/abctl/tui/help_overlay.go index db8d6b163..a9e22cc43 100644 --- a/authbridge/cmd/abctl/tui/help_overlay.go +++ b/authbridge/cmd/abctl/tui/help_overlay.go @@ -340,10 +340,23 @@ func paneName(p paneID) string { return strings.TrimSuffix(paneKeys[p].title, thisPaneSuffix) } -// wrapWords breaks s into lines of at most width display columns, splitting on -// spaces only. Words longer than width are left over-long rather than broken: the -// only such words here are paths and URLs, and a broken path is unusable while a -// clipped one is still recognizable. +// wrapWords breaks s into lines of at most width display columns, on spaces +// first and then — for a single word still too wide — on `/`. +// +// THE SLASH PASS IS NOT FOR PATHS. An earlier version split on spaces only and +// justified it by claiming the only over-long words here were paths and URLs, +// which was wrong: the words that actually overran are the enum lists in the usage +// pane's own bindings — "(tokens/requests/errors/latency/cost)" at 37 columns and +// "(none/status/method/plugin/host;" at 31. Under everyPaneTitle the description +// column starts at 18, so both overran on any terminal below 61 columns and the +// viewport clipped them: 53 lines across the nine panes at widths 40/41/48, and a +// 50-column reader saw "(tokens/requests/erro". That is the exact defect this +// rewrite exists to remove, reintroduced one layer down. +// +// An enum list reads perfectly well broken after a slash, which is why the original +// objection does not apply to the real offenders. A path broken the same way reads +// worse, but only ever when it genuinely does not fit — and a wrapped path still +// shows every character, where a clipped one does not. func wrapWords(s string, width int) []string { if width < 1 { return []string{s} @@ -352,26 +365,65 @@ func wrapWords(s string, width int) []string { lines []string cur string ) + flush := func() { + if cur != "" { + lines = append(lines, cur) + cur = "" + } + } for _, word := range strings.Fields(s) { + if lipgloss.Width(word) > width { + flush() + lines = append(lines, splitOnSlashes(word, width)...) + continue + } switch { case cur == "": cur = word case lipgloss.Width(cur)+1+lipgloss.Width(word) <= width: cur += " " + word default: - lines = append(lines, cur) + flush() cur = word } } - if cur != "" { - lines = append(lines, cur) - } + flush() if len(lines) == 0 { return []string{""} } return lines } +// splitOnSlashes packs word into lines of at most width columns, breaking only +// after `/` and joining the pieces with NO separator — the fragments are parts of +// one token, so the space wrapWords puts between words would invent one the reader +// would take for real ("(tokens/ requests/"). +// +// A piece with no slash left to break on comes back over-long rather than cut +// mid-character. Nothing in the overlay hits that today; if something does the line +// is too wide instead of silently truncated, which the wrap test catches. +func splitOnSlashes(word string, width int) []string { + var ( + out []string + cur string + ) + for _, piece := range strings.SplitAfter(word, "/") { + switch { + case cur == "": + cur = piece + case lipgloss.Width(cur)+lipgloss.Width(piece) <= width: + cur += piece + default: + out = append(out, cur) + cur = piece + } + } + if cur != "" { + out = append(out, cur) + } + return out +} + // proseBlock renders prefix followed by text, wrapped to width with a hanging // indent under the prefix, so a pane's purpose reads as one paragraph attached to // its name rather than as a loose sentence between the keys. @@ -470,16 +522,25 @@ func renderKeyGroup(g keyGroup, emphasize bool, width, indent int) string { } // renderJumpSection renders the keys that leave the pane the reader is on, or — -// when none of them work there — one line saying why. +// when none of them work there — the section's title over one line saying why. // // THE "WHY" LINE IS NOT DECORATION. On the two picker panes every jump key is // inert, and an overlay that simply omitted the section would read as "there is // nothing else to look at" on the very pane a new reader opens first. +// +// IT KEEPS THE TITLE, though, which it did not at first: the bare sentence sat at +// the same 2-column indent and muted style as everything else and read as a +// trailing row of the pane block above it rather than as the answer to "where can I +// go". Carried as the group's purpose, so the section holds its place in the +// overlay's shape on every pane while listing no key that does nothing. func renderJumpSection(pane paneID, width int) string { jumps := jumpsFrom(pane) if len(jumps) == 0 { - return proseBlock(" ", "usage, pipeline and the plugin catalog open once you are "+ - "connected to an agent.", width, styleHint) + return renderKeyGroup(keyGroup{ + title: jumpSectionTitle, + purpose: "usage, pipeline and the plugin catalog open once you are connected " + + "to an agent.", + }, false, width, 0) } // The pane each key opens goes in the KEY column, beside the key, so the four @@ -499,17 +560,33 @@ func renderJumpSection(pane paneID, width int) string { // renderDrillPath renders the spine as one line, with the pane the reader is on // marked. The marker is what makes it a map rather than a list: "you are here" // plus "esc goes left" answers the two questions a lost reader has. +// +// FOUR PANES ARE NOT ON THE SPINE — usage, pipeline, plugin detail and the catalog +// — and they used to get the bare line with no marker at all, which is the one case +// where a reader most needs telling where they are. They are key-opened surfaces +// rather than steps, so there is no position to bracket; they get a sentence naming +// that and naming where esc returns them, which is the fact the missing marker was +// standing in for. func renderDrillPath(pane paneID, width int) string { names := make([]string, 0, len(drillPath)) + onSpine := false for _, p := range drillPath { name := strings.ToLower(paneName(p)) if p == pane { name = "[" + name + "]" + onSpine = true } names = append(names, name) } - return styleHint.Render(drillSectionTitle) + "\n" + + + out := styleHint.Render(drillSectionTitle) + "\n" + proseBlock(" ", strings.Join(names, " → "), width, styleHint) + if !onSpine { + out += "\n" + proseBlock(" ", "you are on "+strings.ToLower(paneName(pane))+ + ", which sits off this path — esc returns you to the pane that opened it.", + width, styleHint) + } + return out } // helpBodyLines builds the scrollable body of the key-help overlay, wrapped to diff --git a/authbridge/cmd/abctl/tui/help_overlay_test.go b/authbridge/cmd/abctl/tui/help_overlay_test.go index 8069407b2..21407d627 100644 --- a/authbridge/cmd/abctl/tui/help_overlay_test.go +++ b/authbridge/cmd/abctl/tui/help_overlay_test.go @@ -100,14 +100,16 @@ func TestHelpOverlayRendersAndEmphasizesCurrentPane(t *testing.T) { if !strings.Contains(view, paneKeys[paneNamespaces].purpose) { t.Fatalf("overlay should say what the active pane shows:\n%s", view) } - // `C` is the binding this overlay exists to make discoverable. On the picker - // it does not work yet, so the overlay names the catalog and says when it opens - // rather than advertising a dead key. + // `C` is the binding this overlay exists to make discoverable. On the picker it + // does not work yet, so the section keeps its heading and names the catalog in + // prose rather than advertising a dead key — see + // TestHelpBody_PickerPanesExplainWhyThereIsNoJumpSection for both halves. if !strings.Contains(view, "plugin catalog") { t.Fatalf("overlay should name the plugin catalog:\n%s", view) } - if strings.Contains(view, jumpSectionTitle) { - t.Fatalf("no jump key works on the picker; overlay should not offer them:\n%s", view) + if !strings.Contains(view, jumpSectionTitle) { + t.Fatalf("overlay should keep the %q heading on the picker:\n%s", + jumpSectionTitle, view) } // Other panes are documented, not omitted. That they are documented IN FULL is // asserted on the body instead (see diff --git a/authbridge/cmd/abctl/tui/help_pane_map_test.go b/authbridge/cmd/abctl/tui/help_pane_map_test.go index 1f732e954..e2ed6f93b 100644 --- a/authbridge/cmd/abctl/tui/help_pane_map_test.go +++ b/authbridge/cmd/abctl/tui/help_pane_map_test.go @@ -137,17 +137,30 @@ func TestHelpBody_JumpSectionMatchesTheKeysThatActuallyWork(t *testing.T) { // The pickers run before a connection exists, so none of the jump keys work // there. Saying nothing would read as "this pane has no way out"; the overlay -// says why instead. +// keeps the section and explains instead. +// +// THE TITLE MUST STAY AND THE KEYS MUST GO — asserted as two separate things, +// because the section went through both failure modes. Omitting the title left the +// explanation reading as a trailing row of the pane block above it; listing the +// keys advertised four that do nothing. func TestHelpBody_PickerPanesExplainWhyThereIsNoJumpSection(t *testing.T) { for _, p := range []paneID{paneNamespaces, panePods} { body := helpBodyLines(p, helpWide) - if strings.Contains(body, jumpSectionTitle) { - t.Errorf("pane %v renders %q, but no jump key works there", + if !strings.Contains(body, jumpSectionTitle) { + t.Errorf("pane %v drops the %q heading, so its explanation has no owner", p, jumpSectionTitle) } if !strings.Contains(body, "connected") { t.Errorf("pane %v neither offers the jump keys nor explains why:\n%s", p, body) } + // No key rows under it: the section names the panes in prose only. + head := body[strings.Index(body, jumpSectionTitle):] + for _, jt := range jumpTargets { + row := jt.key + " " + jt.name() + if strings.Contains(head, row) { + t.Errorf("pane %v lists the jump row %q, where the key does nothing", p, row) + } + } } } @@ -195,6 +208,42 @@ func TestHelpBody_DrillPathIsInOrder(t *testing.T) { } } +// EVERY pane must be located, not just the five on the spine. Asserting the marker +// on paneSessions alone left four panes — usage, pipeline, plugin detail and the +// catalog — rendering a bare list with nothing saying the reader was off the path +// or where esc would return them. +func TestHelpBody_DrillPathLocatesEveryPane(t *testing.T) { + spine := map[paneID]bool{} + for _, p := range drillPath { + spine[p] = true + } + + for p := paneNamespaces; p <= lastPaneID; p++ { + body := helpBodyLines(p, helpWide) + section := body[strings.Index(body, drillSectionTitle):] + if end := strings.Index(section, "\n\n"); end > 0 { + section = section[:end] + } + + if spine[p] { + if want := "[" + strings.ToLower(paneName(p)) + "]"; !strings.Contains(section, want) { + t.Errorf("pane %v is on the spine but unmarked (want %q):\n%s", p, want, section) + } + continue + } + // Off the spine: no position to bracket, so it must be named in prose + // along with where esc goes. + if !strings.Contains(section, strings.ToLower(paneName(p))) { + t.Errorf("pane %v is off the spine and the drill path never names it:\n%s", + p, section) + } + if !strings.Contains(section, "esc") { + t.Errorf("pane %v is off the spine and nothing says where esc returns it:\n%s", + p, section) + } + } +} + // `P`, `C`, `u` and `$` are advertised once, in the jump section. They used to // appear in globalKeys AND be repeated inside the pane groups, so the overlay // carried two copies of each that could disagree. @@ -246,12 +295,21 @@ func TestHelpBindings_NeverHaveAnEmptyKeyColumn(t *testing.T) { // Long prose must wrap to the terminal, not run off it. syncHelpViewport's own // doc comment claimed the body "re-wraps" on resize while helpBodyLines took no // width at all, so every purpose line was simply clipped at narrow widths. +// EVERY PANE AT EVERY INTERESTING WIDTH, because one pane at one width is the +// combination where this happened to hold: the first version checked only +// paneSessions at 56 and passed while the usage pane's enum-list bindings overran +// by up to 15 columns on anything below 61. The widths bracket the wrap floor +// (helpMinProseWidth), the point where the longest enum stops fitting, and a +// roomy terminal. func TestHelpBody_WrapsProseToTheGivenWidth(t *testing.T) { - const width = 56 - body := helpBodyLines(paneSessions, width) - for _, ln := range strings.Split(body, "\n") { - if w := lipgloss.Width(ln); w > width { - t.Errorf("line is %d columns, over the %d budget: %q", w, width, ln) + for _, width := range []int{40, 41, 48, 50, 56, 61, 80, 120} { + for p := paneNamespaces; p <= lastPaneID; p++ { + for _, ln := range strings.Split(helpBodyLines(p, width), "\n") { + if w := lipgloss.Width(ln); w > width { + t.Errorf("pane %v at width %d: line is %d columns, over budget: %q", + p, width, w, ln) + } + } } } } From d558543b9f538544ce3e7f5191cc8b7178253694 Mon Sep 17 00:00:00 2001 From: Hai Huang Date: Tue, 22 Sep 2026 16:01:58 -0400 Subject: [PATCH 3/4] fix(abctl): stop ANYWHERE claiming a key the usage pane rebinds Second round of review on #1100. Seven findings, each confirmed before being changed. ANYWHERE listed "b / f page up / down" on every pane, and paneUsage binds `b` to the breakdown cycle: its handler runs first and returns, and pageActivePane has no case for that pane, so neither half of the row was true there. `a` and `w` were moved out of this group for exactly that reason and `b` was not. The row is now dropped where paging does not exist, and two tests guard it -- one comparing ANYWHERE against the active pane's own keys (splitting "b / f" into b and f, since comparing key COLUMNS is why nothing noticed), one pressing `b` on usage so the exclusion stays tied to behaviour rather than to a switch nobody re-reads. overlayOnlyKeys names the documented exception: `?` and the scroll keys describe the overlay, not the pane, so both meanings are true at once. That check also flagged paneNamespaces restating `q` from ANYWHERE. Its row is now `esc` alone, keeping the pane-specific fact (esc quits, because nothing is above it) without a second copy of quit. TestHelpOverlayScrollHint had been SKIPPING since the first commit here: the body is 86 lines at 100 columns and the test asked for 60 rows, so the guard fired and took the three short-terminal assertions with it while reporting PASS. Split into two tests; the tall one asserts the height floor (89) instead of skipping, and says what to raise it to. Four test-quality findings, all correct: - The jump-label check compared jt.name() against the expression that IS jt.name()'s body, so it could not fail. Pinned to literals, and jumpTarget.label -- ignored for pane targets -- must now be empty so a wrong one cannot hide from the compiler or the reader. - The `$` arm of the jump parity test called spendDrawerHost(), the same function jumpsFrom consults, so it compared the implementation with itself. It now presses `$` on a terminal tall enough for the drawer and checks spend.expanded. - TestHelpOverlayScrollKeys' up-scroll branch was `if ... { return }` with no assertion, so scrolling back up was untested while looking covered. - TestUsageFooterMatchesHandledKeys hand-copied {m,w,b,s} "per the paneUsage switch" and never pressed them, leaving the footer-vs-handler claim in its own name unenforced. Each key is now driven and checked for a state change. Two unchecked strings.Index calls followed non-fatal t.Errorf, so a body missing a section title would index at -1 and panic, aborting the package instead of failing one case. Both go through a helper that t.Fatalf's. Signed-off-by: Hai Huang Assisted-By: Claude (Anthropic AI) Signed-off-by: Hai Huang --- authbridge/cmd/abctl/tui/help_overlay.go | 51 +++++++- authbridge/cmd/abctl/tui/help_overlay_test.go | 81 +++++++++--- .../cmd/abctl/tui/help_pane_map_test.go | 115 ++++++++++++++++-- authbridge/cmd/abctl/tui/pipeline_key_test.go | 33 +++-- 4 files changed, 244 insertions(+), 36 deletions(-) diff --git a/authbridge/cmd/abctl/tui/help_overlay.go b/authbridge/cmd/abctl/tui/help_overlay.go index a9e22cc43..6e4507b57 100644 --- a/authbridge/cmd/abctl/tui/help_overlay.go +++ b/authbridge/cmd/abctl/tui/help_overlay.go @@ -67,11 +67,52 @@ var anywhereKeys = keyGroup{ {"↑↓ / jk", "scroll this help"}, {"p", "pause / resume the stream"}, {"g / G", "jump to top / bottom"}, - {"b / f", "page up / down"}, + {pagingKeys, "page up / down"}, {"q · ctrl+c", "quit"}, }, } +// pagingKeys is the one row of anywhereKeys that is not available everywhere, so +// it is named rather than written twice (here and in the filter that drops it). +const pagingKeys = "b / f" + +// overlayOnlyKeys are the anywhereKeys rows whose description is about THIS +// OVERLAY rather than the pane underneath it. They are the documented exception to +// "ANYWHERE must not claim a key the active pane rebinds": `↑↓`/`jk` navigate a +// pane and scroll the overlay, and both are true at once because the overlay is +// modal. Every other row describes the pane, so a pane rebinding it is a conflict — +// see TestHelpBody_AnywhereKeysAreNotReboundByTheActivePane. +var overlayOnlyKeys = map[string]bool{"?": true, "↑↓ / jk": true} + +// panePages reports whether `b`/`f` page the pane's own content. +// +// paneUsage IS THE ONE THAT DOES NOT, and it is worse than merely inert there: its +// key handler runs first and binds `b` to the breakdown cycle, so an overlay +// listing "b / f page up / down" on that pane names a key that does something +// else entirely. pageActivePane has cases for events, sessions, pipeline, catalog +// and the two detail viewports; the pickers return earlier and page through their +// own table's binding. Usage is absent from both paths. +func panePages(p paneID) bool { + return p != paneUsage +} + +// anywhereKeysFor returns anywhereKeys as it applies on pane, dropping the paging +// row where paging does not exist. +func anywhereKeysFor(pane paneID) keyGroup { + if panePages(pane) { + return anywhereKeys + } + g := anywhereKeys + g.bindings = make([]keyBinding, 0, len(anywhereKeys.bindings)) + for _, kb := range anywhereKeys.bindings { + if kb.keys == pagingKeys { + continue + } + g.bindings = append(g.bindings, kb) + } + return g +} + // spendDrawerKeys are live only while the spend drawer is open. Their own // section, because listing `a` and `w` beside the keys that work everywhere // taught two bindings that do nothing most of the time. @@ -112,7 +153,7 @@ var spendDrawerKeys = keyGroup{ // explaining what `a` and `w` do inside a surface that cannot be opened is three // keys of pure noise. func helpGlobalGroups(pane paneID) []keyGroup { - groups := []keyGroup{anywhereKeys} + groups := []keyGroup{anywhereKeysFor(pane)} if ok, _ := spendDrawerHostPane(pane); ok { groups = append(groups, spendDrawerKeys) } @@ -227,7 +268,11 @@ var paneKeys = map[paneID]keyGroup{ {"↵", "open namespace"}, {"l", "connect to the local session API"}, {"r", "reload agent list"}, - {"q · esc", "quit"}, + // `esc` ALONE, not "q · esc": quit is already in ANYWHERE, and `q` here made + // this the one pane group that restated a key from it. What is pane-specific + // is that esc quits — everywhere else it goes back, and there is nowhere + // above this. + {"esc", "quit — nothing is above this pane"}, }, }, panePods: { diff --git a/authbridge/cmd/abctl/tui/help_overlay_test.go b/authbridge/cmd/abctl/tui/help_overlay_test.go index 21407d627..bdf76819f 100644 --- a/authbridge/cmd/abctl/tui/help_overlay_test.go +++ b/authbridge/cmd/abctl/tui/help_overlay_test.go @@ -325,8 +325,35 @@ func TestFooterHintsMentionUsageKey(t *testing.T) { func TestUsageFooterMatchesHandledKeys(t *testing.T) { m := &model{pane: paneUsage, selectedSess: "s1"} - // Keys the pane handles, per the paneUsage switch in handleKey. - handled := []string{"m", "w", "b", "s"} + // Keys the pane handles, DRIVEN rather than copied. The list used to be + // hand-written "per the paneUsage switch in handleKey" and never pressed, so the + // footer-versus-handler claim in this test's name rested on the two lists having + // been kept in sync by hand — which is the drift the test exists to catch. + // + // Each key is pressed on a fresh model and counted as handled if it changed the + // pane's state. `m`/`b` cycle enums, `w` refetches with a new window, `s` toggles + // scope; a key the switch stopped handling changes nothing and is reported. + handled := []string{} + for _, probe := range []struct { + key rune + changed func(before, after *usageState) bool + }{ + {'m', func(b, a *usageState) bool { return a.metric != b.metric }}, + {'w', func(b, a *usageState) bool { return a.windowIdx != b.windowIdx }}, + {'b', func(b, a *usageState) bool { return a.group != b.group }}, + {'s', func(b, a *usageState) bool { return a.session != b.session }}, + } { + mm := &model{pane: paneUsage, selectedSess: "s1"} + before := mm.usage + mm.handleKey(keyRune(probe.key)) + if probe.changed(&before, &mm.usage) { + handled = append(handled, string(probe.key)) + } else { + t.Errorf("`%c` changed nothing on the usage pane — the footer advertises it", + probe.key) + } + } + footer := m.helpView() for _, k := range handled { if !strings.Contains(footer, "["+k+"]") { @@ -488,34 +515,54 @@ func TestHelpOverlayScrollKeys(t *testing.T) { if mm.helpVp.YOffset <= before { t.Fatalf("%s should scroll down: offset %d → %d", tc.name, before, mm.helpVp.YOffset) } - // And back up with the mirror key where one exists. - up := tea.KeyMsg{Type: tea.KeyUp} - u, _ = mm.Update(up) + // And back up. THIS USED TO ASSERT NOTHING: the branch it had was an + // `if … { return }` that passed on every path, so up-scrolling was + // untested while looking covered. + down := mm.helpVp.YOffset + u, _ = mm.Update(tea.KeyMsg{Type: tea.KeyUp}) mm = u.(*model) - if mm.helpVp.AtBottom() && mm.helpVp.YOffset != 0 { - // fine: single-line step from a clamped bottom - return + if mm.helpVp.YOffset >= down && !mm.helpVp.AtTop() { + t.Errorf("up arrow should scroll back: offset %d → %d (AtTop=%v)", + down, mm.helpVp.YOffset, mm.helpVp.AtTop()) } }) } } -// The scroll affordance appears only when it's needed, and reports -// position so the reader knows there's more below. -func TestHelpOverlayScrollHint(t *testing.T) { - // Tall terminal: whole reference fits, no scroll noise. - tall := helpModelAt(t, paneSessions, 100, 60) - if tall.helpVp.TotalLineCount() > tall.helpVp.VisibleLineCount() { - t.Skip("terminal not tall enough for the no-scroll case") +// helpNoScrollHeight is a terminal tall enough to show the whole reference at +// helpWideTerminal columns, so the no-affordance case is testable. +// +// IT IS BIG, AND THAT IS THE POINT. The body is 86 lines once every pane carries +// its purpose and its descriptions, and 89 rows is the exact floor. The previous +// version of this test asked for 60 and t.Skip()ed when the content did not fit — +// which, the moment the body grew, silently took the three short-terminal +// assertions below with it and reported PASS. A number that has to track the body's +// height is asserted, never skipped. +const ( + helpWideTerminal = 100 + helpNoScrollHeight = 89 +) + +// With everything visible there must be no scroll affordance — it would be noise +// pointing at content that is already on screen. +func TestHelpOverlayScrollHint_AbsentWhenEverythingFits(t *testing.T) { + tall := helpModelAt(t, paneSessions, helpWideTerminal, helpNoScrollHeight) + if got, vis := tall.helpVp.TotalLineCount(), tall.helpVp.VisibleLineCount(); got > vis { + t.Fatalf("%d×%d shows %d of %d body lines — raise helpNoScrollHeight to at "+ + "least %d rather than skipping this case", + helpWideTerminal, helpNoScrollHeight, vis, got, got+helpOverlayFrameH) } // Match the affordance specifically, not the word "scroll" — the // global key list legitimately contains "scroll this help". if strings.Contains(tall.View(), "[↑↓] scroll") { t.Errorf("no scroll affordance expected when everything fits:\n%s", tall.View()) } +} - // Short terminal: hint present, with a percentage. - short := helpModelAt(t, paneSessions, 100, 14) +// When it does overflow the affordance appears and reports position, so the reader +// knows there is more below and how much of it they have seen. +func TestHelpOverlayScrollHint_ReportsPosition(t *testing.T) { + short := helpModelAt(t, paneSessions, helpWideTerminal, 14) v := short.View() if !strings.Contains(v, "[↑↓] scroll") { t.Errorf("scroll affordance expected when content overflows:\n%s", v) diff --git a/authbridge/cmd/abctl/tui/help_pane_map_test.go b/authbridge/cmd/abctl/tui/help_pane_map_test.go index e2ed6f93b..5e2a7ec15 100644 --- a/authbridge/cmd/abctl/tui/help_pane_map_test.go +++ b/authbridge/cmd/abctl/tui/help_pane_map_test.go @@ -11,6 +11,28 @@ import ( // that assert on content are not reading a truncated body. const helpWide = 200 +// sectionFrom returns body from title onwards, stopping at the blank line that ends +// the section when only is true. +// +// t.Fatalf ON A MISSING TITLE, which is the reason this is a helper: the callers +// used body[strings.Index(body, title):] after a non-fatal t.Errorf, so a body that +// lost a heading indexed at -1 and panicked — aborting the whole package instead of +// failing the one case with a readable message. +func sectionFrom(t *testing.T, body, title string, only bool) string { + t.Helper() + i := strings.Index(body, title) + if i < 0 { + t.Fatalf("body has no %q section:\n%s", title, body) + } + section := body[i:] + if only { + if end := strings.Index(section, "\n\n"); end > 0 { + section = section[:end] + } + } + return section +} + // allHelpGroups is every group the overlay can render anywhere: the non-pane // groups unioned over every pane (they are pane-conditional, so asking one pane // would miss any group that applies only elsewhere) plus each pane's own. @@ -106,8 +128,16 @@ func TestHelpBody_JumpSectionMatchesTheKeysThatActuallyWork(t *testing.T) { var works bool switch jt.pane { case paneNone: - // `$` opens no pane — the drawer's own host check is the authority. - works, _ = (&model{pane: p}).spendDrawerHost() + // `$` opens a drawer, not a pane, so "works" is the drawer actually + // opening. PRESSED, NOT ASKED: this used to call spendDrawerHost(), + // which is the very function jumpsFrom consults, so the expected value + // and the implementation came from one source and the check compared + // it with itself. A terminal tall enough for the drawer, so a height + // refusal cannot read as a pane refusal. + m := &model{pane: p, width: 120, height: 48, client: deadClient()} + m.layout() + m.handleKey(keyRune('$')) + works = m.spend.expanded case p: // Never advertise a jump to the pane the reader is already on. works = false @@ -154,7 +184,7 @@ func TestHelpBody_PickerPanesExplainWhyThereIsNoJumpSection(t *testing.T) { t.Errorf("pane %v neither offers the jump keys nor explains why:\n%s", p, body) } // No key rows under it: the section names the panes in prose only. - head := body[strings.Index(body, jumpSectionTitle):] + head := sectionFrom(t, body, jumpSectionTitle, false) for _, jt := range jumpTargets { row := jt.key + " " + jt.name() if strings.Contains(head, row) { @@ -220,10 +250,7 @@ func TestHelpBody_DrillPathLocatesEveryPane(t *testing.T) { for p := paneNamespaces; p <= lastPaneID; p++ { body := helpBodyLines(p, helpWide) - section := body[strings.Index(body, drillSectionTitle):] - if end := strings.Index(section, "\n\n"); end > 0 { - section = section[:end] - } + section := sectionFrom(t, body, drillSectionTitle, true) if spine[p] { if want := "[" + strings.ToLower(paneName(p)) + "]"; !strings.Contains(section, want) { @@ -262,6 +289,80 @@ func TestHelpPaneKeys_DoNotRepeatTheJumpKeys(t *testing.T) { } } +// splitKeys breaks a key column into the individual keys it advertises: "b / f" +// into b and f, "q · ctrl+c" into q and ctrl+c. Without this a comparison of key +// COLUMNS misses every real collision, which is how "b / f page up / down" sat in +// ANYWHERE on the one pane where `b` cycles the breakdown — the strings "b" and +// "b / f" are not equal, so nothing noticed. +func splitKeys(col string) []string { + var out []string + for _, part := range strings.FieldsFunc(col, func(r rune) bool { + return r == '/' || r == '·' + }) { + if k := strings.TrimSpace(part); k != "" { + out = append(out, k) + } + } + return out +} + +// ANYWHERE may not claim a key the pane underneath rebinds to something else. +// +// This is the check whose absence let `b` ship wrong: anywhereKeys' own doc says it +// holds only the keys that work regardless of what is on screen, and `a`/`w` were +// moved out for exactly that reason while `b` was not. The overlayOnlyKeys rows are +// the documented exception — they describe the overlay, not the pane, so both +// meanings are true at once. +func TestHelpBody_AnywhereKeysAreNotReboundByTheActivePane(t *testing.T) { + for p := paneNamespaces; p <= lastPaneID; p++ { + owned := map[string]string{} + for _, kb := range paneKeys[p].bindings { + for _, k := range splitKeys(kb.keys) { + owned[k] = kb.desc + } + } + for _, kb := range anywhereKeysFor(p).bindings { + if overlayOnlyKeys[kb.keys] { + continue + } + for _, k := range splitKeys(kb.keys) { + if desc, clash := owned[k]; clash { + t.Errorf("pane %v: ANYWHERE offers %q as %q while the pane binds %q to %q", + p, k, kb.desc, k, desc) + } + } + } + } +} + +// And the behavioural half: the pane that does not page must not be offered the +// paging row, driven off pageActivePane's real cases rather than a copy of them. +func TestHelpBody_PagingRowOnlyWherePagingExists(t *testing.T) { + for p := paneNamespaces; p <= lastPaneID; p++ { + listed := false + for _, kb := range anywhereKeysFor(p).bindings { + if kb.keys == pagingKeys { + listed = true + } + } + if listed != panePages(p) { + t.Errorf("pane %v: paging row listed=%v but panePages=%v", + p, listed, panePages(p)) + } + } + + // paneUsage is the exclusion, and it is excluded because `b` means something + // else there. Press it and confirm that is still true, so this stops being a + // claim about a switch statement nobody re-reads. + m := &model{pane: paneUsage, selectedSess: "s1"} + before := m.usage.group + m.handleKey(keyRune('b')) + if m.usage.group == before { + t.Errorf("`b` no longer cycles the usage breakdown; if paging reached that pane, " + + "panePages should stop excluding it") + } +} + // `a` and `w` are live only while the drawer is open, so listing them beside // the keys that work everywhere taught a binding that mostly does nothing. func TestHelpBody_SpendDrawerKeysAreTheirOwnSection(t *testing.T) { diff --git a/authbridge/cmd/abctl/tui/pipeline_key_test.go b/authbridge/cmd/abctl/tui/pipeline_key_test.go index f5cd6f52d..a4d6e5451 100644 --- a/authbridge/cmd/abctl/tui/pipeline_key_test.go +++ b/authbridge/cmd/abctl/tui/pipeline_key_test.go @@ -401,17 +401,32 @@ func TestRemappedKeys_MeanTheSameThingInEveryGroup(t *testing.T) { } } - // The labels are derived from paneName, and the jump rows are the only place a - // reader learns which pane a key opens. Assert the derivation rather than - // trusting it: a label that drifts from the pane's own title would have the - // overlay calling one surface two names. + // The jump rows are the only place a reader learns which pane a key opens, so + // the label each row shows is pinned to a LITERAL. + // + // It used to be compared against strings.ToLower(paneName(jt.pane)) — which is + // the body of jt.name() itself, so the assertion could not fail for any pane + // target and passed vacuously. Literals catch what a reader would actually + // notice: a pane retitled without its jump row being reconsidered. + wantLabel := map[string]string{ + "u": "usage", + "P": "pipeline", + "C": "plugin catalog", + "$": "spend", + } for _, jt := range jumpTargets { - if jt.pane == paneNone { - continue + if want, ok := wantLabel[jt.key]; !ok { + t.Errorf("jumpTargets has an undocumented key %q — add it here", jt.key) + } else if jt.name() != want { + t.Errorf("the %q jump row reads %q, want %q", jt.key, jt.name(), want) } - if wantName := strings.ToLower(paneName(jt.pane)); jt.name() != wantName { - t.Errorf("jumpTargets labels %q %q but its pane is titled %q", - jt.key, jt.name(), wantName) + // label is consulted only for paneNone, so a value set on a pane target is + // dead weight the compiler cannot see. Keep it empty so it cannot disagree + // with the name actually rendered. + if jt.pane != paneNone && jt.label != "" { + t.Errorf("jumpTargets sets label %q on %q, whose pane is %v — the label is "+ + "ignored for pane targets, so it can only mislead", + jt.label, jt.key, jt.pane) } } } From 3718009bfbb0411a3f70587583f1857c45da6af6 Mon Sep 17 00:00:00 2001 From: Hai Huang Date: Tue, 22 Sep 2026 16:21:32 -0400 Subject: [PATCH 4/4] fix(abctl): move the motion keys out of ANYWHERE, where neither answer fit MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Third round on #1100. Both findings correct, and the first shows my last fix was wrong in the other direction. Measured, while the overlay is UP: handleKey forwards everything except ?/esc/q/g/G to helpVp.Update, and bubbles' viewport binds b/f to PageUp/PageDown -- so b/f, g/G and ↑↓/jk all move the overlay on every pane, usage included. With it CLOSED they move the pane, and there the coverage is ragged: pageActivePane has no paneUsage case, goTop/goBottom cover neither usage nor the two pickers, and on usage `b` is the breakdown cycle. So listing them under ANYWHERE claimed a pane behaviour that does not hold (the original defect), and gating the paging row off usage denied an overlay behaviour that does -- removing the row from the one pane whose 91-line body most needs it, on the strength of a pane fact that says nothing about what the key does while the reader is looking at the overlay. `g / G` was left advertised there through both rounds, which is the inconsistency this finding names. They are now their own group, MOVING AROUND THIS HELP, described as what they are: the overlay's own navigation, true on every pane, no gating. A note names the single pane where the closed-overlay meaning differs. panePages, anywhereKeysFor, pagingKeys and overlayOnlyKeys all go with it, and ANYWHERE is left holding only keys with one meaning -- so the rebinding check now needs NO exemption list, which is where the next `b` would have hidden. Two behavioural probes replace the gate: one presses f/b/G on all nine panes and asserts the overlay moved, one presses `b` on usage and `f` on a populated sessions table to pin both sides of the note's exception. thisPaneSuffix's claim that "the two forms can never disagree" was unenforced -- all nine titles hardcode the literal and paneName's TrimSuffix is a no-op without it, so a pane added unsuffixed would lose the active-pane marker silently. Asserted in TestHelpPurpose_EveryPaneHasOne. The scroll-hint floor test earned itself immediately: the new group took the body from 86 to 91 lines and it failed with the height to use (94) instead of going quiet the way the version it replaced did. Signed-off-by: Hai Huang Assisted-By: Claude (Anthropic AI) Signed-off-by: Hai Huang --- authbridge/cmd/abctl/README.md | 13 ++- authbridge/cmd/abctl/tui/help_overlay.go | 85 +++++++-------- authbridge/cmd/abctl/tui/help_overlay_test.go | 10 +- .../cmd/abctl/tui/help_pane_map_test.go | 100 ++++++++++++++---- 4 files changed, 131 insertions(+), 77 deletions(-) diff --git a/authbridge/cmd/abctl/README.md b/authbridge/cmd/abctl/README.md index afd0e216f..892acb96f 100644 --- a/authbridge/cmd/abctl/README.md +++ b/authbridge/cmd/abctl/README.md @@ -771,10 +771,13 @@ Layered on top of all of them: If you're on one of its five panes it's in brackets; the four key-opened panes are not steps on it, so they get a line naming that and naming where `Esc` returns them; - 4. **ANYWHERE**, then **INSIDE THE SPEND DRAWER** (`a`/`w` are live - only while it is open, so they are not mixed in with the keys that - always work — and the section is omitted on the panes where `$` - is refused); + 4. **ANYWHERE** — `?`, `p`, `q`, the three keys with one meaning + everywhere — then **MOVING AROUND THIS HELP** (`↑↓`/`jk`, `b`/`f`, + `g`/`G`: these move the overlay on every pane, and a note names the + one pane where their closed-overlay meaning differs), then **INSIDE + THE SPEND DRAWER** (`a`/`w` are live only while it is open, so they + are not mixed in with the keys that always work — and the section is + omitted on the panes where `$` is refused); 5. **EVERY PANE** — the other eight in full, purpose and every binding's description. Not compacted to bare keys: `USAGE m w b s esc` said the pane has five keys and nothing about what any of them @@ -799,7 +802,7 @@ Layered on top of all of them: edge, so the descriptions and the scope note survive a narrow terminal. - Spelling out all nine panes costs roughly three screens at 24 rows, + Spelling out all nine panes costs roughly four screens at 24 rows, which `g`/`G` and the pinned close hint are what make affordable. The overlay is the one surface with no width or height budget to defend, so it is where completeness belongs. diff --git a/authbridge/cmd/abctl/tui/help_overlay.go b/authbridge/cmd/abctl/tui/help_overlay.go index 6e4507b57..123e6323d 100644 --- a/authbridge/cmd/abctl/tui/help_overlay.go +++ b/authbridge/cmd/abctl/tui/help_overlay.go @@ -39,6 +39,7 @@ const ( jumpSectionTitle = "GO TO ANOTHER PANE" drillSectionTitle = "THE DRILL PATH" anywhereTitle = "ANYWHERE" + helpNavTitle = "MOVING AROUND THIS HELP" spendDrawerTitle = "INSIDE THE SPEND DRAWER" everyPaneTitle = "EVERY PANE" @@ -64,53 +65,45 @@ var anywhereKeys = keyGroup{ title: anywhereTitle, bindings: []keyBinding{ {"?", "this help"}, - {"↑↓ / jk", "scroll this help"}, {"p", "pause / resume the stream"}, - {"g / G", "jump to top / bottom"}, - {pagingKeys, "page up / down"}, {"q · ctrl+c", "quit"}, }, } -// pagingKeys is the one row of anywhereKeys that is not available everywhere, so -// it is named rather than written twice (here and in the filter that drops it). -const pagingKeys = "b / f" - -// overlayOnlyKeys are the anywhereKeys rows whose description is about THIS -// OVERLAY rather than the pane underneath it. They are the documented exception to -// "ANYWHERE must not claim a key the active pane rebinds": `↑↓`/`jk` navigate a -// pane and scroll the overlay, and both are true at once because the overlay is -// modal. Every other row describes the pane, so a pane rebinding it is a conflict — -// see TestHelpBody_AnywhereKeysAreNotReboundByTheActivePane. -var overlayOnlyKeys = map[string]bool{"?": true, "↑↓ / jk": true} - -// panePages reports whether `b`/`f` page the pane's own content. +// helpNavKeys move this overlay. Their own group because that is the one framing in +// which every one of them is unconditionally true. // -// paneUsage IS THE ONE THAT DOES NOT, and it is worse than merely inert there: its -// key handler runs first and binds `b` to the breakdown cycle, so an overlay -// listing "b / f page up / down" on that pane names a key that does something -// else entirely. pageActivePane has cases for events, sessions, pipeline, catalog -// and the two detail viewports; the pickers return earlier and page through their -// own table's binding. Usage is absent from both paths. -func panePages(p paneID) bool { - return p != paneUsage -} - -// anywhereKeysFor returns anywhereKeys as it applies on pane, dropping the paging -// row where paging does not exist. -func anywhereKeysFor(pane paneID) keyGroup { - if panePages(pane) { - return anywhereKeys - } - g := anywhereKeys - g.bindings = make([]keyBinding, 0, len(anywhereKeys.bindings)) - for _, kb := range anywhereKeys.bindings { - if kb.keys == pagingKeys { - continue - } - g.bindings = append(g.bindings, kb) - } - return g +// THE FIRST TWO ATTEMPTS AT THIS WERE BOTH WRONG, in opposite directions, and the +// measured behaviour is why. While the overlay is up, handleKey sends everything +// except ?/esc/q/g/G to helpVp.Update, and bubbles' viewport binds `b`/`f` to +// PageUp/PageDown — so all three rows move the overlay on EVERY pane, usage +// included. With it closed they move the pane instead, and there the coverage is +// ragged: pageActivePane has no paneUsage case, goTop/goBottom cover neither usage +// nor the two pickers, and on usage `b` is the breakdown cycle. +// +// So listing them under ANYWHERE claimed a pane behaviour that does not hold +// (the original bug), and gating a row off usage denied an overlay behaviour that +// does (the first fix). Describing them as what they are — the overlay's own +// navigation — is true everywhere and needs no gating at all. The one pane where +// the closed-overlay meaning differs is named in the note rather than by removing a +// row the reader needs to page these 86 lines. +// +// `?`/`esc`/`q` close the overlay and are deliberately NOT here: that is the one +// hint pinned in the frame's footer (see renderHelpOverlay), where it cannot be +// scrolled away, and a second copy could drift from it. +var helpNavKeys = keyGroup{ + title: helpNavTitle, + bindings: []keyBinding{ + {"↑↓ / jk", "scroll a line"}, + {"b / f", "page"}, + {"g / G", "jump to the top / bottom"}, + }, + notes: []string{ + "All three work on every pane, because they move this overlay rather than " + + "what is behind it. With it closed they move the pane's own list instead — " + + "except on the usage pane, which has no list: there b cycles the breakdown " + + "and g / G do nothing.", + }, } // spendDrawerKeys are live only while the spend drawer is open. Their own @@ -148,12 +141,12 @@ var spendDrawerKeys = keyGroup{ // A FUNCTION, AND THE ONE helpBodyLines RENDERS FROM. As a plain slice it was a // registry only the tests read, so a group added to the body and not to the slice // would have escaped every invariant check — the exact drift those checks exist to -// catch. The drawer's group is conditional for the reason the jump section drops -// `$`: on the pickers and on usage the key is refused, and a titled block -// explaining what `a` and `w` do inside a surface that cannot be opened is three -// keys of pure noise. +// catch. The drawer's group is the only conditional one left, for the reason the +// jump section drops `$`: on the pickers and on usage the key is refused, and a +// titled block explaining what `a` and `w` do inside a surface that cannot be +// opened is three keys of pure noise. func helpGlobalGroups(pane paneID) []keyGroup { - groups := []keyGroup{anywhereKeysFor(pane)} + groups := []keyGroup{anywhereKeys, helpNavKeys} if ok, _ := spendDrawerHostPane(pane); ok { groups = append(groups, spendDrawerKeys) } diff --git a/authbridge/cmd/abctl/tui/help_overlay_test.go b/authbridge/cmd/abctl/tui/help_overlay_test.go index bdf76819f..f1b9a76e0 100644 --- a/authbridge/cmd/abctl/tui/help_overlay_test.go +++ b/authbridge/cmd/abctl/tui/help_overlay_test.go @@ -532,15 +532,19 @@ func TestHelpOverlayScrollKeys(t *testing.T) { // helpNoScrollHeight is a terminal tall enough to show the whole reference at // helpWideTerminal columns, so the no-affordance case is testable. // -// IT IS BIG, AND THAT IS THE POINT. The body is 86 lines once every pane carries -// its purpose and its descriptions, and 89 rows is the exact floor. The previous +// IT IS BIG, AND THAT IS THE POINT. The body is 91 lines once every pane carries +// its purpose and its descriptions, and 94 rows is the exact floor. The previous // version of this test asked for 60 and t.Skip()ed when the content did not fit — // which, the moment the body grew, silently took the three short-terminal // assertions below with it and reported PASS. A number that has to track the body's // height is asserted, never skipped. +// +// It has already earned that: adding the MOVING AROUND THIS HELP group took the +// body from 86 lines to 91, and this failed with the value to use rather than going +// quiet again. const ( helpWideTerminal = 100 - helpNoScrollHeight = 89 + helpNoScrollHeight = 94 ) // With everything visible there must be no scroll affordance — it would be noise diff --git a/authbridge/cmd/abctl/tui/help_pane_map_test.go b/authbridge/cmd/abctl/tui/help_pane_map_test.go index 5e2a7ec15..716d7dd26 100644 --- a/authbridge/cmd/abctl/tui/help_pane_map_test.go +++ b/authbridge/cmd/abctl/tui/help_pane_map_test.go @@ -1,10 +1,13 @@ package tui import ( + "fmt" "strings" "testing" "github.com/charmbracelet/lipgloss" + + "github.com/rossoctl/cortex/authbridge/authlib/session" ) // helpWide is a wrap budget wider than any line the overlay builds, so tests @@ -81,6 +84,16 @@ func TestHelpPurpose_EveryPaneHasOne(t *testing.T) { t.Errorf("pane %v (%q) has no purpose line — the overlay would name it "+ "without saying what it shows", p, g.title) } + // thisPaneSuffix's whole claim is that paneName can strip it so the two forms + // cannot disagree — and nothing enforced it: every title hardcodes the literal, + // and paneName's TrimSuffix is a no-op on a title that omits it. A pane added + // without the suffix would lose the active-pane marker while paneName kept + // working, so the omission would be invisible. + if !strings.HasSuffix(g.title, thisPaneSuffix) { + t.Errorf("pane %v is titled %q, which does not end in %q — it would render "+ + "unmarked when the overlay is opened over it", + p, g.title, thisPaneSuffix) + } } } @@ -310,9 +323,13 @@ func splitKeys(col string) []string { // // This is the check whose absence let `b` ship wrong: anywhereKeys' own doc says it // holds only the keys that work regardless of what is on screen, and `a`/`w` were -// moved out for exactly that reason while `b` was not. The overlayOnlyKeys rows are -// the documented exception — they describe the overlay, not the pane, so both -// meanings are true at once. +// moved out for exactly that reason while `b` was not. +// +// NO EXEMPTION LIST, which is the point of having moved the motion keys into +// helpNavKeys. While they sat here the check needed `?` and `↑↓ / jk` excused as +// "these describe the overlay, not the pane" — and an exemption list is where the +// next `b` would have hidden. ANYWHERE now holds only keys with one meaning, so any +// overlap at all is a defect. func TestHelpBody_AnywhereKeysAreNotReboundByTheActivePane(t *testing.T) { for p := paneNamespaces; p <= lastPaneID; p++ { owned := map[string]string{} @@ -321,10 +338,7 @@ func TestHelpBody_AnywhereKeysAreNotReboundByTheActivePane(t *testing.T) { owned[k] = kb.desc } } - for _, kb := range anywhereKeysFor(p).bindings { - if overlayOnlyKeys[kb.keys] { - continue - } + for _, kb := range anywhereKeys.bindings { for _, k := range splitKeys(kb.keys) { if desc, clash := owned[k]; clash { t.Errorf("pane %v: ANYWHERE offers %q as %q while the pane binds %q to %q", @@ -335,31 +349,71 @@ func TestHelpBody_AnywhereKeysAreNotReboundByTheActivePane(t *testing.T) { } } -// And the behavioural half: the pane that does not page must not be offered the -// paging row, driven off pageActivePane's real cases rather than a copy of them. -func TestHelpBody_PagingRowOnlyWherePagingExists(t *testing.T) { +// The behavioural half, for the claim helpNavKeys actually makes: its three rows +// move THIS OVERLAY on every pane, and its note says the closed-overlay meaning +// differs on usage alone. +// +// Both halves are pressed rather than asserted against the switches they describe. +// The overlay half is what the previous gating fix got backwards — it removed the +// paging row from the one pane whose 86-line body most needs it, on the strength of +// a pane behaviour that says nothing about what the key does while the overlay is up. +func TestHelpNavKeys_MoveTheOverlayOnEveryPane(t *testing.T) { for p := paneNamespaces; p <= lastPaneID; p++ { - listed := false - for _, kb := range anywhereKeysFor(p).bindings { - if kb.keys == pagingKeys { - listed = true - } + // A short terminal so the body always overflows and has somewhere to scroll. + m := helpModelAt(t, p, 100, 14) + if m.helpVp.TotalLineCount() <= m.helpVp.VisibleLineCount() { + t.Fatalf("pane %v: body fits at 100x14, so scrolling is untestable", p) + } + + u, _ := m.Update(keyRune('f')) + m = u.(*model) + if m.helpVp.YOffset == 0 { + t.Errorf("pane %v: `f` did not page the overlay, but the nav group offers it", p) + } + u, _ = m.Update(keyRune('b')) + m = u.(*model) + if m.helpVp.YOffset != 0 { + t.Errorf("pane %v: `b` did not page the overlay back to the top (offset %d)", + p, m.helpVp.YOffset) } - if listed != panePages(p) { - t.Errorf("pane %v: paging row listed=%v but panePages=%v", - p, listed, panePages(p)) + u, _ = m.Update(keyRune('G')) + m = u.(*model) + if !m.helpVp.AtBottom() { + t.Errorf("pane %v: `G` did not jump the overlay to the bottom", p) } } +} - // paneUsage is the exclusion, and it is excluded because `b` means something - // else there. Press it and confirm that is still true, so this stops being a - // claim about a switch statement nobody re-reads. +// And the exception the note names: with the overlay closed, usage is the one pane +// where `b` is not paging and `g` moves nothing. +func TestHelpNavKeys_NoteNamesTheOneExceptionCorrectly(t *testing.T) { m := &model{pane: paneUsage, selectedSess: "s1"} before := m.usage.group m.handleKey(keyRune('b')) if m.usage.group == before { - t.Errorf("`b` no longer cycles the usage breakdown; if paging reached that pane, " + - "panePages should stop excluding it") + t.Errorf("`b` no longer cycles the usage breakdown — the nav note says it does") + } + + // The note also claims g/G do nothing on usage. goTop has no case for it; assert + // via the footer contract the existing TestUsagePane_DoesNotShadowGlobalG relies + // on, so both stay true together. + if strings.Contains((&model{pane: paneUsage}).helpView(), "[g]") { + t.Error("the usage footer claims [g] while the nav note says g does nothing there") + } + + // A pane on the other side of the exception, so the note cannot be made true by + // paging breaking everywhere. + s := &model{pane: paneSessions, height: 40, width: 120} + s.sessionsTbl = newSessionsTable() + s.sessionsTbl.SetHeight(10) + for i := 0; i < 40; i++ { + s.sessions = append(s.sessions, session.SessionSummary{ID: fmt.Sprintf("s%02d", i)}) + } + s.rebuildSessionsTable() + s.handleKey(keyRune('f')) + if s.sessionsTbl.Cursor() == 0 { + t.Error("`f` did not page the sessions table, so the nav note's " + + "\"moves the pane's own list\" is wrong outside usage too") } }