feat: add vim-style search to log viewer panel#600
feat: add vim-style search to log viewer panel#600shaikhamaan wants to merge 4 commits intojvanbuel:mainfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughAdds interactive search to the logs panel: new Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~35 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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
shaikhamaan
left a comment
shaikhamaan
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 |
Press / to search logs, n/N to navigate matches, Esc to clear. Matches are highlighted with emerald underline, current match gets a full teal background line highlight. Scroll position correctly accounts for line wrapping.
- Fix Unicode safety in highlight_line using char-based matching
- Fix scroll coordinate drift by normalizing SourceLine to Manual after render
- Fix scroll alignment by switching to Wrap { trim: false }
- Avoid per-line allocations when search is inactive (use borrowed Text)
- Extract current_log(), refresh_matches(), wrapped_line_count() helpers (DRY)
- Fix stale comment, clippy warnings (div_ceil, map_or)
…culations - Use unicode_width::UnicodeWidthStr for wrapped line calculations - Fix search cursor positioning to use display width - Add tests for CJK, emoji, and multibyte character handling
shaikhamaan
force-pushed
the
feature/log-search
branch
from
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. |
2 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