From 2fefb6aa91d64a150ed1a267b6d195990ba61c2d Mon Sep 17 00:00:00 2001 From: Strycher Date: Mon, 20 Jul 2026 18:38:33 -0400 Subject: [PATCH] fix(#343): keep full message history; window only in memory The persisted store was being overwritten with the windowed in-memory list, so any channel or DM with more than _messageWindowSize (200) messages lost everything older than the newest 200 on the first save after load. Observed live: Public went 232 -> 201 in one session on a build that already had the #333 fix, so this was not the race - it was windowing truncating the store. This is the slow-erosion cause behind the whole 566 -> 231 -> 206 -> 201 history. saveChannelMessages / saveMessages now MERGE into the persisted set instead of overwriting: upsert by message identity so older persisted messages are kept, new ones added, and the in-memory copy wins for edits/reactions/status. If the existing history fails to decode, the save aborts loudly rather than merging into an empty base and truncating (SAFELANE 6). Deletion is now an explicit path - removeChannelMessage / removeMessage - since the app deletes individual messages. Routing delete through the merging save would resurrect them; the connector's deleteChannelMessage / deleteMessage now call the explicit remove. Windowing stays for display and memory; it no longer dictates what is stored. Tests: 250-message history survives a 200-window save; new message appends; edit is captured not duplicated; delete does not resurrect. 508 pass, analyze and format clean. Cost: a save now re-reads and re-encodes the channel/contact history. Cheap with drift's per-key writes (#335); a future append-only schema removes even that. --- lib/connector/meshcore_connector.dart | 8 +- lib/storage/channel_message_store.dart | 79 +++++++++++++- lib/storage/drift/blob_store.dart | 13 ++- lib/storage/message_store.dart | 66 +++++++++++- test/storage/channel_message_merge_test.dart | 103 +++++++++++++++++++ 5 files changed, 261 insertions(+), 8 deletions(-) create mode 100644 test/storage/channel_message_merge_test.dart diff --git a/lib/connector/meshcore_connector.dart b/lib/connector/meshcore_connector.dart index fdcfc98..c8e434b 100644 --- a/lib/connector/meshcore_connector.dart +++ b/lib/connector/meshcore_connector.dart @@ -699,7 +699,8 @@ class MeshCoreConnector extends ChangeNotifier { if (messages == null) return; final removed = messages.remove(message); if (!removed) return; - await _messageStore.saveMessages(contactKeyHex, messages); + // Explicit delete: saveMessages now merges and never removes (#343). + await _messageStore.removeMessage(contactKeyHex, message); notifyListeners(); } @@ -833,7 +834,10 @@ class MeshCoreConnector extends ChangeNotifier { if (messages == null) return; final removed = messages.remove(message); if (!removed) return; - await _channelMessageStore.saveChannelMessages(channelIndex, messages); + // Explicit delete path: saveChannelMessages now MERGES and never removes + // (#343), so deletion must go through removeChannelMessage or the message + // would be resurrected on the next save. + await _channelMessageStore.removeChannelMessage(channelIndex, message); notifyListeners(); } diff --git a/lib/storage/channel_message_store.dart b/lib/storage/channel_message_store.dart index e829b69..842a2d5 100644 --- a/lib/storage/channel_message_store.dart +++ b/lib/storage/channel_message_store.dart @@ -65,10 +65,83 @@ class ChannelMessageStore { ); return; } - final jsonList = messages.map((msg) => _messageToJson(msg)).toList(); + // Merge into the persisted full history rather than overwriting it. The + // in-memory list is windowed to the most recent N for memory, so a plain + // overwrite would truncate the store to N and erode old history (#343). + // Upsert by identity: keep older persisted messages, add new ones, and let + // the in-memory copy win so edits/reactions/status updates are captured. + // Deletion has its own path (removeChannelMessage) so this never + // resurrects a message the user deleted. + final key = _storageKey(channelIndex); + final byKey = {}; + + final existing = await BlobStore.instance.readWithPrefsFallback(key); + if (existing != null && existing.isNotEmpty) { + try { + for (final e in jsonDecode(existing) as List) { + final m = _messageFromJson(e as Map); + byKey[_mergeKey(m)] = m; + } + } catch (e) { + // SAFELANE 6: a decode failure here must be loud, not silently drop the + // persisted history by merging into an empty base. + appLogger.error( + 'Failed to decode existing channel $channelIndex history before ' + 'merge; aborting save to avoid truncation: $e', + tag: 'Storage', + ); + return; + } + } + for (final m in messages) { + byKey[_mergeKey(m)] = m; + } + + final merged = byKey.values.toList() + ..sort((a, b) => a.timestamp.compareTo(b.timestamp)); + await BlobStore.instance.write( + key, + jsonEncode(merged.map(_messageToJson).toList()), + ); + } + + /// Stable identity for merge/dedupe. messageId when present, else a composite + /// that distinguishes distinct messages that share no id. + String _mergeKey(ChannelMessage m) { + if (m.messageId.isNotEmpty) return 'id:${m.messageId}'; + return 'x:${m.packetHash ?? ''}:${m.timestamp.millisecondsSinceEpoch}:${m.text}'; + } + + /// Removes a single message from the persisted history. The explicit delete + /// path (#343): save merges and never removes, so deletion cannot go through + /// save. + Future removeChannelMessage( + int channelIndex, + ChannelMessage message, + ) async { + if (publicKeyHex.isEmpty) return; + final key = _storageKey(channelIndex); + final existing = await BlobStore.instance.readWithPrefsFallback(key); + if (existing == null || existing.isEmpty) return; + + final List raw; + try { + raw = jsonDecode(existing) as List; + } catch (e) { + appLogger.error( + 'Failed to decode channel $channelIndex history for delete: $e', + tag: 'Storage', + ); + return; + } + final target = _mergeKey(message); + final kept = raw + .map((e) => _messageFromJson(e as Map)) + .where((m) => _mergeKey(m) != target) + .toList(); await BlobStore.instance.write( - _storageKey(channelIndex), - jsonEncode(jsonList), + key, + jsonEncode(kept.map(_messageToJson).toList()), ); } diff --git a/lib/storage/drift/blob_store.dart b/lib/storage/drift/blob_store.dart index 6cab09b..8239c72 100644 --- a/lib/storage/drift/blob_store.dart +++ b/lib/storage/drift/blob_store.dart @@ -1,3 +1,5 @@ +import 'package:flutter/foundation.dart'; + import '../../utils/app_logger.dart'; import '../prefs_manager.dart'; import 'offband_database.dart'; @@ -19,10 +21,19 @@ class BlobStore { final OffbandDatabase _db; static OffbandDatabase? _sharedDb; + static BlobStore? _override; /// Process-wide instance. The database must be opened once; opening it twice /// is an error on the web backends. - static BlobStore get instance => BlobStore(_sharedDb ??= OffbandDatabase()); + static BlobStore get instance => + _override ?? BlobStore(_sharedDb ??= OffbandDatabase()); + + /// Test seam: point the singleton at an in-memory database. + @visibleForTesting + static void overrideForTest(BlobStore store) => _override = store; + + @visibleForTesting + static void clearTestOverride() => _override = null; /// Key families that hold bulk data. Everything else stays in /// SharedPreferences, which is what it is for. diff --git a/lib/storage/message_store.dart b/lib/storage/message_store.dart index 2d8b02d..b912d0e 100644 --- a/lib/storage/message_store.dart +++ b/lib/storage/message_store.dart @@ -24,9 +24,71 @@ class MessageStore { appLogger.warn('Public key hex is not set. Cannot save messages.'); return; } + // Merge into the persisted full history rather than overwriting it (#343). + // The in-memory list is windowed for memory, so a plain overwrite would + // truncate the store. Upsert by identity; deletion is explicit + // (removeMessage). final key = '$keyFor$contactKeyHex'; - final jsonList = messages.map(_messageToJson).toList(); - await BlobStore.instance.write(key, jsonEncode(jsonList)); + final byKey = {}; + + final existing = await BlobStore.instance.readWithPrefsFallback(key); + if (existing != null && existing.isNotEmpty) { + try { + for (final e in jsonDecode(existing) as List) { + final m = _messageFromJson(e as Map); + byKey[_mergeKey(m)] = m; + } + } catch (e) { + appLogger.error( + 'Failed to decode existing DM history before merge; aborting save ' + 'to avoid truncation: $e', + tag: 'Storage', + ); + return; + } + } + for (final m in messages) { + byKey[_mergeKey(m)] = m; + } + + final merged = byKey.values.toList() + ..sort((a, b) => a.timestamp.compareTo(b.timestamp)); + await BlobStore.instance.write( + key, + jsonEncode(merged.map(_messageToJson).toList()), + ); + } + + String _mergeKey(Message m) { + if (m.messageId.isNotEmpty) return 'id:${m.messageId}'; + return 'x:${m.senderKeyHex}:${m.timestamp.millisecondsSinceEpoch}:${m.text}'; + } + + /// Explicit delete path (#343): save merges and never removes. + Future removeMessage(String contactKeyHex, Message message) async { + if (publicKeyHex.isEmpty) return; + final key = '$keyFor$contactKeyHex'; + final existing = await BlobStore.instance.readWithPrefsFallback(key); + if (existing == null || existing.isEmpty) return; + final List raw; + try { + raw = jsonDecode(existing) as List; + } catch (e) { + appLogger.error( + 'Failed to decode DM history for delete: $e', + tag: 'Storage', + ); + return; + } + final target = _mergeKey(message); + final kept = raw + .map((e) => _messageFromJson(e as Map)) + .where((m) => _mergeKey(m) != target) + .toList(); + await BlobStore.instance.write( + key, + jsonEncode(kept.map(_messageToJson).toList()), + ); } Future> loadMessages(String contactKeyHex) async { diff --git a/test/storage/channel_message_merge_test.dart b/test/storage/channel_message_merge_test.dart new file mode 100644 index 0000000..83f6144 --- /dev/null +++ b/test/storage/channel_message_merge_test.dart @@ -0,0 +1,103 @@ +import 'package:drift/native.dart'; +import 'package:flutter_test/flutter_test.dart'; +import 'package:meshcore_open/models/channel_message.dart'; +import 'package:meshcore_open/storage/channel_message_store.dart'; +import 'package:meshcore_open/storage/drift/blob_store.dart'; +import 'package:meshcore_open/storage/drift/offband_database.dart'; +import 'package:meshcore_open/storage/prefs_manager.dart'; +import 'package:shared_preferences/shared_preferences.dart'; + +/// #343: saving a windowed in-memory list must not truncate the persisted +/// history. The store keeps full history; deletion is explicit. +void main() { + late OffbandDatabase db; + late ChannelMessageStore store; + + setUp(() async { + SharedPreferences.setMockInitialValues({}); + PrefsManager.reset(); + await PrefsManager.initialize(); + db = OffbandDatabase(NativeDatabase.memory()); + BlobStore.overrideForTest(BlobStore(db)); + store = ChannelMessageStore(); + store.setPublicKeyHex = 'a' * 20; + // No PSK resolver -> uses the slot-index key, which is fine for the test. + }); + + tearDown(() async { + await db.close(); + BlobStore.clearTestOverride(); + }); + + ChannelMessage msg(int i) => ChannelMessage( + senderName: 's', + text: 'm$i', + timestamp: DateTime.fromMillisecondsSinceEpoch(1000 + i), + isOutgoing: false, + channelIndex: 0, + messageId: 'id$i', + ); + + test('saving a 200-window does not truncate a 250-message history', () async { + // Seed 250 messages, as if a full history were persisted. + await store.saveChannelMessages(0, [for (var i = 0; i < 250; i++) msg(i)]); + expect((await store.loadChannelMessages(0)).length, 250); + + // The app now saves only the most-recent 200 (its in-memory window). + final window = [for (var i = 50; i < 250; i++) msg(i)]; + await store.saveChannelMessages(0, window); + + // Full history must survive: the older 50 are still there. + final all = await store.loadChannelMessages(0); + expect(all.length, 250, reason: 'the older 50 must not be dropped'); + expect(all.first.text, 'm0'); + expect(all.last.text, 'm249'); + }); + + test('a new message appends without dropping old history', () async { + await store.saveChannelMessages(0, [for (var i = 0; i < 10; i++) msg(i)]); + await store.saveChannelMessages(0, [msg(10)]); + final all = await store.loadChannelMessages(0); + expect(all.length, 11); + expect(all.last.text, 'm10'); + }); + + test('an edit to a message is captured, not duplicated', () async { + await store.saveChannelMessages(0, [msg(1)]); + final edited = ChannelMessage( + senderName: 's', + text: 'm1', + timestamp: DateTime.fromMillisecondsSinceEpoch(1001), + isOutgoing: false, + channelIndex: 0, + messageId: 'id1', + reactions: {'thumbsup': 2}, + ); + await store.saveChannelMessages(0, [edited]); + final all = await store.loadChannelMessages(0); + expect(all, hasLength(1)); + expect(all.single.reactions['thumbsup'], 2); + }); + + test('deleting a message does NOT resurrect it on the next save', () async { + await store.saveChannelMessages(0, [for (var i = 0; i < 5; i++) msg(i)]); + + // Delete via the explicit path. + await store.removeChannelMessage(0, msg(2)); + expect( + (await store.loadChannelMessages(0)).map((m) => m.text), + isNot(contains('m2')), + ); + + // A later save of the remaining in-memory list must not bring it back. + final remaining = [ + for (var i = 0; i < 5; i++) + if (i != 2) msg(i), + ]; + await store.saveChannelMessages(0, remaining); + + final all = await store.loadChannelMessages(0); + expect(all.map((m) => m.text), isNot(contains('m2'))); + expect(all, hasLength(4)); + }); +}