From fad89a63746cb9ab182e4e4ca607e1ecfa4e5062 Mon Sep 17 00:00:00 2001 From: Andres Morfin Veytia <78442543+amorfinv@users.noreply.github.com> Date: Wed, 19 Aug 2026 20:40:19 +0200 Subject: [PATCH 1/7] Fix log streamer out-of-order emits and stuck flush flag Drain the pending buffer in a loop inside a single flush task instead of clearing the scheduled flag before emitting, so a line arriving mid-flush can no longer spawn a second concurrent flush task that interleaves newer lines among older chunks. Clear the flag under the feed lock on exit and on any raising emit/sleep, so the stream can never go silent for good. Co-Authored-By: Claude Fable 5 --- WebATM-integrated/tests/test_log_streamer.py | 77 +++++++++++++++++++ .../webatm_integrated/log_streamer.py | 35 ++++++--- 2 files changed, 103 insertions(+), 9 deletions(-) diff --git a/WebATM-integrated/tests/test_log_streamer.py b/WebATM-integrated/tests/test_log_streamer.py index dc837df..53b94d4 100644 --- a/WebATM-integrated/tests/test_log_streamer.py +++ b/WebATM-integrated/tests/test_log_streamer.py @@ -79,6 +79,83 @@ def test_history_is_bounded_to_max_history(): assert [item["line"] for item in streamer.history()] == ["l2", "l3", "l4"] +class ReentrantSocketIO(FakeSocketIO): + """Runs a newly scheduled flush task *during* an emit. + + FakeSocketIO drains tasks strictly one after another, which hides the real + threading-mode behaviour where a second flush task can emit while the first + is still working through its chunks. This stand-in models that by firing + ``on_first_emit`` mid-flush and giving any task it schedules a turn there + and then. + """ + + def __init__(self): + super().__init__() + self.on_first_emit = None + self._reentered = False + + def emit(self, event, payload): + super().emit(event, payload) + if self.on_first_emit and not self._reentered: + self._reentered = True + hook, self.on_first_emit = self.on_first_emit, None + hook() # a log line arrives mid-flush... + self.run_all() # ...and its flush task gets a turn + + +def test_a_line_arriving_mid_flush_does_not_overtake_the_batch_being_emitted(): + """Only one flush task may be live: a second one emitting concurrently + would interleave its newer lines among the older chunks, delivering the + stream out of order despite seq being assigned in order.""" + sio = ReentrantSocketIO() + streamer = LogStreamer(sio, batch_max=2) + + for i in range(5): + streamer.feed_line(f"l{i}") + sio.on_first_emit = lambda: streamer.feed_line("late") + sio.run_all() + + seqs = [item["seq"] for _, payload in sio.emitted for item in payload["lines"]] + assert seqs == sorted(seqs), f"lines delivered out of order: {seqs}" + assert _lines(sio) == ["l0", "l1", "l2", "l3", "l4", "late"] + + +def test_a_line_arriving_as_the_flusher_winds_down_is_still_delivered(): + """The scheduled flag is cleared under the same lock feed_line appends + under, so a line can never be left pending with no flush task coming.""" + sio = FakeSocketIO() + streamer = LogStreamer(sio) + + streamer.feed_line("first") + sio.run_all() + streamer.feed_line("second") + sio.run_all() + + assert _lines(sio) == ["first", "second"] + + +def test_stream_recovers_after_an_emit_raises(): + """A raising emit must not leave the flush flag stuck True -- feed_line only + schedules while it is False, so the stream would go silent for good.""" + sio = FakeSocketIO() + streamer = LogStreamer(sio) + + def boom(event, payload): + raise RuntimeError("socket write failed") + + sio.emit = boom + streamer.feed_line("during-outage") + with pytest.raises(RuntimeError): + sio.run_all() + + # The next line must schedule a fresh flush and get through. + sio.emit = lambda event, payload: sio.emitted.append((event, payload)) + streamer.feed_line("after-outage") + sio.run_all() + + assert _lines(sio) == ["after-outage"] + + def test_feed_line_recovers_after_a_failed_schedule(): """A failed start_background_task must not leave _flush_scheduled stuck True, which would silence the stream forever.""" diff --git a/WebATM-integrated/webatm_integrated/log_streamer.py b/WebATM-integrated/webatm_integrated/log_streamer.py index 08af5c4..4d5da70 100644 --- a/WebATM-integrated/webatm_integrated/log_streamer.py +++ b/WebATM-integrated/webatm_integrated/log_streamer.py @@ -66,15 +66,32 @@ def feed_line(self, line: str) -> None: raise def _flush_after_delay(self) -> None: - # Cooperative sleep via SocketIO so it matches the active async mode. - self._sio.sleep(self._batch_ms / 1000.0) - with self._lock: - batch = self._pending - self._pending = [] - self._flush_scheduled = False - for start in range(0, len(batch), self._batch_max): - chunk = batch[start : start + self._batch_max] - self._sio.emit(EVENT, {"lines": chunk}) + # Drain until empty rather than flushing once, so only one flush task is + # ever live: clearing the flag before emitting would let a second task + # emit concurrently, interleaving newer lines among the older chunks. + try: + while True: + # Cooperative sleep via SocketIO to match the active async mode. + self._sio.sleep(self._batch_ms / 1000.0) + with self._lock: + batch = self._pending + self._pending = [] + if not batch: + # Cleared under the lock feed_line appends under, so a + # line arriving now always gets a flush task scheduled + # for it -- it can never be stranded unflushed. + self._flush_scheduled = False + return + for start in range(0, len(batch), self._batch_max): + chunk = batch[start : start + self._batch_max] + self._sio.emit(EVENT, {"lines": chunk}) + except BaseException: + # A raising emit/sleep must not leave the flag stuck True: feed_line + # only schedules while it is False, so the stream would go silent + # for good. Clearing it lets the next line start a fresh flush. + with self._lock: + self._flush_scheduled = False + raise def history(self) -> list[dict]: """Return a snapshot of buffered lines for late-joining clients. From 0b0edebabc39d966d71bcbb77a47e887872f7a6d Mon Sep 17 00:00:00 2001 From: Andres Morfin Veytia <78442543+amorfinv@users.noreply.github.com> Date: Wed, 19 Aug 2026 20:40:30 +0200 Subject: [PATCH 2/7] Fix process manager stranded "stopping" state and false kill message If signalling the process group raised anything but ProcessLookupError, stop() left the state stranded at "stopping", making every later start() wait out the 15s shutdown grace and then refuse, with no way to clear it. Teardown is now wrapped so the state is re-derived from whether the process is actually alive, and group signalling tolerates an already-exited group. kill() also no longer reports "BlueSky server killed" when there was no live process to kill; the UI shows that message verbatim. Co-Authored-By: Claude Fable 5 --- .../tests/test_process_manager.py | 53 ++++++++++++++ .../webatm_integrated/process_manager.py | 72 +++++++++++++------ 2 files changed, 102 insertions(+), 23 deletions(-) diff --git a/WebATM-integrated/tests/test_process_manager.py b/WebATM-integrated/tests/test_process_manager.py index d531db6..c7b1782 100644 --- a/WebATM-integrated/tests/test_process_manager.py +++ b/WebATM-integrated/tests/test_process_manager.py @@ -18,6 +18,8 @@ import threading import time +import pytest +import webatm_integrated.process_manager as pm from webatm_integrated.process_manager import BlueSkyProcessManager @@ -147,6 +149,57 @@ def on_line(line: str) -> None: manager.kill() +def test_a_failing_signal_does_not_strand_the_state_at_stopping(monkeypatch): + """If killpg raises anything but ProcessLookupError, stop() must still leave + a usable state: a stranded "stopping" makes every later start() wait 15s and + then refuse, with no control surface able to clear it.""" + manager = BlueSkyProcessManager( + cmd=[sys.executable, "-c", "import time; time.sleep(60)"] + ) + try: + assert manager.start()["success"] is True + + def denied(pgid, sig): + raise PermissionError("operation not permitted") + + monkeypatch.setattr(pm.os, "killpg", denied) + with pytest.raises(PermissionError): + manager.stop() + + # The process really is still up, so that is what status must report -- + # and a retry must be able to proceed rather than hit the stop-wait. + assert manager.status()["status"] == "running" + monkeypatch.undo() + assert manager.stop()["success"] is True + assert manager.status()["running"] is False + finally: + manager.kill() + + +def test_kill_on_a_stopped_server_does_not_claim_it_killed_something(): + """The UI renders this message verbatim, so Kill on an already-stopped + server must say so rather than report a kill that never happened.""" + manager = BlueSkyProcessManager(cmd=[sys.executable, "-c", "pass"]) + + result = manager.kill() + + assert result["success"] is True + assert result["status"] == "stopped" + assert result["message"] == "BlueSky server is not running" + + +def test_kill_reports_a_kill_when_a_process_was_actually_running(): + manager = BlueSkyProcessManager( + cmd=[sys.executable, "-c", "import time; time.sleep(60)"] + ) + assert manager.start()["success"] is True + + result = manager.kill() + + assert result["success"] is True + assert result["message"] == "BlueSky server killed" + + def test_restart_propagates_stop_failure_instead_of_claiming_success(): """restart() must not report "restarted" while the old tree is still alive.""" manager = BlueSkyProcessManager(cmd=[sys.executable, "-c", "pass"]) diff --git a/WebATM-integrated/webatm_integrated/process_manager.py b/WebATM-integrated/webatm_integrated/process_manager.py index aa58298..8735988 100644 --- a/WebATM-integrated/webatm_integrated/process_manager.py +++ b/WebATM-integrated/webatm_integrated/process_manager.py @@ -32,6 +32,14 @@ def _default_spawn(target: Callable, *args) -> threading.Thread: return thread +def _signal_group(pgid: int, sig: int) -> None: + """Signal a process group, tolerating one that has already exited.""" + try: + os.killpg(pgid, sig) + except ProcessLookupError: + pass + + class BlueSkyProcessManager: """Thread-safe lifecycle manager for the ``bluesky --headless`` process tree. @@ -189,59 +197,77 @@ def stop(self, sig: int = signal.SIGTERM, escalate_after: float = 5.0) -> dict: "message": "BlueSky server is not running", } self._state = "stopping" - try: - pgid = os.getpgid(proc.pid) - except ProcessLookupError: - self._state = "stopped" - return { - "success": True, - "status": "stopped", - "message": "BlueSky server already exited", - } - # Signal the whole group (server + all node children) outside the lock. try: - os.killpg(pgid, sig) + return self._terminate(proc, sig, escalate_after) + except BaseException: + # Never leave the state stranded at "stopping": start() waits out a + # shutdown in that state, so it would block for 15s and then refuse + # to start, with no control surface able to clear it. + self._settle(proc) + raise + + def _terminate( + self, proc: subprocess.Popen, sig: int, escalate_after: float + ) -> dict: + """Signal ``proc``'s group, escalating to SIGKILL, and settle the state.""" + try: + pgid = os.getpgid(proc.pid) except ProcessLookupError: - pass + self._settle(proc) + return { + "success": True, + "status": "stopped", + "message": "BlueSky server already exited", + } + # Signal the whole group (server + all node children) outside the lock. + _signal_group(pgid, sig) try: proc.wait(timeout=escalate_after) except subprocess.TimeoutExpired: - try: - os.killpg(pgid, signal.SIGKILL) - except ProcessLookupError: - pass + _signal_group(pgid, signal.SIGKILL) try: proc.wait(timeout=5) except subprocess.TimeoutExpired: logger.error("BlueSky process group %s survived SIGKILL", pgid) - with self._lock: - if self._proc is proc: - self._state = "running" + self._settle(proc) return { "success": False, "status": "error", "message": "BlueSky server did not exit after SIGKILL", } - with self._lock: - if self._proc is proc: - self._state = "stopped" + self._settle(proc) return { "success": True, "status": "stopped", "message": "BlueSky server stopped", } + def _settle(self, proc: subprocess.Popen) -> None: + """Re-derive the state from whether ``proc`` is actually still alive. + + Skipped if a restart already installed a different process, whose own + state must not be clobbered by this one's teardown. + """ + with self._lock: + if self._proc is proc: + self._state = "running" if proc.poll() is None else "stopped" + def kill(self) -> dict: """Force-kill the whole process group immediately (no graceful wait). Returns: dict: Result with ``success``, ``status`` and ``message``. """ + with self._lock: + proc = self._proc + was_running = proc is not None and proc.poll() is None result = self.stop(sig=signal.SIGKILL, escalate_after=2.0) - if result.get("success") and result.get("status") == "stopped": + # Only claim a kill if there was actually a live process to kill -- + # otherwise keep stop()'s "is not running", which the UI shows verbatim. + if was_running and result.get("success") and result.get("status") == "stopped": result["message"] = "BlueSky server killed" return result From 766672b00b58b0cebcb50888a9e6805382a2429d Mon Sep 17 00:00:00 2001 From: Andres Morfin Veytia <78442543+amorfinv@users.noreply.github.com> Date: Wed, 19 Aug 2026 20:40:41 +0200 Subject: [PATCH 3/7] Fix map style selector state and restore persisted MapTiler API key The Apply Map Style button now hides when the placeholder is selected (it used to stay visible after deleting a saved style), and saving a custom style whose URL duplicates a predefined option now selects the saved optgroup entry instead of the predefined one, which hid its Delete button. Selector wiring is characterized by a new test file. SettingsModal now restores the API key embedded in a persisted MapTiler style URL when the modal opens, so Apply works without re-entering it; a key the user already typed is never clobbered. Co-Authored-By: Claude Fable 5 --- frontend/src/ui/SettingsModal.test.ts | 35 ++++- frontend/src/ui/SettingsModal.ts | 14 +- .../ui/map/MapStyleManager.selector.test.ts | 134 ++++++++++++++++++ frontend/src/ui/map/MapStyleManager.ts | 116 +++++++-------- 4 files changed, 226 insertions(+), 73 deletions(-) create mode 100644 frontend/src/ui/map/MapStyleManager.selector.test.ts diff --git a/frontend/src/ui/SettingsModal.test.ts b/frontend/src/ui/SettingsModal.test.ts index 4bc19fb..824799d 100644 --- a/frontend/src/ui/SettingsModal.test.ts +++ b/frontend/src/ui/SettingsModal.test.ts @@ -62,7 +62,12 @@ function buildSettingsDom(): void { - + @@ -207,6 +212,34 @@ describe('SettingsModal connect flow', () => { expect(connect.disabled).toBe(true); }); + it('restores the API key of a persisted MapTiler style on open', async () => { + const maptilerUrl = 'https://api.maptiler.com/maps/streets/style.json?key=ABC123'; + // StorageManager namespaces with 'webatm-' and JSON-encodes values. + localStorage.setItem('webatm-webatm-map-style', JSON.stringify(maptilerUrl)); + + beforeOpenCallback?.('beforeOpen', 'settings-modal'); + await flushAsync(); + + const select = document.getElementById('map-style-select-modal') as HTMLSelectElement; + const keyInput = document.getElementById('maptiler-api-key-input') as HTMLInputElement; + expect(select.value).toBe('https://api.maptiler.com/maps/streets/style.json?key='); + expect(keyInput.value).toBe('ABC123'); + }); + + it('does not clobber a key the user already typed', async () => { + localStorage.setItem( + 'webatm-webatm-map-style', + JSON.stringify('https://api.maptiler.com/maps/streets/style.json?key=OLD') + ); + const keyInput = document.getElementById('maptiler-api-key-input') as HTMLInputElement; + keyInput.value = 'TYPED'; + + beforeOpenCallback?.('beforeOpen', 'settings-modal'); + await flushAsync(); + + expect(keyInput.value).toBe('TYPED'); + }); + it('a failed connect re-enables Connect when the server is still up', async () => { const input = document.getElementById('server-ip-input') as HTMLInputElement; const connect = document.getElementById('connect-server') as HTMLButtonElement; diff --git a/frontend/src/ui/SettingsModal.ts b/frontend/src/ui/SettingsModal.ts index 199f6e2..ac5516e 100644 --- a/frontend/src/ui/SettingsModal.ts +++ b/frontend/src/ui/SettingsModal.ts @@ -359,12 +359,18 @@ export class SettingsModal { // Direct match, or a MapTiler style where the saved value has // ?key=ABC123 and the option value ends with a bare ?key= - if ( - option.value === savedStyle || - (option.value.endsWith('?key=') && savedStyle.startsWith(option.value)) - ) { + const isKeyedMatch = + option.value.endsWith('?key=') && savedStyle.startsWith(option.value); + if (option.value === savedStyle || isKeyedMatch) { this.elements.mapStyleSelect.selectedIndex = i; matchFound = true; + + // Restore the key embedded in the saved URL so Apply works + // without re-entering it; keep any key the user has typed. + const keyInput = this.elements.mapTilerApiKeyInput; + if (isKeyedMatch && keyInput && !keyInput.value.trim()) { + keyInput.value = savedStyle.slice(option.value.length); + } break; } } diff --git a/frontend/src/ui/map/MapStyleManager.selector.test.ts b/frontend/src/ui/map/MapStyleManager.selector.test.ts new file mode 100644 index 0000000..def020f --- /dev/null +++ b/frontend/src/ui/map/MapStyleManager.selector.test.ts @@ -0,0 +1,134 @@ +// @vitest-environment happy-dom +/** + * Characterizes the settings-modal style selector wiring in MapStyleManager: + * which controls are visible for each kind of selection, and the save/delete + * flow for user-saved custom styles. + * + * Regressions covered: + * - the "Apply Map Style" button must hide when the placeholder is selected + * (it used to stay visible after deleting a saved style) + * - saving a style whose URL duplicates a predefined option must select the + * saved optgroup entry, not the predefined option (which hid Delete) + */ +import { describe, it, expect, beforeEach, vi } from 'vitest'; + +vi.mock('../../utils/Logger', () => ({ + logger: { + error: vi.fn(), + warn: vi.fn(), + info: vi.fn(), + debug: vi.fn(), + verbose: vi.fn() + } +})); + +import { MapStyleManager } from './MapStyleManager'; +import { loadSavedStyles } from './customStyles'; + +const PREDEFINED_URL = '/static/map/offline-style-light.json'; + +function buildSelectorDom(): void { + document.body.innerHTML = ` + + + + +
+ `; +} + +const el = (id: string): T => + document.getElementById(id) as T; +const visible = (id: string): boolean => el(id).style.display !== 'none'; + +function selectValue(value: string): void { + const select = el('map-style-select-modal'); + select.value = value; + select.dispatchEvent(new Event('change')); +} + +function saveStyle(name: string, url: string): void { + el('custom-style-url-modal').value = url; + el('custom-style-name-modal').value = name; + el('save-custom-style-modal').click(); +} + +describe('MapStyleManager style selector', () => { + beforeEach(() => { + localStorage.clear(); + buildSelectorDom(); + vi.stubGlobal('alert', vi.fn()); + vi.stubGlobal('confirm', vi.fn(() => true)); + // Map access is irrelevant here: changeStyle() bails out on a null + // map after the persistence step, which is all these tests need. + new MapStyleManager(() => null).setupStyleSelector(); + }); + + it('shows Apply and hides the custom control for a predefined style', () => { + selectValue('https://tiles.example/positron'); + expect(visible('apply-map-style-btn')).toBe(true); + expect(visible('custom-style-control-modal')).toBe(false); + }); + + it('shows the custom control and hides Apply for "custom"', () => { + selectValue('custom'); + expect(visible('custom-style-control-modal')).toBe(true); + expect(visible('apply-map-style-btn')).toBe(false); + }); + + it('hides both Apply and the custom control for the placeholder', () => { + selectValue('https://tiles.example/positron'); + selectValue(''); + expect(visible('apply-map-style-btn')).toBe(false); + expect(visible('custom-style-control-modal')).toBe(false); + }); + + it('saving a style adds it to the Saved Styles group, selects it, and offers Delete', () => { + selectValue('custom'); + saveStyle('My Style', 'https://my/style.json'); + + const select = el('map-style-select-modal'); + const selected = select.selectedOptions[0]; + expect(selected.textContent).toBe('My Style'); + expect(selected.dataset.savedStyle).toBe('true'); + expect(selected.closest('optgroup')?.label).toBe('Saved Styles'); + expect(visible('delete-saved-style-btn')).toBe(true); + expect(loadSavedStyles()).toEqual([ + { name: 'My Style', url: 'https://my/style.json' } + ]); + }); + + it('saving a URL that duplicates a predefined option still selects the saved entry', () => { + selectValue('custom'); + saveStyle('My Light', PREDEFINED_URL); + + const selected = el('map-style-select-modal').selectedOptions[0]; + expect(selected.textContent).toBe('My Light'); + expect(selected.dataset.savedStyle).toBe('true'); + expect(visible('delete-saved-style-btn')).toBe(true); + }); + + it('deleting a saved style resets to the placeholder with Apply and Delete hidden', () => { + selectValue('custom'); + saveStyle('My Style', 'https://my/style.json'); + + el('delete-saved-style-btn').click(); + + const select = el('map-style-select-modal'); + expect(select.value).toBe(''); + expect(visible('apply-map-style-btn')).toBe(false); + expect(visible('delete-saved-style-btn')).toBe(false); + expect(loadSavedStyles()).toEqual([]); + expect(select.querySelector('#saved-custom-styles-group')).toBeNull(); + }); +}); diff --git a/frontend/src/ui/map/MapStyleManager.ts b/frontend/src/ui/map/MapStyleManager.ts index 296bd28..3381b1b 100644 --- a/frontend/src/ui/map/MapStyleManager.ts +++ b/frontend/src/ui/map/MapStyleManager.ts @@ -41,18 +41,14 @@ export class MapStyleManager { private currentStyle: string = ''; // How long the first-load reachability probe waits for the remote style - // document before declaring it unreachable. Generous enough that a slow - // but working connection never trips it (the style document is a few KB), - // short enough that an air-gapped first load recovers in seconds instead - // of hanging on the browser's connect timeout (which can be minutes). + // document (a few KB) before declaring it unreachable — generous for a + // slow link, but far shorter than the browser's connect timeout. private readonly FIRST_LOAD_PROBE_TIMEOUT_MS = 5000; constructor( private readonly getMap: () => Map | null, - // Invoked synchronously right before every map.setStyle() call, from - // every trigger (user selection, first-load probe, network-error - // fallback) - lets other renderers tear down layers they attached to - // the outgoing style's sources before MapLibre's diff runs. + // Invoked synchronously right before every map.setStyle() call, so + // other renderers can tear down layers on the outgoing style first. private readonly onBeforeStyleChange?: () => void ) {} @@ -115,19 +111,13 @@ export class MapStyleManager { * Make the first-load offline fallback deterministic. Call once right * after the map is constructed, when the initial style may be remote. * - * The 'error'-event fallback in handleMapError only helps when the failed - * style fetch actually *rejects*. On an air-gapped network the request to - * the basemap CDN often just hangs (dropped packets, DNS blackhole, proxy - * sink) — no error event ever fires, the map never gets a style, and the - * user stares at a blank basemap until the OS connect timeout (minutes) - * finally rejects the fetch. Only then does the fallback fire and persist - * the offline style, which is why a later reload "fixed" it. - * - * So probe the same style document the map is fetching, with our own - * timeout. If the probe fails (network error, timeout, or HTTP error) - * while the map still has no loaded style, swap to the bundled offline - * basemap immediately. If the map's style loads first — or the user picks - * a different style meanwhile — the probe result is ignored. + * The 'error'-event fallback in handleMapError only fires if the style + * fetch rejects; on an air-gapped network it often just hangs for + * minutes, leaving a blank basemap. So probe the same style document + * with our own timeout, and if the probe fails while the map still has + * no loaded style, swap to the bundled offline basemap immediately. If + * the style loads first — or the user picks another style meanwhile — + * the probe result is ignored. */ public armFirstLoadFallback(): void { const map = this.getMap(); @@ -212,25 +202,14 @@ export class MapStyleManager { /** * Decide whether a MapLibre error warrants swapping to the offline style. + * Only once, and only when the current style is a remote URL — a local + * style failing usually means a config mistake, not missing internet. * - * MapLibre surfaces a few shapes for network failures: AJAXError objects - * with a `status` field (0 when the browser couldn't reach the host at - * all), and generic `TypeError: Failed to fetch` for cross-origin / DNS - * problems. We only fall back once, and only if the current style is a - * remote URL — local styles failing usually mean a config mistake, not - * missing internet. - * - * Crucially we do NOT fall back on individual *tile* fetch failures (errors - * that carry a `tile`). Those are common and transient — a single dropped - * or CORS-blocked vector tile while panning should not swap the entire - * basemap to offline. Swapping the style mid-session reloads every layer: - * the basemap visibly flickers, and (with the 3D overlay on) rebuilding the - * Three.js custom layer disrupts the viewport and snaps the camera back to - * the default view. The map degrades gracefully on a missing tile (the - * tile is simply blank until it succeeds), so a tile error is never reason - * enough to nuke the user's chosen basemap. A genuinely-offline boot still - * triggers the fallback because the *style document* fetch fails, and that - * error carries no `tile`. + * Individual *tile* failures (errors carrying `tile`) never trigger the + * fallback: they are common and transient, and swapping the whole basemap + * mid-session reloads every layer and disrupts the 3D overlay's camera. A + * genuinely-offline boot still falls back, because there the *style + * document* fetch fails and that error carries no `tile`. */ private shouldFallBackToOffline(e: MapErrorEvent): boolean { if (this.hasFallenBackToOffline) return false; @@ -289,35 +268,18 @@ export class MapStyleManager { deleteSavedStyleBtn.style.display = isSaved ? 'block' : 'none'; }; - // Handle style select change - only toggle custom input visibility - styleSelect.addEventListener('change', (e) => { - const target = e.target as HTMLSelectElement; - - if (target.value === 'custom') { - // Show custom style input - if (customStyleControl) { - customStyleControl.style.display = 'block'; - } - // Hide apply button for predefined styles - if (applyMapStyleBtn) { - applyMapStyleBtn.style.display = 'none'; - } - } else if (target.value === '') { - // User selected "Select a map style..." placeholder - if (customStyleControl) { - customStyleControl.style.display = 'none'; - } - } else { - // User selected a predefined style - just hide custom input - if (customStyleControl) { - customStyleControl.style.display = 'none'; - } - // Show apply button for predefined styles - if (applyMapStyleBtn) { - applyMapStyleBtn.style.display = 'block'; - } + // Match the controls to the selection: "custom" shows the custom-URL + // input, the placeholder shows neither, and any concrete style shows + // the Apply button. + styleSelect.addEventListener('change', () => { + const value = styleSelect.value; + if (customStyleControl) { + customStyleControl.style.display = value === 'custom' ? 'block' : 'none'; + } + if (applyMapStyleBtn) { + applyMapStyleBtn.style.display = + value === 'custom' || value === '' ? 'none' : 'block'; } - updateDeleteButton(); }); @@ -385,7 +347,7 @@ export class MapStyleManager { this.renderSavedStyles(styleSelect); // Select and apply the newly-saved style; clear the name field. - styleSelect.value = url; + this.selectSavedStyle(styleSelect, url); if (customStyleNameInput) customStyleNameInput.value = ''; this.changeStyle(url); styleSelect.dispatchEvent(new Event('change')); @@ -439,6 +401,24 @@ export class MapStyleManager { logger.debug('MapStyleManager', 'Map style selector initialized'); } + /** + * Select the just-saved option inside the "Saved Styles" optgroup. A plain + * `select.value = url` would land on the first option with that value — + * which, when the URL duplicates a predefined option's, is the predefined + * one, leaving the saved entry unselectable and hiding its Delete button. + */ + private selectSavedStyle(select: HTMLSelectElement, url: string): void { + const group = select.querySelector(`#${this.SAVED_GROUP_ID}`); + const option = group + ? Array.from(group.querySelectorAll('option')).find(o => o.value === url) + : undefined; + if (option) { + option.selected = true; + } else { + select.value = url; + } + } + /** * (Re)build the "Saved Styles" optgroup in the style dropdown from the * persisted custom-style list. The group is inserted just above the From a48e652fb17044c8605b35310fce1859ac1eca01 Mon Sep 17 00:00:00 2001 From: Andres Morfin Veytia <78442543+amorfinv@users.noreply.github.com> Date: Wed, 19 Aug 2026 20:40:52 +0200 Subject: [PATCH 4/7] Validate shape altitude inputs instead of passing NaN or dropping them The draw modal's top/bottom altitude fields are now parsed by a shared parseAltitudeInputs helper: non-numeric values are rejected instead of leaking into the BlueSky command as NaN, and a half-filled pair is rejected instead of being silently dropped. Modal state is only mutated once validation passes. Co-Authored-By: Claude Fable 5 --- .../ui/map/shapes/ShapeDrawingManager.test.ts | 19 ++++++ .../src/ui/map/shapes/ShapeDrawingManager.ts | 58 +++++++------------ .../src/ui/map/shapes/shapeCommand.test.ts | 38 +++++++++++- frontend/src/ui/map/shapes/shapeCommand.ts | 32 ++++++++++ 4 files changed, 110 insertions(+), 37 deletions(-) diff --git a/frontend/src/ui/map/shapes/ShapeDrawingManager.test.ts b/frontend/src/ui/map/shapes/ShapeDrawingManager.test.ts index a2700e4..9dc7842 100644 --- a/frontend/src/ui/map/shapes/ShapeDrawingManager.test.ts +++ b/frontend/src/ui/map/shapes/ShapeDrawingManager.test.ts @@ -173,6 +173,25 @@ describe('ShapeDrawingManager create validation and finish', () => { expect(manager.isDrawing()).toBe(false); }); + it('rejects a non-numeric altitude instead of sending NaN to BlueSky', () => { + fillModal('TMA1'); + (document.getElementById('polygon-top-input') as HTMLInputElement).value = 'abc'; + (document.getElementById('polygon-bottom-input') as HTMLInputElement).value = '2000'; + clickCreate(); + + expect(alertMock).toHaveBeenCalledWith('Altitudes must be numbers (in feet)'); + expect(manager.isDrawing()).toBe(false); + }); + + it('rejects a half-filled altitude pair instead of silently dropping it', () => { + fillModal('TMA2'); + (document.getElementById('polygon-top-input') as HTMLInputElement).value = '10000'; + clickCreate(); + + expect(alertMock).toHaveBeenCalledWith('Enter both top and bottom altitudes, or leave both empty'); + expect(manager.isDrawing()).toBe(false); + }); + it('draws and sends exactly one command on finish', async () => { fillModal('AREA1'); clickCreate(); diff --git a/frontend/src/ui/map/shapes/ShapeDrawingManager.ts b/frontend/src/ui/map/shapes/ShapeDrawingManager.ts index 4a39ad0..73f0b1f 100644 --- a/frontend/src/ui/map/shapes/ShapeDrawingManager.ts +++ b/frontend/src/ui/map/shapes/ShapeDrawingManager.ts @@ -13,7 +13,7 @@ import { setLayerVisibility, updateSourceFeatures } from '../../../utils/maplibre'; -import { buildShapeCommand, ShapeType } from './shapeCommand'; +import { buildShapeCommand, parseAltitudeInputs, ShapeType } from './shapeCommand'; import { boxCornerPoints, circleRingPoints, distanceNm } from './shapeGeometry'; /** Modal title per shape type. */ @@ -151,33 +151,34 @@ export class ShapeDrawingManager extends BaseDrawingManager { return; } - this.currentShapeName = name; - this.currentShapeType = shapeType; - // Altitudes apply to everything but lines; on an area both filled // makes a POLYALT, on a box/circle they become the trailing // [top,bottom] arguments. + let topAltitude: number | null = null; + let bottomAltitude: number | null = null; if (shapeType !== 'line') { - const topValue = topInput?.value; - const bottomValue = bottomInput?.value; - - if (topValue && bottomValue) { - this.topAltitude = parseFloat(topValue); - this.bottomAltitude = parseFloat(bottomValue); - - if (this.topAltitude <= this.bottomAltitude) { - alert('Top altitude must be greater than bottom altitude'); - return; - } - } else { - this.topAltitude = null; - this.bottomAltitude = null; + // A number input showing unparseable text (e.g. "1e999") reads + // back as value '' - catch that as bad input, not as "empty". + if (topInput?.validity?.badInput || bottomInput?.validity?.badInput) { + alert('Altitudes must be numbers (in feet)'); + topInput?.focus(); + return; } - } else { - this.topAltitude = null; - this.bottomAltitude = null; + const parsed = parseAltitudeInputs(topInput?.value ?? '', bottomInput?.value ?? ''); + if (!parsed.ok) { + alert(parsed.error); + topInput?.focus(); + return; + } + topAltitude = parsed.top; + bottomAltitude = parsed.bottom; } + this.currentShapeName = name; + this.currentShapeType = shapeType; + this.topAltitude = topAltitude; + this.bottomAltitude = bottomAltitude; + modalManager.close('polygon-name-modal'); this.startDrawing(); } @@ -219,24 +220,15 @@ export class ShapeDrawingManager extends BaseDrawingManager { logger.info('ShapeDrawingManager', 'Stopped drawing'); } - /** - * Set up temporary drawing layers when drawing starts. - */ protected onDrawingEnabled(): void { this.setupTemporaryDrawingLayers(); } - /** - * Clear and remove temporary drawing layers when drawing stops. - */ protected onDrawingDisabled(): void { this.clearTemporaryDrawing(); this.removeTemporaryDrawingLayers(); } - /** - * Handle a placed point - update banner and preview. - */ protected onPointAdded(point: DrawingPoint): void { this.drawingPoints.push(point); @@ -258,9 +250,6 @@ export class ShapeDrawingManager extends BaseDrawingManager { return this.currentShapeType === 'box' || this.currentShapeType === 'circle'; } - /** - * Handle mouse move - update cursor preview - */ protected onCursorMove(point: DrawingPoint): void { if (this.drawingPoints.length === 0) return; this.updateCursorPreview(point); @@ -334,9 +323,6 @@ export class ShapeDrawingManager extends BaseDrawingManager { this.stopDrawing(); } - /** - * Generate the BlueSky command string for the current shape. - */ private generateCommand(): string { return buildShapeCommand({ name: this.currentShapeName ?? '', diff --git a/frontend/src/ui/map/shapes/shapeCommand.test.ts b/frontend/src/ui/map/shapes/shapeCommand.test.ts index 2de2f17..4d43041 100644 --- a/frontend/src/ui/map/shapes/shapeCommand.test.ts +++ b/frontend/src/ui/map/shapes/shapeCommand.test.ts @@ -1,5 +1,41 @@ import { describe, it, expect } from 'vitest'; -import { buildShapeCommand } from './shapeCommand'; +import { buildShapeCommand, parseAltitudeInputs } from './shapeCommand'; + +describe('parseAltitudeInputs', () => { + it('accepts both fields empty (no vertical extent)', () => { + expect(parseAltitudeInputs('', '')).toEqual({ ok: true, top: null, bottom: null }); + expect(parseAltitudeInputs(' ', ' ')).toEqual({ ok: true, top: null, bottom: null }); + }); + + it('accepts a valid top/bottom pair, trimming whitespace', () => { + expect(parseAltitudeInputs(' 10000 ', '2000')).toEqual({ ok: true, top: 10000, bottom: 2000 }); + }); + + it('rejects a half-filled pair instead of silently dropping it', () => { + expect(parseAltitudeInputs('10000', '')).toEqual({ + ok: false, + error: 'Enter both top and bottom altitudes, or leave both empty' + }); + expect(parseAltitudeInputs('', '2000')).toMatchObject({ ok: false }); + }); + + it('rejects non-numeric values instead of passing NaN through', () => { + expect(parseAltitudeInputs('abc', '2000')).toEqual({ + ok: false, + error: 'Altitudes must be numbers (in feet)' + }); + expect(parseAltitudeInputs('10000', '20oo')).toMatchObject({ ok: false }); + expect(parseAltitudeInputs('Infinity', '2000')).toMatchObject({ ok: false }); + }); + + it('rejects top at or below bottom', () => { + expect(parseAltitudeInputs('2000', '10000')).toEqual({ + ok: false, + error: 'Top altitude must be greater than bottom altitude' + }); + expect(parseAltitudeInputs('2000', '2000')).toMatchObject({ ok: false }); + }); +}); describe('buildShapeCommand', () => { const points = [ diff --git a/frontend/src/ui/map/shapes/shapeCommand.ts b/frontend/src/ui/map/shapes/shapeCommand.ts index e27ca96..8f371e2 100644 --- a/frontend/src/ui/map/shapes/shapeCommand.ts +++ b/frontend/src/ui/map/shapes/shapeCommand.ts @@ -14,6 +14,38 @@ export interface ShapePoint { /** The shape kinds offered by the draw modal. */ export type ShapeType = 'area' | 'line' | 'circle' | 'box'; +export type AltitudeParseResult = + | { ok: true; top: number | null; bottom: number | null } + | { ok: false; error: string }; + +/** + * Parse the draw modal's optional top/bottom altitude inputs. Both empty + * means no vertical extent; anything else needs two finite numbers with top + * above bottom, so a non-numeric value can't leak into the generated command + * as NaN and a half-filled pair isn't silently dropped. + */ +export function parseAltitudeInputs(topRaw: string, bottomRaw: string): AltitudeParseResult { + const top = topRaw.trim(); + const bottom = bottomRaw.trim(); + + if (!top && !bottom) { + return { ok: true, top: null, bottom: null }; + } + if (!top || !bottom) { + return { ok: false, error: 'Enter both top and bottom altitudes, or leave both empty' }; + } + + const topNum = Number(top); + const bottomNum = Number(bottom); + if (!Number.isFinite(topNum) || !Number.isFinite(bottomNum)) { + return { ok: false, error: 'Altitudes must be numbers (in feet)' }; + } + if (topNum <= bottomNum) { + return { ok: false, error: 'Top altitude must be greater than bottom altitude' }; + } + return { ok: true, top: topNum, bottom: bottomNum }; +} + export interface ShapeCommandSpec { name: string; type: ShapeType; From 30d5422a85fbb09859c4f7bdda01b0a16779cf48 Mon Sep 17 00:00:00 2001 From: Andres Morfin Veytia <78442543+amorfinv@users.noreply.github.com> Date: Wed, 19 Aug 2026 20:41:03 +0200 Subject: [PATCH 5/7] Tie 3D scale and model controls to the overlay toggle The aircraft 3D scale and model containers were pinned visible by CSS !important rules, so they ignored the overlay state entirely. The panel now drives their visibility from the toggle (including initial load from storage), and a failed overlay toggle rolls back storage too, so the failure isn't resurrected on the next page load. Co-Authored-By: Claude Fable 5 --- WebATM/static/css/style.css | 14 --- .../panels/left/DisplayOptionsPanel.test.ts | 92 +++++++++++++++++++ .../src/ui/panels/left/DisplayOptionsPanel.ts | 56 +++++------ 3 files changed, 121 insertions(+), 41 deletions(-) diff --git a/WebATM/static/css/style.css b/WebATM/static/css/style.css index c8bf6d5..d5da0d0 100644 --- a/WebATM/static/css/style.css +++ b/WebATM/static/css/style.css @@ -957,20 +957,6 @@ html.wa-open-threeD-controls #threeD-controls { min-width: 80px; } -/* Aircraft model dropdown - ensure proper right alignment */ -#aircraft-model-container { - display: flex !important; - justify-content: space-between !important; - align-items: center !important; -} - -/* Aircraft 3D scale input - ensure proper right alignment */ -#aircraft-3d-scale-container { - display: flex !important; - justify-content: space-between !important; - align-items: center !important; -} - /* Aircraft appearance controls group */ .aircraft-appearance-controls { margin-top: 8px; diff --git a/frontend/src/ui/panels/left/DisplayOptionsPanel.test.ts b/frontend/src/ui/panels/left/DisplayOptionsPanel.test.ts index b305cec..5c2fcad 100644 --- a/frontend/src/ui/panels/left/DisplayOptionsPanel.test.ts +++ b/frontend/src/ui/panels/left/DisplayOptionsPanel.test.ts @@ -3,6 +3,8 @@ import { describe, it, expect, beforeEach, vi } from 'vitest'; import { DisplayOptionsPanel } from './DisplayOptionsPanel'; import { StateManager } from '../../../core/StateManager'; import { storage } from '../../../utils/StorageManager'; +import { DisplayOptions } from '../../../data/types'; +import type { App } from '../../../core/App'; // loadAvailableAircraftModels fetches the model catalog during setup; // let it fail fast and resolve to []. @@ -38,11 +40,27 @@ function buildDom(): void { + +
+ + +
`; } +/** Minimal App stand-in exposing a MapOverlay with the given toggle result. */ +function fakeApp(updateDisplayOptions: (options: Partial) => Promise): App { + return { + getMapOverlay: () => ({ updateDisplayOptions }), + } as unknown as App; +} + function createPanel(): { panel: DisplayOptionsPanel; stateManager: StateManager } { const panel = new DisplayOptionsPanel(); panel.init(); @@ -216,4 +234,78 @@ describe('DisplayOptionsPanel', () => { expect(panel.setBooleanOption('aircraftIconColor')).toBe(null); }); }); + + describe('3D overlay toggle', () => { + function containerVisible(id: string): boolean { + return (document.getElementById(id) as HTMLElement).style.display !== 'none'; + } + + function toggleOverlay(checked: boolean): void { + const box = checkbox('show-3d-overlay'); + box.checked = checked; + box.dispatchEvent(new Event('change')); + } + + it('enabling the overlay reveals the 3D scale and model controls', async () => { + const { stateManager } = createPanel(); + // Regression: nothing drove these containers' visibility (a CSS + // !important rule kept them shown even with the overlay off), so + // the controls ignored the overlay state entirely. + expect(containerVisible('aircraft-3d-scale-container')).toBe(false); + expect(containerVisible('aircraft-model-container')).toBe(false); + + toggleOverlay(true); + + await vi.waitFor(() => expect(containerVisible('aircraft-3d-scale-container')).toBe(true)); + expect(containerVisible('aircraft-model-container')).toBe(true); + expect(storage.get('show-3d-overlay')).toBe(true); + expect(stateManager.getDisplayOptions().show3DOverlay).toBe(true); + }); + + it('disabling the overlay hides the 3D controls again', async () => { + createPanel(); + toggleOverlay(true); + await vi.waitFor(() => expect(containerVisible('aircraft-3d-scale-container')).toBe(true)); + + toggleOverlay(false); + + await vi.waitFor(() => expect(containerVisible('aircraft-3d-scale-container')).toBe(false)); + expect(containerVisible('aircraft-model-container')).toBe(false); + }); + + it('a stored overlay=true reveals the 3D controls on load', () => { + storage.set('show-3d-overlay', true); + + const { stateManager } = createPanel(); + + expect(stateManager.getDisplayOptions().show3DOverlay).toBe(true); + expect(containerVisible('aircraft-3d-scale-container')).toBe(true); + expect(containerVisible('aircraft-model-container')).toBe(true); + }); + + it('passes the new overlay value to the MapOverlay', async () => { + const update = vi.fn().mockResolvedValue(undefined); + const { panel } = createPanel(); + panel.setApp(fakeApp(update)); + + toggleOverlay(true); + + await vi.waitFor(() => expect(update).toHaveBeenCalledWith({ show3DOverlay: true })); + }); + + it('a failed overlay toggle rolls back checkbox, storage, state and controls', async () => { + const { panel, stateManager } = createPanel(); + panel.setApp(fakeApp(() => Promise.reject(new Error('WebGL unavailable')))); + + toggleOverlay(true); + + await vi.waitFor(() => expect(checkbox('show-3d-overlay').checked).toBe(false)); + // Regression: storage used to keep true here, resurrecting the + // failed overlay on the next page load. + expect(storage.get('show-3d-overlay')).toBe(false); + expect(stateManager.getDisplayOptions().show3DOverlay).toBe(false); + expect(containerVisible('aircraft-3d-scale-container')).toBe(false); + expect(containerVisible('aircraft-model-container')).toBe(false); + }); + }); }); diff --git a/frontend/src/ui/panels/left/DisplayOptionsPanel.ts b/frontend/src/ui/panels/left/DisplayOptionsPanel.ts index c411158..1289d0e 100644 --- a/frontend/src/ui/panels/left/DisplayOptionsPanel.ts +++ b/frontend/src/ui/panels/left/DisplayOptionsPanel.ts @@ -208,6 +208,7 @@ export class DisplayOptionsPanel extends BasePanel { } update.show3DOverlay = show3DOverlay; this.setChecked('show-3d-overlay', show3DOverlay); + this.apply3DControlsVisibility(show3DOverlay); const aircraft3DScale = this.loadStored('aircraft-3d-scale', defaults.aircraft3DScale); update.aircraft3DScale = aircraft3DScale; @@ -324,39 +325,40 @@ export class DisplayOptionsPanel extends BasePanel { private setupRenderModeControl(): void { this.bindCheckbox('show-3d-overlay', async (checked) => { storage.set('show-3d-overlay', checked); - - if (this.stateManager) { - this.stateManager.updateDisplayOptions({ show3DOverlay: checked }); - } - - if (this.app) { - const mapOverlay = this.app.getMapOverlay(); - if (mapOverlay) { - try { - logger.info('DisplayOptionsPanel', `${checked ? 'Enabling' : 'Disabling'} 3D overlay...`); - await mapOverlay.updateDisplayOptions({ show3DOverlay: checked }); - logger.info('DisplayOptionsPanel', `3D overlay ${checked ? 'enabled' : 'disabled'} successfully`); - } catch (error) { - // Roll the checkbox and state back so UI matches reality - logger.error('DisplayOptionsPanel', `Failed to toggle 3D overlay: ${error}`); - this.setChecked('show-3d-overlay', !checked); - if (this.stateManager) { - this.stateManager.updateDisplayOptions({ show3DOverlay: !checked }); - } - } + this.stateManager?.updateDisplayOptions({ show3DOverlay: checked }); + + const mapOverlay = this.app?.getMapOverlay(); + if (mapOverlay) { + try { + logger.info('DisplayOptionsPanel', `${checked ? 'Enabling' : 'Disabling'} 3D overlay...`); + await mapOverlay.updateDisplayOptions({ show3DOverlay: checked }); + } catch (error) { + // Roll the checkbox, storage and state back so the UI and + // the next page load both match reality + logger.error('DisplayOptionsPanel', `Failed to toggle 3D overlay: ${error}`); + this.setChecked('show-3d-overlay', !checked); + storage.set('show-3d-overlay', !checked); + this.stateManager?.updateDisplayOptions({ show3DOverlay: !checked }); + this.apply3DControlsVisibility(!checked); + return; } } - // Reveal the 3D controls section when the overlay turns on - if (checked) { - const threeDControls = document.getElementById('threeD-controls'); - if (threeDControls && threeDControls.style.display === 'none') { - this.toggleCollapsibleSection('threeD-controls', 'threeD-visible'); - } - } + this.apply3DControlsVisibility(checked); }); } + /** + * Show the global 3D scale and model controls only while the overlay + * is on. Clearing the inline style falls back to their CSS flex layout. + */ + private apply3DControlsVisibility(visible: boolean): void { + for (const id of ['aircraft-3d-scale-container', 'aircraft-model-container']) { + const container = document.getElementById(id); + if (container) container.style.display = visible ? '' : 'none'; + } + } + private setup3DScaleControl(): void { // Apply on Enter (then drop focus) and on blur this.bindEvent('aircraft-3d-scale', 'keydown', (e) => { From 85b72b88eb5d0e34649ed845b2fff4982ca3aed5 Mon Sep 17 00:00:00 2001 From: Andres Morfin Veytia <78442543+amorfinv@users.noreply.github.com> Date: Wed, 19 Aug 2026 20:41:14 +0200 Subject: [PATCH 6/7] Fix node_info listener teardown and drop demo-mode leftovers SimulationNodesPanel now unsubscribes only its own node_info handler on destroy and re-wiring; the previous bare off('node_info') also detached SocketManager's forward listener for the same event, and re-setting the socket manager could double-subscribe. Node names in logs and the kill confirmation now use the shared alias helper. Also removes the unused demo-mode Add Node modal from the template and the dead setActiveNode/requestNodes pass-throughs in App/SocketManager. Co-Authored-By: Claude Fable 5 --- WebATM/templates/index.html | 28 --------- frontend/src/core/App.ts | 5 -- frontend/src/core/SocketManager.ts | 16 ----- .../panels/left/SimulationNodesPanel.test.ts | 62 +++++++++++++++++++ .../ui/panels/left/SimulationNodesPanel.ts | 47 ++++---------- 5 files changed, 74 insertions(+), 84 deletions(-) diff --git a/WebATM/templates/index.html b/WebATM/templates/index.html index c718a5e..6d239b7 100644 --- a/WebATM/templates/index.html +++ b/WebATM/templates/index.html @@ -1229,34 +1229,6 @@

Upload a Plugin

- - - diff --git a/frontend/src/core/App.ts b/frontend/src/core/App.ts index 6eb8dda..0e68621 100644 --- a/frontend/src/core/App.ts +++ b/frontend/src/core/App.ts @@ -488,11 +488,6 @@ export class App { return this.aircraftInteractionManager; } - public setActiveNode(nodeId: string): void { - this.socketManager.setActiveNode(nodeId); - this.stateManager.setActiveNode(nodeId); - } - public getState(): AppState { return this.stateManager.getState(); } diff --git a/frontend/src/core/SocketManager.ts b/frontend/src/core/SocketManager.ts index b7931e3..4ce3292 100644 --- a/frontend/src/core/SocketManager.ts +++ b/frontend/src/core/SocketManager.ts @@ -351,22 +351,6 @@ export class SocketManager { return false; } - setActiveNode(nodeId: string): void { - if (this.isConnected() && this.socket) { - this.socket.emit('set_active_node', { node_id: nodeId }); - } else { - logger.warn('SocketManager', 'Cannot set active node: not connected to WebATM'); - } - } - - requestNodes(): void { - if (this.isConnected() && this.socket) { - this.socket.emit('get_nodes'); - } else { - logger.warn('SocketManager', 'Cannot request nodes: not connected to WebATM'); - } - } - destroy(): void { if (this.socket) { this.socket.removeAllListeners(); diff --git a/frontend/src/ui/panels/left/SimulationNodesPanel.test.ts b/frontend/src/ui/panels/left/SimulationNodesPanel.test.ts index 1be0e6c..45c1ee2 100644 --- a/frontend/src/ui/panels/left/SimulationNodesPanel.test.ts +++ b/frontend/src/ui/panels/left/SimulationNodesPanel.test.ts @@ -202,4 +202,66 @@ describe('SimulationNodesPanel', () => { expect(emitted).toEqual([['set_active_node', { node_id: 'b' }]]); }); }); + + describe('socket wiring', () => { + type Listener = (data: NodeInfo) => void; + + // Fake with socket.io off() semantics: off(event) drops every + // listener for the event, off(event, fn) drops only fn. + const makeFakeSocket = () => { + const listeners = new Map>(); + const socket = { + connected: true, + on: (event: string, handler: Listener) => { + if (!listeners.has(event)) listeners.set(event, new Set()); + listeners.get(event)!.add(handler); + }, + off: (event: string, handler?: Listener) => { + if (handler) listeners.get(event)?.delete(handler); + else listeners.delete(event); + }, + emit: vi.fn(), + }; + return { socket, listeners }; + }; + + const asManager = (socket: unknown): SocketManager => + ({ getSocket: () => socket }) as unknown as SocketManager; + + it('destroy() removes only its own node_info listener', () => { + const { socket, listeners } = makeFakeSocket(); + // Stands in for SocketManager's own forward listener, which + // shares the socket and must survive the panel's teardown. + const outsider = vi.fn(); + socket.on('node_info', outsider); + panel.setSocketManager(asManager(socket)); + expect(listeners.get('node_info')!.size).toBe(2); + + panel.destroy(); + + const remaining = listeners.get('node_info'); + expect(remaining && [...remaining]).toEqual([outsider]); + }); + + it('re-setting the socket manager does not double-subscribe', () => { + const { socket, listeners } = makeFakeSocket(); + const manager = asManager(socket); + + panel.setSocketManager(manager); + panel.setSocketManager(manager); + + expect(listeners.get('node_info')!.size).toBe(1); + }); + + it('switching sockets unsubscribes from the old one', () => { + const first = makeFakeSocket(); + const second = makeFakeSocket(); + + panel.setSocketManager(asManager(first.socket)); + panel.setSocketManager(asManager(second.socket)); + + expect(first.listeners.get('node_info')?.size ?? 0).toBe(0); + expect(second.listeners.get('node_info')!.size).toBe(1); + }); + }); }); diff --git a/frontend/src/ui/panels/left/SimulationNodesPanel.ts b/frontend/src/ui/panels/left/SimulationNodesPanel.ts index ad1f722..fdc4d34 100644 --- a/frontend/src/ui/panels/left/SimulationNodesPanel.ts +++ b/frontend/src/ui/panels/left/SimulationNodesPanel.ts @@ -32,6 +32,7 @@ export class SimulationNodesPanel extends BasePanel { private socketManager: SocketManager | null = null; private nodeData: NodeInfo | null = null; private nodeItems: Map = new Map(); + private readonly nodeInfoHandler = (data: NodeInfo): void => this.handleNodeInfo(data); // DOM elements private totalNodesSpan: HTMLElement | null = null; @@ -85,7 +86,7 @@ export class SimulationNodesPanel extends BasePanel { // Add node button if (this.addNodeButton) { this.addEventListener(this.addNodeButton, 'click', () => { - this.showAddNodeModal(); + this.requestAddNode(); }); } } @@ -94,15 +95,10 @@ export class SimulationNodesPanel extends BasePanel { * Set the socket manager for communication with backend */ public setSocketManager(socketManager: SocketManager): void { + // Re-wiring must not leave a previous socket still calling our handler + this.socketManager?.getSocket()?.off('node_info', this.nodeInfoHandler); this.socketManager = socketManager; - - // Subscribe to node_info events - const socket = socketManager.getSocket(); - if (socket) { - socket.on('node_info', (data: NodeInfo) => { - this.handleNodeInfo(data); - }); - } + socketManager.getSocket()?.on('node_info', this.nodeInfoHandler); } /** @@ -335,9 +331,8 @@ export class SimulationNodesPanel extends BasePanel { return; } - // Get friendly node name for logging const nodeData = this.nodeData?.nodes[nodeId]; - const friendlyName = nodeData ? `Node ${nodeData.node_num || 1}` : nodeId; + const friendlyName = nodeData ? this.getNodeAlias(nodeData) : nodeId; logger.info('SimulationNodesPanel', 'Switching to node:', friendlyName); @@ -370,9 +365,9 @@ export class SimulationNodesPanel extends BasePanel { } /** - * Add a new node to the simulation + * Request a new simulation node from the backend */ - private showAddNodeModal(): void { + private requestAddNode(): void { if (!this.socketManager) { logger.warn('SimulationNodesPanel', 'Cannot add node: SocketManager not set'); return; @@ -416,7 +411,7 @@ export class SimulationNodesPanel extends BasePanel { } const nodeData = this.nodeData?.nodes[nodeId]; - const friendlyName = nodeData ? `Node ${nodeData.node_num || 1}` : nodeId; + const friendlyName = nodeData ? this.getNodeAlias(nodeData) : nodeId; if (!confirm(`Kill ${friendlyName}? Its running simulation will be lost.`)) { return; @@ -444,31 +439,13 @@ export class SimulationNodesPanel extends BasePanel { } } - /** - * Get current node data - */ - public getNodeData(): NodeInfo | null { - return this.nodeData; - } - - /** - * Get active node ID - */ - public getActiveNode(): string | null { - return this.nodeData?.active_node || null; - } - /** * Cleanup */ protected override onDestroy(): void { - // Unsubscribe from socket events - if (this.socketManager) { - const socket = this.socketManager.getSocket(); - if (socket) { - socket.off('node_info'); - } - } + // Remove only this panel's listener; a bare off('node_info') would + // also detach SocketManager's forward listener for the same event + this.socketManager?.getSocket()?.off('node_info', this.nodeInfoHandler); this.nodeData = null; this.socketManager = null; From d3d22074fcdd5fc7b8ddd9932efceeef04131eea Mon Sep 17 00:00:00 2001 From: Andres Morfin Veytia <78442543+amorfinv@users.noreply.github.com> Date: Wed, 19 Aug 2026 20:41:25 +0200 Subject: [PATCH 7/7] Normalize package-lock libc fields Artifact of the local npm version rewriting optional-dependency metadata; no dependency changes. Co-Authored-By: Claude Fable 5 --- frontend/package-lock.json | 30 ------------------------------ 1 file changed, 30 deletions(-) diff --git a/frontend/package-lock.json b/frontend/package-lock.json index 1c79917..963c800 100644 --- a/frontend/package-lock.json +++ b/frontend/package-lock.json @@ -559,9 +559,6 @@ "arm64" ], "dev": true, - "libc": [ - "glibc" - ], "license": "MIT", "optional": true, "os": [ @@ -579,9 +576,6 @@ "arm64" ], "dev": true, - "libc": [ - "musl" - ], "license": "MIT", "optional": true, "os": [ @@ -599,9 +593,6 @@ "ppc64" ], "dev": true, - "libc": [ - "glibc" - ], "license": "MIT", "optional": true, "os": [ @@ -619,9 +610,6 @@ "s390x" ], "dev": true, - "libc": [ - "glibc" - ], "license": "MIT", "optional": true, "os": [ @@ -639,9 +627,6 @@ "x64" ], "dev": true, - "libc": [ - "glibc" - ], "license": "MIT", "optional": true, "os": [ @@ -659,9 +644,6 @@ "x64" ], "dev": true, - "libc": [ - "musl" - ], "license": "MIT", "optional": true, "os": [ @@ -2849,9 +2831,6 @@ "arm64" ], "dev": true, - "libc": [ - "glibc" - ], "license": "MPL-2.0", "optional": true, "os": [ @@ -2873,9 +2852,6 @@ "arm64" ], "dev": true, - "libc": [ - "musl" - ], "license": "MPL-2.0", "optional": true, "os": [ @@ -2897,9 +2873,6 @@ "x64" ], "dev": true, - "libc": [ - "glibc" - ], "license": "MPL-2.0", "optional": true, "os": [ @@ -2921,9 +2894,6 @@ "x64" ], "dev": true, - "libc": [ - "musl" - ], "license": "MPL-2.0", "optional": true, "os": [