From a5abe51cc5475ee44c604012a4ec4a3e13f9ee71 Mon Sep 17 00:00:00 2001 From: nesquena-hermes Date: Mon, 13 Apr 2026 23:25:26 -0700 Subject: [PATCH] =?UTF-8?q?fix:=20workspace=20panel=20close=20button=20?= =?UTF-8?q?=E2=80=94=20no=20duplicate=20X=20on=20desktop,=20mobile=20X=20r?= =?UTF-8?q?espects=20file=20preview=20(#414)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix: workspace panel close button — no duplicate X on desktop, mobile X respects file preview Two bugs fixed in the workspace right panel: 1. Duplicate X on desktop (bug): #btnClearPreview (the X icon) was always visible alongside #btnCollapseWorkspacePanel (the chevron), producing two close controls at once. Fixed in syncWorkspacePanelUI() — on desktop, the X is now hidden when no file preview is open (display:none), and only shown when the user is viewing a file. The chevron remains as the sole close control in browse mode. 2. Mobile X collapses panel instead of dismissing file (bug): .mobile-close-btn was calling closeWorkspacePanel() directly, which collapsed the whole panel even when a file was open. Changed to handleWorkspaceClose(), which already has the correct two-step logic: clear preview first, close panel only if no preview is visible. Files changed: - static/boot.js: syncWorkspacePanelUI() hides btnClearPreview on desktop when hasPreview is false, guarded by !isCompact so mobile is unaffected - static/index.html: mobile-close-btn onclick changed from closeWorkspacePanel() to handleWorkspaceClose() - tests/test_sprint44.py: 10 new regression tests - tests/test_mobile_layout.py: updated test_workspace_close_button_present() to accept handleWorkspaceClose() as the valid onclick target * fix: widen test_server_delete_invalidates_index window to 1200 chars The test extracted a 600-char window starting from the session/delete handler to check for SESSION_INDEX_FILE. Commit 3cc5839 added session_id character validation and path traversal guards before the unlink call, pushing SESSION_INDEX_FILE to ~764 chars from the match — beyond the 600-char limit, causing the test to fail on CI. Widened the window to 1200 chars, which accommodates any reasonable amount of guard code before the SESSION_INDEX_FILE.unlink() call. * docs: v0.50.33 release — version badge and CHANGELOG --------- Co-authored-by: Nathan Esquenazi --- CHANGELOG.md | 15 ++++ static/boot.js | 4 ++ static/index.html | 4 +- tests/test_mobile_layout.py | 7 +- tests/test_regressions.py | 5 +- tests/test_sprint44.py | 134 ++++++++++++++++++++++++++++++++++++ 6 files changed, 164 insertions(+), 5 deletions(-) create mode 100644 tests/test_sprint44.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 1ab9cad..f5c410c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,20 @@ # Hermes Web UI -- Changelog +## [v0.50.33] fix: workspace panel close button — no duplicate X on desktop, mobile X respects file preview (#413) + +**Bug 1 — Duplicate X on desktop:** `#btnClearPreview` (the X icon) was always visible regardless of panel state, so desktop browse mode showed both the chevron collapse button and the X simultaneously. Fixed in `syncWorkspacePanelUI()`: on non-compact (desktop) viewports, `clearBtn.style.display` is set to `none` when no file preview is open, and cleared (shown) when a preview is active. + +**Bug 2 — Mobile X collapsed the whole panel instead of dismissing the file:** `.mobile-close-btn` was wired to `closeWorkspacePanel()` directly, bypassing the two-step close logic. Fixed by changing `onclick` to `handleWorkspaceClose()`, which calls `clearPreview()` first if a file is open, and falls through to `closeWorkspacePanel()` otherwise. + +**Also:** widened the `test_server_delete_invalidates_index` window from 600 → 1200 chars to accommodate the session_id validation guards added in v0.50.32 (#412). + +- `static/boot.js`: `syncWorkspacePanelUI()` sets `clearBtn.style.display` based on `hasPreview` when `!isCompact` +- `static/index.html`: `.mobile-close-btn` onclick changed from `closeWorkspacePanel()` to `handleWorkspaceClose()` +- `tests/test_sprint44.py`: 10 new regression tests covering both fixes +- `tests/test_mobile_layout.py`: updated to accept `handleWorkspaceClose()` as valid onclick +- `tests/test_regressions.py`: widened delete handler window to 1200 chars +- 1051 tests total (up from 1041) + ## [v0.50.32] fix(sessions): validate session_id before deleting session files [SECURITY] (#409) `/api/session/delete` accepted arbitrary `session_id` values from the request body and built the delete path directly as `SESSION_DIR / f"{sid}.json"`. Because pathlib discards the prefix when `sid` is an absolute path, an attacker could supply `/tmp/victim` and cause the server to unlink `victim.json` outside the session store. Traversal-style values (`../../etc/target`) were also accepted. CVSS 8.1 High (AV:N/AC:L/PR:L/UI:N/S:U/C:N/I:H/A:H). diff --git a/static/boot.js b/static/boot.js index 67869dd..e07bd13 100644 --- a/static/boot.js +++ b/static/boot.js @@ -118,6 +118,10 @@ function syncWorkspacePanelUI(){ if(clearBtn){ clearBtn.disabled=!isOpen; clearBtn.title=hasPreview?'Close preview':'Hide workspace panel'; + // On desktop, only show the X button when a file preview is open. + // In browse mode the chevron (btnCollapseWorkspacePanel) already serves + // as the close control, so showing both produces a duplicate X. + if(!isCompact) clearBtn.style.display=hasPreview?'':'none'; } } diff --git a/static/index.html b/static/index.html index b69fa9d..e1cd6a0 100644 --- a/static/index.html +++ b/static/index.html @@ -358,7 +358,7 @@ - + @@ -535,7 +535,7 @@
System
Instance version and access controls.
- v0.50.32 + v0.50.33
diff --git a/tests/test_mobile_layout.py b/tests/test_mobile_layout.py index fba9288..5ea7fb4 100644 --- a/tests/test_mobile_layout.py +++ b/tests/test_mobile_layout.py @@ -115,13 +115,16 @@ def test_topbar_chips_mobile_overflow(): def test_workspace_close_button_present(): """Workspace panel must have a close/hide button accessible on mobile.""" - # Either a dedicated mobile close button or the toggle button that closes the panel + # Accept handleWorkspaceClose() (two-step close: file→browse→closed), or the + # lower-level functions directly. handleWorkspaceClose is preferred because + # it dismisses a file preview first before closing the panel. has_close = ( + 'onclick="handleWorkspaceClose()"' in HTML or 'onclick="closeWorkspacePanel()"' in HTML or 'onclick="toggleWorkspacePanel()"' in HTML ) assert has_close, \ - "closeWorkspacePanel() or toggleWorkspacePanel() must be wired to a button to close the workspace panel on mobile" + "handleWorkspaceClose() or closeWorkspacePanel() must be wired to a button to close the workspace panel on mobile" def test_toggle_mobile_files_js_defined(): diff --git a/tests/test_regressions.py b/tests/test_regressions.py index a9a0186..ce06ba1 100644 --- a/tests/test_regressions.py +++ b/tests/test_regressions.py @@ -310,7 +310,10 @@ def test_server_delete_invalidates_index(cleanup_test_sessions): text.find('if parsed.path == "/api/session/delete":'), ) if delete_idx >= 0: - delete_block = text[delete_idx:delete_idx+600] + # Use 1200 chars to accommodate any validation/guard code added + # before the SESSION_INDEX_FILE.unlink() call (e.g. session_id + # character checks, path traversal guards). + delete_block = text[delete_idx:delete_idx+1200] assert "SESSION_INDEX_FILE" in delete_block, \ f"{label} session/delete must invalidate SESSION_INDEX_FILE" return diff --git a/tests/test_sprint44.py b/tests/test_sprint44.py new file mode 100644 index 0000000..9dcb09f --- /dev/null +++ b/tests/test_sprint44.py @@ -0,0 +1,134 @@ +""" +Sprint 44 Tests: Workspace panel close button fixes (PR #413). + +Covers: +- index.html: mobile-close-btn now calls handleWorkspaceClose() instead of + closeWorkspacePanel(), so hitting X while a file is open returns you to the + file browser rather than collapsing the whole panel. +- boot.js: syncWorkspacePanelUI() hides #btnClearPreview (the X icon) on + desktop when no file preview is open, eliminating the duplicate X that + appeared alongside the chevron collapse button. +- boot.js: handleWorkspaceClose() logic — clears preview when one is visible, + closes panel otherwise (existing function, confirmed wired to both buttons). +""" +import pathlib +import re +import unittest + +REPO = pathlib.Path(__file__).parent.parent +HTML = (REPO / "static" / "index.html").read_text(encoding="utf-8") +BOOT_JS = (REPO / "static" / "boot.js").read_text(encoding="utf-8") + + +class TestMobileCloseButtonBehavior(unittest.TestCase): + """mobile-close-btn must call handleWorkspaceClose(), not closeWorkspacePanel().""" + + def test_mobile_close_btn_calls_handle_workspace_close(self): + """mobile-close-btn onclick must be handleWorkspaceClose(), not closeWorkspacePanel().""" + m = re.search(r'class="[^"]*mobile-close-btn[^"]*"[^>]*>', HTML) + self.assertIsNotNone(m, "mobile-close-btn element not found in index.html") + btn_html = m.group(0) + self.assertIn( + 'onclick="handleWorkspaceClose()"', + btn_html, + "mobile-close-btn must call handleWorkspaceClose() so that hitting X " + "while a file is open closes the file first, not the whole panel", + ) + + def test_mobile_close_btn_does_not_call_close_workspace_panel_directly(self): + """mobile-close-btn must NOT call closeWorkspacePanel() directly.""" + m = re.search(r'class="[^"]*mobile-close-btn[^"]*"[^>]*>', HTML) + self.assertIsNotNone(m, "mobile-close-btn element not found in index.html") + btn_html = m.group(0) + self.assertNotIn( + 'onclick="closeWorkspacePanel()"', + btn_html, + "mobile-close-btn must not call closeWorkspacePanel() directly — " + "it would bypass the two-step close logic and collapse the panel even " + "when a file is being viewed", + ) + + def test_handle_workspace_close_defined_in_boot_js(self): + """handleWorkspaceClose() must be defined in boot.js.""" + self.assertIn( + "function handleWorkspaceClose()", + BOOT_JS, + "handleWorkspaceClose() is missing from boot.js", + ) + + def test_handle_workspace_close_clears_preview_first(self): + """handleWorkspaceClose() must call clearPreview() when a preview is visible.""" + # The function must check for visible preview and call clearPreview + self.assertIn( + "clearPreview()", + BOOT_JS, + "handleWorkspaceClose() must call clearPreview() when preview is visible", + ) + def test_handle_workspace_close_falls_back_to_close_panel(self): + """handleWorkspaceClose() must call closeWorkspacePanel() as fallback.""" + # Find the function start and extract until the closing brace by scanning + start = BOOT_JS.find("function handleWorkspaceClose()") + self.assertNotEqual(start, -1, "handleWorkspaceClose() not found in boot.js") + # Extract a generous window after the function start + fn_window = BOOT_JS[start : start + 400] + self.assertIn( + "closeWorkspacePanel()", + fn_window, + "handleWorkspaceClose() must call closeWorkspacePanel() as its fallback path", + ) + + +class TestDesktopNoDuplicateXButton(unittest.TestCase): + """On desktop, only one X/close control should appear at a time.""" + + def test_sync_workspace_panel_ui_hides_clear_preview_on_desktop(self): + """syncWorkspacePanelUI() must set display:none on btnClearPreview when no preview and desktop.""" + self.assertIn( + "clearBtn.style.display", + BOOT_JS, + "syncWorkspacePanelUI() must control clearBtn.style.display to hide it " + "on desktop when no file preview is open", + ) + + def test_clear_preview_hidden_when_no_preview(self): + """The display toggle for btnClearPreview must key off hasPreview.""" + # Expect something like: clearBtn.style.display=hasPreview?'':'none' + # or clearBtn.style.display = hasPreview ? '' : 'none' + pattern = r"clearBtn\.style\.display\s*=\s*hasPreview" + self.assertRegex( + BOOT_JS, + pattern, + "btnClearPreview display must be conditioned on hasPreview in " + "syncWorkspacePanelUI() to avoid a duplicate X on desktop", + ) + + def test_clear_preview_toggle_only_applied_on_desktop(self): + """The display toggle must be guarded by !isCompact so mobile is unaffected.""" + # Expect: if(!isCompact) clearBtn.style.display=... + pattern = r"isCompact.*clearBtn\.style\.display|clearBtn\.style\.display.*isCompact" + self.assertRegex( + BOOT_JS, + pattern, + "btnClearPreview display toggle must be guarded by isCompact so the " + "mobile X button visibility is not accidentally affected", + ) + + def test_btnclearpreview_exists_in_html(self): + """#btnClearPreview must still exist in the HTML (not removed).""" + self.assertIn( + 'id="btnClearPreview"', + HTML, + "#btnClearPreview must remain in index.html", + ) + + def test_btncollapseWorkspacepanel_exists_in_html(self): + """#btnCollapseWorkspacePanel (chevron) must still exist in the HTML.""" + self.assertIn( + 'id="btnCollapseWorkspacePanel"', + HTML, + "#btnCollapseWorkspacePanel must remain in index.html", + ) + + +if __name__ == "__main__": + unittest.main()