fix(coding-agent): paint selectedBg band on focused HookSelector row

The multi-select (checkbox) ask picker signaled focus only by shifting the
label fg to `accent` and flipping the checkbox glyph between `accent`
and `dim`. On themes where `accent` is close to `text` (built-in
`light`, others) the focused row was effectively invisible: `↑/↓` would
change the toggle target with no perceptible cue. Radio pickers dodged this
because the glyph shape changes (`◉` vs `○`); checkboxes always render
`☑`/`☐` regardless of focus.

Root cause: HookSelectorComponent's option rendering picked focus via
`textColor = isSelected ? 'accent' : 'text'` and the fallback
`marker ?? cursorChevron` — so with any marker in play the chevron
disappeared, and the only remaining signal was fg color contrast.

Fix: route rendered lines through a `SelectorRow = { text, highlight }`
carrier. `OutlinedList` paints highlighted rows with
`theme.bg('selectedBg', wrappedLine + padding)` inside the border rails,
and the non-outlined plain list feeds the same painter as `Text`'s
`customBgFn` (which `applyBackgroundToLine` already extends across
wrap continuations). The band spans label plus wrapped description rows
so the focus reads as one continuous bar, independent of accent/text
contrast. Precedent: the Ctrl+R history overlay and plan-review overlay
use the same selectedBg-band pattern.

Regression tests cover both outlined and non-outlined lists, focus
movement, control rows past markableCount, and multi-line description
highlighting.

