refactor(tui): reduce duplication across picker dialogs#2556
Merged
dgageot merged 5 commits intodocker:mainfrom Apr 28, 2026
Merged
Conversation
Reduce duplication across the list-with-filter dialogs (command palette, theme picker, model picker, file picker, working-dir picker) by introducing a shared picker.go with: - pickerKeyMap / defaultPickerKeyMap (replaces commandPaletteKeyMap) - pickerLayout: declarative dimensions - pickerCore: embeddable type owning textInput, scrollview, selected, double-click tracking, sizing, position, mouseListIndex, recordClick, renderEmptyState, renderErrorState - groupedList: builder that tracks line<->item mapping for lists with separators (used by model_picker and theme_picker) - pickerSortKeys / comparePickerSortKeys: shared section/current/default/name comparator Also adds RenderGroupSeparator to base.go (replaces three hand-rolled '── Label ──…' constructions). Result: - command_palette.go: 378 -> 294 lines - theme_picker.go: 549 -> 411 lines - model_picker.go: 873 -> 702 lines - file_picker.go: 419 -> 378 lines No behaviour change. Tests + lint pass. Assisted-By: docker-agent
Builds on the previous picker refactor by:
* Moving the shared picker* sizing constants from model_picker.go into
picker.go where they belong (used by both model and theme pickers).
* Adding three small pickerCore helpers that capture patterns repeated
across every picker:
- updateInput(msg, filter): forward input to the textinput, run the
filter callback, return the textinput cmd
- handleListClick(msg, lineToItem): map the click to an item index,
detect double-click, update selection, return (doubleClicked, changed)
- navigate(delta, num, lineForSelected): bounded selection move that
keeps the new selection visible
Each picker's Update() loop becomes mostly a thin dispatch over these.
* Dropping the cached *groupedList field from theme_picker and model_picker.
buildList(0) is cheap (O(n)), runs at most twice per Update, and removing
the cache eliminates an awkward 'overwritten with empty content' state.
* Replacing the one-shot renderPaddedState with a single renderPlaceholder
used by both renderEmptyState and renderErrorState.
For working_dir_picker.go, the 3 sections (browse/recent/pinned) had triple
copies of selection/scroll/entries logic. Introduced an activeSection()
helper that returns a {entries, *selected, scroll} snapshot, which collapses:
* moveUp / moveDown (3-arm switch -> 4 lines each)
* setSelected (3-arm switch -> 1 line)
* handleSelection (3 bounds-checks -> 1)
* toggleFavorite (3 bounds-checks -> 1, via selectedTogglePath helper)
* mouseYToEntryIndex (3-arm switch -> uses listStartOffset/sectionOverhead)
* cycleSection{Forward,Backward} (two switches -> shared cycleSection(delta))
* renderXList: the empty/error placeholder padding loop (4 copies) is now
a single renderListPlaceholder helper
View() also collapses the three near-identical NewContent.AddX().AddX()
chains into a single builder with a conditional textInput row.
No behaviour change. Tests + lint pass.
Net diff: -112 lines across the 6 picker files.
Assisted-By: docker-agent
Inline the boolCompare helper inside comparePickerSortKeys: the swapped arguments were less clear than spelling out the comparison directly. CommandExecuteMsg has no callers anywhere in the workspace, so remove its declaration from command_palette.go. No behaviour change. Tests + lint pass. Assisted-By: docker-agent
renderEntry computed maxNameLen as maxWidth-20 without a lower bound. When the file picker is rendered in a very narrow terminal (maxWidth < 21), maxNameLen goes <= 0, the unconditional 'len(name) > maxNameLen' branch is taken, and 'name[:maxNameLen-1]' panics with a negative slice index. Clamp maxNameLen to a minimum of 1 so very narrow widths render the file name as a single ellipsis instead of crashing. Pre-existing bug; surfaced during a code-quality review of the picker refactor on this branch. Assisted-By: docker-agent
Two issues found by a careful diff against the pre-refactor baseline: 1. command_palette.filterCommands: the original code always reset d.selected = 0 when the search field was cleared (matching how the file picker behaves). The refactored version preserved the previous selection across the filter call, which caused the cursor to jump to an unrelated command after typing then clearing a query. Restore the original 'reset on empty query, keep otherwise' semantics. 2. working_dir_picker.setSection still inlined the focus-toggle logic that updateSectionFocus already implements. Delegate to updateSectionFocus so the two stay in lockstep. (Pre-existing duplication that the refactor missed.) Assisted-By: docker-agent
trungutt
approved these changes
Apr 27, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Extract shared infrastructure for the TUI list-with-filter picker dialogs (command palette, theme picker, model picker, file picker, working-directory picker) so each dialog focuses on its own filtering / rendering / submission logic instead of repeating the same scrollview/textinput/mouse/keymap plumbing.
Why
The five picker dialogs each carried a near-identical copy of:
textInput+scrollview+selected+ click-tracking fieldscommandPaletteKeyMap(private to one file but used by all five)dialogSize()/Position()/SetSize()Y → item indexmath with double-click detectionmodel_pickerandtheme_picker: section/group separator rendering plus theline ↔ itembookkeeping needed for mouse hit-testing and selection scrollingmodel_pickerandtheme_picker: the same priority/current/default/name sort comparatorBug fixes and improvements to any of these had to be made in 4-5 places.
How
A new
pkg/tui/dialog/picker.gointroduces:pickerKeyMap/defaultPickerKeyMap()commandPaletteKeyMappickerLayoutpickerCoregroupedListline ↔ itemmapping for lists with separators/headers (used by model and theme pickers)pickerSortKeys/comparePickerSortKeysPlus three small `pickerCore` helpers that absorb patterns repeated across every picker:
updateInput(msg, filter)— feed the textInput, run the filter, return the cmdhandleListClick(msg, lineToItem)— mouse hit-test + selection update + double-click detection in one callnavigate(delta, num, lineForSelected)— bounded selection move withEnsureLineVisible`base.go` gains `RenderGroupSeparator(label, contentWidth)` to replace three hand-rolled `"── Label ──…"` constructions.
For `working_dir_picker.go`, an `activeSection()` snapshot returning `{entries, selected, scroll}` collapses what used to be 3-arm switches in `moveUp` / `moveDown` / `setSelected` / `handleSelection` / `toggleFavorite` / `mouseYToEntryIndex` / `cycleSection`.
Per-dialog impact
command_palette.gotheme_picker.gomodel_picker.gofile_picker.goworking_dir_picker.gopicker.go(NEW)Net diff: -275 lines, and each dialog now reads as orchestration over the shared helpers.
No behaviour change
Validation
Commits
```
09beb18 fix(tui/dialog): preserve original behaviour after picker refactor
f246707 fix(tui/dialog): avoid negative slice index when truncating file names
199a80c refactor(tui/dialog): inline boolCompare, drop unused CommandExecuteMsg
0146793 refactor(tui): simplify picker dialogs around shared helpers
6ba31b4 refactor(tui): extract shared picker infrastructure
```