Code Review fixes
This commit is contained in:
@@ -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`)
|
||||
|
||||
Reference in New Issue
Block a user