feat: add vim-style search to log viewer panel - #600
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds an incremental, Vim-style in-tab search feature to the log viewer. It introduces a search module with match-finding logic, extends ChangesLog search feature
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant LogModel
participant SearchData
participant Render
participant Terminal
User->>LogModel: press "/"
LogModel->>LogModel: edit_search (start editing)
User->>LogModel: type characters
LogModel->>SearchData: refresh(content)
SearchData-->>LogModel: matches
LogModel->>LogModel: scroll_to_current_match
LogModel->>Render: render(search state, scroll_mode)
Render->>Render: highlighted_content / wrapped_offset
Render->>Terminal: set search_cursor_position
User->>LogModel: press Enter
LogModel->>LogModel: confirm_search (Editing -> Applied)
User->>LogModel: press "n"/"N"
LogModel->>SearchData: cycle(forward)
SearchData-->>LogModel: current_match
LogModel->>Render: render(updated scroll_mode)
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/app/model/logs.rs (1)
40-52:⚠️ Potential issue | 🟠 MajorKeep search scroll state in one coordinate space.
next_match()/prev_match()store a source-line index inScrollMode::SourceLine,render()converts that to a wrapped offset, and the recompute paths updatecurrent_match/match_lineswithout rebasingscroll_mode. That lets the viewport drift from the selected match: on wrapped logs the firstj/kafter/,n, orNcan jump to the wrong visual row, and after a refresh or task-try switch the footer can show[1/n]while the old line is still in view. Please normalize the active search target whenever the selected match changes, or keepScrollModeentirely in wrapped-line coordinates.Also applies to: 123-135, 146-157, 184-218, 563-569
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/model/logs.rs` around lines 40 - 52, Scroll state mixes source-line and wrapped-line coordinates causing viewport drift; normalize the scroll coordinate when the selected match changes by rebasing ScrollMode::SourceLine to the corresponding wrapped-line offset immediately after updating current_match/match_lines. Update next_match() and prev_match() (and any recompute paths that assign current_match) to call a helper that maps a source line to the wrapped-line offset used by render() and then set ScrollMode to that wrapped offset (e.g., switch to ScrollMode::Manual { position: wrapped_offset }) so ScrollMode stays in the same coordinate space as render and footer rendering.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/app/model/logs.rs`:
- Around line 405-443: highlight_line uses byte offsets from lowercased string
(from lower_line.match_indices) to slice the original string, which is unsafe
for Unicode where lowercase can change byte length; fix by performing the search
on character sequences and mapping character indices back to byte offsets in the
original string before slicing. Concretely, in highlight_line: build a Vec<char>
(or iterator) for both the original line and its lowercase form, search for
occurrences of the query as a sequence of lowercase chars against the lowercased
char vector (or use a Unicode-aware casefold per char), compute the starting and
ending byte offsets by summing the UTF-8 byte lengths of chars up to those char
indices, then use those byte offsets to create Span::styled slices from the
original line; keep existing use of SEARCH_CURRENT_LINE_STYLE and DEFAULT_STYLE
and return Line::from(spans) as before.
---
Outside diff comments:
In `@src/app/model/logs.rs`:
- Around line 40-52: Scroll state mixes source-line and wrapped-line coordinates
causing viewport drift; normalize the scroll coordinate when the selected match
changes by rebasing ScrollMode::SourceLine to the corresponding wrapped-line
offset immediately after updating current_match/match_lines. Update next_match()
and prev_match() (and any recompute paths that assign current_match) to call a
helper that maps a source line to the wrapped-line offset used by render() and
then set ScrollMode to that wrapped offset (e.g., switch to ScrollMode::Manual {
position: wrapped_offset }) so ScrollMode stays in the same coordinate space as
render and footer rendering.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 654700f9-9392-402f-8681-c32cdf5809f4
📒 Files selected for processing (3)
src/app/model/logs.rssrc/ui.rssrc/ui/theme.rs
aweirdmaan
left a comment
aweirdmaan
left a comment
There was a problem hiding this comment.
Code Review — feature/log-search
Clean feature addition. State machine for SearchMode is well-designed and tests are solid. Issues below, ordered by severity.
Bug: ScrollMode::position() broken after search jump
After n/N, scroll_mode is SourceLine { line }. Pressing j/k calls position() which returns the raw source-line index, but the caller treats it as a wrapped-line offset to create Manual { position }. This means scrolling after a search jump computes the wrong position for any content with long wrapped lines.
Fix: Normalize SourceLine to Manual (with correct wrapped offset) after the render pass, or handle the conversion inside position().
Bug: highlight_line can panic on non-ASCII input
match_indices runs on lower_line/lower_query, but the resulting byte offsets are used to slice the original line via start..start + query.len(). to_lowercase() can change byte lengths for certain characters, so offsets from the lowered string may not be valid positions in the original, causing incorrect slicing or a panic.
Fix: Match on the original string with a case-insensitive approach, or use character-based indexing.
Wrapped-line calculation doesn't match Paragraph::wrap(Wrap { trim: true })
source_line_to_wrapped and total_wrapped_lines compute wrapping as ceil(byte_len / width). But Paragraph with Wrap { trim: true } uses Unicode grapheme width and trims leading whitespace. For log lines with leading spaces, tabs, or multi-byte characters (stack traces, JSON), the scroll offset will diverge from actual rendering.
Fix: Switch to Wrap { trim: false } (logs shouldn't be trimmed) and use character-width-based calculations, or match the renderer's width logic.
DRY: Repeated self.current % self.all.len().max(1)
Appears 5+ times. Extract a helper:
fn current_log(&self) -> Option<&Log> {
self.all.get(self.current % self.all.len().max(1))
}DRY: Duplicated search recomputation
update_logs and recompute_search_for_current_tab both destructure SearchMode::Active, call compute_matches, and update fields. They differ only in clamping vs resetting current_match. Extract a shared refresh_matches(&mut self, reset_position: bool).
DRY: Duplicated per-line wrapping formula
Both source_line_to_wrapped and total_wrapped_lines contain the identical:
if len == 0 { 1 } else { (len + wrap_width - 1) / wrap_width }Extract a wrapped_line_count(byte_len: usize, wrap_width: usize) -> usize helper.
Minor: Stale comment at line 264
// Clear pending 'g' on any key... now sits above the search-input block, not above the pending_g clearing logic at line 288.
Minor: Redundant !q.is_empty() in build_content
SearchMode::query() only returns Some for Active, and confirm_search transitions empty queries to Inactive. The query is guaranteed non-empty, making this guard dead code.
Minor: search_cursor_position leaks render detail into model
This pub field is set during render() and read in ui.rs, coupling rendering output into model state. Consider a dedicated method or side channel instead.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/app/model/logs.rs`:
- Around line 500-527: The wrap and cursor calculations currently use byte
lengths (line.len(), query.len()) which break for multi-byte/wide characters;
update wrapped_line_count, source_line_to_wrapped, and total_wrapped_lines to
compute widths using unicode-width (use UnicodeWidthStr::width or equivalent on
the &str/segments) instead of byte counts, and change any search prompt cursor
math (the code handling the search cursor positioning referenced around the
search prompt logic) to use display-width values for query and prefix lengths;
add unit tests exercising accented, emoji and CJK strings to validate wrapped
counts and cursor alignment.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 91099f94-7145-41cf-b8da-0023605c20ee
📒 Files selected for processing (1)
src/app/model/logs.rs
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/app/model/logs.rs (1)
600-610: Minor mutation during render.The
SourceLinetoManualconversion mutatesself.scroll_modeinsiderender(). While this is intentional for coordinate normalization, side effects in render can be surprising. The current approach works correctly, but consider documenting this behavior inline.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/model/logs.rs` around lines 600 - 610, The render() function currently mutates self.scroll_mode when handling ScrollMode::SourceLine by converting to ScrollMode::Manual using source_line_to_wrapped and assigning self.scroll_mode = ScrollMode::Manual { position: pos }; add a concise inline comment at that location (near the match arm for ScrollMode::SourceLine) explaining that this mutation is intentional: it normalizes coordinates from SourceLine to Manual so subsequent navigation (j/k) operates in wrapped-offset space, and that the side effect is expected behavior; reference ScrollMode::SourceLine, ScrollMode::Manual, self.scroll_mode, and source_line_to_wrapped in the comment for clarity.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/app/model/logs.rs`:
- Around line 600-610: The render() function currently mutates self.scroll_mode
when handling ScrollMode::SourceLine by converting to ScrollMode::Manual using
source_line_to_wrapped and assigning self.scroll_mode = ScrollMode::Manual {
position: pos }; add a concise inline comment at that location (near the match
arm for ScrollMode::SourceLine) explaining that this mutation is intentional: it
normalizes coordinates from SourceLine to Manual so subsequent navigation (j/k)
operates in wrapped-offset space, and that the side effect is expected behavior;
reference ScrollMode::SourceLine, ScrollMode::Manual, self.scroll_mode, and
source_line_to_wrapped in the comment for clarity.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: b15ab804-6eed-46e6-8940-84aa1ed0b4bb
📒 Files selected for processing (1)
src/app/model/logs.rs
|
@jvanbuel, the pr is functionally ready for you to review. PTAL |
f80af2a to
1406876
Compare
|
@jvanbuel could please approve the workload? |
|
@shaikhamaan thank you for your interest in Flowrs! Due to the size of the PR, I would like to ask you to first create an issue, so that the feature request can at least be discussed a little bit more in depth. I'm fully onboard with using AI to speed up development, and to get to a working example. But when running Flowrs on your fork, I did not quite agree with some decisions, which is why I'd like to flesh this out a bit more in an issue, before I spend time reviewing this PR in detail. Thanks! |
|
@jvanbuel Apologies for replying late to the conversation, I couldn't make the time earlier. I will create an issue and we can discuss the proposal for it there. I was prototyping and wanted that feature for a use-case hence raised the pr. |
Press / in the Logs panel to open a search input box (styled like the filter boxes of the table panels); matches are highlighted live while typing (incsearch), Enter confirms the query, and n/N cycle through individual match occurrences with wrap-around. Esc cancels the input or clears an applied search's highlights, like :noh. Matches are stored as byte ranges (line, start, end), so highlighting is plain slicing and n/N navigate per occurrence rather than per line. Matching is ASCII case-insensitive and non-overlapping. Search jumps go through a new ScrollMode::SourceLine variant that is resolved to the exact wrapped row at render time via Paragraph::line_count, so jumps land correctly even when long log lines wrap. The search survives log refreshes and tab switches (matches are recomputed, the current index clamped), the bottom title shows the query and [current/total] match count, and highlight styles derive from each theme via reversed video so they stay readable on every palette. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01B5UrV3Lhccx32RKRKz7YqA
1406876 to
2d530e6
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/app/model/logs/search.rs (1)
83-108: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winFull-content rescan on every keystroke.
find_matchesre-scans the entire log content from scratch on every keystroke viarefresh_search()/edit_search(). For very large logs (long-running task logs can be tens of thousands of lines), this could introduce per-keystroke latency. Not a blocker for typical log sizes, but worth keeping in mind if large-log complaints surface.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/model/logs/search.rs` around lines 83 - 108, The search path in find_matches is doing a full content rescan on every edit via refresh_search()/edit_search(), which can cause per-keystroke latency on very large logs. If this becomes a problem, optimize the search flow by reusing prior results or limiting work to the changed query state instead of recomputing everything from scratch, and keep the behavior centered around find_matches and the search refresh/edit methods.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/app/model/logs/render.rs`:
- Line 18: The wrapped log formatting currently trims leading whitespace, which
strips indentation from continuation lines in stack traces, JSON, and nested
logs. Update the WRAP configuration in render.rs to preserve indentation by
switching the Wrap setting used by wrapped_offset and the log rendering flow
from trim true to trim false, keeping the existing wrapping behavior while
retaining leading spaces.
---
Nitpick comments:
In `@src/app/model/logs/search.rs`:
- Around line 83-108: The search path in find_matches is doing a full content
rescan on every edit via refresh_search()/edit_search(), which can cause
per-keystroke latency on very large logs. If this becomes a problem, optimize
the search flow by reusing prior results or limiting work to the changed query
state instead of recomputing everything from scratch, and keep the behavior
centered around find_matches and the search refresh/edit methods.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9bfc9b55-c692-4fb6-a572-f7249c941477
📒 Files selected for processing (6)
src/app/model/logs/mod.rssrc/app/model/logs/render.rssrc/app/model/logs/search.rssrc/ui.rssrc/ui/theme/mod.rssrc/ui/theme/presets/mod.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/ui.rs
| use super::search::{Search, SearchData}; | ||
| use super::{LogModel, ScrollMode}; | ||
|
|
||
| const WRAP: Wrap = Wrap { trim: true }; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== file context =="
sed -n '1,220p' src/app/model/logs/render.rs | cat -n
echo
echo "== ratatui version references =="
rg -n 'ratatui' Cargo.lock Cargo.toml **/Cargo.toml 2>/dev/null || true
echo
echo "== Wrap usage in repo =="
rg -n 'Wrap\s*\{ trim:|wrapped_offset|trim:' src -SRepository: jvanbuel/flowrs
Length of output: 11969
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate ratatui source =="
python3 - <<'PY'
import glob, os
candidates = []
for root in [
os.path.expanduser("~/.cargo/registry/src"),
os.path.expanduser("~/.cargo/registry/cache"),
]:
candidates.extend(glob.glob(root + "/**/ratatui-*", recursive=True))
print("\n".join(sorted(set(candidates))[:50]))
PY
echo
echo "== search for Wrap docs/source =="
for d in "$HOME/.cargo/registry/src"/*/ratatui-* "$HOME/.cargo/registry/src"/*/ratatui-widgets-* "$HOME/.cargo/registry/src"/*/ratatui-core-*; do
[ -d "$d" ] || continue
rg -n -A4 -B4 'struct Wrap|Wrap \{|trim:' "$d" || true
doneRepository: jvanbuel/flowrs
Length of output: 217
🌐 Web query:
ratatui Wrap trim leading whitespace wrapped rows documentation source
💡 Result:
In the Ratatui crate, the Wrap struct is used to configure how text is wrapped within a Paragraph widget [1][2]. It contains a trim field, which is a boolean that determines whether leading whitespace should be trimmed from wrapped rows [1][2]. When trim is set to true, leading whitespace on wrapped lines is removed, which is useful for maintaining clean indentation when wrapping lists or indented text [1][2]. When set to false, the original indentation is preserved on wrapped lines [1][2]. Documentation and source code details: - Struct Definition: pub struct Wrap { pub trim: bool } [1][2]. - Usage: Applied to a Paragraph via the .wrap(Wrap { trim: true }) method [1][3][2]. - Source Code: The logic is implemented in the Paragraph widget source code, specifically within the ratatui-widgets crate, where the wrap method consumes the Wrap configuration [2][4]. You can view the full source code and documentation on docs.rs or the Ratatui GitHub repository [1][4].
Citations:
- 1: https://docs.rs/ratatui/latest/ratatui/widgets/struct.Wrap.html
- 2: https://docs.rs/ratatui-widgets/latest/src/ratatui_widgets/paragraph.rs.html
- 3: https://ratatui.rs/recipes/widgets/paragraph/
- 4: https://github.com/ratatui/ratatui/blob/1ce29d66/ratatui-widgets/src/paragraph.rs
Preserve indentation in wrapped log lines Wrap { trim: true } removes leading whitespace on continuation lines, so stack traces, JSON, and nested logs lose indentation. If that formatting matters, switch this to trim: false; wrapped_offset already uses the same wrap config.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/app/model/logs/render.rs` at line 18, The wrapped log formatting
currently trims leading whitespace, which strips indentation from continuation
lines in stack traces, JSON, and nested logs. Update the WRAP configuration in
render.rs to preserve indentation by switching the Wrap setting used by
wrapped_offset and the log rendering flow from trim true to trim false, keeping
the existing wrapping behavior while retaining leading spaces.
3 participants
Summary
/search to the Logs panel withn/Nnavigation between matchesKeybindings
/— enter search modeEnter— confirm searchEsc— cancel input or clear active searchn/N— next / previous match (wraps around)Files changed
src/app/model/logs.rs— search state machine, match computation, highlighting, key handlingsrc/ui/theme.rs—SEARCH_HIGHLIGHT_STYLEandSEARCH_CURRENT_LINE_STYLEconstantssrc/ui.rs— cursor positioning for search input barTest plan
cargo test)cargo clippycleanSummary by CodeRabbit
New Features
Bug Fixes