From 9b29666e0346f5a62f5d774507999bb6cab038c7 Mon Sep 17 00:00:00 2001 From: Codeman maintainer Date: Thu, 20 Aug 2026 02:40:05 +0200 Subject: [PATCH] fix(tui): keep the way out on the bar, and make Alt+1..9 actually switch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four faults, all reported at once, and three of them were mine from the last two commits. THE HINT VANISHED. Two independent causes. First, a leaked F1 binding: an attach whose TUI was killed leaves `F1 -> detach-client` in tmux's root table, and the claim treated "already bound" as someone else's key, so every later attach fell back to advertising the tmux chord — the bar stopped saying F1 while F1 still worked. A key already bound to `detach-client` now counts as ours. Second, width: tmux truncates a status line that overflows and drops the RIGHT-aligned segment, which is the hint. The strip now gets a budget measured from the terminal's width minus the hint, and it drops tabs from the far end until it fits. ⚠️ Measured on VISIBLE columns, not format bytes: `#[reverse]` costs zero columns, and counting it made a strip that "fitted" still truncate the hint at 80, 100, 120 and 176 columns on a real terminal. ALT+N DID NOT SWITCH. On the dashboard, a bare digit meant jump AND ATTACH, and a terminal sends Alt+N as ESC then N: when those land in separate reads — routine over SSH — the chord decodes as Escape plus a bare digit, so "switch to tab 2" threw the user into tab 2's pane. A digit now SELECTS, matching what Alt+N means in the web UI; Enter is how you go in. Inside a pane the keys never reached the TUI at all, since tmux owns the terminal, so the attach now binds Alt+1..9 in tmux's root table to `switch-client` — the strip is usable rather than decorative. ⚠️ The bar is applied to every session the strip can reach, each highlighting its own tab: with it on the attached session only, switching landed the user in a pane with no strip and no way out on screen. ⚠️ The leaked-state sweep was missing `status-position`, so it removed the marker and left the position behind — and with no marker the leftover no longer matched, making it permanently unsweepable. Found by diffing every session's options after a detach. --- src/tui/tui-app.ts | 137 ++++++++++++++++++++++++++++-------- src/tui/tui-client.ts | 36 ++++++++++ test/tui/tui-app.test.ts | 36 +++++++++- test/tui/tui-client.test.ts | 5 ++ test/tui/tui-render.test.ts | 4 +- 5 files changed, 185 insertions(+), 33 deletions(-) diff --git a/src/tui/tui-app.ts b/src/tui/tui-app.ts index 77ade6da..20d2d284 100644 --- a/src/tui/tui-app.ts +++ b/src/tui/tui-app.ts @@ -57,6 +57,7 @@ import { formatAwayDigest } from './tui-digest.js'; import { ATTACH_BANNER_MARKER, TuiClient, + type TuiSessionOptions, type TuiApprovalAnswer, type TuiEventStream, type TuiLiveSessionMetrics, @@ -348,6 +349,8 @@ export function buildAttachBanner(options: { oneKey?: string; /** The other sessions, drawn as a strip so they stay visible from inside a pane. */ tabs?: readonly TuiAttachTab[]; + /** The attaching terminal's width, so the strip can be kept clear of the hint. */ + cols?: number; }): Record { const chord = escapeTmuxFormat(detachChord(options.prefix, options.detachKey)); // Named on the bar because it is what people actually type: keeping Ctrl held @@ -359,9 +362,17 @@ export function buildAttachBanner(options: { // owns the whole line, which is what removes tmux's window list (`0:bash*`) // from the middle of it. The window-status options that would otherwise hide // it are WINDOW options, so `set-option -t ` cannot even reach them. + // The hint is measured, not estimated, and the strip is given whatever is + // left. tmux truncates a status line that overflows, and what it drops is the + // RIGHT-aligned segment — which is the hint, the one thing on the bar a user + // cannot do without. Every width tested lost it before this budget existed. + const hint = options.oneKey + ? ` ${options.oneKey} ${ATTACH_BANNER_MARKER} ` + : ` ${chord}${alias} ${ATTACH_BANNER_MARKER} `; + const budget = Math.max(0, (options.cols ?? Number.POSITIVE_INFINITY) - hint.length - 1); // The strip names the session it highlights, so the standalone label is only // a fallback for when there is no strip to draw (degraded mode has no list). - const strip = buildAttachTabs(options.tabs ?? []); + const strip = buildAttachTabs(options.tabs ?? [], 6, budget); const left = strip || (label ? ` ${label} ` : ''); return { status: 'on', @@ -387,6 +398,8 @@ export interface TuiAttachTab { index: number; label: string; active: boolean; + /** tmux session to switch to when its number is pressed inside a pane. */ + muxName?: string; } /** Per-tab label cap. Eight of these plus separators still fit an 80-column terminal. */ @@ -403,7 +416,7 @@ const ATTACH_TAB_LABEL_MAX = 12; * lost. Ellipses mark what is not shown, so a truncated strip reads as * truncated rather than as the whole list. */ -export function buildAttachTabs(tabs: readonly TuiAttachTab[], maxTabs = 6): string { +export function buildAttachTabs(tabs: readonly TuiAttachTab[], maxTabs = 6, budget = Number.POSITIVE_INFINITY): string { if (tabs.length === 0) return ''; const active = Math.max( 0, @@ -412,16 +425,28 @@ export function buildAttachTabs(tabs: readonly TuiAttachTab[], maxTabs = 6): str let start = Math.max(0, Math.min(active - Math.floor(maxTabs / 2), tabs.length - maxTabs)); if (start < 0) start = 0; const shown = tabs.slice(start, start + maxTabs); - const parts = shown.map((tab) => { - const label = truncateLabel(tab.label, ATTACH_TAB_LABEL_MAX); + const shownLabels = shown.map((tab) => truncateLabel(tab.label, ATTACH_TAB_LABEL_MAX)); + const parts = shown.map((tab, i) => { + const label = shownLabels[i]; const text = escapeTmuxFormat(`${tab.index} ${label}`); // The active tab is inverted rather than bracketed: brackets cost two // columns per tab and read as punctuation next to the session names. return tab.active ? `#[reverse] ${text} #[noreverse]` : ` ${text} `; }); + // Drop tabs from the far end until the strip fits the space the hint leaves. + // ⚠️ Measured on the VISIBLE text, not the format string: `#[reverse]` and + // friends cost zero columns, and counting them made a strip that "fitted" + // truncate the hint on a real terminal at every width tested. + const visible = (index: number): number => shownLabels[index].length + String(shown[index].index).length + 3; + let width = shown.reduce((total, _tab, index) => total + visible(index), 0); + let last = shown.length; + while (last > 1 && width + 2 > budget) { + last -= 1; + width -= visible(last); + } const head = start > 0 ? '…' : ''; - const tail = start + maxTabs < tabs.length ? '…' : ''; - return `${head}${parts.join('')}${tail}`; + const tail = start + last < tabs.length ? '…' : ''; + return `${head}${parts.slice(0, last).join('')}${tail}`; } /** @@ -504,7 +529,8 @@ export async function beginAttachHandoff( client: TuiClient, muxName: string, label: string, - tabs: readonly TuiAttachTab[] = [] + tabs: readonly TuiAttachTab[] = [], + cols?: number ): Promise { const prefix = (await client.readPrefixKey(muxName)) ?? undefined; // Read, not assumed: see detachChord() for the `d` vs `D` mix-up this closes. @@ -521,30 +547,71 @@ export async function beginAttachHandoff( // binding the user put in their own config. const alias = heldCtrlAlias(detachKey ?? DEFAULT_DETACH_KEY); const claimed = alias && (await client.readPrefixBinding(alias)) === null ? await client.bindDetachKey(alias) : false; - // The one-key way out, in the prefix-less table. Same rule: only if free. + // The one-key way out, in the prefix-less table. + // + // ⚠️ "Already bound to detach-client" counts as CLAIMED, not as taken. An + // attach whose TUI was killed leaks the binding, and treating that leak as + // someone else's binding made every later attach fall back to advertising + // the tmux chord — so the bar stopped saying F1 while F1 still worked, which + // is the worst of both. Anything else there is genuinely the user's and is + // left alone. + const existing = await client.readPrefixBinding(ONE_KEY_DETACH, 'root'); const oneKey = - (await client.readPrefixBinding(ONE_KEY_DETACH, 'root')) === null - ? await client.bindDetachKey(ONE_KEY_DETACH, 'root') - : false; - const banner = buildAttachBanner({ - ...(prefix ? { prefix } : {}), - ...(detachKey ? { detachKey } : {}), - ...(claimed && alias ? { heldAlias: alias } : {}), - ...(oneKey ? { oneKey: ONE_KEY_DETACH } : {}), - label, - tabs, - }); - const options = await client.readSessionOptions(muxName, Object.keys(banner)); - await client.applySessionOptions(muxName, banner); + existing === 'detach-client' + ? true + : existing === null + ? await client.bindDetachKey(ONE_KEY_DETACH, 'root') + : false; + // Alt+1..9 switch sessions from inside the pane, so the strip on the bar is + // usable rather than decorative. Only the first nine, only sessions that + // really have a pane, and only keys tmux reports as free — the same rule the + // way-out key follows, so a binding of the user's own is never shadowed. + const switchKeys: string[] = []; + for (const tab of tabs.slice(0, 9)) { + if (!tab.muxName) continue; + const key = `M-${tab.index}`; + const bound = await client.readPrefixBinding(key, 'root'); + if (bound !== null && !bound.startsWith('switch-client')) continue; + if (await client.bindSwitchKey(key, tab.muxName)) switchKeys.push(key); + } + const bannerFor = (activeMux: string, ownLabel: string): Record => + buildAttachBanner({ + ...(prefix ? { prefix } : {}), + ...(detachKey ? { detachKey } : {}), + ...(claimed && alias ? { heldAlias: alias } : {}), + ...(oneKey ? { oneKey: ONE_KEY_DETACH } : {}), + ...(cols ? { cols } : {}), + label: ownLabel, + tabs: tabs.map((tab) => ({ ...tab, active: tab.muxName === activeMux })), + }); + + // ⚠️ The bar goes on EVERY session the strip can switch to, not just the one + // being attached. `switch-client` moves this client to another session, and + // that session draws its OWN status line: with the bar only on the first one, + // pressing Alt+2 landed the user in a pane with no strip and, worse, no way + // out on screen. Each copy highlights its own tab, so the strip tracks where + // you actually are. + const banner = bannerFor(muxName, label); + const dressed: Array<{ muxName: string; options: TuiSessionOptions }> = []; + const targets = new Map([[muxName, label]]); + for (const tab of tabs.slice(0, 9)) { + if (tab.muxName && !targets.has(tab.muxName)) targets.set(tab.muxName, tab.label); + } + for (const [target, targetLabel] of targets) { + const snapshot = await client.readSessionOptions(target, Object.keys(banner)); + if (snapshot) dressed.push({ muxName: target, options: snapshot }); + await client.applySessionOptions(target, bannerFor(target, targetLabel)); + } return { chord: oneKey ? ONE_KEY_DETACH : detachChord(prefix, detachKey), async restore(): Promise { + for (const key of switchKeys) await client.unbindSwitchKey(key); if (oneKey) await client.unbindDetachKey(ONE_KEY_DETACH, 'root'); if (claimed && alias) await client.unbindDetachKey(alias); // Options first, then the size: dropping the status bar gives its row // back to the pane, and the resize is what re-pins the browser's // authority over the window. - if (options) await client.restoreSessionOptions(muxName, options); + for (const entry of dressed) await client.restoreSessionOptions(entry.muxName, entry.options); if (sizing) await client.restoreWindowSizing(muxName, sizing); }, }; @@ -644,11 +711,11 @@ export function footerKeysFor(mode: TuiUiMode, glyphs: TuiGlyphSet, context: Tui return ['j/k scroll', 'esc close']; case 'list': { if (!context.server) { - return [`${glyphs.updown} select`, `${glyphs.enter} attach`, '1-9 jump', '? help', 'q quit']; + return [`${glyphs.updown} select`, `${glyphs.enter} attach`, '1-9 switch', '? help', 'q quit']; } const keys = [`${glyphs.updown} select`, `${glyphs.enter} attach`]; if (context.approval === 'menu') keys.push('y approve', 'n deny', '1-9 option'); - else keys.push('1-9 jump'); + else keys.push('1-9 switch'); keys.push(context.approval === 'idle' ? 'p reply' : 'p prompt'); if (context.approval !== 'menu') keys.push('n new'); keys.push('x kill', '/ search', 'g digest', '? help', 'q quit'); @@ -665,11 +732,10 @@ export function helpKeysFor(glyphs: TuiGlyphSet, context: TuiKeymapContext): Arr const keys: Array<[string, string]> = [ [`${glyphs.updown} / j k`, 'select'], [glyphs.enter, 'attach — on a RECENT row, resume that conversation'], - ['1-9', 'jump and attach'], + ['1-9', 'switch to that session'], // The web UI's tab switching, as close as a terminal can carry it: Alt+N // matches exactly, while Alt+[ / Alt+] cannot be transmitted (ESC+[ IS the // CSI introducer) so the brackets do that job unmodified. - ['alt+1-9', 'switch to that session, without attaching'], ['[ / ]', 'previous / next session'], ['tab', 'next session'], // The one key that is not the TUI's: an attach hands the terminal to tmux, @@ -1533,7 +1599,13 @@ class TuiApp { for (const row of this.model.rows()) { if (row.group === 'recent') continue; index += 1; - tabs.push({ index, label: rowLabel(row.session), active: row.session.sessionId === activeId }); + const muxName = (row.session.muxName ?? '').trim(); + tabs.push({ + index, + label: rowLabel(row.session), + active: row.session.sessionId === activeId, + ...(muxName ? { muxName } : {}), + }); } return tabs; } @@ -1629,7 +1701,13 @@ class TuiApp { } if (value >= '1' && value <= '9') { - if (this.model.cursorToIndex(Number.parseInt(value, 10))) void this.attachSelected(); + // SELECT, never attach. A digit used to jump AND hand the terminal to + // that pane, which made Alt+N unusable: a terminal sends Alt+N as ESC + // then N, and when those land in separate reads — routine over SSH — the + // chord decodes as Escape plus a bare digit, so "switch to tab 2" threw + // the user into tab 2's pane instead. Selecting matches what Alt+N means + // in the web UI, and Enter is how you go in. + this.model.cursorToIndex(Number.parseInt(value, 10)); return; } switch (value) { @@ -2172,7 +2250,8 @@ class TuiApp { this.client, muxName, rowLabel(row.session), - this.attachTabs(row.session.sessionId) + this.attachTabs(row.session.sessionId), + this.currentLayout().cols ); this.detachChordLabel = handoff.chord; this.screen.leave(); diff --git a/src/tui/tui-client.ts b/src/tui/tui-client.ts index 679cbf27..4302ad0c 100644 --- a/src/tui/tui-client.ts +++ b/src/tui/tui-client.ts @@ -951,6 +951,36 @@ export class TuiClient { } } + /** + * Bind a prefix-less key to switch this client to another session, so the tab + * strip on the attach bar is not just a picture: Alt+1..9 moves between + * sessions from INSIDE a pane, the way it does in the web UI. + * + * `switch-client` rather than detach-then-attach: it keeps the terminal, so + * the move is instant and the TUI stays blocked in its `spawnSync` exactly as + * before, still holding the restore it owes. + */ + async bindSwitchKey(key: string, target: string): Promise { + if (!MUX_NAME_PATTERN.test(target)) return false; + try { + await this.exec('tmux', ['-L', this.socket, 'bind-key', '-T', 'root', key, 'switch-client', '-t', target]); + return true; + } catch { + return false; + } + } + + /** Give back a switch key, but only while it still points at a session. */ + async unbindSwitchKey(key: string): Promise { + const bound = await this.readPrefixBinding(key, 'root'); + if (!bound || !bound.startsWith('switch-client')) return; + try { + await this.exec('tmux', ['-L', this.socket, 'unbind-key', '-T', 'root', key]); + } catch { + /* a stray switch binding is harmless next to failing an attach */ + } + } + /** * Give a key back, but ONLY while it still means `detach-client`. Anything * else there is the user's, arrived after we bound ours, and must not be @@ -1103,6 +1133,12 @@ export class TuiClient { // unsetting one index leaves an empty array, which renders as a blank bar. await this.setOption(['-u', '-t', name, 'status-format']); await this.setOption(['-u', '-t', name, 'status-style']); + // ⚠️ `status-position` MUST be swept with the rest. It was missing here, + // so a sweep cleaned the marker and left the position behind — and with + // the marker gone the leftover no longer matched, which made it + // permanently unsweepable. Every option the banner writes has to be + // undone by the same pass that recognises it. + await this.setOption(['-u', '-t', name, 'status-position']); await this.setOption(['-t', name, 'status', 'off']); cleared += 1; } diff --git a/test/tui/tui-app.test.ts b/test/tui/tui-app.test.ts index 9c1e794c..c76bf5ad 100644 --- a/test/tui/tui-app.test.ts +++ b/test/tui/tui-app.test.ts @@ -161,7 +161,7 @@ describe('footerKeysFor', () => { it('advertises only the verbs this build implements', () => { const keys = footerKeysFor('list', GLYPHS, { server: true }).join(' '); expect(keys).toContain('attach'); - expect(keys).toContain('1-9 jump'); + expect(keys).toContain('1-9 switch'); expect(keys).toContain('n new'); expect(keys).toContain('p prompt'); expect(keys).toContain('/ search'); @@ -179,7 +179,7 @@ describe('footerKeysFor', () => { expect(keys).toContain('1-9 option'); // `n` cannot mean two things at once, and denying is what it does here. expect(keys).not.toContain('n new'); - expect(keys).not.toContain('1-9 jump'); + expect(keys).not.toContain('1-9 switch'); }); it('sends an idle prompt to the composer instead of offering approve/deny', () => { @@ -673,6 +673,38 @@ describe('the way out of an attach', () => { expect(crowded['status-format[0]']).toContain('#[bold]F1#[nobold] back to the codeman dashboard'); }); + it('fits the strip to the terminal, measuring VISIBLE columns not format bytes', () => { + // The test above only checks the hint is in the format STRING, which it + // always was. tmux truncates what it cannot fit and drops the right-aligned + // segment, so on a real terminal the hint vanished at every width tested + // while that assertion stayed green. + const hint = ' F1 back to the codeman dashboard '; + for (const cols of [80, 100, 120, 176]) { + const bar = buildAttachBanner({ + oneKey: 'F1', + cols, + tabs: Array.from({ length: 12 }, (_, i) => ({ + index: i + 1, + label: `w${i + 1}-session-name`, + active: i === 1, + })), + })['status-format[0]']; + const visible = bar.replace(/#\[[^\]]*\]/g, ''); + expect({ cols, fits: visible.length <= cols }).toEqual({ cols, fits: true }); + expect(visible.length).toBeGreaterThanOrEqual(hint.length); + } + }); + + it('keeps at least one tab even when the hint eats almost the whole bar', () => { + const bar = buildAttachBanner({ + oneKey: 'F1', + cols: 40, + tabs: [{ index: 1, label: 'w1-case', active: true }], + })['status-format[0]']; + expect(bar).toContain('1 w1-case'); + expect(bar).toContain('back to the codeman dashboard'); + }); + it("tells the help overlay how to get back, in the socket's own prefix", () => { const keys = helpKeysFor(GLYPHS, { server: true, detach: 'Ctrl+A then d' }); const detach = keys.find(([key]) => key === 'Ctrl+A then d'); diff --git a/test/tui/tui-client.test.ts b/test/tui/tui-client.test.ts index 659dbae5..439814c2 100644 --- a/test/tui/tui-client.test.ts +++ b/test/tui/tui-client.test.ts @@ -659,6 +659,11 @@ describe('TuiClient.clearLeakedAttachBanners', () => { // leaves an EMPTY array, which renders as a blank bar. expect(sets.some((args) => args.includes('-u') && args.includes('status-format'))).toBe(true); expect(sets.some((args) => args.includes('-u') && args.includes('status-style'))).toBe(true); + // ⚠️ Every option the banner writes must be undone by the pass that + // recognises it. `status-position` was missing, so a sweep removed the + // marker and left the position behind — and with no marker the leftover + // stopped matching, making it permanently unsweepable. + expect(sets.some((args) => args.includes('-u') && args.includes('status-position'))).toBe(true); expect(sets.some((args) => args.join(' ').endsWith('status off'))).toBe(true); expect(sets.every((args) => !args.includes('status-format[0]'))).toBe(true); }); diff --git a/test/tui/tui-render.test.ts b/test/tui/tui-render.test.ts index 6f96b088..43e5f2c9 100644 --- a/test/tui/tui-render.test.ts +++ b/test/tui/tui-render.test.ts @@ -158,7 +158,7 @@ describe('renderFrame structure', () => { ' │', ' │', ' │', - ' ↑↓ select · ↵ attach · 1-9 jump · y/n answer · p prompt · n new · x kill · / search · g digest · ?', + ' ↑↓ select · ↵ attach · 1-9 switch · y/n answer · p prompt · n new · x kill · / search · g digest ·', ]); }); @@ -183,7 +183,7 @@ describe('renderFrame structure', () => { '', '', '', - ' ↑↓ select · ↵ attach · 1-9 jump · y/n answe', + ' ↑↓ select · ↵ attach · 1-9 switch · y/n ans', ]); });