regression(federation): upload images on threads #37201
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 |
|
ggazzo
force-pushed
the
regression/upload-fed
branch
from
7a55879 to
dffa7ae
Compare
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughUnifies thread and quote handling for Matrix federation messages, updating control flow in FederationMatrix and events/message to compute reply targets upfront. Adjusts media handling to pass a thread object instead of tmid. Bumps @rocket.chat/federation-sdk from 0.1.25 to 0.1.26 in two package.json files. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Client
participant FederationMatrix
participant EventsMessage
participant MatrixServer as Matrix Server
User->>Client: Send message (text/file) with thread/quote
Client->>FederationMatrix: handleTextMessage/handleFileMessage(payload)
rect rgba(230,245,255,0.6)
note right of FederationMatrix: Resolve reply context
FederationMatrix->>FederationMatrix: getQuoteMessage(...) => quoteMessage?
FederationMatrix->>FederationMatrix: handleThreadedMessage(thread?, quote?)<br/>=> { threadEventId, replyToEventId | latestThreadEventId }?
FederationMatrix->>FederationMatrix: Compute eventToReplyTo (unified)
end
alt File message
FederationMatrix->>EventsMessage: handleMediaMessage(..., thread?)
EventsMessage-->>FederationMatrix: { ..., attachments, thread? }
else Text message
FederationMatrix->>MatrixServer: send message with eventToReplyTo/threadEventId
end
MatrixServer-->>FederationMatrix: federation_event_id
FederationMatrix-->>Client: success or error
%% Error paths
rect rgba(255,235,230,0.6)
note over FederationMatrix: Throws if thread root event ID missing<br/>or no event to reply to when required
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
ricardogarim
left a comment
ricardogarim
left a comment
There was a problem hiding this comment.
code lgtm! however, during testing i noticed a case where files sent from remote threads appear as regular messages on the RC side. all other use cases worked as expected.
this is not what I was trying to solve, but since you raised it... solved |
There was a problem hiding this comment.
Pull Request Overview
This PR addresses a regression in federation where images were not being properly uploaded on threads. The change consolidates message processing logic to ensure proper handling of both threaded and quoted messages.
- Updated federation SDK version from 0.1.23 to 0.1.24
- Refactored message handling methods to return reply metadata instead of directly sending messages
- Changed thread parameter structure from simple string to object with tmid and tshow properties
Reviewed Changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/core-services/package.json | Updated federation SDK dependency to version 0.1.24 |
| ee/packages/federation-matrix/package.json | Updated federation SDK dependency to version 0.1.24 |
| ee/packages/federation-matrix/src/events/message.ts | Changed thread parameter from string to object structure |
| ee/packages/federation-matrix/src/FederationMatrix.ts | Refactored message handling methods to consolidate reply logic |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| parsedMessage, | ||
| userIdSchema.parse(matrixUserId), | ||
| ); | ||
| throw new Error('Thread root event ID not found'); |
There was a problem hiding this comment.
The error message should be more descriptive and include context about which thread message failed to find its root event ID.
| throw new Error('Thread root event ID not found'); | |
| throw new Error(`Thread root event ID not found for thread message ID: ${message.tmid}, message ID: ${message._id}, room ID: ${matrixRoomId}`); |
| : undefined; | ||
|
|
||
| if (!quoteMessageEventId && !latestThreadMessage) { | ||
| throw new Error('No event to reply to found'); |
There was a problem hiding this comment.
The error message should be more specific about what type of event was expected (quote or thread) and include the message ID for debugging.
| throw new Error('No event to reply to found'); | |
| throw new Error( | |
| `No thread or quote event to reply to found for message ID: ${message._id}` | |
| ); |
…d messages - Updated `handleThreadedMessage` and `handleQuoteMessage` methods to improve message processing. - Consolidated logic for replying to messages, ensuring proper handling of threaded and quoted messages. - Added error handling for missing event IDs and improved logging for message processing failures.
…1.26 in package.json and yarn.lock
ggazzo
force-pushed
the
regression/upload-fed
branch
from
0d16a77 to
37006c3
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: 0
♻️ Duplicate comments (1)
ee/packages/federation-matrix/src/FederationMatrix.ts (1)
439-439: Include context in the thrown error.Please carry the relevant identifiers (message
_id,tmid, room/event IDs) into this exception so production logs explain which thread failed to map. That aligns with the earlier review feedback on this spot and makes the incident actionable.- throw new Error('Thread root event ID not found'); + throw new Error( + `Thread root event ID not found (messageId=${message._id}, tmid=${message.tmid}, roomId=${matrixRoomId})`, + );
🧹 Nitpick comments (1)
ee/packages/federation-matrix/src/FederationMatrix.ts (1)
368-394: Avoid double quote resolution work.
handleThreadedMessagealready callsgetQuoteMessage, so invokinghandleQuoteMessageimmediately afterward runs the same DB lookup and formatting twice whenever a threaded reply also carries a quote attachment. Consider caching the quote payload (or threading it through) so you only compute it once per message.
📜 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 ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (4)
ee/packages/federation-matrix/package.json(1 hunks)ee/packages/federation-matrix/src/FederationMatrix.ts(3 hunks)ee/packages/federation-matrix/src/events/message.ts(3 hunks)packages/core-services/package.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- ee/packages/federation-matrix/src/events/message.ts
- ee/packages/federation-matrix/package.json
- packages/core-services/package.json
🧰 Additional context used
🧬 Code graph analysis (1)
ee/packages/federation-matrix/src/FederationMatrix.ts (2)
ee/packages/federation-matrix/src/events/message.ts (1)
message(114-410)packages/core-typings/src/IMessage/IMessage.ts (1)
IMessage(138-239)
⏰ 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)
- GitHub Check: 📦 Build Packages
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## release-7.11.0 #37201 +/- ##
==================================================
+ Coverage 65.32% 66.36% +1.04%
==================================================
Files 3141 3386 +245
Lines 111835 115618 +3783
Branches 20468 21357 +889
==================================================
+ Hits 73058 76732 +3674
+ Misses 36431 36278 -153
- Partials 2346 2608 +262
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
https://rocketchat.atlassian.net/browse/FDR-228
handleThreadedMessageandhandleQuoteMessagemethods to improve message processing.Proposed changes (including videos or screenshots)
Issue(s)
Steps to test or reproduce
Further comments
Summary by CodeRabbit
Bug Fixes
Chores