Mirror: Better search UX (#5726)#20
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello @jeremylongshore, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly improves the search user experience by introducing visual highlighting of matched characters within search results. By integrating a new Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Review Summary by QodoAdd search result highlighting with character position tracking
WalkthroughsDescription• Add character position tracking to fuzzy search results • Implement visual highlighting of matched characters in search results • Create new HighlightedText component for rendering matched positions • Integrate highlighting into SelectDropdown model search UI Diagramflowchart LR
A["Fzf Search Algorithm"] -->|"Returns positions"| B["FzfResult with positions"]
B -->|"Position data"| C["SelectDropdown Filter"]
C -->|"Positions + Label"| D["HighlightedText Component"]
D -->|"Renders"| E["Highlighted Search Results"]
File Changes1. webview-ui/src/lib/word-boundary-fzf.ts
|
|
Failed to generate code suggestions for PR |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request improves the search user experience by highlighting matched characters in search results within dropdowns. This is achieved by introducing a new HighlightedText component and modifying the Fzf search implementation to return the positions of matched characters. The changes are well-structured. I've suggested a small refactoring in the new HighlightedText component to simplify its logic and improve maintainability.
| for (let i = 0; i < text.length; i++) { | ||
| const isMatch = matchingPositions.has(i) | ||
| const isLastMatch = i > 0 && matchingPositions.has(i - 1) | ||
|
|
||
| if (isMatch !== isLastMatch) { | ||
| // specific transition, push previous chunk | ||
| if (i > lastIndex) { | ||
| const chunk = text.slice(lastIndex, i) | ||
| parts.push( | ||
| isLastMatch ? ( | ||
| <span key={lastIndex} className="font-bold text-vscode-textLink-foreground"> | ||
| {chunk} | ||
| </span> | ||
| ) : ( | ||
| <span key={lastIndex}>{chunk}</span> | ||
| ), | ||
| ) | ||
| } | ||
| lastIndex = i | ||
| } | ||
| } | ||
|
|
||
| // Push trailing chunk | ||
| if (lastIndex < text.length) { | ||
| const chunk = text.slice(lastIndex) | ||
| const isMatch = matchingPositions.has(text.length - 1) | ||
| parts.push( | ||
| isMatch ? ( | ||
| <span key={lastIndex} className="font-bold text-vscode-textLink-foreground"> | ||
| {chunk} | ||
| </span> | ||
| ) : ( | ||
| <span key={lastIndex}>{chunk}</span> | ||
| ), | ||
| ) | ||
| } |
There was a problem hiding this comment.
The logic for splitting the text into highlighted and non-highlighted parts can be simplified. The current implementation uses a for loop and then has a separate block of code to handle the 'trailing chunk'. This can be consolidated into a single loop that iterates up to text.length (inclusive). This change eliminates code duplication and makes the component easier to maintain without changing its behavior.
for (let i = 0; i <= text.length; i++) {
const isMatch = matchingPositions.has(i)
const isLastMatch = i > 0 && matchingPositions.has(i - 1)
if (isMatch !== isLastMatch) {
if (i > lastIndex) {
const chunk = text.slice(lastIndex, i)
parts.push(
isLastMatch ? (
<span key={lastIndex} className="font-bold text-vscode-textLink-foreground">
{chunk}
</span>
) : (
<span key={lastIndex}>{chunk}</span>
),
)
}
lastIndex = i
}
}
Code Review by Qodo
1. HighlightedText missing kilocode_change header
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| import React, { memo } from "react" | ||
| import { cn } from "@/lib/utils" | ||
|
|
There was a problem hiding this comment.
1. highlightedtext missing kilocode_change header 📘 Rule violation ⛯ Reliability
A new file was added under webview-ui/ without the required // kilocode_change - new file marker, increasing the risk of upstream sync/merge conflicts. This violates the requirement to annotate upstream-shared code changes.
Agent Prompt
## Issue description
A new file was added in an upstream-shared path (`webview-ui/`) without the required `// kilocode_change - new file` annotation.
## Issue Context
This repo requires Kilo-specific modifications in upstream-shared code to be clearly marked to minimize merge conflicts during upstream syncs.
## Fix Focus Areas
- webview-ui/src/components/ui/highlighted-text.tsx[1-3]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| // Get fuzzy matching items AND their positions | ||
| const matchingResults = fzfInstance.find(searchValue) | ||
| const matchMap = new Map(matchingResults.map((r) => [r.item.original.value, r.positions])) | ||
|
|
||
| // Get fuzzy matching items - only perform search if we have a search value | ||
| const matchingItems = fzfInstance.find(searchValue).map((result) => result.item.original) | ||
|
|
||
| // Always include separators, shortcuts, and labels | ||
| return options.filter((option) => { | ||
| // Always include separators, shortcuts, and labels, and matched items | ||
| return options.reduce((acc, option) => { | ||
| if ( | ||
| option.type === DropdownOptionType.SEPARATOR || | ||
| option.type === DropdownOptionType.SHORTCUT || | ||
| option.type === DropdownOptionType.LABEL // kilocode_change: include LABEL in filtered results | ||
| ) { | ||
| return true | ||
| acc.push(option) | ||
| return acc | ||
| } | ||
|
|
||
| // Include if it's in the matching items | ||
| return matchingItems.some((item) => item.value === option.value) | ||
| }) | ||
| }, [options, searchValue, fzfInstance, disableSearch]) | ||
| const positions = matchMap.get(option.value) | ||
| if (positions) { | ||
| // Clone option to add matching positions without mutating original | ||
| acc.push({ ...option, matchingPositions: positions }) | ||
| } | ||
|
|
||
| return acc | ||
| }, [] as DropdownOption[]) | ||
| }, [options, searchValue, fzfInstance]) |
There was a problem hiding this comment.
2. Unmarked filteredoptions logic change 📘 Rule violation ⛯ Reliability
The new fuzzy-match/position mapping logic in select-dropdown.tsx is not wrapped/annotated with kilocode_change markers even though it modifies upstream-shared webview-ui/ code. This can cause ambiguity and merge conflicts when syncing from upstream.
Agent Prompt
## Issue description
`webview-ui/src/components/ui/select-dropdown.tsx` contains a substantive logic change that is not clearly annotated with `kilocode_change` markers as required for upstream-shared code.
## Issue Context
This repository uses `kilocode_change` markers to reduce merge conflicts during upstream sync.
## Fix Focus Areas
- webview-ui/src/components/ui/select-dropdown.tsx[137-163]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| // Get fuzzy matching items AND their positions | ||
| const matchingResults = fzfInstance.find(searchValue) | ||
| const matchMap = new Map(matchingResults.map((r) => [r.item.original.value, r.positions])) | ||
|
|
||
| // Get fuzzy matching items - only perform search if we have a search value | ||
| const matchingItems = fzfInstance.find(searchValue).map((result) => result.item.original) | ||
|
|
||
| // Always include separators, shortcuts, and labels | ||
| return options.filter((option) => { | ||
| // Always include separators, shortcuts, and labels, and matched items | ||
| return options.reduce((acc, option) => { | ||
| if ( | ||
| option.type === DropdownOptionType.SEPARATOR || | ||
| option.type === DropdownOptionType.SHORTCUT || | ||
| option.type === DropdownOptionType.LABEL // kilocode_change: include LABEL in filtered results | ||
| ) { | ||
| return true | ||
| acc.push(option) | ||
| return acc | ||
| } | ||
|
|
||
| // Include if it's in the matching items | ||
| return matchingItems.some((item) => item.value === option.value) | ||
| }) | ||
| }, [options, searchValue, fzfInstance, disableSearch]) | ||
| const positions = matchMap.get(option.value) | ||
| if (positions) { | ||
| // Clone option to add matching positions without mutating original | ||
| acc.push({ ...option, matchingPositions: positions }) | ||
| } | ||
|
|
||
| return acc | ||
| }, [] as DropdownOption[]) |
There was a problem hiding this comment.
3. Highlight indices mismatch 🐞 Bug ✓ Correctness
Match positions are computed against searchStr (label+value) but later used to highlight only option.label, so queries that match only the value (or later parts of searchStr) can produce no/incorrect highlighting.
Agent Prompt
## Issue description
`SelectDropdown` searches a combined string (`label + " " + value`) but uses the returned character positions to highlight `option.label` only. If a query matches only the `value` portion (or produces positions outside the label prefix), highlighting will be missing/incorrect.
## Issue Context
- `searchStr` is built from `[option.label, option.value].join(" ")` and used as the `Fzf` selector.
- `Fzf` returns `positions` as indices into the selector text.
- UI highlights only `option.label`.
## Fix Focus Areas
- webview-ui/src/components/ui/select-dropdown.tsx[116-163]
- webview-ui/src/components/ui/select-dropdown.tsx[383-389]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Corrected review counts to reflect honest accounting: 22 substantive reviews with verdicts (10 APPROVE, 8 COMMENT, 4 REQUEST_CHANGES), 53 scaffolded templates. Updated DASHBOARD.md with test evidence, PROGRESS.md with 3 missing reviews (#20-22), and all 75 status.json files with accurate phase data. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Review Summary
What ChangedAdds character-level highlighting to the model search dropdown. When typing a search query, the matching characters in each result are bolded and colored (
Checklist
Minor Observations1.
|
| Check | Result |
|---|---|
| compile | PASS |
| test-extension (ubuntu) | PASS |
| test-extension (windows) | PASS |
| test-webview (ubuntu) | PASS |
| test-webview (windows) | PASS |
| test-cli | PASS |
| build-cli | PASS |
| check-translations | PASS |
| test-jetbrains | PASS |
Code Snippets
The core position-tracking change in matchAcronym:
// Before: returns boolean
private matchAcronym(text: string, query: string): boolean
// After: returns Set<number> | null (positions of matching chars)
private matchAcronym(tokens: { word: string; index: number }[], query: string): Set<number> | nullThe tokenize method maps each word to its original character index in the source string, enabling accurate position reporting:
private tokenize(text: string): { word: string; index: number }[] {
const tokens: { word: string; index: number }[] = []
let currentIndex = 0
const words = text.split(WORD_BOUNDARY_REGEX).filter((w) => w.length > 0)
for (const word of words) {
const index = text.indexOf(word, currentIndex)
if (index !== -1) {
tokens.push({ word, index })
currentIndex = index + word.length
}
}
return tokens
}Verdict
APPROVE. Clean implementation of a useful UX improvement. The author (bernaferrari) has a track record of UI polish contributions. The character-level position tracking is well-integrated into the existing Fzf class without breaking the API contract. All CI passes. The two observations are informational, not blocking.
Review Journal: kilocode Kilo-Org#5726
SummaryAdds fuzzy match character highlighting to the model search dropdown. Matching characters are bolded and colored as users type. Clean implementation that extends the existing First Impressionsbernaferrari is a repeat contributor focused on UI polish. The PR description is honest ("this is totally subjective") and includes before/after screenshots plus a video. The diff is 141+/36- across 4 files, which is modest for a feature PR. What I Looked At
AnalysisPosition tracking approachThe key design decision is storing match positions as a The recursive HighlightedText componentThe component groups consecutive matched/unmatched characters into spans for efficient rendering (avoids one span per character). It uses Dropdown integrationThe One subtlety: the Missing position testsThe existing 50+ test cases for the Fzf class all still pass because they only assert on Verification
Lessons LearnedWhen reviewing search UX changes, verify the highlight approach handles edge cases: empty queries (returns all items with empty positions -- correct), multi-word queries (positions accumulated across all words -- correct), and backtracking (position set cloned at branch points -- correct). Review #29 of 75 | Methodology | Reviewed with Claude Code |
2 participants
Mirror of Kilo-Org#5726 for multi-AI review analysis.