Fix file explorer navigation#17
Conversation
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Pull request overview
This PR refactors the file explorer’s “go to parent directory” navigation to be explicit and reusable, and adds a more natural Backspace keybinding while improving UI hints.
Changes:
- Introduces a
FileEntryenum (includingParentDir) and replaces the priorends_with("..")logic. - Extracts parent navigation into
go_up()and binds it toBackspacein the file explorer input handler. - Updates status bar hints and refreshes UI snapshots accordingly.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
src/components/file_explorer.rs |
Refactors entries to FileEntry, adds go_up() + Backspace handling, updates rendering/tests. |
src/components/status_bar.rs |
Adds a “⌫ Go up” hint when the file explorer is active. |
src/snapshots/pdm__ui__tests__bitcoin_screen_render.snap |
Snapshot metadata update reflecting UI changes. |
src/snapshots/pdm__ui__tests__p2pool_screen_render.snap |
Snapshot metadata update reflecting UI changes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| pub struct FileExplorer { | ||
| /// Current directory being explored. | ||
| pub current_dir: PathBuf, | ||
| /// Sorted list of files and folders in `current_dir`. | ||
| pub files: Vec<PathBuf>, | ||
| /// Sorted list of entries in `current_dir`. | ||
| /// Directories appear before files; `ParentDir` is always first when present. | ||
| pub file_entries: Vec<FileEntry>, | ||
| /// Index of the currently selected item. | ||
| pub selected_index: usize, |
There was a problem hiding this comment.
FileExplorer is exported via pub mod components and its fields are pub. Renaming/changing files: Vec<PathBuf> to file_entries: Vec<FileEntry> is a breaking public API change for downstream crates. If this type isn’t intended to be part of the crate’s stable API, consider reducing visibility (pub(crate)/private fields) and exposing accessors instead, or keep a backwards-compatible files() accessor/deprecated field to ease migration.
| pub fn select(&mut self) -> Option<PathBuf> { | ||
| match self.file_entries.get(self.selected_index)?.clone() { | ||
| FileEntry::ParentDir => { | ||
| self.go_up(); | ||
| None | ||
| } | ||
| FileEntry::Directory(path) => { | ||
| self.current_dir = path; | ||
| self.load_directory(); | ||
| None | ||
| } | ||
| } else if selected.is_dir() { | ||
| self.current_dir = selected; | ||
| self.load_directory(); | ||
| } else { | ||
| return Some(selected); | ||
| FileEntry::File(path) => Some(path), | ||
| } |
There was a problem hiding this comment.
select() clones the entire FileEntry (.get(..)?.clone()), which unnecessarily allocates/clones PathBufs for directory selections. You can match on a reference and only clone the PathBuf when you need to assign/return it (e.g., clone only the selected path).
| KeyCode::Backspace => { | ||
| self.go_up(); | ||
| AppAction::None | ||
| } |
There was a problem hiding this comment.
The new Backspace keybinding is introduced here, but there isn’t a test exercising handle_input(KeyCode::Backspace) to confirm it triggers parent navigation and keeps the explorer open. Adding a small unit test would help prevent regressions in this user-facing behavior.
| fn loads_directory_entries() { | ||
| let dir = setup_temp_fs(); | ||
| let mut explorer = FileExplorer { | ||
| current_dir: dir, | ||
| files: vec![], | ||
| file_entries: vec![], | ||
| selected_index: 0, | ||
| }; | ||
|
|
||
| explorer.load_directory(); | ||
| assert!(explorer.files.len() >= 2); | ||
| // Expects at least the "folder" dir and "file.txt" created in setup | ||
| assert!(explorer.file_entries.len() >= 2); |
There was a problem hiding this comment.
These tests create real directories under std::env::temp_dir() via setup_temp_fs() but never clean them up, which can leave residue on developer machines/CI runners over time. Since the repo already uses tempfile::TempDir elsewhere (e.g., src/main.rs tests), consider switching this test helper to use tempfile so the directories are removed automatically.
There was a problem hiding this comment.
yup, use tempfile in tests
|
Improved in #19 |
Improve file explorer parent navigation
Parent navigation already worked via selecting .., but the logic was embedded inside select(). This PR cleans that up and improves usability.
Extracted the logic into a reusable go_up() method
Bound Backspace to go_up() for a more natural UX
Replaced fragile ends_with("..") with a proper FileEntry::ParentDir enum
Updated status bar with ⌫ Go up and removed duplicate config hints
Demo
Backspace navigation (go_up) in file explorer.
