Add a Settings view#21
Conversation
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Pull request overview
Adds a new Settings screen to the TUI and introduces persisted user settings (TOML on disk) so config file paths can be remembered across runs.
Changes:
- Introduce
SettingsViewUI + sidebar item and route rendering/input handling for the new screen. - Add
settingsmodule for loading/savingsettings.toml, and bootstrap app state from persisted paths at startup. - Update status bar hints and snapshot/UI tests to cover the new sidebar item and Settings screen rendering.
Reviewed changes
Copilot reviewed 37 out of 38 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/snapshots/ui_snapshots__home_screen_render.snap | Snapshot update to include the new “Settings” sidebar item. |
| tests/snapshots/ui_snapshots__config_screen_render.snap | Snapshot update to include the new “Settings” sidebar item. |
| src/ui.rs | Render routing updated to include CurrentScreen::Settings + new snapshot test. |
| src/main.rs | Loads settings on startup, adds Settings input handling, persists selected paths, expands action handling/tests. |
| src/settings.rs | New persisted settings module (Settings struct + TOML load/save + tests). |
| src/lib.rs | Exposes new settings module publicly. |
| src/app.rs | Adds CurrentScreen::Settings, ExplorerTrigger, and stores settings/settings_view in App. |
| src/components/status_bar.rs | Adds Settings-specific status bar hints and error display. |
| src/components/settings_view.rs | New Settings screen component: field list rendering + key handling. |
| src/components/file_explorer.rs | Minor rendering/title formatting changes + render test. |
| src/components/bitcoin_config_view.rs | Path shortening refactor + minor render correctness improvements + extra render test. |
| src/components/bitcoin_status_view.rs | Adds compile-time assertion for tab count + #[must_use] on constructor. |
| src/components/home_view.rs | Adds #[must_use] on constructor. |
| src/components/ln_config_view.rs | Adds #[must_use] on constructor. |
| src/components/ln_status_view.rs | Adds #[must_use] on constructor. |
| src/components/p2pool_config_view.rs | Adds #[must_use] on constructor. |
| src/components/p2pool_status_view.rs | Adds #[must_use] on constructor. |
| src/components/shares_market_view.rs | Adds #[must_use] on constructor; fixes stale comment label. |
| src/components/mod.rs | Exports settings_view module. |
| src/bitcoin_config.rs | Adds docs/annotations and refactors parsing fallbacks; adds test for malformed INI fallback behavior. |
| src/snapshots/pdm__ui__tests__shares_market_screen_render.snap | Snapshot update for new sidebar item. |
| src/snapshots/pdm__ui__tests__settings_screen_render.snap | New snapshot for Settings screen rendering. |
| src/snapshots/pdm__ui__tests__p2pool_status_screen_render.snap | Snapshot update for new sidebar item + assertion line metadata. |
| src/snapshots/pdm__ui__tests__p2pool_screen_render.snap | New snapshot for P2Pool screen rendering (with updated sidebar). |
| src/snapshots/pdm__ui__tests__p2pool_config_screen_render.snap | Snapshot update for new sidebar item + assertion line metadata. |
| src/snapshots/pdm__ui__tests__ln_status_screen_render.snap | Snapshot update for new sidebar item + assertion line metadata. |
| src/snapshots/pdm__ui__tests__ln_config_screen_render.snap | Snapshot update for new sidebar item + assertion line metadata. |
| src/snapshots/pdm__ui__tests__home_screen_render.snap | Snapshot update for new sidebar item + assertion line metadata. |
| src/snapshots/pdm__ui__tests__bitcoin_status_tab_system_render.snap | Snapshot update for new sidebar item. |
| src/snapshots/pdm__ui__tests__bitcoin_status_tab_peers_render.snap | Snapshot update for new sidebar item. |
| src/snapshots/pdm__ui__tests__bitcoin_status_tab_logs_render.snap | Snapshot update for new sidebar item. |
| src/snapshots/pdm__ui__tests__bitcoin_status_screen_render.snap | Snapshot update for new sidebar item. |
| src/snapshots/pdm__ui__tests__bitcoin_screen_render.snap | New snapshot for Bitcoin Config screen rendering (with updated sidebar). |
| src/snapshots/pdm__ui__tests__bitcoin_config_screen_render.snap | Snapshot update for new sidebar item + assertion line metadata. |
| src/snapshots/pdm__tests__menu_toggled.snap | Snapshot update for new sidebar item. |
| src/snapshots/pdm__tests__home_screen.snap | Snapshot update for new sidebar item. |
| Cargo.toml | Adds directories, serde, toml, serial_test; bumps ratatui to 0.30.0. |
| Cargo.lock | Lockfile updates reflecting new deps and the ratatui upgrade. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if FIELDS[self.selected_index].1 == FieldKind::FilePicker { | ||
| AppAction::OpenExplorerForSettings(self.selected_index) | ||
| } else { | ||
| AppAction::None | ||
| } |
There was a problem hiding this comment.
The DirectoryInput path (e.g. “Settings directory override”) is currently unconfigurable: on KeyCode::Enter, only FilePicker fields open the explorer and all other fields return AppAction::None, and there’s no text-entry mode. Either implement an input flow for DirectoryInput (editable text field / directory picker) or remove/disable this field to avoid presenting an option that can’t be set.
| match field { | ||
| 0 => app.settings.bitcoin_conf_path = Some(path.clone()), | ||
| 1 => app.settings.p2pool_conf_path = Some(path.clone()), | ||
| 2 => app.settings.ln_conf_path = Some(path.clone()), | ||
| 3 => app.settings.shares_market_conf_path = Some(path.clone()), | ||
| _ => {} | ||
| } | ||
| if let Err(e) = save_settings(&app.settings) { |
There was a problem hiding this comment.
ExplorerTrigger::Settings(field) only updates app.settings.* and saves it. It does not update the active in-memory state used by other screens (e.g., app.bitcoin_conf_path/app.bitcoin_data, app.p2pool_conf_path/app.p2pool_config), so changes made in Settings won’t take effect until restart (or re-selecting in the other screen). Consider reusing the same validation/loading logic here so Settings changes apply immediately.
| match field { | |
| 0 => app.settings.bitcoin_conf_path = Some(path.clone()), | |
| 1 => app.settings.p2pool_conf_path = Some(path.clone()), | |
| 2 => app.settings.ln_conf_path = Some(path.clone()), | |
| 3 => app.settings.shares_market_conf_path = Some(path.clone()), | |
| _ => {} | |
| } | |
| if let Err(e) = save_settings(&app.settings) { | |
| let mut should_save_settings = true; | |
| match field { | |
| 0 => match parse_bitcoin_config(&path) { | |
| Ok(entries) => { | |
| const MIN_KNOWN_KEYS: usize = 1; | |
| let known_key_count = entries | |
| .iter() | |
| .filter(|e| e.enabled && e.schema.is_some()) | |
| .count(); | |
| if known_key_count >= MIN_KNOWN_KEYS { | |
| app.bitcoin_conf_path = Some(path.clone()); | |
| app.bitcoin_data = entries; | |
| app.bitcoin_config_view.selected_index = 0; | |
| app.bitcoin_config_view.dirty = false; | |
| app.bitcoin_config_view.sidebar_focused = false; | |
| app.bitcoin_config_view.warning_message = None; | |
| app.settings.bitcoin_conf_path = Some(path.clone()); | |
| app.settings_view.save_error = None; | |
| } else { | |
| should_save_settings = false; | |
| app.settings_view.save_error = Some( | |
| "Selected file does not appear to be a Bitcoin config." | |
| .to_string(), | |
| ); | |
| } | |
| } | |
| Err(e) => { | |
| should_save_settings = false; | |
| app.settings_view.save_error = Some(format!( | |
| "Failed to read Bitcoin config: {e}. Check permissions and try again." | |
| )); | |
| } | |
| }, | |
| 1 => { | |
| app.p2pool_conf_path = Some(path.clone()); | |
| if let Some(p) = path.to_str() | |
| && let Ok(cfg) = P2PoolConfig::load(p) | |
| { | |
| app.p2pool_config = Some(cfg); | |
| } | |
| app.settings.p2pool_conf_path = Some(path.clone()); | |
| app.settings_view.save_error = None; | |
| } | |
| 2 => { | |
| app.settings.ln_conf_path = Some(path.clone()); | |
| app.settings_view.save_error = None; | |
| } | |
| 3 => { | |
| app.settings.shares_market_conf_path = Some(path.clone()); | |
| app.settings_view.save_error = None; | |
| } | |
| _ => {} | |
| } | |
| if should_save_settings | |
| && let Err(e) = save_settings(&app.settings) | |
| { |
| app.settings.bitcoin_conf_path = Some(path.clone()); | ||
| if let Err(e) = save_settings(&app.settings) { | ||
| app.settings_view.save_error = | ||
| Some(format!("Save failed: {e}")); | ||
| } |
There was a problem hiding this comment.
save_error is only set on failure here, but it’s never cleared on a subsequent successful save_settings call. If a save fails once, the error will keep showing in the Settings status bar even after later saves succeed. Consider clearing app.settings_view.save_error before attempting the save, or explicitly clearing it on success (and apply the same pattern to the other save_settings call sites).
| 0 => app.settings.bitcoin_conf_path = None, | ||
| 1 => app.settings.p2pool_conf_path = None, |
There was a problem hiding this comment.
ClearSettingsField clears the persisted app.settings.* value but doesn’t clear the corresponding active runtime state (e.g., app.bitcoin_conf_path/app.bitcoin_data, app.p2pool_conf_path/app.p2pool_config). This can leave Settings showing “(not set)” while other screens still operate on the previously loaded paths. Consider also clearing/invalidating the related app.* fields here (or explicitly documenting that it only affects the next restart).
| 0 => app.settings.bitcoin_conf_path = None, | |
| 1 => app.settings.p2pool_conf_path = None, | |
| 0 => { | |
| app.settings.bitcoin_conf_path = None; | |
| app.bitcoin_conf_path = None; | |
| app.bitcoin_data.clear(); | |
| } | |
| 1 => { | |
| app.settings.p2pool_conf_path = None; | |
| app.p2pool_conf_path = None; | |
| app.p2pool_config = P2PoolConfig::default(); | |
| } |
| app.settings_view.save_error = | ||
| Some(format!("Save failed: {e}")); |
There was a problem hiding this comment.
On save_settings failure during Bitcoin config selection, the error is stored in app.settings_view.save_error, but the status bar only displays that value on CurrentScreen::Settings. As a result, the user gets no feedback that persistence failed unless they navigate to Settings. Consider surfacing the error on the current screen (e.g., via bitcoin_config_view.warning_message / a generic status bar error), or logging it explicitly.
| app.settings_view.save_error = | |
| Some(format!("Save failed: {e}")); | |
| let save_error = format!("Save failed: {e}"); | |
| app.settings_view.save_error = Some(save_error.clone()); | |
| app.bitcoin_config_view.warning_message = Some(save_error); |
| fn save_settings_public_fn_creates_file() { | ||
| let dir = tempfile::tempdir().unwrap(); | ||
| // Point save_settings at a path we control by writing directly |
There was a problem hiding this comment.
Similar to the other settings tests: this one doesn’t call save_settings() at all (it serializes and writes the file directly), so it isn’t verifying that save_settings() creates the config directory and writes settings.toml. Suggest either rewriting it to call save_settings() (with PDM_CONFIG_DIR redirected) or renaming the test to avoid implying coverage it doesn’t provide.
| fn save_settings_public_fn_creates_file() { | |
| let dir = tempfile::tempdir().unwrap(); | |
| // Point save_settings at a path we control by writing directly | |
| fn serialized_settings_can_be_written_to_file() { | |
| let dir = tempfile::tempdir().unwrap(); | |
| // Write the serialized settings directly to a controlled path. |
| /// If set, the user has chosen a custom settings directory. | ||
| /// Takes effect on restart. | ||
| pub settings_dir_override: Option<PathBuf>, |
There was a problem hiding this comment.
settings_dir_override is documented as “takes effect on restart”, but nothing in this module consults it when resolving where settings.toml lives (see config_dir() below). As implemented, the override can never actually take effect even if the value were set. Consider either removing this field for now, or implementing the restart behavior by incorporating the override into config_dir() resolution with a safe fallback.
| fn save_and_load_via_public_functions() { | ||
| // Use a temp dir as the config dir by writing directly then calling load_settings | ||
| // via the file path rather than the env-var route (avoids unsafe set_var in 2024 edition) | ||
| let dir = tempfile::tempdir().unwrap(); | ||
| let path = dir.path().join("settings.toml"); |
There was a problem hiding this comment.
This test is named as if it exercises the public save_settings/load_settings functions, but the body only writes/reads TOML directly and never calls those functions. That won’t catch regressions in config_dir() resolution, directory creation, or the error-handling behavior of load_settings(). Consider rewriting the test to call save_settings() + load_settings() (using PDM_CONFIG_DIR + #[serial] if needed), or renaming it to reflect what it actually tests.
pool2win
left a comment
pool2win
left a comment
There was a problem hiding this comment.
Nice one. I was able to change the settings and save them in the settings location. On restart everything was there.
Add a Settings View where the user can set:
Bitcoin Config file Path,
LN Config,
P2PoolV2 Config file path