[pve-devel] [PATCH flutter_frontend 1/1] fix #4976: ui: Nodes offline
Michael Köppl
m.koeppl at proxmox.com
Fri May 2 13:32:19 CEST 2025
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.
More information about the pve-devel
mailing list