Fixes #4157
This commit is contained in:
roboomp
2026-07-01 15:15:40 +00:00
parent f70e4f1570
commit 8a5fb33464
3 changed files with 160 additions and 24 deletions
+4
View File
@@ -2,6 +2,10 @@
## [Unreleased]
### Fixed
- Fixed the multi-select (checkbox) `ask` picker showing no visible cursor on themes where `accent` fg is close to the terminal foreground; the focused option now renders as a full-width `selectedBg` highlight band spanning label and wrapped description rows, matching the Ctrl+R history overlay ([#4157](https://github.com/can1357/oh-my-pi/issues/4157)).
## [16.2.12] - 2026-07-01
### Breaking Changes
@@ -113,11 +113,23 @@ function splitLeadingSpacesForWrap(line: string, width: number): { indent: strin
};
}
class OutlinedList extends Container {
#lines: string[] = [];
/** One row fed to {@link OutlinedList} or the plain list container. `highlight`
* causes the row (and its wrapped continuations, plus trailing padding) to be
* painted with the theme's `selectedBg` band — the focus cue that survives
* themes where `accent` fg is close to the terminal foreground. */
type SelectorRow = { text: string; highlight: boolean };
setLines(lines: string[]): void {
this.#lines = lines;
/** Paint `content` with the `selectedBg` background, applied AFTER any inner
* ANSI styling so the band spans padding as well as content. */
function paintSelectedRow(content: string): string {
return theme.bg("selectedBg", content);
}
class OutlinedList extends Container {
#rows: SelectorRow[] = [];
setLines(rows: readonly SelectorRow[]): void {
this.#rows = rows.slice();
this.invalidate();
}
@@ -126,15 +138,17 @@ class OutlinedList extends Container {
const horizontal = borderColor(theme.boxRound.horizontal.repeat(Math.max(1, width)));
const innerWidth = Math.max(1, width - 2);
const content: string[] = [];
for (const line of this.#lines) {
const normalized = replaceTabs(line);
for (const row of this.#rows) {
const normalized = replaceTabs(row.text);
const { indent, body } = splitLeadingSpacesForWrap(normalized, innerWidth);
const wrapped = wrapTextWithAnsi(body, Math.max(1, innerWidth - visibleWidth(indent)));
for (const wrappedBody of wrapped.length > 0 ? wrapped : [""]) {
const wrappedLine = `${indent}${wrappedBody}`;
const pad = Math.max(0, innerWidth - visibleWidth(wrappedLine));
const filled = `${wrappedLine}${padding(pad)}`;
const painted = row.highlight ? paintSelectedRow(filled) : filled;
content.push(
`${borderColor(theme.boxRound.vertical)}${wrappedLine}${padding(pad)}${borderColor(theme.boxRound.vertical)}`,
`${borderColor(theme.boxRound.vertical)}${painted}${borderColor(theme.boxRound.vertical)}`,
);
}
}
@@ -472,7 +486,7 @@ export class HookSelectorComponent extends Container {
}
#updateList(renderWidth = this.#lastRenderWidth): void {
const lines: string[] = [];
const rows: SelectorRow[] = [];
const total = this.#filteredOptions.length;
const mdTheme = getMarkdownTheme();
// Compact mode kicks in exactly when the fully-expanded list (all
@@ -500,34 +514,41 @@ export class HookSelectorComponent extends Container {
const filtered = this.#filteredOptions[i];
if (filtered === undefined) continue;
const isSelected = i === this.#selectedIndex;
const isDisabled = this.#isDisabled(filtered.index);
const descMode: number | "full" = compact ? (isSelected ? selectedDescRows : 0) : "full";
lines.push(
...this.#renderOptionLines(
filtered.option,
isSelected,
this.#isDisabled(filtered.index),
mdTheme,
descMode,
renderWidth,
filtered.index,
),
);
// Highlight the whole option block (label + wrapped description rows)
// so the focus band reads as one continuous bar rather than a stripe
// under the label alone. Disabled rows never claim focus even if the
// index momentarily lands on one during initial coercion.
const highlight = isSelected && !isDisabled;
for (const text of this.#renderOptionLines(
filtered.option,
isSelected,
isDisabled,
mdTheme,
descMode,
renderWidth,
filtered.index,
)) {
rows.push({ text, highlight });
}
}
if (total === 0) {
lines.push(theme.fg("dim", " No matching options"));
rows.push({ text: theme.fg("dim", " No matching options"), highlight: false });
}
if (startIndex > 0 || endIndex < total || this.#shouldRenderSearchStatus(renderWidth, mdTheme)) {
lines.push(this.#renderStatusLine(total));
rows.push({ text: this.#renderStatusLine(total), highlight: false });
}
if (this.#outlinedList) {
this.#outlinedList.setLines(lines);
this.#outlinedList.setLines(rows);
return;
}
this.#listContainer?.clear();
for (const line of lines) {
this.#listContainer?.addChild(new Text(line, 1, 0));
for (const row of rows) {
const bgFn = row.highlight ? paintSelectedRow : undefined;
this.#listContainer?.addChild(new Text(row.text, 1, 0, bgFn));
}
}
@@ -298,4 +298,115 @@ describe("HookSelectorComponent", () => {
expect(done).not.toContain(theme.checkbox.checked);
expect(done).not.toContain(theme.checkbox.unchecked);
});
it("paints a selectedBg focus band across the highlighted checkbox row (outlined)", () => {
// Bug #4157: multi-select checkbox picker used only an accent fg to signal
// focus, which vanished on themes where accent ≈ text. The focused row
// must now carry the selectedBg band regardless of accent/text contrast.
const component = new HookSelectorComponent(
"Pick many",
["Apple", "Banana", "Cherry", "Done selecting", "Other (type your own)"],
() => {},
() => {},
{
outline: true,
selectionMarker: "checkbox",
markableCount: 3,
checkedIndices: [],
initialIndex: 0,
},
);
const bgProbe = theme.bg("selectedBg", "|");
const openBg = bgProbe.slice(0, bgProbe.indexOf("|"));
const rendered = component.render(80);
const appleRow = rendered.find(line => Bun.stripANSI(line).includes("Apple"));
const bananaRow = rendered.find(line => Bun.stripANSI(line).includes("Banana"));
const otherRow = rendered.find(line => Bun.stripANSI(line).includes("Other"));
expect(appleRow).toBeDefined();
expect(bananaRow).toBeDefined();
expect(otherRow).toBeDefined();
expect(appleRow).toContain(openBg);
expect(bananaRow).not.toContain(openBg);
expect(otherRow).not.toContain(openBg);
});
it("moves the selectedBg band with the cursor and paints control rows too", () => {
const build = (initialIndex: number) =>
new HookSelectorComponent(
"Pick many",
["Apple", "Banana", "Cherry", "Done selecting", "Other (type your own)"],
() => {},
() => {},
{
outline: true,
selectionMarker: "checkbox",
markableCount: 3,
checkedIndices: [],
initialIndex,
},
);
const bgProbe = theme.bg("selectedBg", "|");
const openBg = bgProbe.slice(0, bgProbe.indexOf("|"));
const bandedLabel = (component: HookSelectorComponent) => {
const rendered = component.render(80);
const banded = rendered.filter(line => line.includes(openBg));
const labels = ["Apple", "Banana", "Cherry", "Done selecting", "Other"];
return labels.find(label => banded.some(line => Bun.stripANSI(line).includes(label)));
};
expect(bandedLabel(build(1))).toBe("Banana");
// Control rows past markableCount ("Other") still receive the focus band
// even though they carry no checkbox marker.
expect(bandedLabel(build(4))).toBe("Other");
});
it("highlights the whole selected block including wrapped description rows", () => {
const component = new HookSelectorComponent(
"Choose",
[
{
label: "Alpha",
description:
"Detailed first-choice explanation long enough to wrap across multiple rendered rows once outlined inside a tight width.",
},
{ label: "Beta", description: "Second." },
],
() => {},
() => {},
{ outline: true, initialIndex: 0 },
);
const bgProbe = theme.bg("selectedBg", "|");
const openBg = bgProbe.slice(0, bgProbe.indexOf("|"));
const rendered = component.render(60);
const bandedRows = rendered.filter(line => line.includes(openBg));
// Label row plus at least one wrapped description continuation, all under
// the Alpha option; Beta must stay unbanded.
expect(bandedRows.length).toBeGreaterThanOrEqual(2);
const bandedContainsBeta = bandedRows.some(line => Bun.stripANSI(line).includes("Beta"));
expect(bandedContainsBeta).toBe(false);
});
it("paints the selectedBg band on the non-outlined plain list too", () => {
const component = new HookSelectorComponent(
"Pick",
["Apple", "Banana", "Cherry"],
() => {},
() => {},
{ initialIndex: 1 },
);
const bgProbe = theme.bg("selectedBg", "|");
const openBg = bgProbe.slice(0, bgProbe.indexOf("|"));
const rendered = component.render(80);
const appleRow = rendered.find(line => Bun.stripANSI(line).includes("Apple"));
const bananaRow = rendered.find(line => Bun.stripANSI(line).includes("Banana"));
const cherryRow = rendered.find(line => Bun.stripANSI(line).includes("Cherry"));
expect(appleRow).toBeDefined();
expect(bananaRow).toBeDefined();
expect(cherryRow).toBeDefined();
expect(bananaRow).toContain(openBg);
expect(appleRow).not.toContain(openBg);
expect(cherryRow).not.toContain(openBg);
});
});