improve model search matching and highlight hits in selectors#6469
improve model search matching and highlight hits in selectors#6469bernaferrari wants to merge 5 commits intoKilo-Org:mainfrom
Conversation
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
|
|
||
| const word = lowerWords[wordIndex] | ||
| const token = tokens[wordIndex] | ||
| const nextPositions = new Set(currentPositions) |
There was a problem hiding this comment.
[SUGGESTION]: Eager Set copy before checking for a match
new Set(currentPositions) is allocated on every recursive call, but when matchedInWord ends up being 0 (no prefix match in the current word), the copy is discarded and the function falls through to line 131 using the original currentPositions. Moving the allocation inside the if (matchedInWord > 0) block would avoid unnecessary copies on non-matching branches.
| const nextPositions = new Set(currentPositions) | |
| let matchedInWord = 0 |
Then build nextPositions only after the while loop confirms matchedInWord > 0.
Code Review SummaryStatus: No New Issues Found | Recommendation: Address existing comments before merge OverviewThis PR replaces the simple All significant issues have already been flagged in existing inline comments (11 active). No additional issues were found in this review pass. Existing Comments Summary
Key items from existing comments to address:
Files Reviewed (6 files)
|
| const positions = nameMatchesByKey.get(key) | ||
|
|
||
| if (positions) { | ||
| result.push({ ...model, matchingPositions: positions }) |
There was a problem hiding this comment.
[WARNING]: Object spread breaks referential identity for SolidJS <For>
Every call to filterModels creates a new object via { ...model, matchingPositions: positions } for name-matched models. SolidJS's <For> uses referential identity to track items — new object identity means the framework treats these as entirely new items, tearing down and recreating DOM nodes on every keystroke rather than updating them in place.
Consider mutating a property on the original object, or using a keyed approach (e.g. <For each={...} fallback={...}> with an explicit key via <Index> or a wrapper), or returning the original model alongside positions in a way that preserves identity (e.g. a parallel Map<model, positions> instead of spreading into a new object).
| } | ||
|
|
||
| private matchAcronym(tokens: WordToken[], query: string, startWordIndex: number): MatchResult | null { | ||
| const lowerWords = tokens.map((token) => token.word.toLowerCase()) |
There was a problem hiding this comment.
[SUGGESTION]: lowerWords is recomputed on every matchAcronym call
tokens.map(token => token.word.toLowerCase()) allocates a new array and lowercases every token word each time matchAcronym is invoked. In the multi-word query path (lines 104-113), this is called once per query word. Consider computing lowerWords once in find() (or caching it alongside the tokens) and passing it in.
| const tokens = this.tokenize(text) | ||
|
|
||
| if (queryWords.length > 1) { | ||
| let startWordIndex = 0 |
There was a problem hiding this comment.
[WARNING]: Multi-word matching is order-dependent and greedy
startWordIndex only advances forward, so query words must appear in the same order as the model name tokens. For example, searching "sonnet claude" will NOT match "Claude Sonnet". Additionally, the greedy forward advancement means the first query word consumes as many tokens as possible, potentially preventing later query words from matching.
This may be intentional (ordered matching), but it could surprise users who expect word-order-independent search. Worth documenting the behavior or adding a test that explicitly asserts this constraint.
bernaferrari
force-pushed
the
chore/port-pr-5726-model-search-highlight
branch
from
f85bc0f to
f819d7b
Compare
|
Stale and I think this was solved differently. Thanks though, feel free to reopen if you think I am wrong |
2 participants
Adapted from #5726
I'm having a hard time doing a few specific tasks with the new plugin locally (like choosing the model). Is there anything I need to do besides clicking run into the extension? Is server wired? Maybe it is using local development server on development, so I need another kilo repo to execute it?
Screen.Recording.2026-02-27.at.18.47.45.mov
Before:
After:
Screen.Recording.2026-02-06.at.13.09.03.mov