From 5da5fa8a455ea4fabaa791ccb49858d61dfdca31 Mon Sep 17 00:00:00 2001 From: Brian Krabach Date: Wed, 8 Apr 2026 11:29:55 -0700 Subject: [PATCH] refactor: rewrite sidebar functions to use server-side settings - Remove SIDEBAR_KEY constant (was an undefined variable bug) - Rewrite initSidebar() to read sidebarOpen from _serverSettings - Rewrite toggleSidebar() to derive state from DOM class and persist to server via patchServerSetting - Rewrite bindSidebarClickAway() to persist collapsed state via patchServerSetting instead of localStorage - Add 7 tests verifying SIDEBAR_KEY removal and _serverSettings usage --- muxplex/frontend/app.js | 80 ++++++++------------- muxplex/tests/test_frontend_js.py | 116 ++++++++++++++++++++++++++++-- 2 files changed, 141 insertions(+), 55 deletions(-) diff --git a/muxplex/frontend/app.js b/muxplex/frontend/app.js index 4bd93fe..98d10a8 100644 --- a/muxplex/frontend/app.js +++ b/muxplex/frontend/app.js @@ -598,24 +598,24 @@ const SIDEBAR_NARROW_THRESHOLD = 960; /** * Initialise sidebar open/closed state on page load. - * Reads muxplex.sidebarOpen from localStorage (JSON.parse with try/catch). + * Reads sidebarOpen from _serverSettings cache. * Defaults to open on wide screens (innerWidth >= 960) when no stored value. * Applies sidebar--collapsed class accordingly and persists the initial state. */ function initSidebar() { - let isOpen; - try { - const stored = localStorage.getItem(SIDEBAR_KEY); - if (stored !== null) { - isOpen = JSON.parse(stored); - } else { - isOpen = window.innerWidth >= SIDEBAR_NARROW_THRESHOLD; - } - } catch (_) { + var stored = _serverSettings ? _serverSettings.sidebarOpen : null; + var isOpen; + + if (stored !== null && stored !== undefined) { + isOpen = !!stored; + } else { isOpen = window.innerWidth >= SIDEBAR_NARROW_THRESHOLD; + // Persist the auto-detected value (fire-and-forget) + if (_serverSettings) _serverSettings.sidebarOpen = isOpen; + patchServerSetting('sidebarOpen', isOpen); } - const sidebar = $('session-sidebar'); + var sidebar = $('session-sidebar'); if (sidebar) { if (isOpen) { sidebar.classList.remove('sidebar--collapsed'); @@ -623,51 +623,32 @@ function initSidebar() { sidebar.classList.add('sidebar--collapsed'); } } - - // Persist initial state - try { - localStorage.setItem(SIDEBAR_KEY, JSON.stringify(isOpen)); - } catch (_) { /* blocked — ok */ } } /** * Toggle the sidebar open/closed state. - * Reads current state from localStorage, inverts it, persists, applies - * sidebar--collapsed class, and updates the collapse button text. + * Derives current state from DOM class, inverts it, persists to server, + * applies sidebar--collapsed class, and updates the collapse button text. * Button shows ‹ when open, › when closed. */ function toggleSidebar() { - let isOpen; - try { - const stored = localStorage.getItem(SIDEBAR_KEY); - isOpen = stored !== null ? JSON.parse(stored) : true; - } catch (_) { - isOpen = true; - } + var sidebar = $('session-sidebar'); + if (!sidebar) return; - // Invert state + var isOpen = !sidebar.classList.contains('sidebar--collapsed'); isOpen = !isOpen; - // Persist - try { - localStorage.setItem(SIDEBAR_KEY, JSON.stringify(isOpen)); - } catch (_) { /* blocked — ok */ } - - // Apply class - const sidebar = $('session-sidebar'); - if (sidebar) { - if (isOpen) { - sidebar.classList.remove('sidebar--collapsed'); - } else { - sidebar.classList.add('sidebar--collapsed'); - } + if (isOpen) { + sidebar.classList.remove('sidebar--collapsed'); + } else { + sidebar.classList.add('sidebar--collapsed'); } - // Update collapse button text (‹ when open, › when closed) - const collapseBtn = $('sidebar-collapse-btn'); - if (collapseBtn) { - collapseBtn.textContent = isOpen ? '\u2039' : '\u203a'; - } + if (_serverSettings) _serverSettings.sidebarOpen = isOpen; + patchServerSetting('sidebarOpen', isOpen); + + var collapseBtn = $('sidebar-collapse-btn'); + if (collapseBtn) collapseBtn.textContent = isOpen ? '\u2039' : '\u203a'; } /** @@ -679,17 +660,16 @@ function toggleSidebar() { * - the sidebar is already collapsed */ function bindSidebarClickAway() { - const container = $('terminal-container'); + var container = $('terminal-container'); if (!container) return; - container.addEventListener('click', () => { + container.addEventListener('click', function() { if (window.innerWidth >= SIDEBAR_NARROW_THRESHOLD) return; - const sidebar = $('session-sidebar'); + var sidebar = $('session-sidebar'); if (!sidebar) return; if (sidebar.classList.contains('sidebar--collapsed')) return; sidebar.classList.add('sidebar--collapsed'); - try { - localStorage.setItem(SIDEBAR_KEY, JSON.stringify(false)); - } catch (_) { /* blocked — ok */ } + if (_serverSettings) _serverSettings.sidebarOpen = false; + patchServerSetting('sidebarOpen', false); }); } diff --git a/muxplex/tests/test_frontend_js.py b/muxplex/tests/test_frontend_js.py index ffd09eb..551922d 100644 --- a/muxplex/tests/test_frontend_js.py +++ b/muxplex/tests/test_frontend_js.py @@ -344,7 +344,9 @@ def test_get_display_settings_reads_server_settings() -> None: ) assert match, "getDisplaySettings function not found" body = match.group(1) - assert "_serverSettings" in body, "getDisplaySettings must read from _serverSettings" + assert "_serverSettings" in body, ( + "getDisplaySettings must read from _serverSettings" + ) assert "localStorage" not in body, ( "getDisplaySettings must not use localStorage — display settings are server-side" ) @@ -429,7 +431,9 @@ def test_get_display_settings_reads_from_server_settings() -> None: ) assert match, "getDisplaySettings function not found" body = match.group(1) - assert "_serverSettings" in body, "getDisplaySettings must read from _serverSettings" + assert "_serverSettings" in body, ( + "getDisplaySettings must read from _serverSettings" + ) assert "localStorage" not in body, ( "getDisplaySettings must not use localStorage — display settings are server-side" ) @@ -492,9 +496,7 @@ def test_on_display_setting_change_catches_errors() -> None: ) assert match, "onDisplaySettingChange function not found" body = match.group(1) - assert ".catch" in body, ( - "onDisplaySettingChange must handle errors via .catch()" - ) + assert ".catch" in body, "onDisplaySettingChange must handle errors via .catch()" # ── openSettings implementation ─────────────────────────────────────────────── @@ -2597,3 +2599,107 @@ def test_open_session_bell_clear_is_fire_and_forget() -> None: assert "await" not in line, ( f"openSession bell-clear POST must NOT be awaited (fire-and-forget): {line.strip()}" ) + + +# ─── Task 4: sidebar functions use server-side settings ────────────────────── + + +def test_no_sidebar_key_constant() -> None: + """SIDEBAR_KEY constant must be removed — sidebar state moves to _serverSettings.""" + assert "SIDEBAR_KEY" not in _JS, ( + "SIDEBAR_KEY constant must be removed from app.js; " + "sidebar open/closed state is now stored in _serverSettings.sidebarOpen" + ) + + +def test_init_sidebar_reads_server_settings() -> None: + """initSidebar must read sidebarOpen from _serverSettings, not localStorage.""" + match = re.search( + r"function initSidebar\s*\(\s*\)\s*\{(.*?)(?=\nfunction |\n// ─|\n/\*\*)", + _JS, + re.DOTALL, + ) + assert match, "initSidebar function not found" + body = match.group(1) + assert "_serverSettings" in body, ( + "initSidebar must read sidebarOpen from _serverSettings" + ) + assert "localStorage" not in body, ( + "initSidebar must not use localStorage — sidebar state is now server-side" + ) + + +def test_init_sidebar_calls_patch_server_setting() -> None: + """initSidebar must call patchServerSetting to persist the auto-detected state.""" + match = re.search( + r"function initSidebar\s*\(\s*\)\s*\{(.*?)(?=\nfunction |\n// ─|\n/\*\*)", + _JS, + re.DOTALL, + ) + assert match, "initSidebar function not found" + body = match.group(1) + assert "patchServerSetting" in body, ( + "initSidebar must call patchServerSetting to persist the auto-detected sidebar state" + ) + + +def test_toggle_sidebar_reads_server_settings() -> None: + """toggleSidebar must derive state from the DOM class, not from localStorage.""" + match = re.search( + r"function toggleSidebar\s*\(\s*\)\s*\{(.*?)(?=\nfunction |\n// ─|\n/\*\*)", + _JS, + re.DOTALL, + ) + assert match, "toggleSidebar function not found" + body = match.group(1) + assert "_serverSettings" in body, ( + "toggleSidebar must write new state to _serverSettings" + ) + assert "localStorage" not in body, ( + "toggleSidebar must not use localStorage — sidebar state is now server-side" + ) + + +def test_toggle_sidebar_calls_patch_server_setting() -> None: + """toggleSidebar must call patchServerSetting to persist the toggled state.""" + match = re.search( + r"function toggleSidebar\s*\(\s*\)\s*\{(.*?)(?=\nfunction |\n// ─|\n/\*\*)", + _JS, + re.DOTALL, + ) + assert match, "toggleSidebar function not found" + body = match.group(1) + assert "patchServerSetting" in body, ( + "toggleSidebar must call patchServerSetting to persist the toggled sidebar state" + ) + + +def test_bind_sidebar_click_away_uses_server_settings() -> None: + """bindSidebarClickAway must write false to _serverSettings on collapse.""" + match = re.search( + r"function bindSidebarClickAway\s*\(\s*\)\s*\{(.*?)(?=\nfunction |\n// ─|\n/\*\*)", + _JS, + re.DOTALL, + ) + assert match, "bindSidebarClickAway function not found" + body = match.group(1) + assert "_serverSettings" in body, ( + "bindSidebarClickAway must write false to _serverSettings.sidebarOpen on collapse" + ) + assert "localStorage" not in body, ( + "bindSidebarClickAway must not use localStorage — sidebar state is now server-side" + ) + + +def test_bind_sidebar_click_away_calls_patch_server_setting() -> None: + """bindSidebarClickAway must call patchServerSetting to persist collapsed state.""" + match = re.search( + r"function bindSidebarClickAway\s*\(\s*\)\s*\{(.*?)(?=\nfunction |\n// ─|\n/\*\*)", + _JS, + re.DOTALL, + ) + assert match, "bindSidebarClickAway function not found" + body = match.group(1) + assert "patchServerSetting" in body, ( + "bindSidebarClickAway must call patchServerSetting to persist collapsed state" + )