From d3d293a3005baa8e58a2bfe2c87e0290fb07d87a Mon Sep 17 00:00:00 2001 From: Strycher Date: Mon, 22 Jun 2026 22:49:16 -0400 Subject: [PATCH] feat(#80): broker editor screen (staged save, validation, dirty guard) The edit UI for one broker slot. Staged-save: controls edit local state; Save sends only the CHANGED fields through saveBroker (enabled written last), so a partial save can't leave a live-corrupt broker. - Fields: url, port, transport, auth_type, username, write-only password, topic_prefix, iata_override, jwt_* (when auth=jwt), ca_cert (when tls/wss), plus the enabled toggle. - Validation before any SET: port 1-65535, jwt audience+owner when auth=jwt. - Write-only password: sent only when typed; blank keeps the stored secret. - Refresh: single-slot re-GET (getBroker) + reseed, with a dirty discard guard. - Back button guards unsaved edits (PopScope -> Discard? dialog) -- Gemini's state-loss finding. 4/4 widget tests green. Brokers list screen + Observer-pane wiring next. Co-Authored-By: Claude Opus 4.8 --- .../settings/broker_editor_screen.dart | 376 ++++++++++++++++++ test/screens/broker_editor_screen_test.dart | 175 ++++++++ 2 files changed, 551 insertions(+) create mode 100644 lib/screens/settings/broker_editor_screen.dart create mode 100644 test/screens/broker_editor_screen_test.dart diff --git a/lib/screens/settings/broker_editor_screen.dart b/lib/screens/settings/broker_editor_screen.dart new file mode 100644 index 0000000..00eeda2 --- /dev/null +++ b/lib/screens/settings/broker_editor_screen.dart @@ -0,0 +1,376 @@ +import 'package:flutter/material.dart'; +import 'package:flutter/services.dart'; +import 'package:provider/provider.dart'; + +import '../../models/observer_config.dart'; +import '../../services/observer_config_service.dart'; + +/// Edit one MQTT broker slot (#80). Staged-save: controls edit LOCAL state and +/// nothing reaches the device until Save, which writes only the CHANGED fields +/// field-at-a-time with `enabled` written LAST (the activation guard) via +/// [ObserverConfigService.saveBroker]. A partial save leaves the slot disabled, +/// never live-corrupt. Refresh re-reads the single slot; the back button guards +/// unsaved edits. +class BrokerEditorScreen extends StatefulWidget { + const BrokerEditorScreen({super.key, required this.broker}); + + final BrokerConfig broker; + + @override + State createState() => _BrokerEditorScreenState(); +} + +class _BrokerEditorScreenState extends State { + late final TextEditingController _url; + late final TextEditingController _port; + late final TextEditingController _username; + final _password = TextEditingController(); + late final TextEditingController _topicPrefix; + late final TextEditingController _iataOverride; + late final TextEditingController _jwtAudience; + late final TextEditingController _jwtRefresh; + late final TextEditingController _jwtOwner; + late final TextEditingController _jwtEmail; + late final TextEditingController _caCert; + late BrokerTransport _transport; + late BrokerAuthType _authType; + late bool _enabled; + + /// The device snapshot the form is diffed against. Updated by Refresh so a + /// re-read becomes the new "unchanged" baseline. + late BrokerConfig _baseline; + bool _busy = false; + + String get _portText => _baseline.isPopulated ? '${_baseline.port}' : ''; + String get _jwtRefreshText => + _baseline.jwtRefresh == 0 ? '' : '${_baseline.jwtRefresh}'; + + @override + void initState() { + super.initState(); + _baseline = widget.broker; + _url = TextEditingController(text: _baseline.url); + _port = TextEditingController(text: _portText); + _username = TextEditingController(text: _baseline.username); + _topicPrefix = TextEditingController(text: _baseline.topicPrefix); + _iataOverride = TextEditingController(text: _baseline.iataOverride); + _jwtAudience = TextEditingController(text: _baseline.jwtAudience); + _jwtRefresh = TextEditingController(text: _jwtRefreshText); + _jwtOwner = TextEditingController(text: _baseline.jwtOwner); + _jwtEmail = TextEditingController(text: _baseline.jwtEmail); + _caCert = TextEditingController(text: _baseline.caCert); + _transport = _baseline.transport; + _authType = _baseline.authType; + _enabled = _baseline.enabled; + } + + @override + void dispose() { + for (final c in [ + _url, + _port, + _username, + _password, + _topicPrefix, + _iataOverride, + _jwtAudience, + _jwtRefresh, + _jwtOwner, + _jwtEmail, + _caCert, + ]) { + c.dispose(); + } + super.dispose(); + } + + /// Fields whose local value differs from [_baseline]. Password is write-only: + /// it is sent ONLY when the user typed a new one (the stored value is never + /// read back, so a blank field keeps it). + Map _changedFields() { + final f = {}; + void diff(String key, String now, String was) { + if (now != was) f[key] = now; + } + + diff('url', _url.text, _baseline.url); + diff('port', _port.text, _portText); + if (_transport != _baseline.transport) f['transport'] = _transport.wire; + if (_authType != _baseline.authType) f['auth_type'] = _authType.wire; + diff('username', _username.text, _baseline.username); + diff('topic_prefix', _topicPrefix.text, _baseline.topicPrefix); + diff('iata_override', _iataOverride.text, _baseline.iataOverride); + diff('jwt_audience', _jwtAudience.text, _baseline.jwtAudience); + diff('jwt_refresh', _jwtRefresh.text, _jwtRefreshText); + diff('jwt_owner', _jwtOwner.text, _baseline.jwtOwner); + diff('jwt_email', _jwtEmail.text, _baseline.jwtEmail); + diff('ca_cert', _caCert.text, _baseline.caCert); + if (_password.text.isNotEmpty) f['password'] = _password.text; + return f; + } + + bool get _dirty => + _changedFields().isNotEmpty || _enabled != _baseline.enabled; + + /// Reject predictably-bad values before they reach the wire (#80). Returns an + /// error message, or null when the form is valid. + String? _validate() { + if (_url.text.trim().isEmpty) return 'URL is required'; + final port = int.tryParse(_port.text.trim()); + if (port == null || port < 1 || port > 65535) { + return 'Port must be between 1 and 65535'; + } + if (_authType == BrokerAuthType.jwt && + (_jwtAudience.text.trim().isEmpty || _jwtOwner.text.trim().isEmpty)) { + return 'JWT auth requires an audience and an owner'; + } + return null; + } + + void _snack(String msg, {bool isError = false}) { + ScaffoldMessenger.of(context).showSnackBar( + SnackBar( + content: Text(msg), + backgroundColor: isError ? Theme.of(context).colorScheme.error : null, + ), + ); + } + + Future _save() async { + final err = _validate(); + if (err != null) { + _snack(err, isError: true); + return; + } + final svc = context.read(); + final messenger = ScaffoldMessenger.of(context); + final navigator = Navigator.of(context); + final errorColor = Theme.of(context).colorScheme.error; + setState(() => _busy = true); + final result = await svc.saveBroker( + _baseline.slot, + fields: _changedFields(), + enable: _enabled, + wasLive: _baseline.enabled, + ); + if (!mounted) return; + setState(() => _busy = false); + if (result.ok) { + messenger.showSnackBar( + SnackBar(content: Text('Broker ${_baseline.slot} saved')), + ); + navigator.pop(true); + } else { + messenger.showSnackBar( + SnackBar( + content: Text( + 'Save failed at "${result.failedField ?? 'a field'}" — the slot ' + 'was left disabled. Re-read and retry.', + ), + backgroundColor: errorColor, + ), + ); + } + } + + Future _refresh() async { + final svc = context.read(); + final messenger = ScaffoldMessenger.of(context); + final errorColor = Theme.of(context).colorScheme.error; + if (_dirty && !await _confirmDiscard()) return; + if (!mounted) return; + setState(() => _busy = true); + final fresh = await svc.getBroker(_baseline.slot); + if (!mounted) return; + setState(() => _busy = false); + if (fresh == null) { + messenger.showSnackBar( + SnackBar( + content: Text('Could not read broker ${_baseline.slot}'), + backgroundColor: errorColor, + ), + ); + return; + } + _seedFrom(fresh); + } + + void _seedFrom(BrokerConfig b) { + setState(() { + _baseline = b; + _url.text = b.url; + _port.text = _portText; + _username.text = b.username; + _topicPrefix.text = b.topicPrefix; + _iataOverride.text = b.iataOverride; + _jwtAudience.text = b.jwtAudience; + _jwtRefresh.text = _jwtRefreshText; + _jwtOwner.text = b.jwtOwner; + _jwtEmail.text = b.jwtEmail; + _caCert.text = b.caCert; + _transport = b.transport; + _authType = b.authType; + _enabled = b.enabled; + _password.clear(); + }); + } + + Future _confirmDiscard() async { + final discard = await showDialog( + context: context, + builder: (c) => AlertDialog( + title: const Text('Discard changes?'), + content: const Text('Your edits to this broker have not been saved.'), + actions: [ + TextButton( + onPressed: () => Navigator.pop(c, false), + child: const Text('Keep editing'), + ), + TextButton( + onPressed: () => Navigator.pop(c, true), + child: const Text('Discard'), + ), + ], + ), + ); + return discard ?? false; + } + + @override + Widget build(BuildContext context) { + return PopScope( + canPop: !_dirty, + onPopInvokedWithResult: (didPop, _) async { + if (didPop) return; + final navigator = Navigator.of(context); + if (await _confirmDiscard()) navigator.pop(); + }, + child: Scaffold( + appBar: AppBar( + centerTitle: true, + title: Text('Broker ${_baseline.slot}'), + actions: [ + IconButton( + icon: const Icon(Icons.refresh), + tooltip: 'Refresh', + onPressed: _busy ? null : _refresh, + ), + IconButton( + icon: const Icon(Icons.save), + tooltip: 'Save', + onPressed: _busy ? null : _save, + ), + ], + ), + body: ListView( + padding: const EdgeInsets.all(16), + children: [ + SwitchListTile( + contentPadding: EdgeInsets.zero, + title: const Text('Enabled'), + subtitle: const Text('Written last on save (activation guard)'), + value: _enabled, + onChanged: (v) => setState(() => _enabled = v), + ), + const Divider(height: 24), + _field('broker_url', _url, 'URL'), + _field('broker_port', _port, 'Port', number: true), + const SizedBox(height: 12), + _sectionLabel('Transport'), + SegmentedButton( + segments: const [ + ButtonSegment(value: BrokerTransport.tcp, label: Text('tcp')), + ButtonSegment(value: BrokerTransport.tls, label: Text('tls')), + ButtonSegment(value: BrokerTransport.wss, label: Text('wss')), + ], + selected: {_transport}, + onSelectionChanged: (s) => setState(() => _transport = s.first), + ), + const SizedBox(height: 16), + _sectionLabel('Auth'), + SegmentedButton( + segments: const [ + ButtonSegment(value: BrokerAuthType.none, label: Text('none')), + ButtonSegment( + value: BrokerAuthType.basic, + label: Text('basic'), + ), + ButtonSegment(value: BrokerAuthType.jwt, label: Text('jwt')), + ], + selected: {_authType}, + onSelectionChanged: (s) => setState(() => _authType = s.first), + ), + if (_authType == BrokerAuthType.basic) ...[ + const SizedBox(height: 12), + _field('broker_username', _username, 'Username'), + _secretField(), + ], + if (_authType == BrokerAuthType.jwt) ...[ + const SizedBox(height: 12), + _field('broker_jwt_audience', _jwtAudience, 'JWT audience'), + _field('broker_jwt_owner', _jwtOwner, 'JWT owner'), + _field('broker_jwt_email', _jwtEmail, 'JWT email'), + _field( + 'broker_jwt_refresh', + _jwtRefresh, + 'JWT refresh (sec)', + number: true, + ), + ], + const Divider(height: 24), + _field('broker_topic_prefix', _topicPrefix, 'Topic prefix'), + _field('broker_iata_override', _iataOverride, 'IATA override'), + if (_transport != BrokerTransport.tcp) + _field( + 'broker_ca_cert', + _caCert, + 'CA certificate (PEM)', + lines: 3, + ), + ], + ), + ), + ); + } + + Widget _sectionLabel(String t) => Padding( + padding: const EdgeInsets.only(bottom: 6), + child: Text(t, style: Theme.of(context).textTheme.labelLarge), + ); + + Widget _field( + String key, + TextEditingController c, + String label, { + bool number = false, + int lines = 1, + }) => Padding( + padding: const EdgeInsets.only(bottom: 12), + child: TextField( + key: Key(key), + controller: c, + maxLines: lines, + keyboardType: number ? TextInputType.number : null, + inputFormatters: number ? [FilteringTextInputFormatter.digitsOnly] : null, + decoration: InputDecoration( + labelText: label, + border: const OutlineInputBorder(), + ), + ), + ); + + Widget _secretField() => Padding( + padding: const EdgeInsets.only(bottom: 12), + child: TextField( + key: const Key('broker_password'), + controller: _password, + obscureText: true, + decoration: InputDecoration( + labelText: _baseline.passwordSet + ? 'Password (set — leave blank to keep)' + : 'Password', + border: const OutlineInputBorder(), + ), + ), + ); +} diff --git a/test/screens/broker_editor_screen_test.dart b/test/screens/broker_editor_screen_test.dart new file mode 100644 index 0000000..662be25 --- /dev/null +++ b/test/screens/broker_editor_screen_test.dart @@ -0,0 +1,175 @@ +// Widget tests for the broker editor (#80). +// +// Pins the save contract the firmware depends on: only CHANGED fields go on the +// wire, a blank password keeps the stored one (write-only), `enable`/`wasLive` +// are derived from the slot, and a predictably-invalid value never reaches the +// device. Refresh re-reads the single slot and reseeds the form. + +import 'package:flutter/material.dart'; +import 'package:flutter_test/flutter_test.dart'; +import 'package:provider/provider.dart'; +import 'package:meshcore_open/connector/meshcore_connector.dart'; +import 'package:meshcore_open/models/observer_config.dart'; +import 'package:meshcore_open/screens/settings/broker_editor_screen.dart'; +import 'package:meshcore_open/services/observer_config_service.dart'; + +class _DummyConn extends MeshCoreConnector {} + +class _SaveCall { + _SaveCall(this.slot, this.fields, this.enable, this.wasLive); + final int slot; + final Map fields; + final bool enable; + final bool wasLive; +} + +class _FakeSvc extends ObserverConfigService { + _FakeSvc() : super(_DummyConn()); + + final List<_SaveCall> saveCalls = []; + BrokerSaveResult result = const BrokerSaveResult.ok(); + BrokerConfig? fresh; + + @override + Future saveBroker( + int slot, { + required Map fields, + required bool enable, + required bool wasLive, + }) async { + saveCalls.add(_SaveCall(slot, Map.of(fields), enable, wasLive)); + return result; + } + + @override + Future getBroker(int slot) async => fresh; +} + +// Push the editor as a second route so its pop-on-success has somewhere to go. +Future _open( + WidgetTester tester, + _FakeSvc fake, + BrokerConfig broker, +) async { + await tester.pumpWidget( + ChangeNotifierProvider.value( + value: fake, + child: MaterialApp( + home: Scaffold( + body: Builder( + builder: (context) => ElevatedButton( + onPressed: () => Navigator.of(context).push( + MaterialPageRoute( + builder: (_) => BrokerEditorScreen(broker: broker), + ), + ), + child: const Text('open'), + ), + ), + ), + ), + ), + ); + await tester.tap(find.text('open')); + await tester.pumpAndSettle(); +} + +Future _drainSnack(WidgetTester tester) async { + await tester.pump(const Duration(seconds: 5)); + await tester.pumpAndSettle(); +} + +void main() { + testWidgets('saves only changed fields; enable/wasLive come from the slot', ( + tester, + ) async { + final fake = _FakeSvc(); + await _open( + tester, + fake, + const BrokerConfig(slot: 2, url: 'old', port: 1883, enabled: true), + ); + + await tester.enterText(find.byKey(const Key('broker_url')), 'newhost'); + await tester.tap(find.byTooltip('Save')); + await tester.pumpAndSettle(); + + expect(fake.saveCalls, hasLength(1)); + final c = fake.saveCalls.single; + expect(c.slot, 2); + expect(c.fields['url'], 'newhost'); + expect( + c.fields.containsKey('port'), + isFalse, + reason: 'an unchanged field must not be re-sent', + ); + expect(c.wasLive, isTrue, reason: 'the slot was enabled on the device'); + expect(c.enable, isTrue); + await _drainSnack(tester); + }); + + testWidgets('an out-of-range port blocks the save (never reaches the wire)', ( + tester, + ) async { + final fake = _FakeSvc(); + await _open( + tester, + fake, + const BrokerConfig(slot: 1, url: 'h', port: 1883), + ); + + await tester.enterText(find.byKey(const Key('broker_port')), '99999'); + await tester.tap(find.byTooltip('Save')); + await tester.pumpAndSettle(); + + expect(fake.saveCalls, isEmpty); + expect(find.textContaining('Port must be'), findsOneWidget); + await _drainSnack(tester); + }); + + testWidgets('a blank password is never sent (write-only keep)', ( + tester, + ) async { + final fake = _FakeSvc(); + await _open( + tester, + fake, + const BrokerConfig( + slot: 0, + url: 'h', + port: 1883, + authType: BrokerAuthType.basic, + passwordSet: true, + ), + ); + + await tester.enterText(find.byKey(const Key('broker_username')), 'newuser'); + await tester.tap(find.byTooltip('Save')); + await tester.pumpAndSettle(); + + final c = fake.saveCalls.single; + expect(c.fields['username'], 'newuser'); + expect( + c.fields.containsKey('password'), + isFalse, + reason: 'a blank password field keeps the stored secret', + ); + await _drainSnack(tester); + }); + + testWidgets('Refresh re-reads the slot and reseeds the form', (tester) async { + final fake = _FakeSvc() + ..fresh = const BrokerConfig(slot: 3, url: 'fromdevice', port: 8883); + await _open( + tester, + fake, + const BrokerConfig(slot: 3, url: 'stale', port: 1883), + ); + + await tester.tap(find.byTooltip('Refresh')); + await tester.pumpAndSettle(); + + expect(find.text('fromdevice'), findsOneWidget); + expect(find.text('8883'), findsOneWidget); + }); +}