diff --git a/crates/librarian-app/src/columns.rs b/crates/librarian-app/src/columns.rs index a041b48..def4153 100644 --- a/crates/librarian-app/src/columns.rs +++ b/crates/librarian-app/src/columns.rs @@ -120,7 +120,14 @@ fn decode_rule(field: &str) -> Option { match field { "fill" => Some(ColRule::Fill), "auto" => Some(ColRule::Auto), - other => other.parse::().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::() + .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(); diff --git a/crates/librarian-app/src/main.rs b/crates/librarian-app/src/main.rs index 07563ce..a23954b 100644 --- a/crates/librarian-app/src/main.rs +++ b/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), - WslLoaded(Vec), + ThisPcLoaded(u64, Vec), + WslLoaded(u64, Vec), Loaded(u64, Result, 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 { 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(_) ) } diff --git a/crates/librarian-core/src/lib.rs b/crates/librarian-core/src/lib.rs index 73c7c4b..ac47440 100644 --- a/crates/librarian-core/src/lib.rs +++ b/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}; diff --git a/crates/librarian-core/src/sort.rs b/crates/librarian-core/src/sort.rs index 2b336d3..4a6c8bc 100644 --- a/crates/librarian-core/src/sort.rs +++ b/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. diff --git a/docs/debug_log.md b/docs/debug_log.md index aadced6..344fe48 100644 --- a/docs/debug_log.md +++ b/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`)