gabogg/hikcentral
Watch
1
0
Fork
You've already forked hikcentral
 0
Code Issues 113 Pull requests 8 Projects Releases Packages Wiki Activity Actions

refactor(ui): fold diagnostic decks into one DIAGNOSTICS menu #173

Merged
gabogg merged 3 commits from refactor/admin-diagnostics-menu into master 2026-09-28 14:11:30 +00:00
Conversation 3 Commits 3 Files changed 5 +540 -11
gabogg commented 2026-09-28 09:13:00 +00:00
Owner
Copy link

Summary

The admin deck selector had seven entries. SYSTEM_DIAG, API_CONSOLE and EVENT_LOGS are now one [ F4: DIAGNOSTICS ▾ ] entry that opens a menu. This clears up the main tab strip and makes room for the upcoming historical-flow import menu (#170).

Before: [F1 DUAL_OPS_DECK] [F2 VIDEO_SURVEILLANCE] [F3 CONFIGURATION] [F4 SYSTEM_DIAG] [F5 STATISTICS] [F6 API_CONSOLE] [F7 EVENT_LOGS]
After:  [F1 DUAL_OPS_DECK] [F2 VIDEO_SURVEILLANCE] [F3 CONFIGURATION] [F4 DIAGNOSTICS ▾] [F5 STATISTICS]
                                                                       ├ [F4: SYSTEM_DIAG]
                                                                       ├ [F6: API_CONSOLE]
                                                                       └ [F7: EVENT_LOGS]
  • Active deck on the button: while a grouped deck is open, the button is highlighted and names it, e.g. DIAGNOSTICS: API_CONSOLE.
  • Shortcuts: F4, F6 and F7 still jump straight to each deck, and role-based access is unchanged.
  • Keyboard support:
    • Arrow Down/Up opens the menu from the button, and arrows, Home and End move between items.
    • Escape closes the menu and returns focus to the button; Tab or a click outside also closes it.
    • Items use menuitemradio with aria-checked.
  • Narrow screens: deck buttons no longer wrap onto two lines; the strip scrolls horizontally instead (the tab strip already had overflow-x: auto).

Architectural impact

  • New app/static/js/src/ui/deck_menu.js (DeckMenu, DIAGNOSTICS_DECKS), loaded by the module block in index.html. app.js creates it and syncs the active state in switchTab.
  • The menu element sits outside the scrolling selector and uses position: fixed under its button, clamped to the viewport, so the strip's overflow cannot clip it. It follows the button on scroll and resize.
  • No backend changes. The deck IDs (probes, console, logs) and their content panels are unchanged.

Redesign alternatives considered

  1. Dropdown menu (this PR): a small change that keeps the tab-strip model and the F-keys.
  2. Tools/sub-tab bar: one DIAGNOSTICS deck with its own inner strip (SYSTEM_DIAG | API_CONSOLE | EVENT_LOGS). One click is saved when moving between them, but the three panels would become one deck.
  3. Overflow "⋯" menu: move every admin-only secondary deck behind an overflow button at the strip's right end. This would also absorb the future import deck, but it hides more.

Say if you prefer 2 or 3. The DeckMenu module can back either.

Verification

  • New node tests tests/frontend/test_deck_menu.test.js (8 tests: grouping, keyboard navigation, Escape/outside close, positioning and viewport clamping, active-deck sync, focus on reopen).
  • Full pytest: 417 passed, including the node frontend suite. Ruff clean. Pre-commit passed.
  • Checked in a real browser (Playwright/Chromium, local server on a scratch DB, HikCentral blackholed):
    • At 1280 px, the menu opens under the button, choosing API_CONSOLE switches deck and closes the menu, F7 opens EVENT_LOGS, and F1 clears the highlight.
    • At 700 px, the strip scrolls horizontally without wrapping, and the menu follows the scrolled button.
  • Pre-existing bug seen on master, not changed here: opening EVENT_LOGS throws loadEventLogs is not defined. switchTab calls a function that no longer exists. Tracked in #174.

Checklist

  • Fold SYSTEM_DIAG, API_CONSOLE and EVENT_LOGS into one menu.
  • Keep F-key shortcuts and RBAC.
  • Scrollable, non-wrapping selector.
  • Review passes per docs/standards/git-and-workflow.md.

🤖 Generated with Claude Code

## Summary The admin deck selector had seven entries. SYSTEM_DIAG, API_CONSOLE and EVENT_LOGS are now one **`[ F4: DIAGNOSTICS ▾ ]`** entry that opens a menu. This clears up the main tab strip and makes room for the upcoming historical-flow import menu (#170). ``` Before: [F1 DUAL_OPS_DECK] [F2 VIDEO_SURVEILLANCE] [F3 CONFIGURATION] [F4 SYSTEM_DIAG] [F5 STATISTICS] [F6 API_CONSOLE] [F7 EVENT_LOGS] After: [F1 DUAL_OPS_DECK] [F2 VIDEO_SURVEILLANCE] [F3 CONFIGURATION] [F4 DIAGNOSTICS ▾] [F5 STATISTICS] ├ [F4: SYSTEM_DIAG] ├ [F6: API_CONSOLE] └ [F7: EVENT_LOGS] ``` - **Active deck on the button:** while a grouped deck is open, the button is highlighted and names it, e.g. `DIAGNOSTICS: API_CONSOLE`. - **Shortcuts:** F4, F6 and F7 still jump straight to each deck, and role-based access is unchanged. - **Keyboard support:** - Arrow Down/Up opens the menu from the button, and arrows, Home and End move between items. - Escape closes the menu and returns focus to the button; Tab or a click outside also closes it. - Items use `menuitemradio` with `aria-checked`. - **Narrow screens:** deck buttons no longer wrap onto two lines; the strip scrolls horizontally instead (the tab strip already had `overflow-x: auto`). ## Architectural impact - New `app/static/js/src/ui/deck_menu.js` (`DeckMenu`, `DIAGNOSTICS_DECKS`), loaded by the module block in `index.html`. `app.js` creates it and syncs the active state in `switchTab`. - The menu element sits **outside** the scrolling selector and uses `position: fixed` under its button, clamped to the viewport, so the strip's overflow cannot clip it. It follows the button on scroll and resize. - No backend changes. The deck IDs (`probes`, `console`, `logs`) and their content panels are unchanged. ## Redesign alternatives considered 1. **Dropdown menu (this PR):** a small change that keeps the tab-strip model and the F-keys. 2. **Tools/sub-tab bar:** one DIAGNOSTICS deck with its own inner strip (SYSTEM_DIAG | API_CONSOLE | EVENT_LOGS). One click is saved when moving between them, but the three panels would become one deck. 3. **Overflow "⋯" menu:** move every admin-only secondary deck behind an overflow button at the strip's right end. This would also absorb the future import deck, but it hides more. Say if you prefer 2 or 3. The `DeckMenu` module can back either. ## Verification - [x] New node tests `tests/frontend/test_deck_menu.test.js` (8 tests: grouping, keyboard navigation, Escape/outside close, positioning and viewport clamping, active-deck sync, focus on reopen). - [x] Full `pytest`: 417 passed, including the node frontend suite. Ruff clean. Pre-commit passed. - [x] Checked in a real browser (Playwright/Chromium, local server on a scratch DB, HikCentral blackholed): - At 1280 px, the menu opens under the button, choosing API_CONSOLE switches deck and closes the menu, F7 opens EVENT_LOGS, and F1 clears the highlight. - At 700 px, the strip scrolls horizontally without wrapping, and the menu follows the scrolled button. - **Pre-existing bug seen on master, not changed here:** opening EVENT_LOGS throws `loadEventLogs is not defined`. `switchTab` calls a function that no longer exists. Tracked in #174. ## Checklist - [x] Fold SYSTEM_DIAG, API_CONSOLE and EVENT_LOGS into one menu. - [x] Keep F-key shortcuts and RBAC. - [x] Scrollable, non-wrapping selector. - [ ] Review passes per `docs/standards/git-and-workflow.md`. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
refactor(ui): fold diagnostic decks into one DIAGNOSTICS menu
All checks were successful
CI / lint-and-test (pull_request) Successful in 2m9s
Details
3ce41dff63 
The admin deck selector carried seven entries. SYSTEM_DIAG, API_CONSOLE and
EVENT_LOGS now share a single "[ F4: DIAGNOSTICS ▾ ]" entry that opens a
menu, making room for the historical-flow import deck (#170) and reducing
clutter in the main tab strip.

- New DeckMenu module (src/ui/deck_menu.js): menu rendered outside the
  horizontally scrolling selector with fixed positioning clamped to the
  viewport, so the strip's overflow never clips it; follows the button on
  scroll/resize. Keyboard support: ArrowUp/Down/Home/End, Escape returns
  focus, Tab or an outside click closes. Items are menuitemradio with
  aria-checked; the button shows the active deck (e.g. DIAGNOSTICS:
  API_CONSOLE) and is highlighted while any grouped deck is active.
- F4/F6/F7 shortcuts still switch directly to each deck; RBAC unchanged.
- Deck selector buttons no longer wrap on narrow screens: the strip scrolls
  horizontally instead.
- Node tests for the menu (grouping, keyboard, positioning, active sync).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
gabogg changed title from WIP: refactor(ui): fold diagnostic decks into one DIAGNOSTICS menu to refactor(ui): fold diagnostic decks into one DIAGNOSTICS menu 2026-09-28 09:13:18 +00:00
gabogg commented 2026-09-28 10:32:16 +00:00
Author
Owner
Copy link

Review pass 1 — 3ce41df (DIAGNOSTICS menu)

Reviewed master...3ce41df against the PR's stated behavior, AGENTS.md, the code standards, the UI design guidelines, and the Fowler smell baseline.

Standards

No actionable findings. The menu follows the documented frontend design rules and has focused tests.

Spec

  • P2 — The menu can overflow a short viewport vertically. DeckMenu._position() (app/static/js/src/ui/deck_menu.js, around lines 159–167) clamps left to innerWidth but always places top below the button. After a resize or scroll in a short viewport, the menu can extend below the visible area. The PR says the menu is “clamped to the viewport” and “follows the button on scroll and resize.” Clamp its vertical position or available height as well, and test that case.
  • P2 — Function keys leave an open menu over the selected deck. Pressing F4, F6, or F7 while the menu is open switches decks through the global shortcut handler, but syncActive() closes the menu only when the new deck is outside the diagnostics group (app/static/js/src/ui/deck_menu.js, around line 136). The PR says these shortcuts “still jump straight to each deck.” Close the menu on a direct shortcut so the selected deck is visible, and cover that interaction in a test.

Totals: Standards 0 findings; Spec 2 findings (both P2). First-pass P1/P2/P3 findings require fixes before requesting pass 2.

## Review pass 1 — 3ce41df (DIAGNOSTICS menu) Reviewed `master...3ce41df` against the PR's stated behavior, AGENTS.md, the code standards, the UI design guidelines, and the Fowler smell baseline. ## Standards No actionable findings. The menu follows the documented frontend design rules and has focused tests. ## Spec - **P2 — The menu can overflow a short viewport vertically.** `DeckMenu._position()` (`app/static/js/src/ui/deck_menu.js`, around lines 159–167) clamps `left` to `innerWidth` but always places `top` below the button. After a resize or scroll in a short viewport, the menu can extend below the visible area. The PR says the menu is “clamped to the viewport” and “follows the button on scroll and resize.” Clamp its vertical position or available height as well, and test that case. - **P2 — Function keys leave an open menu over the selected deck.** Pressing F4, F6, or F7 while the menu is open switches decks through the global shortcut handler, but `syncActive()` closes the menu only when the new deck is outside the diagnostics group (`app/static/js/src/ui/deck_menu.js`, around line 136). The PR says these shortcuts “still jump straight to each deck.” Close the menu on a direct shortcut so the selected deck is visible, and cover that interaction in a test. **Totals:** Standards 0 findings; Spec 2 findings (both P2). First-pass P1/P2/P3 findings require fixes before requesting pass 2.
fix(ui): keep the DIAGNOSTICS menu on screen and close it on F-keys
All checks were successful
CI / lint-and-test (pull_request) Successful in 2m20s
Details
ce9204907e 
Review pass 1 on #173:
- P2: the menu could extend below a short viewport. New menuPlacement()
  opens it below the button, or above when that side has more room and
  it does not fit below, and caps max-height to the visible space on the
  chosen side so the menu scrolls instead of overflowing.
- P2: pressing F4/F6/F7 with the menu open switched decks but left the
  menu over the selected deck. syncActive(), called on every deck switch,
  now always closes the menu.
- Tests for below/above/scroll placement, the short-viewport flip and the
  F-key close.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
gabogg commented 2026-09-28 11:08:16 +00:00
Author
Owner
Copy link

Review pass 1: resolution in ce92049

Both findings are fixed. Current master, including #166 and #179, is merged into the branch (b96a6a5).

  • P2: vertical overflow in a short viewport.
    • New pure menuPlacement() opens the menu below the button, or above when that side has more room and the menu does not fit below.
    • It caps max-height to the visible space on the chosen side, so the menu scrolls instead of overflowing.
    • _position() uses it on open, scroll and resize.
  • P2: F-keys left the menu open over the selected deck. syncActive() runs on every deck switch (menu choice, F4/F6/F7 or another tab) and now always closes the menu.

Tests: 11 menu tests (was 8), adding below/above/scroll placement, the short-viewport flip and the F-key close.

Checked in Chromium:

  • In a 1000×320 window, the open menu stays inside the viewport.
  • Pressing F6 with the menu open hides it and shows API_CONSOLE.

Suite: full suite 450 passed; pre-commit passed. It reported 2 warnings from an intermittent aiosqlite teardown thread in test_counting_integrity.py; they are unrelated to this frontend-only change.

Ready for review pass 2.

## Review pass 1: resolution in `ce92049` Both findings are fixed. Current `master`, including #166 and #179, is merged into the branch (`b96a6a5`). - **P2: vertical overflow in a short viewport.** - New pure `menuPlacement()` opens the menu below the button, or above when that side has more room and the menu does not fit below. - It caps `max-height` to the visible space on the chosen side, so the menu scrolls instead of overflowing. - `_position()` uses it on open, scroll and resize. - **P2: F-keys left the menu open over the selected deck.** `syncActive()` runs on every deck switch (menu choice, F4/F6/F7 or another tab) and now always closes the menu. **Tests:** 11 menu tests (was 8), adding below/above/scroll placement, the short-viewport flip and the F-key close. **Checked in Chromium:** - In a 1000×320 window, the open menu stays inside the viewport. - Pressing F6 with the menu open hides it and shows API_CONSOLE. **Suite:** full suite 450 passed; pre-commit passed. It reported 2 warnings from an intermittent aiosqlite teardown thread in `test_counting_integrity.py`; they are unrelated to this frontend-only change. Ready for review pass 2.
gabogg commented 2026-09-28 14:08:49 +00:00
Author
Owner
Copy link

Review pass 2 — ce92049

Reviewed master...ce92049 on two axes: Standards (AGENTS.md, code standards, UI design guidelines, Fowler smell baseline) and Spec (the PR description plus the pass-1 resolution). Frontend tests: 242/242, including 11/11 for test_deck_menu. CI lint-and-test passed on ce92049.

Neither axis has P1 or P2 findings, and both pass-1 fixes are verified:

  • menuPlacement flips the menu above the button and caps max-height.
  • syncActive always closes the menu. Focus still returns to the button after a menu choice.

RBAC is unchanged. The button keeps admin-only, and menu items still go through the ROLE_ALLOWED_DECKS check in switchTab.

Standards — 5 × P3

  1. Duplicated code: DIAGNOSTICS_DECKS (deck_menu.js:10-14) repeats the F-key-to-deck pairs from KEY_DECK_MAP (app.js:44-53). Filed as #183.
  2. .deck-menu.hidden (css:1386) repeats the global .hidden rule. Filed as #183.
  3. §3.3 asks for <kbd>, but shortcuts are plain text. No action, since this matches the existing deck buttons.
  4. §5.4 asks for a 1px pressed offset on .deck-menu-item. No action, since this matches the existing deck buttons.
  5. The button label names only F4. No action, since the tooltip and the active-deck suffix cover it.

Spec — 3 × P3 (all filed as #182)

  1. Once the button scrolls out of the strip, the menu stays pinned to the viewport edge. The left and right margins also differ (0 vs 8px).
  2. An F-key pressed while a menu item has focus drops focus to <body>.
  3. The document and window listeners are never removed, and there is no destroy().

Totals: Standards 5 × P3 (worst: duplicated shortcut map). Spec 3 × P3 (worst: the menu detaching from its button). The deferred P3s are filed as #182 and #183, both in the milestone Historical passenger-flow backfill with ready-for-agent. Merging.

## Review pass 2 — ce92049 Reviewed master...ce92049 on two axes: Standards (AGENTS.md, code standards, UI design guidelines, Fowler smell baseline) and Spec (the PR description plus the pass-1 resolution). Frontend tests: 242/242, including 11/11 for `test_deck_menu`. CI `lint-and-test` passed on ce92049. Neither axis has P1 or P2 findings, and both pass-1 fixes are verified: - `menuPlacement` flips the menu above the button and caps `max-height`. - `syncActive` always closes the menu. Focus still returns to the button after a menu choice. RBAC is unchanged. The button keeps `admin-only`, and menu items still go through the `ROLE_ALLOWED_DECKS` check in `switchTab`. ## Standards — 5 × P3 1. Duplicated code: `DIAGNOSTICS_DECKS` (deck_menu.js:10-14) repeats the F-key-to-deck pairs from `KEY_DECK_MAP` (app.js:44-53). Filed as **#183**. 2. `.deck-menu.hidden` (css:1386) repeats the global `.hidden` rule. Filed as **#183**. 3. §3.3 asks for `<kbd>`, but shortcuts are plain text. No action, since this matches the existing deck buttons. 4. §5.4 asks for a 1px pressed offset on `.deck-menu-item`. No action, since this matches the existing deck buttons. 5. The button label names only F4. No action, since the tooltip and the active-deck suffix cover it. ## Spec — 3 × P3 (all filed as **#182**) 1. Once the button scrolls out of the strip, the menu stays pinned to the viewport edge. The left and right margins also differ (0 vs 8px). 2. An F-key pressed while a menu item has focus drops focus to `<body>`. 3. The document and window listeners are never removed, and there is no `destroy()`. **Totals:** Standards 5 × P3 (worst: duplicated shortcut map). Spec 3 × P3 (worst: the menu detaching from its button). The deferred P3s are filed as #182 and #183, both in the milestone *Historical passenger-flow backfill* with `ready-for-agent`. Merging.
gabogg merged commit fb89880df1 into master 2026-09-28 14:11:30 +00:00
gabogg deleted branch refactor/admin-diagnostics-menu 2026-09-28 14:11:30 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
blocked
Waits on the issues listed in its "Blocked by:" line before implementation

  
bug
Category: something is broken; behaviour differs from the spec or docs

  
enhancement
Category: new feature or improvement, including refactors, chores and docs

  
high-priority
Address before ordinary-priority work; independent of triage readiness and blockers

  
low-priority
Address after ordinary-priority work; independent of triage readiness and blockers

  
needs-info
Waiting on reporter for more information

  
needs-triage
Maintainer needs to evaluate this issue

  
ready-for-agent
Fully specified, ready for an AFK agent

  
ready-for-human
Requires human implementation

  
referenced
Previously closed issue reopened or referenced by an open issue; visible history marker

  
research
Needs investigation (internet or local resources) before it can be triaged down; used alongside a category and a triage role

  
wontfix
Will not be actioned

No labels
blocked
bug
enhancement
high-priority
low-priority
needs-info
needs-triage
ready-for-agent
ready-for-human
referenced
research
wontfix
Milestone
Clear milestone
No items
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
gabogg/hikcentral!173
No description provided.