Fix natsort and viewer bugs, remove duplicate toggle action#22
Conversation
- natsort: unify natsort_key with natsort for leading-zero numbers - viewer: visual-row-based scroll when wrap_lines is enabled - watcher: bypass debounce for Deleted/Renamed events - menu_actions: remove duplicate TogglePanelHidden, merge into ToggleHiddenFiles - main: fix search Esc wiping file list when unfiltered_entries is empty
Reviewer's GuideImplements visual-row-based scrolling and layout for wrapped viewer text, adjusts viewer status line reporting, fixes natural sort behavior around leading zeros, refines filesystem watcher debounce logic, removes a duplicate panel hidden toggle action, and updates project configuration docs/tests accordingly. Sequence diagram for viewer wrap layout and visual-row scrollingsequenceDiagram
participant App as AppState
participant Main as handle_viewer_mode
participant Viewer as ViewerState
participant UI as render_viewer
App->>Main: handle_viewer_mode(key, terminal_size, terminal_width)
Main->>Viewer: update_wrap_layout(content_width)
Viewer-->>Main: visual_heights updated
Main->>UI: render_viewer(frame, area, ViewerState)
UI->>Viewer: is_hex_mode()
UI->>Viewer: visual_row_to_logical(scroll_offset)
UI->>Viewer: max_scroll()
UI-->>Main: paragraph rendered with wrap and sub_row scroll
Main-->>App: updated viewer display with visual-row scrolling
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Code Review
This pull request introduces visual line wrapping support in the file viewer, ensuring that scrolling and search navigation correctly account for lines that span multiple visual rows. It also refactors menu actions by removing the redundant TogglePanelHidden action, updates the natural sorting algorithm to handle leading zeros more predictably, and expands the project configuration to support additional language servers. The review feedback identifies a potential integer underflow in the viewer's rendering logic and a mismatch in width calculation that could lead to incorrect wrapping behavior.
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The visual scrolling logic (e.g.
is_visual_scrollinViewerStatevs theuse_visualcondition inrender_viewer) duplicates the same predicate in multiple places; consider centralizing this into a single helper to avoid divergence if the conditions change later. - Both
visual_row_to_logicalandlogical_to_visual_rowwalkvisual_heightslinearly on each call; if you expect large files or frequent calls (e.g. during scrolling), consider precomputing prefix sums or another lightweight index to avoid repeated O(n) scans.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The visual scrolling logic (e.g. `is_visual_scroll` in `ViewerState` vs the `use_visual` condition in `render_viewer`) duplicates the same predicate in multiple places; consider centralizing this into a single helper to avoid divergence if the conditions change later.
- Both `visual_row_to_logical` and `logical_to_visual_row` walk `visual_heights` linearly on each call; if you expect large files or frequent calls (e.g. during scrolling), consider precomputing prefix sums or another lightweight index to avoid repeated O(n) scans.
## Individual Comments
### Comment 1
<location path="src/ui/viewer.rs" line_range="615-624" />
<code_context>
- let start_idx = state.scroll_offset;
- let end_idx = (start_idx + visible_height).min(state.content.len());
+
+ let use_visual = state.wrap_lines && !state.is_hex_mode() && !state.visual_heights.is_empty();
+
+ let (start_idx, sub_row, end_idx) = if use_visual {
+ let (logical_start, sub) = state.visual_row_to_logical(state.scroll_offset);
+ let mut visual_budget = visible_height + sub;
+ let mut end = logical_start;
+ while end < state.content.len() && visual_budget > 0 {
+ visual_budget -= state.visual_heights[end];
+ end += 1;
+ }
+ (logical_start, sub, end)
+ } else {
+ let start = state.scroll_offset;
+ let end = (start + visible_height).min(state.content.len());
+ (start, 0, end)
+ };
</code_context>
<issue_to_address>
**suggestion:** Reuse `is_visual_scroll` instead of duplicating the visual-scroll condition.
`use_visual` duplicates the logic in `ViewerState::is_visual_scroll`, except for the `!self.visual_heights.is_empty()` check. Keeping this condition in two places risks them drifting apart over time. Prefer calling `state.is_visual_scroll()` here, or adjust `is_visual_scroll` to cover the required semantics so the logic is defined in one place.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
… use_visual dedup - viewer: use saturating_sub for visual_budget to prevent underflow panic - viewer: reuse is_visual_scroll() instead of duplicating the condition - main: remove incorrect -2 from content_width (block has no side borders)
Summary
Improve viewer line-wrapping behavior, adjust natural sort ordering with leading zeros, refine file watcher debounce, and simplify panel hidden-toggle handling.
New Features:
Bug Fixes:
Enhancements:
Tests: