feat: Enhance running-apps with mDNS discovery, deduplication, and table output #180098
Conversation
…ble output Includes: - Display running apps in a formatted table with age calculation - Implement mDNS discovery for running apps (multiple devices/interfaces) - Deduplicate apps by WebSocket URI - Centralize mDNS device advertisement in MDNSDeviceDiscovery - Ensure ResidentRunner advertises correct app name and cleans up - Add network utility functions and JSON support for running-apps - Add comprehensive tests for discovery and list command
There was a problem hiding this comment.
Code Review
This pull request introduces a significant feature: mDNS-based discovery and advertisement of running Flutter applications. It adds a new running-apps command to display these apps, enhances network utilities, and integrates mDNS advertisement into the resident runners for both mobile/desktop and web platforms. The changes are well-structured and include comprehensive tests for the new functionality. My review includes a few suggestions for improving type safety and performance in the new code.
| apps.sort((Map<String, dynamic> a, Map<String, dynamic> b) { | ||
| final int? epochA = int.tryParse(a['epoch'] as String? ?? ''); | ||
| final int? epochB = int.tryParse(b['epoch'] as String? ?? ''); | ||
| if (epochA == null && epochB == null) { | ||
| return 0; | ||
| } | ||
| if (epochA == null) { | ||
| return 1; // Put unknown age last | ||
| } | ||
| if (epochB == null) { | ||
| return -1; // Put unknown age last | ||
| } | ||
| return epochB.compareTo(epochA); | ||
| }); |
There was a problem hiding this comment.
The apps list is of type List<Map<String, String>>. The sort callback should use Map<String, String> for its parameters to match this type. This improves type safety and allows removing the unnecessary cast.
| apps.sort((Map<String, dynamic> a, Map<String, dynamic> b) { | |
| final int? epochA = int.tryParse(a['epoch'] as String? ?? ''); | |
| final int? epochB = int.tryParse(b['epoch'] as String? ?? ''); | |
| if (epochA == null && epochB == null) { | |
| return 0; | |
| } | |
| if (epochA == null) { | |
| return 1; // Put unknown age last | |
| } | |
| if (epochB == null) { | |
| return -1; // Put unknown age last | |
| } | |
| return epochB.compareTo(epochA); | |
| }); | |
| apps.sort((Map<String, String> a, Map<String, String> b) { | |
| final int? epochA = int.tryParse(a['epoch'] ?? ''); | |
| final int? epochB = int.tryParse(b['epoch'] ?? ''); | |
| if (epochA == null && epochB == null) { | |
| return 0; | |
| } | |
| if (epochA == null) { | |
| return 1; // Put unknown age last | |
| } | |
| if (epochB == null) { | |
| return -1; // Put unknown age last | |
| } | |
| return epochB.compareTo(epochA); | |
| }); |
| for (final Map<String, dynamic> app in apps) { | ||
| final String projectName = app['project_name'] as String? ?? 'Unknown'; | ||
| final String mode = app['mode'] as String? ?? 'Unknown'; | ||
| final String deviceName = app['device_name'] as String? ?? 'Unknown'; | ||
| final String deviceId = app['device_id'] as String? ?? 'Unknown'; | ||
| final String platform = app['target_platform'] as String? ?? 'Unknown'; | ||
| final String vmServiceUri = app['ws_uri'] as String? ?? 'Unknown'; | ||
| final String age = getProcessAge(app['epoch'] as String?, globals.systemClock); |
There was a problem hiding this comment.
The apps list is of type List<Map<String, String>>. The loop variable app should be typed as Map<String, String> to match. This improves type safety and allows removing the unnecessary casts.
| for (final Map<String, dynamic> app in apps) { | |
| final String projectName = app['project_name'] as String? ?? 'Unknown'; | |
| final String mode = app['mode'] as String? ?? 'Unknown'; | |
| final String deviceName = app['device_name'] as String? ?? 'Unknown'; | |
| final String deviceId = app['device_id'] as String? ?? 'Unknown'; | |
| final String platform = app['target_platform'] as String? ?? 'Unknown'; | |
| final String vmServiceUri = app['ws_uri'] as String? ?? 'Unknown'; | |
| final String age = getProcessAge(app['epoch'] as String?, globals.systemClock); | |
| for (final Map<String, String> app in apps) { | |
| final String projectName = app['project_name'] ?? 'Unknown'; | |
| final String mode = app['mode'] ?? 'Unknown'; | |
| final String deviceName = app['device_name'] ?? 'Unknown'; | |
| final String deviceId = app['device_id'] ?? 'Unknown'; | |
| final String platform = app['target_platform'] ?? 'Unknown'; | |
| final String vmServiceUri = app['ws_uri'] ?? 'Unknown'; | |
| final String age = getProcessAge(app['epoch'], globals.systemClock); |
| final MDNSService mdnsService = await MDNSService.create( | ||
| instance: 'Flutter Tools on $hostname', | ||
| service: '_flutter_devices._tcp', | ||
| port: await findUnusedPort(), |
There was a problem hiding this comment.
The port for the mDNS service should correspond to the service being advertised, which is the VM service. Using vmServiceUri.port is more accurate and avoids the overhead of calling findUnusedPort() repeatedly in a loop for each network interface. findUnusedPort() binds and closes a socket, which is an unnecessary operation here.
| port: await findUnusedPort(), | |
| port: vmServiceUri.port, |
| appName: FlutterProject.fromDirectory( | ||
| globals.fs.directory(projectRootPath), | ||
| ).manifest.appName, |
There was a problem hiding this comment.
Calling FlutterProject.fromDirectory inside this loop is inefficient as it may perform file I/O on each iteration. It's better to determine the appName once before the loop starts.
Includes: