Skip to content

fix: add offset and count pagination params support for chat.getMessageReadReceipts - #41720

Open
tejaswiverma121-byte wants to merge 1 commit into
RocketChat:developfrom
tejaswiverma121-byte:fix/chat-getMessageReadReceipts-pagination
Open

fix: add offset and count pagination params support for chat.getMessageReadReceipts#41720
tejaswiverma121-byte wants to merge 1 commit into
RocketChat:developfrom
tejaswiverma121-byte:fix/chat-getMessageReadReceipts-pagination

Conversation

@tejaswiverma121-byte

@tejaswiverma121-byte tejaswiverma121-byte commented Aug 8, 2026

Copy link
Copy Markdown

Proposed changes (including videos or screenshots)

Issue(s)

Steps to test or reproduce

Further comments

Proposed changes (including videos or screenshots)

  • Added optional offset and count pagination query parameters to ChatGetMessageReadReceiptsSchema and updated ChatGetMessageReadReceipts to extend PaginatedRequest in @rocket.chat/rest-typings.
  • Switched schema compiler for isChatGetMessageReadReceiptsProps from ajv.compile to ajvQuery.compile to properly coerce query string parameters (e.g. "0" and "10") to numbers.
  • Updated GetMessageReadReceiptsProps in apps/meteor/ee/server/api/chat.ts to extend PaginatedRequest.
  • Added an end-to-end test in apps/meteor/tests/end-to-end/api/chat.ts ensuring requests with offset and count query parameters return 200 OK.
  • Added changeset file .changeset/chat-getmessagereadreceipts-pagination-params.md.

Issue(s)

Closes #41719

Steps to test or reproduce

  1. Make an authenticated GET request to /api/v1/chat.getMessageReadReceipts passing messageId, offset, and count query parameters:
    GET /api/v1/chat.getMessageReadReceipts?messageId=<message_id>&offset=0&count=10
    
    

Review in cubic

Summary by CodeRabbit

  • Bug Fixes

    • Fixed validation for chat.getMessageReadReceipts requests using pagination parameters.
    • The endpoint now accepts optional count and offset values while continuing to require a message ID.
    • Read receipts can now be retrieved using pagination.
  • Tests

    • Added coverage confirming paginated receipt requests return successfully with a receipts array.

@tejaswiverma121-byte
tejaswiverma121-byte requested review from a team as code owners August 8, 2026 08:57
@dionisio-bot

dionisio-bot Bot commented Aug 8, 2026

Copy link
Copy Markdown
Contributor

Looks like this PR is not ready to merge, because of the following issues:

  • This PR is missing the 'stat: QA assured' label
  • This PR is missing the required milestone or project

Please fix the issues and try again

If you have any trouble, please check the PR guidelines

@changeset-bot

changeset-bot Bot commented Aug 8, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 8f00823

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages
Name Type
@rocket.chat/rest-typings Patch
@rocket.chat/meteor Patch
@rocket.chat/core-typings Patch

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

coderabbitai Bot commented Aug 8, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 847e04a2-96ce-4bfe-91c0-67818b97c72d

📥 Commits

Reviewing files that changed from the base of the PR and between ef223a9 and 8f00823.

📒 Files selected for processing (5)
  • apps/meteor/ee/server/api/chat.ts
  • apps/meteor/ee/server/lib/message-read-receipt/ReadReceipt.ts
  • apps/meteor/ee/server/meteor-methods/getReadReceipts.ts
  • apps/meteor/tests/end-to-end/api/chat.ts
  • packages/rest-typings/src/v1/chat.ts
🚧 Files skipped from review as they are similar to previous changes (2)
  • apps/meteor/tests/end-to-end/api/chat.ts
  • packages/rest-typings/src/v1/chat.ts
