feat(settings): add file tree visibility setting

[msgongora]

Summary

• Add show_file_tree config support, defaulting to true.
• Allow starting with the file tree hidden while preserving H as the runtime toggle.
• Expose file tree visibility under Settings > Panels.

Test plan

• Manually tested on macOS
• cargo fmt --all
• cargo clippy --all-targets --all-features -- -D warnings
• cargo test --all-targets

Related Issues

#14

Pending discussion

1. Behavioral test coverage. The serde round-trip tests are good, but the interesting logic — lazy discovery, initial focus when hidden, the settings-popup cursor offsets, and the H-after-hidden path — isn't covered yet. I'd like to add tests there.
2. Settings popup state sync. The "Show file tree" bullet reflects show_file_tree, so if you start visible, press H to hide, then open settings, it still reads as ON. Minor, but worth either a label tweak ("Show file tree at launch") or syncing the displayed state.

[leboiko]

Review — feat(settings): add file tree visibility setting

Thanks for the thoughtful PR, @msgongora! Three parallel review passes (architecture, code quality, testing). Overall this is a well-built, backward-compatible change — the show_file_tree default keeps existing users unaffected, the lazy ensure_tree_discovered() path is a genuinely nice perf touch, and the docs + settings modal are all updated. The config offset math (PANELS_ROWS 2→3) was traced by two agents and is correct — no off-by-one in the Search/Mermaid routing.

A few things to address before merge, none of them deep:

🟠 Major

