Optimize panel refresh and hot paths#46
Merged
Merged
Conversation
Reviewer's GuideRefactors several hot paths (sorting, search, panel refresh, viewer, compare, copy, search I/O, dialogs) to reduce allocations, avoid unnecessary full-panel refreshes, and preserve cursor/focus, while tightening tests and adding memchr-based byte searching and better watcher sync behavior. Sequence diagram for optimized panel refresh on search/filter changessequenceDiagram
actor User
participant ModeDispatch as input_mode_dispatch
participant AppState
participant PanelOps as panel_ops
User->>ModeDispatch: clear_search_state(state)
ModeDispatch->>AppState: mutate active_panel.filter = None
ModeDispatch->>PanelOps: refresh_active(state)
Note over PanelOps: Only if panel.unfiltered_dirty || unfiltered_entries.is_empty()
User->>ModeDispatch: handle_search_mode(state, key, terminal_height)
ModeDispatch->>AppState: update state.search_query
ModeDispatch->>AppState: panel.filter = Some(query)
alt panel.unfiltered_dirty || unfiltered_entries.is_empty()
ModeDispatch->>PanelOps: refresh_active(state)
else reuse_unfiltered_entries
ModeDispatch->>PanelOps: rebuild_visible_entries(panel)
end
Flow diagram for panel_ops::rebuild_visible_entries hot pathflowchart TD
A["rebuild_visible_entries(panel)"] --> B["sync_unfiltered_selection(panel)"]
B --> C["current_panel_entry_name(panel)"]
C --> D["filtered_sorted_entries(
&panel.unfiltered_entries,
panel.filter,
panel.sort_mode,
panel.sort_options,
panel.show_hidden
)"]
D --> E["panel.entries = result"]
E --> F["panel.recalculate_selection_stats()"]
F --> G["restore_panel_cursor(panel, current_name)"]
G --> H["panel.ensure_cursor_visible(
current_visible_height()
)"]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![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 centralizes panel entry management through a new rebuild_visible_entries function and introduces several performance optimizations across the codebase, including the use of memchr for searching, pre-allocating collection capacities, and reducing redundant system calls in the copy and UI logic. Feedback identifies a performance regression in the case-insensitive sorting implementation due to repeated string allocations and suggests passing terminal dimensions directly to the rebuild function to avoid frequent ioctl calls during search operations.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The new case-insensitive name comparison (
cmp_name_insensitive) now allocates lowercaseStrings on every comparison andsort_byre-computes these per comparison, which can significantly regress performance compared to the previoussort_by_cached_key; consider caching lowercase (and possibly original) names per entry or restoring a key-based approach to avoid repeated allocations. - Several call sites now delegate to
panel_ops::rebuild_visible_entries, which restores cursor based on the current entry; if a caller wants to preserve or prefer a different entry (e.g. a path from an external event), it might be cleaner to expose an overload that accepts a preferred name instead of relying on additional cursor adjustments afterward.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new case-insensitive name comparison (`cmp_name_insensitive`) now allocates lowercase `String`s on every comparison and `sort_by` re-computes these per comparison, which can significantly regress performance compared to the previous `sort_by_cached_key`; consider caching lowercase (and possibly original) names per entry or restoring a key-based approach to avoid repeated allocations.
- Several call sites now delegate to `panel_ops::rebuild_visible_entries`, which restores cursor based on the current entry; if a caller wants to preserve or prefer a different entry (e.g. a path from an external event), it might be cleaner to expose an overload that accepts a preferred name instead of relying on additional cursor adjustments afterward.
## Individual Comments
### Comment 1
<location path="src/ops/sorting.rs" line_range="175-182" />
<code_context>
}
}
+fn cmp_name_insensitive(a: &str, b: &str) -> Ordering {
+ let a_lower = a.to_lowercase();
+ let b_lower = b.to_lowercase();
+
+ a_lower.cmp(&b_lower).then_with(|| {
+ let a_tie = if a_lower == a { "" } else { a };
+ let b_tie = if b_lower == b { "" } else { b };
+ a_tie.cmp(b_tie)
+ })
+}
</code_context>
<issue_to_address>
**issue (performance):** Case-insensitive name comparison now allocates per comparison, which can significantly degrade sort performance on large directory listings.
Previously, `sort_by_cached_key` + `name_key` ensured each lowercase name was allocated once. Now `cmp_name_insensitive` does `to_lowercase` on both sides for every `sort_by` comparison, leading to `O(n log n)` allocations and a likely slowdown on large/long directory listings.
To keep the new tie‑breaking while avoiding repeated allocations, consider either:
- Returning to a cached-key sort (e.g., `sort_by_cached_key` on `(lower, original)`), or
- Storing a precomputed lowercase name on `FileEntry`.
Both options avoid per-comparison allocations while preserving behavior.
</issue_to_address>
### Comment 2
<location path="src/tests/menu.rs" line_range="203" />
<code_context>
+#[test]
+fn menu_sort_preserves_current_entry_focus() {
</code_context>
<issue_to_address>
**issue (testing):** This test does not currently prove that sorting occurred, only that focus stayed on the same index.
With the initial state `entries = ["zeta.txt", "alpha.txt"]` and `cursor = 0`, the final assertion `current_entry() == "zeta.txt"` would still pass if the menu action were a no‑op, so the test doesn’t prove that sorting occurred.
To ensure it does:
- Assert the final order of `entries` (e.g. `["alpha.txt", "zeta.txt"]` after the sort).
- Assert that the focused entry remains "zeta.txt" even though its index changes (cursor moves with it).
This will cause the test to fail if sorting is removed or broken, not just if focus behavior changes.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Optimize panel refresh, comparisons, search, and viewer hot paths while preserving panel focus and improving case-insensitive sorting behavior.
Enhancements:
Tests: