From 4133f4bd37cfbd8b4bf457794ecaf6ff734233a1 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Sat, 14 Feb 2026 19:15:38 +0100 Subject: [PATCH] refactor(tui): clarify searchable select list width layout (#16378) Merged via /review-pr -> /prepare-pr -> /merge-pr. Prepared head SHA: fecbade822f8163f12b7da441b567acb42e6f809 Co-authored-by: steipete <58493+steipete@users.noreply.github.com> Co-authored-by: steipete <58493+steipete@users.noreply.github.com> Reviewed-by: @steipete --- CHANGELOG.md | 1 + .../server.control-server.test-harness.ts | 19 ++--- src/commands/doctor.e2e-harness.ts | 71 +++++++++---------- .../bot.create-telegram-bot.test-harness.ts | 22 +++--- src/test-utils/vitest-mock-fn.ts | 6 ++ .../components/searchable-select-list.test.ts | 55 ++++++++++++++ src/tui/components/searchable-select-list.ts | 57 +++++++++++---- 7 files changed, 163 insertions(+), 68 deletions(-) create mode 100644 src/test-utils/vitest-mock-fn.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 0959984824..6c1e8929d6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -39,6 +39,7 @@ Docs: https://docs.openclaw.ai - Security/BlueBubbles: require explicit `mediaLocalRoots` allowlists for local outbound media path reads to prevent local file disclosure. (#16322) Thanks @mbelinky. - Cron/Slack: preserve agent identity (name and icon) when cron jobs deliver outbound messages. (#16242) Thanks @robbyczgw-cla. - Discord: prefer gateway guild id when logging inbound messages so cached-miss guilds do not appear as `guild=dm`. Thanks @thewilloftheshadow. +- TUI: refactor searchable select list description layout and add regression coverage for ANSI-highlight width bounds. ## 2026.2.14 diff --git a/src/browser/server.control-server.test-harness.ts b/src/browser/server.control-server.test-harness.ts index b8c4dcda2b..630440806c 100644 --- a/src/browser/server.control-server.test-harness.ts +++ b/src/browser/server.control-server.test-harness.ts @@ -3,6 +3,7 @@ import { type AddressInfo, createServer } from "node:net"; import os from "node:os"; import path from "node:path"; import { afterAll, afterEach, beforeAll, beforeEach, vi } from "vitest"; +import type { MockFn } from "../test-utils/vitest-mock-fn.js"; type HarnessState = { testPort: number; @@ -22,23 +23,23 @@ const state: HarnessState = { prevGatewayPort: undefined, }; -export function getBrowserControlServerTestState() { +export function getBrowserControlServerTestState(): HarnessState { return state; } -export function getBrowserControlServerBaseUrl() { +export function getBrowserControlServerBaseUrl(): string { return `http://127.0.0.1:${state.testPort}`; } -export function setBrowserControlServerCreateTargetId(targetId: string | null) { +export function setBrowserControlServerCreateTargetId(targetId: string | null): void { state.createTargetId = targetId; } -export function setBrowserControlServerAttachOnly(attachOnly: boolean) { +export function setBrowserControlServerAttachOnly(attachOnly: boolean): void { state.cfgAttachOnly = attachOnly; } -export function setBrowserControlServerReachable(reachable: boolean) { +export function setBrowserControlServerReachable(reachable: boolean): void { state.reachable = reachable; } @@ -51,8 +52,8 @@ const cdpMocks = vi.hoisted(() => ({ })), })); -export function getCdpMocks() { - return cdpMocks; +export function getCdpMocks(): { createTargetViaCdp: MockFn; snapshotAria: MockFn } { + return cdpMocks as unknown as { createTargetViaCdp: MockFn; snapshotAria: MockFn }; } const pwMocks = vi.hoisted(() => ({ @@ -97,8 +98,8 @@ const pwMocks = vi.hoisted(() => ({ waitForViaPlaywright: vi.fn(async () => {}), })); -export function getPwMocks() { - return pwMocks; +export function getPwMocks(): Record { + return pwMocks as unknown as Record; } const chromeUserDataDir = vi.hoisted(() => ({ dir: "/tmp/openclaw" })); diff --git a/src/commands/doctor.e2e-harness.ts b/src/commands/doctor.e2e-harness.ts index 62de3c7ca3..6aef868d55 100644 --- a/src/commands/doctor.e2e-harness.ts +++ b/src/commands/doctor.e2e-harness.ts @@ -2,6 +2,7 @@ import fs from "node:fs"; import os from "node:os"; import path from "node:path"; import { afterEach, beforeEach, vi } from "vitest"; +import type { MockFn } from "../test-utils/vitest-mock-fn.js"; let originalIsTTY: boolean | undefined; let originalStateDir: string | undefined; @@ -19,40 +20,40 @@ function setStdinTty(value: boolean | undefined) { } } -export const readConfigFileSnapshot: ReturnType = vi.fn(); -export const confirm: ReturnType = vi.fn().mockResolvedValue(true); -export const select: ReturnType = vi.fn().mockResolvedValue("node"); -export const note: ReturnType = vi.fn(); -export const writeConfigFile: ReturnType = vi.fn().mockResolvedValue(undefined); -export const resolveOpenClawPackageRoot: ReturnType = vi.fn().mockResolvedValue(null); -export const runGatewayUpdate: ReturnType = vi.fn().mockResolvedValue({ +export const readConfigFileSnapshot = vi.fn() as unknown as MockFn; +export const confirm = vi.fn().mockResolvedValue(true) as unknown as MockFn; +export const select = vi.fn().mockResolvedValue("node") as unknown as MockFn; +export const note = vi.fn() as unknown as MockFn; +export const writeConfigFile = vi.fn().mockResolvedValue(undefined) as unknown as MockFn; +export const resolveOpenClawPackageRoot = vi.fn().mockResolvedValue(null) as unknown as MockFn; +export const runGatewayUpdate = vi.fn().mockResolvedValue({ status: "skipped", mode: "unknown", steps: [], durationMs: 0, -}); -export const migrateLegacyConfig: ReturnType = vi.fn((raw: unknown) => ({ +}) as unknown as MockFn; +export const migrateLegacyConfig = vi.fn((raw: unknown) => ({ config: raw as Record, changes: ["Moved routing.allowFrom → channels.whatsapp.allowFrom."], -})); +})) as unknown as MockFn; -export const runExec: ReturnType = vi.fn().mockResolvedValue({ +export const runExec = vi.fn().mockResolvedValue({ stdout: "", stderr: "", -}); -export const runCommandWithTimeout: ReturnType = vi.fn().mockResolvedValue({ +}) as unknown as MockFn; +export const runCommandWithTimeout = vi.fn().mockResolvedValue({ stdout: "", stderr: "", code: 0, signal: null, killed: false, -}); +}) as unknown as MockFn; -export const ensureAuthProfileStore: ReturnType = vi +export const ensureAuthProfileStore = vi .fn() - .mockReturnValue({ version: 1, profiles: {} }); + .mockReturnValue({ version: 1, profiles: {} }) as unknown as MockFn; -export const legacyReadConfigFileSnapshot: ReturnType = vi.fn().mockResolvedValue({ +export const legacyReadConfigFileSnapshot = vi.fn().mockResolvedValue({ path: "/tmp/openclaw.json", exists: false, raw: null, @@ -61,30 +62,28 @@ export const legacyReadConfigFileSnapshot: ReturnType = vi.fn().mo config: {}, issues: [], legacyIssues: [], -}); -export const createConfigIO: ReturnType = vi.fn(() => ({ +}) as unknown as MockFn; +export const createConfigIO = vi.fn(() => ({ readConfigFileSnapshot: legacyReadConfigFileSnapshot, -})); +})) as unknown as MockFn; -export const findLegacyGatewayServices: ReturnType = vi.fn().mockResolvedValue([]); -export const uninstallLegacyGatewayServices: ReturnType = vi +export const findLegacyGatewayServices = vi.fn().mockResolvedValue([]) as unknown as MockFn; +export const uninstallLegacyGatewayServices = vi.fn().mockResolvedValue([]) as unknown as MockFn; +export const findExtraGatewayServices = vi.fn().mockResolvedValue([]) as unknown as MockFn; +export const renderGatewayServiceCleanupHints = vi .fn() - .mockResolvedValue([]); -export const findExtraGatewayServices: ReturnType = vi.fn().mockResolvedValue([]); -export const renderGatewayServiceCleanupHints: ReturnType = vi - .fn() - .mockReturnValue(["cleanup"]); -export const resolveGatewayProgramArguments: ReturnType = vi.fn().mockResolvedValue({ + .mockReturnValue(["cleanup"]) as unknown as MockFn; +export const resolveGatewayProgramArguments = vi.fn().mockResolvedValue({ programArguments: ["node", "cli", "gateway", "--port", "18789"], -}); -export const serviceInstall: ReturnType = vi.fn().mockResolvedValue(undefined); -export const serviceIsLoaded: ReturnType = vi.fn().mockResolvedValue(false); -export const serviceStop: ReturnType = vi.fn().mockResolvedValue(undefined); -export const serviceRestart: ReturnType = vi.fn().mockResolvedValue(undefined); -export const serviceUninstall: ReturnType = vi.fn().mockResolvedValue(undefined); -export const callGateway: ReturnType = vi +}) as unknown as MockFn; +export const serviceInstall = vi.fn().mockResolvedValue(undefined) as unknown as MockFn; +export const serviceIsLoaded = vi.fn().mockResolvedValue(false) as unknown as MockFn; +export const serviceStop = vi.fn().mockResolvedValue(undefined) as unknown as MockFn; +export const serviceRestart = vi.fn().mockResolvedValue(undefined) as unknown as MockFn; +export const serviceUninstall = vi.fn().mockResolvedValue(undefined) as unknown as MockFn; +export const callGateway = vi .fn() - .mockRejectedValue(new Error("gateway closed")); + .mockRejectedValue(new Error("gateway closed")) as unknown as MockFn; vi.mock("@clack/prompts", () => ({ confirm, diff --git a/src/telegram/bot.create-telegram-bot.test-harness.ts b/src/telegram/bot.create-telegram-bot.test-harness.ts index 6201d4cb90..60824b8a2e 100644 --- a/src/telegram/bot.create-telegram-bot.test-harness.ts +++ b/src/telegram/bot.create-telegram-bot.test-harness.ts @@ -1,8 +1,10 @@ -import { beforeEach, vi, type Mock } from "vitest"; +import { beforeEach, vi } from "vitest"; +import type { MockFn } from "../test-utils/vitest-mock-fn.js"; import { resetInboundDedupe } from "../auto-reply/reply/inbound-dedupe.js"; -type AnyMock = Mock<(...args: unknown[]) => unknown>; -type AnyAsyncMock = Mock<(...args: unknown[]) => Promise>; +type AnyMock = MockFn<(...args: unknown[]) => unknown>; +type AnyAsyncMock = MockFn<(...args: unknown[]) => Promise>; + type ReplyOpts = | { onReplyStart?: () => void | Promise; @@ -74,12 +76,12 @@ vi.mock("../pairing/pairing-store.js", () => ({ upsertChannelPairingRequest, })); -export const useSpy: Mock<(arg: unknown) => void> = vi.fn(); -export const middlewareUseSpy: Mock<(...args: unknown[]) => unknown> = vi.fn(); -export const onSpy: Mock<(...args: unknown[]) => unknown> = vi.fn(); -export const stopSpy: Mock<(...args: unknown[]) => unknown> = vi.fn(); -export const commandSpy: Mock<(...args: unknown[]) => unknown> = vi.fn(); -export const botCtorSpy: Mock<(...args: unknown[]) => unknown> = vi.fn(); +export const useSpy: MockFn<(arg: unknown) => void> = vi.fn(); +export const middlewareUseSpy: AnyMock = vi.fn(); +export const onSpy: AnyMock = vi.fn(); +export const stopSpy: AnyMock = vi.fn(); +export const commandSpy: AnyMock = vi.fn(); +export const botCtorSpy: AnyMock = vi.fn(); export const answerCallbackQuerySpy: AnyAsyncMock = vi.fn(async () => undefined); export const sendChatActionSpy: AnyMock = vi.fn(); export const setMessageReactionSpy: AnyAsyncMock = vi.fn(async () => undefined); @@ -154,7 +156,7 @@ vi.mock("@grammyjs/transformer-throttler", () => ({ apiThrottler: () => throttlerSpy(), })); -export const replySpy: Mock<(ctx: unknown, opts?: ReplyOpts) => Promise> = vi.fn( +export const replySpy: MockFn<(ctx: unknown, opts?: ReplyOpts) => Promise> = vi.fn( async (_ctx, opts) => { await opts?.onReplyStart?.(); return undefined; diff --git a/src/test-utils/vitest-mock-fn.ts b/src/test-utils/vitest-mock-fn.ts new file mode 100644 index 0000000000..9e9526d7bd --- /dev/null +++ b/src/test-utils/vitest-mock-fn.ts @@ -0,0 +1,6 @@ +// Centralized Vitest mock type for harness modules under `src/`. +// Using an explicit named type avoids exporting inferred `vi.fn()` types that can trip TS2742. +// +// oxlint-disable-next-line typescript/no-explicit-any +export type MockFn any = (...args: any[]) => any> = + import("vitest").Mock; diff --git a/src/tui/components/searchable-select-list.test.ts b/src/tui/components/searchable-select-list.test.ts index b618036743..35bada1f09 100644 --- a/src/tui/components/searchable-select-list.test.ts +++ b/src/tui/components/searchable-select-list.test.ts @@ -1,4 +1,5 @@ import { describe, expect, it } from "vitest"; +import { visibleWidth } from "../../terminal/ansi.js"; import { SearchableSelectList, type SearchableSelectListTheme } from "./searchable-select-list.js"; const mockTheme: SearchableSelectListTheme = { @@ -48,6 +49,60 @@ describe("SearchableSelectList", () => { expect(output).toContain(tail); }); + it("does not show description layout at width 40 (boundary)", () => { + const items = [ + { value: "one", label: "one", description: "desc" }, + { value: "two", label: "two", description: "desc" }, + ]; + const list = new SearchableSelectList(items, 5, mockTheme); + list.setSelectedIndex(1); // ensure first row is not selected so description styling is applied + + const output = list.render(40).join("\n"); + expect(output).not.toContain("(desc)"); + }); + + it("shows description layout at width 41 (boundary)", () => { + const items = [ + { value: "one", label: "one", description: "desc" }, + { value: "two", label: "two", description: "desc" }, + ]; + const list = new SearchableSelectList(items, 5, mockTheme); + list.setSelectedIndex(1); // ensure first row is not selected so description styling is applied + + const output = list.render(41).join("\n"); + expect(output).toContain("(desc)"); + }); + + it("keeps ANSI-highlighted description rows within terminal width", () => { + const ansiTheme: SearchableSelectListTheme = { + selectedPrefix: (t) => t, + selectedText: (t) => t, + description: (t) => t, + scrollInfo: (t) => t, + noMatch: (t) => t, + searchPrompt: (t) => t, + searchInput: (t) => t, + matchHighlight: (t) => `\u001b[31m${t}\u001b[0m`, + }; + const label = `provider/${"x".repeat(80)}`; + const items = [ + { value: label, label, description: "Some description text that should not overflow" }, + { value: "other", label: "other", description: "Other description" }, + ]; + const list = new SearchableSelectList(items, 5, ansiTheme); + list.setSelectedIndex(1); // make first row non-selected so description styling is applied + + for (const ch of "provider") { + list.handleInput(ch); + } + + const width = 80; + const output = list.render(width); + for (const line of output) { + expect(visibleWidth(line)).toBeLessThanOrEqual(width); + } + }); + it("filters items when typing", () => { const list = new SearchableSelectList(testItems, 5, mockTheme); diff --git a/src/tui/components/searchable-select-list.ts b/src/tui/components/searchable-select-list.ts index 6fced886ea..0d00fb8135 100644 --- a/src/tui/components/searchable-select-list.ts +++ b/src/tui/components/searchable-select-list.ts @@ -33,6 +33,12 @@ export class SearchableSelectList implements Component { onCancel?: () => void; onSelectionChange?: (item: SelectItem) => void; + private static readonly DESCRIPTION_LAYOUT_MIN_WIDTH = 40; + private static readonly DESCRIPTION_MIN_WIDTH = 12; + private static readonly DESCRIPTION_SPACING_WIDTH = 2; + // Keep a small right margin so we don't risk wrapping due to styling/terminal quirks. + private static readonly RIGHT_MARGIN_WIDTH = 2; + constructor(items: SelectItem[], maxVisible: number, theme: SearchableSelectListTheme) { this.items = items; this.filteredItems = items; @@ -218,23 +224,20 @@ export class SearchableSelectList implements Component { const prefixWidth = prefix.length; const displayValue = this.getItemLabel(item); - if (item.description && width > 40) { - const minDescriptionWidth = 12; - const spacingWidth = 2; - const availableWidth = Math.max(1, width - prefixWidth - 2); - - if (availableWidth > minDescriptionWidth + spacingWidth + 1) { - const maxValueWidth = availableWidth - minDescriptionWidth - spacingWidth; - const truncatedValue = truncateToWidth(displayValue, maxValueWidth, ""); + const description = item.description; + if (description) { + const descriptionLayout = this.getDescriptionLayout(width, prefixWidth); + if (descriptionLayout) { + const truncatedValue = truncateToWidth(displayValue, descriptionLayout.maxValueWidth, ""); const valueText = this.highlightMatch(truncatedValue, query); const usedByValue = visibleWidth(valueText); - const remainingWidth = availableWidth - usedByValue; + const remainingWidth = descriptionLayout.availableWidth - usedByValue; + const descriptionWidth = remainingWidth - descriptionLayout.spacingWidth; - if (remainingWidth > spacingWidth + 1) { - const descriptionWidth = remainingWidth - spacingWidth; - const spacing = " ".repeat(spacingWidth); - const truncatedDesc = truncateToWidth(item.description, descriptionWidth, ""); + if (descriptionWidth >= SearchableSelectList.DESCRIPTION_MIN_WIDTH) { + const spacing = " ".repeat(descriptionLayout.spacingWidth); + const truncatedDesc = truncateToWidth(description, descriptionWidth, ""); // Highlight plain text first, then apply theme styling to avoid corrupting ANSI codes const highlightedDesc = this.highlightMatch(truncatedDesc, query); const descText = isSelected ? highlightedDesc : this.theme.description(highlightedDesc); @@ -251,6 +254,34 @@ export class SearchableSelectList implements Component { return isSelected ? this.theme.selectedText(line) : line; } + private getDescriptionLayout( + width: number, + prefixWidth: number, + ): { availableWidth: number; maxValueWidth: number; spacingWidth: number } | null { + if (width <= SearchableSelectList.DESCRIPTION_LAYOUT_MIN_WIDTH) { + return null; + } + + const availableWidth = Math.max( + 1, + width - prefixWidth - SearchableSelectList.RIGHT_MARGIN_WIDTH, + ); + const maxValueWidth = + availableWidth - + SearchableSelectList.DESCRIPTION_MIN_WIDTH - + SearchableSelectList.DESCRIPTION_SPACING_WIDTH; + + if (maxValueWidth < 1) { + return null; + } + + return { + availableWidth, + maxValueWidth, + spacingWidth: SearchableSelectList.DESCRIPTION_SPACING_WIDTH, + }; + } + handleInput(keyData: string): void { if (isKeyRelease(keyData)) { return;