Re: [pve-devel] [PATCH flutter_frontend 1/1] fix #4976: ui: Nodes offline

["Michael Köppl" <m.koeppl@proxmox.com>]

The commit message is not very descriptive. Also, please run dart format 
on these changes. There are some wrong indentations, missing spaces, 
trailing spaces, etc. I also added 3 comments inline.

Apart from that, I tested this by manually stopping a node I was 
currently looking at in the node overview. The app navigates me back to 
the previous screen once it detects that the node is offline and 
afterwards stops me from opening that node's overview again while it is 
offline. Opening the overview once the node is online again works as 
well. Everything seems to work as expected. It's a good change, well done!

On 4/29/25 15:16, Alexander Abraham wrote:
> diff --git a/lib/bloc/pve_node_overview_bloc.dart b/lib/bloc/pve_node_overview_bloc.dart
> index 19d6563..98005fd 100644
> --- a/lib/bloc/pve_node_overview_bloc.dart
> +++ b/lib/bloc/pve_node_overview_bloc.dart
> @@ -1,5 +1,5 @@
>   import 'dart:async';
> -
> +import 'package:flutter/material.dart';
>   import 'package:proxmox_dart_api_client/proxmox_dart_api_client.dart';
>   import 'package:pve_flutter_frontend/bloc/proxmox_base_bloc.dart';
>   import 'package:pve_flutter_frontend/states/pve_node_overview_state.dart';
> @@ -8,12 +8,13 @@ class PveNodeOverviewBloc
>       extends ProxmoxBaseBloc<PveNodeOverviewEvent, PveNodeOverviewState> {
>     final ProxmoxApiClient apiClient;
>     final String nodeID;
> +  final BuildContext context;
>     final PveNodeOverviewState init;
>     @override
>     PveNodeOverviewState get initialState => init;
>   
>     PveNodeOverviewBloc(
> -      {required this.apiClient, required this.nodeID, required this.init});
> +      {required this.apiClient, required this.nodeID, required this.init, required this.context});
>   
>     Timer? sTimer;
>     @override
> @@ -32,36 +33,56 @@ class PveNodeOverviewBloc
>     @override
>     Stream<PveNodeOverviewState> processEvents(
>         PveNodeOverviewEvent event) async* {
> -    if (event is UpdateNodeStatus) {
> -      final status = await apiClient.getNodeStatus(nodeID);
> -      yield latestState.rebuild((b) => b..status.replace(status));
> -      final rrdData =
> -          await apiClient.getNodeRRDdata(nodeID, PveRRDTimeframeType.hour);
> -      yield latestState.rebuild((b) => b..rrdData.replace(rrdData));
> -      final services = await apiClient.getNodeServices(nodeID);
> -      yield latestState.rebuild((b) => b..services.replace(services));
> -      try {
> -        final updates = await apiClient.getNodeAptUpdate(nodeID);
> -        yield latestState.rebuild((b) => b..updates.replace(updates));
> -        yield latestState
> -            .rebuild((b) => b..updatesQueryPermissionFailure = false);
> -      } on ProxmoxApiException catch (e) {
> -        // only throw on non permission related errors
> -        if (e.statusCode != 403) {
> -          rethrow;
> -        } else {
> +    PveNodeStatusModel? status;
> +    try {
> +      status = await apiClient.getNodeStatus(nodeID);
> +      if (event is UpdateNodeStatus) {
> +        yield latestState.rebuild((b) => b..status.replace(status!));
> +        final rrdData =
> +        await apiClient.getNodeRRDdata(nodeID, PveRRDTimeframeType.hour);
> +        yield latestState.rebuild((b) => b..rrdData.replace(rrdData));
> +        final services = await apiClient.getNodeServices(nodeID);
> +        yield latestState.rebuild((b) => b..services.replace(services));
> +        try {
> +          final updates = await apiClient.getNodeAptUpdate(nodeID);
> +          yield latestState.rebuild((b) => b..updates.replace(updates));
>             yield latestState
> -              .rebuild((b) => b..updatesQueryPermissionFailure = true);
> +              .rebuild((b) => b..updatesQueryPermissionFailure = false);
> +        }
> +        on ProxmoxApiException catch (e) {
> +            // only throw on non permission related errors
> +            if (e.statusCode != 403) {
> +              rethrow;
> +            }
> +            else {
> +              yield latestState
> +                .rebuild((b) => b..updatesQueryPermissionFailure = true);
> +            }
> +          }
> +          final disks = await apiClient.getNodeDisksList(nodeID);
> +          yield latestState.rebuild((b) => b..disks.replace(disks));
> +        }
> +
> +        if (event is PerformNodeAction) {
> +          await apiClient.doResourceAction(
> +            nodeID,
> +            '',
> +            'node',
> +            event.action,
> +            parameters: <String, String>{});
> +          yield latestState;
>           }
>         }
> -      final disks = await apiClient.getNodeDisksList(nodeID);
> -      yield latestState.rebuild((b) => b..disks.replace(disks));
> -    }
> -    if (event is PerformNodeAction) {
> -      await apiClient.doResourceAction(nodeID, '', 'node', event.action,
> -          parameters: <String, String>{});
> -      yield latestState;
> -    }
> +      catch(e){
> +        yield latestState.rebuild(
> +          (b) => b
> +            ..updatesQueryPermissionFailure = true
> +        );
> +        if (context.mounted){
> +          Navigator.of(context).pop();

Instead of navigating back immediately, I think it would make sense to 
instead display an AlertDialog, showing the error message to the user 
and letting them leave the screen through a "Close" button in the 
dialog. Otherwise, it feels a bit like the app is taking control away 
from the user. We use this approach in other places in the app (e.g. . 
What do you think?

> +        }
> +        else {}
> +      }
>     }
>   }

Wrapping the whole block in a try-catch without a rethrow also means 
that users will be navigated back to the home screen without an error 
message if e.g. the getNodeDisksList call returns an error. I think it'd 
make sense to rethrow here, similar to above.

>   
> diff --git a/lib/main.dart b/lib/main.dart
> index a985e3a..bbd3c97 100644
> --- a/lib/main.dart
> +++ b/lib/main.dart
> @@ -330,15 +330,18 @@ class MyApp extends StatelessWidget {
>                     final rbloc = Provider.of<PveResourceBloc>(context);
>                     return MultiProvider(
>                       providers: [
> +
>                         Provider<PveNodeOverviewBloc>(
>                           create: (context) => PveNodeOverviewBloc(
>                             apiClient: state.apiClient,
>                             nodeID: nodeID,
> +                          context: context,
>                             init: PveNodeOverviewState.init(
>                                 rbloc.latestState.isStandalone),
>                           )..events.add(UpdateNodeStatus()),
>                           dispose: (context, bloc) => bloc.dispose(),
>                         ),
> +
>                         Provider<PveTaskLogBloc>(
>                           create: (context) => PveTaskLogBloc(
>                               apiClient: state.apiClient,
> diff --git a/lib/pages/main_layout_slim.dart b/lib/pages/main_layout_slim.dart
> index 322bcac..5fa6199 100644
> --- a/lib/pages/main_layout_slim.dart
> +++ b/lib/pages/main_layout_slim.dart
> @@ -254,7 +254,7 @@ class MobileDashboard extends StatelessWidget {
>                                   fontWeight: FontWeight.bold,
>                                   color: Colors.white),
>                             ),
> -                          onPressed: () => showDialog(
> +                          onPressed: () => (

This does not seem to be related to the issue? The subscription dialog 
cannot be opened anymore with this change.

>                               context: context,
>                               builder: (c) => const PveSubscriptionAlertDialog(),
>                             ),
> @@ -556,7 +556,7 @@ class MobileDashboard extends StatelessWidget {
>     }
>   }
>   
> -class PveNodeListTile extends StatelessWidget {
> +class PveNodeListTile extends StatelessWidget{
>     final String name;
>     final bool online;
>     final String type;
> @@ -564,21 +564,58 @@ class PveNodeListTile extends StatelessWidget {
>     final String? ip;
>     const PveNodeListTile(
>         {super.key,
> -      required this.name,
> -      required this.online,
> -      required this.type,
> -      this.level,
> -      this.ip = ''});
> +        required this.name,
> +        required this.online,
> +        required this.type,
> +        this.level,
> +        this.ip = ''});
> +
>     @override
>     Widget build(BuildContext context) {
> +    late Color splashColor;
> +    if (online){
> +      splashColor = ProxmoxColors.greyShade40;
> +    }
> +    else {
> +      splashColor = Colors.transparent;
> +    }
>       return ListTile(
>         leading: Icon(
>           Renderers.getDefaultResourceIcon(type),
>         ),
>         title: Text(name),
> +      splashColor: splashColor,
>         subtitle: Text(getNodeTileSubtitle(online, level, ip)),
>         trailing: Icon(Icons.power, color: online ? Colors.green : Colors.grey),
> -      onTap: () => Navigator.pushNamed(context, '/nodes/$name'),
> +      onTap: () => {
> +        if (online){
> +          Navigator.pushNamed(context, '/nodes/$name')
> +        }
> +        else{
> +          showDialog(
> +            context: context,
> +            builder: (context) {
> +              return AlertDialog(
> +                contentPadding: const EdgeInsets.fromLTRB(24.0, 12.0, 24.0, 16.0),
> +                title: Row(
> +                  mainAxisAlignment: MainAxisAlignment.spaceBetween,
> +                  children: <Widget>[
> +                    Text('Error'),
> +                    Icon(Icons.warning),
> +                  ],
> +                ),
> +                content: Text("Node offline."),

nit: I think "Node is offline" would feel more polished. Or it should 
not have a period, since it's not a sentence.


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