Code Review fixes

crates/librarian-app/src/columns.rs

@@ -120,7 +120,14 @@ fn decode_rule(field: &str) -> Option<ColRule> {
     match field {
         "fill" => Some(ColRule::Fill),
         "auto" => Some(ColRule::Auto),
-        other => other.parse::<f32>().ok().map(ColRule::Fixed),
+        // Reject non-finite widths: a hand-edited/corrupt "nan"/"inf" entry
+        // parses as a valid f32 but would propagate NaN through the layout math
+        // (f32::clamp returns NaN unchanged) into iced's widget sizing.
+        other => other
+            .parse::<f32>()
+            .ok()
+            .filter(|w| w.is_finite())
+            .map(ColRule::Fixed),
     }
 }
 
@@ -153,6 +160,15 @@ mod tests {
         assert_eq!(decode_layout("fill;auto;bogus;auto"), None); // bad field
     }
 
+    #[test]
+    fn rejects_non_finite_fixed_widths() {
+        // A corrupt/hand-edited config must not push a NaN/Inf width into layout.
+        assert_eq!(decode_rule("nan"), None);
+        assert_eq!(decode_rule("inf"), None);
+        assert_eq!(decode_rule("-inf"), None);
+        assert_eq!(decode_rule("240"), Some(ColRule::Fixed(240.0)));
+    }
+
     #[test]
     fn set_and_rule_address_each_column() {
         let mut layout = ColumnLayout::default();

crates/librarian-app/src/main.rs

@@ -27,8 +27,8 @@ use iced::widget::{
 use iced::{Border, Center, Element, Length::Fill, Point, Size, Subscription, Task, Theme};
 
 use librarian_core::{
-    Entry, History, Location, Sort, SortKey, SortOrder, is_visible, read_dir_all, read_subdirs,
-    sort_entries,
+    Entry, History, Location, Sort, SortKey, SortOrder, cmp_name_str, is_visible, read_dir_all,
+    read_subdirs, sort_entries,
 };
 use librarian_win::{
     Apartment, DriveInfo, IconImage, KnownFolder, ShellWorker, WslDistro, copy_items,
@@ -190,7 +190,13 @@ struct Clip {
 
 /// An in-progress inline rename of the row at `index`.
 struct Rename {
+    /// Which row hosts the edit field, for rendering. May go stale if a
+    /// background refresh re-sorts the list mid-edit, so the commit keys off
+    /// `path` (the file's identity), never this index.
     index: usize,
+    /// The file being renamed, captured when editing began. Survives a re-sort,
+    /// so the commit always targets the intended file.
+    path: PathBuf,
     value: String,
 }
 
@@ -597,8 +603,8 @@ enum Message {
     MoveSelection(Nav, bool, bool),
     SelectAll,
     Activate,
-    ThisPcLoaded(Vec<DriveInfo>),
-    WslLoaded(Vec<WslDistro>),
+    ThisPcLoaded(u64, Vec<DriveInfo>),
+    WslLoaded(u64, Vec<WslDistro>),
     Loaded(u64, Result<Vec<Entry>, String>),
     /// The current directory changed on disk; re-enumerate it in place.
     DirChanged,
@@ -834,6 +840,13 @@ impl Librarian {
         self.search_mode = SearchMode::default();
         self.search_active = None;
         self.search_seq = self.search_seq.wrapping_add(1);
+        // Supersede any thumbnail session the previous tab left running so the
+        // new tab doesn't inherit its loading overlay or keep pumping its
+        // background queue; the landing load opens a fresh session.
+        self.overlay_loading = false;
+        self.thumb_token = self.thumb_token.wrapping_add(1);
+        let abandoned = std::mem::take(&mut self.bg_queue);
+        self.thumbs.release(abandoned);
     }
 
     /// The persisted column layout for `location`'s folder, or the default
@@ -1134,7 +1147,13 @@ impl Librarian {
                     existing.extend(hits);
                     let found = existing.len();
                     *done = false;
+                    // Re-sorting the grown result set reorders rows, so preserve
+                    // the selection by file identity (not index) the way an
+                    // in-place folder refresh does — otherwise a click made while
+                    // results stream in would silently retarget a different hit.
+                    let previously = self.selected_paths();
                     self.recompute_rows();
+                    self.restore_selection(&previously);
                     self.status = format!("Searching… {found} found");
                     return Task::batch([self.request_icons(), self.prefetch_thumbs(false)]);
                 }
@@ -1170,13 +1189,19 @@ impl Librarian {
                 self.status = error;
             }
             Message::RowClicked(index) => return self.on_click(index),
-            Message::ThisPcLoaded(drives) => {
+            Message::ThisPcLoaded(token, drives) => {
+                if token != self.load_token {
+                    return Task::none(); // a newer navigation superseded this drive list
+                }
                 self.content = Content::ThisPc { drives };
                 self.recompute_rows();
                 self.status = format!("{} items", self.rows.len());
                 return Task::batch([self.request_icons(), self.begin_grid_session(true)]);
             }
-            Message::WslLoaded(distros) => {
+            Message::WslLoaded(token, distros) => {
+                if token != self.load_token {
+                    return Task::none(); // a newer navigation superseded this distro list
+                }
                 self.content = Content::Wsl { distros };
                 self.recompute_rows();
                 self.status = format!("{} items", self.rows.len());
@@ -1219,6 +1244,12 @@ impl Librarian {
             Message::DirChanged => {
                 // An external change to the open directory: re-enumerate it in
                 // place, preserving selection and scroll (unlike navigation).
+                // While a search owns the screen the folder listing isn't shown
+                // (and is re-read fresh when the search clears), so skip the
+                // refresh rather than letting it land and replace the results.
+                if matches!(self.content, Content::Search { .. }) {
+                    return Task::none();
+                }
                 if let Location::Path(path) = self.history.current().clone() {
                     let token = self.next_load_token();
                     return Task::perform(
@@ -1241,6 +1272,13 @@ impl Librarian {
                 let fulfilling_load = std::mem::take(&mut self.load_pending);
                 match result {
                     Ok(entries) => {
+                        // A background refresh must not tear down an active search
+                        // view: the search owns the screen until it's cleared
+                        // (which re-reads the folder fresh). Only reachable when not
+                        // fulfilling a navigation — navigation ends any search.
+                        if !fulfilling_load && matches!(self.content, Content::Search { .. }) {
+                            return Task::none();
+                        }
                         // Fulfilling a navigation: commit to the new folder's columns
                         // before `recompute_rows` re-measures. An ordinary in-place
                         // refresh (no pending load) keeps the current columns.
@@ -1578,11 +1616,12 @@ impl Librarian {
             Message::RenameStart => {
                 self.menu = None;
                 if let Some(index) = self.selection.lead()
-                    && self.lead_path().is_some()
+                    && let Some(path) = self.lead_path()
                     && let Some(row) = self.rows.get(index)
                 {
                     self.renaming = Some(Rename {
                         index,
+                        path,
                         value: row.label.clone(),
                     });
                     return iced::widget::operation::focus(RENAME_ID);
@@ -1594,13 +1633,19 @@ impl Librarian {
                 }
             }
             Message::RenameCommit => {
-                if let Some(state) = self.renaming.take()
-                    && let Some(row) = self.rows.get(state.index)
-                    && let Location::Path(path) = &row.target
-                {
+                // Commit against the file captured when editing began, not the
+                // current row at `state.index`: a background refresh may have
+                // re-sorted the list mid-edit, leaving that index pointing at a
+                // different file (which would otherwise be renamed by mistake).
+                if let Some(state) = self.renaming.take() {
                     let new_name = state.value.trim().to_string();
-                    if !new_name.is_empty() && new_name != row.label {
-                        let path = path.clone();
+                    let old_name = state
+                        .path
+                        .file_name()
+                        .and_then(|n| n.to_str())
+                        .unwrap_or_default();
+                    if !new_name.is_empty() && new_name != old_name {
+                        let path = state.path.clone();
                         return self
                             .dispatch_op("Renaming", move |apt| rename(apt, &path, &new_name));
                     }
@@ -1737,29 +1782,33 @@ impl Librarian {
 
         let load = match location {
             Location::ThisPc => {
-                // A virtual root owes no directory read. Its landing message
-                // (`ThisPcLoaded`/`WslLoaded`) carries no load token, so a Path
-                // read still in flight from the folder we left would otherwise
-                // keep matching `load_token` and clobber this page when it lands
-                // (e.g. a slow `\\wsl.localhost\…` refresh finishing after you
-                // click "This PC"). Bump the token to mark that read stale and
-                // clear the pending flag so `show_active` doesn't try to resume it.
-                self.next_load_token();
+                // A virtual root owes no directory read. Bump the load token so a
+                // Path read still in flight from the folder we left goes stale and
+                // can't clobber this page (e.g. a slow `\\wsl.localhost\…` refresh
+                // finishing after you click "This PC"), and tag the landing message
+                // with that token so the reverse also holds: a *slow drive list*
+                // can't clobber a folder the user navigates into next. Clear the
+                // pending flag so `show_active` doesn't try to resume the read.
+                let token = self.next_load_token();
                 self.load_pending = false;
                 self.adopt_column_layout();
                 self.content = Content::default();
                 self.rows.clear();
-                Task::perform(offload(list_drives), Message::ThisPcLoaded)
+                Task::perform(offload(list_drives), move |drives| {
+                    Message::ThisPcLoaded(token, drives)
+                })
             }
             Location::Wsl => {
-                self.next_load_token();
+                let token = self.next_load_token();
                 self.load_pending = false;
                 self.adopt_column_layout();
                 self.content = Content::Wsl {
                     distros: Vec::new(),
                 };
                 self.rows.clear();
-                Task::perform(offload(list_wsl_distros), Message::WslLoaded)
+                Task::perform(offload(list_wsl_distros), move |distros| {
+                    Message::WslLoaded(token, distros)
+                })
             }
             Location::Path(path) => {
                 let token = self.next_load_token();
@@ -2019,7 +2068,12 @@ impl Librarian {
             return Task::none();
         };
         self.selection.select_one(index);
-        self.renaming = Some(Rename { index, value: name });
+        let path = dir.join(&name);
+        self.renaming = Some(Rename {
+            index,
+            path,
+            value: name,
+        });
         Task::batch([
             self.ensure_visible(index),
             iced::widget::operation::focus(RENAME_ID),
@@ -2066,7 +2120,7 @@ impl Librarian {
                 rows.sort_by(|a, b| {
                     b.is_container
                         .cmp(&a.is_container)
-                        .then_with(|| a.label.to_lowercase().cmp(&b.label.to_lowercase()))
+                        .then_with(|| cmp_name_str(&a.label, &b.label))
                 });
                 rows
             }
@@ -2309,10 +2363,15 @@ impl Librarian {
         let rows_visible = (self.viewport_h / tile_h).ceil() as usize + 1;
         let first_row = (self.scroll_y / tile_h).floor() as usize;
         let start_row = first_row.saturating_sub(GRID_OVERSCAN_ROWS);
-        let end_row = first_row + rows_visible + GRID_OVERSCAN_ROWS;
+        let end_row = first_row
+            .saturating_add(rows_visible)
+            .saturating_add(GRID_OVERSCAN_ROWS);
 
-        let start = (start_row * cols).min(self.rows.len());
-        let end = (end_row * cols).min(self.rows.len());
+        // Saturating throughout: an extreme stale `scroll_y` (a deep scroll over
+        // a since-shrunk listing) must clamp to the end, never overflow a `usize`
+        // or invert the `start..end` range the slice below relies on.
+        let start = start_row.saturating_mul(cols).min(self.rows.len());
+        let end = end_row.saturating_mul(cols).min(self.rows.len());
         self.rows[start..end]
             .iter()
             .filter_map(|row| thumb_key(row, px))
@@ -3442,7 +3501,7 @@ fn fetch_tree_children(location: &Location, show_hidden: bool) -> Result<Vec<Tre
         Location::Path(dir) => {
             let mut dirs = read_subdirs(dir).map_err(|e| e.to_string())?;
             dirs.retain(|e| is_visible(e, show_hidden));
-            dirs.sort_by(|a, b| a.name.to_lowercase().cmp(&b.name.to_lowercase()));
+            dirs.sort_by(|a, b| cmp_name_str(&a.name, &b.name));
             let children = dirs
                 .into_iter()
                 .map(|e| TreeChild::lazy(e.name, IconKey::Folder, Location::Path(e.path)))
@@ -3516,6 +3575,14 @@ fn is_input_shortcut(message: &Message) -> bool {
             | Message::CloseActiveTab
             | Message::NextTab
             | Message::PrevTab
+            // Not keyboard shortcuts, but they must also be dropped while the
+            // modal overlay is up: each can start a competing grid session or
+            // run a stale search and release the lock early. A pick_list's open
+            // dropdown or an already-scheduled search debounce can still fire
+            // even though the scrim swallows ordinary pointer input.
+            | Message::ViewModeChanged(_)
+            | Message::SortBy(_)
+            | Message::SearchDebounced(_)
     )
 }
 

crates/librarian-core/src/lib.rs

@@ -16,4 +16,4 @@ pub use enumerate::{DEFAULT_BATCH, read_dir_all, read_dir_batched, read_subdirs}
 pub use history::History;
 pub use matcher::{NameMatcher, find_matching_dirs};
 pub use model::{Attributes, Entry, EntryKind, Location, extension_of, is_wsl_host};
-pub use sort::{Sort, SortKey, SortOrder, is_visible, sort_entries};
+pub use sort::{Sort, SortKey, SortOrder, cmp_name_str, is_visible, sort_entries};

crates/librarian-core/src/sort.rs

@@ -70,8 +70,16 @@ pub fn sort_entries(entries: &mut [Entry], sort: &Sort) {
     });
 }
 
+/// Case-insensitive lexical comparison of two display names — the single point
+/// all name ordering routes through (folder listings, search results, the tree),
+/// so a later switch to `StrCmpLogicalW` (natural numeric order; see the module
+/// docs) lands everywhere at once.
+pub fn cmp_name_str(a: &str, b: &str) -> Ordering {
+    a.to_lowercase().cmp(&b.to_lowercase())
+}
+
 fn cmp_name(a: &Entry, b: &Entry) -> Ordering {
-    a.name.to_lowercase().cmp(&b.name.to_lowercase())
+    cmp_name_str(&a.name, &b.name)
 }
 
 /// Decide whether an entry should be visible given the current view options.

docs/debug_log.md

@@ -6,6 +6,117 @@ files/lines touched, with the commit that carries the fix for context.
 
 ---
 
+## 2026-06-17 — Inline rename could rename the WRONG file after a background refresh
+
+**Commit:** _pending_ — staged in the `feat-phase2` working tree from a comprehensive
+code review (2026-06-17). Stamp with the fix commit hash once committed.
+
+### Symptom
+Start an inline rename (F2) on a file, then — before pressing Enter — let the open
+folder change on disk (an external tool adds/removes/renames a sibling, e.g. an
+editor's temp/`.lock` churn). Pressing Enter renames a *different* file than the one
+whose name is being edited. Silent data corruption.
+
+### Root cause
+`Rename` stored only `{ index, value }` — a *positional* row index. The folder watcher
+fires `DirChanged` → `Reloaded`, whose `recompute_rows()` re-sorts and rebuilds
+`self.rows`. After the re-sort, `renaming.index` points at whatever file now occupies
+that row, and `RenameCommit` did `self.rows.get(state.index)` and renamed that row's
+target. The same positional-index hazard scrambled the selection during a *streaming
+search*: `SearchBatch` re-sorts every batch but — unlike `Reloaded` — did not re-map
+the selection, so a click / Delete made mid-stream acted on the wrong hit.
+
+### Fix
+Carry **file identity, not row position**, across a recompute:
+- `Rename` gained a `path: PathBuf` captured at `RenameStart`; `RenameCommit` renames
+  that path (and derives the old name from it), ignoring the possibly-stale index.
+- `SearchBatch` now captures `selected_paths()` before `recompute_rows()` and calls
+  `restore_selection()` after, exactly as the `Reloaded` arm already did (re-select by
+  path).
+
+### Affected files & lines
+`crates/librarian-app/src/main.rs`:
+- **L192–202** — `Rename`: added the `path` identity field (doc'd why `index` can go stale).
+- **L1616–1628** — `RenameStart`: capture the lead path into `Rename.path`.
+- **L1635–1655** — `RenameCommit`: rename `state.path`, not `rows[index]`.
+- **L2071–2076** — `begin_pending_rename`: set `path` for a programmatic rename.
+- **L1137–1160** — `SearchBatch`: capture/restore selection by path across the re-sort.
+
+---
+
+## 2026-06-17 — Background refresh / stale landing list clobbered the visible view
+
+**Commit:** _pending_ — staged in the `feat-phase2` working tree (2026-06-17 review).
+Stamp with the fix commit hash once committed.
+
+### Symptom
+Two related glitches:
+1. Run a search in a folder; while results show, anything changes the folder on disk →
+   the results vanish, replaced by the normal folder listing, and the search silently
+   stops streaming.
+2. From "This PC", open a drive and immediately navigate into a folder. The slow
+   drive-list enumeration finishes *after* you've arrived and overwrites the folder with
+   the "This PC" drive grid, dropping the loading overlay early.
+
+### Root cause
+Both are "a stale/background producer clobbers the current view":
+1. `Reloaded`'s success arm set `self.content = Content::Folder { entries }`
+   unconditionally — even with `Content::Search` on screen. The search subscription was
+   still alive, so subsequent `SearchBatch`es were then dropped (content no longer
+   `Search`).
+2. `ThisPcLoaded` / `WslLoaded` carried no load token, so a slow `list_drives` result
+   applied even after a newer navigation. (Last session's F2 guarded the *reverse*
+   direction — navigating away from a virtual root — by bumping the token in
+   `load_current`; this is the other half.)
+
+### Fix
+- `Reloaded` (Ok) bails when `Content::Search` is active, and `DirChanged` skips the read
+  entirely during a search — the search owns the screen; clearing it re-reads the folder
+  fresh.
+- `ThisPcLoaded` / `WslLoaded` now carry the load token (captured when `load_current`
+  dispatches `list_drives` / `list_wsl_distros`) and drop on mismatch.
+
+### Affected files & lines
+`crates/librarian-app/src/main.rs`:
+- **L606–607** — `ThisPcLoaded` / `WslLoaded`: added a `u64` load-token field.
+- **L1192–1210** — handlers: drop on token mismatch.
+- **L1247–1252** — `DirChanged`: skip the refresh while a search is active.
+- **L1275–1281** — `Reloaded` (Ok): don't overwrite an active `Content::Search`.
+- **L1792–1812** — `load_current` (`ThisPc` / `Wsl`): capture and tag the load token.
+
+---
+
+## 2026-06-17 — Review hardening: overlay lock, tab session reset, config & grid guards
+
+**Commit:** _pending_ — staged in the `feat-phase2` working tree (2026-06-17 review).
+Stamp with the fix commit hash once committed.
+
+### Summary
+Lower-severity fixes from the same comprehensive review:
+
+- **Modal-overlay total-lock gaps** — `ViewModeChanged`, `SortBy`, and `SearchDebounced`
+  bypassed the loading overlay's input lock (a floating `pick_list` dropdown or a deferred
+  search debounce could fire during a cold load and release the lock early). Added them to
+  `is_input_shortcut`. → `main.rs` **L3578–3586**.
+- **New tab inherited a stale thumbnail session** — `reset_flat` didn't clear
+  `overlay_loading` / `thumb_token` / `bg_queue`, so a new tab could show the previous
+  tab's spinner and keep pumping its background thumbnail queue. `reset_flat` now supersedes
+  the session. → `main.rs` **L843–849**.
+- **Non-finite persisted column width** — a corrupt `nan` / `inf` entry in the columns
+  config parsed to a valid `f32` and pushed a NaN width through `compute_col_px` into iced
+  layout (`f32::clamp` returns NaN unchanged). `decode_rule` now rejects non-finite values
+  (+ regression test). → `columns.rs` **L119–131**.
+- **Grid window arithmetic** — `visible_thumb_keys` computed `row * cols` before clamping;
+  switched to `saturating_*` so an extreme stale `scroll_y` can't overflow a `usize` or
+  invert the slice range (matches the `window_for` hardening in `afd23ed`). → `main.rs`
+  **L2370–2376**.
+- **De-duplicated name comparison** — hoisted the triplicated case-insensitive name compare
+  into `librarian_core::cmp_name_str`, now called by the folder, search-results, and
+  tree-children sorts (a single point to later switch to `StrCmpLogicalW`). → `sort.rs`
+  **L77**, `lib.rs` **L19**, `main.rs` search/tree sorts.
+
+---
+
 ## 2026-06-17 — WSL folder load: column "pinch" + inconsistent loading screen
 
 **Commit:** `20347e3` — "WSL loading screen fix" (branch `feat-phase2`)