Skip to content

Include the originator when assembling a reply thread - #230

Open
Xare123 wants to merge 1 commit into
OpenBubbles:v2-rustpushfrom
Xare123:v2/fix-reply-thread-assembly
Open

Include the originator when assembling a reply thread#230
Xare123 wants to merge 1 commit into
OpenBubbles:v2-rustpushfrom
Xare123:v2/fix-reply-thread-assembly

Conversation

@Xare123

@Xare123 Xare123 commented Jul 25, 2026

Copy link
Copy Markdown

Two related defects in reply thread assembly. Both are present on v2-rustpush today and both are reproducible by hand.

1. The thread omits the message being replied to

ChatMessages.threads() returns the originator only if it is already present in _threads[originatorGuid]. The only place that registers it is addMessages:

if (_threads.keys.contains(m.guid)) {
  // add thread 'originator'
  _threads[m.guid]![m.guid!] = m;
}

That branch requires the map entry to already exist, which is true only when a reply was added first. So the behavior depends on load order:

Order Result
Replies load, then originator Entry exists, originator is registered. Works.
Originator loads, then replies Guard is false when the originator is added, and nothing registers it afterward. Originator missing.

The second ordering is the common one, because the originator is older than its replies. In practice, opening a threaded reply shows the reply with no context — you cannot see what it was a reply to.

The class already has the fix. getThreadOriginator() registers the originator on demand and its comment describes exactly this case:

// It isn't guaranteed that the thread originator will be in the regular
// messages list, in case it is much older than the currently loaded messages.
// Prefer to use this method to find originator.

threads() just never calls it. This change calls it before filtering, so the fix reuses existing behavior rather than adding a new mechanism.

The returnOriginator ? ... : false ternary is also simplified to returnOriginator && ..., which is equivalent.

2. Multi-digit reply part indexes are truncated

int get normalizedThreadPart => threadOriginatorPart == null ? 0 : int.parse(threadOriginatorPart![0]);

threadOriginatorPart is a colon-delimited string such as "12:0:0". Indexing [0] takes a single character, so part 12 resolves to 1 and the reply is matched against the wrong part of the originating message. Any message with more than ten parts can attach a reply to the wrong bubble.

int.parse also throws on a non-numeric leading character, from what reads as a plain int getter with a safe default.

Fixed by splitting on : and taking the first field, with tryParse and a 0 fallback so the getter cannot throw. Applied to both the io and html variants, which carried identical code.

Scope

Deliberately narrow: three files, no behavior changes outside thread assembly. This does not touch the thread popup's rendering, keyboard handling, or the delivery path.

I also have a lazy-rendering change for the thread popup that helps noticeably on chains of a few hundred messages, but it is an enhancement rather than a correctness fix, so I left it out to keep this reviewable. Happy to open it separately.

Testing

Both defects are demonstrated, not just described. Seven tests, run twice on v2-rustpush — once against the branch unmodified, once with this change applied. Nothing else differed between the runs.

Unmodified v2-rustpush4 passed, 3 failed:

includes the originator when it loads before its replies
  Expected: contains all of ['thread-1', 'reply-1']
  Actual:   MappedListIterable<Message, String?>:['reply-1']

parses a multi-digit part instead of truncating
  Expected: <12>
  Actual:   <1>

does not throw on a non-numeric part
  FormatException: Invalid radix-10 number (at character 1)

With this change7 passed.

The four that already passed unmodified are the control: originator-loads-after-replies (the ordering that happens to work today), returnOriginator: false, a single-digit part, and a null part. They still pass, so existing behavior is unchanged.

flutter analyze on Dart 3.12.2: 36 issues before this change, the same 36 after. No new errors or warnings.

v2-rustpush has no test/ directory and flutter_test is commented out in pubspec.yaml, so I kept the harness out of this PR rather than add test infrastructure inside a bug fix. The seven tests are written and ready — happy to add them in a follow-up commit if you want the harness enabled.

A note on CI

CI on this branch will still fail, for reasons unrelated to this change: v2-rustpush cannot resolve dependencies at all right now. Its pinned Flutter 3.24.0 predates three dependency constraints, and the pinned telephony_plus requires permission_handler ^11.3.1 while this branch requires ^12.0.1. The companion PR fixes the version blockers and diagnoses the rest. The test runs above used that workflow fix plus a temporary permission_handler override.

A full APK build with these three files also succeeds under the same conditions.

Behavior was not re-tested on-device against v2, only unit-tested and compiled. The same two defects were observed and fixed on a rustpush-based build running on a Pixel 10 Pro.

threads() returned the originator only when addMessages had already
registered it, which happens only if a reply reached the thread map
first. A thread whose originator loaded before its replies rendered
without the message being replied to. getThreadOriginator already
registers it on demand, so call it before filtering.

Also parse threadOriginatorPart properly. int.parse(part[0]) read a
single character, so '12:0:0' resolved to 1 and matched the reply
against the wrong part. A non-numeric leading character also threw
from a plain int getter. Split on ':' and fall back to 0.
@Xare123
Xare123 marked this pull request as ready for review July 26, 2026 00:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant