Re: [pdm-devel] [PATCH datacenter-manager v2] fix: show empty-state message inside the top entities panels

["Shan Shaji" <s.shaji@proxmox.com>]

Hi Lukas, Thank you so much for the review. I will update 
the changes accordingly in v3. 

On Mon Jan 12, 2026 at 1:28 PM CET, Lukas Wagner wrote:
> Looking good mostly.
>
> Regarding the commit message, usually we only use the 'fix' prefix only
> if there is some concrete BZ entry that can be referenced. Also, usually
> we use 'subsystem tags', for this commit this could be (slightly
> reworded)
>
> ui: top entities: show hint message if there are no entities


> On Wed Dec 3, 2025 at 5:43 PM CET, Shan Shaji wrote:
>> When the entities list was empty, the dashboard did not display any
>> message inside the Top Entities panels. This change adds a empty-state
>> message so the panels provide proper feedback when no entries are
>> available.
>>
>> Signed-off-by: Shan Shaji <s.shaji@proxmox.com>
>> ---
>>  changes since v1: Thanks @Dominik
>>  - Removed the extra property inside the TopEntities struct.
>>  - Added optional_child to the Panel to show the empty error message. 
>>  - There was an empty space caused by the `TopEntities` component when the
>>    entities list was empty. Inorder to fix that now the TopEntities will
>>    only be rendered if the entities list is not empty.
>>
>>  ui/src/dashboard/top_entities.rs | 68 ++++++++++++++++++++------------
>>  1 file changed, 42 insertions(+), 26 deletions(-)
>>
>> diff --git a/ui/src/dashboard/top_entities.rs b/ui/src/dashboard/top_entities.rs
>> index ab8c703..f79dab7 100644
>> --- a/ui/src/dashboard/top_entities.rs
>> +++ b/ui/src/dashboard/top_entities.rs
>> @@ -1,6 +1,7 @@
>>  use std::rc::Rc;
>>  
>>  use pwt::state::SharedState;
>> +use pwt::widget::Fa;
>>  use web_sys::HtmlElement;
>>  use yew::virtual_dom::{VComp, VNode};
>>  
>> @@ -132,6 +133,7 @@ impl Component for TopEntitiesComp {
>>              .style("gap", "var(--pwt-spacer-3)");
>>          let mut tooltip = None;
>>          let data = &props.entities;
>> +
>
> Unrelated newline change; please remove this one for v3 or at least
> split it out to a separate patch, in case you think the improved code
> formatting is worth it.
>
>>          for entity in data.iter().rev() {
>>              let resource = &entity.resource;
>>              let rrd = &entity.rrd_data;
>> @@ -199,6 +201,7 @@ impl Component for TopEntitiesComp {
>>                      .with_optional_child(tooltip_anchor),
>>              );
>>          }
>> +
>
> Same here :)
>
>>          Container::new()
>>              .class(FlexFit)
>>              .with_child(list)
>> @@ -336,34 +339,47 @@ pub fn create_top_entities_panel(
>>      leaderboard_type: LeaderboardType,
>>  ) -> Panel {
>>      let top_entities = top_entities.read();
>> -    let (entities, icon, title, metrics_title, threshold) = match leaderboard_type {
>> -        LeaderboardType::GuestCpu => (
>> -            top_entities.data.as_ref().map(|e| e.guest_cpu.clone()),
>> -            "desktop",
>> -            tr!("Guests With the Highest CPU Usage"),
>> -            tr!("CPU usage"),
>> -            0.85,
>> -        ),
>> -        LeaderboardType::NodeCpu => (
>> -            top_entities.data.as_ref().map(|e| e.node_cpu.clone()),
>> -            "building",
>> -            tr!("Nodes With the Highest CPU Usage"),
>> -            tr!("CPU usage"),
>> -            0.85,
>> -        ),
>> -        LeaderboardType::NodeMemory => (
>> -            top_entities.data.as_ref().map(|e| e.node_memory.clone()),
>> -            "building",
>> -            tr!("Nodes With the Highest Memory Usage"),
>> -            tr!("Memory usage"),
>> -            0.95,
>> -        ),
>> -    };
>> +    let (entities, icon, title, metrics_title, metrics_empty_message, threshold) =
>> +        match leaderboard_type {
>> +            LeaderboardType::GuestCpu => (
>> +                top_entities.data.as_ref().map(|e| e.guest_cpu.clone()),
>> +                "desktop",
>> +                tr!("Guests With the Highest CPU Usage"),
>> +                tr!("CPU usage"),
>> +                tr!("No guests available"),
>> +                0.85,
>> +            ),
>> +            LeaderboardType::NodeCpu => (
>> +                top_entities.data.as_ref().map(|e| e.node_cpu.clone()),
>> +                "building",
>> +                tr!("Nodes With the Highest CPU Usage"),
>> +                tr!("CPU usage"),
>> +                tr!("No nodes available"),
>> +                0.85,
>> +            ),
>> +            LeaderboardType::NodeMemory => (
>> +                top_entities.data.as_ref().map(|e| e.node_memory.clone()),
>> +                "building",
>> +                tr!("Nodes With the Highest Memory Usage"),
>> +                tr!("Memory usage"),
>> +                tr!("No nodes available"),
>> +                0.95,
>> +            ),
>> +        };
>>      Panel::new()
>>          .title(create_title_with_icon(icon, title))
>> -        .with_optional_child(
>> -            entities.map(|entities| TopEntities::new(entities, metrics_title, threshold)),
>> -        )
>> +        .with_optional_child(entities.as_ref().and_then(|entities| {
>> +            entities.is_empty().then_some(
>> +                Row::new()
>> +                    .padding(4)
>> +                    .gap(2)
>> +                    .with_child(Fa::new("info-circle").fixed_width())
>> +                    .with_child(&metrics_empty_message),
>> +            )
>> +        }))
>> +        .with_optional_child(entities.and_then(|entities| {
>> +            (!entities.is_empty()).then_some(TopEntities::new(entities, metrics_title, threshold))
>> +        }))
>
> I think it would a bit more readable if you used `if` to distinguish
> both cases, since it's always one of the two (assuming that `entities`
> is not None) cases.
>
> e.g. like this:
>
> .with_optional_child(entities.as_ref().and_then(|entities| {
>     if entities.is_empty() {
>         Row::new()
>             .padding(4)
>             .gap(2)
>             .with_child(Fa::new("info-circle").fixed_width())
>             .with_child(&metrics_empty_message)
>     } else {
> 	TopEntities::new(entities, metrics_title, threshold)
>     }
> }))
>
> What do you think?

It does makes sense. Writing explicit `if` is much more readable here.
Thanks! 

>>          .with_optional_child((!top_entities.has_data()).then_some(loading_column()))
>>          .with_optional_child(
>>              top_entities



_______________________________________________
pdm-devel mailing list
pdm-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pdm-devel