1. Behavioral test coverage is the main gap. The 3 serde tests are good but only cover the TOML layer. The new behavior is untested: the lazy-discovery state machine, initial-focus-when-hidden, and the config-Esc focus redirect. Highest-value test to add is a cursor-offset guard for apply_config_selection — pin that cursor == panels_start toggles show_file_tree and cursor == search_start still toggles the Search preview. That constant (PANELS_ROWS = 3) is exactly the kind of mechanical value that silently mis-routes every setting below Panels if a future row is added without bumping it, and nothing currently guards it. See inline comments for the specific tests.
2. Config popup misreports live state. The "Show file tree" bullet renders show_file_tree (the persisted startup preference), not !tree_hidden (live visibility). Hide the tree with H, open settings → it still reads ON, and the first Enter on that row is a no-op-feeling double-toggle. Either drive the bullet from !tree_hidden, or relabel it "Show file tree at launch". (This is the popup-sync item we discussed on #14.)
3. Discovery spawn is duplicated. The spawn_blocking { discover → TreeDiscovered } body in the FilesChanged handler is byte-identical to ensure_tree_discovered's body. Extract a spawn_tree_discovery(&mut self) primitive and call it from both.

🟡 Minor

• ensure_tree_discovered sets tree_discovered = true after the let Some(tx) … else return guard, so with no action_tx (unit-test context) the flag never flips — worth moving the flag set above the guard for idempotency + testability.
• Redundant self.tree_discovered = true; in the TreeDiscovered handler (it's always already true at that point) — remove or comment why it's defensive.
• centered_rect(46, 35, …) — the 35 is a correct-but-undocumented magic number; derive it (build_lines().len() + 2) or note the 32-content + 2-border minimum.
• The config-popup "hide" path in apply_config_selection doesn't redirect focus off Focus::Tree the way the H handler does.

Out of scope (pre-existing — follow-up, NOT blocking this PR)

Several key handlers set self.focus = Focus::Tree with no tree_hidden guard (Tab in viewer key_handlers.rs:357, last-tab close :365, search-Esc :675, copy-menu :727/:731, mouse tab-close mod.rs:1113). These can strand focus on a hidden panel — but they pre-date this PR and were already reachable via the existing H toggle; this PR didn't introduce them. It does make the hidden-tree state a startup default, so they're now more reachable. Recommend a separate follow-up issue to add a focus_tree_or_viewer() helper and route all these through it. The eager FileEntry::discover() blocking call in App::new is likewise pre-existing and out of scope here.

Verdict

Solid work. Minimum bar to merge: the cursor-offset guard test + the lazy-discovery/focus tests (Major #1), and the popup state-sync fix (Major #2). The duplication and minors can ride along or follow up. I'll re-review once tests land. 🚀

[leboiko]

🟠 Major — popup misreports live visibility. This passes app.show_file_tree (the persisted startup preference), but the bullet the user reads represents current visibility. After a runtime H press, tree_hidden flips while show_file_tree stays put, so the popup shows "Show file tree ● ON" with the tree actually hidden — and the first Enter on that row feels like a no-op double-toggle.

Two clean options:

1. Drive the bullet from live state: pass !app.tree_hidden here (the Enter handler already syncs both fields, so behavior stays consistent), or
2. Keep the field but relabel the row "Show file tree at launch" so it's clearly a startup default, not a live toggle.

[leboiko]

Fixed in b816f56. The popup now reflects live visibility: ui/mod.rs passes file_tree_visible: !app.tree_hidden (param renamed from show_file_tree for clarity), and the toggle in apply_config_selection now flips tree_hidden directly and syncs show_file_tree = !tree_hidden. So the bullet always matches the panel, and the double-toggle-after-H is gone.

[leboiko]

🟠 Major — duplicated discovery spawn. This spawn_blocking { FileEntry::discover → send(TreeDiscovered) } block is byte-for-byte identical to the body of ensure_tree_discovered (around line 773). Any future change to discovery (different channel, error handling, cancellation) has to be made in two places.

Suggest extracting a primitive and calling it from both sites:

fn spawn_tree_discovery(&self) {
    let Some(tx) = self.action_tx.clone() else { return };
    let root = self.root.clone();
    tokio::task::spawn_blocking(move || {
        let _ = tx.send(Action::TreeDiscovered(FileEntry::discover(&root)));
    });
}

Then ensure_tree_discovered becomes the if !self.tree_discovered { self.tree_discovered = true; self.spawn_tree_discovery(); } wrapper.

[leboiko]

Fixed in b816f56. Extracted spawn_tree_discovery(&self) (clone tx + root, spawn_blocking the walk, send TreeDiscovered). Both ensure_tree_discovered and the FilesChanged handler now call it, so the walk logic lives in one place.

[leboiko]

🟡 Minor — flag set after the early return. self.tree_discovered = true; runs after the let Some(tx) … else { return }; guard, so when action_tx is None the flag never flips. In production action_tx is always wired by the time keys are handled, so this is benign — but it means the idempotency guarantee ("a second reveal won't spawn again") depends on tx being present, and it makes the path awkward to unit-test. Consider setting self.tree_discovered = true; before the tx guard so re-entry is idempotent regardless.

[leboiko]

Fixed in b816f56. ensure_tree_discovered now sets self.tree_discovered = true before delegating to spawn_tree_discovery(), so the latch flips regardless of whether action_tx is wired — repeated reveals are idempotent and unit-testable.

[leboiko]

🟡 Minor — redundant assignment. Action::TreeDiscovered is only ever sent from ensure_tree_discovered (which already set tree_discovered = true) and from the FilesChanged handler (gated on if self.tree_discovered). So tree_discovered is always true when this line runs. Either remove it, or add a comment explaining the defensive intent and which path could fire it as false.

[leboiko]

Fixed in b816f56. Removed the redundant assignment; the latch is always set before the walk is spawned, so it's guaranteed true by the time TreeDiscovered is handled. Left a one-line comment noting that.

[leboiko]

🟠 Major (test) — guard the cursor-offset routing. This panels_start / +1 / +2 dispatch (and the PANELS_ROWS = 3 constant behind it) is correct today, but it's mechanical and unguarded: if a future Panels row is added without bumping the constant, cursor == search_start silently toggles a Panels row instead of the Search preview — a no-warning mis-route of every section below Panels.

Add a guard test, e.g. apply_config_selection_routing_is_stable:

let theme = Theme::ALL.len();
let panels_start = theme + 1;          // +1 for the Markdown row
let search_start = panels_start + 3;   // 3 Panels rows after this PR
// panels_start toggles show_file_tree, NOT search
let mut a = App::new(".".into(), None, None);
let (sft, sp) = (a.show_file_tree, a.search_preview);
a.apply_config_selection(panels_start);
assert_ne!(a.show_file_tree, sft);
assert_eq!(a.search_preview, sp);
// search_start still pins to the Search preview
let mut b = App::new(".".into(), None, None);
b.apply_config_selection(search_start);
assert_eq!(b.search_preview, SearchPreview::FullLine);

Assertions pin exact values, so a regressed PANELS_ROWS fails loudly.

[leboiko]

Done in b816f56. Added config_selection_routing_pins_settings_to_their_sections, which pins that panels_start toggles tree visibility (not Search) and search_start/mermaid_start route to their own sections with exact expected values. Verified by mutation: reverting PANELS_ROWS to 2 makes the test fail loudly (search_preview stays Snippet instead of FullLine).

[leboiko]

🟡 Minor — hide-via-popup doesn't redirect focus. When this path hides the tree (show_file_tree → false ⇒ tree_hidden = true), focus may still be on Focus::Tree if the user Tab'd to the tree before opening settings. The H handler has the complementary guard (if self.tree_hidden && self.focus == Focus::Tree { self.focus = Focus::Viewer; }); mirror it here so the user isn't left on an invisible panel.

[leboiko]

Fixed in b816f56. The popup toggle's hide branch now mirrors the H handler: if self.focus == Focus::Tree { self.focus = Focus::Viewer; } so focus never lands on the hidden panel. Covered by config_toggle_syncs_visibility_focus_and_preference.

[leboiko]

🟡 Minor — undocumented magic height. The bump 30 → 35 is correct (content is ~32 lines + 2 borders = 34 min, 35 leaves one line of margin), but the value isn't derivable from the code, so the next person adding a section is guessing. Either derive it from build_lines(...).len() as u16 + 2, or add a one-line comment with the derivation (// 32 content + 2 borders = 34 min; 35 = +1 margin).

[leboiko]

Fixed in b816f56. The height is now derived: u16_sat(lines.len() + 2) (content rows + top/bottom border) clamped to f.area().height. Adding a section can no longer silently clip the popup, and the magic 35 is gone.

[leboiko]

✅ Nice — these three serde tests (missing-field default, explicit-false, round-trip) are exactly the right coverage for the config layer and match the existing house style. The gap is purely at the behavior layer (lazy discovery, focus, popup routing) — see the inline notes in app/mod.rs and key_handlers.rs and the summary for the specific behavioral tests to add.

[leboiko]

Thanks! Kept these as-is and added the behavioral layer on top: lazy-discovery state machine, popup toggle sync, H-doesn't-rewrite-startup-preference, and the cursor-offset guard — see b816f56. Also added a #[cfg(test)] config-dir override so the suite never reads or clobbers the real config.toml, and made Config::save() atomic.

[leboiko]

Review findings addressed in b816f56

#	Finding	Severity	Status
1	Popup misreports live visibility (stale bullet / double-toggle after H)	🟠 Major	Fixed — bullet driven by !tree_hidden; toggle flips visibility, syncs the persisted pref, redirects focus, ensures discovery
2	Behavioral test coverage gap	🟠 Major	Fixed — 5 behavioral tests incl. a mutation-verified cursor-offset guard
3	Duplicated discovery spawn	🟠 Major	Fixed — extracted spawn_tree_discovery(), reused in both sites
4	tree_discovered set after the action_tx guard	🟡 Minor	Fixed — latch set before spawn; idempotent reveals
5	Redundant tree_discovered = true	🟡 Minor	Fixed — removed
6	Magic popup height 35	🟡 Minor	Fixed — derived from line count, clamped to terminal
7	Hide-via-popup didn't redirect focus	🟡 Minor	Fixed — mirrors the H handler

Extra fixes found while implementing

• Atomic Config::save() — the previous fs::write was a non-atomic truncate-then-write, so a crash or a concurrent reader could observe a truncated/corrupt config. Now writes to a unique temp file and renames (atomic on the same FS). This was also the root cause of a flaky test (torn reads of the config file).
• Test config isolation — added a #[cfg(test)] config-dir override so the unit suite never reads from — or clobbers — the user's real config.toml. (Heads-up: before this, running the suite would overwrite the real config.)

Deferred (pre-existing — not introduced by this PR)

Several key handlers set focus = Focus::Tree without a tree_hidden guard (Tab in viewer, search-Esc, copy-menu, last-tab close, mouse tab-close), which can strand focus on a hidden panel. These predate this PR (already reachable via the existing H toggle); this PR just makes hidden-tree a startup default, raising their visibility. Tracking as a follow-up so this PR stays scoped.

Net: +240 / −31 across 6 files. cargo fmt, clippy -D warnings, cargo deny, and all 472 tests pass — and the suite is now deterministic over 25 consecutive runs.