fix(api): order same-second messages by arrival on SQLite - #5
Merged
Conversation
The message list needs a second sort key, because `createdAt` is not unique and a walk without a total order repeats some rows and never returns others. The key chosen was `id`, a random v4 uuid, which orders a tie group at random: five messages sharing one second came back shuffled, and the dashboard renders whatever the server sends. SQLite stores whole seconds, so any rapid burst, bulk send, media-plus-caption pair or history backfill makes a tie group, and SQLite is the default. It also cost a sort. `id` is in no index, so the plan grew a temp b-tree for the last ORDER BY term where v0.23.3 had none. SQLite already stores the insertion sequence as `rowid`, the implicit trailing column of every index, so `(createdAt DESC, rowid DESC)` is a plain backward scan of `(sessionId, createdAt)`. Measured on the pinned better-sqlite3 with the shipped indexes: arrival order restored, and the temp b-tree gone. PostgreSQL keeps `id`. It has no equivalent, since `ctid` is physical position and moves on every ack update, and a monotonic column would mean rewriting the hottest table with no recoverable insertion order to backfill from. So the walk is equally correct there and a same-second group keeps its uuid order. docs/06 says so rather than promising arrival order everywhere. The keyset cursor uses the same key, so `after` and the page order stay in agreement on both dialects.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughMessage history queries now apply deterministic secondary ordering when ChangesMessage ordering
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to The update supports deterministic ordering for messages with matching timestamps, with no current merge-blocking risk identified. Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Prior commit added tests referencing makeQb() and .addOrderBy outside its describe scope, and left 4 other query-builder mocks without addOrderBy — the service now calls it unconditionally in getMessages, so those spies came back typed as errors under CI's clean install.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Ports 1 commit from upstream (rmyndharis/OpenWA) from a bug-fix batch review of the last 2 weeks of upstream changes.
8a412c74fix(api): order same-second messages by arrival on SQLitecreatedAtis not unique (SQLite stores whole seconds, a bulk send/history backfill ties every row it inserts), and the prior tiebreak was a random uuid — same-second messages came back shuffled on every request. Now breaks the tie withrowidon SQLite (the actual insertion order, no extra sort needed) and keepsidon PostgreSQL, where there's norowidequivalent.Not included from the same batch
Everything else surveyed in this batch depends on upstream features this fork hasn't ported yet:
f7520a28tiebreak paged lists on id — bundles unrelated CI workflow changes plus arelease-gate-parity.spec.tsCI gate this fork doesn't havecd4932caquote mediaPath in the partial index — patches amediaPath/mediaMimetypechat-media-archive feature that doesn't exist on this fork89b7d65c,1b435310,6c08d1d6,84460c0e,b16b8029— the whole "dead page → 503" classification sweep depends on awithPagehelper inwwebjs-host.tsthis fork never gotd997d86b,550c2b1e— depend on the samewithPageinfra and theafterkeyset-cursor feature, respectivelyPorting those needs the underlying feature commits first — separate, larger effort.
Validation
npm run build— cleannpm run lint— cleannpm test— 4054/4059 passed, 5 pre-existing skips, 0 failuresafter-cursor branch this fork lacks, kept the tiebreak logic), message.service.spec.ts (dropped cursor tests, added 2 tests for the tiebreak itself +addOrderByto existing query-builder mocks), docs/06 (dropped theaftercursor mention), CHANGELOG.md (new entry)Summary by CodeRabbit
Bug Fixes
Documentation