feat: Add ruTorrent discovery via Zeroconf/mDNS (#75)#198
Open
ananyaaa66 wants to merge 1 commit intoCCExtractor:mainfrom
Open
feat: Add ruTorrent discovery via Zeroconf/mDNS (#75)#198ananyaaa66 wants to merge 1 commit intoCCExtractor:mainfrom
ananyaaa66 wants to merge 1 commit intoCCExtractor:mainfrom
Conversation
- Create DiscoveryService using RawDatagramSocket for mDNS queries - Implement _rutorrent_mobile._tcp service type discovery - Add 'Discover Servers' button to LoginView - Auto-fill URL from discovered server selection - No external dependencies used for mDNS to ensure simple build process Closes CCExtractor#75
📝 WalkthroughWalkthroughThis PR introduces local network discovery for ruTorrent servers using mDNS/DNS-SD protocol. A new Changes
Sequence DiagramsequenceDiagram
participant User as User
participant LoginView as Login View
participant LoginVM as Login ViewModel
participant DiscoveryService as Discovery Service
participant Network as mDNS Network
User->>LoginView: Tap "Discover Servers on Network"
LoginView->>LoginVM: discoverServers()
activate LoginVM
LoginVM->>DiscoveryService: discover(timeout: 5s)
activate DiscoveryService
DiscoveryService->>Network: Bind UDP socket & join multicast group
DiscoveryService->>Network: Send mDNS PTR query
rect rgba(100, 150, 200, 0.5)
note over Network: Listen for UDP responses until timeout
Network-->>DiscoveryService: mDNS response packets
DiscoveryService->>DiscoveryService: Parse DNS response, extract SRV/TXT records
DiscoveryService->>DiscoveryService: Deduplicate by (host, port)
end
DiscoveryService-->>LoginVM: List<DiscoveredServer>
deactivate DiscoveryService
LoginVM->>LoginVM: Update discoveredServers ValueNotifier
LoginVM-->>LoginView: notifyListeners()
deactivate LoginVM
LoginView->>LoginView: Render discovered servers in modal
LoginView-->>User: Display list of available servers
User->>LoginView: Select a server
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
lib/ui/views/login/login_view.dart (1)
50-58: RedundantsetModalState(() {})call before async operation.The first
setModalState(() {})on line 53 doesn't change any state and serves no purpose. The rebuild will happen when thethencallback invokessetModalState. However, sincemodel.isDiscoveringis aValueNotifierand the UI usesValueListenableBuilder, the UI should update automatically without needingsetModalStateat all.♻️ Simplified refresh handler
IconButton( icon: Icon(Icons.refresh), onPressed: () { - setModalState(() {}); - model.discoverServers().then((_) { - setModalState(() {}); - }); + model.discoverServers(); }, ),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ui/views/login/login_view.dart` around lines 50 - 58, Remove the redundant immediate call to setModalState in the IconButton onPressed handler: it does nothing before the async discover operation and the UI is already driven by model.isDiscovering (a ValueNotifier) and ValueListenableBuilder. Update the onPressed to simply call model.discoverServers() (or await/then the Future) and remove the first setModalState(() {}) invocation so only the async call runs (any necessary UI updates will come from model.discoverServers() updating model.isDiscovering and the ValueListenableBuilder); reference: IconButton, onPressed, model.discoverServers(), setModalState, model.isDiscovering, ValueListenableBuilder.lib/services/functional_services/discovery_service.dart (1)
176-178: Misleading comment: unicast-response bit is not actually set.The comment says "Class: IN (1) with unicast-response bit" but the value
0x0001is just the IN class without the unicast-response bit. The unicast-response bit would be0x8001. The current value is fine for standard multicast mDNS queries—just the comment is inaccurate.📝 Fix comment
// Type: PTR (12) packet.addAll([0x00, 0x0C]); - // Class: IN (1) with unicast-response bit + // Class: IN (1) packet.addAll([0x00, 0x01]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/services/functional_services/discovery_service.dart` around lines 176 - 178, The comment next to the DNS class bytes is incorrect: the bytes added by packet.addAll([0x00, 0x01]) represent the IN class only and do not set the unicast-response bit (that would be 0x8001); update the comment in discovery_service.dart adjacent to the packet.addAll([0x00, 0x01]) call to accurately say something like "Class: IN (1) — no unicast-response bit set (unicast bit would be 0x8001)" so the code and comment match.lib/ui/views/login/login_viewmodel.dart (1)
52-63:notifyListeners()is redundant here.Since the UI uses
ValueListenableBuilderbound directly to_discoveryService.discoveredServersand_discoveryService.isDiscovering, the UI updates automatically when those notifiers change. ThenotifyListeners()call fromBaseViewModeldoesn't contribute to the discovery UI updates.♻️ Remove redundant call
Future<List<DiscoveredServer>> discoverServers() async { log.i('Starting server discovery...'); final servers = await _discoveryService.discover(); if (servers.isEmpty) { Fluttertoast.showToast( msg: 'No servers found. Make sure your server advertises via mDNS.', ); } - notifyListeners(); return servers; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ui/views/login/login_viewmodel.dart` around lines 52 - 63, The notifyListeners() call inside discoverServers() is redundant because the UI is driven by ValueListenableBuilder listening to _discoveryService.discoveredServers and _discoveryService.isDiscovering; remove the notifyListeners() invocation from the Future<List<DiscoveredServer>> discoverServers() method (leave the logging, discovery call, toast, and return intact) so updates come solely from the discovery service notifiers and avoid unnecessary BaseViewModel notifications.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/services/functional_services/discovery_service.dart`:
- Around line 109-134: The socket.listen subscription in discover() (where
Timer(effectiveTimeout, ...), socket.listen(...), completer, and
discoveredServers are used) must be stored and explicitly cancelled to avoid
resource leaks and conflicting concurrent runs: create a StreamSubscription
variable for the listen call and call subscription.cancel() both inside the
timeout handler and when the completer completes (after socket.close()), and add
a simple concurrency guard (e.g., an _isDiscovering flag or checking an existing
non-completed completer) at the start of discover() to prevent starting a new
discovery while one is in progress; ensure you still close the socket and
complete the completer after cancelling the subscription and update
discoveredServers as before.
- Around line 185-261: _parseMdnsResponse currently returns a DiscoveredServer
for any DNS response; add a boolean (e.g., foundService) initialized false and
set it to true when you encounter a PTR (type 12) or SRV (type 33) record whose
service name matches "_rutorrent_mobile._tcp" (use _readName or the PTR target
text), and only construct/return DiscoveredServer if foundService is true
(otherwise return null). Also ensure you only apply SRV/TXT-derived
port/protocol values when foundService is true so unrelated packets don't
default to port 80; reference symbols: _parseMdnsResponse, _skipName, _readName,
type 12 (PTR), type 33 (SRV), and type 16 (TXT).
---
Nitpick comments:
In `@lib/services/functional_services/discovery_service.dart`:
- Around line 176-178: The comment next to the DNS class bytes is incorrect: the
bytes added by packet.addAll([0x00, 0x01]) represent the IN class only and do
not set the unicast-response bit (that would be 0x8001); update the comment in
discovery_service.dart adjacent to the packet.addAll([0x00, 0x01]) call to
accurately say something like "Class: IN (1) — no unicast-response bit set
(unicast bit would be 0x8001)" so the code and comment match.
In `@lib/ui/views/login/login_view.dart`:
- Around line 50-58: Remove the redundant immediate call to setModalState in the
IconButton onPressed handler: it does nothing before the async discover
operation and the UI is already driven by model.isDiscovering (a ValueNotifier)
and ValueListenableBuilder. Update the onPressed to simply call
model.discoverServers() (or await/then the Future) and remove the first
setModalState(() {}) invocation so only the async call runs (any necessary UI
updates will come from model.discoverServers() updating model.isDiscovering and
the ValueListenableBuilder); reference: IconButton, onPressed,
model.discoverServers(), setModalState, model.isDiscovering,
ValueListenableBuilder.
In `@lib/ui/views/login/login_viewmodel.dart`:
- Around line 52-63: The notifyListeners() call inside discoverServers() is
redundant because the UI is driven by ValueListenableBuilder listening to
_discoveryService.discoveredServers and _discoveryService.isDiscovering; remove
the notifyListeners() invocation from the Future<List<DiscoveredServer>>
discoverServers() method (leave the logging, discovery call, toast, and return
intact) so updates come solely from the discovery service notifiers and avoid
unnecessary BaseViewModel notifications.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 289f6f09-861f-4ecb-8da9-2e81de288077
📒 Files selected for processing (5)
lib/app/app.dartlib/app/app.locator.dartlib/services/functional_services/discovery_service.dartlib/ui/views/login/login_view.dartlib/ui/views/login/login_viewmodel.dart
Comment on lines
+109
to
+134
| Timer(effectiveTimeout, () { | ||
| if (!completer.isCompleted) { | ||
| socket.close(); | ||
| completer.complete(servers); | ||
| } | ||
| }); | ||
|
|
||
| // Listen for responses | ||
| socket.listen((event) { | ||
| if (event == RawSocketEvent.read) { | ||
| final datagram = socket.receive(); | ||
| if (datagram != null) { | ||
| try { | ||
| final server = _parseMdnsResponse(datagram); | ||
| if (server != null && | ||
| !servers.any((s) => s.host == server.host && s.port == server.port)) { | ||
| servers.add(server); | ||
| _log.i('Discovered server: ${server.url}'); | ||
| discoveredServers.value = List.from(servers); | ||
| } | ||
| } catch (e) { | ||
| _log.w('Error parsing mDNS response: $e'); | ||
| } | ||
| } | ||
| } | ||
| }); |
There was a problem hiding this comment.
Socket subscription not explicitly cancelled; potential resource leak on repeated calls.
The socket.listen() subscription is not stored or cancelled when the timeout fires. While socket.close() should eventually trigger the stream to end, explicitly cancelling the subscription is safer and clearer. Additionally, if discover() is called while a previous discovery is still in progress, both will run concurrently with conflicting state updates.
🛠️ Proposed fix: Store and cancel subscription, guard against concurrent calls
Future<List<DiscoveredServer>> discover({Duration? timeout}) async {
final effectiveTimeout = timeout ?? _discoveryTimeout;
+
+ // Prevent concurrent discovery runs
+ if (isDiscovering.value) {
+ _log.w('Discovery already in progress');
+ return discoveredServers.value;
+ }
+
isDiscovering.value = true;
discoveredServers.value = [];And for the listener:
// Listen for responses
- socket.listen((event) {
+ final subscription = socket.listen((event) {
if (event == RawSocketEvent.read) {
// ... existing code ...
}
});
// Set up timeout
Timer(effectiveTimeout, () {
if (!completer.isCompleted) {
+ subscription.cancel();
socket.close();
completer.complete(servers);
}
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/services/functional_services/discovery_service.dart` around lines 109 -
134, The socket.listen subscription in discover() (where Timer(effectiveTimeout,
...), socket.listen(...), completer, and discoveredServers are used) must be
stored and explicitly cancelled to avoid resource leaks and conflicting
concurrent runs: create a StreamSubscription variable for the listen call and
call subscription.cancel() both inside the timeout handler and when the
completer completes (after socket.close()), and add a simple concurrency guard
(e.g., an _isDiscovering flag or checking an existing non-completed completer)
at the start of discover() to prevent starting a new discovery while one is in
progress; ensure you still close the socket and complete the completer after
cancelling the subscription and update discoveredServers as before.
Comment on lines
+185
to
+261
| DiscoveredServer? _parseMdnsResponse(Datagram datagram) { | ||
| final data = datagram.data; | ||
|
|
||
| // Basic validation - minimum DNS header size | ||
| if (data.length < 12) return null; | ||
|
|
||
| // Check if this is a response (bit 15 of flags should be 1) | ||
| if ((data[2] & 0x80) == 0) return null; | ||
|
|
||
| // Use the sender's address as the host | ||
| final host = datagram.address.address; | ||
| int port = 80; | ||
| String protocol = 'http'; | ||
| String? name; | ||
|
|
||
| // Try to extract SRV record (port) and TXT record (protocol) | ||
| // by scanning the response data | ||
| int offset = 12; | ||
| try { | ||
| // Skip questions section | ||
| final qdCount = (data[4] << 8) | data[5]; | ||
| for (int i = 0; i < qdCount && offset < data.length; i++) { | ||
| offset = _skipName(data, offset); | ||
| offset += 4; // Skip QTYPE and QCLASS | ||
| } | ||
|
|
||
| // Parse answer/additional sections for SRV and TXT records | ||
| final anCount = (data[6] << 8) | data[7]; | ||
| final nsCount = (data[8] << 8) | data[9]; | ||
| final arCount = (data[10] << 8) | data[11]; | ||
| final totalRecords = anCount + nsCount + arCount; | ||
|
|
||
| for (int i = 0; i < totalRecords && offset < data.length - 2; i++) { | ||
| offset = _skipName(data, offset); | ||
| if (offset + 10 > data.length) break; | ||
|
|
||
| final type = (data[offset] << 8) | data[offset + 1]; | ||
| final rdLength = (data[offset + 8] << 8) | data[offset + 9]; | ||
| offset += 10; | ||
|
|
||
| if (offset + rdLength > data.length) break; | ||
|
|
||
| if (type == 33 && rdLength >= 6) { | ||
| // SRV record | ||
| port = (data[offset + 4] << 8) | data[offset + 5]; | ||
| } else if (type == 16) { | ||
| // TXT record | ||
| final txtData = String.fromCharCodes( | ||
| data.sublist(offset, offset + rdLength)); | ||
| if (txtData.contains('protocol=https')) { | ||
| protocol = 'https'; | ||
| } else if (txtData.contains('protocol=http')) { | ||
| protocol = 'http'; | ||
| } | ||
| } else if (type == 12) { | ||
| // PTR record - might contain the service name | ||
| try { | ||
| name = _readName(data, offset); | ||
| } catch (_) {} | ||
| } | ||
|
|
||
| offset += rdLength; | ||
| } | ||
| } catch (e) { | ||
| _log.w('Error parsing DNS records: $e'); | ||
| } | ||
|
|
||
| // Determine protocol from port if not specified in TXT | ||
| if (port == 443) protocol = 'https'; | ||
|
|
||
| return DiscoveredServer( | ||
| host: host, | ||
| port: port, | ||
| protocol: protocol, | ||
| name: name, | ||
| ); | ||
| } |
There was a problem hiding this comment.
Any mDNS response from the network is returned as a "discovered server", even if unrelated.
The method returns a DiscoveredServer for any packet with the response flag set, regardless of whether it actually advertises _rutorrent_mobile._tcp. If another device on the network responds to a different mDNS query around the same time, it could be incorrectly listed as a ruTorrent server with default port 80.
Consider validating that at least one record in the response matches the expected service type before returning a server.
🔧 Suggested approach
Add a flag to track whether a relevant record was found:
DiscoveredServer? _parseMdnsResponse(Datagram datagram) {
final data = datagram.data;
// Basic validation - minimum DNS header size
if (data.length < 12) return null;
// Check if this is a response (bit 15 of flags should be 1)
if ((data[2] & 0x80) == 0) return null;
// Use the sender's address as the host
final host = datagram.address.address;
int port = 80;
String protocol = 'http';
String? name;
+ bool foundRelevantRecord = false;
// Try to extract SRV record (port) and TXT record (protocol)
// ... parsing logic ...
+ if (type == 33 && rdLength >= 6) {
+ // SRV record
+ port = (data[offset + 4] << 8) | data[offset + 5];
+ foundRelevantRecord = true;
+ }
// ... rest of parsing ...
+ if (!foundRelevantRecord) return null;
+
// Determine protocol from port if not specified in TXT
if (port == 443) protocol = 'https';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/services/functional_services/discovery_service.dart` around lines 185 -
261, _parseMdnsResponse currently returns a DiscoveredServer for any DNS
response; add a boolean (e.g., foundService) initialized false and set it to
true when you encounter a PTR (type 12) or SRV (type 33) record whose service
name matches "_rutorrent_mobile._tcp" (use _readName or the PTR target text),
and only construct/return DiscoveredServer if foundService is true (otherwise
return null). Also ensure you only apply SRV/TXT-derived port/protocol values
when foundService is true so unrelated packets don't default to port 80;
reference symbols: _parseMdnsResponse, _skipName, _readName, type 12 (PTR), type
33 (SRV), and type 16 (TXT).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #75
Summary
Adds a native mDNS/DNS-SD discovery feature to the login screen, allowing users to automatically find their ruTorrent servers on the local network without manual IP entry.
Changes
This feature expects the ruTorrent server to be configured to advertise itself (e.g., via Avahi) using the _rutorrent_mobile._tcp service type.
Summary by CodeRabbit