diff --git a/WebATM/proxy/managers/data_manager.py b/WebATM/proxy/managers/data_manager.py index 4026b31..bf6e89a 100644 --- a/WebATM/proxy/managers/data_manager.py +++ b/WebATM/proxy/managers/data_manager.py @@ -204,8 +204,6 @@ def get_current_data(self) -> dict[str, Any]: else: logger.debug(" No active node - not including any shapes in initial data") - from ...bluesky_client import safe_decode - return { "traffic_data": self.proxy.traffic_data, "sim_data": self.proxy.sim_data, @@ -218,13 +216,7 @@ def get_current_data(self) -> dict[str, Any]: "server_ip": self.proxy.server_ip, "last_update": self.proxy.last_successful_update, }, - "node_info": { - "nodes": self.proxy.tracked_nodes.copy(), - "servers": { - safe_decode(k): v for k, v in self.proxy.tracked_servers.items() - }, - "active_node": active_node_id, - "total_nodes": len(self.proxy.tracked_nodes), - }, + # Same serialized shape as the node_info event (no raw bytes). + "node_info": self.proxy.node_mgr.serialize_node_info(), "timestamp": time.time(), } diff --git a/WebATM/proxy/managers/node_manager.py b/WebATM/proxy/managers/node_manager.py index e3f69d2..4a9e20d 100644 --- a/WebATM/proxy/managers/node_manager.py +++ b/WebATM/proxy/managers/node_manager.py @@ -2,6 +2,7 @@ import threading import time +import traceback from ...bluesky_client import safe_decode, seqid2idx, seqidx2id from ...logger import get_logger @@ -37,8 +38,7 @@ def _get_safe_active_node(self): return None try: - # act_id is raw bytes in the network client; tracked_nodes is - # keyed by the hex-string form. + # act_id is raw bytes; tracked_nodes is keyed by hex strings. active_id_str = id2str(self.proxy.bluesky_client.act_id) if active_id_str in self.proxy.tracked_nodes: return active_id_str @@ -65,8 +65,6 @@ def _emit_active_node_poly_data(self): ) except Exception as e: logger.error(f" Error emitting active node POLY/POLYLINE data: {e}") - import traceback - traceback.print_exc() def _emit_shapes(self, active_node_id, data_by_node, event): @@ -89,27 +87,26 @@ def _emit_shapes(self, active_node_id, data_by_node, event): logger.debug(f"Emitted empty {event} data to clear") def _on_node_added(self, node_id): - """Callback when a new node is discovered.""" + """Callback when a new node is discovered. + + Tracks the node (keyed by hex string, matching SIMINFO sender IDs), + flips the proxy to connected on the first node, and re-activates the + client when its recorded active node no longer exists. + """ try: - # Hex string keys for consistency with SIMINFO node_id_str = id2str(node_id) if node_id_str not in self.proxy.tracked_nodes: server_id = node_id[:-1] + seqidx2id(0) - if ( - server_id not in self.proxy.bluesky_client.servers - ): # Check standalone proxy's known servers + if server_id not in self.proxy.bluesky_client.servers: server_id = b"0" # Ungrouped if server_id not in self.proxy.tracked_servers: self._on_server_added(server_id) - node_num = seqid2idx(node_id[-1]) - - # Store using hex string key for consistency with SIMINFO self.proxy.tracked_nodes[node_id_str] = { - "node_id": node_id, # Keep original binary for internal use - "node_id_str": node_id_str, # Hex string for display - "node_num": node_num, + "node_id": node_id, # original binary, for internal use + "node_id_str": node_id_str, + "node_num": seqid2idx(node_id[-1]), "server_id": server_id, "status": "init", "time": "00:00:00", @@ -119,36 +116,49 @@ def _on_node_added(self, node_id): f"Node {safe_decode(node_id)} added (total: {len(self.proxy.tracked_nodes)})" ) - # Update connection status immediately when nodes are detected - if not self.proxy.was_connected and len(self.proxy.tracked_nodes) > 0: + self._reactivate_if_active_node_gone(node_id) + + if not self.proxy.was_connected: self.proxy.was_connected = True - # Start the data-flow timeout clock from "first node - # appeared" (no data could arrive before a node existed). - # This handler runs inside bluesky_client.update() and flips - # was_connected first, so the network timer's own reset (only - # runs while `not was_connected`) is skipped — reset here too, - # else the stale start_client() timestamp times out at once. + # Restart the data-flow timeout clock from "first node + # appeared". This runs inside bluesky_client.update() and + # flips was_connected before the network timer's own reset + # (guarded on `not was_connected`) can run, so without this + # a stale start_client() timestamp times out immediately. self.proxy.last_successful_update = time.time() logger.info(" Connection established") self.proxy._emit_connection_status(True) - # The standalone proxy auto-selects the first node, so we don't need to do it manually here - # Emit updated node list to connected clients if self.proxy.running: self._emit_node_info() except Exception as e: logger.error(f" Error in _on_node_added: {e}") - import traceback - traceback.print_exc() + def _reactivate_if_active_node_gone(self, node_id): + """Activate a newly discovered node if the client's active node is dead. + + The network client auto-selects only the very first node it ever sees; + after that, ``_failover_active_node`` re-activates a survivor when the + active node is removed. But when the last node vanishes (e.g. its + process crashes) there is no survivor: ``act_id`` keeps pointing at the + dead node, and a node added afterwards would never be activated. The + actonly topics (ACDATA/ROUTEDATA) would stay subscribed to the dead + node, no traffic would flow, and the data-flow timeout would tear down + a live connection. Activating the newcomer here re-subscribes those + topics and resumes the data flow. + """ + client = self.proxy.bluesky_client + if client.act_id is None or client.act_id not in client.nodes: + logger.info( + f"No live active node; activating new node {safe_decode(node_id)}" + ) + client.actnode(node_id) + def _on_server_added(self, server_id): """Callback when a server is discovered.""" if server_id not in self.proxy.tracked_servers: - # Simple server tracking - just store the ID self.proxy.tracked_servers[server_id] = {"server_id": server_id} - - # Emit updated server list to connected clients if self.proxy.running: self._emit_node_info() @@ -160,15 +170,14 @@ def _on_node_removed(self, node_id): self._failover_active_node(node_id) self._emit_node_info() - # Check if all nodes have been removed - this indicates server shutdown - # Add a small delay to avoid false positives during normal node transitions + # All nodes gone usually means server shutdown; re-check after a short + # delay to avoid false positives during normal node transitions. if ( len(self.proxy.tracked_nodes) == 0 and self.proxy.was_connected and self.proxy.running ): logger.warning(" All nodes removed - checking for server shutdown...") - # Use a timer to check again in a moment to confirm it's really a shutdown threading.Timer(1.0, self._check_node_shutdown).start() def _failover_active_node(self, removed_node_id): @@ -213,51 +222,52 @@ def _on_server_removed(self, server_id): del self.proxy.tracked_servers[server_id] self._emit_node_info() + def serialize_node_info(self): + """Build the JSON-serializable ``node_info`` payload. + + Decodes the binary node/server IDs kept in ``tracked_nodes`` and + ``tracked_servers`` into the string forms the frontend expects + (``NodeData`` in ``frontend/src/data/types.ts``). Used for both the + ``node_info`` Socket.IO event and the ``initial_data`` snapshot. + + Returns: + dict: ``nodes``, ``servers``, ``active_node`` and ``total_nodes``. + """ + nodes_data = {} + for node_id_str, tracked in self.proxy.tracked_nodes.items(): + node_data = tracked.copy() + if "node_id" in node_data: + node_data["node_id"] = safe_decode(node_data["node_id"]) + if "server_id" in node_data: + raw_server_id = node_data["server_id"] + node_data["server_id"] = safe_decode(raw_server_id) + node_data["server_id_hex"] = id2str(raw_server_id) + node_data["server_id_raw"] = str(raw_server_id) + nodes_data[node_id_str] = node_data + + servers_data = {} + for server_id, tracked in self.proxy.tracked_servers.items(): + server_data = tracked.copy() + if "server_id" in server_data: + server_data["server_id"] = safe_decode(server_data["server_id"]) + servers_data[safe_decode(server_id)] = server_data + + return { + "nodes": nodes_data, + "servers": servers_data, + "active_node": self._get_safe_active_node(), + "total_nodes": len(self.proxy.tracked_nodes), + } + def _emit_node_info(self): """Emit current node and server information to connected clients.""" - if self.proxy.socketio and self.proxy.connected_clients > 0: - try: - # Convert node data for JSON serialization - nodes_data = {} - for k, v in self.proxy.tracked_nodes.items(): - # k is already a hex string, v contains node data - # Make a copy and ensure all values are JSON serializable - node_data = v.copy() - if "node_id" in node_data: - node_data["node_id"] = safe_decode(node_data["node_id"]) - if "server_id" in node_data: - # Include decoded, hex, and raw server ID - raw_server_id = node_data["server_id"] - node_data["server_id"] = safe_decode(raw_server_id) - node_data["server_id_hex"] = id2str(raw_server_id) - node_data["server_id_raw"] = str( - raw_server_id - ) # Raw byte string representation - nodes_data[k] = node_data # k is already the hex string - - servers_data = {} - for k, v in self.proxy.tracked_servers.items(): - key = safe_decode(k) - server_data = v.copy() - if "server_id" in server_data: - server_data["server_id"] = safe_decode(server_data["server_id"]) - servers_data[key] = server_data - - # Get active node safely - active_node = self._get_safe_active_node() - - node_info = { - "nodes": nodes_data, - "servers": servers_data, - "active_node": active_node, - "total_nodes": len(self.proxy.tracked_nodes), - } - self.proxy.socketio.emit("node_info", node_info) - except Exception as e: - logger.error(f" Error emitting node info: {e}") - import traceback - - traceback.print_exc() + if not (self.proxy.socketio and self.proxy.connected_clients > 0): + return + try: + self.proxy.socketio.emit("node_info", self.serialize_node_info()) + except Exception as e: + logger.error(f" Error emitting node info: {e}") + traceback.print_exc() def actnode(self, node_id): """Select the active simulation node via the network client. diff --git a/frontend/src/core/App.ts b/frontend/src/core/App.ts index 1ca02b1..ba3c380 100644 --- a/frontend/src/core/App.ts +++ b/frontend/src/core/App.ts @@ -373,13 +373,15 @@ export class App { this.navaidSnapper ); - // Let AircraftInteractionManager know when route drawing is active so - // it can skip its empty-map-click "unselect aircraft" behavior while - // waypoints are being placed. - if (this.aircraftInteractionManager && this.routeDrawingManager) { + // Let AircraftInteractionManager know when a drawing tool is active + // so it can skip its empty-map-click "unselect aircraft" behavior + // while points are being placed. + if (this.aircraftInteractionManager) { const rdm = this.routeDrawingManager; - this.aircraftInteractionManager.setRouteDrawingActiveCheck( - () => rdm.isDrawing() + const sdm = this.shapeDrawingManager; + const acm = this.aircraftCreationManager; + this.aircraftInteractionManager.setDrawingToolActiveCheck( + () => !!(rdm?.isDrawing() || sdm?.isDrawing() || acm?.isDrawing()) ); } diff --git a/frontend/src/core/ConnectionStatusService.test.ts b/frontend/src/core/ConnectionStatusService.test.ts index 07831ad..ae64365 100644 --- a/frontend/src/core/ConnectionStatusService.test.ts +++ b/frontend/src/core/ConnectionStatusService.test.ts @@ -76,7 +76,6 @@ describe('ConnectionStatusService', () => { service.setWebSocketConnected(true); const status = service.getStatus(); expect(status.webSocketConnected).toBe(true); - expect(status.webSocketState).toBe('connected'); expect(echoManager.success).toHaveBeenCalledWith('Connected to WebATM server'); }); @@ -98,6 +97,17 @@ describe('ConnectionStatusService', () => { expect(service.isBlueSkyConnected()).toBe(false); expect(service.isFullyConnected()).toBe(false); }); + + it('a WebSocket disconnect also clears receivingData', () => { + // Regression: receivingData used to stay true forever after a + // WebSocket drop, because the guarded timeout path skipped it. + service.setWebSocketConnected(true); + service.onSimInfoReceived(); + expect(service.isReceivingData()).toBe(true); + + service.setWebSocketConnected(false); + expect(service.isReceivingData()).toBe(false); + }); }); describe('BlueSky state via data reception', () => { @@ -126,6 +136,71 @@ describe('ConnectionStatusService', () => { expect.stringContaining('No data received')); }); + it('an explicit disconnect cancels the pending no-data timer', () => { + // Regression: after a deliberate disconnect (QUIT, + // server_disconnected, Stop in the integrated build) the armed + // 5s timer used to fire anyway and echo a spurious + // "No data received ... connection may be lost" warning. + service.onSimInfoReceived(); // arms the 5s timer + + service.setBlueSkyConnected(false); // e.g. QUIT + expect(service.isReceivingData()).toBe(false); + + (echoManager.warning as ReturnType).mockClear(); + vi.advanceTimersByTime(10000); + expect(echoManager.warning).not.toHaveBeenCalledWith( + expect.stringContaining('No data received')); + }); + + it('a deliberate disconnect ignores in-flight data during the grace window', () => { + // Regression: after QUIT, data events already buffered in the + // socket flipped the status straight back to "Connected" and + // re-armed the timer, producing a phantom connect flash and a + // spurious warning 5s later. + const nowSpy = vi.spyOn(Date, 'now'); + nowSpy.mockReturnValue(1000); + service.onSimInfoReceived(); + + service.expectDisconnect(); // QUIT + service.setBlueSkyConnected(false); + + // A trailing acdata event arrives 100ms later - ignored. + nowSpy.mockReturnValue(1100); + service.onAircraftDataReceived(); + expect(service.isBlueSkyConnected()).toBe(false); + + (echoManager.warning as ReturnType).mockClear(); + vi.advanceTimersByTime(10000); + expect(echoManager.warning).not.toHaveBeenCalledWith( + expect.stringContaining('No data received')); + }); + + it('data still flowing after the grace window reconnects as usual', () => { + const nowSpy = vi.spyOn(Date, 'now'); + nowSpy.mockReturnValue(1000); + service.onSimInfoReceived(); + + service.expectDisconnect(); // disconnect request that failed + service.setBlueSkyConnected(false); + + // Data keeps arriving well past the 2s grace window. + nowSpy.mockReturnValue(4000); + service.onAircraftDataReceived(); + expect(service.isBlueSkyConnected()).toBe(true); + }); + + it('exposes the grace window so data consumers can drop in-flight frames', () => { + const nowSpy = vi.spyOn(Date, 'now'); + nowSpy.mockReturnValue(1000); + expect(service.isInDisconnectGrace()).toBe(false); + + service.expectDisconnect(); + expect(service.isInDisconnectGrace()).toBe(true); + + nowSpy.mockReturnValue(4000); // past the 2s default + expect(service.isInDisconnectGrace()).toBe(false); + }); + it('continuous data keeps the connection alive past the timeout window', () => { service.onSimInfoReceived(); for (let i = 0; i < 5; i++) { diff --git a/frontend/src/core/ConnectionStatusService.ts b/frontend/src/core/ConnectionStatusService.ts index ecabfa7..7aec558 100644 --- a/frontend/src/core/ConnectionStatusService.ts +++ b/frontend/src/core/ConnectionStatusService.ts @@ -1,16 +1,9 @@ /** - * Connection Status Service + * Centralized singleton service holding every connection state: the WebSocket + * to the WebATM server, the BlueSky server connection, and data reception. * - * Centralized singleton service for managing all connection states in the application. - * This service tracks: - * - WebSocket connection to WebATM server - * - BlueSky server connection status (derived from receiving data) - * - Data reception status - * - * The connection status should be available across all TypeScript classes. - * - * Key concept: As long as we're receiving any data (nodeinfo, siminfo, or acdata), - * we're connected to BlueSky server. If no data is received for DATA_TIMEOUT_MS, + * Key concept: receiving any server data (nodeinfo, siminfo, acdata, shapes) + * proves the BlueSky connection is up. If nothing arrives for DATA_TIMEOUT_MS, * the connection is considered lost. */ @@ -18,16 +11,12 @@ import { echoManager } from '../ui/EchoManager'; import { logger } from '../utils/Logger'; import { EventEmitter } from '../utils/events'; -export type ConnectionState = 'disconnected' | 'connecting' | 'connected' | 'error'; - export interface ConnectionStatusData { // WebSocket connection to WebATM webSocketConnected: boolean; - webSocketState: ConnectionState; - // BlueSky server connection (determined by nodeinfo receipt) + // BlueSky server connection (derived from receiving data) blueSkyConnected: boolean; - blueSkyState: ConnectionState; // Data flow indicators receivingData: boolean; @@ -45,18 +34,14 @@ type ConnectionStatusListener = (status: ConnectionStatusData) => void; type ConnectionEventCallback = (connected: boolean) => void; /** - * Centralized Connection Status Service - * - * This service maintains the single source of truth for all connection states. + * Single source of truth for all connection states. */ export class ConnectionStatusService { private static instance: ConnectionStatusService | null = null; private status: ConnectionStatusData = { webSocketConnected: false, - webSocketState: 'disconnected', blueSkyConnected: false, - blueSkyState: 'disconnected', receivingData: false, lastDataReceived: null, lastNodeInfoReceived: null, @@ -68,23 +53,23 @@ export class ConnectionStatusService { private dataTimeoutId: number | null = null; private readonly DATA_TIMEOUT_MS = 5000; // Consider disconnected if no data for 5 seconds - // Wall-clock time the active no-data timeout was armed, plus how many times - // we have deferred a disconnect because that timeout fired late. A - // setTimeout that fires far past its delay means the event loop was blocked - // (a heavy synchronous task such as loading a large GeoJSON, a long GC - // pause, or a backgrounded tab) - during that window the socket cannot - // deliver data even when the server is perfectly healthy, so "no data" is - // not evidence of a real disconnect. We defer (bounded) and let live data - // confirm the connection instead of flashing a false "BlueSky disconnected". + // A setTimeout that fires far past its delay means the event loop was + // blocked (heavy synchronous work, GC pause, backgrounded tab). During + // that window the socket couldn't deliver data anyway, so "no data" is + // not evidence of a real disconnect - we defer (bounded) and let live + // data confirm the connection instead of flashing a false disconnect. private dataTimeoutArmedAt = 0; private stallDeferrals = 0; private readonly MAX_STALL_DEFERRALS = 2; private readonly STALL_OVERSHOOT_MS = 1500; + // End of the deliberate-disconnect grace window (see expectDisconnect). + private ignoreDataUntil = 0; + // Initial connection tracking private isInitialLoadComplete: boolean = false; private initialConnectionCheckTimer: number | null = null; - private readonly INITIAL_CONNECTION_CHECK_DELAY_MS = 500; // Wait 0.5s before checking initial connection + private readonly INITIAL_CONNECTION_CHECK_DELAY_MS = 500; // Connection event callbacks private blueSkyDisconnectEmitter = new EventEmitter('ConnectionStatus.disconnect'); @@ -123,9 +108,6 @@ export class ConnectionStatusService { return unsubscribe; } - /** - * Notify all listeners of status change - */ private notifyListeners(): void { this.statusEmitter.emit(this.getStatus()); } @@ -147,12 +129,10 @@ export class ConnectionStatusService { public setWebSocketConnected(connected: boolean): void { const changed = this.status.webSocketConnected !== connected; this.status.webSocketConnected = connected; - this.status.webSocketState = connected ? 'connected' : 'disconnected'; if (changed) { logger.info('ConnectionStatus', `WebSocket: ${connected ? 'connected' : 'disconnected'}`); - // Log to echo messages if (connected) { echoManager.success('Connected to WebATM server'); } else { @@ -168,44 +148,35 @@ export class ConnectionStatusService { } } - /** - * Set WebSocket to connecting state - */ - public setWebSocketConnecting(): void { - this.status.webSocketState = 'connecting'; - echoManager.info('⟳ Connecting to WebATM server...'); - this.notifyListeners(); - } - - /** - * Set WebSocket to error state - */ - public setWebSocketError(): void { - this.status.webSocketState = 'error'; - this.status.webSocketConnected = false; - echoManager.error('✗ WebATM server connection error'); - this.notifyListeners(); - } - // ======================================== // BlueSky Server Connection Methods // ======================================== /** - * Update BlueSky server connection status - * This should be called when nodeinfo is received or connection is explicitly lost + * Update BlueSky server connection status. + * Called when data is received, or when the connection is known to be + * down (explicit disconnect, server_disconnected, WebSocket drop). */ public setBlueSkyConnected(connected: boolean): void { const changed = this.status.blueSkyConnected !== connected; const wasConnected = this.status.blueSkyConnected; this.status.blueSkyConnected = connected; - this.status.blueSkyState = connected ? 'connected' : 'disconnected'; + + if (!connected) { + // The connection is known to be down: the pending no-data timer + // is now meaningless. Without this it fires up to DATA_TIMEOUT_MS + // later and echoes a false "no data received" warning after a + // deliberate disconnect (e.g. QUIT), and receivingData would + // stay stale after a WebSocket drop. + this.clearDataTimeout(); + this.stallDeferrals = 0; + this.setReceivingData(false); + } if (changed) { logger.info('ConnectionStatus', `BlueSky: ${connected ? 'connected' : 'disconnected'}`); - // Log to echo messages if (connected) { echoManager.success('Connected to BlueSky server'); } else { @@ -222,12 +193,24 @@ export class ConnectionStatusService { } /** - * Set BlueSky to connecting state + * A deliberate disconnect (QUIT, server_disconnected) is happening: data + * events already in flight would otherwise flip the status straight back + * to connected and re-arm the no-data timer, which then fires a spurious + * "connection may be lost" warning 5s after a clean disconnect. Ignore + * data for a short grace window; data still flowing after it (e.g. the + * disconnect request failed) reconnects as usual. */ - public setBlueSkyConnecting(): void { - this.status.blueSkyState = 'connecting'; - echoManager.info('⟳ Connecting to BlueSky server...'); - this.notifyListeners(); + public expectDisconnect(graceMs: number = 2000): void { + this.ignoreDataUntil = Date.now() + graceMs; + } + + /** + * True while the deliberate-disconnect grace window is open. Lets data + * consumers (e.g. SocketManager) drop in-flight frames that would + * repaint the traffic the disconnect just cleared. + */ + public isInDisconnectGrace(): boolean { + return Date.now() < this.ignoreDataUntil; } /** @@ -237,6 +220,10 @@ export class ConnectionStatusService { * the receivingData flag. */ private onServerDataReceived(kind: string, marksReceivingData: boolean): void { + if (Date.now() < this.ignoreDataUntil) { + logger.debug('ConnectionStatus', `${kind} ignored - disconnect grace window`); + return; + } logger.debug('ConnectionStatus', `${kind} received`); if (!this.status.blueSkyConnected) { @@ -254,12 +241,10 @@ export class ConnectionStatusService { /** * Called when nodeinfo is received - * This is an indicator that we're connected to BlueSky server */ public onNodeInfoReceived(): void { const now = Date.now(); - // Calculate interval between nodeinfo messages if (this.status.lastNodeInfoReceived !== null) { this.status.nodeInfoInterval = now - this.status.lastNodeInfoReceived; } @@ -270,7 +255,6 @@ export class ConnectionStatusService { /** * Called when simulation info (siminfo) is received - * This is a strong indicator that we're connected and receiving data */ public onSimInfoReceived(): void { this.status.lastDataReceived = Date.now(); @@ -279,7 +263,6 @@ export class ConnectionStatusService { /** * Called when aircraft data (acdata) is received - * This is a strong indicator that we're connected and receiving data */ public onAircraftDataReceived(): void { this.status.lastDataReceived = Date.now(); @@ -288,7 +271,6 @@ export class ConnectionStatusService { /** * Called when shape data (poly/polyline) is received - * This is an indicator that we're connected to BlueSky server */ public onShapeDataReceived(): void { this.status.lastDataReceived = Date.now(); @@ -297,7 +279,6 @@ export class ConnectionStatusService { /** * Reset the timeout that detects when we stop receiving any data - * This is called when we receive nodeinfo, siminfo, or acdata */ private resetDataTimeout(): void { // Fresh data clears any stall-deferral budget and re-arms the timer. @@ -307,9 +288,7 @@ export class ConnectionStatusService { /** (Re)arm the no-data timeout, recording when it was armed. */ private armDataTimeout(): void { - if (this.dataTimeoutId !== null) { - window.clearTimeout(this.dataTimeoutId); - } + this.clearDataTimeout(); this.dataTimeoutArmedAt = Date.now(); this.dataTimeoutId = window.setTimeout( () => this.onDataTimeoutExpired(), @@ -317,16 +296,23 @@ export class ConnectionStatusService { ); } + /** Cancel the pending no-data timeout, if any. */ + private clearDataTimeout(): void { + if (this.dataTimeoutId !== null) { + window.clearTimeout(this.dataTimeoutId); + this.dataTimeoutId = null; + } + } + /** - * Called when the no-data timeout fires. Distinguishes a genuine server - * silence from a local main-thread stall: if the callback ran far later - * than its scheduled delay, the page was frozen (so no data could have been - * received regardless of server health). In that case we defer the - * disconnect and re-arm, giving live data a chance to reset the timer. - * Bounded by MAX_STALL_DEFERRALS so a persistently blocked loop still - * eventually reports a real outage. + * Called when the no-data timeout fires. A callback that ran far later + * than its scheduled delay means the page was frozen, so the silence is + * a local stall rather than a server outage: defer the disconnect and + * re-arm (bounded by MAX_STALL_DEFERRALS). Otherwise treat the silence + * as a real disconnect. */ private onDataTimeoutExpired(): void { + this.dataTimeoutId = null; const overshoot = Date.now() - this.dataTimeoutArmedAt - this.DATA_TIMEOUT_MS; if (overshoot > this.STALL_OVERSHOOT_MS && this.stallDeferrals < this.MAX_STALL_DEFERRALS) { @@ -340,14 +326,11 @@ export class ConnectionStatusService { return; } - // Either the timer fired on schedule (a real silence) or we have - // deferred as long as we are willing to - treat it as a disconnect. this.stallDeferrals = 0; - logger.warn('ConnectionStatus', 'No data received (nodeinfo, siminfo, or acdata) - BlueSky may be disconnected'); - echoManager.warning('⚠ No data received from BlueSky server - connection may be lost'); if (this.status.blueSkyConnected) { + logger.warn('ConnectionStatus', 'No data received (nodeinfo, siminfo, or acdata) - BlueSky may be disconnected'); + echoManager.warning('⚠ No data received from BlueSky server - connection may be lost'); this.setBlueSkyConnected(false); - this.setReceivingData(false); } } @@ -357,7 +340,6 @@ export class ConnectionStatusService { /** * Update data reception status - * Called when simulation data (siminfo, acdata) is received */ public setReceivingData(receiving: boolean): void { const changed = this.status.receivingData !== receiving; @@ -373,17 +355,6 @@ export class ConnectionStatusService { } } - /** - * Called when any simulation data is received - */ - public onDataReceived(): void { - this.status.lastDataReceived = Date.now(); - - if (!this.status.receivingData) { - this.setReceivingData(true); - } - } - // ======================================== // Server Configuration Methods // ======================================== @@ -410,13 +381,6 @@ export class ConnectionStatusService { return this.status.webSocketConnected && this.status.blueSkyConnected; } - /** - * Check if WebSocket is connected - */ - public isWebSocketConnected(): boolean { - return this.status.webSocketConnected; - } - /** * Check if BlueSky is connected */ @@ -460,15 +424,9 @@ export class ConnectionStatusService { return 'Disconnected from BlueSky server. Please visit settings and make sure that (1) BlueSky server has been started and (2) You are connected to server.'; } - if (this.status.blueSkyConnected && this.status.receivingData) { - return `Connected to BlueSky server at ${this.status.serverIP}.`; - } - - if (this.status.blueSkyConnected && !this.status.receivingData) { - return `Connected to BlueSky server at ${this.status.serverIP} (No Data).`; - } - - return 'Unknown connection status'; + return this.status.receivingData + ? `Connected to BlueSky server at ${this.status.serverIP}.` + : `Connected to BlueSky server at ${this.status.serverIP} (No Data).`; } /** @@ -479,9 +437,7 @@ export class ConnectionStatusService { this.status = { webSocketConnected: false, - webSocketState: 'disconnected', blueSkyConnected: false, - blueSkyState: 'disconnected', receivingData: false, lastDataReceived: null, lastNodeInfoReceived: null, @@ -489,30 +445,24 @@ export class ConnectionStatusService { nodeInfoInterval: null }; - if (this.dataTimeoutId !== null) { - window.clearTimeout(this.dataTimeoutId); - this.dataTimeoutId = null; - } + this.clearDataTimeout(); this.stallDeferrals = 0; this.dataTimeoutArmedAt = 0; + this.ignoreDataUntil = 0; logger.info('ConnectionStatus', 'Reset all connection states'); this.notifyListeners(); } /** - * Get detailed status for debugging + * Get detailed status for debugging (window.connectionStatus helper) */ public getDetailedStatus(): string { const status = this.getStatus(); return JSON.stringify({ - webSocket: { - connected: status.webSocketConnected, - state: status.webSocketState - }, + webSocket: { connected: status.webSocketConnected }, blueSky: { connected: status.blueSkyConnected, - state: status.blueSkyState, lastNodeInfo: status.lastNodeInfoReceived ? `${Date.now() - status.lastNodeInfoReceived}ms ago` : 'never', interval: status.nodeInfoInterval ? `${status.nodeInfoInterval}ms` : 'unknown' @@ -532,16 +482,13 @@ export class ConnectionStatusService { // Initial Connection Checking Methods // ======================================== - /** - * Load initial load state from sessionStorage - */ private loadInitialLoadState(): void { this.isInitialLoadComplete = sessionStorage.getItem('bluesky-initial-load-complete') === 'true'; } /** - * Start initial connection check - * This checks if we're connected after a delay on first page load + * On the first page load of a session, invoke onNotConnected if no + * BlueSky connection is established shortly after startup. */ public startInitialConnectionCheck(onNotConnected: () => void): void { if (this.isInitialLoadComplete) { @@ -582,21 +529,11 @@ export class ConnectionStatusService { }); } - /** - * Mark initial load as complete - */ private markInitialLoadComplete(): void { this.isInitialLoadComplete = true; sessionStorage.setItem('bluesky-initial-load-complete', 'true'); } - /** - * Check if initial load is complete - */ - public isInitialLoad(): boolean { - return !this.isInitialLoadComplete; - } - // ======================================== // Connection Event Callbacks // ======================================== @@ -609,9 +546,6 @@ export class ConnectionStatusService { return this.blueSkyDisconnectEmitter.subscribe(callback); } - /** - * Trigger all disconnect callbacks - */ private triggerDisconnectCallbacks(): void { logger.debug('ConnectionStatus', 'Triggering disconnect callbacks'); this.blueSkyDisconnectEmitter.emit(false); diff --git a/frontend/src/core/SocketManager.ts b/frontend/src/core/SocketManager.ts index c55b50d..b7931e3 100644 --- a/frontend/src/core/SocketManager.ts +++ b/frontend/src/core/SocketManager.ts @@ -119,10 +119,14 @@ export class SocketManager { } }, onSimInfo: (data: SimInfo) => { + if (connectionStatus.isInDisconnectGrace()) return; this.stateManager.updateSimInfo(data); connectionStatus.onSimInfoReceived(); }, onAircraftData: (data: AircraftData) => { + // In-flight frames arriving right after a deliberate + // disconnect would repaint the traffic that was just cleared. + if (connectionStatus.isInDisconnectGrace()) return; this.stateManager.updateAircraftData(data); connectionStatus.onAircraftDataReceived(); }, @@ -161,8 +165,11 @@ export class SocketManager { } }, onServerDisconnected: () => { + // Also clears receivingData and the pending no-data timer; + // the grace window keeps in-flight data events from flipping + // the status straight back to connected. + connectionStatus.expectDisconnect(); connectionStatus.setBlueSkyConnected(false); - connectionStatus.setReceivingData(false); }, onReset: () => { // RESET clears simulation data only - we stay connected to BlueSky. diff --git a/frontend/src/data/CommandHandler.test.ts b/frontend/src/data/CommandHandler.test.ts index e70751b..821e4db 100644 --- a/frontend/src/data/CommandHandler.test.ts +++ b/frontend/src/data/CommandHandler.test.ts @@ -15,7 +15,7 @@ vi.mock('../ui/EchoManager', () => ({ })); vi.mock('../core/ConnectionStatusService', () => ({ - connectionStatus: { setBlueSkyConnected: vi.fn() }, + connectionStatus: { setBlueSkyConnected: vi.fn(), expectDisconnect: vi.fn() }, })); const addMessage = vi.mocked(echoManager.addMessage); @@ -230,7 +230,9 @@ describe('CommandHandler', () => { '/api/server/disconnect', expect.objectContaining({ method: 'POST' }), ); - // ...reflects it immediately in the shared connection status... + // ...reflects it immediately in the shared connection status, + // shielding it from data events already in flight... + expect(connectionStatus.expectDisconnect).toHaveBeenCalled(); expect(setBlueSkyConnected).toHaveBeenCalledWith(false); // ...and crucially does NOT drop the browser↔WebATM socket. expect(mocks.socketManager.disconnect).not.toHaveBeenCalled(); diff --git a/frontend/src/data/CommandHandler.ts b/frontend/src/data/CommandHandler.ts index 6ef4002..2534c1f 100644 --- a/frontend/src/data/CommandHandler.ts +++ b/frontend/src/data/CommandHandler.ts @@ -635,7 +635,9 @@ export class CommandHandler { // Reflect the explicit disconnect immediately in the single source of // truth the header — and, in the integrated build, the server-control // status — both read, so the indicators agree without waiting on the - // data-flow timeout. + // data-flow timeout. expectDisconnect() keeps data events already in + // flight from flipping the status right back to connected. + connectionStatus.expectDisconnect(); connectionStatus.setBlueSkyConnected(false); this.sendEcho('Disconnected from BlueSky server (the server itself is left running)', 'info'); diff --git a/frontend/src/ui/SettingsModal.ts b/frontend/src/ui/SettingsModal.ts index 05dfbb8..199f6e2 100644 --- a/frontend/src/ui/SettingsModal.ts +++ b/frontend/src/ui/SettingsModal.ts @@ -410,6 +410,9 @@ export class SettingsModal { if (result.success) { this.saveServerIP(serverIp); + // Reflect the real host in the header status string, which + // otherwise reports its 'localhost' default forever. + connectionStatus.setServerIP(serverIp); this.close(); // Move focus back to command console diff --git a/frontend/src/ui/map/BaseDrawingManager.test.ts b/frontend/src/ui/map/BaseDrawingManager.test.ts index 5fb1d65..537e038 100644 --- a/frontend/src/ui/map/BaseDrawingManager.test.ts +++ b/frontend/src/ui/map/BaseDrawingManager.test.ts @@ -57,7 +57,11 @@ class TestDrawingManager extends BaseDrawingManager { protected onPointAdded(point: DrawingPoint): void { this.points.push(point); } protected onCursorMove(point: DrawingPoint): void { this.cursorMoves.push(point); } protected finishDrawing(): void { this.finished(); } - protected cancelDrawing(): void { this.cancelled(); } + // Mirrors the real subclasses: cancelling stops the draw. + protected cancelDrawing(): void { + this.cancelled(); + this.stop(); + } } describe('BaseDrawingManager', () => { @@ -68,10 +72,14 @@ describe('BaseDrawingManager', () => { const clickEvent = (lat: number, lng: number) => ({ lngLat: { lat, lng }, preventDefault: vi.fn() }) as unknown as MapMouseEvent; - function createManager(finishOnEnter = false): TestDrawingManager { - map = createFakeMap(); + function createManager( + finishOnEnter = false, + getMap?: () => ReturnType | null + ): TestDrawingManager { + const ownMap = createFakeMap(); + map = ownMap; snapper = { snap: vi.fn(() => null), highlight: vi.fn(), clearHighlight: vi.fn() }; - const mapDisplay = { getMap: () => map } as unknown as MapDisplay; + const mapDisplay = { getMap: getMap ?? (() => ownMap) } as unknown as MapDisplay; return new TestDrawingManager(mapDisplay, snapper as unknown as NavaidSnapper, finishOnEnter); } @@ -178,4 +186,52 @@ describe('BaseDrawingManager', () => { idle.destroy(); expect(idle.cancelled).not.toHaveBeenCalled(); }); + + it('starting a second drawing tool cancels the first (mutual exclusion)', () => { + const first = createManager(); + const firstMap = map; + first.start(); + + const second = createManager(); + second.start(); + + expect(first.cancelled).toHaveBeenCalledTimes(1); + expect(first.isDrawing()).toBe(false); + expect(second.isDrawing()).toBe(true); + + // Clicks reach only the second tool now. + firstMap.fire('click', clickEvent(51, 3)); + map.fire('click', clickEvent(52, 4)); + expect(first.points).toEqual([]); + expect(second.points).toEqual([{ lat: 52, lng: 4 }]); + }); + + it('stopping again after a draw was cancelled does not re-cancel the new tool', () => { + const first = createManager(); + first.start(); + const second = createManager(); + second.start(); + + // The first tool releasing its (already lost) claim must not affect + // the second tool's active draw. + first.stop(); + expect(second.isDrawing()).toBe(true); + expect(second.cancelled).not.toHaveBeenCalled(); + }); + + it('stop with the map already gone still releases the keydown listener and hooks', () => { + let liveMap: ReturnType | null = null; + const gone = createManager(false, () => liveMap); + liveMap = map; + gone.start(); + + const removeSpy = vi.spyOn(document, 'removeEventListener'); + liveMap = null; + gone.stop(); + + expect(gone.disabled).toHaveBeenCalledTimes(1); + expect(removeSpy).toHaveBeenCalledWith('keydown', expect.any(Function)); + expect(snapper.clearHighlight).toHaveBeenCalled(); + removeSpy.mockRestore(); + }); }); diff --git a/frontend/src/ui/map/BaseDrawingManager.ts b/frontend/src/ui/map/BaseDrawingManager.ts index df21fe3..443445f 100644 --- a/frontend/src/ui/map/BaseDrawingManager.ts +++ b/frontend/src/ui/map/BaseDrawingManager.ts @@ -3,6 +3,7 @@ import type { MapMouseEvent } from 'maplibre-gl'; import type { NavaidSnapper } from './navdata/NavaidSnapper'; import { DRAWING_CURSOR } from '../../utils/maplibre'; import { isTextEntryTarget } from '../../utils/dom'; +import { claimDrawing, releaseDrawing } from './drawingExclusion'; /** * BaseDrawingManager - shared interactive point-drawing lifecycle for the @@ -97,6 +98,10 @@ export abstract class BaseDrawingManager { const map = this.mapDisplay.getMap(); if (!map) return; + // Cancel any other tool's in-progress draw (shape vs route vs + // aircraft placement) so two tools never consume the same clicks. + claimDrawing(this, () => this.cancelDrawing()); + map.getCanvas().style.cursor = DRAWING_CURSOR; this.onDrawingEnabled(); @@ -116,11 +121,9 @@ export abstract class BaseDrawingManager { * Restore the map to normal mode and tear down all drawing handlers. */ protected disableMapDrawing(): void { - const map = this.mapDisplay.getMap(); - if (!map) return; - this.suspendMapInteraction(); this.onDrawingDisabled(); + releaseDrawing(this); } /** @@ -131,23 +134,19 @@ export abstract class BaseDrawingManager { * Idempotent. */ protected suspendMapInteraction(): void { + // The map can already be gone at teardown; the document-level keydown + // listener and the navaid highlight must be released regardless. const map = this.mapDisplay.getMap(); - if (!map) return; - - map.getCanvas().style.cursor = ''; - - if (this.mapClickHandler) { - map.off('click', this.mapClickHandler); - this.mapClickHandler = null; - } - if (this.mapRightClickHandler) { - map.off('contextmenu', this.mapRightClickHandler); - this.mapRightClickHandler = null; - } - if (this.mapMouseMoveHandler) { - map.off('mousemove', this.mapMouseMoveHandler); - this.mapMouseMoveHandler = null; + if (map) { + map.getCanvas().style.cursor = ''; + if (this.mapClickHandler) map.off('click', this.mapClickHandler); + if (this.mapRightClickHandler) map.off('contextmenu', this.mapRightClickHandler); + if (this.mapMouseMoveHandler) map.off('mousemove', this.mapMouseMoveHandler); } + this.mapClickHandler = null; + this.mapRightClickHandler = null; + this.mapMouseMoveHandler = null; + if (this.keyDownHandler) { document.removeEventListener('keydown', this.keyDownHandler); this.keyDownHandler = null; diff --git a/frontend/src/ui/map/aircraft/Aircraft3DFleet.ts b/frontend/src/ui/map/aircraft/Aircraft3DFleet.ts index 03095fc..b56ea0d 100644 --- a/frontend/src/ui/map/aircraft/Aircraft3DFleet.ts +++ b/frontend/src/ui/map/aircraft/Aircraft3DFleet.ts @@ -60,6 +60,15 @@ export class Aircraft3DFleet { this.aircraft.forEach(cb); } + /** Apply the transform matching the projection the mesh lives in. */ + private applyTransform(mesh: THREE.Object3D, data: AircraftMeshData, isGlobe: boolean): void { + if (isGlobe) { + this.deps.transforms.updateMeshTransformForGlobe(mesh, data); + } else { + this.deps.transforms.updateMeshTransform(mesh, data); + } + } + /** * Scale factor that converts this model's raw GLB units into * real-world meters for the given ICAO. Multiply by the user's @@ -108,14 +117,8 @@ export class Aircraft3DFleet { } const isGlobe = this.deps.isGlobeProjection(); - - if (isGlobe) { - this.deps.transforms.updateMeshTransformForGlobe(mesh, data); - this.deps.getGlobeGroup()?.add(mesh); - } else { - this.deps.transforms.updateMeshTransform(mesh, data); - this.deps.getMercatorGroup()?.add(mesh); - } + this.applyTransform(mesh, data, isGlobe); + (isGlobe ? this.deps.getGlobeGroup() : this.deps.getMercatorGroup())?.add(mesh); this.aircraft.set(id, { mesh, @@ -147,13 +150,9 @@ export class Aircraft3DFleet { ); } - // Update transform appropriate for the current group the mesh is in - // Note: If projection changed, switchGroups handles moving - if (aircraftMesh.currentGroup === 'globe') { - this.deps.transforms.updateMeshTransformForGlobe(aircraftMesh.mesh, data); - } else { - this.deps.transforms.updateMeshTransform(aircraftMesh.mesh, data); - } + // Transform for the group the mesh is currently in; if the + // projection changed, switchGroups handles moving it. + this.applyTransform(aircraftMesh.mesh, data, aircraftMesh.currentGroup === 'globe'); aircraftMesh.data = data; aircraftMesh.lastUpdate = Date.now(); @@ -249,11 +248,7 @@ export class Aircraft3DFleet { reapplyAllTransforms(): void { const isGlobe = this.deps.isGlobeProjection(); this.aircraft.forEach((aircraftMesh) => { - if (isGlobe) { - this.deps.transforms.updateMeshTransformForGlobe(aircraftMesh.mesh, aircraftMesh.data); - } else { - this.deps.transforms.updateMeshTransform(aircraftMesh.mesh, aircraftMesh.data); - } + this.applyTransform(aircraftMesh.mesh, aircraftMesh.data, isGlobe); }); } @@ -288,20 +283,9 @@ export class Aircraft3DFleet { logger.debug('Aircraft3DFleet', `Switching ${this.aircraft.size} aircraft to ${toGlobe ? 'globe' : 'mercator'} group`); this.aircraft.forEach((aircraftMesh) => { - // Remove from current group sourceGroup.remove(aircraftMesh.mesh); - - // Add to target group targetGroup.add(aircraftMesh.mesh); - - // Update the transform for the new projection mode - if (toGlobe) { - this.deps.transforms.updateMeshTransformForGlobe(aircraftMesh.mesh, aircraftMesh.data); - } else { - this.deps.transforms.updateMeshTransform(aircraftMesh.mesh, aircraftMesh.data); - } - - // Update tracking + this.applyTransform(aircraftMesh.mesh, aircraftMesh.data, toGlobe); aircraftMesh.currentGroup = toGlobe ? 'globe' : 'mercator'; }); } diff --git a/frontend/src/ui/map/aircraft/Aircraft3DModelLoader.test.ts b/frontend/src/ui/map/aircraft/Aircraft3DModelLoader.test.ts index f2b52e5..38141fd 100644 --- a/frontend/src/ui/map/aircraft/Aircraft3DModelLoader.test.ts +++ b/frontend/src/ui/map/aircraft/Aircraft3DModelLoader.test.ts @@ -66,6 +66,52 @@ describe('Aircraft3DModelLoader cache disposal', () => { matSpies.forEach((spy) => expect(spy).toHaveBeenCalledTimes(1)); }); + it('drops the recorded raw dimensions on clearCache()', () => { + const loader = makeLoader(); + const { model } = multiMaterialModel(); + + loader.load('A320.glb'); + captured.onLoad?.({ scene: model }); + expect(loader.rawMaxDim('A320.glb')).toBeGreaterThan(0); + + loader.clearCache(); + + expect(loader.rawMaxDim('A320.glb')).toBeUndefined(); + }); + + it('lets an in-flight load re-populate the cache after clearCache()', () => { + const loader = makeLoader(); + const { model } = multiMaterialModel(); + + loader.load('A320.glb'); + loader.clearCache(); // load still in flight + + captured.onLoad?.({ scene: model }); + + expect(loader.get('A320.glb')).toBe(model); + }); + + it('discards a load that completes after clearAll()', () => { + const onModelLoaded = vi.fn(); + const loader = new Aircraft3DModelLoader({ + getMaxAnisotropy: () => 1, + onModelLoaded, + }); + const { model, geometry, materials } = multiMaterialModel(); + const geomSpy = vi.spyOn(geometry, 'dispose'); + const matSpies = materials.map((m) => vi.spyOn(m, 'dispose')); + + loader.load('A320.glb'); + loader.clearAll(); // teardown while the load is in flight + + captured.onLoad?.({ scene: model }); + + expect(loader.get('A320.glb')).toBeUndefined(); + expect(onModelLoaded).not.toHaveBeenCalled(); + expect(geomSpy).toHaveBeenCalledTimes(1); + matSpies.forEach((spy) => expect(spy).toHaveBeenCalledTimes(1)); + }); + it('disposes cached model resources on clearAll()', () => { const loader = makeLoader(); const { model, geometry, materials } = multiMaterialModel(); diff --git a/frontend/src/ui/map/aircraft/Aircraft3DModelLoader.ts b/frontend/src/ui/map/aircraft/Aircraft3DModelLoader.ts index 32eaa31..9934e54 100644 --- a/frontend/src/ui/map/aircraft/Aircraft3DModelLoader.ts +++ b/frontend/src/ui/map/aircraft/Aircraft3DModelLoader.ts @@ -63,6 +63,13 @@ export class Aircraft3DModelLoader { this.loader.load( path, (gltf) => { + // clearAll() ran while this request was in flight: the + // owner tore everything down, so discard the result + // instead of resurrecting the cache. + if (!this.loadingModels.has(path)) { + this.disposeModel(gltf.scene); + return; + } this.loadingModels.delete(path); this.normalizeModel(gltf.scene, path); this.loadedModels.set(path, gltf.scene); @@ -92,38 +99,37 @@ export class Aircraft3DModelLoader { * re-populate the cache. */ public clearCache(): void { - this.disposeCachedModels(); + this.loadedModels.forEach((model) => this.disposeModel(model)); this.loadedModels.clear(); + this.rawMaxDims.clear(); this.animationClips.clear(); } - /** Full teardown: dispose and drop the cache, forget in-flight loads. */ + /** + * Full teardown: dispose and drop the cache, forget in-flight loads + * (their completions are discarded in load()'s callback). + */ public clearAll(): void { - this.disposeCachedModels(); - this.loadedModels.clear(); - this.animationClips.clear(); + this.clearCache(); this.loadingModels.clear(); } /** - * Dispose the geometry and materials of every cached model. Aircraft - * meshes are clones that share these resources, so callers must detach - * all live meshes before clearing the cache. Materials may be a single - * instance or an array (multi-material meshes), so handle both. + * Dispose a model's geometry and materials. Aircraft meshes are clones + * that share these resources, so callers must detach all live meshes + * first. Materials may be a single instance or an array. */ - private disposeCachedModels(): void { - this.loadedModels.forEach((model) => { - model.traverse((child) => { - if (child instanceof THREE.Mesh) { - child.geometry.dispose(); - const material = child.material; - if (Array.isArray(material)) { - material.forEach((m) => m.dispose()); - } else if (material instanceof THREE.Material) { - material.dispose(); - } + private disposeModel(model: THREE.Group): void { + model.traverse((child) => { + if (child instanceof THREE.Mesh) { + child.geometry.dispose(); + const material = child.material; + if (Array.isArray(material)) { + material.forEach((m) => m.dispose()); + } else if (material instanceof THREE.Material) { + material.dispose(); } - }); + } }); } @@ -205,10 +211,8 @@ export class Aircraft3DModelLoader { } }); - // Don't set model.rotation here - it gets ignored anyway because we use mesh.matrix - // The rotation is applied in updateMeshTransform() via the transform matrix - // This ensures heading rotation works correctly - + // No rotation here: heading is applied per-aircraft in the + // updateMeshTransform* methods. logger.debug('Aircraft3DModelLoader', `Model normalized: path=${path}, rawMax=${rawMax.toFixed(2)}, size=${size.x.toFixed(1)}x${size.y.toFixed(1)}x${size.z.toFixed(1)}, anisotropy=${maxAnisotropy}`); } } diff --git a/frontend/src/ui/map/aircraft/Aircraft3DTransforms.test.ts b/frontend/src/ui/map/aircraft/Aircraft3DTransforms.test.ts new file mode 100644 index 0000000..a55ed3e --- /dev/null +++ b/frontend/src/ui/map/aircraft/Aircraft3DTransforms.test.ts @@ -0,0 +1,98 @@ +/** + * Tests for Aircraft3DTransforms scene-origin management. The scene origin + * is shared by every mesh transform and the mercator camera matrix, so an + * invalid aircraft record (NaN or out-of-range lat/lon, which BlueSky can + * deliver, e.g. after MOVE beyond lat 90) must never leak into it: + * MercatorCoordinate.fromLngLat throws on invalid input, which crashes the + * whole data tick and the per-frame render. + */ +import { describe, it, expect } from 'vitest'; +import * as THREE from 'three'; +import { Aircraft3DTransforms } from './Aircraft3DTransforms'; +import type { AircraftMeshData } from './Aircraft3DTransforms'; +import type { AircraftData, DisplayOptions } from '../../../data/types'; + +function makeTransforms(): Aircraft3DTransforms { + return new Aircraft3DTransforms({ + getMap: () => null, + getCamera: () => new THREE.PerspectiveCamera(), + getDisplayOptions: () => ({ aircraft3DScale: 2 } as unknown as DisplayOptions), + createFallbackMatrix: () => new THREE.Matrix4(), + stateManager: null, + }); +} + +/** AircraftData carrying only the positions the origin logic reads. */ +function acData(positions: Array<[number, number]>): AircraftData { + return { + id: positions.map((_, i) => `AC${i}`), + lat: positions.map((p) => p[0]), + lon: positions.map((p) => p[1]), + } as unknown as AircraftData; +} + +function meshData(lat: number, lon: number): AircraftMeshData { + return { lat, lon, alt: 3000, hdg: 90, selected: false, inconf: false, actype: 'A320' }; +} + +describe('Aircraft3DTransforms scene origin vs invalid coordinates', () => { + it('initializes the origin from the first valid aircraft, skipping invalid ones', () => { + const t = makeTransforms(); + t.updateSceneOrigin(acData([[NaN, NaN], [52, 4]])); + + const mesh = new THREE.Object3D(); + t.updateMeshTransform(mesh, meshData(52, 4)); + + // Origin is the valid aircraft, so its own offsets are ~zero. + expect(mesh.position.x).toBeCloseTo(0); + expect(mesh.position.z).toBeCloseTo(0); + }); + + it('returns false (no origin) when no valid aircraft and no map exist', () => { + const t = makeTransforms(); + expect(t.updateSceneOrigin(acData([[91, 181]]))).toBe(false); + }); + + it('an aircraft moved beyond lat 90 does not drag the origin away from valid traffic', () => { + const t = makeTransforms(); + t.updateSceneOrigin(acData([[52.2, 4.7]])); + // BAD sits at lat 91 (as ACDATA reports after `MOVE BAD 91,181`) + // next to an in-range aircraft. + t.updateSceneOrigin(acData([[91, 181], [52.3, 4.62]])); + + const mesh = new THREE.Object3D(); + t.updateMeshTransform(mesh, meshData(52.3, 4.62)); + + // The origin must stay near the valid traffic: offsets are small + // and finite (a poisoned centroid would put it thousands of km away). + expect(Number.isFinite(mesh.position.x)).toBe(true); + expect(Math.abs(mesh.position.x)).toBeLessThan(50_000); + expect(Math.abs(mesh.position.z)).toBeLessThan(50_000); + }); + + it('a NaN record does not poison the reposition centroid', () => { + const t = makeTransforms(); + t.updateSceneOrigin(acData([[52.2, 4.7]])); + // The valid aircraft is >10 km out, so the origin repositions; the + // NaN record must be excluded from the centroid. + t.updateSceneOrigin(acData([[NaN, 4], [53.5, 6]])); + + const mesh = new THREE.Object3D(); + t.updateMeshTransform(mesh, meshData(53.5, 6)); + + expect(Number.isFinite(mesh.position.x)).toBe(true); + expect(Math.abs(mesh.position.x)).toBeLessThan(50_000); + expect(Math.abs(mesh.position.z)).toBeLessThan(50_000); + }); + + it('still repositions onto the centroid of valid traffic', () => { + const t = makeTransforms(); + t.updateSceneOrigin(acData([[52, 4]])); + expect(t.updateSceneOrigin(acData([[53.5, 6]]))).toBe(true); + + const mesh = new THREE.Object3D(); + t.updateMeshTransform(mesh, meshData(53.5, 6)); + expect(mesh.position.x).toBeCloseTo(0); + expect(mesh.position.z).toBeCloseTo(0); + }); +}); diff --git a/frontend/src/ui/map/aircraft/Aircraft3DTransforms.ts b/frontend/src/ui/map/aircraft/Aircraft3DTransforms.ts index b70f680..b5fa640 100644 --- a/frontend/src/ui/map/aircraft/Aircraft3DTransforms.ts +++ b/frontend/src/ui/map/aircraft/Aircraft3DTransforms.ts @@ -1,6 +1,8 @@ import * as THREE from 'three'; import type { Map as MapLibreMap } from 'maplibre-gl'; import { altitudeScaledForOrigin, mercatorCameraMatrix, relativePositionMeters } from '../rendering/mercatorUtils'; +import type { LngLatPoint } from '../rendering/mercatorUtils'; +import { isValidCoordinate } from '../../../utils/maplibre'; import { getGlobeModelMatrix } from '../rendering/globeMatrix'; import type { Render3DArgs } from '../rendering/CustomLayer3D'; import type { AircraftData, DisplayOptions } from '../../../data/types'; @@ -76,75 +78,64 @@ export class Aircraft3DTransforms { * Initialize or update scene origin based on aircraft positions. * Returns true when the origin was repositioned, in which case the * caller must re-apply mercator transforms to existing meshes. + * + * Invalid records (NaN or out-of-range lat/lon — BlueSky delivers + * these, e.g. after a MOVE beyond lat 90) are ignored, mirroring the + * per-aircraft guard in the renderers. A poisoned origin would make + * MercatorCoordinate.fromLngLat throw on every mesh and camera update. */ updateSceneOrigin(aircraftData: AircraftData): boolean { + const valid: LngLatPoint[] = []; + for (let i = 0; i < aircraftData.lat.length; i++) { + if (isValidCoordinate(aircraftData.lat[i], aircraftData.lon[i])) { + valid.push({ lng: aircraftData.lon[i], lat: aircraftData.lat[i] }); + } + } + if (!this.sceneOrigin) { - // Initialize scene origin with first aircraft or map center - if (aircraftData.lat.length > 0) { - this.sceneOrigin = { - lng: aircraftData.lon[0], - lat: aircraftData.lat[0] - }; + if (valid.length > 0) { + this.sceneOrigin = { ...valid[0] }; } else { // Fallback to map center if available const center = this.deps.getMap()?.getCenter(); if (!center) return false; - this.sceneOrigin = { - lng: center.lng, - lat: center.lat - }; + this.sceneOrigin = { lng: center.lng, lat: center.lat }; } logger.debug('Aircraft3DTransforms', `Scene origin set to: ${this.sceneOrigin.lng.toFixed(6)}, ${this.sceneOrigin.lat.toFixed(6)}`); } - // Check if any aircraft is too far from current origin - let needsRepositioning = false; - for (let i = 0; i < aircraftData.lat.length; i++) { - const distance = this.calculateDistance( - this.sceneOrigin.lat, this.sceneOrigin.lng, - aircraftData.lat[i], aircraftData.lon[i] - ); - if (distance > this.maxDistanceFromOrigin) { - needsRepositioning = true; - break; - } - } + const origin = this.sceneOrigin; + const needsRepositioning = valid.some((p) => + this.calculateDistance(origin.lat, origin.lng, p.lat, p.lng) > this.maxDistanceFromOrigin + ); - if (needsRepositioning) { - return this.repositionSceneOrigin(aircraftData); - } - return false; + return needsRepositioning ? this.repositionSceneOrigin(valid) : false; } /** - * Reposition scene origin to aircraft centroid. Returns true when the - * origin actually moved (changes under 10 m are skipped). + * Reposition scene origin to the centroid of the given (valid) aircraft + * positions. Returns true when the origin actually moved (changes under + * 10 m are skipped). */ - private repositionSceneOrigin(aircraftData: AircraftData): boolean { - if (aircraftData.lat.length === 0) return false; + private repositionSceneOrigin(coords: LngLatPoint[]): boolean { + if (coords.length === 0) return false; - // Calculate centroid of all aircraft let sumLat = 0; let sumLng = 0; - for (let i = 0; i < aircraftData.lat.length; i++) { - sumLat += aircraftData.lat[i]; - sumLng += aircraftData.lon[i]; + for (const c of coords) { + sumLat += c.lat; + sumLng += c.lng; } - const newOrigin = { - lng: sumLng / aircraftData.lon.length, - lat: sumLat / aircraftData.lat.length + lng: sumLng / coords.length, + lat: sumLat / coords.length }; - // Check if the new origin is significantly different from current origin - // Only reposition if difference is meaningful (> 10 meters) const distanceToNewOrigin = this.calculateDistance( this.sceneOrigin!.lat, this.sceneOrigin!.lng, newOrigin.lat, newOrigin.lng ); - if (distanceToNewOrigin < 10) { - // Origin change is too small to be meaningful, skip repositioning return false; } diff --git a/frontend/src/ui/map/aircraft/AircraftCreationManager.ts b/frontend/src/ui/map/aircraft/AircraftCreationManager.ts index c5db1f6..5ec3ba4 100644 --- a/frontend/src/ui/map/aircraft/AircraftCreationManager.ts +++ b/frontend/src/ui/map/aircraft/AircraftCreationManager.ts @@ -13,6 +13,7 @@ import { import { pointFeature, lineStringFeature } from '../../../utils/geojson'; import { roundedBearing } from '../../../utils/geo'; import { isTextEntryTarget } from '../../../utils/dom'; +import { claimDrawing, releaseDrawing } from '../drawingExclusion'; import { AircraftCreationForm, AircraftCreationData, @@ -62,6 +63,14 @@ export class AircraftCreationManager { this.form.showModal(); } + /** + * Whether click-to-place aircraft creation is currently active. Consumed + * by AircraftInteractionManager to suppress empty-map-click deselection. + */ + public isDrawing(): boolean { + return this.aircraftDrawingMode; + } + /** * Begin the map drawing state machine with validated form data. * Invoked by AircraftCreationForm after the modal closes. @@ -118,6 +127,10 @@ export class AircraftCreationManager { return; } + // Cancel any in-progress shape/route draw so two tools never consume + // the same map clicks. + claimDrawing(this, () => this.stopAircraftDrawing()); + // Match the crosshair cursor used by the console map picker and the // shape/route drawing modes so every drawing mode looks the same. map.getCanvas().style.cursor = DRAWING_CURSOR; @@ -154,26 +167,19 @@ export class AircraftCreationManager { * Disable aircraft map drawing */ private disableAircraftMapDrawing(): void { + // The map can already be gone at teardown; the document-level Escape + // listener and the drawing claim must be released regardless. const map = this.mapDisplay.getMap(); - if (!map) return; - - // Restore MapLibre's default cursor when leaving drawing mode. - map.getCanvas().style.cursor = ''; - - if (this.aircraftMapClickHandler) { - map.off('click', this.aircraftMapClickHandler); - this.aircraftMapClickHandler = null; - } - - if (this.aircraftMouseMoveHandler) { - map.off('mousemove', this.aircraftMouseMoveHandler); - this.aircraftMouseMoveHandler = null; - } - - if (this.aircraftSnapHoverHandler) { - map.off('mousemove', this.aircraftSnapHoverHandler); - this.aircraftSnapHoverHandler = null; + if (map) { + // Restore MapLibre's default cursor when leaving drawing mode. + map.getCanvas().style.cursor = ''; + if (this.aircraftMapClickHandler) map.off('click', this.aircraftMapClickHandler); + if (this.aircraftMouseMoveHandler) map.off('mousemove', this.aircraftMouseMoveHandler); + if (this.aircraftSnapHoverHandler) map.off('mousemove', this.aircraftSnapHoverHandler); } + this.aircraftMapClickHandler = null; + this.aircraftMouseMoveHandler = null; + this.aircraftSnapHoverHandler = null; this.navaidSnapper.clearHighlight(); if (this.aircraftEscapeHandler) { @@ -183,6 +189,7 @@ export class AircraftCreationManager { this.aircraftPosition = null; this.clearTemporaryAircraftDrawing(); + releaseDrawing(this); } /** diff --git a/frontend/src/ui/map/aircraft/AircraftInteractionManager.ts b/frontend/src/ui/map/aircraft/AircraftInteractionManager.ts index 6eb2ffa..81de6af 100644 --- a/frontend/src/ui/map/aircraft/AircraftInteractionManager.ts +++ b/frontend/src/ui/map/aircraft/AircraftInteractionManager.ts @@ -39,9 +39,10 @@ export class AircraftInteractionManager { private lastExplicitPosCommand: string | null = null; private lastExplicitPosTime: number = 0; - // Optional reference to the route drawing manager - when route drawing is - // active, empty-map clicks are waypoint placements, not aircraft deselects. - private isRouteDrawingActive: (() => boolean) | null = null; + // Optional predicate reporting whether any map drawing tool (route, + // shape, aircraft placement) is active - those clicks are point + // placements, not aircraft deselects. + private isDrawingToolActive: (() => boolean) | null = null; // Document-level listeners and state subscriptions, released in destroy() // (map listeners die with the map, but these would outlive it). @@ -108,9 +109,9 @@ export class AircraftInteractionManager { setTimeout(() => { if (!this.map) return; - // If the route drawing tool is active, an empty-map click is a - // waypoint placement, not a request to unselect the aircraft. - if (this.isRouteDrawingActive && this.isRouteDrawingActive()) { + // If a drawing tool is active, an empty-map click is a point + // placement, not a request to unselect the aircraft. + if (this.isDrawingToolActive && this.isDrawingToolActive()) { return; } @@ -478,12 +479,13 @@ export class AircraftInteractionManager { } /** - * Register a predicate that reports whether interactive route drawing is - * currently in progress. When true, empty-map clicks are suppressed from - * the "unselect aircraft" path so they can be consumed as waypoint drops. + * Register a predicate that reports whether any interactive drawing tool + * (route, shape, aircraft placement) is in progress. When true, empty-map + * clicks are suppressed from the "unselect aircraft" path so they can be + * consumed as point drops. */ - public setRouteDrawingActiveCheck(check: () => boolean): void { - this.isRouteDrawingActive = check; + public setDrawingToolActiveCheck(check: () => boolean): void { + this.isDrawingToolActive = check; } /** diff --git a/frontend/src/ui/map/drawingExclusion.ts b/frontend/src/ui/map/drawingExclusion.ts new file mode 100644 index 0000000..a6a3873 --- /dev/null +++ b/frontend/src/ui/map/drawingExclusion.ts @@ -0,0 +1,30 @@ +/** + * Mutual exclusion between the map's click-to-draw tools (route drawing, + * shape drawing, aircraft placement). Each tool claims the map when its + * interactive mode starts; claiming cancels whichever tool held it, so two + * tools can never consume the same map clicks at once. + */ + +let activeOwner: object | null = null; +let activeCancel: (() => void) | null = null; + +/** + * Claim interactive drawing for `owner`, cancelling any other tool's + * in-progress draw first. `cancel` must fully stop the owner's draw; it is + * invoked when another tool claims the map. + */ +export function claimDrawing(owner: object, cancel: () => void): void { + if (activeOwner && activeOwner !== owner) { + activeCancel?.(); + } + activeOwner = owner; + activeCancel = cancel; +} + +/** Release the claim if `owner` still holds it (no-op otherwise). */ +export function releaseDrawing(owner: object): void { + if (activeOwner === owner) { + activeOwner = null; + activeCancel = null; + } +} diff --git a/frontend/src/ui/map/routes/RouteDrawingManager.ts b/frontend/src/ui/map/routes/RouteDrawingManager.ts index 86ed0a8..bcc970d 100644 --- a/frontend/src/ui/map/routes/RouteDrawingManager.ts +++ b/frontend/src/ui/map/routes/RouteDrawingManager.ts @@ -212,10 +212,12 @@ export class RouteDrawingManager extends BaseDrawingManager { drawBtn.classList.add('active'); } + // Claim the map first (cancelling any other tool's draw, which hides + // the shared banner), then show this draw's banner. + this.enableMapDrawing(); this.showDrawingBanner( `Drawing route for ${this.targetAircraftId} (leader from ${this.leaderAnchorLabel}) - Click to add waypoints, right-click or Enter to finish, Esc to cancel` ); - this.enableMapDrawing(); this.preview.updateDrawing(this.routePoints, this.leaderAnchor); logger.info( diff --git a/frontend/src/ui/map/shapes/ShapeDrawingManager.ts b/frontend/src/ui/map/shapes/ShapeDrawingManager.ts index 9619cb6..4a39ad0 100644 --- a/frontend/src/ui/map/shapes/ShapeDrawingManager.ts +++ b/frontend/src/ui/map/shapes/ShapeDrawingManager.ts @@ -192,10 +192,12 @@ export class ShapeDrawingManager extends BaseDrawingManager { drawBtn.classList.add('active'); } + // Claim the map first (cancelling any other tool's draw, which hides + // the shared banner), then show this draw's banner. + this.enableMapDrawing(); this.showDrawingBanner( `${this.getBannerPrefix()} - ${START_INSTRUCTIONS[this.currentShapeType]}` ); - this.enableMapDrawing(); logger.info('ShapeDrawingManager', `Started drawing ${this.currentShapeType}: ${this.currentShapeName}`); } diff --git a/frontend/src/ui/panels/left/MapControlsPanel.ts b/frontend/src/ui/panels/left/MapControlsPanel.ts index efe3d25..aadbc7c 100644 --- a/frontend/src/ui/panels/left/MapControlsPanel.ts +++ b/frontend/src/ui/panels/left/MapControlsPanel.ts @@ -29,61 +29,25 @@ export class MapControlsPanel extends BasePanel { protected onInit(): void { logger.debug('MapControlsPanel', 'MapControlsPanel initialized'); - // Set up button event handlers - this.setupButtonHandlers(); + this.bindClick('create-aircraft-btn', () => + this.withManager(this.aircraftCreationManager, 'AircraftCreationManager', m => m.showModal())); + this.bindClick('draw-shape-btn', () => + this.withManager(this.shapeDrawingManager, 'ShapeDrawingManager', m => m.toggleDrawing())); + this.bindClick('draw-route-btn', () => + this.withManager(this.routeDrawingManager, 'RouteDrawingManager', m => m.toggleDrawing())); - // Set up map info update handlers this.setupMapInfoHandlers(); } /** - * Set up button event handlers + * Run a drawing-button action against its manager, or tell the user the + * map is still loading when the manager hasn't been wired up yet. */ - private setupButtonHandlers(): void { - this.bindClick('create-aircraft-btn', () => this.onCreateAircraftClick()); - this.bindClick('draw-shape-btn', () => this.onDrawShapeClick()); - this.bindClick('draw-route-btn', () => this.onDrawRouteClick()); - } - - /** - * Handle Create Aircraft button click - */ - private onCreateAircraftClick(): void { - logger.debug('MapControlsPanel', 'Create Aircraft button clicked'); - // Will be connected to AircraftCreationManager - if (this.aircraftCreationManager) { - this.aircraftCreationManager.showModal(); - } else { - logger.warn('MapControlsPanel', 'AircraftCreationManager not set - map may still be loading'); - // Optionally show a user-friendly message - alert('Map is still loading. Please wait a moment and try again.'); - } - } - - /** - * Handle Draw Shape button click - */ - private onDrawShapeClick(): void { - logger.debug('MapControlsPanel', 'Draw Shape button clicked'); - // Will be connected to ShapeDrawingManager - if (this.shapeDrawingManager) { - this.shapeDrawingManager.toggleDrawing(); - } else { - logger.warn('MapControlsPanel', 'ShapeDrawingManager not set - map may still be loading'); - // Optionally show a user-friendly message - alert('Map is still loading. Please wait a moment and try again.'); - } - } - - /** - * Handle Draw Route button click - */ - private onDrawRouteClick(): void { - logger.debug('MapControlsPanel', 'Draw Route button clicked'); - if (this.routeDrawingManager) { - this.routeDrawingManager.toggleDrawing(); + private withManager(manager: T | null, name: string, action: (manager: T) => void): void { + if (manager) { + action(manager); } else { - logger.warn('MapControlsPanel', 'RouteDrawingManager not set - map may still be loading'); + logger.warn('MapControlsPanel', `${name} not set - map may still be loading`); alert('Map is still loading. Please wait a moment and try again.'); } } @@ -163,121 +127,69 @@ export class MapControlsPanel extends BasePanel { logger.debug('MapControlsPanel', 'Map event listeners attached for zoom and bbox updates'); } - /** - * Update the zoom level display - */ private updateZoomDisplay(): void { - if (!this.mapDisplay) return; - - const map = this.mapDisplay.getMap(); + const map = this.mapDisplay?.getMap(); if (!map) return; this.setText('current-zoom', map.getZoom().toFixed(1)); } - /** - * Update the bounding box display - */ private updateBoundingBoxDisplay(): void { - if (!this.mapDisplay) return; - - const map = this.mapDisplay.getMap(); + const map = this.mapDisplay?.getMap(); if (!map) return; const bounds = map.getBounds(); - this.setText('bbox-north', bounds.getNorth().toFixed(2)); this.setText('bbox-south', bounds.getSouth().toFixed(2)); this.setText('bbox-east', bounds.getEast().toFixed(2)); this.setText('bbox-west', bounds.getWest().toFixed(2)); } - /** - * Set the AircraftCreationManager instance - */ public setAircraftCreationManager(manager: AircraftCreationManager): void { this.aircraftCreationManager = manager; - logger.debug('MapControlsPanel', 'MapControlsPanel connected to AircraftCreationManager'); } - /** - * Set the ShapeDrawingManager instance - */ public setShapeDrawingManager(manager: ShapeDrawingManager): void { this.shapeDrawingManager = manager; - logger.debug('MapControlsPanel', 'MapControlsPanel connected to ShapeDrawingManager'); } - /** - * Set the RouteDrawingManager instance - */ public setRouteDrawingManager(manager: RouteDrawingManager): void { this.routeDrawingManager = manager; - logger.debug('MapControlsPanel', 'MapControlsPanel connected to RouteDrawingManager'); } - /** - * Zoom in by one level - */ + /** Zoom in by one level */ public zoomIn(): void { - if (!this.mapDisplay) { - logger.warn('MapControlsPanel', 'Cannot zoom in: MapDisplay not set'); - return; - } - - const map = this.mapDisplay.getMap(); + const map = this.mapDisplay?.getMap(); if (!map) { - logger.warn('MapControlsPanel', 'Cannot zoom in: Map not initialized'); + logger.warn('MapControlsPanel', 'Cannot zoom in: map not ready'); return; } - - const currentZoom = map.getZoom(); - map.easeTo({ zoom: currentZoom + 1, duration: 300 }); - logger.debug('MapControlsPanel', 'Zooming in to level:', currentZoom + 1); + map.easeTo({ zoom: map.getZoom() + 1, duration: 300 }); } - /** - * Zoom out by one level - */ + /** Zoom out by one level */ public zoomOut(): void { - if (!this.mapDisplay) { - logger.warn('MapControlsPanel', 'Cannot zoom out: MapDisplay not set'); - return; - } - - const map = this.mapDisplay.getMap(); + const map = this.mapDisplay?.getMap(); if (!map) { - logger.warn('MapControlsPanel', 'Cannot zoom out: Map not initialized'); + logger.warn('MapControlsPanel', 'Cannot zoom out: map not ready'); return; } - - const currentZoom = map.getZoom(); - map.easeTo({ zoom: currentZoom - 1, duration: 300 }); - logger.debug('MapControlsPanel', 'Zooming out to level:', currentZoom - 1); + map.easeTo({ zoom: map.getZoom() - 1, duration: 300 }); } - /** - * Reset view to default center and zoom - */ + /** Reset view to the default center and zoom */ public resetView(): void { - if (!this.mapDisplay) { - logger.warn('MapControlsPanel', 'Cannot reset view: MapDisplay not set'); - return; - } - - const map = this.mapDisplay.getMap(); + const map = this.mapDisplay?.getMap(); if (!map) { - logger.warn('MapControlsPanel', 'Cannot reset view: Map not initialized'); + logger.warn('MapControlsPanel', 'Cannot reset view: map not ready'); return; } - map.flyTo({ center: this.DEFAULT_CENTER, zoom: this.DEFAULT_ZOOM, duration: 1000, essential: true }); - logger.debug('MapControlsPanel', 'Resetting view to default center and zoom'); } /** diff --git a/tests/test_data_manager.py b/tests/test_data_manager.py index 8ee8074..cda0155 100644 --- a/tests/test_data_manager.py +++ b/tests/test_data_manager.py @@ -124,3 +124,24 @@ def test_get_current_data_uses_proxy_node_mgr(self, proxy, monkeypatch): ) data = proxy.data_mgr.get_current_data() assert data["node_info"]["active_node"] == "deadbeef81" + + def test_get_current_data_node_info_is_serialized(self, proxy): + """The initial_data snapshot must carry the same JSON-safe node_info + payload as the node_info event — no raw bytes (which Socket.IO would + ship as binary attachments and the frontend NodeData type can't use).""" + node_bytes = b"\x01\x02\x03\x04\x81" + proxy.tracked_nodes[node_bytes.hex()] = { + "node_id": node_bytes, + "server_id": b"SRV\x80\x80", + } + proxy.tracked_servers[b"SRV\x80\x80"] = {"server_id": b"SRV\x80\x80"} + + node_info = proxy.data_mgr.get_current_data()["node_info"] + + node_entry = node_info["nodes"][node_bytes.hex()] + assert isinstance(node_entry["node_id"], str) + assert isinstance(node_entry["server_id"], str) + assert "server_id_hex" in node_entry + assert all(isinstance(k, str) for k in node_info["servers"]) + # The proxy's internal store keeps its binary IDs untouched. + assert proxy.tracked_nodes[node_bytes.hex()]["node_id"] == node_bytes diff --git a/tests/test_node_manager.py b/tests/test_node_manager.py index ed46fe7..6ace317 100644 --- a/tests/test_node_manager.py +++ b/tests/test_node_manager.py @@ -137,6 +137,45 @@ def test_check_node_shutdown_uses_proxy_connection_mgr(self, proxy, monkeypatch) assert reasons == ["All nodes removed (server shutdown)"] +class TestReactivateOnNodeAdded: + """A node discovered while the client's active node is dead must become + the new active node. + + Regression test: the network client auto-selects only the very first node + it ever sees, and removal-failover needs a survivor. When the last node + vanishes (e.g. its process crashes) and a node is added afterwards, + ``act_id`` kept pointing at the dead node, the actonly subscriptions + (ACDATA/ROUTEDATA) never moved to the new node, and the data-flow timeout + tore down a live connection ~10s later.""" + + def test_node_added_with_stale_act_id_reactivates(self, proxy, fake_client): + dead_bytes = b"\x01\x02\x03\x04\x81" + new_bytes = b"\x01\x02\x03\x04\x82" + proxy.bluesky_client = fake_client + proxy.running = True + proxy.was_connected = True + fake_client.act_id = dead_bytes # removed node; not in client.nodes + fake_client.nodes = {new_bytes} + + proxy.node_mgr._on_node_added(new_bytes) + + assert fake_client.act_id == new_bytes + + def test_node_added_with_live_act_id_keeps_it(self, proxy, fake_client): + active_bytes = b"\x01\x02\x03\x04\x81" + new_bytes = b"\x01\x02\x03\x04\x82" + proxy.bluesky_client = fake_client + proxy.running = True + proxy.was_connected = True + proxy.tracked_nodes[active_bytes.hex()] = {"node_id": active_bytes} + fake_client.act_id = active_bytes + fake_client.nodes = {active_bytes, new_bytes} + + proxy.node_mgr._on_node_added(new_bytes) + + assert fake_client.act_id == active_bytes + + class TestNodeRemoval: def test_on_node_removed_deletes_tracked_node(self, proxy): node_bytes = b"\x01\x02\x03\x04\x81"