Conversation
WalkthroughAdds Matrix read-receipt EDU support: new ReceiptEDU type and type guard, federation SDK methods to send receipts, event/service processing for incoming receipts, and a configuration flag to enable receipt processing. Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant SDK as FederationSDK
participant EduSvc as EduService
participant Remote as Remote Server
App->>SDK: sendReadReceipt(roomId, userId, eventIds, threadId)
SDK->>EduSvc: sendReadReceipt(...)
EduSvc->>EduSvc: Build ReceiptEDU (m.receipt, m.read)
EduSvc->>Remote: Broadcast EDU to room servers (except origin)
Remote-->>EduSvc: Acknowledgment / response
sequenceDiagram
participant Remote as Remote Server
participant EventSvc as EventService
participant Config as AppConfig
participant App as Homeserver
Remote->>EventSvc: POST incoming ReceiptEDU
EventSvc->>Config: Check edu.processReceipt
alt processReceipt enabled
EventSvc->>EventSvc: isReceiptEDU & validate content
EventSvc->>App: Emit homeserver.matrix.receipt (room_id, user_id, event_ids, ts, thread_id?)
App-->>EventSvc: Consume/handle event
else processReceipt disabled
EventSvc-->>Remote: Skip processing / drop EDU
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 8 files
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="packages/federation-sdk/src/services/event.service.ts">
<violation number="1" location="packages/federation-sdk/src/services/event.service.ts:384">
P1: `processReceiptEDU` dereferences `data.ts` without validating `data`, so malformed receipt payloads can throw and stop processing the rest of that EDU.</violation>
</file>
<file name="packages/core/src/events/edu/m.receipt.ts">
<violation number="1" location="packages/core/src/events/edu/m.receipt.ts:33">
P2: Receipt EDU construction is duplicated instead of reusing a single builder, which can cause schema drift and inconsistent payloads.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
packages/core/src/events/edu/index.ts (1)
9-11: Consider addingReceiptEDUto theMatrixEDUTypesunion.The new
ReceiptEDUtype is exported but not included in theMatrixEDUTypesunion on line 11. WhileReceiptEDUextendsBaseEDUso it's technically compatible, adding it explicitly would improve type clarity and discoverability:♻️ Proposed change
import type { BaseEDU } from './base'; import type { PresenceEDU } from './m.presence'; +import type { ReceiptEDU } from './m.receipt'; import type { TypingEDU } from './m.typing'; export * from './base'; export * from './m.typing'; export * from './m.presence'; export * from './m.receipt'; -export type MatrixEDUTypes = TypingEDU | PresenceEDU | BaseEDU; +export type MatrixEDUTypes = TypingEDU | PresenceEDU | ReceiptEDU | BaseEDU;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/events/edu/index.ts` around lines 9 - 11, MatrixEDUTypes currently unions TypingEDU | PresenceEDU | BaseEDU but omits the exported ReceiptEDU; update the MatrixEDUTypes type to include ReceiptEDU (i.e., TypingEDU | PresenceEDU | ReceiptEDU | BaseEDU) so the ReceiptEDU type is explicitly discoverable and documented alongside TypingEDU, PresenceEDU and BaseEDU.packages/federation-sdk/src/services/event.service.ts (1)
281-294: Inconsistent early return pattern inprocessEDU.The
isTypingEDUbranch (line 286) has an explicitreturnafter processing, but theisPresenceEDUandisReceiptEDUbranches do not. While this works because the EDU types are mutually exclusive (each EDU has exactly oneedu_type), the inconsistent pattern is confusing and could lead to bugs if additional logic is added later.♻️ Proposed fix to add consistent returns
private async processEDU(edu: BaseEDU): Promise<void> { const { origin } = edu; if (isTypingEDU(edu)) { await this.processTypingEDU(edu, origin); return; } if (isPresenceEDU(edu)) { await this.processPresenceEDU(edu, origin); + return; } if (isReceiptEDU(edu)) { await this.processReceiptEDU(edu); + return; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/federation-sdk/src/services/event.service.ts` around lines 281 - 294, The processEDU method uses an early return only for the isTypingEDU branch, which is inconsistent and confusing; update processEDU so that after calling processTypingEDU, processPresenceEDU, or processReceiptEDU it returns immediately (i.e., add explicit returns after the processPresenceEDU and processReceiptEDU calls) to make the control flow consistent and prevent accidental fall-through when modifying logic later; locate the method processEDU and the calls to processTypingEDU, processPresenceEDU, and processReceiptEDU to apply this change.packages/federation-sdk/src/services/edu.service.ts (1)
84-99: Avoid includingthread_id: undefinedin the EDU payload.When
threadIdis not provided, the EDU will containthread_id: undefined. While JSON serialization typically omitsundefinedvalues, it's cleaner and more explicit to conditionally include the field only when it has a value.♻️ Proposed fix using spread operator
const receiptEDU: ReceiptEDU = { edu_type: 'm.receipt', content: { [roomId]: { 'm.read': { [userId]: { data: { ts: Date.now(), - thread_id: threadId, + ...(threadId && { thread_id: threadId }), }, event_ids: eventIds, }, }, }, }, };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/federation-sdk/src/services/edu.service.ts` around lines 84 - 99, The receiptEDU payload construction in edu.service.ts currently always includes thread_id which can be undefined; update the data object inside the ReceiptEDU (the receiptEDU variable) to only add thread_id when threadId is present (e.g., use a conditional/spread so data contains ts and conditionally {...thread_id: threadId} only if threadId is defined) so the EDU does not carry thread_id: undefined.packages/core/src/events/edu/m.receipt.ts (1)
33-47: Factory function missingthreadIdparameter.The
createReceiptEDUfactory doesn't accept an optionalthreadIdparameter, even though theReceiptEDUinterface supportsthread_idin the data object. This forces callers to construct the EDU manually when thread support is needed (as done inpackages/federation-sdk/src/services/edu.service.tslines 84-99).Consider adding the parameter for consistency and reusability:
♻️ Proposed fix to add threadId support
-export const createReceiptEDU = (roomId: RoomID, userId: UserID, eventIds: string[]): ReceiptEDU => ({ +export const createReceiptEDU = (roomId: RoomID, userId: UserID, eventIds: string[], threadId?: string): ReceiptEDU => ({ edu_type: 'm.receipt', content: { [roomId]: { 'm.read': { [userId]: { data: { ts: Date.now(), + thread_id: threadId, }, event_ids: eventIds, }, }, }, }, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/events/edu/m.receipt.ts` around lines 33 - 47, The createReceiptEDU factory should accept an optional threadId parameter and include it as thread_id inside the data object when provided; update the function signature for createReceiptEDU to add an optional threadId: string | undefined parameter and, inside the returned ReceiptEDU content for [roomId]['m.read'][userId].data, set thread_id: threadId only when threadId is defined so existing behavior is preserved, ensuring the produced object matches the ReceiptEDU interface that supports thread_id.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/federation-sdk/src/services/event.service.ts`:
- Around line 378-399: The loop handling readReceipts must validate that
receiptData.data.ts is present and a number before emitting; update the
for-await block that destructures receiptData (and the threadId extraction) to
check typeof data?.ts === 'number' (or Number.isFinite(data.ts)) and if invalid
call this.logger.warn with context (userId, roomId) and continue, then emit via
this.eventEmitterService.emit('homeserver.matrix.receipt', { room_id: roomId,
user_id: userId, event_ids, ts: data.ts, thread_id: threadId }) only when the ts
validation passes so listeners never receive undefined ts.
---
Nitpick comments:
In `@packages/core/src/events/edu/index.ts`:
- Around line 9-11: MatrixEDUTypes currently unions TypingEDU | PresenceEDU |
BaseEDU but omits the exported ReceiptEDU; update the MatrixEDUTypes type to
include ReceiptEDU (i.e., TypingEDU | PresenceEDU | ReceiptEDU | BaseEDU) so the
ReceiptEDU type is explicitly discoverable and documented alongside TypingEDU,
PresenceEDU and BaseEDU.
In `@packages/core/src/events/edu/m.receipt.ts`:
- Around line 33-47: The createReceiptEDU factory should accept an optional
threadId parameter and include it as thread_id inside the data object when
provided; update the function signature for createReceiptEDU to add an optional
threadId: string | undefined parameter and, inside the returned ReceiptEDU
content for [roomId]['m.read'][userId].data, set thread_id: threadId only when
threadId is defined so existing behavior is preserved, ensuring the produced
object matches the ReceiptEDU interface that supports thread_id.
In `@packages/federation-sdk/src/services/edu.service.ts`:
- Around line 84-99: The receiptEDU payload construction in edu.service.ts
currently always includes thread_id which can be undefined; update the data
object inside the ReceiptEDU (the receiptEDU variable) to only add thread_id
when threadId is present (e.g., use a conditional/spread so data contains ts and
conditionally {...thread_id: threadId} only if threadId is defined) so the EDU
does not carry thread_id: undefined.
In `@packages/federation-sdk/src/services/event.service.ts`:
- Around line 281-294: The processEDU method uses an early return only for the
isTypingEDU branch, which is inconsistent and confusing; update processEDU so
that after calling processTypingEDU, processPresenceEDU, or processReceiptEDU it
returns immediately (i.e., add explicit returns after the processPresenceEDU and
processReceiptEDU calls) to make the control flow consistent and prevent
accidental fall-through when modifying logic later; locate the method processEDU
and the calls to processTypingEDU, processPresenceEDU, and processReceiptEDU to
apply this change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b4f8b535-20e2-41d5-aab8-a07da940da7c
📒 Files selected for processing (8)
packages/core/src/events/edu/index.tspackages/core/src/events/edu/m.receipt.tspackages/federation-sdk/src/index.tspackages/federation-sdk/src/sdk.tspackages/federation-sdk/src/services/config.service.tspackages/federation-sdk/src/services/edu.service.tspackages/federation-sdk/src/services/event.service.tspackages/homeserver/src/homeserver.module.ts
📜 Review details
⏰ 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). (1)
- GitHub Check: cubic · AI code reviewer
🔇 Additional comments (5)
packages/federation-sdk/src/services/config.service.ts (1)
31-31: LGTM!The optional
processReceiptflag is correctly added to both the interface and schema. The optional nature aligns with the "turned on by default" behavior inprocessReceiptEDUwhereundefinedis treated as enabled.Also applies to: 63-63
packages/federation-sdk/src/index.ts (1)
70-76: LGTM!The
'homeserver.matrix.receipt'event signature is well-defined with appropriate types. The fields align correctly with what's emitted inprocessReceiptEDU.packages/federation-sdk/src/sdk.ts (1)
269-272: LGTM!The
sendReadReceiptmethod follows the established delegation pattern used by other SDK methods.packages/federation-sdk/src/services/event.service.ts (1)
1-19: LGTM!The import changes correctly add
ReceiptEDUtype andisReceiptEDUtype guard from federation-core.packages/homeserver/src/homeserver.module.ts (1)
78-78: I need the review comment to rewrite. Please provide the review comment that needs to be verified and rewritten within<review_comment>tags.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #339 +/- ##
==========================================
- Coverage 50.78% 50.33% -0.45%
==========================================
Files 97 97
Lines 10949 11050 +101
==========================================
+ Hits 5560 5562 +2
- Misses 5389 5488 +99 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/federation-sdk/src/services/edu.service.ts`:
- Around line 84-99: The ReceiptEDU construction always sets thread_id to 'main'
which incorrectly marks receipts as threaded; change the build in
services/edu.service.ts (the receiptEDU object created in the function that uses
roomId, userId, eventIds, threadId) so that the data object only includes a
thread_id property when threadId is provided—i.e., construct the inner data
payload (used inside ReceiptEDU.content[roomId]['m.read'][userId].data) and add
data.thread_id = threadId only if threadId is truthy, otherwise omit the
thread_id field entirely.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 03958c06-5163-4042-8b9c-a0cc7a4f716d
📒 Files selected for processing (4)
packages/core/src/events/edu/m.receipt.tspackages/federation-sdk/src/services/edu.service.tspackages/federation-sdk/src/services/event.service.tspackages/homeserver/src/homeserver.module.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/core/src/events/edu/m.receipt.ts
📜 Review details
⏰ 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). (1)
- GitHub Check: cubic · AI code reviewer
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-10-10T22:18:31.655Z
Learnt from: sampaiodiego
Repo: RocketChat/homeserver PR: 224
File: packages/federation-sdk/src/services/event-authorization.service.ts:261-268
Timestamp: 2025-10-10T22:18:31.655Z
Learning: In packages/federation-sdk/src/services/state.service.ts, the method `StateService.getLatestRoomState(roomId: string)` has return type `Promise<State>` and never returns undefined. If the state is not found, it throws an error with message "No state found for room ${roomId}" instead of returning undefined.
Applied to files:
packages/federation-sdk/src/services/event.service.ts
📚 Learning: 2025-10-07T13:17:41.878Z
Learnt from: debdutdeb
Repo: RocketChat/homeserver PR: 252
File: packages/federation-sdk/src/repositories/state-graph.repository.ts:60-63
Timestamp: 2025-10-07T13:17:41.878Z
Learning: In the codebase, when types explicitly declare a field as a non-nullable, non-optional string (e.g., `stateKey: string` in `StateGraphStore`), trust the type system and avoid adding defensive coalescing or null-checks in data pipelines. If violations occur, they should be fixed at the source where the contract is broken, not masked with defensive code.
Applied to files:
packages/federation-sdk/src/services/event.service.ts
🔇 Additional comments (6)
packages/homeserver/src/homeserver.module.ts (1)
75-79: LGTM! Consistent with commit intent for receipts off by default.The
=== 'true'pattern correctly defaultsprocessReceipttofalsewhen the env var is unset, matching the commit message "read receipts turned off by default". This contrasts withprocessTypingwhich uses!== 'false'(default ON), but the difference is intentional per the PR objectives.packages/federation-sdk/src/services/event.service.ts (3)
1-19: LGTM! Imports correctly structured.The
ReceiptEDUtype andisReceiptEDUguard are properly imported from@rocket.chat/federation-core.
291-293: LGTM! Receipt EDU routing correctly wired.The
isReceiptEDUtype guard routes toprocessReceiptEDU, following the same pattern as presence handling.
378-394: Missing validation fordata.tsbefore emitting.The code validates
event_idsbut doesn't validate thatdata.tsexists and is a number. Per theHomeserverEventSignaturescontract inpackages/federation-sdk/src/index.ts(lines 70-76),tsis declared asnumber(not optional). Malformed EDUs could passts: undefinedto subscribers.packages/federation-sdk/src/services/edu.service.ts (2)
1-1: LGTM! Import correctly updated.
ReceiptEDUtype properly imported alongside existing types.
101-118: LGTM! Server resolution and error handling follow established patterns.The implementation correctly filters out the origin server, sends the EDU to remaining servers, and handles errors consistently with other EDU methods.
| const receiptEDU: ReceiptEDU = { | ||
| edu_type: 'm.receipt', | ||
| content: { | ||
| [roomId]: { | ||
| 'm.read': { | ||
| [userId]: { | ||
| data: { | ||
| ts: Date.now(), | ||
| thread_id: threadId || 'main', | ||
| }, | ||
| event_ids: eventIds, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Matrix m.receipt EDU thread_id field specification
💡 Result:
In Matrix, the thread_id field is part of the read receipt metadata (and related receipt objects) to indicate which thread a receipt applies to.
Server-server (m.receipt EDU)
In the federation m.receipt EDU, thread_id appears under Read Receipt Metadata (data) and is defined as: a string which is the root thread event’s ID, or the literal "main", identifying which thread the receipt is intended to be under. [1]
Client-server (m.receipt ephemeral event + /receipt API)
In the client-server m.receipt ephemeral event content, each user’s receipt object can include: [2]
thread_id(string): the root thread event’s ID or"main".- If not specified, the receipt is unthreaded (default).
- This threaded receipt support was added in Matrix spec v1.4. [2]
The POST /_matrix/client/v3/rooms/{roomId}/receipt/{receiptType}/{eventId} endpoint also accepts an optional thread_id in the request body with the same meaning (root event ID or "main"; omitted = unthreaded). [2]
Sources: Matrix Spec server-server receipts (m.receipt EDU) [1], Matrix Spec client-server receipts (m.receipt) and /receipt endpoint [2].
Fix thread_id field to be conditionally included, not always present.
The code sets thread_id: threadId || 'main' (line 92), which always includes the field. However, according to the Matrix spec, thread_id should be omitted when the receipt is unthreaded. Omitting the field results in an unthreaded receipt (default), while setting thread_id: 'main' explicitly targets the main thread—these are distinct states. When threadId is not provided, the field should be excluded entirely rather than defaulting to 'main'.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/federation-sdk/src/services/edu.service.ts` around lines 84 - 99,
The ReceiptEDU construction always sets thread_id to 'main' which incorrectly
marks receipts as threaded; change the build in services/edu.service.ts (the
receiptEDU object created in the function that uses roomId, userId, eventIds,
threadId) so that the data object only includes a thread_id property when
threadId is provided—i.e., construct the inner data payload (used inside
ReceiptEDU.content[roomId]['m.read'][userId].data) and add data.thread_id =
threadId only if threadId is truthy, otherwise omit the thread_id field
entirely.
3 participants
FGA-12
Summary by CodeRabbit
New Features
Configuration