diff --git a/src/ui/tui/__tests__/picker-menu.test.ts b/src/ui/tui/__tests__/picker-menu.test.ts new file mode 100644 index 000000000..c8829651c --- /dev/null +++ b/src/ui/tui/__tests__/picker-menu.test.ts @@ -0,0 +1,47 @@ +import { pruneSelected, selectedValues } from '@ui/tui/primitives/PickerMenu'; + +type Opt = { label: string; value: string; disabled?: boolean }; + +const opts = (...labels: string[]): Opt[] => + labels.map((l) => ({ label: l, value: l })); + +describe('selectedValues', () => { + it('resolves selected indices to values in option order', () => { + const options = opts('a', 'b', 'c'); + expect(selectedValues(options, new Set([2, 0]))).toEqual(['a', 'c']); + }); + + it('drops indices that no longer resolve to an option instead of crashing', () => { + // The multi-select set was built against a 3-item list, then the list + // shrank to 1 before confirm() ran — index 2 now points at nothing. + const options = opts('a'); + expect(() => selectedValues(options, new Set([0, 2]))).not.toThrow(); + expect(selectedValues(options, new Set([0, 2]))).toEqual(['a']); + }); + + it('returns an empty array when nothing resolves', () => { + expect(selectedValues(opts(), new Set([0, 1]))).toEqual([]); + }); +}); + +describe('pruneSelected', () => { + it('removes indices past the end of the list', () => { + const options = opts('a', 'b'); + expect([...pruneSelected(options, new Set([0, 1, 5]))]).toEqual([0, 1]); + }); + + it('removes indices that now point at a disabled option', () => { + const options: Opt[] = [ + { label: 'a', value: 'a' }, + { label: 'b', value: 'b', disabled: true }, + ]; + expect([...pruneSelected(options, new Set([0, 1]))]).toEqual([0]); + }); + + it('returns the same Set reference when nothing was pruned', () => { + const options = opts('a', 'b', 'c'); + const selected = new Set([0, 2]); + // Referential stability lets the caller skip a needless setState/re-render. + expect(pruneSelected(options, selected)).toBe(selected); + }); +}); diff --git a/src/ui/tui/primitives/PickerMenu.tsx b/src/ui/tui/primitives/PickerMenu.tsx index b7a4b541e..823476556 100644 --- a/src/ui/tui/primitives/PickerMenu.tsx +++ b/src/ui/tui/primitives/PickerMenu.tsx @@ -90,6 +90,39 @@ function lastEnabled(options: PickerOption[]): number { return options.length - 1; } +/** + * Drop selected indices that no longer resolve to an enabled option — a list + * that shrank, reordered, or disabled entries while mounted can leave the + * multi-select `selected` set pointing at missing options. Returns the same + * Set reference when nothing changed so callers can skip a needless state + * update (and re-render). + */ +export function pruneSelected( + options: PickerOption[], + selected: Set, +): Set { + const pruned = new Set( + [...selected].filter((i) => i < options.length && !options[i]?.disabled), + ); + return pruned.size === selected.size ? selected : pruned; +} + +/** + * Resolve a multi-select `selected` set to the option values, in option order. + * Defensive: an index whose option vanished before this ran resolves to + * `undefined`; drop it rather than crash reading `.value`. + */ +export function selectedValues( + options: PickerOption[], + selected: Set, +): T[] { + return [...selected] + .sort((a, b) => a - b) + .map((i) => options[i]) + .filter((o): o is PickerOption => Boolean(o)) + .map((o) => o.value); +} + interface PickerMenuProps { message?: string; options: PickerOption[]; @@ -312,20 +345,20 @@ const MultiPickerMenu = ({ const [selected, setSelected] = useState>(new Set()); const rows = Math.ceil(options.length / columns); - // Re-validate focus when the options change while mounted — a list - // that shrinks or disables entries can leave `focused` pointing at a - // missing or disabled option, which would make enter a no-op. + // Re-validate focus AND selection when the options change while mounted — a + // list that shrinks or reorders can leave `focused` pointing at a missing or + // disabled option (making enter a no-op) and leave `selected` holding stale + // indices that no longer resolve to an option (crashing confirm() when it + // reads `.value`). Repair both so the two stay consistent. useEffect(() => { if (focused >= options.length || options[focused]?.disabled) { setFocused(firstEnabled(options)); } + setSelected((prev) => pruneSelected(options, prev)); }, [options, focused]); const confirm = () => { - const values = [...selected] - .sort((a, b) => a - b) - .map((i) => options[i].value); - onSelect(values); + onSelect(selectedValues(options, selected)); }; const bindings: KeyBinding[] = [