Add per-network device store and targeted discovery - #49
Merged
Merged
Conversation
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved critical concurrency, shutdown-lifetime, and input-validation issues remain, along with discovery, persistence, and UI defects.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds gateway-scoped device/ISP persistence, targeted identity refresh, and enhanced Windows network discovery.
Changes:
- Migrates XML storage to schema v2 with per-network records.
- Adds mDNS, private-MAC classification, and proxy-ARP handling.
- Adds selectable device details and background identity refresh.
File summaries
| File | Description |
|---|---|
src/ui/tray_win32.c |
Coordinates gateway state, persistence, and refresh workers. |
src/ui/network_map_win32.h |
Defines device-selection messaging. |
src/ui/network_map_win32.c |
Adds device selection and detail rendering. |
src/platform/win32/discovery_win32.c |
Implements enhanced discovery and identity refresh. |
src/platform/win32/device_store_win32.c |
Implements schema v2 and per-network storage. |
include/linkpulse/discovery.h |
Declares the targeted identity refresh API. |
include/linkpulse/device_store.h |
Defines network-scoped store APIs. |
Review details
Suppressed comments (5)
src/platform/win32/device_store_win32.c:305
- The new ISP attributes are parsed into the private
network->isprecord, but no read path applies that record to the active network snapshot or tray state;lp_win32_device_store_applyonly applies device metadata. After a restart with no successful live lookup, the persisted ISP identity is therefore never shown or reused. Add an accessor for the current gateway's cached ISP and consume it before live refresh.
else if (strcmp(name_utf8, "isp") == 0)
copy_text(network->isp.isp, sizeof(network->isp.isp), value_utf8);
else if (strcmp(name_utf8, "isp-asn") == 0)
copy_text(network->isp.asn, sizeof(network->isp.asn), value_utf8);
else if (strcmp(name_utf8, "isp-public-ip") == 0)
src/platform/win32/discovery_win32.c:436
- These entries are intentionally blanked to support IP-based proxy-ARP identity, but the map's
is_device_neighborpredicate still rejects every neighbor whose MAC is empty. Consequently proxy-ARP clients never reach the map, cannot be selected, and cannot trigger the targeted refresh. Allow valid IP-only neighbors in the UI (while keeping gateway/multicast filtering safe).
for (size_t j = i; j < list->count; ++j) {
if (strcmp(list->items[j].mac, ambiguous_mac) == 0) {
list->items[j].mac[0] = '\0';
src/platform/win32/discovery_win32.c:333
- The locally-administered-bit heuristic is applied to every interface, so a wired or virtual adapter with a locally administered MAC is labeled
Private Wi-Fi MACand classified as a mobile device. Since this feature is specifically for randomized Wi-Fi identities, gate the fallback onneighbor->connection_type == LP_CONNECTION_WIFI.
if (neighbor->device_type == LP_DEVICE_UNKNOWN &&
is_locally_administered_mac(neighbor->mac)) {
snprintf(neighbor->vendor, sizeof(neighbor->vendor), "Private Wi-Fi MAC");
neighbor->device_type = LP_DEVICE_MOBILE;
neighbor->device_confidence = 35;
src/platform/win32/discovery_win32.c:346
- The map can contain IPv6 neighbors because the snapshot enumerates
AF_UNSPEC, but this new refresh path constructs only an IPv4 address and returnsLP_ERR_UNSUPPORTEDfor every selected IPv6 device. Targeted identity refresh is therefore nonfunctional for part of the discovery set. Parse IPv6 as well, or explicitly route IPv6 through a supported resolver.
SOCKADDR_INET address;
memset(&address, 0, sizeof(address));
address.Ipv4.sin_family = AF_INET;
if (InetPtonA(AF_INET, neighbor->ip, &address.Ipv4.sin_addr) != 1) {
return LP_ERR_UNSUPPORTED;
src/ui/network_map_win32.c:7
- The popup is positioned with
work_area.bottom - LP_MAP_HEIGHT - 8and no top clamp. A 740-pixel window cannot fit in common 768-pixel displays once the taskbar reduces the work area, so its target Y becomes negative and the map is clipped. Size the popup to the available work area or clamp the target position.
#define LP_MAP_HEIGHT 740
- Files reviewed: 7/7 changed files
- Comments generated: 15
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| not just the map, so a transient DNS miss doesn't hide a known name. */ | ||
| for (size_t i = 0; i < event_count; ++i) { | ||
| lp_win32_device_store_apply(state->device_store, &events[i].neighbor); | ||
| lp_win32_device_store_apply(state->device_store, gateway_mac, &events[i].neighbor); |
Comment on lines
+613
to
+615
| for (size_t i = 0; i < state->map_neighbors.count; ++i) { | ||
| if (strcmp(state->map_neighbors.items[i].ip, neighbor.ip) == 0) { | ||
| state->map_neighbors.items[i] = neighbor; |
Comment on lines
+923
to
+925
| if (copy_data != NULL && copy_data->dwData == LP_NETWORK_MAP_COPYDATA_DEVICE_SELECTED && | ||
| copy_data->lpData != NULL && copy_data->cbData > 0) { | ||
| refresh_selected_device_identity(state, (const char *)copy_data->lpData); |
Comment on lines
+1026
to
+1029
| if (wait_result == WAIT_TIMEOUT) { | ||
| LP_WARN("device-refresh thread did not exit within %u ms; continuing shutdown", | ||
| (unsigned)LP_UPDATE_THREAD_SHUTDOWN_TIMEOUT_MS); | ||
| can_delete_lock = false; |
Comment on lines
+1038
to
+1040
| if (state->isp_refresh_event != NULL) { | ||
| CloseHandle(state->isp_refresh_event); | ||
| state->isp_refresh_event = NULL; |
Comment on lines
+692
to
+695
| if (neighbor->hostname[0] == '\0' && row->Address.si_family == AF_INET) { | ||
| (void)resolve_mdns_hostname_ipv4(&row->Address, false, neighbor->hostname, | ||
| sizeof(neighbor->hostname)); | ||
| } |
Comment on lines
+317
to
+320
| /* Public/isolated Wi-Fi often proxy-ARPs, so the real MAC is never visible. */ | ||
| snprintf(lines[count++], LP_ISP_DETAIL_LINE_MAX, | ||
| "MAC hidden by this network"); | ||
| } |
Comment on lines
+470
to
+471
| const int panel_top = client.bottom - 130; | ||
| RECT divider = {30, panel_top, client.right - 30, panel_top}; |
Comment on lines
+309
to
+315
| const HANDLE wait_handles[2] = {state->update_stop_event, state->isp_refresh_event}; | ||
| const DWORD wait_result = WaitForMultipleObjects(2, wait_handles, FALSE, | ||
| LP_ISP_LOOKUP_INTERVAL_MS); | ||
| if (wait_result == WAIT_OBJECT_0) { | ||
| break; /* stop event */ | ||
| } | ||
| /* WAIT_OBJECT_0 + 1 (refresh) or WAIT_TIMEOUT both fall through to loop and re-lookup. */ |
Comment on lines
+394
to
+396
| const lp_network_context_t network_context = { | ||
| gateway_mac, gateway_hostname, gateway_vendor, | ||
| state->isp_available ? &state->isp_info : NULL}; |
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
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.
Summary
Validation