feat: dpns subscreens#18
Conversation
WalkthroughThe changes in this pull request involve significant modifications to the handling of the Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Outside diff range and nitpick comments (3)
src/ui/components/dpns_subscreen_chooser_panel.rs (2)
6-9: Add documentation for the public function.As this is a public function, it should have documentation explaining its purpose, parameters, and return value.
Add documentation like this:
+/// Creates a side panel for choosing between different DPNS subscreens. +/// +/// # Arguments +/// * `ctx` - The egui context for rendering +/// * `app_context` - The application context +/// +/// # Returns +/// An `AppAction` indicating any navigation changes requested by the user pub fn add_dpns_subscreen_chooser_panel(ctx: &Context, app_context: &Arc<AppContext>) -> AppAction {
11-18: Document panel configuration values.Consider extracting and documenting the magic numbers used for panel width and margins to make their purpose clear.
+// Default width provides enough space for subscreen names while maintaining compact layout +const PANEL_DEFAULT_WIDTH: f32 = 250.0; +// Consistent margin around panel content for visual spacing +const PANEL_MARGIN: f32 = 10.0; SidePanel::left("dpns_subscreen_chooser_panel") - .default_width(250.0) + .default_width(PANEL_DEFAULT_WIDTH) .frame( Frame::none() .fill(ctx.style().visuals.panel_fill) - .inner_margin(Margin::same(10.0)), + .inner_margin(Margin::same(PANEL_MARGIN)), )src/ui/mod.rs (1)
Line range hint
280-280: Fix incorrect screen type mapping for AddNewIdentityScreen.There appears to be a bug where AddNewIdentityScreen is incorrectly mapped to ScreenType::AddExistingIdentity.
-Screen::AddNewIdentityScreen(_) => ScreenType::AddExistingIdentity, +Screen::AddNewIdentityScreen(_) => ScreenType::AddNewIdentity,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
- src/app.rs (4 hunks)
- src/context.rs (2 hunks)
- src/main.rs (1 hunks)
- src/ui/components/dpns_subscreen_chooser_panel.rs (1 hunks)
- src/ui/components/left_panel.rs (1 hunks)
- src/ui/components/mod.rs (1 hunks)
- src/ui/dpns_contested_names_screen.rs (14 hunks)
- src/ui/mod.rs (7 hunks)
✅ Files skipped from review due to trivial changes (2)
- src/main.rs
- src/ui/components/mod.rs
🧰 Additional context used
🔇 Additional comments (14)
src/ui/components/dpns_subscreen_chooser_panel.rs (2)
1-4: LGTM: Imports are well-organized and appropriate.All imports are properly used and organized by their source (crate-local vs external).
53-53: LGTM: Clean and idiomatic return.The implicit return of the action variable is clean and follows Rust conventions.
src/ui/components/left_panel.rs (1)
50-50: LGTM! Verify the new screen type.The change from
RootScreenDPNSContestedNamestoRootScreenDPNSActiveContestsaligns with the PR's objective to implement DPNS subscreens.Let's verify the new screen type exists and is properly defined:
✅ Verification successful
Let me analyze the results:
- The new
RootScreenDPNSActiveContestsvariant is properly defined and used across multiple files:
- Defined in
src/ui/mod.rsas an enum variant- Used in screen type mapping and conversion logic in
src/ui/mod.rs- Referenced in UI components and screens
- No lingering references to the old
RootScreenDPNSContestedNamestypeLet me verify one more aspect - the screen implementation:
Based on the verification results, I can now generate the final response:
The new screen type is properly integrated
The
RootScreenDPNSActiveContestsvariant is correctly:
- Defined in the enum within
src/ui/mod.rs- Mapped to
ScreenType::DPNSActiveContests- Implemented using
DPNSContestedNamesScreenwithDPNSSubscreen::Active- Integrated into the navigation system
- No lingering references to the old screen type exist
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the new RootScreenDPNSActiveContests enum variant exists # and check for any remaining references to the old RootScreenDPNSContestedNames # Test 1: Search for the new enum variant definition echo "Checking for new screen type definition..." rg "RootScreenDPNSActiveContests" # Test 2: Verify no lingering references to old screen type echo "Checking for any remaining references to old screen type..." rg "RootScreenDPNSContestedNames"Length of output: 1008
Script:
#!/bin/bash # Check for the screen implementation echo "Checking for DPNSActiveContests screen implementation..." rg "DPNSActiveContests" -A 5Length of output: 4789
src/context.rs (2)
12-12: LGTM!The import is correctly added and is necessary for accessing the identity ID through the
IdentityGettersV0trait.
134-164: LGTM! Robust error handling and safe implementation.The method correctly implements the filtering of contested names owned by local identities with proper error handling and logging.
src/ui/mod.rs (4)
17-17: LGTM: Import added for new DPNS subscreen types.The import is correctly placed and necessary for the new DPNS screen categorization.
100-102: LGTM: Screen creation logic properly handles new DPNS variants.The implementation correctly instantiates DPNSContestedNamesScreen with the appropriate subscreen type for each variant, maintaining consistency with the existing pattern.
Also applies to: 121-129
261-272: LGTM: Pattern matching for DPNS subscreens is well-implemented.The implementation correctly uses struct destructuring to map DPNS subscreens to their corresponding screen types.
43-45: Verify the non-sequential integer mappings for screen types.The integer mappings for the new DPNS screen types are non-sequential (1, 5, 6). While this works, it's worth verifying if these specific values are required for:
- Backward compatibility with persisted data
- External system integration
- Future screen type allocations
If not required, consider using sequential values for better maintainability.
Also applies to: 56-61, 69-74
✅ Verification successful
Let's gather more information about how these integer values are used in the codebase, particularly in the database settings.
This will help us understand:
- How the screen type integers are persisted
- Whether there's any specific requirement for these non-sequential values
- If there are any compatibility concerns we need to be aware of
Let's check the git history to understand if there were previous values being used and if this is a backward compatibility concern.
This will help us understand:
- The evolution of RootScreenType and its integer mappings
- If there are any database migrations that depend on these specific values
- Whether these non-sequential values are intentionally preserved for compatibility
Based on the git history, I can now generate a final response as we have sufficient information about the evolution of these screen types and their integer mappings.
Non-sequential screen type integers are intentionally maintained for compatibility
The git history reveals that these integer mappings are deliberately preserved for backward compatibility:
- Value 1 was originally used for
RootScreenDPNSContestedNamesand is now used forRootScreenDPNSActiveContests- Values 2-4 were already assigned to existing screens
- Values 5-6 are new additions for the split DPNS functionality
Since these values are persisted in the database (as seen in
settings.rs) and there's an existing user base, maintaining these specific integer mappings is the correct approach to ensure database compatibility.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any persistence or serialization of these screen type integers rg -l "to_int|from_int" --type rustLength of output: 76
Script:
#!/bin/bash # Check how these values are used in settings.rs and any related persistence rg -A 5 -B 5 "to_int|from_int" src/database/settings.rs # Also check for any serialization or database-related code around RootScreenType rg -A 5 -B 5 "RootScreenType" src/database/settings.rsLength of output: 2388
Script:
#!/bin/bash # Check git history for RootScreenType changes git log -p src/ui/mod.rs | grep -A 10 -B 10 "RootScreenType" # Also check for any database migrations that might be related fd "migration" --type f --exec rg -l "start_root_screen|RootScreenType" {}Length of output: 7853
src/app.rs (3)
6-6: LGTM: Clean import additionThe import is properly scoped and aligns with the new DPNS subscreen functionality.
169-178: LGTM: Screen mapping is well-structuredThe screen mapping is correctly implemented with unique keys for each DPNS subscreen type.
#!/bin/bash # Verify consistency between RootScreenType enum and screen mappings rg -A 10 "RootScreenType = enum" src/
144-149: 🛠️ Refactor suggestionVerify testnet context safety and reduce duplication
Two concerns to address:
- The testnet context unwrap could fail if not properly guarded.
- The screen initialization duplicates the mainnet logic.
Apply the same helper method here:
- dpns_active_contests_screen = - DPNSContestedNamesScreen::new(&testnet_app_context, DPNSSubscreen::Active); - dpns_past_contests_screen = - DPNSContestedNamesScreen::new(&testnet_app_context, DPNSSubscreen::Past); - dpns_my_usernames_screen = - DPNSContestedNamesScreen::new(&testnet_app_context, DPNSSubscreen::Owned); + (dpns_active_contests_screen, dpns_past_contests_screen, dpns_my_usernames_screen) = + create_dpns_screens(testnet_app_context);src/ui/dpns_contested_names_screen.rs (2)
38-42: AddingDPNSSubscreenEnum Enhances Screen CategorizationIntroducing the
DPNSSubscreenenum with variantsActive,Past, andOwnedeffectively categorizes the DPNS screens, improving the modularity and readability of the code.
58-79: Constructor Update Appropriately Handles Subscreen InitializationThe
newmethod updates correctly to acceptdpns_subscreenand initializescontested_namesbased on the selected subscreen. Error handling withunwrap_or_elseensures resilience against loading failures.
| pub fn add_dpns_subscreen_chooser_panel(ctx: &Context, app_context: &Arc<AppContext>) -> AppAction { | ||
| let mut action = AppAction::None; | ||
|
|
||
| let subscreens = vec!["Active contests", "Past contests", "My usernames"]; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using an enum for subscreen names.
Using string literals for screen names makes the code more prone to typos and harder to maintain. Consider defining an enum for the subscreens.
enum DPNSSubscreenType {
ActiveContests,
PastContests,
MyUsernames,
}
impl DPNSSubscreenType {
fn display_name(&self) -> &'static str {
match self {
Self::ActiveContests => "Active contests",
Self::PastContests => "Past contests",
Self::MyUsernames => "My usernames",
}
}
}
// Then use it like:
let subscreens = vec![
DPNSSubscreenType::ActiveContests,
DPNSSubscreenType::PastContests,
DPNSSubscreenType::MyUsernames,
];| // Display subscreen names | ||
| ui.vertical(|ui| { | ||
| ui.label("DPNS Subscreens"); | ||
| ui.add_space(10.0); | ||
|
|
||
| for subscreen in subscreens { | ||
| // Show the subscreen name as a clickable option | ||
| if ui.button(subscreen).clicked() { | ||
| // Handle navigation based on which subscreen is selected | ||
| match subscreen { | ||
| "Active contests" => { | ||
| action = AppAction::SetMainScreen( | ||
| RootScreenType::RootScreenDPNSActiveContests, | ||
| ) | ||
| } | ||
| "Past contests" => { | ||
| action = AppAction::SetMainScreen( | ||
| RootScreenType::RootScreenDPNSPastContests, | ||
| ) | ||
| } | ||
| "My usernames" => { | ||
| action = AppAction::SetMainScreen( | ||
| RootScreenType::RootScreenDPNSOwnedNames, | ||
| ) | ||
| } | ||
| _ => {} | ||
| } | ||
| } | ||
|
|
||
| ui.add_space(5.0); | ||
| } | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Several improvements for the UI implementation.
- The
app_contextparameter is unused and could be removed. - Consider adding tooltips or aria-labels for better accessibility.
- The match statement could be simplified using a more direct mapping.
Here's how you could improve the implementation:
-pub fn add_dpns_subscreen_chooser_panel(ctx: &Context, app_context: &Arc<AppContext>) -> AppAction {
+pub fn add_dpns_subscreen_chooser_panel(ctx: &Context) -> AppAction {
let mut action = AppAction::None;
- let subscreens = vec!["Active contests", "Past contests", "My usernames"];
+ let subscreens = vec![
+ ("Active contests", RootScreenType::RootScreenDPNSActiveContests),
+ ("Past contests", RootScreenType::RootScreenDPNSPastContests),
+ ("My usernames", RootScreenType::RootScreenDPNSOwnedNames),
+ ];
// ... panel setup ...
ui.vertical(|ui| {
ui.label("DPNS Subscreens");
ui.add_space(10.0);
- for subscreen in subscreens {
+ for (label, screen_type) in subscreens {
// Show the subscreen name as a clickable option
- if ui.button(subscreen).clicked() {
- match subscreen {
- "Active contests" => {
- action = AppAction::SetMainScreen(
- RootScreenType::RootScreenDPNSActiveContests,
- )
- }
- "Past contests" => {
- action = AppAction::SetMainScreen(
- RootScreenType::RootScreenDPNSPastContests,
- )
- }
- "My usernames" => {
- action = AppAction::SetMainScreen(
- RootScreenType::RootScreenDPNSOwnedNames,
- )
- }
- _ => {}
- }
+ let button = egui::Button::new(label)
+ .wrap(false)
+ .sense(egui::Sense::click());
+ if ui.add_enabled(true, button)
+ .on_hover_text(format!("View {}", label.to_lowercase()))
+ .clicked() {
+ action = AppAction::SetMainScreen(screen_type);
}
ui.add_space(5.0);
}
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Display subscreen names | |
| ui.vertical(|ui| { | |
| ui.label("DPNS Subscreens"); | |
| ui.add_space(10.0); | |
| for subscreen in subscreens { | |
| // Show the subscreen name as a clickable option | |
| if ui.button(subscreen).clicked() { | |
| // Handle navigation based on which subscreen is selected | |
| match subscreen { | |
| "Active contests" => { | |
| action = AppAction::SetMainScreen( | |
| RootScreenType::RootScreenDPNSActiveContests, | |
| ) | |
| } | |
| "Past contests" => { | |
| action = AppAction::SetMainScreen( | |
| RootScreenType::RootScreenDPNSPastContests, | |
| ) | |
| } | |
| "My usernames" => { | |
| action = AppAction::SetMainScreen( | |
| RootScreenType::RootScreenDPNSOwnedNames, | |
| ) | |
| } | |
| _ => {} | |
| } | |
| } | |
| ui.add_space(5.0); | |
| } | |
| }); | |
| }); | |
| // Display subscreen names | |
| ui.vertical(|ui| { | |
| ui.label("DPNS Subscreens"); | |
| ui.add_space(10.0); | |
| for (label, screen_type) in subscreens { | |
| // Show the subscreen name as a clickable option | |
| let button = egui::Button::new(label) | |
| .wrap(false) | |
| .sense(egui::Sense::click()); | |
| if ui.add_enabled(true, button) | |
| .on_hover_text(format!("View {}", label.to_lowercase())) | |
| .clicked() { | |
| action = AppAction::SetMainScreen(screen_type); | |
| } | |
| ui.add_space(5.0); | |
| } | |
| }); | |
| }); |
| // Collect the identifiers from local qualified identities | ||
| let local_identifiers: Vec<_> = local_qualified_identities | ||
| .iter() | ||
| .map(|identity| identity.identity.id()) | ||
| .collect(); | ||
|
|
||
| // Filter the contested names by checking if they were awarded to any local identifier | ||
| let owned_contested_names: Vec<ContestedName> = all_contested_names | ||
| .into_iter() | ||
| .filter(|contested_name| { | ||
| if let Some(awarded_to) = &contested_name.awarded_to { | ||
| local_identifiers.contains(awarded_to) | ||
| } else { | ||
| false | ||
| } | ||
| }) | ||
| .collect(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider optimizing the filtering logic.
The current implementation creates intermediate vectors which could be avoided for better performance.
Consider this more efficient implementation using iterator chaining:
- // Collect the identifiers from local qualified identities
- let local_identifiers: Vec<_> = local_qualified_identities
- .iter()
- .map(|identity| identity.identity.id())
- .collect();
-
- // Filter the contested names by checking if they were awarded to any local identifier
- let owned_contested_names: Vec<ContestedName> = all_contested_names
- .into_iter()
- .filter(|contested_name| {
- if let Some(awarded_to) = &contested_name.awarded_to {
- local_identifiers.contains(awarded_to)
- } else {
- false
- }
- })
- .collect();
+ let owned_contested_names = all_contested_names
+ .into_iter()
+ .filter(|contested_name| {
+ contested_name.awarded_to.as_ref().map_or(false, |awarded_to| {
+ local_qualified_identities
+ .iter()
+ .any(|identity| identity.identity.id() == awarded_to)
+ })
+ })
+ .collect();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Collect the identifiers from local qualified identities | |
| let local_identifiers: Vec<_> = local_qualified_identities | |
| .iter() | |
| .map(|identity| identity.identity.id()) | |
| .collect(); | |
| // Filter the contested names by checking if they were awarded to any local identifier | |
| let owned_contested_names: Vec<ContestedName> = all_contested_names | |
| .into_iter() | |
| .filter(|contested_name| { | |
| if let Some(awarded_to) = &contested_name.awarded_to { | |
| local_identifiers.contains(awarded_to) | |
| } else { | |
| false | |
| } | |
| }) | |
| .collect(); | |
| let owned_contested_names = all_contested_names | |
| .into_iter() | |
| .filter(|contested_name| { | |
| contested_name.awarded_to.as_ref().map_or(false, |awarded_to| { | |
| local_qualified_identities | |
| .iter() | |
| .any(|identity| identity.identity.id() == awarded_to) | |
| }) | |
| }) | |
| .collect(); |
| let mut dpns_active_contests_screen = | ||
| DPNSContestedNamesScreen::new(&mainnet_app_context, DPNSSubscreen::Active); | ||
| let mut dpns_past_contests_screen = | ||
| DPNSContestedNamesScreen::new(&mainnet_app_context, DPNSSubscreen::Past); | ||
| let mut dpns_my_usernames_screen = | ||
| DPNSContestedNamesScreen::new(&mainnet_app_context, DPNSSubscreen::Owned); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider extracting DPNS screen initialization logic
While the initialization is correct, consider extracting this into a helper method to reduce code duplication and improve maintainability.
+ fn create_dpns_screens(app_context: &Arc<AppContext>) -> (
+ DPNSContestedNamesScreen,
+ DPNSContestedNamesScreen,
+ DPNSContestedNamesScreen,
+ ) {
+ (
+ DPNSContestedNamesScreen::new(app_context, DPNSSubscreen::Active),
+ DPNSContestedNamesScreen::new(app_context, DPNSSubscreen::Past),
+ DPNSContestedNamesScreen::new(app_context, DPNSSubscreen::Owned),
+ )
+ }
- let mut dpns_active_contests_screen =
- DPNSContestedNamesScreen::new(&mainnet_app_context, DPNSSubscreen::Active);
- let mut dpns_past_contests_screen =
- DPNSContestedNamesScreen::new(&mainnet_app_context, DPNSSubscreen::Past);
- let mut dpns_my_usernames_screen =
- DPNSContestedNamesScreen::new(&mainnet_app_context, DPNSSubscreen::Owned);
+ let (mut dpns_active_contests_screen, mut dpns_past_contests_screen, mut dpns_my_usernames_screen) =
+ create_dpns_screens(&mainnet_app_context);Committable suggestion was skipped due to low confidence.
| SortColumn::AbstainVotes => a.abstain_votes.cmp(&b.abstain_votes), | ||
| SortColumn::EndingTime => a.end_time.cmp(&b.end_time), | ||
| SortColumn::LastUpdated => a.last_updated.cmp(&b.last_updated), | ||
| SortColumn::AwardedTo => a.awarded_to.cmp(&b.awarded_to), |
There was a problem hiding this comment.
Handle Potential None Values in awarded_to During Sorting
When sorting by SortColumn::AwardedTo, there might be cases where awarded_to is None, leading to panics. Ensure that Option values are handled safely to prevent runtime errors.
Apply this diff to safely handle None values:
SortColumn::AwardedTo => a.awarded_to.cmp(&b.awarded_to),
+// Add handling for None values to prevent panics
+SortColumn::AwardedTo => a.awarded_to.unwrap_or_default().cmp(&b.awarded_to.unwrap_or_default()),Committable suggestion was skipped due to low confidence.
| let first_6_chars_of_id: String = contestant | ||
| .id | ||
| .to_string(Encoding::Base58) | ||
| .chars() | ||
| .take(6) | ||
| .collect(); | ||
| let button_text = | ||
| format!("{}... - {} votes", first_6_chars_of_id, contestant.votes); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider Displaying More Characters for Contestant IDs
Displaying only the first 6 characters of the contestant ID might lead to ambiguity if IDs share the same prefix. To enhance uniqueness and user clarity, consider showing more characters.
let first_6_chars_of_id: String = contestant
.id
.to_string(Encoding::Base58)
.chars()
- .take(6)
+ .take(8)
.collect();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let first_6_chars_of_id: String = contestant | |
| .id | |
| .to_string(Encoding::Base58) | |
| .chars() | |
| .take(6) | |
| .collect(); | |
| let button_text = | |
| format!("{}... - {} votes", first_6_chars_of_id, contestant.votes); | |
| let first_6_chars_of_id: String = contestant | |
| .id | |
| .to_string(Encoding::Base58) | |
| .chars() | |
| .take(8) | |
| .collect(); | |
| let button_text = | |
| format!("{}... - {} votes", first_6_chars_of_id, contestant.votes); |
| // Convert the timestamp to a DateTime object using timestamp_millis_opt | ||
| if let LocalResult::Single(datetime) = | ||
| Utc.timestamp_opt(last_updated as i64, 0) | ||
| { | ||
| // Use chrono-humanize to get the relative time |
There was a problem hiding this comment.
Ensure Consistent Timestamp Conversion for last_updated
The last_updated timestamp is converted using Utc.timestamp_opt, which expects seconds, while other timestamps use Utc.timestamp_millis_opt, expecting milliseconds. This inconsistency might lead to incorrect date displays.
Apply this diff to use milliseconds consistently:
if let LocalResult::Single(datetime) =
- Utc.timestamp_opt(last_updated as i64, 0)
+ Utc.timestamp_millis_opt(last_updated as i64)
{
let relative_time = HumanTime::from(datetime).to_string();
ui.label(relative_time);
} else {
ui.label("Invalid timestamp");
}Committable suggestion was skipped due to low confidence.
| match self.dpns_subscreen { | ||
| DPNSSubscreen::Active => { | ||
| ui.label( | ||
| egui::RichText::new("No active contests at the moment.") | ||
| .heading() | ||
| .strong() | ||
| .color(egui::Color32::GRAY), | ||
| ); | ||
| } | ||
| DPNSSubscreen::Past => { | ||
| ui.label( | ||
| egui::RichText::new("No active or past contests at the moment.") | ||
| .heading() | ||
| .strong() | ||
| .color(egui::Color32::GRAY), | ||
| ); | ||
| } | ||
| DPNSSubscreen::Owned => { | ||
| ui.label( | ||
| egui::RichText::new("No owned usernames.") | ||
| .heading() | ||
| .strong() | ||
| .color(egui::Color32::GRAY), | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
Clarify Messages When No Contests Are Available
The message for the Past subscreen states "No active or past contests at the moment," which might confuse users expecting to see past contests. Refine the message to accurately reflect the subscreen context.
Apply this diff to adjust the message:
DPNSSubscreen::Past => {
ui.label(
- egui::RichText::new("No active or past contests at the moment.")
+ egui::RichText::new("No past contests at the moment.")
.heading()
.strong()
.color(egui::Color32::GRAY),
);
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| match self.dpns_subscreen { | |
| DPNSSubscreen::Active => { | |
| ui.label( | |
| egui::RichText::new("No active contests at the moment.") | |
| .heading() | |
| .strong() | |
| .color(egui::Color32::GRAY), | |
| ); | |
| } | |
| DPNSSubscreen::Past => { | |
| ui.label( | |
| egui::RichText::new("No active or past contests at the moment.") | |
| .heading() | |
| .strong() | |
| .color(egui::Color32::GRAY), | |
| ); | |
| } | |
| DPNSSubscreen::Owned => { | |
| ui.label( | |
| egui::RichText::new("No owned usernames.") | |
| .heading() | |
| .strong() | |
| .color(egui::Color32::GRAY), | |
| ); | |
| } | |
| } | |
| match self.dpns_subscreen { | |
| DPNSSubscreen::Active => { | |
| ui.label( | |
| egui::RichText::new("No active contests at the moment.") | |
| .heading() | |
| .strong() | |
| .color(egui::Color32::GRAY), | |
| ); | |
| } | |
| DPNSSubscreen::Past => { | |
| ui.label( | |
| egui::RichText::new("No past contests at the moment.") | |
| .heading() | |
| .strong() | |
| .color(egui::Color32::GRAY), | |
| ); | |
| } | |
| DPNSSubscreen::Owned => { | |
| ui.label( | |
| egui::RichText::new("No owned usernames.") | |
| .heading() | |
| .strong() | |
| .color(egui::Color32::GRAY), | |
| ); | |
| } | |
| } |
| match self.dpns_subscreen { | ||
| DPNSSubscreen::Active => { | ||
| self.render_table_active_contests(ui); | ||
| } | ||
| DPNSSubscreen::Past => { | ||
| self.render_table_past_contests(ui); | ||
| } | ||
| DPNSSubscreen::Owned => { | ||
| self.render_table_past_contests(ui); | ||
| } | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Based on the search results, I can now generate a final response as we have enough context about how owned names are handled.
render_table_past_contests is Inappropriate for Owned Names View
The code reveals that owned names have distinct handling:
- There's a dedicated
owned_contested_names()method that filters contested names specifically for those awarded to local identities - The UI shows specific messages for owned usernames ("No owned usernames" message exists)
- There's a separate root screen type
RootScreenDPNSOwnedNamesfor owned names
The current implementation using render_table_past_contests is incorrect as it:
- Uses unfiltered contested names data instead of the owned-specific data
- Likely displays columns and information relevant to past contests rather than owned names
Recommendation: Create a dedicated render_table_owned_names function that:
- Uses
owned_contested_names()for data - Shows columns relevant to owned usernames
- Implements UI specific to ownership status
🔗 Analysis chain
Verify Rendering Logic for the Owned Subscreen
The Owned subscreen currently calls render_table_past_contests, which is designed for past contests. This may not correctly display owned usernames if the data or columns differ. Confirm that the rendering function suits the Owned subscreen's requirements.
Let me gather more information about the rendering functions and the data structures they use.
Let me search for more specific information about the rendering functions and their implementations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if `render_table_past_contests` is appropriate for the Owned subscreen.
# Test: Search for the use of `render_table_past_contests` with `DPNSSubscreen::Owned`.
rg 'DPNSSubscreen::Owned.*render_table_past_contests'
# Expected Outcome: Ensure that the rendering function displays the owned usernames correctly.
Length of output: 55
Script:
#!/bin/bash
# Let's examine the implementation of render_table_past_contests and any other related rendering functions
ast-grep --pattern 'fn render_table_past_contests'
# Also check for any other render_table functions that might be more appropriate
ast-grep --pattern 'fn render_table'
# Let's also look for the DPNSSubscreen enum definition to understand its purpose
rg -A 5 "enum DPNSSubscreen"
Length of output: 411
Script:
#!/bin/bash
# Search for the implementation of render_table_past_contests with context
rg -A 20 "fn render_table_past_contests"
# Look for any data structures or methods specific to owned names
rg -A 10 "owned.*names" --ignore-case
# Check if there are any specific table columns or data handling for owned names
rg "Owned.*{" -A 10 src/ui/dpns_contested_names_screen.rs
Length of output: 10956
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
src/ui/components/dpns_subscreen_chooser_panel.rs (1)
8-12: Remove unnecessary catch-all pattern in match expression.Since all variants of
DPNSSubscreenare explicitly handled in the match expression, the catch-all pattern_ => {}is unnecessary and can be removed.Also applies to: 47-47
src/ui/dpns_contested_names_screen.rs (1)
Line range hint
68-107: Consider adding error recovery strategyWhile the error handling uses
unwrap_or_elseappropriately, consider implementing a retry mechanism or user feedback for failed loads.let contested_names = Arc::new(Mutex::new(match dpns_subscreen { DPNSSubscreen::Active => { app_context.ongoing_contested_names().unwrap_or_else(|e| { error!("Failed to load contested names: {:?}", e); + // Consider displaying error to user via UI + self.display_message(&format!("Failed to load contested names: {}", e), MessageType::Error); Vec::new() // Use default value if loading fails }) } // Similar changes for Past and Owned cases
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- src/ui/components/dpns_subscreen_chooser_panel.rs (1 hunks)
- src/ui/dpns_contested_names_screen.rs (14 hunks)
🧰 Additional context used
🔇 Additional comments (9)
src/ui/components/dpns_subscreen_chooser_panel.rs (2)
1-5: LGTM! Clean imports and function signature.The imports are specific and the function signature is correctly implemented without unused parameters.
6-6: LGTM! Proper action handling.The initialization and return of the action is clean and follows the expected pattern.
Also applies to: 56-57
src/ui/dpns_contested_names_screen.rs (7)
38-52: LGTM: Clean enum implementation for screen typesThe
DPNSSubscreenenum and its implementation provide a clear and maintainable way to handle different screen types.
501-505: Inconsistent timestamp handling
206-231: Improve message clarity for Past contestsThe message "No active or past contests at the moment" for the Past subscreen might be confusing as it mentions active contests.
120-127: Consider displaying more characters for contestant IDs
625-638: LGTM: Clean refresh implementationThe refresh logic correctly handles different subscreens and maintains consistent error handling.
688-710: LGTM: Proper routing implementationThe UI routing logic correctly maps subscreens to their respective root screen types.
758-768:⚠️ Potential issueIncorrect rendering function used for Owned subscreen
The Owned subscreen is using
render_table_past_contests, which may not be appropriate as owned names might need different columns or handling.Consider creating a dedicated
render_table_owned_namesfunction that's tailored for owned names display.
| SidePanel::left("dpns_subscreen_chooser_panel") | ||
| .default_width(250.0) | ||
| .frame( | ||
| Frame::none() | ||
| .fill(ctx.style().visuals.panel_fill) | ||
| .inner_margin(Margin::same(10.0)), | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding accessibility attributes to the panel.
While the panel setup is well-structured, consider adding accessibility attributes such as ARIA labels to improve screen reader support.
SidePanel::left("dpns_subscreen_chooser_panel")
.default_width(250.0)
+ .id(egui::Id::new("dpns_navigation_panel"))
.frame(
Frame::none()
.fill(ctx.style().visuals.panel_fill)
.inner_margin(Margin::same(10.0)),
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| SidePanel::left("dpns_subscreen_chooser_panel") | |
| .default_width(250.0) | |
| .frame( | |
| Frame::none() | |
| .fill(ctx.style().visuals.panel_fill) | |
| .inner_margin(Margin::same(10.0)), | |
| ) | |
| SidePanel::left("dpns_subscreen_chooser_panel") | |
| .default_width(250.0) | |
| .id(egui::Id::new("dpns_navigation_panel")) | |
| .frame( | |
| Frame::none() | |
| .fill(ctx.style().visuals.panel_fill) | |
| .inner_margin(Margin::same(10.0)), | |
| ) |
| for subscreen in subscreens { | ||
| // Show the subscreen name as a clickable option | ||
| if ui.button(subscreen.display_name()).clicked() { | ||
| // Handle navigation based on which subscreen is selected | ||
| match subscreen { | ||
| DPNSSubscreen::Active => { | ||
| action = AppAction::SetMainScreen( | ||
| RootScreenType::RootScreenDPNSActiveContests, | ||
| ) | ||
| } | ||
| DPNSSubscreen::Past => { | ||
| action = AppAction::SetMainScreen( | ||
| RootScreenType::RootScreenDPNSPastContests, | ||
| ) | ||
| } | ||
| DPNSSubscreen::Owned => { | ||
| action = AppAction::SetMainScreen( | ||
| RootScreenType::RootScreenDPNSOwnedNames, | ||
| ) | ||
| } | ||
| _ => {} | ||
| } | ||
| } | ||
|
|
||
| ui.add_space(5.0); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance user experience with tooltips and keyboard navigation.
Consider adding tooltips and improving keyboard navigation for better user experience.
for subscreen in subscreens {
- if ui.button(subscreen.display_name()).clicked() {
+ let button = egui::Button::new(subscreen.display_name())
+ .shortcut_text(format!("Alt+{}", (subscreen as u8 + 1)))
+ .wrap(false);
+ if ui.add(button)
+ .on_hover_text(format!("View {}", subscreen.display_name().to_lowercase()))
+ .clicked() {
match subscreen {
// ... existing match arms ...
}
}Committable suggestion was skipped due to low confidence.
| let mut dpns_active_contests_screen = | ||
| DPNSContestedNamesScreen::new(&mainnet_app_context, DPNSSubscreen::Active); | ||
| let mut dpns_past_contests_screen = | ||
| DPNSContestedNamesScreen::new(&mainnet_app_context, DPNSSubscreen::Past); | ||
| let mut dpns_my_usernames_screen = | ||
| DPNSContestedNamesScreen::new(&mainnet_app_context, DPNSSubscreen::Owned); |
…plate Add `fn sqlite_err(e: rusqlite::Error) -> Box<dyn Error + Send + Sync>` helper. Replace 24 instances of the multi-line `.map_err(|e| Box::new(SqlitePersistError::from(e)) as Box<_>)?` closure with `.map_err(sqlite_err)?`. Pure mechanical cleanup — same behavior, ~70 lines of noise removed. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
3 participants
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
These updates enhance user experience by providing more detailed control over contested name management and improving the overall interface.