Skip to content

Conversation

@dhulke
Copy link
Contributor

@dhulke dhulke commented Jan 13, 2026

When inviting a user from the local server, we were doing the federation validation of the domain which isn't necessary. This broke integration tests being run locally because the domain name resolved the the container's ip and when trying to reach <container's ip>:443 that port wasn't open since in docker, we use traefik to listen on that port on a different ip. This fix addresses the change introduced in #305 but also makes a prophylactic fix by avoiding domain checks on the local server.

Summary by CodeRabbit

  • Refactor
    • Enhanced federation validation to skip unnecessary reachability checks when a domain matches the local server, improving system performance and reducing redundant network operations
    • Streamlined invite processing to apply validation checks exclusively for remote invites, allowing local invites to process directly without additional validation overhead and improving overall user experience

✏️ Tip: You can customize this high-level summary in your review settings.

@dhulke dhulke requested a review from ricardogarim January 13, 2026 12:22
@CLAassistant
Copy link

CLAassistant commented Jan 13, 2026

CLA assistant check
All committers have signed the CLA.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jan 13, 2026

Walkthrough

Optimizations to federation validation: adds a guard in domain reachability checks to skip verification for the server's own domain, and restructures invite validation to only validate remote invites while skipping validation for local invites processed directly.

Changes

Cohort / File(s) Summary
Federation Domain Reachability
packages/federation-sdk/src/services/federation-validation.service.ts
Added early return guard in checkDomainReachable when domain matches the server's own name, skipping unnecessary reachability verification for local domains.
Invite Validation Flow
packages/federation-sdk/src/services/invite.service.ts
Restructured control flow to skip outbound invite validation for local invites (processed directly) and perform validation only for remote invites after the local/server check block.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

  • feat: add improved errors messaging #305: Introduced FederationValidationService and wired validateOutboundInvite into invite and room flows; these changes directly modify the same validation functions and control flow around outbound invite handling.

Poem

🐰 A local hop needs no far-flung check,
Domain guards prevent the beckoning neck,
Local invites whisper their quick "yes,"
Remote ones validated—we pass the test! ✨

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main change: adding an optimization to skip domain reachability checks when the domain matches the local server.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix/dont-check-local-server

📜 Recent 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.

📥 Commits

Reviewing files that changed from the base of the PR and between 74e2e1f and ca0a6ba.

📒 Files selected for processing (2)
  • packages/federation-sdk/src/services/federation-validation.service.ts
  • packages/federation-sdk/src/services/invite.service.ts
🧰 Additional context used
🧬 Code graph analysis (1)
packages/federation-sdk/src/services/invite.service.ts (1)
packages/room/src/manager/event-wrapper.ts (1)
  • roomId (111-113)
🔇 Additional comments (2)
packages/federation-sdk/src/services/federation-validation.service.ts (1)

65-69: LGTM! Sensible early-return optimization.

Skipping the network reachability check for the server's own domain is correct since the local server is inherently reachable. This also aligns with the fix for the integration test failures described in the PR.

packages/federation-sdk/src/services/invite.service.ts (1)

117-120: LGTM! Correctly validates only remote invites.

Moving validateOutboundInvite after the local server check ensures validation is only performed for remote invites. Local invites handled in lines 100-115 correctly bypass this validation since they don't require federation domain reachability checks.

This change, combined with the early-return guard in checkDomainReachable, provides defense-in-depth for skipping unnecessary federation validation on local invites.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@dhulke dhulke requested review from ggazzo and sampaiodiego January 13, 2026 12:22
@codecov-commenter
Copy link

Codecov Report

❌ Patch coverage is 40.00000% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 52.02%. Comparing base (74e2e1f) to head (ca0a6ba).

Files with missing lines Patch % Lines
...-sdk/src/services/federation-validation.service.ts 0.00% 5 Missing ⚠️
...ages/federation-sdk/src/services/invite.service.ts 80.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #325      +/-   ##
==========================================
- Coverage   52.04%   52.02%   -0.02%     
==========================================
  Files          97       97              
  Lines       13153    13158       +5     
==========================================
  Hits         6845     6845              
- Misses       6308     6313       +5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link

@cubic-dev-ai cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

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

No issues found across 2 files

@ggazzo ggazzo merged commit b6fd1de into main Jan 15, 2026
4 checks passed
@ggazzo ggazzo deleted the fix/dont-check-local-server branch January 15, 2026 16:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants