feat: add improved errors messaging #305
Conversation
|
Warning Rate limit exceeded@sampaiodiego has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 20 minutes and 20 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (4)
WalkthroughAdds a FederationValidationService and FederationValidationError, exposes them and extractDomainFromId from the package index, wires the validation service into FederationSDK, InviteService, and RoomService, adds federation.validation config schema, and makes checkServerAcl non-private. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant SDK as FederationSDK
participant Inv as InviteService
participant Val as FederationValidationService
participant Auth as EventAuthorizationService
participant Req as FederationRequestService
participant State as StateService
Client->>SDK: inviteUserToRoom(userId, roomId)
SDK->>Inv: inviteUserToRoom(userId, roomId)
Inv->>Val: validateOutboundInvite(userId, roomId)
rect rgb(220,235,250)
Note over Val: Policy Check
Val->>State: getRoomLatest(roomId)
Val->>Auth: checkServerAcl(aclEvent, domain)
end
rect rgb(220,235,250)
Note over Val: Network Check
Val->>Req: GET /.well-known/...
Req-->>Val: reachable / timeout
end
rect rgb(220,235,250)
Note over Val: Protocol Check
Val->>Req: GET /_matrix/profile or user endpoint
Req-->>Val: user found / not found
end
alt Validation Success
Val-->>Inv: OK
Inv->>Inv: proceed with invite flow
else Validation Failure
Val-->>Inv: FederationValidationError
Inv-->>Client: Error response
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
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
feat/errors-messaging
branch
2 times, most recently
from
997ac81 to
0899cb7
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #305 +/- ##
========================================
Coverage 53.14% 53.15%
========================================
Files 96 97 +1
Lines 12798 12942 +144
========================================
+ Hits 6802 6879 +77
- Misses 5996 6063 +67 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
packages/federation-sdk/src/services/federation-validation.service.ts (2)
72-89: Consider logging the underlying error for troubleshooting.The catch block (line 82) discards the original error details, making it difficult to diagnose why a domain is unreachable (network timeout, DNS failure, TLS error, etc.).
Add logging to preserve diagnostic information:
- } catch (_error) { + } catch (error) { + // Log for debugging while maintaining user-friendly message + console.debug('Domain reachability check failed:', { domain, error }); throw new FederationValidationError( 'CONNECTION_FAILED', 'Connection Failed. The server domain could not be reached or does not support federation.', 502, ); }
91-112: Consider logging the underlying error for troubleshooting.Similar to
checkDomainReachable, the catch block (line 105) discards error details that could help diagnose issues (user not found vs. server error vs. timeout).Add logging to preserve diagnostic information:
- } catch (_error) { + } catch (error) { + // Log for debugging while maintaining user-friendly message + console.debug('User existence check failed:', { userId, domain, error }); throw new FederationValidationError( 'USER_NOT_FOUND', 'Invitation blocked. The specified user couldn't be found on their homeserver.', 502, ); }
📜 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 (7)
packages/federation-sdk/src/index.ts(2 hunks)packages/federation-sdk/src/sdk.ts(3 hunks)packages/federation-sdk/src/services/config.service.ts(2 hunks)packages/federation-sdk/src/services/event-authorization.service.ts(1 hunks)packages/federation-sdk/src/services/federation-validation.service.ts(1 hunks)packages/federation-sdk/src/services/invite.service.ts(3 hunks)packages/federation-sdk/src/services/room.service.ts(3 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-10-10T22:18:31.655Z
Learnt from: sampaiodiego
Repo: RocketChat/homeserver PR: 224
File: packages/federation-sdk/src/services/event-authorization.service.ts:261-268
Timestamp: 2025-10-10T22:18:31.655Z
Learning: In packages/federation-sdk/src/services/state.service.ts, the method `StateService.getLatestRoomState(roomId: string)` has return type `Promise<State>` and never returns undefined. If the state is not found, it throws an error with message "No state found for room ${roomId}" instead of returning undefined.
Applied to files:
packages/federation-sdk/src/services/room.service.ts
📚 Learning: 2025-09-14T13:34:12.448Z
Learnt from: ricardogarim
Repo: RocketChat/homeserver PR: 184
File: packages/federation-sdk/src/services/event-authorization.service.ts:213-223
Timestamp: 2025-09-14T13:34:12.448Z
Learning: ACL events (m.room.server_acl) are not yet implemented in the homeserver codebase, so ACL-related code currently handles the simpler case where no ACL events exist in room state.
Applied to files:
packages/federation-sdk/src/services/event-authorization.service.ts
🧬 Code graph analysis (1)
packages/federation-sdk/src/services/invite.service.ts (2)
packages/federation-sdk/src/index.ts (1)
FederationValidationService(66-66)packages/room/src/manager/event-wrapper.ts (1)
roomId(101-103)
🔇 Additional comments (8)
packages/federation-sdk/src/services/config.service.ts (1)
38-100: LGTM! Configuration schema is well-structured.The federation validation timeouts are properly configured with sensible defaults (5s network, 10s user check) and minimum bounds (1000ms). The optional validation block enables gradual adoption.
Note: Ensure existing configuration files include at least an empty
federation: {}block to avoid schema validation errors, since thefederationproperty is required (not marked optional) in the interface and schema.packages/federation-sdk/src/index.ts (1)
65-68: LGTM! Public API exports are appropriate.The new exports for
FederationValidationService,FederationValidationError, andextractDomainFromIdproperly expose the validation functionality to consumers.Also applies to: 142-142
packages/federation-sdk/src/services/event-authorization.service.ts (1)
213-213: LGTM! Visibility broadening is appropriate.Changing
checkServerAclfrom private to public enablesFederationValidationServiceto perform ACL checks during outbound validation, aligning with the pre-federation validation requirements.Based on learnings, ACL events are not yet fully implemented in the homeserver codebase, so the simpler case handling here is expected.
packages/federation-sdk/src/services/room.service.ts (1)
40-40: LGTM! Dependency injection is properly implemented.The
FederationValidationServiceis correctly imported and injected intoRoomService.Also applies to: 67-67
packages/federation-sdk/src/sdk.ts (1)
10-10: LGTM! SDK wrapper follows established patterns.The
validateOutboundUsermethod properly delegates toFederationValidationServiceand uses the same wrapper pattern as other SDK methods.Also applies to: 41-41, 208-214
packages/federation-sdk/src/services/invite.service.ts (1)
17-17: LGTM! Validation is properly positioned.The
validateOutboundInvitecall is well-placed to fail fast before processing the invite (either locally or remotely), preventing invalid invites from being persisted.Also applies to: 48-48, 107-110
packages/federation-sdk/src/services/federation-validation.service.ts (2)
114-125: LGTM! Timeout helper is correctly implemented.The
withTimeoutmethod properly races the operation against a timeout. Note that JavaScript Promises don't support native cancellation, so the underlying operation continues after timeout (this is expected Promise behavior, not a bug).For HTTP requests, this is acceptable behavior since most client libraries handle aborted connections gracefully.
33-44: No action required—error handling is already properly implemented.The
extractDomainFromIdfunction already validates the identifier format and throws a clear error ("Invalid identifier {identifier}, no domain found") when the domain cannot be extracted. The error propagates to the calling validation methods, causing validation to fail gracefully as intended.Likely an incorrect or invalid review comment.
| private async checkRoomAcl(roomId: RoomID, domain: string): Promise<void> { | ||
| try { | ||
| const state = await this.stateService.getLatestRoomState(roomId); | ||
| const aclEvent = state.get('m.room.server_acl:'); | ||
| if (!aclEvent || !aclEvent.isServerAclEvent()) { | ||
| return; | ||
| } | ||
|
|
||
| const isAllowed = await this.eventAuthorizationService.checkServerAcl( | ||
| aclEvent, | ||
| domain, | ||
| ); | ||
| if (!isAllowed) { | ||
| throw new FederationValidationError( | ||
| 'POLICY_DENIED', | ||
| "Action Blocked. The room's access control policy blocks communication with this domain.", | ||
| 403, | ||
| ); | ||
| } | ||
| } catch (error) { | ||
| if (error instanceof FederationValidationError) { | ||
| throw error; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Critical: Fix the error handling in ACL check.
The catch block (lines 65-69) silently swallows non-FederationValidationError exceptions, causing the ACL check to fail open. If getLatestRoomState or checkServerAcl throw unexpected errors, the validation succeeds inappropriately.
This is a security risk: ACL checks must fail closed. Any error should block the operation.
Apply this diff to fail closed on errors:
private async checkRoomAcl(roomId: RoomID, domain: string): Promise<void> {
try {
const state = await this.stateService.getLatestRoomState(roomId);
const aclEvent = state.get('m.room.server_acl:');
if (!aclEvent || !aclEvent.isServerAclEvent()) {
return;
}
const isAllowed = await this.eventAuthorizationService.checkServerAcl(
aclEvent,
domain,
);
if (!isAllowed) {
throw new FederationValidationError(
'POLICY_DENIED',
"Action Blocked. The room's access control policy blocks communication with this domain.",
403,
);
}
} catch (error) {
if (error instanceof FederationValidationError) {
throw error;
}
+ // Fail closed on unexpected errors
+ throw new FederationValidationError(
+ 'POLICY_DENIED',
+ "Action Blocked. Unable to verify room access policy.",
+ 403,
+ );
}
}📝 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.
| private async checkRoomAcl(roomId: RoomID, domain: string): Promise<void> { | |
| try { | |
| const state = await this.stateService.getLatestRoomState(roomId); | |
| const aclEvent = state.get('m.room.server_acl:'); | |
| if (!aclEvent || !aclEvent.isServerAclEvent()) { | |
| return; | |
| } | |
| const isAllowed = await this.eventAuthorizationService.checkServerAcl( | |
| aclEvent, | |
| domain, | |
| ); | |
| if (!isAllowed) { | |
| throw new FederationValidationError( | |
| 'POLICY_DENIED', | |
| "Action Blocked. The room's access control policy blocks communication with this domain.", | |
| 403, | |
| ); | |
| } | |
| } catch (error) { | |
| if (error instanceof FederationValidationError) { | |
| throw error; | |
| } | |
| } | |
| } | |
| private async checkRoomAcl(roomId: RoomID, domain: string): Promise<void> { | |
| try { | |
| const state = await this.stateService.getLatestRoomState(roomId); | |
| const aclEvent = state.get('m.room.server_acl:'); | |
| if (!aclEvent || !aclEvent.isServerAclEvent()) { | |
| return; | |
| } | |
| const isAllowed = await this.eventAuthorizationService.checkServerAcl( | |
| aclEvent, | |
| domain, | |
| ); | |
| if (!isAllowed) { | |
| throw new FederationValidationError( | |
| 'POLICY_DENIED', | |
| "Action Blocked. The room's access control policy blocks communication with this domain.", | |
| 403, | |
| ); | |
| } | |
| } catch (error) { | |
| if (error instanceof FederationValidationError) { | |
| throw error; | |
| } | |
| // Fail closed on unexpected errors | |
| throw new FederationValidationError( | |
| 'POLICY_DENIED', | |
| "Action Blocked. Unable to verify room access policy.", | |
| 403, | |
| ); | |
| } | |
| } |
🤖 Prompt for AI Agents
In packages/federation-sdk/src/services/federation-validation.service.ts around
lines 46 to 70, the current catch block swallows unexpected errors and lets ACL
checks fail open; change it so any caught non-FederationValidationError causes
the check to fail closed by throwing a FederationValidationError. Specifically,
keep rethrowing FederationValidationError instances as-is, but for any other
error throw a new FederationValidationError('POLICY_DENIED', "Action Blocked.
The room's access control policy blocks communication with this domain." , 403)
— optionally include the original error message/details in the new error for
diagnostics.
| if (isExternalUser) { | ||
| await this.federationValidationService.validateOutboundInvite( | ||
| targetUserId, | ||
| roomCreateEvent.roomId, | ||
| ); | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
Consider the room cleanup implications.
The validation occurs after the DM room has been fully created and persisted (lines 1406-1522). If validateOutboundInvite fails, the room remains in the database with only the creator as a member.
This design trade-off exists because the ACL check requires room state. However, it could lead to orphaned single-member rooms accumulating over time. Consider:
- Whether orphaned rooms are acceptable for your use case
- Adding cleanup logic to remove the room if validation fails
- Documenting this behavior
🏁 Script executed:
#!/bin/bash
# Check if there's any cleanup logic for failed room creation elsewhere
rg -nP --type=ts -C3 'createDirectMessageRoom.*catch|try.*createDirectMessageRoom' Length of output: 0
🏁 Script executed:
#!/bin/bash
# Find the createDirectMessageRoom method implementation
ast-grep --pattern $'$$$createDirectMessageRoom($$$) {
$$$
}'Length of output: 0
🏁 Script executed:
#!/bin/bash
# Search for validateOutboundInvite to understand what it does
rg -nP --type=ts -A10 'validateOutboundInvite\s*\(' -g '!node_modules'Length of output: 3203
🏁 Script executed:
#!/bin/bash
# Check the broader createDirectMessageRoom implementation for transaction or error handling
rg -nP --type=ts -B5 -A30 'createDirectMessageRoom.*\{' packages/federation-sdk/src/services/room.service.ts | head -80Length of output: 0
🏁 Script executed:
#!/bin/bash
# Find who calls createDirectMessageRoom
rg -nP --type=ts 'createDirectMessageRoom\s*\(' -B3 -A3 -g '!node_modules' | grep -v testLength of output: 2735
🏁 Script executed:
#!/bin/bash
# Check what errors checkDomainReachable and checkUserExists can throw
rg -nP --type=ts -A15 'private async checkDomainReachable\s*\(|private async checkUserExists\s*\(' packages/federation-sdk/src/services/federation-validation.service.tsLength of output: 1304
🏁 Script executed:
#!/bin/bash
# Check if the entire createDirectMessageRoom is wrapped in a transaction or has error handling
rg -nP --type=ts -B10 'isExternalUser' packages/federation-sdk/src/services/room.service.ts | grep -A10 -B10 'isExternalUser.*{'Length of output: 290
🏁 Script executed:
#!/bin/bash
# Get the full try-catch block from direct-message.controller.ts
rg -nP --type=ts -B5 -A20 'createDirectMessageRoom' packages/homeserver/src/controllers/internal/direct-message.controller.tsLength of output: 721
🏁 Script executed:
#!/bin/bash
# Check what type of error response is returned
rg -nP --type=ts -A30 'catch.*error' packages/homeserver/src/controllers/internal/direct-message.controller.tsLength of output: 701
Add cleanup logic or validate external user before room creation.
The concern is confirmed: if validateOutboundInvite fails (e.g., domain unreachable, user not found), the DM room persists in the database with only the creator as a member. The controller's error handler (direct-message.controller.ts:38-44) returns an error response but does not remove the orphaned room.
While the validation must occur after room creation (because the ACL check requires room state), consider:
- Moving domain/user validation checks before room creation, keeping only the ACL check after
- Adding transaction rollback or explicit cleanup if validation fails
- Documenting this behavior if orphaned rooms are acceptable
ricardogarim
force-pushed
the
feat/errors-messaging
branch
from
0899cb7 to
ce82e70
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/federation-sdk/src/services/config.service.ts (1)
85-100: Schema must match interface iffederationbecomes optional.The schema currently requires the
federationproperty (no.optional()modifier), which is consistent with the interface. However, if the interface is updated to makefederationoptional (as suggested above), the schema must also be updated accordingly.If you make
federationoptional in the interface, apply this diff to match the schema:- federation: z.object({ + federation: z.object({ validation: z .object({ networkCheckTimeoutMs: z .number() .int() .min(1000, 'Network check timeout must be at least 1000ms') .default(5000), userCheckTimeoutMs: z .number() .int() .min(1000, 'User check timeout must be at least 1000ms') .default(10000), }) .optional(), - }), + }).optional(),Otherwise, the timeout constraints and error messages look good.
📜 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 (7)
packages/federation-sdk/src/index.ts(2 hunks)packages/federation-sdk/src/sdk.ts(3 hunks)packages/federation-sdk/src/services/config.service.ts(2 hunks)packages/federation-sdk/src/services/event-authorization.service.ts(1 hunks)packages/federation-sdk/src/services/federation-validation.service.ts(1 hunks)packages/federation-sdk/src/services/invite.service.ts(3 hunks)packages/federation-sdk/src/services/room.service.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/federation-sdk/src/index.ts
- packages/federation-sdk/src/services/invite.service.ts
- packages/federation-sdk/src/services/federation-validation.service.ts
- packages/federation-sdk/src/sdk.ts
- packages/federation-sdk/src/services/event-authorization.service.ts
- packages/federation-sdk/src/services/room.service.ts
🔇 Additional comments (1)
packages/federation-sdk/src/services/config.service.ts (1)
38-43:federationproperty is now required and will break existing production configurations without it.The
setConfig()method validates configuration usingAppConfigSchema.parse()in production (whenNODE_ENV !== 'test'). The test at lines 74-105 explicitly validates that configurations missing thefederationproperty throw an error matching/federation|required/i. Both the interface definition (lines 38-43) and Zod schema (lines 85-100) makefederationrequired.Tests bypass this validation by checking
NODE_ENV === 'test', which is why test cases withoutfederationdon't fail.Any existing deployment with a configuration missing the
federationproperty will fail at runtime. Confirm whether:
- All existing configurations already include
federation, or- This breaking change is intentional with a documented migration path
If backward compatibility is needed, make
federationoptional:- federation: { + federation?: { validation?: { networkCheckTimeoutMs?: number; userCheckTimeoutMs?: number; }; };
ricardogarim
force-pushed
the
feat/errors-messaging
branch
from
e3c120d to
7e52690
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/federation-sdk/src/services/federation-validation.service.ts (2)
64-80: Consider logging original errors for debugging.The catch block swallows the original error, which may contain valuable debugging information (e.g., network timeout, DNS failure, HTTP status codes). While the user-facing message is appropriately generic, logging the original error would aid troubleshooting.
🔎 Suggested enhancement
Add a logger and log the original error:
+import { createLogger } from '@rocket.chat/federation-core'; + @singleton() export class FederationValidationService { + private readonly logger = createLogger('FederationValidationService'); + constructor( private readonly configService: ConfigService, private readonly federationRequestService: FederationRequestService, private readonly stateService: StateService, private readonly eventAuthorizationService: EventAuthorizationService, ) {}Then in
checkDomainReachable:- } catch (_error) { + } catch (error) { + this.logger.warn({ + msg: 'Domain reachability check failed', + domain, + error, + }); throw new FederationValidationError(
82-102: Consider logging original errors for debugging.Similar to
checkDomainReachable, this method swallows the original error. Logging would help diagnose whether failures are due to network issues, 404 responses, or other causes.🔎 Suggested enhancement
- } catch (_error) { + } catch (error) { + this.logger.warn({ + msg: 'User existence check failed', + userId, + domain, + error, + }); throw new FederationValidationError(
📜 Review details
Configuration used: Organization 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 (7)
packages/federation-sdk/src/index.ts(2 hunks)packages/federation-sdk/src/sdk.ts(3 hunks)packages/federation-sdk/src/services/config.service.ts(2 hunks)packages/federation-sdk/src/services/event-authorization.service.ts(1 hunks)packages/federation-sdk/src/services/federation-validation.service.ts(1 hunks)packages/federation-sdk/src/services/invite.service.ts(3 hunks)packages/federation-sdk/src/services/room.service.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/federation-sdk/src/services/room.service.ts
- packages/federation-sdk/src/services/invite.service.ts
- packages/federation-sdk/src/services/config.service.ts
- packages/federation-sdk/src/sdk.ts
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-10-10T22:18:31.655Z
Learnt from: sampaiodiego
Repo: RocketChat/homeserver PR: 224
File: packages/federation-sdk/src/services/event-authorization.service.ts:261-268
Timestamp: 2025-10-10T22:18:31.655Z
Learning: In packages/federation-sdk/src/services/state.service.ts, the method `StateService.getLatestRoomState(roomId: string)` has return type `Promise<State>` and never returns undefined. If the state is not found, it throws an error with message "No state found for room ${roomId}" instead of returning undefined.
Applied to files:
packages/federation-sdk/src/services/federation-validation.service.ts
📚 Learning: 2025-09-14T13:34:12.448Z
Learnt from: ricardogarim
Repo: RocketChat/homeserver PR: 184
File: packages/federation-sdk/src/services/event-authorization.service.ts:213-223
Timestamp: 2025-09-14T13:34:12.448Z
Learning: ACL events (m.room.server_acl) are not yet implemented in the homeserver codebase, so ACL-related code currently handles the simpler case where no ACL events exist in room state.
Applied to files:
packages/federation-sdk/src/services/federation-validation.service.tspackages/federation-sdk/src/services/event-authorization.service.ts
🧬 Code graph analysis (1)
packages/federation-sdk/src/services/federation-validation.service.ts (3)
packages/federation-sdk/src/index.ts (2)
FederationValidationError(68-68)FederationValidationService(67-67)packages/room/src/manager/event-wrapper.ts (2)
roomId(111-113)reject(458-462)packages/federation-sdk/src/specs/federation-api.ts (1)
FederationEndpoints(24-71)
🔇 Additional comments (7)
packages/federation-sdk/src/services/event-authorization.service.ts (1)
214-214: LGTM! Visibility change enables external validation.Making
checkServerAclpublic allowsFederationValidationServiceto reuse ACL enforcement logic, supporting the outbound validation workflow introduced in this PR.packages/federation-sdk/src/index.ts (2)
66-69: LGTM! Public API expanded appropriately.Exposing
FederationValidationServiceandFederationValidationErrorenables consumers to perform pre-federation validation and handle validation errors consistently.
143-143: LGTM! Utility export supports validation workflows.Exposing
extractDomainFromIdprovides a reusable utility for domain extraction during outbound validation.packages/federation-sdk/src/services/federation-validation.service.ts (4)
10-21: LGTM! Clean error class design.The
FederationValidationErrorprovides consistent error handling with clear error codes and user-friendly messages. Theerrorfield derivation fromcodeensures consistency.
32-36: LGTM! Clear validation sequence.The
validateOutboundUsermethod follows a logical flow: domain extraction → reachability check → user existence check. This fail-fast approach aligns with the PR objectives.
38-43: LGTM! Prioritizes ACL checks appropriately.The
validateOutboundInvitemethod correctly checks room ACL before performing network calls, failing fast on policy violations and avoiding unnecessary external requests.
104-115: LGTM! Standard timeout implementation.The
withTimeouthelper correctly races the promise against a timeout, providing predictable timeout behavior for validation checks.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/federation-sdk/src/services/room.service.spec.ts (1)
46-56: Fix typo and consider type-safe mock pattern.The validation bypass is appropriate for testing core RoomService functionality in isolation. However, consider these improvements:
- Typo: "dont" → "don't" on line 46
- Type safety: The
as unknown as FederationValidationServiceassertion bypasses type checking. If the interface evolves, this mock won't reflect changes. Consider creating a proper mock class or using a type-safe approach:// Type-safe alternative const mockValidationService: Pick<FederationValidationService, 'validateOutboundUser' | 'validateOutboundInvite'> = { async validateOutboundUser() { return true; }, async validateOutboundInvite() { return true; }, }; container.register(FederationValidationService, { useValue: mockValidationService as FederationValidationService, });Note: The Codecov report indicates
federation-validation.service.tshas only 17.27% coverage. While bypassing validation here is correct for RoomService tests, ensure the validation logic itself has dedicated test coverage.
📜 Review details
Configuration used: Organization 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)
packages/federation-sdk/src/services/invite.service.ts(3 hunks)packages/federation-sdk/src/services/room.service.spec.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/federation-sdk/src/services/invite.service.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-10T22:18:31.655Z
Learnt from: sampaiodiego
Repo: RocketChat/homeserver PR: 224
File: packages/federation-sdk/src/services/event-authorization.service.ts:261-268
Timestamp: 2025-10-10T22:18:31.655Z
Learning: In packages/federation-sdk/src/services/state.service.ts, the method `StateService.getLatestRoomState(roomId: string)` has return type `Promise<State>` and never returns undefined. If the state is not found, it throws an error with message "No state found for room ${roomId}" instead of returning undefined.
Applied to files:
packages/federation-sdk/src/services/room.service.spec.ts
🧬 Code graph analysis (1)
packages/federation-sdk/src/services/room.service.spec.ts (1)
packages/federation-sdk/src/index.ts (1)
FederationValidationService(67-67)
🔇 Additional comments (2)
packages/federation-sdk/src/services/room.service.spec.ts (2)
11-11: LGTM! Import aligns with public API expansion.The import of
FederationValidationServiceis consistent with its exposure from the package root index, as needed for the test mock registration.
31-34: LGTM! Simplified initialization.Removing the explicit
emitter: undefinedsimplifies the init call, which is appropriate if the parameter is optional.
sampaiodiego
force-pushed
the
feat/errors-messaging
branch
from
bd88cb4 to
5fe88e4
Compare
sampaiodiego
force-pushed
the
feat/errors-messaging
branch
from
5fe88e4 to
0c34985
Compare
Summary by CodeRabbit
New Features
Configuration
Bug Fixes / Reliability
✏️ Tip: You can customize this high-level summary in your review settings.