refactor(federation): make getEventBy to return PduType instead of any #37130
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 |
|
WalkthroughThe FederationMatrix.getEventById method was simplified to directly call the homeserver event service without local guards or logging. The IFederationMatrixService typing for getEventById was changed to return Promise<EventStore | null> and EventStore was imported from @rocket.chat/federation-sdk. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller
participant FederationMatrix
participant HomeserverEvent as HomeserverServices.event
rect rgb(240,248,255)
Note over Caller,FederationMatrix: Simplified delegation flow
Caller->>FederationMatrix: getEventById(eventId)
FederationMatrix->>HomeserverEvent: getEventById(eventId)
HomeserverEvent-->>FederationMatrix: EventStore | null (or throws)
FederationMatrix-->>Caller: EventStore | null (propagates errors)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
⏰ 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)
🔇 Additional comments (1)
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
force-pushed
the
refactor/getEventById-types
branch
from
f1772e0 to
583f455
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: 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 (3)
ee/packages/federation-matrix/src/FederationMatrix.ts(1 hunks)packages/core-services/package.json(1 hunks)packages/core-services/src/types/IFederationMatrixService.ts(2 hunks)
🔇 Additional comments (2)
packages/core-services/src/types/IFederationMatrixService.ts (1)
2-2: Confirm compatibility of getEventById callers
Existing guards againstnulland subsequent type checks onevent.typeensure no breaking changes with the newEventStore<PduForType<PduType>>return type.packages/core-services/package.json (1)
38-38: @rocket.chat/[email protected] is published; no changes needed.
| async getEventById(eventId: EventID) { | ||
| return this.homeserverServices.event.getEventById(eventId); | ||
| } |
There was a problem hiding this comment.
Add null check for homeserverServices and explicit return type annotation.
The method now directly delegates to this.homeserverServices.event.getEventById(eventId) without checking if homeserverServices is initialized. Other methods in this class (e.g., createRoom at lines 111-114, sendMessage at lines 444-446) guard against missing homeserverServices before use. Additionally, the return type is no longer explicitly annotated, reducing type safety.
If homeserverServices is unavailable (e.g., when the homeserver module fails to initialize at line 106), calling this method will result in a null-pointer exception instead of returning null gracefully.
Apply this diff to add a null check and explicit return type:
-async getEventById(eventId: EventID) {
- return this.homeserverServices.event.getEventById(eventId);
+async getEventById(eventId: EventID): Promise<EventStore<PduForType<PduType>> | null> {
+ if (!this.homeserverServices) {
+ this.logger.warn('Homeserver services not available, cannot retrieve event');
+ return null;
+ }
+ return this.homeserverServices.event.getEventById(eventId);
}Additionally, verify that all callers of getEventById handle potential exceptions if you choose not to add the null check:
#!/bin/bash
# Find all calls to getEventById to verify error handling
rg -nP --type=ts -C5 '\.getEventById\s*\('🤖 Prompt for AI Agents
In ee/packages/federation-matrix/src/FederationMatrix.ts around lines 649 to
651, the async getEventById method lacks an explicit return type and does not
check that this.homeserverServices is initialized; add an explicit return type
(e.g., Promise<Event|null> or the appropriate Event type union) and guard the
call with a null-check that returns null if homeserverServices is absent,
otherwise delegate to this.homeserverServices.event.getEventById(eventId);
ensure the method remains async and update any callers only if you decide not to
return null so they still handle the potential null result.
ggazzo
force-pushed
the
refactor/getEventById-types
branch
from
583f455 to
ecaba8b
Compare
ggazzo
force-pushed
the
refactor/getEventById-types
branch
from
ecaba8b to
905a9a0
Compare
…n message handler
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## release-7.11.0 #37130 +/- ##
==================================================
- Coverage 67.46% 67.44% -0.02%
==================================================
Files 3329 3329
Lines 113381 113381
Branches 20579 20579
==================================================
- Hits 76490 76472 -18
- Misses 34291 34314 +23
+ Partials 2600 2595 -5
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
#37130) Co-authored-by: Guilherme Gazzo <[email protected]>
Proposed changes (including videos or screenshots)
Issue(s)
Steps to test or reproduce
Further comments
Summary by CodeRabbit
No user-facing changes are expected in this release; these updates improve internal consistency and reliability and prepare the system for future enhancements.