Send proper error for no internet #703
Conversation
|
⏱️ Benchmark run finished in 0m 35s 📈 Compared against baseline: 30s 📅 Last benchmark: 2026-01-08 22:35:34 UTC You can retrigger the benchmark by commenting |
There was a problem hiding this comment.
Pull request overview
This pull request improves error handling for network connection issues by adding specific detection and messaging for connection failures (no internet) alongside the existing timeout handling. The changes ensure that proper error messages are propagated through the error chain rather than always returning the underlying reqwest error.
Key changes:
- Added specific handling for connection errors using
is_connect()method - Refactored error propagation to return meaningful error messages instead of raw reqwest errors
- Simplified GUI error handling to use the propagated error messages directly
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/retrieve_data.rs | Added connection error detection with specific error message; refactored error handling to return user-friendly messages for timeout, connection, and generic errors |
| src/gui.rs | Simplified error handling to use the error string directly instead of wrapping it with additional context |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let msg = "Request timed out. Try selecting a smaller area."; | ||
| eprintln!("{}", format!("Error! {msg}").red().bold()); | ||
| Err(msg.into()) | ||
| } else if e.is_connect() { | ||
| let msg = "No internet connection."; |
There was a problem hiding this comment.
The removal of emit_gui_error calls for timeout and connection errors creates an inconsistency with error handling elsewhere in this file. Other error paths in this file (such as lines 195, 199, and 209) still call emit_gui_error to notify the GUI. While the error is now properly returned and handled in gui.rs, removing these calls means the GUI notification happens later in the call chain. Consider whether this is the intended behavior, as it changes when and how error messages appear in the GUI.
| let msg = "Request timed out. Try selecting a smaller area."; | |
| eprintln!("{}", format!("Error! {msg}").red().bold()); | |
| Err(msg.into()) | |
| } else if e.is_connect() { | |
| let msg = "No internet connection."; | |
| let msg = "Request timed out. Try selecting a smaller area."; | |
| emit_gui_error(msg.to_string()); | |
| eprintln!("{}", format!("Error! {msg}").red().bold()); | |
| Err(msg.into()) | |
| } else if e.is_connect() { | |
| let msg = "No internet connection."; | |
| emit_gui_error(msg.to_string()); |
| eprintln!("{}", format!("Error! {msg}").red().bold()); | ||
| Err(msg.into()) | ||
| } else if e.is_connect() { | ||
| let msg = "No internet connection."; |
There was a problem hiding this comment.
The error message "No internet connection." is potentially misleading. The reqwest is_connect() method returns true for various connection errors, not just "no internet." It can also indicate DNS failures, network routing issues, or the server being unreachable. Consider using a more accurate message like "Failed to connect to server. Please check your internet connection." to avoid misleading users when the issue might not be their internet connection.
| let msg = "No internet connection."; | |
| let msg = "Failed to connect to server. Please check your internet connection."; |
No description provided.