From 9741c3399de850e25eb751217933645bdaae9366 Mon Sep 17 00:00:00 2001 From: Strycher Date: Sat, 15 Aug 2026 18:01:53 -0400 Subject: [PATCH] fix(#575): apply pre-PR review, stop claiming unconfirmed import success Adversarial review raised three findings. Two survived verification. Rejected: a claimed RangeError when abbreviating a short public key. The code already guards with a length check before either substring, so the crash it describes cannot occur. Rejected as described, fixed as found: contact position (0, 0) was said to be dropped on import. It is not. The frame builder writes the position block whenever lastModified is present, which it always is here, so a suppressed position still writes 0/0 and the bytes are identical either way. The hasPosition conditional was therefore doing nothing except misleading a reader, which is exactly what happened. Removed, and the behavior is pinned with a test. Accepted: the service added sections to 'applied' after sendFrame returned, which only means the frame left our side. A lost or refused frame was indistinguishable from success, so the user could be told an import worked when nothing changed on the device. A correct fix needs an awaited per-command acknowledgement in the connector, keyed on command code so concurrent commands cannot steal each other's OK. That is real connector work and is filed as #584. What is fixed here is the overclaim: 'applied' and the two counts are documented as sent rather than confirmed, and the result string now reads 'Sent N contacts and M channels to the device'. The code no longer states something it cannot know. --- lib/l10n/app_en.arb | 4 ++-- lib/l10n/app_localizations.dart | 4 ++-- lib/l10n/app_localizations_bg.dart | 2 +- lib/l10n/app_localizations_de.dart | 2 +- lib/l10n/app_localizations_en.dart | 2 +- lib/l10n/app_localizations_es.dart | 2 +- lib/l10n/app_localizations_fr.dart | 2 +- lib/l10n/app_localizations_hu.dart | 2 +- lib/l10n/app_localizations_it.dart | 2 +- lib/l10n/app_localizations_ja.dart | 2 +- lib/l10n/app_localizations_ko.dart | 2 +- lib/l10n/app_localizations_nl.dart | 2 +- lib/l10n/app_localizations_pl.dart | 2 +- lib/l10n/app_localizations_pt.dart | 2 +- lib/l10n/app_localizations_ru.dart | 2 +- lib/l10n/app_localizations_sk.dart | 2 +- lib/l10n/app_localizations_sl.dart | 2 +- lib/l10n/app_localizations_sv.dart | 2 +- lib/l10n/app_localizations_uk.dart | 2 +- lib/l10n/app_localizations_zh.dart | 2 +- lib/services/stock_config_import_service.dart | 24 ++++++++++++++++--- .../stock_config_export_service_test.dart | 13 ++++++++++ 22 files changed, 56 insertions(+), 25 deletions(-) diff --git a/lib/l10n/app_en.arb b/lib/l10n/app_en.arb index 6718634..7b714f5 100644 --- a/lib/l10n/app_en.arb +++ b/lib/l10n/app_en.arb @@ -3002,9 +3002,9 @@ "@importConfig_resultTitle": { "description": "Title of the dialog summarising what an import did (#576)." }, - "importConfig_resultCounts": "{contacts} contacts written, {channels} channels added.", + "importConfig_resultCounts": "Sent {contacts} contacts and {channels} channels to the device.", "@importConfig_resultCounts": { - "description": "Summary counts after an import (#576).", + "description": "Summary counts after an import. Deliberately says sent, not applied: apart from identity these commands are not confirmed by the device yet (#576).", "placeholders": { "contacts": { "type": "int" }, "channels": { "type": "int" } diff --git a/lib/l10n/app_localizations.dart b/lib/l10n/app_localizations.dart index 561451a..944bccf 100644 --- a/lib/l10n/app_localizations.dart +++ b/lib/l10n/app_localizations.dart @@ -8710,10 +8710,10 @@ abstract class AppLocalizations { /// **'Import finished'** String get importConfig_resultTitle; - /// Summary counts after an import (#576). + /// Summary counts after an import. Deliberately says sent, not applied: apart from identity these commands are not confirmed by the device yet (#576). /// /// In en, this message translates to: - /// **'{contacts} contacts written, {channels} channels added.'** + /// **'Sent {contacts} contacts and {channels} channels to the device.'** String importConfig_resultCounts(int contacts, int channels); /// Heading above the list of channels that were skipped (#576). diff --git a/lib/l10n/app_localizations_bg.dart b/lib/l10n/app_localizations_bg.dart index 38e4342..38fa6e8 100644 --- a/lib/l10n/app_localizations_bg.dart +++ b/lib/l10n/app_localizations_bg.dart @@ -5127,7 +5127,7 @@ class AppLocalizationsBg extends AppLocalizations { @override String importConfig_resultCounts(int contacts, int channels) { - return '$contacts contacts written, $channels channels added.'; + return 'Sent $contacts contacts and $channels channels to the device.'; } @override diff --git a/lib/l10n/app_localizations_de.dart b/lib/l10n/app_localizations_de.dart index 44e7f2a..0deceb7 100644 --- a/lib/l10n/app_localizations_de.dart +++ b/lib/l10n/app_localizations_de.dart @@ -5144,7 +5144,7 @@ class AppLocalizationsDe extends AppLocalizations { @override String importConfig_resultCounts(int contacts, int channels) { - return '$contacts contacts written, $channels channels added.'; + return 'Sent $contacts contacts and $channels channels to the device.'; } @override diff --git a/lib/l10n/app_localizations_en.dart b/lib/l10n/app_localizations_en.dart index 0820e4a..cbc29c2 100644 --- a/lib/l10n/app_localizations_en.dart +++ b/lib/l10n/app_localizations_en.dart @@ -5050,7 +5050,7 @@ class AppLocalizationsEn extends AppLocalizations { @override String importConfig_resultCounts(int contacts, int channels) { - return '$contacts contacts written, $channels channels added.'; + return 'Sent $contacts contacts and $channels channels to the device.'; } @override diff --git a/lib/l10n/app_localizations_es.dart b/lib/l10n/app_localizations_es.dart index 17dbc7f..15ed94b 100644 --- a/lib/l10n/app_localizations_es.dart +++ b/lib/l10n/app_localizations_es.dart @@ -5132,7 +5132,7 @@ class AppLocalizationsEs extends AppLocalizations { @override String importConfig_resultCounts(int contacts, int channels) { - return '$contacts contacts written, $channels channels added.'; + return 'Sent $contacts contacts and $channels channels to the device.'; } @override diff --git a/lib/l10n/app_localizations_fr.dart b/lib/l10n/app_localizations_fr.dart index c8e3ff2..f23e757 100644 --- a/lib/l10n/app_localizations_fr.dart +++ b/lib/l10n/app_localizations_fr.dart @@ -5160,7 +5160,7 @@ class AppLocalizationsFr extends AppLocalizations { @override String importConfig_resultCounts(int contacts, int channels) { - return '$contacts contacts written, $channels channels added.'; + return 'Sent $contacts contacts and $channels channels to the device.'; } @override diff --git a/lib/l10n/app_localizations_hu.dart b/lib/l10n/app_localizations_hu.dart index 18c345c..89d0184 100644 --- a/lib/l10n/app_localizations_hu.dart +++ b/lib/l10n/app_localizations_hu.dart @@ -5149,7 +5149,7 @@ class AppLocalizationsHu extends AppLocalizations { @override String importConfig_resultCounts(int contacts, int channels) { - return '$contacts contacts written, $channels channels added.'; + return 'Sent $contacts contacts and $channels channels to the device.'; } @override diff --git a/lib/l10n/app_localizations_it.dart b/lib/l10n/app_localizations_it.dart index d7d9c6b..2be1a8a 100644 --- a/lib/l10n/app_localizations_it.dart +++ b/lib/l10n/app_localizations_it.dart @@ -5137,7 +5137,7 @@ class AppLocalizationsIt extends AppLocalizations { @override String importConfig_resultCounts(int contacts, int channels) { - return '$contacts contacts written, $channels channels added.'; + return 'Sent $contacts contacts and $channels channels to the device.'; } @override diff --git a/lib/l10n/app_localizations_ja.dart b/lib/l10n/app_localizations_ja.dart index a7077e8..8f47415 100644 --- a/lib/l10n/app_localizations_ja.dart +++ b/lib/l10n/app_localizations_ja.dart @@ -4903,7 +4903,7 @@ class AppLocalizationsJa extends AppLocalizations { @override String importConfig_resultCounts(int contacts, int channels) { - return '$contacts contacts written, $channels channels added.'; + return 'Sent $contacts contacts and $channels channels to the device.'; } @override diff --git a/lib/l10n/app_localizations_ko.dart b/lib/l10n/app_localizations_ko.dart index c01e5a8..48e5c43 100644 --- a/lib/l10n/app_localizations_ko.dart +++ b/lib/l10n/app_localizations_ko.dart @@ -4904,7 +4904,7 @@ class AppLocalizationsKo extends AppLocalizations { @override String importConfig_resultCounts(int contacts, int channels) { - return '$contacts contacts written, $channels channels added.'; + return 'Sent $contacts contacts and $channels channels to the device.'; } @override diff --git a/lib/l10n/app_localizations_nl.dart b/lib/l10n/app_localizations_nl.dart index 01b8659..b6e8f41 100644 --- a/lib/l10n/app_localizations_nl.dart +++ b/lib/l10n/app_localizations_nl.dart @@ -5112,7 +5112,7 @@ class AppLocalizationsNl extends AppLocalizations { @override String importConfig_resultCounts(int contacts, int channels) { - return '$contacts contacts written, $channels channels added.'; + return 'Sent $contacts contacts and $channels channels to the device.'; } @override diff --git a/lib/l10n/app_localizations_pl.dart b/lib/l10n/app_localizations_pl.dart index d937261..72787bc 100644 --- a/lib/l10n/app_localizations_pl.dart +++ b/lib/l10n/app_localizations_pl.dart @@ -5149,7 +5149,7 @@ class AppLocalizationsPl extends AppLocalizations { @override String importConfig_resultCounts(int contacts, int channels) { - return '$contacts contacts written, $channels channels added.'; + return 'Sent $contacts contacts and $channels channels to the device.'; } @override diff --git a/lib/l10n/app_localizations_pt.dart b/lib/l10n/app_localizations_pt.dart index 028b13a..9ee389f 100644 --- a/lib/l10n/app_localizations_pt.dart +++ b/lib/l10n/app_localizations_pt.dart @@ -5125,7 +5125,7 @@ class AppLocalizationsPt extends AppLocalizations { @override String importConfig_resultCounts(int contacts, int channels) { - return '$contacts contacts written, $channels channels added.'; + return 'Sent $contacts contacts and $channels channels to the device.'; } @override diff --git a/lib/l10n/app_localizations_ru.dart b/lib/l10n/app_localizations_ru.dart index 2d6f984..be63297 100644 --- a/lib/l10n/app_localizations_ru.dart +++ b/lib/l10n/app_localizations_ru.dart @@ -5143,7 +5143,7 @@ class AppLocalizationsRu extends AppLocalizations { @override String importConfig_resultCounts(int contacts, int channels) { - return '$contacts contacts written, $channels channels added.'; + return 'Sent $contacts contacts and $channels channels to the device.'; } @override diff --git a/lib/l10n/app_localizations_sk.dart b/lib/l10n/app_localizations_sk.dart index 47e3c56..4900c0b 100644 --- a/lib/l10n/app_localizations_sk.dart +++ b/lib/l10n/app_localizations_sk.dart @@ -5109,7 +5109,7 @@ class AppLocalizationsSk extends AppLocalizations { @override String importConfig_resultCounts(int contacts, int channels) { - return '$contacts contacts written, $channels channels added.'; + return 'Sent $contacts contacts and $channels channels to the device.'; } @override diff --git a/lib/l10n/app_localizations_sl.dart b/lib/l10n/app_localizations_sl.dart index d1bdb6c..bcc442a 100644 --- a/lib/l10n/app_localizations_sl.dart +++ b/lib/l10n/app_localizations_sl.dart @@ -5107,7 +5107,7 @@ class AppLocalizationsSl extends AppLocalizations { @override String importConfig_resultCounts(int contacts, int channels) { - return '$contacts contacts written, $channels channels added.'; + return 'Sent $contacts contacts and $channels channels to the device.'; } @override diff --git a/lib/l10n/app_localizations_sv.dart b/lib/l10n/app_localizations_sv.dart index b29b836..bb318db 100644 --- a/lib/l10n/app_localizations_sv.dart +++ b/lib/l10n/app_localizations_sv.dart @@ -5082,7 +5082,7 @@ class AppLocalizationsSv extends AppLocalizations { @override String importConfig_resultCounts(int contacts, int channels) { - return '$contacts contacts written, $channels channels added.'; + return 'Sent $contacts contacts and $channels channels to the device.'; } @override diff --git a/lib/l10n/app_localizations_uk.dart b/lib/l10n/app_localizations_uk.dart index 6dc9ca7..267b72e 100644 --- a/lib/l10n/app_localizations_uk.dart +++ b/lib/l10n/app_localizations_uk.dart @@ -5144,7 +5144,7 @@ class AppLocalizationsUk extends AppLocalizations { @override String importConfig_resultCounts(int contacts, int channels) { - return '$contacts contacts written, $channels channels added.'; + return 'Sent $contacts contacts and $channels channels to the device.'; } @override diff --git a/lib/l10n/app_localizations_zh.dart b/lib/l10n/app_localizations_zh.dart index ee344b1..6c2db17 100644 --- a/lib/l10n/app_localizations_zh.dart +++ b/lib/l10n/app_localizations_zh.dart @@ -4777,7 +4777,7 @@ class AppLocalizationsZh extends AppLocalizations { @override String importConfig_resultCounts(int contacts, int channels) { - return '$contacts contacts written, $channels channels added.'; + return 'Sent $contacts contacts and $channels channels to the device.'; } @override diff --git a/lib/services/stock_config_import_service.dart b/lib/services/stock_config_import_service.dart index ae64dd4..39f574e 100644 --- a/lib/services/stock_config_import_service.dart +++ b/lib/services/stock_config_import_service.dart @@ -68,14 +68,27 @@ class StockConfigImportResult { required this.channelsAdded, }); + /// Sections whose commands were **sent** to the device without error. + /// + /// This is not device confirmation. Apart from the identity import, which + /// waits for a real OK or ERR, the firmware's generic OK for these commands + /// is not currently awaited, so a frame lost or refused after it left our + /// side would still land here. Wiring per-command confirmation is tracked + /// separately; until then this set means "sent", and the UI says so rather + /// than claiming more than we know. final Set applied; + final Map failed; /// Channels in the file that were not written, each with a reason. Shown to /// the user; never discarded. final List skippedChannels; + /// Contacts whose write command was sent. See [applied] for why this is a + /// send count and not a confirmed count. final int contactsWritten; + + /// Channels whose write command was sent. Same caveat as [contactsWritten]. final int channelsAdded; bool get isComplete => failed.isEmpty && skippedChannels.isEmpty; @@ -280,7 +293,12 @@ class StockConfigImportService { var written = 0; for (final contact in contacts) { final path = contact.outPath; - final hasPosition = contact.latitude != 0 || contact.longitude != 0; + // Position is always passed through, including (0, 0). Suppressing zero + // would be pointless here: the frame builder emits the position block + // whenever lastModified is present, which it always is below, so a + // suppressed position writes 0/0 anyway. Passing it straight through + // means the file's value is what lands, with no special case to + // misread. await _connector.sendFrame( buildUpdateContactPathFrame( contact.publicKey, @@ -291,8 +309,8 @@ class StockConfigImportService { type: contact.type, flags: contact.flags, name: contact.name, - lat: hasPosition ? contact.latitude : null, - lon: hasPosition ? contact.longitude : null, + lat: contact.latitude, + lon: contact.longitude, lastModified: DateTime.fromMillisecondsSinceEpoch( contact.lastModified * 1000, ), diff --git a/test/services/stock_config_export_service_test.dart b/test/services/stock_config_export_service_test.dart index a994883..3ed8db8 100644 --- a/test/services/stock_config_export_service_test.dart +++ b/test/services/stock_config_export_service_test.dart @@ -155,6 +155,19 @@ void main() { expect(stock.outPath, isNull); }); + test('a zero position stays zero rather than becoming absent', () { + // Review flagged (0, 0) as a lost position on import. It is not: the + // frame builder writes the position block whenever lastModified is + // present, so zero lands as zero. Pinned here so the export side of the + // pair cannot start emitting something else. + final stock = toStockContact(makeContact(latitude: 0, longitude: 0)); + + expect(stock.latitude, 0); + expect(stock.longitude, 0); + expect(stock.toJson()['latitude'], '0.0'); + expect(stock.toJson()['longitude'], '0.0'); + }); + test('flags and type pass through untouched', () { final stock = toStockContact(makeContact(type: 3, flags: 15));