Skip to content

chore: enhance message service with reply handling and deprecate old methods #273

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
ggazzo merged 1 commit into from
Oct 10, 2025
Merged

chore: enhance message service with reply handling and deprecate old methods #273

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
195 changes: 97 additions & 98 deletions packages/federation-sdk/src/services/message.service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,21 @@ import { FederationService } from './federation.service';
import { RoomService } from './room.service';
import { StateService } from './state.service';

type Reply =
| {
threadEventId: EventID;
replyToEventId: EventID;
showInMainChat?: boolean;
}
| {
threadEventId: EventID;
latestThreadEventId: EventID;
showInMainChat?: boolean;
}
| {
replyToEventId: EventID;
};

// File message content type
export type FileMessageContent = {
body: string;
Expand Down Expand Up @@ -52,11 +67,56 @@ export class MessageService {
private readonly eventRepository: EventRepository,
) {}

private buildReplyContent(reply: Reply) {
if (
'replyToEventId' in reply &&
reply?.replyToEventId &&
'threadEventId' in reply &&
reply?.threadEventId
) {
return {
'm.relates_to': {
...(!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 },
},
Comment on lines +79 to +99
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

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.

Suggested change
...(!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.

Copy link
Member Author

@ggazzo ggazzo Oct 10, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@coderabbitai coderabbitai bot Oct 10, 2025

Choose a reason for hiding this comment

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

🧩 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:

  1. 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]
  2. Matrix blog / Synapse notes and ecosystem commentary about threads and server/client support. [2]
  3. 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 → omits rel_type → message appears in main timeline (as a rich reply to the thread root)
  • showInMainChat: false → includes rel_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.

Copy link
Member Author

@ggazzo ggazzo Oct 10, 2025

Choose a reason for hiding this comment

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

no problem :)

Copy link
Contributor

@coderabbitai coderabbitai bot Oct 10, 2025

Choose a reason for hiding this comment

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

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

} as const;
}

if ('replyToEventId' in reply && reply?.replyToEventId) {
return {
'm.relates_to': {
'm.in_reply_to': {
event_id: reply.replyToEventId,
},
},
} as const;
}
}

async sendMessage(
roomId: RoomID,
rawMessage: string,
formattedMessage: string,
senderUserId: UserID,
reply?: Reply,
): Promise<PersistentEventBase> {
const roomVersion = await this.stateService.getRoomVersion(roomId);
if (!roomVersion) {
Expand All @@ -73,6 +133,7 @@ export class MessageService {
body: rawMessage,
format: 'org.matrix.custom.html',
formatted_body: formattedMessage,
...(reply && this.buildReplyContent(reply)),
},
room_id: roomId,
auth_events: [],
Expand All @@ -94,6 +155,10 @@ export class MessageService {
return event;
}

/**
*
* @deprecated Use sendMessage and replyToEventId instead
*/
async sendReplyToMessage(
roomId: RoomID,
rawMessage: string,
Expand All @@ -108,44 +173,22 @@ export class MessageService {
);
}

const event = await this.stateService.buildEvent<'m.room.message'>(
return this.sendMessage(
roomId,
rawMessage,
formattedMessage,
senderUserId,
{
type: 'm.room.message',
content: {
msgtype: 'm.text',
body: rawMessage,
format: 'org.matrix.custom.html',
formatted_body: formattedMessage,
'm.relates_to': {
'm.in_reply_to': {
event_id: eventToReplyTo,
},
},
},
room_id: roomId,
auth_events: [],
depth: 0,
prev_events: [],
origin_server_ts: Date.now(),
sender: senderUserId,
replyToEventId: eventToReplyTo,
},
roomVersion,
);

await this.stateService.handlePdu(event);
if (event.rejected) {
throw new Error(event.rejectReason);
}

void this.federationService.sendEventToAllServersInRoom(event);

return event;
}

async sendFileMessage(
roomId: RoomID,
content: FileMessageContent,
senderUserId: UserID,
reply?: Reply,
): Promise<PersistentEventBase> {
const roomVersion = await this.stateService.getRoomVersion(roomId);
if (!roomVersion) {
Expand All @@ -157,7 +200,10 @@ export class MessageService {
const event = await this.stateService.buildEvent<'m.room.message'>(
{
type: 'm.room.message',
content: content,
content: {
...content,
...(reply && this.buildReplyContent(reply)),
},
room_id: roomId,
auth_events: [],
depth: 0,
Expand All @@ -178,13 +224,16 @@ export class MessageService {
return event;
}

/**
* @deprecated Use sendMessage and threadEventId/replyToEventId instead
*/
async sendThreadMessage(
roomId: RoomID,
rawMessage: string,
formattedMessage: string,
senderUserId: UserID,
threadRootEventId: EventID,
latestThreadEventId?: EventID,
latestThreadEventId: EventID,
): Promise<PersistentEventBase> {
const roomVersion = await this.stateService.getRoomVersion(roomId);
if (!roomVersion) {
Expand All @@ -193,47 +242,21 @@ export class MessageService {
);
}

const event = await this.stateService.buildEvent<'m.room.message'>(
return this.sendMessage(
roomId,
rawMessage,
formattedMessage,
senderUserId,
{
type: 'm.room.message',
content: {
msgtype: 'm.text',
body: rawMessage,
format: 'org.matrix.custom.html',
formatted_body: formattedMessage,
'm.relates_to': !latestThreadEventId
? {
rel_type: 'm.thread',
event_id: threadRootEventId,
is_falling_back: true,
}
: {
rel_type: 'm.thread',
event_id: threadRootEventId,
is_falling_back: true,
'm.in_reply_to': { event_id: latestThreadEventId },
},
},
room_id: roomId,
auth_events: [],
depth: 0,
prev_events: [],
origin_server_ts: Date.now(),
sender: senderUserId,
threadEventId: threadRootEventId,
latestThreadEventId: latestThreadEventId,
},
roomVersion,
);

await this.stateService.handlePdu(event);
if (event.rejected) {
throw new Error(event.rejectReason);
}

void this.federationService.sendEventToAllServersInRoom(event);

return event;
}

/**
* @deprecated Use sendMessage and threadEventId/replyToEventId instead
*/
async sendReplyToInsideThreadMessage(
roomId: RoomID,
rawMessage: string,
Expand All @@ -249,40 +272,16 @@ export class MessageService {
);
}

const event = await this.stateService.buildEvent<'m.room.message'>(
return this.sendMessage(
roomId,
rawMessage,
formattedMessage,
senderUserId,
{
type: 'm.room.message',
content: {
msgtype: 'm.text',
body: rawMessage,
format: 'org.matrix.custom.html',
formatted_body: formattedMessage,
'm.relates_to': {
rel_type: 'm.thread',
event_id: threadRootEventId,
'm.in_reply_to': {
event_id: eventToReplyTo,
},
},
},
room_id: roomId,
auth_events: [],
depth: 0,
prev_events: [],
origin_server_ts: Date.now(),
sender: senderUserId,
threadEventId: threadRootEventId,
replyToEventId: eventToReplyTo,
},
roomVersion,
);

await this.stateService.handlePdu(event);
if (event.rejected) {
throw new Error(event.rejectReason);
}

void this.federationService.sendEventToAllServersInRoom(event);

return event;
}

async sendReaction(
Expand Down