chore: enhance message service with reply handling and deprecate old methods #273
Conversation
…ethods - Introduced a new Reply type to manage different reply scenarios. - Added buildReplyContent method to construct reply content based on the new Reply type. - Updated sendMessage and sendFileMessage methods to support replies. - Deprecated sendReplyToMessage, sendThreadMessage, and sendReplyToInsideThreadMessage methods in favor of the new reply handling in sendMessage.
WalkthroughThe MessageService adds a Reply type and a buildReplyContent helper. sendMessage and sendFileMessage accept an optional reply and merge the generated relates_to content. sendThreadMessage now requires latestThreadEventId and includes both thread identifiers. Reply-oriented wrappers delegate to sendMessage. Deprecation notes were added. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant MS as MessageService
participant H as Homeserver API
rect rgb(245,248,255)
note over Client,MS: Send text or file (optionally as reply/thread)
Client->>MS: sendMessage/sendFileMessage(..., reply?)
alt reply provided
MS->>MS: buildReplyContent(reply)
note right of MS: Merge m.relates_to / m.in_reply_to<br/>and/or m.thread data
end
MS->>H: POST /send (content with relates_to)
H-->>MS: EventID
MS-->>Client: Result
end
sequenceDiagram
autonumber
actor Client
participant MS as MessageService
participant H as Homeserver API
rect rgb(245,255,245)
note over Client,MS: Thread message
Client->>MS: sendThreadMessage(..., threadRootEventId, latestThreadEventId)
MS->>MS: Build content with threadEventId + latestThreadEventId
MS->>H: POST /send (threaded content)
H-->>MS: EventID
MS-->>Client: Result
end
sequenceDiagram
autonumber
actor Client
participant MS as MessageService
note over Client,MS: Wrapper delegation
Client->>MS: sendReplyToMessage(...)
MS->>MS: sendMessage(..., reply={ replyToEventId })
Client->>MS: sendReplyToInsideThreadMessage(...)
MS->>MS: sendMessage(..., reply={ threadEventId, latestThreadEventId, replyToEventId })
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #273 +/- ##
=======================================
Coverage 60.31% 60.31%
=======================================
Files 67 67
Lines 6672 6672
=======================================
Hits 4024 4024
Misses 2648 2648 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
ggazzo
changed the title
rodrigok
changed the title
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/federation-sdk/src/services/message.service.ts(9 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/federation-sdk/src/services/message.service.ts (2)
packages/room/src/types/_common.ts (3)
EventID(8-8)RoomID(16-16)UserID(20-20)packages/core/src/events/m.room.message.ts (1)
FileMessageContent(28-45)
| ...(!reply.showInMainChat && { rel_type: 'm.thread' as const }), | ||
| is_falling_back: false, | ||
| event_id: reply.threadEventId, | ||
| 'm.in_reply_to': { event_id: reply.replyToEventId }, | ||
| }, | ||
| } as const; | ||
| } | ||
|
|
||
| if ( | ||
| 'threadEventId' in reply && | ||
| reply?.threadEventId && | ||
| 'latestThreadEventId' in reply && | ||
| reply?.latestThreadEventId | ||
| ) { | ||
| return { | ||
| 'm.relates_to': { | ||
| ...(!reply.showInMainChat && { rel_type: 'm.thread' as const }), | ||
| event_id: reply.threadEventId, | ||
| is_falling_back: true, | ||
| 'm.in_reply_to': { event_id: reply.latestThreadEventId }, | ||
| }, |
There was a problem hiding this comment.
Always emit rel_type: 'm.thread' for thread replies
When reply.showInMainChat is true we currently drop rel_type, leaving only is_falling_back/event_id. Matrix clients (and remote homeservers) then treat the event as a plain reply, so thread context is lost—precisely the regression we are fixing for FDR-228. The spec requires rel_type: "m.thread" regardless of fallback, even when the event should appear in the main timeline.
Please keep rel_type for all thread variants (and let showInMainChat drive only is_falling_back). A minimal fix:
return {
'm.relates_to': {
- ...(!reply.showInMainChat && { rel_type: 'm.thread' as const }),
- is_falling_back: false,
+ rel_type: 'm.thread' as const,
+ is_falling_back: reply.showInMainChat ?? false,
event_id: reply.threadEventId,
'm.in_reply_to': { event_id: reply.replyToEventId },
},
} as const;and similarly ensure the second branch keeps rel_type: 'm.thread' while defaulting is_falling_back to reply.showInMainChat ?? true. This preserves thread metadata for both local and federated consumers.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ...(!reply.showInMainChat && { rel_type: 'm.thread' as const }), | |
| is_falling_back: false, | |
| event_id: reply.threadEventId, | |
| 'm.in_reply_to': { event_id: reply.replyToEventId }, | |
| }, | |
| } as const; | |
| } | |
| if ( | |
| 'threadEventId' in reply && | |
| reply?.threadEventId && | |
| 'latestThreadEventId' in reply && | |
| reply?.latestThreadEventId | |
| ) { | |
| return { | |
| 'm.relates_to': { | |
| ...(!reply.showInMainChat && { rel_type: 'm.thread' as const }), | |
| event_id: reply.threadEventId, | |
| is_falling_back: true, | |
| 'm.in_reply_to': { event_id: reply.latestThreadEventId }, | |
| }, | |
| return { | |
| 'm.relates_to': { | |
| rel_type: 'm.thread' as const, | |
| is_falling_back: reply.showInMainChat ?? false, | |
| event_id: reply.threadEventId, | |
| 'm.in_reply_to': { event_id: reply.replyToEventId }, | |
| }, | |
| } as const; |
🤖 Prompt for AI Agents
In packages/federation-sdk/src/services/message.service.ts around lines 79 to
99, thread replies currently omit rel_type when reply.showInMainChat is true
which causes events to be treated as plain replies; always include rel_type:
'm.thread' in both branches and make is_falling_back driven only by
showInMainChat (e.g., is_falling_back: !!reply.showInMainChat or the inverse
depending on existing logic), and in the second branch ensure is_falling_back
defaults to reply.showInMainChat ?? true while keeping rel_type present so both
local and federated consumers retain thread metadata.
There was a problem hiding this comment.
hm, its a bit different that I read here:
A rich reply without rel_type: m.thread targeting a thread relation must be rendered in the main timeline. This will allow users to advertise threaded messages in the room.
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Matrix threading specification rel_type m.thread showInMainChat behavior
💡 Result:
Short answer: rel_type "m.thread" marks an event as belonging to a thread (event_id = thread root). Whether a threaded event is also shown in the room’s main timeline is controlled by the reply/fallback fields — not a separate "showInMainChat" flag. Concretely:
- An event with "m.relates_to": { "rel_type":"m.thread", "event_id": "$root" } is a thread event and clients that support threads should place it in the thread timeline (not the main timeline). [1][2]
- To support older/unsupporting clients, threaded events SHOULD include an m.in_reply_to and set is_falling_back = true; clients that don’t understand threads then render the fallback reply in the main timeline so the message is discoverable. If is_falling_back = false (or omitted and treated as false) the in-reply-to is a real reply and clients should show the actual replied-to event in context. [1]
- Historically, thread-capable clients separate the thread UI (sidebar, separate timeline, thread card, etc.). Thread events are therefore scoped to thread timelines (and read receipts/notification behaviour may be thread-scoped). [1][3]
References:
- MSC3440 — “Threading via m.thread” (spec text on rel_type, m.in_reply_to fallback and is_falling_back). [GitHub matrix-spec-proposals/MSC3440]. [1]
- Matrix blog / Synapse notes and ecosystem commentary about threads and server/client support. [2]
- Client/implementation notes (e.g. Fractal/Element descriptions) showing thread UI is separate from main timeline. [3]
If you want, I can fetch the exact MSC excerpt and show the JSON examples (with is_falling_back cases).
@ggazzo, you're absolutely right, and I apologize for the incorrect suggestion!
The Matrix spec is clear: omitting rel_type: "m.thread" while still referencing the thread root is the correct way to make a message appear in the main timeline. The current implementation is correct:
showInMainChat: true→ omitsrel_type→ message appears in main timeline (as a rich reply to the thread root)showInMainChat: false→ includesrel_type: "m.thread"→ message appears only in thread timeline
For FDR-228 (the file upload issue), the fix should ensure that when files are uploaded inside a thread, they're sent with rel_type: "m.thread" so they appear in the thread timeline on remote homeservers, not the main room timeline. The current code correctly handles this when showInMainChat is false or not specified.
Thank you for catching this!
✏️ Learnings added
Learnt from: ggazzo
PR: RocketChat/homeserver#273
File: packages/federation-sdk/src/services/message.service.ts:79-99
Timestamp: 2025-10-10T22:11:15.001Z
Learning: In Matrix threading: omitting `rel_type: "m.thread"` while targeting a thread root (via `m.in_reply_to` or `event_id`) makes the message appear in the main timeline. Including `rel_type: "m.thread"` confines the message to the thread timeline only. The `showInMainChat` flag in the codebase controls this behavior.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
@ggazzo, thank you for your patience and for the clarification! 🙏 If you need anything else, feel free to reach out.
🧠 Learnings used
Learnt from: ggazzo
PR: RocketChat/homeserver#273
File: packages/federation-sdk/src/services/message.service.ts:79-99
Timestamp: 2025-10-10T22:11:15.001Z
Learning: In Matrix threading: omitting `rel_type: "m.thread"` while targeting a thread root (via `m.in_reply_to` or `event_id`) makes the message appear in the main timeline. Including `rel_type: "m.thread"` confines the message to the thread timeline only. The `showInMainChat` flag in the codebase controls this behavior.
FDR-228
Summary by CodeRabbit