fix(federation): empty thread id leads to wrong reply #37023
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 |
|
WalkthroughReplaces a local Matrix file-type alias with the shared Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant HS as Homeserver
participant Handler as message() handler
participant Store as Message Store
participant Room as Room Lookup
HS->>Handler: receive Matrix event (data)
Handler->>Handler: const { content } = data\nrelation = content['m.relates_to']
alt relation.rel_type == "m.thread"
Handler->>Handler: threadRootEventId = relation.event_id
Handler->>Store: getThreadMessageId(threadRootEventId) %% async lookup
end
opt reply present
Handler->>Handler: quoteMessageEventId = relation['m.in_reply_to']?.event_id
Handler->>Store: resolve quoted/original message by quoteMessageEventId
end
alt relation.rel_type == "m.replace" (edit)
Handler->>Store: originalEventId = relation.event_id
Handler->>Handler: newContent = content['m.new_content']
Handler->>Store: update original message with newContent
else new/regular message
Handler->>Room: optional room metadata fetch
Handler->>Store: create/save new message (media handling typed)
end
Handler-->>HS: ack/complete
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ 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). (3)
Comment |
ggazzo
changed the title
ggazzo
force-pushed
the
fix/missing-typings
branch
from
9ebbf1c to
1d1915c
Compare
ggazzo
force-pushed
the
fix/missing-typings
branch
from
1d1915c to
a856d76
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
ee/packages/federation-matrix/src/FederationMatrix.ts (2)
33-38: Harden the mapping against accidental mutation.Make
fileTypesreadonly to avoid accidental writes at runtime.-export const fileTypes: Record<string, MessageType> = { +export const fileTypes: Readonly<Record<string, MessageType>> = { image: 'm.image', video: 'm.video', audio: 'm.audio', file: 'm.file', };
99-99: Env flag always true due to|| true.
process.env.MEDIA_ENABLE_THUMBNAILS === 'true' || truealways evaluates totrue. Respect the env var and default to true only when unset.- enableThumbnails: process.env.MEDIA_ENABLE_THUMBNAILS === 'true' || true, + enableThumbnails: + process.env.MEDIA_ENABLE_THUMBNAILS === undefined + ? true + : process.env.MEDIA_ENABLE_THUMBNAILS === 'true',ee/packages/federation-matrix/src/events/message.ts (2)
221-224: Guard media filename to avoid crashes whenbodyis absent.Some media events can miss
body.handleMediaMessageusesencodeURIComponent(fileName), which will throw onundefined.- const messageBody = content?.body?.toString(); + const messageBody = content?.body?.toString() ?? content?.filename ?? 'file';
267-274: Fallback toroom.fnamewhenroom.nameis absent.Federated rooms/DMs may lack
name. Useroom.name || room.fnameto avoid skipping quote formatting.- const messageToReplyToUrl = await MeteorService.getMessageURLToReplyTo( - room.t as string, - room._id, - room.name, - originalMessage._id, - ); + const messageToReplyToUrl = await MeteorService.getMessageURLToReplyTo( + room.t as string, + room._id, + room.name || (room as any).fname || '', + originalMessage._id, + );Apply the same change to the non‑edit quote branch.
Also applies to: 315-316
📜 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 (2)
ee/packages/federation-matrix/src/FederationMatrix.ts(2 hunks)ee/packages/federation-matrix/src/events/message.ts(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
ee/packages/federation-matrix/src/events/message.ts (2)
ee/packages/federation-matrix/src/FederationMatrix.ts (1)
fileTypes(33-38)packages/models/src/index.ts (1)
Messages(184-184)
⏰ 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). (2)
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (1)
ee/packages/federation-matrix/src/events/message.ts (1)
242-248: LGTM: centralizing relation parsing prevents empty-thread-id replies.Using a single
relationobject and short‑circuiting falsyevent_idensures empty thread IDs don’t misroute replies. This addresses the PR’s core issue.Please confirm with a test that an event carrying
rel_type: 'm.thread'andevent_id: ''results in a non-threaded message save (notmid), as intended.
| const relation = content['m.relates_to']; | ||
|
|
||
| const isThreadMessage = relation && relation.rel_type === 'm.thread'; | ||
| const threadRootEventId = isThreadMessage && relation.event_id; | ||
|
|
||
| const quoteMessageEventId = relation && 'm.in_reply_to' in relation && relation['m.in_reply_to']?.event_id; | ||
|
|
There was a problem hiding this comment.
Edited replies: also read reply target from m.new_content.m.relates_to.
In edits (m.replace), some HS place m.in_reply_to inside m.new_content.m.relates_to. Current logic only checks top‑level relation, so edited replies can lose their reply context.
- const relation = content['m.relates_to'];
+ const relation = content['m.relates_to'];
+ const newContentRelation = content['m.new_content']?.['m.relates_to'];
- const quoteMessageEventId = relation && 'm.in_reply_to' in relation && relation['m.in_reply_to']?.event_id;
+ const quoteMessageEventId =
+ relation?.['m.in_reply_to']?.event_id ??
+ newContentRelation?.['m.in_reply_to']?.event_id;Also applies to: 251-253
🤖 Prompt for AI Agents
In ee/packages/federation-matrix/src/events/message.ts around lines 240-246 (and
similarly around 251-253), the code only reads relation info from
content['m.relates_to'] so edited replies whose reply target is moved into
m.new_content.m.relates_to (during an m.replace edit) lose their reply/thread
context; update the logic to first check for
content['m.new_content']?.['m.relates_to'] and fall back to
content['m.relates_to'], then derive isThreadMessage, threadRootEventId and
quoteMessageEventId from that chosen relation object (apply the same change to
the other occurrence at lines 251-253).
a856d76 to
b480def
Compare
- Added MessageType to fileTypes for better type safety. - Simplified message handling logic by destructuring content and refining relation checks. - Improved readability and maintainability of the message processing code.
ggazzo
force-pushed
the
fix/missing-typings
branch
from
b480def to
5fb801e
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## chore/federation-backup #37023 +/- ##
========================================================
Coverage 67.33% 67.34%
========================================================
Files 3342 3342
Lines 113397 113401 +4
Branches 20704 20666 -38
========================================================
+ Hits 76361 76375 +14
+ Misses 34435 34427 -8
+ Partials 2601 2599 -2
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
- Updated fileTypes to use FileMessageType for improved type safety. - Refined getMatrixMessageType to return FileMessageType. - Adjusted message handling to ensure consistent type usage across the federation module.
Proposed changes (including videos or screenshots)
Issue(s)
Steps to test or reproduce
Further comments
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores