feat: highlight all search matches - #254
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a5f4f2bbba
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
| let query_len = results.query.len() as u16; | ||
| for Match(row, col) in results.matches.iter() { | ||
| self.highlight_cell(*row, *col, query_len, color)?; |
There was a problem hiding this comment.
For queries with many hits, highlight_search_matches invokes highlight_cell once per match, and highlight_cell resolves each row via all_rows_mut().nth(num_row) from the start of the iterator. This makes highlighting roughly proportional to matches × rows, so common terms on long task output can noticeably stall or freeze the TUI during both run_search and clear_search_highlights; applying highlights in a single pass (or using direct row indexing) would avoid this regression.
Useful? React with 👍 / 👎.
| let query_len = results.query.len(); | ||
| let task = self.active_task_mut()?; | ||
| if task.name != results.task { | ||
| self.highlight_search_matches(&results, vt100::Color::Default)?; |
There was a problem hiding this comment.
clear_search_highlights now clears every matched span by writing vt100::Color::Default, but this change also paints every match, so searches over output that already uses non-default ANSI backgrounds will permanently lose those original colors after exiting search. This regression appears whenever the query matches colored regions; the clear path needs to restore each cell's previous bgcolor rather than hardcoding Default.
Useful? React with 👍 / 👎.
fba6c78 to
770704c
Compare
There was a problem hiding this comment.
Pull request overview
This PR updates the TUI search behavior to highlight all matches in the active task’s terminal output (rather than only the current match), and refactors the highlighting logic to operate over a set of match locations.
Changes:
- Added shared search highlight color constant and new helpers to (re)apply or clear highlighting for all matches.
- Refactored highlight logic to accept and process multiple match locations.
- Updated search navigation/exit flows to use the new highlight/clear functions.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| self.highlight_current_search_match(&results, Self::SEARCH_MATCH_BG)?; | ||
| let previous = results.previous(); | ||
| self.highlight_current_search_match(&results, Self::SEARCH_MATCH_BG)?; | ||
| if let Some(Match(row, _)) = previous { |
There was a problem hiding this comment.
previous_search_result calls highlight_current_search_match both before and after moving, but with the same color. This is redundant and can become costly with large scrollback. Either remove the first call or apply distinct styling for previous vs current match.
| fn highlight_search_matches(&mut self, results: &SearchResults, color: vt100::Color) -> anyhow::Result<()> { | ||
| let active_task_name = self.active_task()?.name.clone(); | ||
| if active_task_name != results.task { | ||
| return Ok(()); | ||
| } | ||
| if let Some(Match(row, col)) = results.current() { | ||
| self.highlight_cell(row, col, query_len as u16, false)?; | ||
| let query_len = results.query.len() as u16; | ||
| self.highlight_cell(&results.matches, query_len, color)?; | ||
| Ok(()) |
There was a problem hiding this comment.
query_len is derived from results.query.len() (byte length) but match columns are computed using .chars().count() earlier to handle multibyte characters. For non-ASCII queries, using byte length here will highlight the wrong number of terminal cells. Consider computing the highlight length in character cells instead (e.g., query.chars().count() or a width-aware approach if wide Unicode should be supported).
| fn highlight_cell(&mut self, matches: &Vec<Match>, length: u16, color: vt100::Color) -> anyhow::Result<()> { | ||
| let task = self.active_task_mut()?; |
There was a problem hiding this comment.
highlight_cell takes matches: &Vec<Match> and then clones it internally. Accepting a slice (&[Match]) would be more flexible for callers and avoid forcing temporary Vec allocations at call sites (e.g., when highlighting a single match).
| self.scroll_to_row(row)?; | ||
| self.highlight_search_matches(&search_results, Self::SEARCH_MATCH_BG)?; | ||
| if let Some(m) = search_results.current() { | ||
| self.highlight_cell(&vec![m.clone()], query_len as u16, Self::SEARCH_MATCH_BG)?; |
There was a problem hiding this comment.
run_search highlights all matches and then re-highlights the current match with the same background color. As a result the “current” match is not visually distinguishable, and the extra highlight pass is redundant work. Consider introducing a separate color/style for the current match (and using it here), or remove the second highlight if all matches share the same styling.
| self.highlight_cell(&vec![m.clone()], query_len as u16, Self::SEARCH_MATCH_BG)?; | |
| self.highlight_cell(&vec![m.clone()], query_len as u16, vt100::Color::Indexed(15))?; |
| self.highlight_current_search_match(&results, Self::SEARCH_MATCH_BG)?; | ||
| let next = results.next(); | ||
| self.highlight_current_search_match(&results, Self::SEARCH_MATCH_BG)?; | ||
| if let Some(Match(row, _)) = next { |
There was a problem hiding this comment.
next_search_result calls highlight_current_search_match both before and after advancing, but with the same color both times. This does not change the UI and adds avoidable work (including scanning rows in highlight_cell). Either remove the redundant pre-highlight call, or use different styling to de-emphasize the previous current match and emphasize the new one.
dc6ad48 to
12c6e0b
Compare
12c6e0b to
6a6aee9
Compare
No description provided.