fix: sort pinned messages#36768
Conversation
|
|
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 |
|
|
| "@types/node": "~22.16.1", | ||
| "ts-node": "^10.9.2", | ||
| "turbo": "~2.5.5", | ||
| "turbo": "^2.5.6", |
There was a problem hiding this comment.
I dont think this is related with the fix
This reverts commit ae03478.
…nto fix/sort_pinned_messages
WalkthroughAdds sort parsing to chat.getPinnedMessages by extracting a sort object from the JSON query and forwarding it to Messages.findPaginatedPinnedByRoom. Introduces an end-to-end test verifying descending timestamp sorting when a sort parameter is supplied. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Client
participant API as chat.getPinnedMessages
participant PQ as parseJsonQuery
participant DB as Messages.findPaginatedPinnedByRoom
C->>API: GET pinned messages (roomId, query with sort)
API->>PQ: parse offset, count, sort
PQ-->>API: { offset, count, sort }
API->>DB: findPaginatedPinnedByRoom(roomId, { offset, count, sort })
DB-->>API: { messages, total }
API-->>C: 200 OK { success: true, messages, count, offset, total }
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related issues
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
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. 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
🧹 Nitpick comments (2)
apps/meteor/tests/end-to-end/api/chat.ts (1)
2961-2981: Test can pass vacuously with a single pinned message; ensure 2+ pins and fix title typoCreate at least two pinned messages before asserting order, and drop the stray brace in the test title.
-it('should return a list of pinned messages sorted by timestamp descending when sort is provided as {"ts":-1}}', (done) => { - void request - .get(api('chat.getPinnedMessages')) - .set(credentials) - .query({ - roomId, - sort: JSON.stringify({ ts: -1 }), - }) - .expect('Content-Type', 'application/json') - .expect(200) - .expect((res) => { - expect(res.body).to.have.property('success', true); - expect(res.body).to.have.property('messages').and.to.be.an('array'); - - const { messages } = res.body; - for (let i = 0; i < messages.length - 1; i++) { - expect(new Date(messages[i].ts).getTime()).to.be.greaterThan(new Date(messages[i + 1].ts).getTime()); - } - }) - .end(done); -}); +it('should return a list of pinned messages sorted by timestamp descending when sort is provided as {"ts":-1}', async () => { + // Ensure at least 2 pinned messages with different timestamps + const msgA = (await sendSimpleMessage({ roomId })).body.message._id; + await pinMessage({ msgId: msgA }); + await new Promise((r) => setTimeout(r, 10)); + const msgB = (await sendSimpleMessage({ roomId })).body.message._id; + await pinMessage({ msgId: msgB }); + + const res = await request + .get(api('chat.getPinnedMessages')) + .set(credentials) + .query({ roomId, sort: JSON.stringify({ ts: -1 }) }) + .expect('Content-Type', 'application/json') + .expect(200); + + expect(res.body).to.have.property('success', true); + expect(res.body).to.have.property('messages').and.to.be.an('array'); + const { messages } = res.body; + expect(messages.length).to.be.greaterThan(1); + for (let i = 0; i < messages.length - 1; i++) { + expect(new Date(messages[i].ts).getTime()).to.be.greaterThan(new Date(messages[i + 1].ts).getTime()); + } +});apps/meteor/app/api/server/v1/chat.ts (1)
579-583: Only include sort when provided — preserve defaults and match file styleBaseRaw.findPaginated forwards options to col.find (packages/models/src/models/BaseRaw.ts); avoid passing an explicit sort: undefined — use the conditional spread pattern.
- const { cursor, totalCount } = Messages.findPaginatedPinnedByRoom(roomId, { - skip: offset, - limit: count, - sort: sort, - }); + const { cursor, totalCount } = Messages.findPaginatedPinnedByRoom(roomId, { + skip: offset, + limit: count, + ...(sort && { sort }), + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/meteor/app/api/server/v1/chat.ts(2 hunks)apps/meteor/tests/end-to-end/api/chat.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/meteor/tests/end-to-end/api/chat.ts (1)
apps/meteor/tests/data/api-data.ts (3)
request(10-10)api(46-48)credentials(39-42)
🔇 Additional comments (1)
apps/meteor/app/api/server/v1/chat.ts (1)
573-573: Sort parsing added — LGTMParsing sort via parseJsonQuery aligns this route with others. No concerns.
3 participants
Proposed changes (including videos or screenshots)
The below changes are made:
This change allows the /api/v1/chat.getPinnedMessages query to sort pinned messages according to the sort parameter value.
Issue(s)
This PR resolves the following issue:
Reference to the issue - [API Bug] chat.getPinnedMessages does not support sort parameter despite documentation #36087
Steps to test or reproduce
Further comments
Summary by CodeRabbit
New Features
Tests