chore: add compound index { tmid: 1, ts: -1 } to messages collection - #40407
IgorOhrimenko wants to merge 2 commits into
Conversation
Speeds up paginated thread reply loads (chat.getThreadMessages) on large workspaces. Without this index the query planner can pick an IXSCAN over ts_1 and filter by tmid in memory, which becomes a near full collection scan once the messages collection grows large.
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
|
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📜 Recent review details⏰ Context from checks skipped due to timeout. (1)
WalkthroughAdds a sparse compound index ChangesThread Reply Pagination Index
Estimated code review effort: 1 (Trivial) | ~3 minutes Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🦋 Changeset detectedLatest commit: 79ac06a The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/models/src/models/Messages.ts (1)
72-72: ⚡ Quick winRemove inline implementation comment from index definition
Please drop the inline
// used for ...comment and keep the index entry self-contained to match repo style for TS implementation files.As per coding guidelines: "
**/*.{ts,tsx,js}... Avoid code comments in the implementation".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/models/src/models/Messages.ts` at line 72, Remove the trailing inline comment on the index definition so the entry is self-contained; locate the index object that contains { key: { tmid: 1, ts: -1 }, sparse: true } in Messages.ts (the index definition used for paginated thread reply loads / chat.getThreadMessages) and delete the inline "// used for paginated thread reply loads (chat.getThreadMessages)" comment, leaving only the index object per repo TS style.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/models/src/models/Messages.ts`:
- Line 72: Remove the trailing inline comment on the index definition so the
entry is self-contained; locate the index object that contains { key: { tmid: 1,
ts: -1 }, sparse: true } in Messages.ts (the index definition used for paginated
thread reply loads / chat.getThreadMessages) and delete the inline "// used for
paginated thread reply loads (chat.getThreadMessages)" comment, leaving only the
index object per repo TS style.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 31df4de8-7679-47b4-9f98-b3788c5fbcd2
📒 Files selected for processing (2)
.changeset/compound-index-thread-replies.mdpackages/models/src/models/Messages.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
packages/models/src/models/Messages.ts
🧠 Learnings (3)
📚 Learning: 2026-03-16T21:50:37.589Z
Learnt from: amitb0ra
Repo: RocketChat/Rocket.Chat PR: 39676
File: .changeset/migrate-users-register-openapi.md:3-3
Timestamp: 2026-03-16T21:50:37.589Z
Learning: For changes related to OpenAPI migrations in Rocket.Chat/OpenAPI, when removing endpoint types and validators from rocket.chat/rest-typings (e.g., UserRegisterParamsPOST, /v1/users.register) document this as a minor changeset (not breaking) per RocketChat/Rocket.Chat-Open-API#150 Rule 7. Note that the endpoint type is re-exposed via a module augmentation .d.ts in the consuming package (e.g., packages/web-ui-registration/src/users-register.d.ts). In reviews, ensure the changeset clearly states: this is a non-breaking change, the major version should not be bumped, and the changeset reflects a minor version bump. Do not treat this as a breaking change during OpenAPI migrations.
Applied to files:
.changeset/compound-index-thread-replies.md
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In the Rocket.Chat repository, do not reference Biome lint rules in code review feedback. Biome is not used even if biome.json exists; only reference Biome rules if there is explicit, project-wide usage documented. For TypeScript files, review lint implications without Biome guidance unless the project enables Biome rules.
Applied to files:
packages/models/src/models/Messages.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In this repository (RocketChat/Rocket.Chat), Biome lint rules are not used even if a biome.json exists. When reviewing TypeScript files (e.g., packages/ui-voip/src/providers/useMediaSession.ts), ensure lint suggestions do not reference Biome-specific rules. Rely on general ESLint/TypeScript lint rules and project conventions instead.
Applied to files:
packages/models/src/models/Messages.ts
🪛 LanguageTool
.changeset/compound-index-thread-replies.md
[style] ~5-~5: As an alternative to the over-used intensifier ‘very’, consider replacing this phrase.
Context: ...ring by tmid in memory, which becomes very expensive on collections with millions of message...
(EN_WEAK_ADJECTIVE)
🔇 Additional comments (1)
.changeset/compound-index-thread-replies.md (1)
1-5: Changeset entry looks goodPatch bump and rationale are clear, and the note accurately describes the index change and its performance impact.
|
@coderabbitai Thanks — addressed in 79ac06a: the inline |
|
Tip For best results, initiate chat on the files or code changes.
If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
|
thanks for your contribution @IgorOhrimenko :) do you mind testing it out how it behaves if you replace the current |
5 participants
Proposed changes (including videos or screenshots)
Adds a compound index
{ tmid: 1, ts: -1 }(sparse) on therocketchat_messagecollection.Why. The current
tmid_1index (sparse) covers thread membership lookups, but it does not satisfy thesort: { ts: -1 }clause used by paginated thread reply loads (chat.getThreadMessages, REST/api/v1/chat.getThreadMessages, and the correspondingfind({ tmid }).sort({ ts: -1 }).limit(N)calls). On large workspaces MongoDB's planner can pickIXSCAN { ts: 1 }instead — walking the global time index from newest backwards and filtering bytmidin memory. This works fine when the messages collection is small, but as it grows into the millions of documents the same query repeatedly degrades into a near-full collection scan. The planner is also seen to flap (replanned: true,replanReason: "cached plan was less efficient") between the two indexes, which causes intermittent multi-second response times for any client opening a thread.The new compound index covers both the equality filter on
tmidand the descending sort onts, so the query is served entirely from the index and a single bounded scan returns exactly the requested page.The index is
sparse, so only thread replies (messages withtmid) are indexed — the storage cost on the (much larger) set of regular messages withouttmidis zero.Benchmark — production workspace,
rocketchat_message≈ 5M documents (~5.2 GB)Heaviest thread on this workspace contains 957 replies. Same query (
find({ _hidden: { $ne: true }, tmid: <id> }).sort({ ts: -1 })) with three plans forced viahint(...), three differentlimitvalues to show how each plan scales:hint)ts_1(planner's choice on this WS)tmid_1(current upstream index)tmid_1_ts_-1(this PR)ts_1tmid_1tmid_1_ts_-1(this PR)ts_1tmid_1tmid_1_ts_-1(this PR)docsExaminedis omitted because for this query it is always equal tokeysExamined— the_hidden: { $ne: true }predicate is not covered by any of the candidate indexes, so every matched key requires a document fetch.What the table shows:
ts_1— scales with the messages collection: limit barely matters, every page costs the same overscan, and the no-limit query scans the entire 5M-row collection (9.4 s).tmid_1— scales with the thread size: it can find the right rows but cannot honoursort: { ts: -1 }from the index, so all 957 thread documents are fetched and sorted in memory regardless of pagination.tmid_1_ts_-1— scales with the page size:keysExamined == limit. This is the only plan where opening a thread costs O(page) rather than O(thread) or O(collection).For threads where the planner picks
ts_1and gets unlucky (replanning), real production slow-query log entries on the same workspace reportkeysExamined: 5,049,435 / docsExamined: 5,049,435for a single thread page load — i.e. an effective full collection scan.Issue(s)
Open reports about slow queries on
rocketchat_message:Steps to test or reproduce
On any workspace large enough that
rocketchat_messageis in the millions, pick a thread with at least a few hundred replies and run:totalKeysExaminedshould equal the requested page size. End-to-end test: open the same thread in the client and observe time-to-first-reply render.Further comments
filesattribute on the messages collection #38087 forfiles._id).modelIndexes()is reconciled at startup;createIndexis idempotent on existing keys.tmid_1index, so the additional storage cost is bounded by the number of threaded replies (a small fraction of total messages on typical workspaces).Summary by CodeRabbit