From ded010a52fc8b46575db5f0c3668e78ec0676e23 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 4060115..59eee0e 100644 --- a/lib/l10n/app_en.arb +++ b/lib/l10n/app_en.arb @@ -2937,9 +2937,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 596f221..73743d1 100644 --- a/lib/l10n/app_localizations.dart +++ b/lib/l10n/app_localizations.dart @@ -8506,10 +8506,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 e4fb0d7..fe81087 100644 --- a/lib/l10n/app_localizations_bg.dart +++ b/lib/l10n/app_localizations_bg.dart @@ -5002,7 +5002,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 9f3d3d6..739a086 100644 --- a/lib/l10n/app_localizations_de.dart +++ b/lib/l10n/app_localizations_de.dart @@ -5019,7 +5019,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 553c94e..9876316 100644 --- a/lib/l10n/app_localizations_en.dart +++ b/lib/l10n/app_localizations_en.dart @@ -4925,7 +4925,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 4e0d96e..0592cc1 100644 --- a/lib/l10n/app_localizations_es.dart +++ b/lib/l10n/app_localizations_es.dart @@ -5007,7 +5007,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 12268d5..f34d90b 100644 --- a/lib/l10n/app_localizations_fr.dart +++ b/lib/l10n/app_localizations_fr.dart @@ -5035,7 +5035,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 f1147cc..68024dd 100644 --- a/lib/l10n/app_localizations_hu.dart +++ b/lib/l10n/app_localizations_hu.dart @@ -5024,7 +5024,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 ea7a665..40f9a6c 100644 --- a/lib/l10n/app_localizations_it.dart +++ b/lib/l10n/app_localizations_it.dart @@ -5012,7 +5012,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 7249e03..a3b47b0 100644 --- a/lib/l10n/app_localizations_ja.dart +++ b/lib/l10n/app_localizations_ja.dart @@ -4778,7 +4778,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 13a00d8..f7dcd01 100644 --- a/lib/l10n/app_localizations_ko.dart +++ b/lib/l10n/app_localizations_ko.dart @@ -4779,7 +4779,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 92b6fd7..520425f 100644 --- a/lib/l10n/app_localizations_nl.dart +++ b/lib/l10n/app_localizations_nl.dart @@ -4987,7 +4987,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 fac6303..b34b51d 100644 --- a/lib/l10n/app_localizations_pl.dart +++ b/lib/l10n/app_localizations_pl.dart @@ -5024,7 +5024,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 385512d..4caaa20 100644 --- a/lib/l10n/app_localizations_pt.dart +++ b/lib/l10n/app_localizations_pt.dart @@ -5000,7 +5000,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 ddda87d..9c646c5 100644 --- a/lib/l10n/app_localizations_ru.dart +++ b/lib/l10n/app_localizations_ru.dart @@ -5018,7 +5018,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 ea04832..9c7d1da 100644 --- a/lib/l10n/app_localizations_sk.dart +++ b/lib/l10n/app_localizations_sk.dart @@ -4984,7 +4984,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 bda4536..4d0838e 100644 --- a/lib/l10n/app_localizations_sl.dart +++ b/lib/l10n/app_localizations_sl.dart @@ -4982,7 +4982,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 e93f9cf..0a62185 100644 --- a/lib/l10n/app_localizations_sv.dart +++ b/lib/l10n/app_localizations_sv.dart @@ -4957,7 +4957,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 fd73f12..5f56209 100644 --- a/lib/l10n/app_localizations_uk.dart +++ b/lib/l10n/app_localizations_uk.dart @@ -5019,7 +5019,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 26c728c..24e6fff 100644 --- a/lib/l10n/app_localizations_zh.dart +++ b/lib/l10n/app_localizations_zh.dart @@ -4652,7 +4652,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));