diff --git a/REFACTOR_TICKETS.md b/REFACTOR_TICKETS.md new file mode 100644 index 0000000..e7eed11 --- /dev/null +++ b/REFACTOR_TICKETS.md @@ -0,0 +1,318 @@ +# Librarian Refactor — Ticket Log (self-implemented) + +## Context + +`crates/librarian-app/src/main.rs` has grown to **4308 lines** — nearly half the +whole workspace in one file. Everything else is already reasonably modularized +(next-largest source file is 646 lines, and most of *that* is tests). The goal is +to break `main.rs` into ~9–10 focused modules following SRP, then a final DRY +pass to remove the repeated patterns the analysis found — **without changing any +behavior**. + +You (not Claude) are implementing this to learn Rust, ticket by ticket, asking +questions as you go. The tickets are ordered **safest-first**: pure mechanical +moves you can verify with `cargo build` before anything that could change runtime +behavior. DRY consolidation is deliberately saved for the end (Phase 5) so every +earlier ticket is a "no behavior change" move. + +Scope: `main.rs` + `tree.rs` (test relocation only). `librarian-win/icon.rs` is +out of scope. + +--- + +## The 3 Rust mechanics you need (read this first) + +Everything below uses one of three moves. Learn these and the tickets are +mechanical. + +**1. Leaf module (for *pure functions* — no `self`).** +The styling functions, keyboard predicates, and column-math functions don't touch +the app struct. Move them to a new file, add `mod foo;` to `main.rs`, and call +them as `foo::bar(...)`. This is exactly how `metrics.rs` / `rows.rs` already +work. *Zero behavioral risk.* + +**2. Split `impl` block (for *methods* — `&self` / `&mut self`).** +`view()`, `update()`, and all the `view_*` / handler methods take `self`. Rust +lets an `impl Librarian { … }` block live in **any file in the same crate**. So +you create `view.rs`, write `use crate::*;` then `impl Librarian { pub(crate) fn +view_toolbar(&self) -> … { … } }`, and the call site `self.view_toolbar()` in +`main.rs` is **unchanged**. You are physically moving the method, not rewriting +the call. + +**3. The one visibility rule that will bite you.** +A child module (e.g. `view`) can freely *see* private items of `main.rs` — +descendants can read their ancestors' privates. But `main.rs` **cannot** see a +private item of its child. So: +- Any method/type you move *out* of `main.rs` must become **`pub(crate)`** (so + `main.rs` and sibling modules can still call it). +- Anything that *stays* in `main.rs` (e.g. `recompute_rows`, the `Librarian` + struct, the `Message` enum, constants) needs **no change** — the new child + modules can already see it. + +Rule of thumb: **the thing you move gets `pub(crate)`; the things it calls back +into don't.** + +> Shrinking `update()` (a 600-line `match`): you can't split one `match` across +> files. Instead, move each complex arm's *body* into a `pub(crate)` handler +> method in the right module, and replace the arm with a one-liner, e.g. +> `Message::Delete => return self.delete_selected(),`. The `match` stays in +> `main.rs` but collapses to a dispatch table. + +--- + +## Target module layout (`crates/librarian-app/src/`) + +Existing (leave as-is): `columns config ellipsis icons metrics rows search +selection theme thumbs tree`. + +New files this refactor adds: + +| New file | Responsibility | Mechanism | +|---|---|---| +| `styles.rs` | All `*_style()` fns + tiny pure element helpers (dividers, tooltip, `loading_overlay`, `view_header`, `gap`) | Leaf | +| `keyboard.rs` | `key_to_message`, `is_input_shortcut`, `is_text_edit`, `clear_focus` | Leaf | +| `columns.rs` (extend) | `compute_col_px`, `measure_columns`, `sample_widest`, `width_bounds`, `sort_key_of` | Leaf (into existing) | +| `view.rs` | Window chrome: `view`, tabs, toolbar, status, command bar, tree view, context menu, empty-state | impl-split | +| `view_details.rs` | Details list: `view_details`, `view_list`, `view_row` | impl-split | +| `view_grid.rs` | Icon grid: `view_body`, `view_grid`, `view_tile` | impl-split | +| `grid_session.rs` | Thumbnail session coordination + `ThumbsCached/ThumbsLoaded` arms | impl-split | +| `tabs.rs` | `TabState` + snapshot/restore/switch/new/close + tab arms | impl-split + struct | +| `fileops.rs` | `dispatch_op`, file-op arms, selection-path helpers, `unique_folder_name` | impl-split + free fn | +| `navload.rs` | `load_current`/`navigate`, folder-load arms, dir-watch, tree building/reveal | impl-split + free fns | + +`main.rs` keeps: the `Librarian` struct, the `Message` enum, shared small enums +(`Content`, `ViewMode`, `PaneKind`, `Nav`, …), constants, `new()`/`title()`/ +`subscription()`, the `update()` dispatch `match`, and a handful of cross-cutting +orchestration methods (`recompute_rows`, `request_icons`, `settings`, +`apply_sort`, `scroll_to`, `ensure_visible`, `move_selection`, `on_click`, +`activate`). Target: **~600–800 lines**. + +> Line numbers below are anchors from the current file and *will drift* as you +> edit. Navigate by **function name** (search), not by line number. + +--- + +## Conventions to follow (match the existing code) + +- Files `snake_case`; types `PascalCase`; free fns `snake_case`. +- One `mod foo;` per new file, alphabetically grouped with the others at the top + of `main.rs`. Add `use crate::foo::{…}` imports next to the existing block. +- Modules don't define `Message` and don't take `&mut Librarian` as a *parameter* + — instead they're either leaf free-fns or `impl Librarian` blocks (methods). +- Keep each module's doc-comment header (`//! …`) describing its one job, like + `tree.rs` and `thumbs.rs` do. +- Colocate any moved `#[cfg(test)]` with its code. + +--- + +## Phase 0 — Baseline (do once, before Ticket 1) + +**T0. Establish a green baseline & checkpoint discipline.** +- Run `cargo build -p librarian-app`, `cargo clippy --workspace`, + `cargo test --workspace`, and launch `cargo run -p librarian-app` once. Note the + current warning count. +- Commit the clean state. **After every ticket: `cargo build` + `cargo clippy` + + launch the app + commit.** A ticket isn't done until it builds clean and the app + still looks/behaves identically. + +--- + +## Phase 1 — Pure leaf extractions (lowest risk, do first) + +These move *only* free functions. If it compiles, it's correct. + +**T1. Extract `styles.rs`.** +Move the styling block (~`3029–3432`): every `*_style()` fn plus the small pure +element/builder helpers in that range — `gap`, `full_path_text`, `path_tooltip`, +`header_rule`, `tree_section_divider`, `column_divider`, `view_header`, +`loading_overlay`. Add `mod styles;`, mark each moved fn `pub(crate)`, add +`use crate::styles::*;` (or explicit names) in `main.rs`. Some take `&Theme` / +`spinner_frame: usize` — pass-through, no `self`. +*Verify:* compiles; app visuals identical (selection highlight, dividers, tabs, +loading overlay). + +**T2. Extract `keyboard.rs`.** +Move `is_input_shortcut`, `is_text_edit`, `clear_focus`, `key_to_message` +(~`3556–3650`). All free fns over `&Message` / `Key`. Mark `pub(crate)`; fix the +call sites in `subscription()` and at the top of `update()`. +*Verify:* compiles; keyboard nav, Ctrl-shortcuts, and typing in the address/search +/rename fields all still work (the overlay-suppression check at `update` line +~1065 still calls `is_input_shortcut`/`is_text_edit`). + +**T3. Consolidate column math into `columns.rs`.** +Move `compute_col_px`, `width_bounds`, `sort_key_of` (~`3815–3870`), +`measure_columns` (~`2167–2220`), and `sample_widest` out of `main.rs` into the +existing `columns.rs` (their natural home — they already operate on +`ColumnLayout`/`Column`). Make them `pub` in `columns.rs`. If any need +`measure_width`/`LIST_TEXT_SIZE`, import from `metrics` there. +*Verify:* compiles; column auto-fit, drag-resize, and header sort-arrow widths +behave identically. + +--- + +## Phase 2 — View impl-split (low risk: moving `&self` methods) + +Do **T1 first** — these call the now-`pub(crate)` style fns. Pure render methods, +no state mutation, so behavior can't change if it compiles; spot-check visuals. + +**T4. Extract `view.rs` (chrome).** +Move methods: `view`, `view_tabs`, `view_tab`, `view_toolbar`, `view_status`, +`view_command_bar`, `view_tree`, `view_tree_row`, `view_context_menu`, +`empty_list_message`. Create `view.rs` with `use crate::*;` and `impl Librarian { +… }`; mark each `pub(crate)`. (`view` itself is required by the Iced `update`/`view` +wiring — confirm it's still found; if the framework needs it on the original impl, +keep just `view`'s signature delegating, or move it too and ensure the trait/inherent +lookup still resolves.) +*Verify:* compiles; toolbar, tabs, status bar, command bar, tree sidebar, and +right-click menu all render and respond. + +**T5. Extract `view_details.rs`.** +Move `view_details`, `view_list`, `view_row` (~`2547–2587`, `2729–2844`). These +call `compute_col_px`/`measure_columns` (now in `columns.rs` from T3) and styles +(T1) — import them. +*Verify:* details list renders; virtualization (scroll), inline rename field, and +per-column ellipsis all intact. + +**T6. Extract `view_grid.rs`.** +Move `view_body`, `view_grid`, `view_tile` (~`2594–2597`, `2850–2953`). +*Verify:* all icon-grid sizes (Tiny→ExtraLarge) render; thumbnails appear; tile +rename field works. + +--- + +## Phase 3 — Stateful impl-split (moves `&mut self` methods + update arms) + +Higher care: these mutate state and own update-arm logic. Move the methods, then +collapse each related `update` arm to a one-line delegating call. + +**T7. Extract `grid_session.rs` (thumbnail coordination).** +Move `begin_grid_session`, `prefetch_thumbs`, `retain_visible_misses`, +`extract_full_task`, `seed_background`, `next_background_chunk`, +`visible_thumb_keys` (~`2243–2366`) and the `thumb_key` free fn. Add a +`pub(crate) fn handle_thumbs_cached/loaded(...)` and move the `ThumbsCached` / +`ThumbsLoaded` arm bodies (~`1335–1397`) into them; replace arms with delegating +calls. Keep `ThumbKind` where it's referenced (move to this module as `pub(crate)` +if only used here). +*Verify:* switch to a grid view in an image folder — loading overlay raises/clears, +viewport thumbs load first, background pre-cache fills in on scroll. + +**T8. Extract `tabs.rs`.** +Move `TabState` (~`745–819`) and `snapshot_flat`/`restore_flat`/`reset_flat`/ +`show_active`/`switch_tab`/`new_tab`/`close_tab`. Move the tab arm bodies +(`NewTab`/`SelectTab`/`CloseTab`/`CloseActiveTab`/`NextTab`/`PrevTab`/ +`OpenInNewTab`, ~`1401–1430`) into `pub(crate)` handler methods. `TabState` and its +fields used by `main.rs` become `pub(crate)`. +*Note:* the active tab's state lives directly on `Librarian`; only parked tabs are +`Some(TabState)`. Preserve that exactly — `snapshot_flat`/`restore_flat` are the +only places that convert between the two. +*Verify:* open/close/switch/cycle tabs; each tab restores its own folder, scroll, +selection, columns. + +**T9. Extract `fileops.rs`.** +Move `dispatch_op` and the file-op arm bodies (`OpenSelected`, `NewFolder`, +`DeleteSelected`, `Copy`, `Cut`, `Paste`, `RenameStart/Changed/Commit`, +`OpFinished`, ~`1560–1657`) into `pub(crate)` handler methods; move +`unique_folder_name` (~`3544–3554`), `selected_paths`, `current_dir`, `lead_path`, +`restore_selection`, `begin_pending_rename`. Keep calling `librarian-win` +(`copy_items`, `move_items`, `delete_to_recycle`, `rename`, `create_folder`) from +here — that's the right layering. +*Verify:* create folder (+ auto-rename), rename, delete-to-recycle, cut/copy/paste, +open — and the listing auto-refreshes after each (`OpFinished`). + +**T10. Extract `navload.rs` (navigation + async loading + dir watch + tree build).** +Move `load_current`/`navigate`/reset helpers (~`1754–1831`), the load arm bodies +(`ThisPcLoaded`, `WslLoaded`, `Loaded`, `Reloaded`, `DirChanged`, ~`1192–1313`), +`watch_stream` (~`3684–3725`), and the tree-building free fns + factories +(`fetch_tree_roots`, `fetch_tree_children`, `home/this_pc/wsl_tree_child`, +`tree_child_from_*`, ~`3444–3542`) plus tree arm bodies (`TreeToggle`, +`TreeNavigate`, `TreeChildrenLoaded`, `PaneResized`, ~`1432–1473`) and +`load_tree_roots`/`load_tree_children`/`drive_reveal`. +*This is the biggest stateful ticket — consider splitting into T10a (folder load + +watch) and T10b (tree build/reveal) if it feels too large.* +*Verify:* navigate via address bar, tree clicks, back/forward/up; This PC + WSL +listings; external file changes auto-refresh; revealing a deep path expands the +tree. + +--- + +## Phase 4 — `tree.rs` (test relocation only) + +**T11. Move `tree.rs` tests into `src/tree/tests.rs`.** +`tree.rs` logic (lines 1–371) is already SRP-clean — **leave it alone.** Only the +275-line `#[cfg(test)] mod tests` block (372–646) inflates it. Replace that block +in `tree.rs` with `#[cfg(test)] mod tests;`, create `src/tree/tests.rs`, paste the +test bodies there with `use super::*;` at the top. (A file-module `tree.rs` can own +a child module file at `src/tree/tests.rs` — no `mod.rs` needed.) +*Verify:* `cargo test -p librarian-app` runs the same tree tests, all green. + +--- + +## Phase 5 — DRY consolidation (separate final phase) + +Now that concerns live in the right modules, remove the repeated patterns the +analysis found. **These change code paths**, so do them one at a time with a build ++ run between each. Each becomes a small helper on `Librarian` (or a free fn) +placed in the owning module. + +**T12. `recompute_rows` + status string (4×).** Sites: load/reload arms +(~`1196`, `1205`, `1221`, `1289`). Extract `fn refresh_listing_status(&mut self)` +that does `recompute_rows()` + `self.status = format!("{n} items")`. + +**T13. Selection preserve/restore (2×).** Sites ~`1154`, `1288`. Extract +`fn recompute_preserving_selection(&mut self)` wrapping +`selected_paths` → `recompute_rows` → `restore_selection`. + +**T14. Icon+thumbnail warm batches (5×).** Sites ~`1095`, `1158`, `1178`, `1199`, +`1224`. Extract small helpers like `warm_content(lock)` / `warm_after_search()` +returning the `Task::batch([...])` so handlers read intent, not wiring. (Land this +in `grid_session.rs`.) + +**T15. File-op status strings (3×).** Sites ~`1582/1590/1598`. Extract +`fn op_label(verb: &str, count: usize) -> String` (uses existing `plural`). Lives +in `fileops.rs`. + +**T16. Icon-or-placeholder render (2×).** Sites `view_tree_row` ~`2469`, `view_row` +~`2766`. Extract `fn icon_element(&self, key, size) -> Element<'_, Message>`. + +**T17. Inline rename field (2×).** Sites `view_row` ~`2775`, `view_tile` ~`2923`. +Extract `fn rename_field(value) -> Element` (free fn; uses `RENAME_ID`, +`RenameChanged`, `RenameCommit`). + +**T18. Tree-child factories (3×).** `tree_child_from_drive/distro/known` +(~`3527–3542`) are identical `row → TreeChild::lazy(label, icon, target)`. Collapse +to one `tree_child_from_row(row: Row) -> TreeChild`. Lives in `navload.rs`. + +**T19. (Optional / stretch) Async token-supersession guard (9×).** The +`if token != self.X_token { return Task::none() }` pattern recurs in every async +arm. A macro or `fn is_stale(token, current) -> bool` helper trims it, but each +arm checks a *different* token field — only worth it if it reads cleaner to you. +Skip if it feels forced. + +--- + +## Global verification + +After **each** ticket: +1. `cargo build -p librarian-app` — clean. +2. `cargo clippy --workspace` — no new warnings vs. the T0 baseline. +3. `cargo test --workspace` — all green (esp. after T3, T10, T11). +4. `cargo run -p librarian-app` — exercise the area the ticket touched (see each + ticket's *Verify*). +5. `git commit` — one commit per ticket, so any regression is trivially bisectable. + +At the end: `main.rs` should be ~600–800 lines and read as *struct + Message + +dispatch*, with each feature's logic one `mod` away. Confirm the full app still +behaves identically to the T0 baseline (navigation, tabs, search, grid/details, +file ops, rename, tree reveal, WSL, external-change refresh). + +--- + +## Suggested order & dependencies + +`T0 → T1 → T2 → T3` (independent leaf moves; T1 before Phase 2) `→ T4 → T5 → T6` +(views; need T1/T3) `→ T7 → T8 → T9 → T10` (stateful; independent of each other, +any order) `→ T11` (tree tests; independent — can be done anytime) `→ Phase 5` +(T12–T19; after their owning module exists). + +Lowest-risk first stop if you want a quick early win and a feel for the pattern: +**T1 (styles)** removes ~400 lines with zero behavioral risk.