Skip to content

feat(tui): filter the model picker by typing, and stop it leaving stray glyphs - #68

Open
sosidudku1 wants to merge 3 commits into
feat/full-provider-catalogsfrom
feat/model-picker-search
Open

feat(tui): filter the model picker by typing, and stop it leaving stray glyphs#68
sosidudku1 wants to merge 3 commits into
feat/full-provider-catalogsfrom
feat/model-picker-search

Conversation

@sosidudku1

@sosidudku1 sosidudku1 commented Aug 7, 2026

Copy link
Copy Markdown
Collaborator

Companion to #67, which raises the catalogs to 337 and 305 models. Based on that branch; merge #67 first and this retargets to main.

Filtering

Typing in the picker filters the list by substring, case-insensitively. Backspace trims the query, arrows and Enter operate on the filtered rows, and the cursor resets to the top on every keystroke so it can never point at a row that was just filtered away. Backspace with an empty query is a no-op, so it cannot reset the cursor. Ctrl and Meta combos are not typed into the filter; the modal swallows them like every other key, and Ctrl+C keeps working only because handleAppKey runs before the modal handler ever sees the key.

The counter reads 3/12 of 337 when a filter is active, and no match on an empty result set.

Two repaint bugs

Both were visible in a screenshot of the panel before this change: model rows rendering as toolss and toolsls, and the header reading Press ←/→ to switch modeternal llama.cpp.

  1. The box changed height between renders. Ink repaints a frame by overwriting the previous one line for line, so a shorter frame leaves the tail of the taller one on screen. The window is now fixed height and pads with blank rows when the result set is shorter.

  2. Rows were keyed by model id. Ids repeat across a re-filter, and duplicate keys made Ink reuse the previous row's text, which left half-erased names behind (reported as "the last letter of the model sticks"). Rows are now keyed by slot.

The window-start computation is also clamped defensively for lists shorter than the window. The previous expression already produced correct slices in that case; the clamp just makes the invariant explicit instead of implicit.

Testing

12 new tests: query narrows the rows and resets the cursor, case-insensitive substring matching, empty query shows everything, no-match yields an empty list, printable keys dispatch a query update, backspace trims, backspace on an empty query is a no-op that keeps the cursor put, arrows wrap within the filtered list, Enter selects from the filtered list, Enter on an empty result does nothing, Esc closes with a query active, Ctrl combos are not typed.

src/tui/providers: 39 passing. tsc clean. Full suite shows the same pre-existing failures as main.

sosidudku1 and others added 2 commits August 7, 2026 14:03
…ay glyphs

Follow-up to the full catalogs branch: with 300+ models per provider the
picker needs a filter, and rendering it exposed two repaint bugs.

- typing filters the list by substring, case-insensitively; backspace
  trims the query, arrows and Enter operate on the filtered rows, and the
  cursor resets to the top on every keystroke so it can never point at a
  row that was filtered away. Ctrl and Meta combos are passed through
  instead of being typed
- the box now renders a fixed WINDOW of rows, padding with blanks when
  the result set is shorter. Ink repaints frame over frame, so a box that
  shrinks between renders leaves the tail of the taller frame behind:
  that is where the stray characters and the header bleed came from
- rows are keyed by slot rather than by model id. Ids repeat across a
  re-filter, and duplicate keys made Ink reuse the previous row's text,
  leaving half-erased model names on screen
- the window start is clamped for short lists, which previously produced
  a negative offset and an off-by-one slice
- the counter shows the filtered total against the catalog size
  (3/12 of 337) and reads "no match" on an empty result

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Backspace with nothing typed used to dispatch a query update anyway,
which reset the cursor to the top of the list. Swallow the key instead.
Also drop a dead ternary in the modal renderer and correct the comment
about ctrl/meta combos: the modal swallows them, and Ctrl+C survives
only because handleAppKey runs first.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

@Ooooze Ooooze left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Read through the diff against the current tree. The state design is the right shape: query on the picker plus a derived filteredPickerModels selector means the filter can never drift out of sync with models, and resetting the cursor on every keystroke removes the whole class of "cursor points at a row that was just filtered away" bugs. The key routing also holds up — chatModelPicker is already part of llmTabBusy (app-key-bindings.ts), /model switches to the LLM tab so editorFocus is false and typed characters cannot leak into the chat editor, and the Ctrl+C note is accurate since handleAppKey runs first in tui-app.tsx. Typing is gated on status === "ready", so a fetch settling after the user typed cannot desync the cursor. Four things worth a look before this lands.

