fix: implement real-time network IP auto-update via server polling#58
fix: implement real-time network IP auto-update via server polling#58Th-Shivam wants to merge 6 commits into
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughServer now detects local IP changes on a configurable polling interval and broadcasts updates to connected WebSocket clients; the client retains the open socket to receive IP updates and adds a Ping Delay control to configure the polling interval. Config gains mouse sensitivity and networkPollingInterval entries. Changes
Sequence DiagramsequenceDiagram
participant Server as Server
participant Poller as IP Poller
participant WSS as WebSocket Server
participant Client as Client
Server->>Poller: start polling (interval from config)
Client->>WSS: open WebSocket
WSS->>Client: send "connected" with currentIp
Client->>Client: display initial IP
loop every polling interval
Poller->>Server: call getLocalIp()
alt ip changed
Poller->>Server: update currentIp
Server->>WSS: broadcast "server-ip" (new IP)
WSS->>Client: send "server-ip"
Client->>Client: update displayed IP
end
end
Client->>Client: user adjusts Ping Delay
Client->>Server: save config with networkPollingInterval
Server->>Poller: adjust polling interval
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
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 |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@src/routes/settings.tsx`:
- Around line 73-76: The cleanup currently only closes the socket when
socket.readyState === WebSocket.OPEN, missing the CONNECTING state; update the
teardown in the component's cleanup (the return callback where `socket` is
referenced) to close the socket whenever it's not already closing/closed — e.g.
check `socket && socket.readyState !== WebSocket.CLOSING && socket.readyState
!== WebSocket.CLOSED` (or explicitly allow OPEN and CONNECTING) before calling
`socket.close()` so connecting sockets are properly torn down.
In `@src/server/websocket.ts`:
- Line 40: The messages use inconsistent IP property names (some send { serverIp
} while others send { ip }); standardize on a single property (use "ip") across
all message payloads: update the construction of updateMsg (currently
JSON.stringify({ type: 'server-ip', ip: currentIp })) is correct, so change the
payload for the "connected" message and any other sends/broadcasts that use
serverIp to use ip instead (look for occurrences of "serverIp" in the
send/broadcast logic and in functions building the "connected" message) so the
client can always read data.ip.
- Around line 33-47: The setInterval polling in websocket.ts is never cleared
causing a resource leak; modify createWsServer (or the scope where currentIp and
wss are created) to store the interval ID (e.g., pollingIntervalId) returned by
setInterval and add cleanup to clearInterval(pollingIntervalId) when the
WebSocket server closes or is recreated: attach a 'close' handler on wss (or
call clearInterval before creating a new server) to call clearInterval, and also
ensure any process shutdown hooks clear it so intervals don't stack if
createWsServer runs multiple times.
🧹 Nitpick comments (2)
src/routes/settings.tsx (2)
44-77: No reconnection logic if the WebSocket drops.If the server restarts or the connection is lost, the client silently stops receiving IP updates. Consider adding an
onerror/onclosehandler with a retry (e.g., exponential backoff or a simplesetTimeoutreconnect) so the live-update feature is resilient.
59-67: Dual-property handling is a workaround for server inconsistency.The
(data.ip || data.serverIp)check on line 64 exists only because the server sends different property names for different message types. Once the server-side naming is unified (as suggested in the other file), simplify this to a single property.
4970310 to
ff4cae8
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/server/websocket.ts (1)
110-126:⚠️ Potential issue | 🟠 MajorArbitrary config write — no validation or allowlist on
msg.config.
update-configmerges whatever the client sends directly into the JSON config file ({ ...current, ...msg.config }). A malicious WebSocket client could inject arbitrary keys (e.g., overwritehost,address, or add unexpected fields). Consider validating against an allowlist of permitted keys and sanitizing values (e.g., ensuringnetworkPollingIntervalis a number within bounds).Proposed allowlist approach
if (msg.type === 'update-config') { console.log('Updating config:', msg.config); try { + const ALLOWED_KEYS = ['frontendPort', 'networkPollingInterval', 'mouseInvert', 'mouseSensitivity']; + const sanitized: Record<string, unknown> = {}; + for (const key of ALLOWED_KEYS) { + if (key in msg.config) sanitized[key] = msg.config[key]; + } const configPath = './src/server-config.json'; const current = fs.existsSync(configPath) ? JSON.parse(fs.readFileSync(configPath, 'utf-8')) : {}; - const newConfig = { ...current, ...msg.config }; + const newConfig = { ...current, ...sanitized };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/websocket.ts` around lines 110 - 126, The update-config handler currently merges msg.config blindly into the file (see the if (msg.type === 'update-config') block, configPath, current and newConfig), so add an allowlist and sanitization step before merging: define a permitted keys list (e.g., ['networkPollingInterval','someOtherKey']) and only copy those keys from msg.config, coerce/validate types (ensure networkPollingInterval is a number within acceptable bounds) and reject or ignore any extra keys; if validation fails, send ws.send a descriptive error and do not write the file; then merge the validated subset with current and write as before.
🧹 Nitpick comments (5)
src/routes/settings.tsx (3)
225-255: Save Config opens a throwaway WebSocket — fire-and-forget with no delivery guarantee.The handler creates a new WebSocket, sends the config, waits 1 second, then navigates away. There's no check that the server acknowledged the write (the
config-updatedresponse is ignored). If the WebSocket takes longer to connect or the write fails, the user is redirected under the false impression that the config was saved.Consider listening for the
config-updatedresponse before navigating, with a timeout as a fallback:Sketch
socket.onopen = () => { socket.send(JSON.stringify({ type: 'update-config', config: { ... } })); - setTimeout(() => { - socket.close(); - ... - window.location.href = newUrl; - }, 1000); }; +socket.onmessage = (event) => { + const data = JSON.parse(event.data); + if (data.type === 'config-updated') { + socket.close(); + const newUrl = `${window.location.protocol}//${window.location.hostname}:${frontendPort}/settings`; + window.location.href = newUrl; + } +}; +// Fallback timeout in case server doesn't respond +setTimeout(() => { + if (socket.readyState === WebSocket.OPEN) socket.close(); +}, 5000);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/settings.tsx` around lines 225 - 255, The current Save Config handler creates a throwaway WebSocket (new WebSocket(wsUrl)), sends the update in socket.onopen and then blindly navigates after a 1s setTimeout; change this to wait for an acknowledgement by adding socket.onmessage to listen for a "config-updated" response before navigating, and also add socket.onerror/onclose handling and a fallback timeout (e.g., 5s) that navigates only if no ack arrives; ensure you still send the payload using the existing payload construction (parseInt(frontendPort), pingDelay, invertScroll, sensitivity), clear the timeout and close the socket when the ack is received, and only then set window.location.href to the newUrl.
93-98:data.serverIpfallback is now dead code.The server was updated to use the
ipproperty consistently in bothconnectedandserver-ipmessages (seewebsocket.tslines 97 and 105). Thedata.serverIpfallback on line 95 will never be truthy. Consider removing it to avoid confusion.Proposed cleanup
- if ((data.type === 'server-ip' || data.type === 'connected') && (data.ip || data.serverIp)) { - const newIp = data.ip || data.serverIp; + if ((data.type === 'server-ip' || data.type === 'connected') && data.ip) { + const newIp = data.ip;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/settings.tsx` around lines 93 - 98, The check using data.serverIp is dead code—update the IP handling in the component by removing the fallback to data.serverIp and only use data.ip; specifically simplify the conditional that currently reads (data.type === 'server-ip' || data.type === 'connected') && (data.ip || data.serverIp) to rely solely on data.ip, and setIp(newIp) using newIp = data.ip (remove references to data.serverIp) so the component matches the server's websocket message shape and avoids confusing dead branches.
76-110: WebSocket stays open for the lifetime of the settings page — consider reconnection logic.If the WebSocket connection drops (e.g., server restart after config save), the component won't attempt to reconnect, silently losing the live IP update capability. Since the whole point is to detect network changes, adding a basic reconnect-on-close with backoff would improve resilience.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/settings.tsx` around lines 76 - 110, The WebSocket created inside the useEffect is never re-established if it closes; add reconnection logic with exponential/backoff retry and proper cleanup: wrap the socket creation logic (currently using socket, socket.onopen, socket.onmessage) in a reconnect function inside the same useEffect, install socket.onclose to schedule a retry (incrementing delay up to a cap), ensure you cancel pending retry timers and avoid creating multiple sockets by tracking a current socket/aborted flag, and on unmount clear timers and close the active socket and stop further retries; keep setIp usage the same in socket.onmessage and limit max retries to avoid infinite loops.src/server/websocket.ts (2)
14-24:getLocalIpreturns the first non-internal IPv4 address found — order is non-deterministic.
os.networkInterfaces()does not guarantee interface ordering across platforms or reboots. If the machine has multiple network interfaces (e.g., Ethernet + Wi-Fi + Docker bridge), the returned IP could vary unpredictably. This is acceptable as a best-effort heuristic, but worth documenting in the JSDoc that the result may be ambiguous on multi-homed hosts.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/websocket.ts` around lines 14 - 24, getLocalIp currently returns the first non-internal IPv4 address found but interface ordering from os.networkInterfaces() is non-deterministic on multi-homed hosts; add a JSDoc comment above the getLocalIp function to document this best-effort behavior, that the returned IP may vary when multiple interfaces exist (Ethernet/Wi‑Fi/containers), and note the fallback to 'localhost' so callers know the ambiguity and can choose a deterministic strategy if needed.
44-47: Extract hardcoded config path to a constant using__dirname.The hardcoded path
'./src/server-config.json'on lines 45 and 113 depends on the process CWD being the project root and assumessrc/exists at runtime—both issues in compiled/bundled deployments. Extract to a shared constant at the top of the file:Proposed fix
+import path from 'path'; + +const CONFIG_PATH = path.resolve(__dirname, '../server-config.json'); + // Then use CONFIG_PATH instead of './src/server-config.json' on lines 45 and 113🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/websocket.ts` around lines 44 - 47, Replace the hardcoded './src/server-config.json' with a module-level constant built using __dirname so path resolution is deterministic in compiled/bundled runs: create a constant (e.g., SERVER_CONFIG_PATH) at the top of websocket.ts computed via path.join(__dirname, 'server-config.json') and use that constant everywhere you currently reference configPath (the variable inside the try block and any other spots such as the later reference around line ~113); ensure you import/require path and update fs.existsSync and fs.readFileSync calls to use SERVER_CONFIG_PATH.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/routes/settings.tsx`:
- Line 19: The state initializer for pingDelay uses a falsy fallback operator;
replace CONFIG.NETWORK_POLLING_INTERVAL || 5000 with a nullish coalescing
fallback so a valid 0 is preserved—update the useState call that sets pingDelay
(and setPingDelay) to use CONFIG.NETWORK_POLLING_INTERVAL ?? 5000, matching the
config.tsx style.
- Around line 195-214: The Ping Delay label isn't associated with its range
input; add an id to the range input (e.g., "pingDelay") and set the label's
htmlFor to that id so the <label> for the Ping Delay control correctly
references the input (follow the same pattern used by the sensitivity slider
around sensitivity/sensitivity input and setPingDelay/pingDelay usage to locate
the elements).
In `@src/server-config.json`:
- Around line 4-7: The server config sets mouseInvert to true while the client
settings UI reads invertScroll from localStorage defaulting to false, causing a
mismatch; decide the intended default and make them consistent by either
changing the server's "mouseInvert" default to false or updating the client to
default invertScroll to true, and ensure the settings save/read code (the
client-side invertScroll/localStorage logic and the server-side mouseInvert
value) are aligned so the first save doesn't unintentionally flip the setting.
In `@src/server/websocket.ts`:
- Around line 48-50: The falsy check on config.networkPollingInterval
incorrectly ignores valid 0 values; change the conditional around where
pollingInterval is set (the config.networkPollingInterval -> pollingInterval
assignment) to explicitly test for undefined/null (e.g.,
config.networkPollingInterval !== undefined && config.networkPollingInterval !==
null) and then clamp the value to a safe minimum before assignment (e.g.,
Math.max(config.networkPollingInterval, MIN_POLL_INTERVAL) or similar) so the
default 5000ms is only used when config provides no value and overly-small
values are prevented.
- Around line 79-81: The current signal handlers added in createWsServer use
process.on and stack on repeated calls and suppress default exit; change them to
process.once for 'SIGTERM' and 'SIGINT' and have each handler first
clearInterval(pollingIntervalId) then call process.exit(0) so the interval is
cleaned up and the process terminates normally; locate the handlers in
websocket.ts near createWsServer and replace the process.on(...) callbacks
accordingly.
- Around line 43-55: The code reads POLLING_INTERVAL once at startup (from
pollingInterval/pollingInterval const) so changes via the WebSocket
"update-config" handler don't affect the running setInterval; refactor to keep a
mutable currentPollingInterval variable, encapsulate the polling logic in a
startPolling() function that returns the interval id (pollingIntervalId), and in
the "update-config" handler (inside createWsServer) compare
msg.config.networkPollingInterval to currentPollingInterval and if different
update currentPollingInterval, clearInterval(pollingIntervalId) and set
pollingIntervalId = startPolling() so the new interval takes effect immediately.
---
Outside diff comments:
In `@src/server/websocket.ts`:
- Around line 110-126: The update-config handler currently merges msg.config
blindly into the file (see the if (msg.type === 'update-config') block,
configPath, current and newConfig), so add an allowlist and sanitization step
before merging: define a permitted keys list (e.g.,
['networkPollingInterval','someOtherKey']) and only copy those keys from
msg.config, coerce/validate types (ensure networkPollingInterval is a number
within acceptable bounds) and reject or ignore any extra keys; if validation
fails, send ws.send a descriptive error and do not write the file; then merge
the validated subset with current and write as before.
---
Nitpick comments:
In `@src/routes/settings.tsx`:
- Around line 225-255: The current Save Config handler creates a throwaway
WebSocket (new WebSocket(wsUrl)), sends the update in socket.onopen and then
blindly navigates after a 1s setTimeout; change this to wait for an
acknowledgement by adding socket.onmessage to listen for a "config-updated"
response before navigating, and also add socket.onerror/onclose handling and a
fallback timeout (e.g., 5s) that navigates only if no ack arrives; ensure you
still send the payload using the existing payload construction
(parseInt(frontendPort), pingDelay, invertScroll, sensitivity), clear the
timeout and close the socket when the ack is received, and only then set
window.location.href to the newUrl.
- Around line 93-98: The check using data.serverIp is dead code—update the IP
handling in the component by removing the fallback to data.serverIp and only use
data.ip; specifically simplify the conditional that currently reads (data.type
=== 'server-ip' || data.type === 'connected') && (data.ip || data.serverIp) to
rely solely on data.ip, and setIp(newIp) using newIp = data.ip (remove
references to data.serverIp) so the component matches the server's websocket
message shape and avoids confusing dead branches.
- Around line 76-110: The WebSocket created inside the useEffect is never
re-established if it closes; add reconnection logic with exponential/backoff
retry and proper cleanup: wrap the socket creation logic (currently using
socket, socket.onopen, socket.onmessage) in a reconnect function inside the same
useEffect, install socket.onclose to schedule a retry (incrementing delay up to
a cap), ensure you cancel pending retry timers and avoid creating multiple
sockets by tracking a current socket/aborted flag, and on unmount clear timers
and close the active socket and stop further retries; keep setIp usage the same
in socket.onmessage and limit max retries to avoid infinite loops.
In `@src/server/websocket.ts`:
- Around line 14-24: getLocalIp currently returns the first non-internal IPv4
address found but interface ordering from os.networkInterfaces() is
non-deterministic on multi-homed hosts; add a JSDoc comment above the getLocalIp
function to document this best-effort behavior, that the returned IP may vary
when multiple interfaces exist (Ethernet/Wi‑Fi/containers), and note the
fallback to 'localhost' so callers know the ambiguity and can choose a
deterministic strategy if needed.
- Around line 44-47: Replace the hardcoded './src/server-config.json' with a
module-level constant built using __dirname so path resolution is deterministic
in compiled/bundled runs: create a constant (e.g., SERVER_CONFIG_PATH) at the
top of websocket.ts computed via path.join(__dirname, 'server-config.json') and
use that constant everywhere you currently reference configPath (the variable
inside the try block and any other spots such as the later reference around line
~113); ensure you import/require path and update fs.existsSync and
fs.readFileSync calls to use SERVER_CONFIG_PATH.
| "address": "rein.local", | ||
| "mouseInvert": true, | ||
| "mouseSensitivity": 1.0, | ||
| "networkPollingInterval": 5000 |
There was a problem hiding this comment.
mouseInvert defaults to true — verify this is intentional.
Most applications default to natural (non-inverted) scrolling. The settings UI initializes invertScroll from localStorage (defaulting to false), so there's a mismatch between the server config default (true) and the client-side default (false). This could cause confusing behavior when saving config, as the first "Save Config" click would write mouseInvert: false (from the client default) over the server's true.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/server-config.json` around lines 4 - 7, The server config sets
mouseInvert to true while the client settings UI reads invertScroll from
localStorage defaulting to false, causing a mismatch; decide the intended
default and make them consistent by either changing the server's "mouseInvert"
default to false or updating the client to default invertScroll to true, and
ensure the settings save/read code (the client-side invertScroll/localStorage
logic and the server-side mouseInvert value) are aligned so the first save
doesn't unintentionally flip the setting.
There was a problem hiding this comment.
Pull request overview
This PR implements automatic detection and broadcasting of network IP changes to fix issue #51, where the displayed IP address and QR code became stale when the host machine switched networks. The solution introduces a server-side polling mechanism that checks for IP changes every 5 seconds (configurable) and broadcasts updates to all connected WebSocket clients in real-time.
Changes:
- Added network interface polling on the server with configurable intervals (1-30 seconds)
- Implemented WebSocket broadcast system to push IP updates to all connected clients
- Modified the settings page to maintain a persistent WebSocket connection for receiving live IP updates
- Added UI controls for configuring the network polling interval
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 16 comments.
| File | Description |
|---|---|
| src/server/websocket.ts | Implements network polling loop with setInterval, reads polling configuration, and broadcasts IP changes to all connected clients |
| src/server-config.json | Adds networkPollingInterval setting and client-side preferences (mouseInvert, mouseSensitivity) |
| src/routes/settings.tsx | Establishes persistent WebSocket connection for live IP updates and adds UI slider for polling interval configuration |
| src/config.tsx | Exports new config values for MOUSE_SENSITIVITY and NETWORK_POLLING_INTERVAL |
Comments suppressed due to low confidence (1)
src/routes/settings.tsx:251
- The WebSocket connection created in the Save Config button click handler lacks error handling. Based on the pattern in useRemoteConnection.ts (line 24-27), WebSocket connections should include an onerror handler. Without proper error handling, connection failures will result in uncaught errors, and the user may be stuck waiting without feedback if the connection fails.
const socket = new WebSocket(wsUrl);
socket.onopen = () => {
socket.send(JSON.stringify({
type: 'update-config',
config: {
frontendPort: parseInt(frontendPort),
networkPollingInterval: pingDelay,
mouseInvert: invertScroll,
mouseSensitivity: sensitivity,
}
}));
setTimeout(() => {
socket.close();
const newProtocol = window.location.protocol;
const newHostname = window.location.hostname;
const newUrl = `${newProtocol}//${newHostname}:${frontendPort}/settings`;
window.location.href = newUrl;
}, 1000);
};
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| process.on('SIGTERM', () => clearInterval(pollingIntervalId)); | ||
| process.on('SIGINT', () => clearInterval(pollingIntervalId)); |
There was a problem hiding this comment.
The process signal handlers on lines 80-81 are registered globally but never removed. If createWsServer is called multiple times (e.g., during server restarts in development), this will create duplicate signal handlers that accumulate over time. Each handler will try to clear the same interval ID, which could lead to issues. Consider using process.once() instead of process.on(), or implement a cleanup mechanism to remove these handlers when the WebSocket server is closed.
| process.on('SIGTERM', () => clearInterval(pollingIntervalId)); | |
| process.on('SIGINT', () => clearInterval(pollingIntervalId)); | |
| process.once('SIGTERM', () => clearInterval(pollingIntervalId)); | |
| process.once('SIGINT', () => clearInterval(pollingIntervalId)); |
| let pollingInterval = 5000; | ||
| try { | ||
| const configPath = './src/server-config.json'; | ||
| if (fs.existsSync(configPath)) { | ||
| const config = JSON.parse(fs.readFileSync(configPath, 'utf-8')); | ||
| if (config.networkPollingInterval) { | ||
| pollingInterval = config.networkPollingInterval; | ||
| } | ||
| } | ||
| } catch (e) { | ||
| console.error('Failed to read initial polling interval:', e); | ||
| } |
There was a problem hiding this comment.
Reading the server-config.json file synchronously on every server initialization could cause issues if the file is being written to at the same time (e.g., when a user updates config via the settings page). While the current implementation has try-catch, a more robust approach would be to read the config once during server startup or implement proper file locking. Additionally, changes to networkPollingInterval in the config file won't take effect until the server restarts, which might be confusing to users.
| mouseInvert: invertScroll, | ||
| mouseSensitivity: sensitivity, |
There was a problem hiding this comment.
The mouseInvert and mouseSensitivity settings are client-side preferences (stored in localStorage on lines 52-57) but are being saved to the server-config.json file (lines 239-240). This creates confusion about the source of truth for these settings. When a new client connects, they will get the server's config values instead of their own previously saved preferences. These client-specific settings should either be kept entirely in localStorage or the server should support per-client settings. Saving them to the server config is inconsistent with their localStorage-based initialization.
| mouseInvert: invertScroll, | |
| mouseSensitivity: sensitivity, |
| ws.send(JSON.stringify({ type: 'server-ip', ip: currentIp })); | ||
| return; | ||
| } | ||
| // ... existing update-config logic ... |
There was a problem hiding this comment.
The comment "// ... existing update-config logic ..." on line 108 is misleading. This comment appears before the actual update-config logic (lines 110-126), suggesting that there's additional update-config logic elsewhere that's being omitted. In reality, the update-config handler immediately follows on line 110. This comment should be removed or clarified to avoid confusion about the code structure.
| // ... existing update-config logic ... |
|
|
||
| <div className="form-control w-full"> | ||
| <label className="label"> | ||
| <span className="label-text">Ping Delay (ms)</span> |
There was a problem hiding this comment.
The label "Ping Delay (ms)" is misleading. This setting controls the network interface polling interval (how often the server checks for IP changes), not a ping delay or network latency. A more accurate label would be "Network Check Interval" or "IP Polling Interval" to avoid confusion. Users might expect this to affect network latency or connection timeout, when it actually controls how quickly IP changes are detected.
| <span className="label-text">Ping Delay (ms)</span> | |
| <span className="label-text">Network Check Interval (ms)</span> |
| const pollingIntervalId = setInterval(() => { | ||
| const newIp = getLocalIp(); | ||
| if (newIp !== currentIp) { | ||
| console.log(`Network Change Detected! IP: ${currentIp} -> ${newIp}`); | ||
| currentIp = newIp; | ||
|
|
||
| // Broadcast the new IP to all connected clients | ||
| const updateMsg = JSON.stringify({ type: 'server-ip', ip: currentIp }); | ||
| wss.clients.forEach((client) => { | ||
| if (client.readyState === WebSocket.OPEN) { | ||
| client.send(updateMsg); | ||
| } | ||
| }); | ||
| } | ||
| }, POLLING_INTERVAL); | ||
|
|
||
| // Cleanup interval when the WebSocket server closes | ||
| wss.on('close', () => { | ||
| console.log('Clearing network polling interval'); | ||
| clearInterval(pollingIntervalId); | ||
| }); | ||
|
|
||
| // Also handle process exit to ensure cleanup | ||
| process.on('SIGTERM', () => clearInterval(pollingIntervalId)); | ||
| process.on('SIGINT', () => clearInterval(pollingIntervalId)); |
There was a problem hiding this comment.
Polling os.networkInterfaces() every 5 seconds (or potentially as low as 1 second based on the UI slider) could have performance implications on systems with many network interfaces or on resource-constrained environments. The os.networkInterfaces() call involves system calls that could be expensive when called frequently. Consider implementing a more event-driven approach using OS-level network change events (e.g., via node libraries like 'network' or 'node-network-monitor') instead of polling, which would be more efficient and responsive.
| const pollingIntervalId = setInterval(() => { | |
| const newIp = getLocalIp(); | |
| if (newIp !== currentIp) { | |
| console.log(`Network Change Detected! IP: ${currentIp} -> ${newIp}`); | |
| currentIp = newIp; | |
| // Broadcast the new IP to all connected clients | |
| const updateMsg = JSON.stringify({ type: 'server-ip', ip: currentIp }); | |
| wss.clients.forEach((client) => { | |
| if (client.readyState === WebSocket.OPEN) { | |
| client.send(updateMsg); | |
| } | |
| }); | |
| } | |
| }, POLLING_INTERVAL); | |
| // Cleanup interval when the WebSocket server closes | |
| wss.on('close', () => { | |
| console.log('Clearing network polling interval'); | |
| clearInterval(pollingIntervalId); | |
| }); | |
| // Also handle process exit to ensure cleanup | |
| process.on('SIGTERM', () => clearInterval(pollingIntervalId)); | |
| process.on('SIGINT', () => clearInterval(pollingIntervalId)); | |
| // Adaptive polling configuration to reduce calls to os.networkInterfaces() | |
| const MIN_POLL_INTERVAL = 5000; // enforce a safe minimum (ms) | |
| const MAX_POLL_INTERVAL = 60000; // cap backoff (ms) | |
| let dynamicInterval = Math.max(POLLING_INTERVAL, MIN_POLL_INTERVAL); | |
| let pollTimeout: NodeJS.Timeout | null = null; | |
| const scheduleNextPoll = (delay: number) => { | |
| if (pollTimeout) { | |
| clearTimeout(pollTimeout); | |
| } | |
| pollTimeout = setTimeout(() => { | |
| const newIp = getLocalIp(); | |
| if (newIp !== currentIp) { | |
| console.log(`Network Change Detected! IP: ${currentIp} -> ${newIp}`); | |
| currentIp = newIp; | |
| // Broadcast the new IP to all connected clients | |
| const updateMsg = JSON.stringify({ type: 'server-ip', ip: currentIp }); | |
| wss.clients.forEach((client) => { | |
| if (client.readyState === WebSocket.OPEN) { | |
| client.send(updateMsg); | |
| } | |
| }); | |
| // Reset interval on change to be more responsive | |
| dynamicInterval = Math.max(POLLING_INTERVAL, MIN_POLL_INTERVAL); | |
| } else { | |
| // Exponentially back off when no change is detected | |
| dynamicInterval = Math.min(dynamicInterval * 2, MAX_POLL_INTERVAL); | |
| } | |
| // Schedule the next poll with the (possibly adjusted) interval | |
| scheduleNextPoll(dynamicInterval); | |
| }, delay); | |
| }; | |
| // Start adaptive polling | |
| scheduleNextPoll(dynamicInterval); | |
| // Cleanup timeout when the WebSocket server closes | |
| wss.on('close', () => { | |
| console.log('Clearing network polling timeout'); | |
| if (pollTimeout) { | |
| clearTimeout(pollTimeout); | |
| pollTimeout = null; | |
| } | |
| }); | |
| // Also handle process exit to ensure cleanup | |
| process.on('SIGTERM', () => { | |
| if (pollTimeout) { | |
| clearTimeout(pollTimeout); | |
| pollTimeout = null; | |
| } | |
| }); | |
| process.on('SIGINT', () => { | |
| if (pollTimeout) { | |
| clearTimeout(pollTimeout); | |
| pollTimeout = null; | |
| } | |
| }); |
| <span>1s (Fast)</span> | ||
| <span>5s (Default)</span> | ||
| <span>30s (Slow)</span> | ||
| </div> |
There was a problem hiding this comment.
The pingDelay state is initialized from CONFIG.NETWORK_POLLING_INTERVAL on mount, but changes to this value through the "Save Config" button won't take effect in the running server until it restarts. This creates a confusing user experience where the UI slider suggests the change is immediate, but the polling interval won't actually change until a server restart. Consider either: 1) adding a message to inform users that this change requires a restart, or 2) implementing dynamic interval updates by sending a message to the server to update the polling interval without requiring a restart.
| </div> | |
| </div> | |
| <p className="mt-1 text-xs opacity-70 text-warning"> | |
| Note: Changes to ping delay take effect after restarting the server. | |
| </p> |
| console.error('WS Message Error:', e); | ||
| } | ||
| }; | ||
|
|
There was a problem hiding this comment.
The WebSocket connection lacks an onclose handler to detect when the connection is lost. While the cleanup function (lines 104-109) handles the case where the component unmounts, it doesn't handle scenarios where the server closes the connection or network issues cause a disconnect. Based on the pattern in useRemoteConnection.ts (line 20-23), an onclose handler should be added to implement automatic reconnection or at least notify the user that live IP updates are no longer active.
| socket.onclose = (event) => { | |
| console.warn( | |
| 'IP detection WebSocket connection closed; live IP updates are no longer active.', | |
| { code: event.code, reason: event.reason } | |
| ); | |
| }; |
| console.log('Auto-detected IP:', data.ip); | ||
| setIp(data.ip); | ||
| socket.close(); | ||
| // Handle both initial response and subsequent broadcasts |
There was a problem hiding this comment.
The frontend code handles both 'data.ip' and 'data.serverIp' (line 94-95) for backward compatibility, but the backend only sends 'ip' in the 'connected' message (line 97). The fallback to 'data.serverIp' appears to be for handling old message formats that are no longer sent by the server. If backward compatibility with old clients is not needed, the 'data.serverIp' fallback should be removed. If it is needed, document why both formats are supported.
| // Handle both initial response and subsequent broadcasts | |
| // Handle both initial response and subsequent broadcasts. | |
| // Note: The backend currently sends `ip` in the 'connected' message. | |
| // Support for `serverIp` is retained for backward compatibility with | |
| // older server message formats that used `serverIp` instead of `ip`. |
| const socket = new WebSocket(wsUrl); | ||
|
|
||
| socket.onopen = () => { | ||
| console.log('Connected to local server for IP detection'); | ||
| console.log('Connected to local server for live IP tracking'); | ||
| socket.send(JSON.stringify({ type: 'get-ip' })); | ||
| }; |
There was a problem hiding this comment.
The WebSocket connection established in the IP auto-detection useEffect lacks error handling. Based on the pattern in useRemoteConnection.ts (line 24-27), WebSocket connections should include an onerror handler to properly handle connection failures and prevent silent errors. This could lead to uncaught errors when the WebSocket connection fails during IP tracking.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/server/websocket.ts (2)
14-24:⚠️ Potential issue | 🟡 Minor
nets[name]!non-null assertion can throw at runtime.
os.networkInterfaces()returns values typed asNetworkInterfaceInfo[] | undefined. If a listed interface name maps toundefined, the innerfor…ofwill throw. Use optional chaining or a guard.Proposed fix
function getLocalIp() { const nets = os.networkInterfaces(); for (const name of Object.keys(nets)) { - for (const net of nets[name]!) { + for (const net of nets[name] ?? []) { if (net.family === 'IPv4' && !net.internal) { return net.address; } } } return 'localhost'; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/websocket.ts` around lines 14 - 24, The getLocalIp function uses a non-null assertion on nets[name]! which can throw if os.networkInterfaces() yields undefined for a given name; change the inner loop to guard against undefined (e.g., const ifaceList = nets[name]; if (!ifaceList) continue) or use optional chaining before iterating, then iterate over ifaceList and check net.family and net.internal as before so you never iterate over undefined.
119-135:⚠️ Potential issue | 🟠 Major
update-configmerges arbitrary keys into the config file without validation.
msg.configis spread directly into the persisted JSON ({ ...current, ...msg.config }). A malicious or buggy client can inject unexpected keys or overwrite critical values. Whitelist the accepted fields.Proposed fix
const current = fs.existsSync(configPath) ? JSON.parse(fs.readFileSync(configPath, 'utf-8')) : {}; - const newConfig = { ...current, ...msg.config }; + const allowedKeys = ['frontendPort', 'networkPollingInterval', 'mouseSensitivity']; + const sanitized: Record<string, unknown> = {}; + for (const key of allowedKeys) { + if (key in msg.config) { + sanitized[key] = msg.config[key]; + } + } + const newConfig = { ...current, ...sanitized };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/websocket.ts` around lines 119 - 135, The update-config handler currently merges msg.config directly into the saved JSON (see the 'update-config' branch, variables configPath, current, newConfig), allowing arbitrary keys; restrict this by implementing a whitelist of allowed config fields and validate types/values before merging: enumerate accepted keys (e.g., allowedKeys = [...]) and build newConfigOnlyFromMsg by copying only those keys present in msg.config after type checks; if msg.config contains disallowed keys or invalid types, return an error over ws and do not write the file; keep the existing try/catch and logging but replace the direct spread merge with the validated/whitelisted merge logic.
🧹 Nitpick comments (3)
src/routes/settings.tsx (2)
86-120: No reconnection logic — a dropped WebSocket silently kills IP updates.If the WebSocket connection drops (e.g., server restart, network blip), the component never attempts to reconnect. The user sees stale IP/QR data with no indication that live updates stopped. Consider adding an
onerror/onclosehandler that retries with backoff.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/settings.tsx` around lines 86 - 120, The WebSocket created inside the useEffect (the socket variable in the IP auto-detection useEffect) lacks onerror/onclose handlers and reconnection/backoff logic; add onerror and onclose callbacks that attempt to reconnect with exponential backoff (and optional jitter) up to a reasonable max retry count, ensure you clear any timers and stop retrying on component unmount, avoid opening duplicate sockets (check/track an active socket or a reconnecting flag), and reuse existing message parsing logic (setIp) when a reconnect succeeds so IP/QR updates resume correctly.
100-112:data.serverIpfallback is now dead code.The server was updated to send
ipconsistently in bothconnectedandserver-ipmessages, sodata.serverIpon line 105 will never be truthy. Consider removing it to avoid confusion about what the server actually sends.Proposed cleanup
- if ((data.type === 'server-ip' || data.type === 'connected') && (data.ip || data.serverIp)) { - const newIp = data.ip || data.serverIp; + if ((data.type === 'server-ip' || data.type === 'connected') && data.ip) { + const newIp = data.ip; console.log('Received IP Update:', newIp); setIp(newIp); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/settings.tsx` around lines 100 - 112, The code handling WebSocket messages in socket.onmessage contains a dead fallback to data.serverIp; update the logic to only use data.ip for both 'server-ip' and 'connected' message types, remove references to data.serverIp to avoid confusion, and ensure you still call setIp(data.ip) and log the received IP via the existing console log; adjust the conditional to check (data.type === 'server-ip' || data.type === 'connected') && data.ip and remove the unused data.serverIp branch.src/server/websocket.ts (1)
46-53: Config path is hardcoded and duplicated.
'./src/server-config.json'appears in two places (lines 47 and 122) and is relative to CWD, which is fragile if the process starts from a different directory. Extract to a shared constant.Proposed fix
+import path from 'path'; + +const CONFIG_PATH = path.resolve(__dirname, '../server-config.json');Then replace both
'./src/server-config.json'references withCONFIG_PATH.Also applies to: 119-135
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/websocket.ts` around lines 46 - 53, Extract the hardcoded './src/server-config.json' string into a single exported constant CONFIG_PATH (computed with path.resolve(__dirname, '..', 'server-config.json') or similar to avoid CWD fragility) and replace both occurrences in this file where the config is read (the block that sets pollingInterval and the other location around lines 119-135) with CONFIG_PATH; keep the existing fs.existsSync and JSON.parse logic but reference CONFIG_PATH instead of the literal string so the path is defined once and used consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/server/websocket.ts`:
- Around line 14-24: The getLocalIp function uses a non-null assertion on
nets[name]! which can throw if os.networkInterfaces() yields undefined for a
given name; change the inner loop to guard against undefined (e.g., const
ifaceList = nets[name]; if (!ifaceList) continue) or use optional chaining
before iterating, then iterate over ifaceList and check net.family and
net.internal as before so you never iterate over undefined.
- Around line 119-135: The update-config handler currently merges msg.config
directly into the saved JSON (see the 'update-config' branch, variables
configPath, current, newConfig), allowing arbitrary keys; restrict this by
implementing a whitelist of allowed config fields and validate types/values
before merging: enumerate accepted keys (e.g., allowedKeys = [...]) and build
newConfigOnlyFromMsg by copying only those keys present in msg.config after type
checks; if msg.config contains disallowed keys or invalid types, return an error
over ws and do not write the file; keep the existing try/catch and logging but
replace the direct spread merge with the validated/whitelisted merge logic.
---
Duplicate comments:
In `@src/routes/settings.tsx`:
- Around line 223-242: The "Ping Delay" label isn't associated with its input;
add an id to the range input (e.g., "pingDelayInput" or "ping-delay") and set
the label's htmlFor to that same id so the label element in the <div
className="form-control w-full"> correctly targets the input (the input using
value={pingDelay} and onChange={(e) => setPingDelay(...)}). Ensure the id is
unique in this component and update only the <label> and corresponding <input>
attributes.
- Line 20: The default for pingDelay currently uses the || operator which treats
falsy values like 0 as missing; update the useState initializer for pingDelay to
use the nullish coalescing operator (??) with CONFIG.NETWORK_POLLING_INTERVAL so
that explicit 0 is respected (change the initializer in the useState call that
defines pingDelay / setPingDelay to use CONFIG.NETWORK_POLLING_INTERVAL ??
5000).
In `@src/server/websocket.ts`:
- Around line 45-57: The code reads POLLING_INTERVAL once into a const
(POLLING_INTERVAL / pollingInterval) so runtime updates via the "update-config"
flow don't change the timer; change the logic to use a mutable variable for the
interval and when handling the update-config event (or wherever the JSON is
written) clear the existing timer and restart it with the new interval (or
replace setInterval with a self-scheduling setTimeout loop that re-reads the
current pollingInterval each iteration). Update references to POLLING_INTERVAL
to use the mutable pollingInterval (or a getPollingInterval() accessor) and
ensure the timer restart occurs in the same module so the new
networkPollingInterval takes effect immediately.
- Around line 50-52: The current truthy check rejects 0 for
config.networkPollingInterval; change the logic that sets pollingInterval (the
variable and the use of config.networkPollingInterval) to explicitly check for
null/undefined (e.g., if (config.networkPollingInterval !== undefined &&
config.networkPollingInterval !== null)) and assign the value directly, and
enforce a minimum bound (e.g., validate with Number.isFinite and use
Math.max(minMs, config.networkPollingInterval)) so valid 0 or other numeric
values are preserved while preventing too-small intervals.
- Around line 81-83: Signal handlers are being appended on each createWsServer
call and adding a SIGINT listener suppresses Node's default exit; change the
listeners to one-time handlers and explicitly exit after cleanup: replace the
process.on('SIGTERM', ...) and process.on('SIGINT', ...) uses with process.once
and use a named handler that calls clearInterval(pollingIntervalId), then for
SIGINT call process.exit(0) (or process.exit with appropriate code) so the
process exits as expected; refer to createWsServer and pollingIntervalId when
making this change.
---
Nitpick comments:
In `@src/routes/settings.tsx`:
- Around line 86-120: The WebSocket created inside the useEffect (the socket
variable in the IP auto-detection useEffect) lacks onerror/onclose handlers and
reconnection/backoff logic; add onerror and onclose callbacks that attempt to
reconnect with exponential backoff (and optional jitter) up to a reasonable max
retry count, ensure you clear any timers and stop retrying on component unmount,
avoid opening duplicate sockets (check/track an active socket or a reconnecting
flag), and reuse existing message parsing logic (setIp) when a reconnect
succeeds so IP/QR updates resume correctly.
- Around line 100-112: The code handling WebSocket messages in socket.onmessage
contains a dead fallback to data.serverIp; update the logic to only use data.ip
for both 'server-ip' and 'connected' message types, remove references to
data.serverIp to avoid confusion, and ensure you still call setIp(data.ip) and
log the received IP via the existing console log; adjust the conditional to
check (data.type === 'server-ip' || data.type === 'connected') && data.ip and
remove the unused data.serverIp branch.
In `@src/server/websocket.ts`:
- Around line 46-53: Extract the hardcoded './src/server-config.json' string
into a single exported constant CONFIG_PATH (computed with
path.resolve(__dirname, '..', 'server-config.json') or similar to avoid CWD
fragility) and replace both occurrences in this file where the config is read
(the block that sets pollingInterval and the other location around lines
119-135) with CONFIG_PATH; keep the existing fs.existsSync and JSON.parse logic
but reference CONFIG_PATH instead of the literal string so the path is defined
once and used consistently.
|
Hi @imxade , I noticed this PR was closed. |
Implement Automatic Network IP & QR Code Update
Fixes #51
Description
This PR addresses the issue where the application's displayed IP address and QR code remained stale when the host machine switched networks (e.g., changing WiFi or connecting to a hotspot). Previously, the IP was only fetched on initial load, requiring a manual refresh to update the connection details.
Changes Made
1. Server-side (Backend)
src/server/websocket.tsto implement a network interface polling mechanism.POLLING_INTERVAL(5 seconds) that checks for IP changes usingos.networkInterfaces().2. Frontend (UI)
src/routes/settings.tsxto maintain a persistent WebSocket connection for IP tracking.server-ipupdate messages and updates the local state in real-time.How to Test
npm run dev.http://localhost:3000/settings).Checklist
npm run build).Summary by CodeRabbit
New Features
Improvements