fix: order mesh public/channel timeline by source packet timestamp (#525) - #738
fix: order mesh public/channel timeline by source packet timestamp (#525)#738gunjanjaswal wants to merge 3 commits into
Conversation
Incoming public and channel messages were appended in receive order, so when a peer store-forwards or gossip-syncs an older backlog on reconnect, hour-old messages landed at the bottom interleaved with current ones. BitchatMessage.timestamp already carries the source packet time, so insert each message at its timestamp position instead of appending. Ordering is a stable binary-search insertion (equal timestamps keep insertion order), applied in the AppStateStore public/channel add paths (the timeline's source of truth) and the matching MessageManager add paths. Fixes permissionlesstech#525.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9aedd06dfb
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| var hi = messages.size | ||
| while (lo < hi) { | ||
| val mid = (lo + hi) ushr 1 | ||
| if (messages[mid].timestamp.time <= ts) lo = mid + 1 else hi = mid |
There was a problem hiding this comment.
Normalize remote timestamps before ordering timelines
When a connected peer's clock is slow—MessageHandler.handleAnnounce explicitly accepts up to ten minutes of clock skew—a live message is inserted minutes above the end of a busy public or channel timeline. MessagesList reacts to the size change by scrolling to index 0 (the timestamp-latest item), so the newly received message can remain off-screen despite the haptic/notification; a fast peer can similarly pin future-dated messages at the bottom. Clamp or normalize remote timestamps, or apply source-timestamp insertion only to identified replay/backlog messages.
Useful? React with 👍 / 👎.
| @@ -0,0 +1,42 @@ | |||
| package com.bitchat.android.util | |||
|
Good flag on clock skew. It is a real consequence of ordering by the source packet timestamp: a peer whose clock runs slow (we accept up to ~10 min skew on announces) can land a live message a few minutes back in the list instead of at the bottom. I left it as-is because ordering by packet timestamp is what #525 asks for, and it matches the |
Android UI verificationCode-level review identifies a visible message-timeline ordering behavior change.
Expected comparison: the same synthetic messages displayed in prior insertion order versus source-packet timestamp order. Limitation: the required Java runtime, Android SDK, emulator, and |
|
Thanks for running this. Since there's no emulator in the loop here, a quick note on what to look for by hand, because this one won't show up as a pixel diff. Nothing about the styling or layout changes. The only thing that moves is the order messages land in. To see it: let a peer drop off a channel for a while as messages keep flowing, then bring it back so it store-forwards its backlog. Before this change that hour-old backlog appended to the bottom, interleaved with current messages. After it, each message slots in at its source-packet timestamp, so the backlog drops into its correct chronological place and the tail stays "now". Messages that share a timestamp keep their arrival order (the insertion is a stable binary search), so nothing reshuffles under normal live use. So there isn't really a before/after screenshot to capture, it's purely timeline ordering, which the insertion logic handles deterministically. |
Chessing234
left a comment
There was a problem hiding this comment.
this already matches ios, which is worth putting in the body — it makes the change parity rather than a proposed behaviour change.
ConversationStore.insert(_:) does the same thing: upper-bound binary search, <= comparison so equal timestamps keep arrival order. your insertByTimestamp is the same algorithm line for line. i'd cite it; "ios has ordered timelines this way all along and android appends in receive order" is a much easier thing to merge than "here is a new ordering policy".
two differences worth acting on.
ios applies it to direct conversations too. ConversationStore.append(_:to:) routes every ConversationID — including .direct — through that same insert. this PR deliberately leaves private chats on append order, so the bug you're fixing stays for DMs on android, and the two platforms would still disagree there. a store-forwarded DM backlog is arguably the more likely case, since that's what the courier path exists for.
ios keeps a fast path. it only binary-searches when the incoming message is older than the tail:
if let last = messages.last, message.timestamp < last.timestamp { ...binary search... }
else { messages.append(message) }in-order arrival is the overwhelmingly common case and it stays O(1) there. cheap to add and it keeps the hot path identical to what it is today, which also shrinks the surface of this change.
the dedup-unchanged and stable-equal-timestamp tests are the right things to have pinned.
Two refinements from review, matching how iOS handles this in ConversationStore: - Private chats now insert by source packet timestamp as well. iOS routes .direct through the same insert path; leaving DMs on append order kept the permissionlesstech#525 bug for private chats, and a store-forwarded DM backlog (the courier path's whole purpose) is the likelier case. Wired through both MessageManager.addPrivateMessage/…NoUnread and AppStateStore.addPrivateMessage. - insertByTimestamp appends in O(1) when the message is at or after the tail, and only binary-searches when it predates the tail. In-order arrival is the common case, so the hot path stays what it was; mirrors iOS's fast path. Tests: a store-forwarded DM backlog orders by timestamp, and a mixed sequence exercises both the fast-path append and the binary-search branch.
|
All three landed — thanks, these were good calls.
Added tests for the DM backlog ordering and a mixed sequence that hits both the append and binary-search branches. I don't have an Android SDK on this machine so I couldn't run the suite locally, but the JUnit tests will run in CI. |
…ges-by-timestamp # Conflicts: # app/src/main/java/com/bitchat/android/services/AppStateStore.kt # app/src/main/java/com/bitchat/android/ui/MessageManager.kt
|
Following up — rebasing this on Since your review, Rather than fight that, I've pulled the DM ordering back out. The PR now covers public and channel only (where timestamp ordering is uncontested and matches iOS), and leaves DMs to the arrival-order path. If the maintainers would rather DMs were timestamp-ordered after all, that's really a conversation about The fast path you suggested stayed in — that one's a clean win regardless. Thanks again for the review; it made the public/channel side better and flagged the DM question that turned out to already have an answer upstream. |
Incoming public and channel messages were added to the timeline in receive order. When a peer reconnects and store-forwards or gossip-syncs an older backlog, those hour-old messages ended up at the bottom of the list, interleaved with current ones.
This is parity with iOS, which orders these timelines the same way:
ConversationStore.insert(_:)does an upper-bound binary search with a<=comparison so equal timestamps keep arrival order.insertByTimestampis that algorithm. Android was the one appending in receive order.Each
BitchatMessagealready carries the source packet timestamp, so the fix inserts each message at its timestamp position instead of appending. It's applied in theAppStateStoreadd paths (which the UI timeline reads from) and the matchingMessageManagerpaths, for the public and channel timelines.insertByTimestampkeeps a fast path, also matching iOS: a message at or after the tail is appended in O(1), and only an out-of-order message pays for the binary search. In-order arrival is the common case, so the hot path is unchanged.On private messages
An earlier version of this PR also ordered DMs by timestamp. While it sat in review,
maingainedPrivateMessageArrivalOrder(withContactDirectory.canonicalizePrivateChats), which deliberately orders DMs by arrival sequence rather than sender timestamp — its own comment explains why: "Peer-provided timestamps cannot safely order a conversation because clocks can differ."That's a deliberate decision, and it's in direct tension with timestamp-ordering DMs, so I've dropped that part: this PR now covers public and channel only, and leaves DM ordering to that arrival-order path (
canonicalizePrivateChatswould re-sort a timestamp insert straight back anyway). Happy to revisit if the maintainers decide DMs should be timestamp-ordered after all.Tests
MessageOrderingTestcovers out-of-order backlog insertion (public and channel), dedup, stable equal-timestamp ordering, and a mixed sequence that exercises both the fast-path append and the binary-search branch.Fixes #525. Same underlying problem as #425, #420, and #302.