fix: replace expect() on settings save in network chooser with warnings#623
fix: replace expect() on settings save in network chooser with warnings#623thepastaclaw wants to merge 3 commits intodashpay:v1.0-devfrom
Conversation
Replace 3 panicking expect() calls on self.save() in the network chooser screen with if-let-Err + tracing::warn. A failure to persist a UI setting should not crash the application. Cherry-picked from ralph/improvements (commit 73452b6).
📝 WalkthroughWalkthroughThe pull request improves error handling and state management in the network chooser screen. When users select or clear Dash-Qt paths or toggle the dash.conf overwrite setting, the code now preserves the previous value and reverts to it if the save operation fails, while displaying user-facing error messages. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/ui/network_chooser_screen.rs (1)
891-894: New user-facing save-failure messages bypass theMessageBannercomponent.The three new error strings set by this PR are displayed via a bespoke inline
Frame(lines 940–957) rather than theMessageBannercomponent, which is the required approach for user-facing messages insrc/ui/**/*.rs. The outerisland_central_panelcall at line 2054 already provides the correct hook for global banners.As per coding guidelines: "User-facing messages must use the
MessageBannercomponent fromsrc/ui/components/message_banner.rs, with global banners rendered byisland_central_panel()."Also applies to: 919-922, 979-982
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/network_chooser_screen.rs` around lines 891 - 894, Replace the inline Frame-based user-facing error strings with the MessageBanner flow: stop setting self.custom_dash_qt_error_message directly to raw strings and instead expose an error state that island_central_panel will render via the MessageBanner component (use the existing MessageBanner in src/ui/components/message_banner.rs); update where the three new error messages are set (the places that assign custom_dash_qt_error_message and the similar fields at the other two locations) to set a unified banner error state (or call a helper that creates the banner message) and ensure island_central_panel reads that state to render MessageBanner so all global user-facing messages go through MessageBanner instead of an inline Frame.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/ui/network_chooser_screen.rs`:
- Line 913: The Clear button guard uses self.custom_dash_qt_path.is_some() which
stays true after clearing because you set it to Some(PathBuf::new()); change the
Clear button condition to match the path display check by ensuring the concrete
PathBuf is non-empty (e.g. compute the PathBuf used for display from
self.custom_dash_qt_path and guard on !file.as_os_str().is_empty()). Update the
same logic where the button is created (referencing self.custom_dash_qt_path and
the save() call) so the Clear button is only shown when an actual path is
present and save() is only invoked when clearing a non-empty path.
- Around line 971-984: The checkbox handler currently clears and reuses
custom_dash_qt_error_message causing unrelated Dash-Qt path errors to disappear
and placing the message far from the checkbox; add a new field
overwrite_dash_conf_error_message: Option<String> on the same struct, stop
clearing custom_dash_qt_error_message in the StyledCheckbox click branch, set
overwrite_dash_conf_error_message (not custom_dash_qt_error_message) when save()
returns Err(e), and render overwrite_dash_conf_error_message immediately after
the "Overwrite dash.conf" checkbox so the error message is colocated with the
control; keep the existing previous_overwrite_dash_conf rollback behavior if
save fails (refer to self.overwrite_dash_conf, self.save(), and
previous_overwrite_dash_conf).
---
Nitpick comments:
In `@src/ui/network_chooser_screen.rs`:
- Around line 891-894: Replace the inline Frame-based user-facing error strings
with the MessageBanner flow: stop setting self.custom_dash_qt_error_message
directly to raw strings and instead expose an error state that
island_central_panel will render via the MessageBanner component (use the
existing MessageBanner in src/ui/components/message_banner.rs); update where the
three new error messages are set (the places that assign
custom_dash_qt_error_message and the similar fields at the other two locations)
to set a unified banner error state (or call a helper that creates the banner
message) and ensure island_central_panel reads that state to render
MessageBanner so all global user-facing messages go through MessageBanner
instead of an inline Frame.
| @@ -904,24 +911,33 @@ impl NetworkChooserScreen { | |||
| } | |||
|
|
|||
| if self.custom_dash_qt_path.is_some() && ui.button("Clear").clicked() { | |||
There was a problem hiding this comment.
"Clear" button remains visible after a successful clear due to condition mismatch.
Line 913 guards the "Clear" button with self.custom_dash_qt_path.is_some(), but after a successful clear custom_dash_qt_path is set to Some(PathBuf::new()), which still satisfies is_some(). Meanwhile, the path display at lines 928–929 correctly gates on !file.as_os_str().is_empty() and shows nothing. The result is a "Clear" button with no path shown — pressing it again is a silent no-op that still triggers a save() call.
Align the button guard with the display guard:
🛠 Proposed fix
- if self.custom_dash_qt_path.is_some() && ui.button("Clear").clicked() {
+ if self.custom_dash_qt_path.as_ref().is_some_and(|p| !p.as_os_str().is_empty()) && ui.button("Clear").clicked() {Also applies to: 928-929
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/ui/network_chooser_screen.rs` at line 913, The Clear button guard uses
self.custom_dash_qt_path.is_some() which stays true after clearing because you
set it to Some(PathBuf::new()); change the Clear button condition to match the
path display check by ensuring the concrete PathBuf is non-empty (e.g. compute
the PathBuf used for display from self.custom_dash_qt_path and guard on
!file.as_os_str().is_empty()). Update the same logic where the button is created
(referencing self.custom_dash_qt_path and the save() call) so the Clear button
is only shown when an actual path is present and save() is only invoked when
clearing a non-empty path.
| let previous_overwrite_dash_conf = self.overwrite_dash_conf; | ||
| if StyledCheckbox::new(&mut self.overwrite_dash_conf, "Overwrite dash.conf") | ||
| .show(ui) | ||
| .clicked() | ||
| { | ||
| self.save().expect("Expected to save db settings"); | ||
| self.custom_dash_qt_error_message = None; | ||
| if let Err(e) = self.save() { | ||
| tracing::warn!("Failed to save overwrite_dash_conf setting: {}", e); | ||
| self.custom_dash_qt_error_message = Some( | ||
| "Failed to save overwrite dash.conf setting. Please try again." | ||
| .to_string(), | ||
| ); | ||
| self.overwrite_dash_conf = previous_overwrite_dash_conf; | ||
| } |
There was a problem hiding this comment.
overwrite_dash_conf save errors appear in the wrong section and clear unrelated errors.
custom_dash_qt_error_message is rendered inside the "Dash Core Executable Path" section (lines 940–957), which sits above the "Configuration Options" separator. When the overwrite_dash_conf save fails and sets custom_dash_qt_error_message (line 979), the error banner appears far above the checkbox that triggered it, making the cause unclear to the user. Additionally, line 976 clears any pre-existing Dash-Qt path error the moment the user clicks this checkbox, even if that error is still relevant.
Consider a dedicated overwrite_dash_conf_error_message: Option<String> field rendered immediately after the checkbox, keeping errors co-located with their source.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/ui/network_chooser_screen.rs` around lines 971 - 984, The checkbox
handler currently clears and reuses custom_dash_qt_error_message causing
unrelated Dash-Qt path errors to disappear and placing the message far from the
checkbox; add a new field overwrite_dash_conf_error_message: Option<String> on
the same struct, stop clearing custom_dash_qt_error_message in the
StyledCheckbox click branch, set overwrite_dash_conf_error_message (not
custom_dash_qt_error_message) when save() returns Err(e), and render
overwrite_dash_conf_error_message immediately after the "Overwrite dash.conf"
checkbox so the error message is colocated with the control; keep the existing
previous_overwrite_dash_conf rollback behavior if save fails (refer to
self.overwrite_dash_conf, self.save(), and previous_overwrite_dash_conf).
1 participant
Replace 3 panicking
expect()calls onself.save()in the network chooser screen withif let Err+tracing::warn.Problem
The network chooser screen calls
self.save().expect("Expected to save db settings")in three places — when setting the Dash-Qt path, clearing it, and toggling the overwrite-dash-conf checkbox. If the save fails (e.g., disk full, permissions), the entire application panics.Fix
Replace each
expect()withif let Err(e) = self.save()and log a warning. A failure to persist a UI preference should degrade gracefully, not crash the app.Cherry-picked from
ralph/improvements(commit 73452b6).Validation
What was tested:
cargo clippy --all-features --all-targets -- -D warnings— lint check confirming no remainingexpect()on save pathscargo test --all-features --workspace— full workspace test suiteResults:
ClippyCI check — pass (5m24s)Test SuiteCI check — pass (7m32s)Environment: Local macOS arm64; GitHub Actions CI (ubuntu-latest)
Summary by CodeRabbit
Bug Fixes
Improvements