1. The React-key explanation looks wrong. Model ids are unique within a single list, and a key repeating across renders is exactly what keys are for — that is not a duplicate-key situation. Switching to key={row-${i}} is harmless here (the rows are plain Text), but it is an index key, and the stated cause does not explain the artifact. The more likely culprit is Ink falling back to non-erasing rendering once the frame exceeds the terminal height. Could you confirm the actual cause, or drop that justification from the commit message so the next reader is not misled?

2. The fixed-height window does not cover every transition. The loading and error branches in llm-panel-modals.tsx still render a different number of lines, so the loading -> ready height change remains. And a 12-row window plus the filter line makes the modal taller, which matters most on the short terminals where the original artifact showed up.

3. The wizard picker now behaves differently for the same keystroke. CompatChatModelStep in providers-wizard.tsx (PICK_WINDOW = 12) treats typing as "enter an id by hand", while this modal now treats it as "filter". With 337-model catalogs the wizard has the same usability problem, and two visually identical pickers reacting differently to the same key is a trap. Follow-up is fine, but it should not be forgotten.

4. The printable-key guard is too broad. input && input.length > 0 && !key.ctrl && !key.meta also admits Tab (\t) and, depending on what Ink puts in input, left/right arrows and page-up/down — all typed into the filter as junk. Worth excluding special keys explicitly, or checking that the character is actually printable.

Minor: trim() in the selector means typed spaces show up in the filter line but do not affect the result, and once a filter is active there is no way back to the currently active model (the cursor only lands on it at load time).

Nothing blocking. 1 and 4 are small; 2 and 3 can be a follow-up.

Follow-ups to the maintainer review of the picker filter:

- the fixed-height treatment now covers every status branch: loading
  and error pad their body with blanks to the same line count as the
  ready list, so loading -> ready -> error transitions never change
  the frame height. Model rows, the filter line, the error line and
  the footer are truncate-end so wrapping cannot change it either
- the list window is sized from the tab row budget (3 to 12 rows), so
  short terminals get a smaller, still constant, window
- the printable-key guard is explicit: Tab, Enter, Esc, arrows,
  PgUp/PgDn and backspace/delete are excluded by key flag, and C0
  control characters plus DEL by code point, before anything is
  appended to the filter query
- the React-key justification was wrong (ids are unique within a
  render) and is replaced with what we verified in Ink 7: repaint
  erases the previous frame's line count and rewrites (log-update),
  and a frame taller than the viewport falls back to a full
  clear-terminal rewrite (shouldClearTerminalForFrame in ink); the
  artifact was observed with the unwindowed 337-row catalog, i.e. in
  exactly that non-erasing regime
- tests: equal frame heights across all branches at two terminal
  budgets, height stability under a narrowing filter, the key guard
  for Tab/PgDn/arrows/control bytes, and the trim-on-filter but
  display-as-typed query behavior

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@sosidudku1

Copy link
Copy Markdown
Collaborator Author

Thanks for the careful read, all four points addressed in the latest commit.

  1. You were right on both counts. Ids are unique within a render, so the key explanation was wrong and is gone. We read through Ink 7's renderer to confirm the real mechanism: the standard log-update path repaints by erasing the previous frame's line count and rewriting, and shouldClearTerminalForFrame in ink.js switches to a full clear-terminal rewrite once a frame outgrows the viewport, since eraseLines cannot reach rows that scrolled past the top. The artifact showed up exactly when the unwindowed 337-row catalog pushed frames into that regime. The code comment now states what we verified and no more.
  2. Loading and error now pad to the same line count as the ready branch, so no transition changes the frame height, and long ids/errors are truncate-end so wrapping cannot either. The window is also sized from the tab's row budget now (3 to 12 rows), so short terminals get a smaller but still constant window. Covered by height-equality tests at two budgets.
  3. Agreed on the wizard picker. A typeable filter for it is already written and being tested locally; it lands in the next series of model-UX PRs rather than widening this one.
  4. Done. The guard now excludes Tab, Enter, Esc, arrows, PgUp/PgDn and backspace/delete by flag, and rejects C0 controls and DEL by code point before anything reaches the query. Tests cover Tab, PgDn (including a raw [6~ input), arrows and control bytes. Also added a test pinning the trim behavior: whitespace stays visible in the filter line but is ignored when matching.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants