fix: resolve unknown sender in group notifications and infinite loading spinner#3062
fix: resolve unknown sender in group notifications and infinite loading spinner#3062blue-archon wants to merge 5 commits into
Conversation
|
I can't reproduce the original rows anymore so I can't say with certainty whether the bare form came from chat.db itself or something created locally through another path. The scenario was two rapid messages from the same sender, the second resolved to a handle the exact lookup missed, and the notification showed Unknown, and the fallback was written against that incident. It only runs when the exact address+service lookup has already missed. If the bare form never occurs it never matches and behavior is identical to today, just one extra indexed query on the miss path. If it does occur from whatever source (server payload variance, SMS relay entries, a locally created row), it links to the canonical handle instead of creating an orphaned duplicate. So worst case it's inert, best case it prevents the Unknown sender case. |
i wonder if this was moreso just a race case between when the contact relationship was loaded in vs. when it wasn't. But interesting anecdote. Not sure if i want to approve all of the changes just yet. Or maybe there's a slightly better way to implement the changes that is resolving it at the root. I'll have to investigate some more. |
|
Pushed a small follow-up to this branch ( During testing we hit a crash coming out of The fix rebuilds the fallback map key by key with a guarded per-value read, so a null can't take down the normalize. Since it's the exact path this PR introduces, it seemed to belong here rather than as a separate change. It showed up a handful of times over a couple days of normal use on desktop and Android before the changes. |
|
For the most part, i think this looks good. The main change that concerns me is in That's the main thing I'd revert or change. I just dont think it makes sense right now. At some point it might be, but right now, I'm not so sure. There are also a few small things I might want to change a little bit:
Also, there are some conflicts now with the development branch |
|
Reworked per your review:
Also folded in a related fix I hit while validating the above: the participant-add de-duped only on |
… email guard, lint Per zlshames' review on BlueBubblesApp#3062: - _hydrateChat no longer re-fetches the full participant list on every incoming group message; only re-fetch when the sender's handle isn't already resolvable locally (skips the per-message server round-trip that doesn't scale) - guard the +/- prefix handle fallback with an @ check so it doesn't build a nonsensical +user@example.com query for email addresses - drop two redundant ! operators in message_summary_info.dart Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
zlshames
left a comment
There was a problem hiding this comment.
Here are some additional things to resolve:
1. deepNormalizeJson can silently corrupt legitimate string content (lib/utils/deep_map_normalize.dart:848-889)
The fallback path (triggered exactly when jsonEncode(raw) throws. For instance, the CastMap-with-null case this utility was written to handle) rebuilds the map key-by-key and calls deepNormalizeJson(value) on every value, including plain-text strings. For a String value, the function unconditionally attempts jsonDecode(raw):
if (raw is String) {
try {
return deepNormalizeJson(jsonDecode(raw));
} catch (_) {
return raw;
}
}This is applied to fields like AttributedBody.string, which is the message text. Any message body that happens to be valid JSON on its own (a purely numeric text like "1234", or literally "true"/"null"/"false") will be silently converted from a String to an int/bool/null. Since AttributedBody.string is typed final String string;, assigning a non-string value via json["string"] ?? "" will throw a runtime TypeError on that message. For example, the exact null-containing-map scenario this PR targets is also the scenario that can now crash message parsing for ordinary numeric-text messages. Worth scoping the JSON-string-reinterpretation to fields that are actually expected to be JSON-encoded strings, rather than applying it to every string value found while walking a map.
2. Reintroduces the "mislinked recipient as sender" bug for fromMe messages (lib/services/backend/actions/chat_actions.dart, new else if branch)
The existing code deliberately guards handle resolution with:
final bool hasSenderHandle = !(inputMessage.isFromMe ?? false) && (inputMessage.handleId ?? 0) != 0;
if (inputMessage.handle == null && hasSenderHandle) { ... }
else if (inputMessage.handle != null) { ... }with a comment explaining hasSenderHandle exists specifically because "in 1:1 chats, [it] mislinked the RECIPIENT's handle as the message's sender." This PR adds a third branch:
} else if (inputMessage.handleId != null && inputMessage.handleId! > 0) {
final handleQuery = handleBox.query(Handle_.originalROWID.equals(inputMessage.handleId!)).build();
...
handleToLink = handleQuery.findFirst();
}This branch is reachable when inputMessage.handle == null and hasSenderHandle is false, which includes the case isFromMe == true with a non-zero handleId. It performs the handle lookup without checking isFromMe, so an outgoing message that happens to carry a non-zero handleId (plausible, since that's precisely the historical failure mode called out in the adjacent comment) can get handleToLink set and later assigned via dbMessage.handleRelation.target = handleToLink, reintroducing the bug the hasSenderHandle guard exists to prevent. This branch should also require hasSenderHandle (or explicitly !inputMessage.isFromMe!).
Other minor things I think makes sense to change:
incoming_message_handler.dartadds a private_chatIsGroup(Chat chat)(style == 43 || handles.length > 1 || participants.length > 1) that duplicatesChat.isGroup(chat.dart:768,style == 43 || handles.length > 1) with a slightly different definition (adds theparticipants.length > 1check). Reusechat.isGroupinstead.deepNormalizeJson's "fast path" does a fulljsonEncode+jsonDecoderound-trip, and this is invoked again at every nesting level by the variousfromMap/fromJsonfactories (each callsasStringDynamicMapRequiredon its own sub-map). For deeply nested structures (e.g.messageSummaryInfo.editedContent.values) this is quadratic-ish rework, though likely immaterial given payload sizes.deep_map_normalize.dartexposesnormalizeMethodChannelArgumentswhich appears unused in this diff. Can you confirm it's wired up somewhere (Android method channel handlers) or drop it if it's dead code introduced ahead of need.
I am a bit concerned about the performance impact of the json encoding round-trip. The object sizes usually aren't too large, but it still could add jank if done alongside any UI rendering.
…ng spinner Bug 1: second rapid message in a group chat showed "Unknown" as sender. Root cause: handle lookup on the Message object fell through to a DB query that raced with ObjectBox relation loading; the handle arrived after the notification was built. Fix: check the transient handle field first (populated by addMessageToChat before the DB put) before issuing the query. Also normalize all nested server payload maps with a shared deep_map_normalize utility so that dynamic fields (attributedBody, messageSummaryInfo, etc.) never arrive as LinkedHashMap<Object,Object> after cross-isolate transfer. Bug 2: opening a conversation from a notification left an infinite "Loading more messages…" spinner. Root cause: getMessagesAsync filtered all messages when chat style was unknown (single-member participant list on a group chat). Fix: gate the style-43/participant-length group check on chatStyle so messages are never silently dropped, and set noMoreMessages on the first empty page response so the spinner clears.
deepNormalizeJson's fallback rebuilt the map via raw.entries, which throws on a CastMap<String, Object>: the call sites pass `.cast<String, Object>()` over a JSON-decoded payload that legitimately holds null fields, and reading a null through the non-nullable Object cast raises "type 'Null' is not a subtype of type 'Object'". The jsonEncode fast path above fails the same way, so a single null field aborted the whole Chat/Attachment parse and dropped the event. Rebuild key-by-key with a guarded per-value lookup so a null can't blow up the normalize.
- deepNormalizeJson: rebuild maps in a single guarded pass and return strings
untouched, so numeric/bool-like message text ("1234", "true") is no longer
reinterpreted into an int/bool and rejected by AttributedBody.string. Also
drops the jsonEncode/jsonDecode round-trip that ran at every nesting level.
- chat_actions: drop the handle-link branch that only ran for fromMe messages
(handle == null and not hasSenderHandle), which could mislink the recipient
as the sender.
- chat_actions: drop the speculative +/- prefix fallback (no evidence the bare,
no-plus address form actually occurs).
- incoming_message_handler: reuse Chat.isGroup instead of the near-duplicate
_chatIsGroup helper.
- deep_map_normalize: remove the unused normalizeMethodChannelArguments.
|
Addressed as follows:
|
|
Looks like we missed one thing: if (dbChat != null) {
inputMessage.chat.target = dbChat;
// Fallback: resolve sender against this chat's linked participants
if (handleToLink == null && inputMessage.handleId != null && inputMessage.handleId! > 0) {
handleToLink = List<Handle>.from(dbChat.handles).firstWhereOrNull((h) => h.originalROWID == inputMessage.handleId);
}
...
}This runs unconditionally with no |
|
Added the same !isFromMe guard to the dbChat participant fallback so it matches the hasSenderHandle branch above; a from-me message with a non-zero handleId now skips it instead of resolving to the recipient. |
Problem
Two notification pipeline bugs present in v2.0.0+85:
Root causes and fixes
Unknown sender: The handle lookup on
Message.getHandle()fell through to a DB query that raced with ObjectBox relation loading. Fixed by checking the in-memory transienthandlefield (populated byaddMessageToChatbefore the DB put) before issuing the query.Also adds
lib/utils/deep_map_normalize.dart— a utility to fixLinkedHashMap<Object,Object>type corruption on nested server payload maps after cross-isolate transfer. This affectsattributedBody,messageSummaryInfo,payloadData, andserverPayloaddeserialization on both FCM and UnifiedPush background paths.Infinite loading spinner:
getMessagesAsyncfiltered all messages whenchatStylewas not yet known for a group chat (single-member participant list). Fixed by threadingchatStylethrough the interface so the group detection gate has the correct value, and settingnoMoreMessageson the first empty-page response so the spinner clears.What changed
lib/utils/deep_map_normalize.dart— new utilitylib/database/global/—attributedBody,messageSummaryInfo,payloadData,serverPayloadnormalizationlib/database/io/—message,handle,attachment,chatlib/app/state/message_state.dartlib/app/layouts/conversation_view/pages/messages_view.dartlib/services/backend/actions/chat_actions.dart,sync_actions.dartlib/services/backend/interfaces/chat_interface.dartlib/services/backend/incoming_message_handler.dart