-
Notifications
You must be signed in to change notification settings - Fork 3.1k
fix(vscode): optimize model selector search and auto-jump to active match #12810
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
6c31e56
ca14e42
f9e0119
ac6710c
bd2aedb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| "kilo-code": patch | ||
| --- | ||
|
|
||
| Update model search results instantly and keep the active match visible while typing. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -159,7 +159,6 @@ export const ModelSelectorBase: Component<ModelSelectorBaseProps> = (props) => { | |
| const expanded = vscode.getModelSelectorExpanded | ||
| const setExpanded = vscode.setModelSelectorExpanded | ||
| const [search, setSearch] = createSignal("") | ||
| const [debouncedSearch, setDebouncedSearch] = createSignal("") | ||
| const [selectedKey, setSelectedKey] = createSignal(CLEAR_KEY) | ||
| const [browsing, setBrowsing] = createSignal(false) | ||
| const [navigating, setNavigating] = createSignal(false) | ||
|
|
@@ -181,6 +180,10 @@ export const ModelSelectorBase: Component<ModelSelectorBaseProps> = (props) => { | |
| let listRef: HTMLDivElement | undefined | ||
| let bodyRef: HTMLDivElement | undefined | ||
| let previewTimer: ReturnType<typeof setTimeout> | undefined | ||
| let scrollFrame: number | undefined | ||
| let pointerX: number | undefined | ||
| let pointerY: number | undefined | ||
| let previousSearch: string | undefined | ||
| const [virtualizer, setVirtualizer] = createSignal<VirtualizerHandle>() | ||
| const [pointer, setPointer] = createSignal(true) | ||
|
|
||
|
|
@@ -223,16 +226,9 @@ export const ModelSelectorBase: Component<ModelSelectorBaseProps> = (props) => { | |
| const hasProviders = () => visibleModels().length > 0 | ||
| const canOpen = () => hasProviders() || ((props.allowClear ?? false) && !!props.value) | ||
|
|
||
| // Debounce search input to avoid re-filtering on every keystroke | ||
| createEffect(() => { | ||
| const q = search() | ||
| const t = setTimeout(() => setDebouncedSearch(q), 250) | ||
| onCleanup(() => clearTimeout(t)) | ||
| }) | ||
|
|
||
| // Flat filtered list for keyboard navigation | ||
| const filtered = createMemo(() => { | ||
| const q = debouncedSearch().trim() | ||
| const q = search().trim() | ||
| if (!q) { | ||
| return visibleModels() | ||
| } | ||
|
|
@@ -251,7 +247,7 @@ export const ModelSelectorBase: Component<ModelSelectorBaseProps> = (props) => { | |
|
|
||
| const favoriteModels = createMemo(() => { | ||
| if (props.favorites === false) return [] | ||
| if (!session || debouncedSearch()) return [] | ||
| if (!session || search()) return [] | ||
| const map = new Map(visibleModels().map((m) => [modelKey(m.providerID, m.id), m])) | ||
| const list = session | ||
| .favoriteModels() | ||
|
|
@@ -400,7 +396,7 @@ export const ModelSelectorBase: Component<ModelSelectorBaseProps> = (props) => { | |
| if (!m) return props.allowClear ? CLEAR_KEY : defaultKey() | ||
| const key = modelKey(m.providerID, m.id) | ||
| const favorite = favoriteKey(m) | ||
| if (!debouncedSearch() && favoriteKeys().has(key) && rowMap().has(favorite)) return favorite | ||
| if (!search() && favoriteKeys().has(key) && rowMap().has(favorite)) return favorite | ||
| return canonicalKey(m) | ||
| } | ||
| const chosen = (row: ModelRow) => { | ||
|
|
@@ -453,7 +449,10 @@ export const ModelSelectorBase: Component<ModelSelectorBaseProps> = (props) => { | |
| // which would cause star/unstar to reset selection mid-interaction. | ||
| // Falls back to defaultKey when the active model is filtered out. | ||
| createEffect(() => { | ||
| const query = search() | ||
| const list = filtered() | ||
| const searchChanged = query !== previousSearch | ||
| previousSearch = query | ||
| untrack(() => { | ||
| const active = activeModel() | ||
| const canon = active ? canonicalKey(active) : null | ||
|
|
@@ -468,10 +467,16 @@ export const ModelSelectorBase: Component<ModelSelectorBaseProps> = (props) => { | |
| ? CLEAR_KEY | ||
| : defaultKey() | ||
| setSelectedKey(next) | ||
| setBrowsing(!!debouncedSearch() && nodeMap().has(next)) | ||
| setBrowsing(!!search() && nodeMap().has(next)) | ||
| setNavigating(false) | ||
| setPreActiveKey(next) | ||
| setPreviewKey(next) | ||
| if (!open() || !searchChanged) return | ||
| if (scrollFrame !== undefined) cancelAnimationFrame(scrollFrame) | ||
| scrollFrame = requestAnimationFrame(() => { | ||
| scrollFrame = undefined | ||
| scrollRow(next, "nearest") | ||
| }) | ||
| }) | ||
| }) | ||
|
|
||
|
|
@@ -500,8 +505,9 @@ export const ModelSelectorBase: Component<ModelSelectorBaseProps> = (props) => { | |
| setBrowsing(false) | ||
| setNavigating(false) | ||
| setSearch("") | ||
| setDebouncedSearch("") | ||
| clearTimeout(previewTimer) | ||
| if (scrollFrame !== undefined) cancelAnimationFrame(scrollFrame) | ||
| scrollFrame = undefined | ||
| }) | ||
|
|
||
| // Register before the popover mounts so programmatic slash-command opens | ||
|
|
@@ -519,6 +525,7 @@ export const ModelSelectorBase: Component<ModelSelectorBaseProps> = (props) => { | |
| window.removeEventListener("openModelPicker", onTrigger) | ||
| window.removeEventListener("keydown", onEscape, true) | ||
| clearTimeout(previewTimer) | ||
| if (scrollFrame !== undefined) cancelAnimationFrame(scrollFrame) | ||
| }) | ||
|
|
||
| function pick(model: EnrichedModel) { | ||
|
|
@@ -552,6 +559,20 @@ export const ModelSelectorBase: Component<ModelSelectorBaseProps> = (props) => { | |
| previewTimer = setTimeout(() => setPreviewKey(key), 200) | ||
| } | ||
|
|
||
| function pointerMove(e: MouseEvent) { | ||
| const target = e.target | ||
| if (!(target instanceof Element)) return | ||
| const item = target.closest<HTMLElement>('[role="treeitem"][data-key]') | ||
| const key = item?.dataset.key | ||
| if (!key) return | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. SUGGESTION: Pointer coordinates are only recorded when the event lands on a Now that the handler is delegated to the whole list, it receives Concrete case: the cursor sits over a row's star button while Recording the position unconditionally before the function pointerMove(e: MouseEvent) {
const moved = pointerX !== undefined && pointerY !== undefined && (e.clientX !== pointerX || e.clientY !== pointerY)
pointerX = e.clientX
pointerY = e.clientY
const target = e.target
if (!moved || !(target instanceof Element)) return
const key = target.closest<HTMLElement>('[role="treeitem"][data-key]')?.dataset.key
if (!key) return
setPointer(true)
setSelectedKey(key)
}Reply with |
||
| const moved = pointerX !== undefined && pointerY !== undefined && (e.clientX !== pointerX || e.clientY !== pointerY) | ||
| pointerX = e.clientX | ||
| pointerY = e.clientY | ||
| if (!moved) return | ||
| setPointer(true) | ||
| setSelectedKey(key) | ||
| } | ||
|
|
||
| function scrollRow(key: string | null | undefined, block: ScrollLogicalPosition = "nearest") { | ||
| if (!key) return | ||
| const idx = nodeIndex().get(key) | ||
|
|
@@ -808,6 +829,7 @@ export const ModelSelectorBase: Component<ModelSelectorBaseProps> = (props) => { | |
| placeholder={language.t("dialog.model.search.placeholder")} | ||
| value={search()} | ||
| onInput={(e) => { | ||
| setPointer(false) | ||
| setBrowsing(false) | ||
| setNavigating(false) | ||
| setSearch(e.currentTarget.value) | ||
|
|
@@ -846,7 +868,14 @@ export const ModelSelectorBase: Component<ModelSelectorBaseProps> = (props) => { | |
| </Tooltip> | ||
| </div> | ||
|
|
||
| <div id={listID} class="model-selector-list" role="tree" aria-label={label()} ref={listRef}> | ||
| <div | ||
| id={listID} | ||
| class="model-selector-list" | ||
| role="tree" | ||
| aria-label={label()} | ||
| ref={listRef} | ||
| onMouseMove={pointerMove} | ||
| > | ||
| <Show when={groups().length === 0}> | ||
| <div class="model-selector-empty" role="status" aria-live="polite"> | ||
| {language.t("dialog.model.empty")} | ||
|
|
@@ -871,16 +900,13 @@ export const ModelSelectorBase: Component<ModelSelectorBaseProps> = (props) => { | |
| return ( | ||
| <div | ||
| id={optionID(key)} | ||
| data-key={key} | ||
| class={`model-selector-group-label${props.allowClear || group.key !== groups()[0]?.key ? " model-selector-group-label--divided" : ""}${isSelected(key) ? " selected" : ""}${isSelected(key) && !pointer() ? " keyboard-focused" : ""}`} | ||
| role="treeitem" | ||
| aria-level={1} | ||
| aria-expanded={shown()} | ||
| onMouseDown={(e) => e.preventDefault()} | ||
| onClick={() => toggleGroup(group.key)} | ||
| onMouseMove={() => setPointer(true)} | ||
| onMouseEnter={() => { | ||
| if (pointer()) setSelectedKey(key) | ||
| }} | ||
| > | ||
| <svg | ||
| class={`model-selector-group-chevron${shown() ? "" : " model-selector-group-chevron--collapsed"}`} | ||
|
|
@@ -893,7 +919,7 @@ export const ModelSelectorBase: Component<ModelSelectorBaseProps> = (props) => { | |
| <path d="M4 6l4 5 4-5H4z" /> | ||
| </svg> | ||
| <span>{group.label}</span> | ||
| <Show when={!shown() && !!debouncedSearch()}> | ||
| <Show when={!shown() && !!search()}> | ||
| <span class="model-selector-group-match-dot" aria-hidden="true" /> | ||
| </Show> | ||
| </div> | ||
|
|
@@ -906,15 +932,12 @@ export const ModelSelectorBase: Component<ModelSelectorBaseProps> = (props) => { | |
| return ( | ||
| <div | ||
| id={optionID(CLEAR_KEY)} | ||
| data-key={CLEAR_KEY} | ||
| class={`model-selector-item${isSelected(CLEAR_KEY) && !pointer() ? " keyboard-focused" : ""}${isSelected(CLEAR_KEY) ? " selected" : ""}${!props.value?.providerID ? " active" : ""}`} | ||
| role="treeitem" | ||
| aria-level={1} | ||
| aria-selected={!props.value?.providerID} | ||
| onClick={() => pickClear()} | ||
| onMouseMove={() => setPointer(true)} | ||
| onMouseEnter={() => { | ||
| if (pointer()) setSelectedKey(CLEAR_KEY) | ||
| }} | ||
| > | ||
| <span class="model-selector-item-name" style={{ "font-style": "italic", opacity: 0.7 }}> | ||
| {props.clearLabel ?? language.t("dialog.model.notSet")} | ||
|
|
@@ -939,6 +962,7 @@ export const ModelSelectorBase: Component<ModelSelectorBaseProps> = (props) => { | |
| > | ||
| <div | ||
| id={optionID(row.key)} | ||
| data-key={row.key} | ||
| class={`model-selector-item${(hovered() && !pointer()) || preActive() ? " keyboard-focused" : ""}${hovered() || preActive() ? " selected" : ""}${chosen(row) ? " active" : ""}`} | ||
| role="treeitem" | ||
| aria-level={2} | ||
|
|
@@ -955,10 +979,6 @@ export const ModelSelectorBase: Component<ModelSelectorBaseProps> = (props) => { | |
| onDblClick={() => { | ||
| if (expanded()) selectRow(row) | ||
| }} | ||
| onMouseMove={() => setPointer(true)} | ||
| onMouseEnter={() => { | ||
| if (pointer()) setSelectedKey(row.key) | ||
| }} | ||
| > | ||
| <div class="model-selector-item-left"> | ||
| <span class="model-selector-item-name"> | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SUGGESTION: The auto-scroll is keyed to
filtered(), not to a search changeThis effect re-runs on every
filtered()recompute, andfiltered()depends onvisibleModels(), which returns a fresh array whenevermodels()orconnected()changes (and whenever a parent passes a newprops.modelsidentity). So a background provider/model refresh while the popover is open will now also scroll the list back to the active/first row, throwing away wherever the user had scrolled to — previously that recompute only reset selection, with no visible jump.Since the intent is "latest search only", consider keeping the previous search text in a closure variable and scheduling the frame only when
search()actually changed.Reply with
@kilocode-bot fix itto have Kilo Code address this issue.