fix: Exclude _hidden messages from API responses
#36661
Conversation
|
Looks like this PR is ready to merge! 🎉 |
🦋 Changeset detectedLatest commit: 76b3ec6 The changes in this PR will be included in the next version bump. This PR includes changesets to release 39 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 |
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #36661 +/- ##
===========================================
+ Coverage 66.26% 66.27% +0.01%
===========================================
Files 3339 3340 +1
Lines 113726 113723 -3
Branches 21171 21193 +22
===========================================
+ Hits 75357 75372 +15
+ Misses 35677 35670 -7
+ Partials 2692 2681 -11
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
yash-rajpal
added
the
stat: QA assured
dionisio-bot
bot
added
the
stat: ready to merge
yash-rajpal
dismissed
sampaiodiego’s stale review
Addressed review and pushed tests.
dionisio-bot
bot
removed
the
stat: ready to merge
WalkthroughPatch adds filtering of hidden messages (_hidden: { $ne: true }) to channels.messages, groups.messages, and dm/im.messages endpoints. Test helpers gain updateMessage and an unpin option for pinMessage. E2E tests for channels, groups, and direct messages validate KeepHistory behavior, unpinning, pinned queries, and updates. A changeset records the patch. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Client
participant API as REST Endpoint (channels/groups/dm).messages
participant S as Messages Service
participant DB as Database
C->>API: GET /.../messages?{query}
API->>S: buildQuery(rid, filters, pagination)
Note right of S: Add filter: _hidden: { $ne: true }
S->>DB: find({ rid, ..., _hidden: { $ne: true } }, { sort, limit, skip })
DB-->>S: messages[], count
S-->>API: normalized payload
API-->>C: 200 OK { messages, count, total }
sequenceDiagram
autonumber
participant T as Test
participant H as chat.helper
participant API as REST API
participant DB as Database
T->>API: POST /settings/Message_KeepHistory enable
T->>H: pinMessage({ messageId })
H->>API: POST chat.pinMessage { messageId }
API-->>T: { message: {..., pinned: true} }
T->>H: pinMessage({ messageId, unpin: true })
H->>API: POST chat.unPinMessage { messageId }
T->>H: updateMessage({ msgId, updatedMessage, roomId })
H->>API: POST chat.update { msgId, text, roomId }
T->>API: GET /.../messages
Note right of API: Hidden messages excluded by _hidden: { $ne: true }
API-->>T: messages without hidden/pinned
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Poem
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. ✨ Finishing Touches
🧪 Generate unit tests
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
.changeset/fast-phones-poke.md (1)
5-5: Fix wording and briefly explain what “_hidden” means to users.The change is about excluding hidden messages (documents with _hidden: true), not “deleted” ones. Also add a short end-user note per earlier feedback.
Apply:
-Fixes `channels.messages`, `groups.messages`, `dm.messages` and `im.messages` APIs to filter out deleted messages. +Fixes `channels.messages`, `groups.messages`, `dm.messages`, and `im.messages` to exclude hidden messages (documents with `_hidden: true`), e.g., historical duplicates created by Message_KeepHistory or pin/unpin actions, from API responses.
🧹 Nitpick comments (5)
apps/meteor/app/api/server/v1/channels.ts (1)
314-315: Good fix; consider mirroring in channels.anonymousread.Anonymous read currently doesn’t filter
_hidden; users could still see hidden duplicates there. Suggest adding the same clause.Apply:
- const ourQuery = Object.assign({}, query, { rid: findResult._id }); + const ourQuery = Object.assign({}, query, { rid: findResult._id, _hidden: { $ne: true } });apps/meteor/app/api/server/v1/im.ts (1)
506-511: Optional: align admin “others” view with hidden exclusion (or document exception).If admins shouldn’t see hidden duplicates either, add the same
_hidden: { $ne: true }filter todm.messages.others. If admins are intended to see everything, note that in docs.Apply:
- const ourQuery = Object.assign({}, query, { rid: room._id }); + const ourQuery = Object.assign({}, query, { rid: room._id, _hidden: { $ne: true } });apps/meteor/tests/end-to-end/api/channels.ts (1)
4130-4201: Make assertions tolerant to_hidden: falseto prevent future flakiness.Writers could set
_hidden: false; asserting property absence would fail unnecessarily. Check “not true” instead.Apply:
- res.body.messages.forEach((msg: IMessage) => { - expect(msg).to.not.have.property('pinned', true); - expect(msg).to.not.have.property('_hidden'); - }); + res.body.messages.forEach((msg: IMessage) => { + expect(msg).to.not.have.property('pinned', true); + expect(msg._hidden).to.not.equal(true); + });And similarly below:
- res.body.messages.forEach((msg: IMessage) => { - expect(msg).to.not.have.property('_hidden'); - }); + res.body.messages.forEach((msg: IMessage) => { + expect(msg._hidden).to.not.equal(true); + });apps/meteor/tests/end-to-end/api/groups.ts (1)
677-748: Remove async from describe and restore the original KeepHistory value.
- Using an async describe callback is unnecessary and may confuse Mocha. Prefer a non-async describe and keep async in hooks/tests.
- Consider restoring Message_KeepHistory to its previous value rather than hardcoding false, to avoid cross-suite leakage if defaults differ.
Apply this minimal diff to drop the async on describe:
-describe('_hidden messages behavior when Message_KeepHistory is enabled', async () => { +describe('_hidden messages behavior when Message_KeepHistory is enabled', () => {Optionally, cache and restore the prior value (pseudo-code; wire to whatever settings getter you have available):
let prevKeepHistory: boolean; before(async () => { // prevKeepHistory = await getSetting('Message_KeepHistory'); await updateSetting('Message_KeepHistory', true); await pinMessage({ messageId: pinnedMessageId, unpin: true }); }); after(async () => { // await updateSetting('Message_KeepHistory', prevKeepHistory); await updateSetting('Message_KeepHistory', false); });If you want, I can add a tiny test helper to fetch a setting so we can truly restore the prior value.
apps/meteor/tests/end-to-end/api/direct-message.ts (1)
708-782: Same nits as in groups: avoid async describe and restore original setting.
- Drop async on the describe callback.
- Prefer restoring Message_KeepHistory to its previous value to prevent suite coupling, especially if files run in parallel.
Diff to remove async:
-describe('_hidden messages behavior when Message_KeepHistory is enabled', async () => { +describe('_hidden messages behavior when Message_KeepHistory is enabled', () => {Optional pattern to cache/restore:
let prevKeepHistory: boolean; before(async () => { // prevKeepHistory = await getSetting('Message_KeepHistory'); await updateSetting('Message_KeepHistory', true); await pinMessage({ messageId: pinnedMessageId, unpin: true, requestCredentials: testUserCredentials }); }); after(async () => { // await updateSetting('Message_KeepHistory', prevKeepHistory); await updateSetting('Message_KeepHistory', false); });I can factor a small settings-getter helper (and update imports) if you want this hardened.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
.changeset/fast-phones-poke.md(1 hunks)apps/meteor/app/api/server/v1/channels.ts(1 hunks)apps/meteor/app/api/server/v1/groups.ts(1 hunks)apps/meteor/app/api/server/v1/im.ts(1 hunks)apps/meteor/tests/data/chat.helper.ts(2 hunks)apps/meteor/tests/end-to-end/api/channels.ts(4 hunks)apps/meteor/tests/end-to-end/api/direct-message.ts(4 hunks)apps/meteor/tests/end-to-end/api/groups.ts(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
apps/meteor/app/api/server/v1/im.ts (1)
apps/meteor/app/lib/server/lib/notifyListener.ts (1)
getMessageToBroadcast(425-464)
apps/meteor/tests/data/chat.helper.ts (5)
packages/core-typings/src/IRoom.ts (1)
IRoom(21-95)apps/meteor/tests/data/api-data.ts (1)
credentials(39-42)apps/meteor/server/services/federation/infrastructure/matrix/Bridge.ts (1)
updateMessage(571-601)apps/meteor/app/threads/server/methods/unfollowMessage.ts (1)
unfollowMessage(22-57)apps/meteor/app/threads/server/methods/followMessage.ts (1)
followMessage(22-57)
apps/meteor/tests/end-to-end/api/channels.ts (1)
apps/meteor/tests/data/api-data.ts (1)
credentials(39-42)
apps/meteor/tests/end-to-end/api/direct-message.ts (1)
apps/meteor/tests/data/chat.helper.ts (2)
pinMessage(56-69)updateMessage(108-123)
apps/meteor/tests/end-to-end/api/groups.ts (1)
apps/meteor/tests/data/api-data.ts (1)
credentials(39-42)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (10)
apps/meteor/app/api/server/v1/groups.ts (1)
795-796: LGTM: consistent hidden-message exclusion.Adding
_hidden: { $ne: true }aligns this endpoint with how we suppress hidden messages elsewhere (e.g., notifyListener).apps/meteor/app/api/server/v1/im.ts (1)
458-459: LGTM: dm/im messages now exclude hidden.Matches channels/groups behavior.
apps/meteor/tests/data/chat.helper.ts (2)
56-67: LGTM: simple pin/unpin toggle improves reuse.
108-123: LGTM: updateMessage helper mirrors API contract cleanly.apps/meteor/tests/end-to-end/api/channels.ts (2)
8-8: LGTM: importing updateMessage for KeepHistory checks.
3949-3987: LGTM: capturing pinnedMessageId for later assertions.apps/meteor/tests/end-to-end/api/groups.ts (2)
7-7: Importing updateMessage is correct and aligns with helper API.
The added import matches the helper signature and usage in this file. Looks good.
503-504: Pinned message tracking is sound.
Capturing and reusing the pinned message _id for unpin/edit assertions is a clean approach and keeps the tests deterministic.Also applies to: 538-539
apps/meteor/tests/end-to-end/api/direct-message.ts (2)
8-8: Importing updateMessage is correct and consistent with its usage.
Matches the helper’s API and enables the edit flow assertions below.
485-485: Pinned message id capture is correct.
Declaring the variable up-front and assigning once after pinning keeps the later unpin/update tests straightforward.Also applies to: 542-542
yash-rajpal
added
the
stat: ready to merge
Proposed changes (including videos or screenshots)
Issue(s)
Message_KeepHistorygroups.messagesAPI_hiddenmessageSteps to test or reproduce
Further comments
SUP-789
Summary by CodeRabbit
Bug Fixes
Chores