fix: network chooser screen statuses#89
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe pull request introduces significant updates to the Changes
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
pauldelucia
changed the title
|
@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: 1
🧹 Outside diff range and nitpick comments (7)
src/backend_task/core/mod.rs (3)
53-74: Refactor duplicated code for fetching chain locks into a helper functionThe code for fetching the best chain lock for mainnet and testnet is nearly identical. To improve maintainability and reduce code duplication, consider creating a helper function that encapsulates this logic.
Here’s how you might refactor the code:
Create a helper function:
fn get_best_chain_lock_for_network( network_config: &Option<NetworkConfig>, network: Network, ) -> Result<ChainLock, String> { if let Some(config) = network_config { let addr = format!("http://{}:{}", config.core_host, config.core_rpc_port); let client = Client::new( &addr, Auth::UserPass( config.core_rpc_user.to_string(), config.core_rpc_password.to_string(), ), ) .map_err(|e| format!("Failed to create {:?} client: {}", network, e))?; client.get_best_chain_lock().map_err(|e| { format!( "Failed to get best chain lock for {:?}: {}", network, e.to_string() ) }) } else { Err(format!("{:?} config not found", network)) } }Update the
run_core_taskmethod:CoreTask::GetBestChainLocks => { tracing::info!("Getting best chain locks for testnet and mainnet"); // Load configs let config = match Config::load() { Ok(config) => config, Err(e) => { return Err(format!("Failed to load config: {}", e)); } }; - let maybe_mainnet_config = config.config_for_network(Network::Dash); - let maybe_testnet_config = config.config_for_network(Network::Testnet); - - // Get mainnet best chainlock - let mainnet_result = if let Some(mainnet_config) = maybe_mainnet_config { - // [Mainnet fetching logic...] - } else { - Err("Mainnet config not found".to_string()) - }; - - // Get testnet best chainlock - let testnet_result = if let Some(testnet_config) = maybe_testnet_config { - // [Testnet fetching logic...] - } else { - Err("Testnet config not found".to_string()) - }; + let mainnet_result = get_best_chain_lock_for_network( + &config.config_for_network(Network::Dash), + Network::Dash, + ); + let testnet_result = get_best_chain_lock_for_network( + &config.config_for_network(Network::Testnet), + Network::Testnet, + ); // Handle results match (mainnet_result, testnet_result) { (Ok(mainnet_chainlock), Ok(testnet_chainlock)) => { Ok(BackendTaskSuccessResult::CoreItem(CoreItem::ChainLocks( Some(mainnet_chainlock), Some(testnet_chainlock), ))) } (Ok(mainnet_chainlock), Err(_)) => Ok(BackendTaskSuccessResult::CoreItem( CoreItem::ChainLocks(Some(mainnet_chainlock), None), )), (Err(_), Ok(testnet_chainlock)) => Ok(BackendTaskSuccessResult::CoreItem( CoreItem::ChainLocks(None, Some(testnet_chainlock)), )), (Err(_), Err(_)) => Err( "Failed to get best chain lock for both mainnet and testnet".to_string(), ), } }This refactoring reduces code duplication and makes the codebase cleaner and easier to maintain.
Also applies to: 77-98
65-65: Include error details when failing to create mainnet clientCurrently, the error message when failing to create the mainnet client does not include the specific error, which can make debugging more difficult. Including the error details will aid in troubleshooting.
Apply this diff to include the error details:
let mainnet_client = Client::new( &mainnet_addr, Auth::UserPass( mainnet_config.core_rpc_user.to_string(), mainnet_config.core_rpc_password.to_string(), ), ) - .map_err(|_| "Failed to create mainnet client".to_string())?; + .map_err(|e| format!("Failed to create mainnet client: {}", e))?;
89-89: Include error details when failing to create testnet clientThe error message when failing to create the testnet client omits the specific error, hindering effective debugging. Including the error details will facilitate troubleshooting.
Apply this diff to include the error details:
let testnet_client = Client::new( &testnet_addr, Auth::UserPass( testnet_config.core_rpc_user.to_string(), testnet_config.core_rpc_password.to_string(), ), ) - .map_err(|_| "Failed to create testnet client".to_string())?; + .map_err(|e| format!("Failed to create testnet client: {}", e))?;src/ui/network_chooser_screen.rs (4)
164-167: Consider defining status colors as constants for maintainabilityDefining
ONLINE_COLORandOFFLINE_COLORas constants improves readability and makes future changes easier.Apply this diff to define the colors as constants:
+const ONLINE_COLOR: Color32 = Color32::from_rgb(0, 255, 0); // Green +const OFFLINE_COLOR: Color32 = Color32::from_rgb(255, 0, 0); // Red fn render_network_row(&mut self, ui: &mut Ui, network: Network, name: &str) -> AppAction { // Check network status let is_working = self.check_network_status(network); let status_color = if is_working { - Color32::from_rgb(0, 255, 0) // Green if working + ONLINE_COLOR } else { - Color32::from_rgb(255, 0, 0) // Red if not working + OFFLINE_COLOR };
Line range hint
207-211: Refactor repeated time calculations into a helper methodThe time calculation for
self.recheck_timeis repeated. Creating a helper method enhances code maintainability.Suggested refactor:
fn schedule_recheck_in(&mut self, secs: u64) { self.recheck_time = Some( (SystemTime::now() .duration_since(UNIX_EPOCH) .expect("Time went backwards") + Duration::from_secs(secs)) .as_millis() as u64, ); }Replace the code with:
// Recheck in 1 second self.schedule_recheck_in(1);
249-261: Simplify option matching by usingis_some()You can simplify the assignment of
mainnet_core_status_onlineandtestnet_core_status_onlineusingOption::is_some().Apply this diff to simplify the code:
)) => { - match mainnet_chainlock { - Some(_) => self.mainnet_core_status_online = true, - None => self.mainnet_core_status_online = false, - } + self.mainnet_core_status_online = mainnet_chainlock.is_some(); - match testnet_chainlock { - Some(_) => self.testnet_core_status_online = true, - None => self.testnet_core_status_online = false, - } + self.testnet_core_status_online = testnet_chainlock.is_some(); }
285-300: Encapsulate recheck logic to improve readabilityThe recheck logic in the
uimethod can be encapsulated into a separate function to enhance readability and reduce complexity.Suggested refactor:
fn handle_recheck(&mut self, action: &mut AppAction) { let recheck_interval = Duration::from_secs(3); if *action == AppAction::None { let current_time = SystemTime::now() .duration_since(UNIX_EPOCH) .expect("Time went backwards"); if let Some(time) = self.recheck_time { if current_time.as_millis() as u64 >= time { *action = AppAction::BackendTask(BackendTask::CoreTask(CoreTask::GetBestChainLocks)); self.recheck_time = Some((current_time + recheck_interval).as_millis() as u64); } } else { self.recheck_time = Some((current_time + recheck_interval).as_millis() as u64); } } }Then, in the
uimethod, replace the recheck logic with:self.handle_recheck(&mut action);
| fn display_message(&mut self, message: &str, _message_type: super::MessageType) { | ||
| if message.contains("Failed to get best chain lock for both mainnet and testnet") { | ||
| self.mainnet_core_status_online = false; | ||
| self.testnet_core_status_online = false; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid string matching for error handling
Relying on string matching in display_message is fragile. Consider using error types or codes for more robust error handling.
Propose defining a custom error type or variant and updating the method accordingly:
// Define a custom message type or error enum
enum Message {
FailedToGetBestChainLock,
// other variants...
}
fn display_message(&mut self, message: &Message, _message_type: super::MessageType) {
match message {
Message::FailedToGetBestChainLock => {
self.mainnet_core_status_online = false;
self.testnet_core_status_online = false;
}
// handle other messages...
}
}
QuantumExplorer
left a comment
QuantumExplorer
left a comment
There was a problem hiding this comment.
No, this is wrong, we don't want to get chain locks for both networks with that call, because if one network is offline you can't get the chain lock for the other network.
Yes you can |
2 participants
Now we check both networks every 3 seconds while the screen is active.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor