chore: allow using v4 over v6 for resolved dns results#237
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughAdded an environment-configurable DNS lookup order to the federation resolver (default: Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Env as Environment
participant App as App Startup
participant Res as _Resolver (singleton)
participant DNS as DNS
note over App,Res: Initialization
App->>Env: read HOMESERVER_CONFIG_DNS_LOOKUP_ORDER
App->>Res: new _Resolver()
Res->>Res: validate env ∈ {ipv4first, ipv6first, verbatim}
Res->>Res: set lookupOrder (default: ipv6first)
note over App,Res: Runtime resolution
App->>Res: resolve(host)
Res->>DNS: dns.lookup(host, { all: true, order: lookupOrder })
DNS-->>Res: addresses ordered per lookupOrder
Res-->>App: resolved addresses
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (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. 🧪 Early access (Sonnet 4.5): enabledWe are currently testing the Sonnet 4.5 model, which is expected to improve code review quality. However, this model may lead to increased noise levels in the review comments. Please disable the early access features if the noise level causes any inconvenience. Note:
Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #237 +/- ##
==========================================
+ Coverage 81.76% 81.86% +0.09%
==========================================
Files 63 63
Lines 4701 4709 +8
==========================================
+ Hits 3844 3855 +11
+ Misses 857 854 -3 ☔ 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: 0
🧹 Nitpick comments (1)
packages/federation-sdk/src/server-discovery/_resolver.ts (1)
19-30: Consider refactoring for improved type safety.The environment variable reading and validation logic works correctly, but the type assertion on lines 22-23 bypasses TypeScript's type checking. While the subsequent validation catches invalid values, consider using a type guard for better type safety:
- const order = process.env - .HOMESERVER_CONFIG_DNS_LOOKUP_ORDER as typeof this.lookupOrder; - if ( - order === 'ipv4first' || - order === 'ipv6first' || - order === 'verbatim' - ) { + const order = process.env.HOMESERVER_CONFIG_DNS_LOOKUP_ORDER; + if (order && ( + order === 'ipv4first' || + order === 'ipv6first' || + order === 'verbatim' + )) { this.lookupOrder = order; }This removes the type assertion and adds an explicit undefined check, making the code more explicit about its intent.
📜 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 (1)
packages/federation-sdk/src/server-discovery/_resolver.ts(3 hunks)
🔇 Additional comments (4)
packages/federation-sdk/src/server-discovery/_resolver.ts (4)
1-1: LGTM!The import of
LookupAllOptionstype properly supports the new configurable lookup order feature.
6-6: LGTM!The private member with a default of
'ipv6first'preserves existing behavior while enabling configuration.
39-39: LGTM!The method now correctly applies the configurable lookup order instead of the hardcoded value.
46-46: Approve singletonresolverexport.No other
_Resolverinstantiations or imports were found; using this sharedresolverinstance ensures consistent behavior across the application.
3 participants
Summary by CodeRabbit