📜 Recent review details
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{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:

  • apps/meteor/ee/server/meteor-methods/getReadReceipts.ts
  • apps/meteor/ee/server/lib/message-read-receipt/ReadReceipt.ts
  • apps/meteor/ee/server/api/chat.ts
apps/meteor/**

📄 CodeRabbit inference engine (CLAUDE.md)

The main Rocket.Chat Meteor application resides in apps/meteor/; place its application code there rather than in other monorepo areas.

Files:

  • apps/meteor/ee/server/meteor-methods/getReadReceipts.ts
  • apps/meteor/ee/server/lib/message-read-receipt/ReadReceipt.ts
  • apps/meteor/ee/server/api/chat.ts
🧠 Learnings (5)
📚 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:

  • apps/meteor/ee/server/meteor-methods/getReadReceipts.ts
  • apps/meteor/ee/server/lib/message-read-receipt/ReadReceipt.ts
  • apps/meteor/ee/server/api/chat.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:

  • apps/meteor/ee/server/meteor-methods/getReadReceipts.ts
  • apps/meteor/ee/server/lib/message-read-receipt/ReadReceipt.ts
  • apps/meteor/ee/server/api/chat.ts
📚 Learning: 2026-05-06T12:21:44.083Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 40256
File: apps/meteor/client/components/CreateDiscussion/CreateDiscussion.tsx:121-149
Timestamp: 2026-05-06T12:21:44.083Z
Learning: Field wrappers in rocket.chat/fuselage-forms (Field, FieldLabel, FieldRow, FieldError, FieldHint) auto-create htmlFor/id associations, aria-describedby, and role="alert" for errors. Do not manually set htmlFor, id, aria-describedby, or role attributes when using these wrappers. This automatic wiring does not apply to plain rocket.chat/fuselage components, which require explicit ID wiring per the accessibility docs. In code reviews, prefer using fuselage-forms wrappers for form fields and verify there is no unnecessary manual ID/aria wiring in files that use these wrappers. If a component uses plain fuselage components, ensure proper id wiring as per docs.

Applied to files:

  • apps/meteor/ee/server/meteor-methods/getReadReceipts.ts
  • apps/meteor/ee/server/lib/message-read-receipt/ReadReceipt.ts
  • apps/meteor/ee/server/api/chat.ts
📚 Learning: 2026-07-31T00:32:03.839Z
Learnt from: ggazzo
Repo: RocketChat/Rocket.Chat PR: 41635
File: apps/meteor/ee/server/api/roles.ts:101-111
Timestamp: 2026-07-31T00:32:03.839Z
Learning: In Rocket.Chat's typed EE REST API routes that use apps/meteor/ee/server/api/v1/middlewares/license.ts, account for unavailable route-level license modules returning HTTP 400 with { success: false, error, errorType: 'error-action-not-allowed' }, not HTTP 403. Declare the existing 400 bad-request response schema for these routes.

Applied to files:

  • apps/meteor/ee/server/api/chat.ts
📚 Learning: 2026-07-31T02:44:35.111Z
Learnt from: ggazzo
Repo: RocketChat/Rocket.Chat PR: 41635
File: apps/meteor/ee/server/api/sessions.ts:114-138
Timestamp: 2026-07-31T02:44:35.111Z
Learning: In Rocket.Chat typed REST response schemas, accept the composition of a Typia-generated entity schema with an `allOf` branch requiring `success: true`: `allOf: [{ $ref: <entity schema> }, { properties: { success: { type: 'boolean', enum: [true] } }, required: ['success'] }]`. Do not flag this pattern when used for REST endpoints, provided TEST_MODE response validation passes, as demonstrated by the `IOAuthApps` and `IEmailInbox` endpoints.

Applied to files:

  • apps/meteor/ee/server/api/chat.ts
🔇 Additional comments (4)
apps/meteor/ee/server/api/chat.ts (1)

16-17: LGTM!

Also applies to: 57-60

apps/meteor/ee/server/meteor-methods/getReadReceipts.ts (2)

40-40: LGTM!


19-23: 🗄️ Data Integrity & Integration

No change needed.

The DDP getReadReceipts method contract exposes only messageId, and the DDP call path forwards only messageId, so pagination values are not silently ignored as a DDP contract mismatch.

			> Likely an incorrect or invalid review comment.
apps/meteor/ee/server/lib/message-read-receipt/ReadReceipt.ts (1)

140-143: LGTM!

Also applies to: 154-160


Walkthrough

The read-receipts endpoint now accepts optional offset and count query parameters. The schema validates both values as non-negative integers. The server applies pagination to deduplicated receipts, and an Enterprise test verifies the response.

Changes

Read-receipts pagination

Layer / File(s) Summary
Pagination contract and validation
packages/rest-typings/src/v1/chat.ts
ChatGetMessageReadReceipts now accepts optional offset and count fields. The validator uses ajvQuery and retains the required messageId and unknown-property checks.
Endpoint pagination flow
apps/meteor/ee/server/api/chat.ts, apps/meteor/ee/server/meteor-methods/getReadReceipts.ts, apps/meteor/ee/server/lib/message-read-receipt/ReadReceipt.ts
The API handler forwards pagination options through getReadReceiptsFunction to ReadReceipt.getReceipts. The receipt list is sliced after deduplication and before user data resolution.
Pagination coverage and release
apps/meteor/tests/end-to-end/api/chat.ts, .changeset/chat-getmessagereadreceipts-pagination-params.md
The Enterprise test verifies offset: 0 and count: 1. The changeset records patch releases for the affected packages.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Client
  participant ChatAPI
  participant getReadReceiptsFunction
  participant ReadReceipt
  Client->>ChatAPI: Request receipts with offset and count
  ChatAPI->>getReadReceiptsFunction: Forward messageId and pagination options
  getReadReceiptsFunction->>ReadReceipt: Retrieve paginated receipts
  ReadReceipt-->>getReadReceiptsFunction: Return sliced receipt list
  getReadReceiptsFunction-->>ChatAPI: Return receipt data
  ChatAPI-->>Client: Return receipts response
Loading

Possibly related PRs

Suggested labels: type: bug

Suggested reviewers: ggazzo, sampaiodiego

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Linked Issues check ⚠️ Warning The PR adds pagination support instead of removing the unsupported parameters from the API documentation as required by issue #41719. Update the API documentation to remove offset and count, or provide explicit issue scope approving implementation of pagination support.
Out of Scope Changes check ⚠️ Warning The pagination implementation and related tests are outside the linked issue scope, which requests documentation changes for unsupported parameters. Limit the PR to removing offset and count from the API documentation, unless the linked issue scope is updated to require pagination support.
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly identifies the main change: adding offset and count pagination support to chat.getMessageReadReceipts.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
packages/ui-client/src/components/GenericMenu/GenericMenu.tsx (1)

57-59: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Keep custom triggers disabled when the menu is empty.

When isMenuEmpty is true and disabled is false or undefined, this branch passes that value to cloneElement. The custom trigger remains enabled even though no menu is rendered. Set disabled: true for this branch and add a regression test for an empty menu with a custom button.

Proposed fix
-			return cloneElement(button, { small: true, icon, disabled, title, className } as any);
+			return cloneElement(button, { small: true, icon, disabled: true, title, className } as any);
🤖 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/ui-client/src/components/GenericMenu/GenericMenu.tsx` around lines
57 - 59, Update the custom trigger handling in GenericMenu so the cloneElement
call always passes disabled: true when isMenuEmpty is true, regardless of the
incoming disabled value; preserve the existing disabled behavior for non-empty
menus and add a regression test covering an empty menu with a custom button.
🤖 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.

Inline comments:
In `@packages/rest-typings/src/v1/chat.ts`:
- Around line 490-508: The ChatGetMessageReadReceipts and
GetMessageReadReceiptsProps request contracts must match the runtime validator,
which rejects PaginatedRequest’s sort and query fields. In
packages/rest-typings/src/v1/chat.ts lines 490-508, and
apps/meteor/ee/server/api/chat.ts lines 15-17, constrain both types to
messageId, offset, and count, unless the runtime schema is intentionally
expanded to support sort and query; keep both definitions consistent.

---

Outside diff comments:
In `@packages/ui-client/src/components/GenericMenu/GenericMenu.tsx`:
- Around line 57-59: Update the custom trigger handling in GenericMenu so the
cloneElement call always passes disabled: true when isMenuEmpty is true,
regardless of the incoming disabled value; preserve the existing disabled
behavior for non-empty menus and add a regression test covering an empty menu
with a custom button.
🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: af6b257c-75a9-4c1f-9f5f-68b2dee8adad

📥 Commits

Reviewing files that changed from the base of the PR and between 73b6281 and 86a3b6c.

📒 Files selected for processing (6)
  • .changeset/chat-getmessagereadreceipts-pagination-params.md
  • .changeset/deprecate-genericmenu-button-prop.md
  • apps/meteor/ee/server/api/chat.ts
  • apps/meteor/tests/end-to-end/api/chat.ts
  • packages/rest-typings/src/v1/chat.ts
  • packages/ui-client/src/components/GenericMenu/GenericMenu.tsx
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
  • GitHub Check: cubic · AI code reviewer
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{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:

  • apps/meteor/tests/end-to-end/api/chat.ts
  • packages/ui-client/src/components/GenericMenu/GenericMenu.tsx
  • apps/meteor/ee/server/api/chat.ts
  • packages/rest-typings/src/v1/chat.ts
apps/meteor/**

📄 CodeRabbit inference engine (CLAUDE.md)

The main Rocket.Chat Meteor application resides in apps/meteor/; place its application code there rather than in other monorepo areas.

Files:

  • apps/meteor/tests/end-to-end/api/chat.ts
  • apps/meteor/ee/server/api/chat.ts
packages/**

📄 CodeRabbit inference engine (CLAUDE.md)

Shared libraries belong in packages/, while other services belong in apps/ and ee/.

Files:

  • packages/ui-client/src/components/GenericMenu/GenericMenu.tsx
  • packages/rest-typings/src/v1/chat.ts
🧠 Learnings (8)
📚 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/deprecate-genericmenu-button-prop.md
  • .changeset/chat-getmessagereadreceipts-pagination-params.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:

  • apps/meteor/tests/end-to-end/api/chat.ts
  • apps/meteor/ee/server/api/chat.ts
  • packages/rest-typings/src/v1/chat.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:

  • apps/meteor/tests/end-to-end/api/chat.ts
  • apps/meteor/ee/server/api/chat.ts
  • packages/rest-typings/src/v1/chat.ts
📚 Learning: 2026-05-06T12:21:44.083Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 40256
File: apps/meteor/client/components/CreateDiscussion/CreateDiscussion.tsx:121-149
Timestamp: 2026-05-06T12:21:44.083Z
Learning: Field wrappers in rocket.chat/fuselage-forms (Field, FieldLabel, FieldRow, FieldError, FieldHint) auto-create htmlFor/id associations, aria-describedby, and role="alert" for errors. Do not manually set htmlFor, id, aria-describedby, or role attributes when using these wrappers. This automatic wiring does not apply to plain rocket.chat/fuselage components, which require explicit ID wiring per the accessibility docs. In code reviews, prefer using fuselage-forms wrappers for form fields and verify there is no unnecessary manual ID/aria wiring in files that use these wrappers. If a component uses plain fuselage components, ensure proper id wiring as per docs.

Applied to files:

  • apps/meteor/tests/end-to-end/api/chat.ts
  • packages/ui-client/src/components/GenericMenu/GenericMenu.tsx
  • apps/meteor/ee/server/api/chat.ts
  • packages/rest-typings/src/v1/chat.ts
📚 Learning: 2026-03-27T14:52:56.865Z
Learnt from: dougfabris
Repo: RocketChat/Rocket.Chat PR: 39892
File: apps/meteor/client/views/room/contextualBar/Threads/Thread.tsx:150-155
Timestamp: 2026-03-27T14:52:56.865Z
Learning: In Rocket.Chat, there are two different `ModalBackdrop` components with different prop APIs. During review, confirm the import source: (1) `rocket.chat/fuselage` `ModalBackdrop` uses `ModalBackdropProps` based on `BoxProps` (so it supports `onClick` and other Box/DOM props) and does not have an `onDismiss` prop; (2) `rocket.chat/ui-client` `ModalBackdrop` uses a narrower props interface like `{ children?: ReactNode; onDismiss?: () => void }` and handles Escape keypress and outside mouse-up, and it does not forward arbitrary DOM props such as `onClick`. Flag mismatched props (e.g., `onDismiss` passed to the fuselage component or `onClick` passed to the ui-client component) and ensure the usage matches the correct component being imported.

Applied to files:

  • packages/ui-client/src/components/GenericMenu/GenericMenu.tsx
📚 Learning: 2026-07-31T00:32:03.839Z
Learnt from: ggazzo
Repo: RocketChat/Rocket.Chat PR: 41635
File: apps/meteor/ee/server/api/roles.ts:101-111
Timestamp: 2026-07-31T00:32:03.839Z
Learning: In Rocket.Chat's typed EE REST API routes that use apps/meteor/ee/server/api/v1/middlewares/license.ts, account for unavailable route-level license modules returning HTTP 400 with { success: false, error, errorType: 'error-action-not-allowed' }, not HTTP 403. Declare the existing 400 bad-request response schema for these routes.

Applied to files:

  • apps/meteor/ee/server/api/chat.ts
📚 Learning: 2026-07-31T02:44:35.111Z
Learnt from: ggazzo
Repo: RocketChat/Rocket.Chat PR: 41635
File: apps/meteor/ee/server/api/sessions.ts:114-138
Timestamp: 2026-07-31T02:44:35.111Z
Learning: In Rocket.Chat typed REST response schemas, accept the composition of a Typia-generated entity schema with an `allOf` branch requiring `success: true`: `allOf: [{ $ref: <entity schema> }, { properties: { success: { type: 'boolean', enum: [true] } }, required: ['success'] }]`. Do not flag this pattern when used for REST endpoints, provided TEST_MODE response validation passes, as demonstrated by the `IOAuthApps` and `IEmailInbox` endpoints.

Applied to files:

  • apps/meteor/ee/server/api/chat.ts
📚 Learning: 2026-05-11T23:14:59.316Z
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 40469
File: packages/rest-typings/src/v1/users.ts:337-337
Timestamp: 2026-05-11T23:14:59.316Z
Learning: In Rocket.Chat REST endpoint typings (e.g., packages/rest-typings/src/v1/users.ts and other rest-typings files), keep the established convention of deriving field types from the domain model (e.g., use IUser indexed access like IUser['statusExpiresAt']) rather than swapping individual fields to serialized primitives (like string) in an ad-hoc way. If a truly different “serialized” representation is needed, perform the refactor consistently across the codebase (not just a single endpoint/field) and ensure all related REST typings stay aligned with the shared serialization types.

Applied to files:

  • packages/rest-typings/src/v1/chat.ts
🔇 Additional comments (4)
apps/meteor/tests/end-to-end/api/chat.ts (1)

2993-3014: LGTM!

.changeset/chat-getmessagereadreceipts-pagination-params.md (1)

1-6: LGTM!

packages/ui-client/src/components/GenericMenu/GenericMenu.tsx (1)

2-2: LGTM!

Also applies to: 31-38

.changeset/deprecate-genericmenu-button-prop.md (1)

1-5: LGTM!

Comment thread packages/rest-typings/src/v1/chat.ts Outdated
@tejaswiverma121-byte
tejaswiverma121-byte force-pushed the fix/chat-getMessageReadReceipts-pagination branch from 86a3b6c to ef223a9 Compare August 8, 2026 09:13
@coderabbitai coderabbitai Bot removed the type: chore label Aug 8, 2026

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 issue found and verified against the latest diff

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="apps/meteor/tests/end-to-end/api/chat.ts">

<violation number="1" location="apps/meteor/tests/end-to-end/api/chat.ts:3010">
P2: This test only asserts HTTP 200 and that `receipts` is an array, so it would pass even if the server ignored `offset`/`count` entirely — which it currently does (the endpoint action destructures only `messageId` and calls `getReadReceiptsFunction(messageId, this.userId)` with no pagination applied). It therefore provides no coverage of the pagination behavior this PR claims to add. Consider verifying actual pagination, e.g. assert that a request with `count` returns at most `count` receipts and that `offset` shifts the returned window.</violation>
</file>

Tip: cubic used a learning from your PR history. Let your coding agent read cubic learnings directly with the cubic MCP.

Re-trigger cubic

.expect('Content-Type', 'application/json')
.expect(200)
.expect((res) => {
expect(res.body).to.have.property('receipts').and.to.be.an('array');

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: This test only asserts HTTP 200 and that receipts is an array, so it would pass even if the server ignored offset/count entirely — which it currently does (the endpoint action destructures only messageId and calls getReadReceiptsFunction(messageId, this.userId) with no pagination applied). It therefore provides no coverage of the pagination behavior this PR claims to add. Consider verifying actual pagination, e.g. assert that a request with count returns at most count receipts and that offset shifts the returned window.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/meteor/tests/end-to-end/api/chat.ts, line 3010:

<comment>This test only asserts HTTP 200 and that `receipts` is an array, so it would pass even if the server ignored `offset`/`count` entirely — which it currently does (the endpoint action destructures only `messageId` and calls `getReadReceiptsFunction(messageId, this.userId)` with no pagination applied). It therefore provides no coverage of the pagination behavior this PR claims to add. Consider verifying actual pagination, e.g. assert that a request with `count` returns at most `count` receipts and that `offset` shifts the returned window.</comment>

<file context>
@@ -2990,6 +2990,28 @@ describe('[Chat]', () => {
+					.expect('Content-Type', 'application/json')
+					.expect(200)
+					.expect((res) => {
+						expect(res.body).to.have.property('receipts').and.to.be.an('array');
+						expect(res.body).to.have.property('success', true);
+					})
</file context>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 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.

Inline comments:
In `@apps/meteor/ee/server/api/chat.ts`:
- Around line 16-17: Forward the request’s offset and count through the
read-receipts handler and retrieval call in
apps/meteor/ee/server/api/chat.ts#L16-L17, preserving pagination semantics end
to end. Retain the public offset/count fields in
packages/rest-typings/src/v1/chat.ts#L492-L493 only with that server
implementation; otherwise remove them from both contracts. Add or update the
end-to-end test to assert paginated results.

In `@packages/rest-typings/src/v1/chat.ts`:
- Around line 502-513: The ChatGetMessageReadReceiptsSchema pagination fields
offset and count currently accept invalid fractional and negative values. Update
both fields to require non-negative integers, and add validation cases covering
fractional and negative inputs while preserving valid pagination behavior.
🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: ce95c761-b154-43b8-b3f6-0fd4ea403242

📥 Commits

Reviewing files that changed from the base of the PR and between 86a3b6c and ef223a9.

📒 Files selected for processing (2)
  • apps/meteor/ee/server/api/chat.ts
  • packages/rest-typings/src/v1/chat.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{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:

  • apps/meteor/ee/server/api/chat.ts
  • packages/rest-typings/src/v1/chat.ts
apps/meteor/**

📄 CodeRabbit inference engine (CLAUDE.md)

The main Rocket.Chat Meteor application resides in apps/meteor/; place its application code there rather than in other monorepo areas.

Files:

  • apps/meteor/ee/server/api/chat.ts
packages/**

📄 CodeRabbit inference engine (CLAUDE.md)

Shared libraries belong in packages/, while other services belong in apps/ and ee/.

Files:

  • packages/rest-typings/src/v1/chat.ts
🧠 Learnings (6)
📚 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:

  • apps/meteor/ee/server/api/chat.ts
  • packages/rest-typings/src/v1/chat.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:

  • apps/meteor/ee/server/api/chat.ts
  • packages/rest-typings/src/v1/chat.ts
📚 Learning: 2026-05-06T12:21:44.083Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 40256
File: apps/meteor/client/components/CreateDiscussion/CreateDiscussion.tsx:121-149
Timestamp: 2026-05-06T12:21:44.083Z
Learning: Field wrappers in rocket.chat/fuselage-forms (Field, FieldLabel, FieldRow, FieldError, FieldHint) auto-create htmlFor/id associations, aria-describedby, and role="alert" for errors. Do not manually set htmlFor, id, aria-describedby, or role attributes when using these wrappers. This automatic wiring does not apply to plain rocket.chat/fuselage components, which require explicit ID wiring per the accessibility docs. In code reviews, prefer using fuselage-forms wrappers for form fields and verify there is no unnecessary manual ID/aria wiring in files that use these wrappers. If a component uses plain fuselage components, ensure proper id wiring as per docs.

Applied to files:

  • apps/meteor/ee/server/api/chat.ts
  • packages/rest-typings/src/v1/chat.ts
📚 Learning: 2026-07-31T00:32:03.839Z
Learnt from: ggazzo
Repo: RocketChat/Rocket.Chat PR: 41635
File: apps/meteor/ee/server/api/roles.ts:101-111
Timestamp: 2026-07-31T00:32:03.839Z
Learning: In Rocket.Chat's typed EE REST API routes that use apps/meteor/ee/server/api/v1/middlewares/license.ts, account for unavailable route-level license modules returning HTTP 400 with { success: false, error, errorType: 'error-action-not-allowed' }, not HTTP 403. Declare the existing 400 bad-request response schema for these routes.

Applied to files:

  • apps/meteor/ee/server/api/chat.ts
📚 Learning: 2026-07-31T02:44:35.111Z
Learnt from: ggazzo
Repo: RocketChat/Rocket.Chat PR: 41635
File: apps/meteor/ee/server/api/sessions.ts:114-138
Timestamp: 2026-07-31T02:44:35.111Z
Learning: In Rocket.Chat typed REST response schemas, accept the composition of a Typia-generated entity schema with an `allOf` branch requiring `success: true`: `allOf: [{ $ref: <entity schema> }, { properties: { success: { type: 'boolean', enum: [true] } }, required: ['success'] }]`. Do not flag this pattern when used for REST endpoints, provided TEST_MODE response validation passes, as demonstrated by the `IOAuthApps` and `IEmailInbox` endpoints.

Applied to files:

  • apps/meteor/ee/server/api/chat.ts
📚 Learning: 2026-05-11T23:14:59.316Z
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 40469
File: packages/rest-typings/src/v1/users.ts:337-337
Timestamp: 2026-05-11T23:14:59.316Z
Learning: In Rocket.Chat REST endpoint typings (e.g., packages/rest-typings/src/v1/users.ts and other rest-typings files), keep the established convention of deriving field types from the domain model (e.g., use IUser indexed access like IUser['statusExpiresAt']) rather than swapping individual fields to serialized primitives (like string) in an ad-hoc way. If a truly different “serialized” representation is needed, perform the refactor consistently across the codebase (not just a single endpoint/field) and ensure all related REST typings stay aligned with the shared serialization types.

Applied to files:

  • packages/rest-typings/src/v1/chat.ts

Comment thread apps/meteor/ee/server/api/chat.ts
Comment thread packages/rest-typings/src/v1/chat.ts
@AlgoArtist06

Copy link
Copy Markdown
Contributor

@tejaswiverma121-byte I had opened #41723 with essentially this same change before noticing yours - closing it, since you were here first. Two details from it that may be worth folding in:

  1. Ordering is not deterministic before slicing. getReceipts merges hot and cold storage into a Map and slices the result, but the merge order depends on which receipts happen to live in each store, so the same offset/count can return different rows across calls. Sorting before slicing pins it:

    let receipts = [...new Map([...hotReceipts, ...coldReceipts].map((r) => [r._id, r])).values()].sort((a, b) =>
        a._id.localeCompare(b._id),
    );
  2. count=0. Elsewhere in the REST API count=0 means "no limit". Here receipts.slice(offset, offset + 0) returns an empty array instead:

    receipts = receipts.slice(offset, count === 0 ? undefined : offset + count);

Both are small; feel free to take or ignore.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Docs for chat.getMessageReadReceipts incorrectly document offset/count pagination params

2 participants