[dborup]

What

The Period select on #/perf (WebSocket clients history) stood out from the rest of the UI. It only had inline min-height:44px;padding:6px, so the browser drew its native control: grey #3b3b3b background, #858585 square border, 13.3px/400 text. Those are system colours that ignore the theme. The "Period" text was body-sized (15.6px).

Measured live on meshview.dk next to the Analytics "Time window" select, which does the same job:

	Perf → Period (before)	Analytics → Time window
Background	#3b3b3b (browser default)	var(--input-bg)
Border	#858585, 0px radius	var(--border), 6px radius
Font	13.3px / 400	13px / 500
Height	44px inline	34px

Changes

• One shared class. .analytics-time-window-select → .time-window-select, used by Analytics (time window + scatter X/Y axes) and the Perf Period select. Same CSS, new name only.
• Label. "Period" uses the existing .region-filter-label, matching "Region:" on Analytics.
• Heading icon. "WebSocket clients" gets ph-chart-line, like the page heading and "Go Runtime". Its accessible name is unchanged.
• Font. The shared rule sets font-family: var(--font), as .filter-bar select does.
• Touch target. .time-window-select joins the upstream follow-up (2292): feat(observers): radio setting per observation from the observer's radio history #630-1 48px touch-target block, so on touch screens it matches the region dropdown it sits next to on Analytics. This also applies to the Analytics selects.

The owner didn't answer two choices before "go", so I picked defaults. Both are easy to drop:

• 48px on touch, Analytics included.
• The heading icon.

Layout (owner request, commit 0ff1ec34)

The overview row (Total Requests, Avg Response, Uptime, Slow, Version) now sits above "WebSocket clients".

The cards were built inside #perfContent, which is rebuilt every 5 s. The history section stays outside that rebuild on purpose, so the Period select keeps focus and an open menu across refreshes. So instead of moving the section into the rebuild, the cards get their own #perfOverview container above it. renderPerfOverview() is the same markup, extracted.

The unit tests check the order overview → WS history → rest, and that the cards render only in #perfOverview. The render-race tests now read the overview container.

Tests

• test-perf-ws-clients.js (+2):
  • The Period select has class="time-window-select", no inline style, the .region-filter-label label and the heading icon.
  • The shared CSS rule uses theme tokens and var(--font), the old class name is gone, and the select is in the upstream follow-up (2292): feat(observers): radio setting per observation from the observer's radio history #630-1 48px touch block.
• test-perf-ws-history-e2e.js (+2 checks, +1 assertion):
  • The computed style of the Period select, font-family included, equals the real #analyticsTimeWindow in light and dark, and its background differs between the two themes. Transitions are switched off for the comparison, so no fixed sleep is needed.
  • On touch, both the Period select and the Analytics time window are at least 48px high.
  • The existing role check now requires the select's accessible name to be exactly "Period".
• E2E is 9/9, three runs in a row, against a local server on the migrated fixture. With master's perf.js the new style check fails. With the 48px rule removed, both the unit and the E2E test fail.
• Also run: test-perf-*.js, test-packet-filter.js, test-aging.js, test-frontend-helpers.js, test-analytics-fluid-charts.js, test-repeater-metric-scatter.js. All pass.
• scripts/check-xss-sinks.sh --diff origin/master exits 0.

Browser validation

On a local server:

• Perf, light and dark at 820px: the Period select is themed like Analytics.
• Analytics: unchanged.
• 375px mobile: the select fits within 375px, and the page has no horizontal overflow.

Noticed, not changed

Editing ?wsRange= by hand while already on #/perf updates the URL but not the select, until a reload. That's existing range-routing behaviour this PR doesn't touch.

Perf

CSS class rename and markup only. The overview row is the same string, written to a different element. No new work per render.

🤖 Generated with Claude Code

[dborup]

Review — review-agent — head 431e894

Dom: changes suggested (no blockers; one should-fix on the touch-target size)

All line numbers refer to the head branch. I read the code only and did not run the tests or a browser.

Blockers

None.

Should-fix

1. Touch height is 44px, but the repo's touch standard is 48px, and the neighbours on Analytics are 48px.
   • public/style.css:3725 sets .time-window-select to 44px under pointer: coarse.
   • The existing block at public/style.css:4620-4640 ("upstream follow-up (2292): feat(observers): radio setting per observation from the observer's radio history #630-1: Touch targets — 48px house minimum on touch devices") gives .region-dropdown-trigger, .multi-select-trigger, .tab-btn and .filter-bar select 48px.
   • .region-dropdown-trigger sits in the same .analytics-filters row as #analyticsTimeWindow (public/analytics.js:216-219). On a touch device the row therefore shows a 48px region trigger next to a 44px select, centred by align-items:center. Before the PR the gap was larger (34px next to 48px), so this is an improvement but still not aligned.
   • Suggested fix: add .time-window-select to the #630-1 selector list, so it gets min-height: 48px. Drop the new standalone @media line at style.css:3725, or raise it to 48px. The Period select then matches the other touch controls and the Analytics row aligns. If 44px is chosen on purpose (WCAG AAA 2.5.5), the PR description should say it deviates from the 48px rule.
   • Current CSS mechanics, for reference: the global select { min-height: 48px } (style.css:659) has no media query. The .time-window-select base rule (min-height: 34px) overrides it on desktop, as intended. On touch the new rule overrides it down to 44px.
   • The unit test (test-perf-ws-clients.js, the new CSS test) and the E2E assertion (rect.height >= 44) encode 44px. If you move to 48px, update both.

Nits

2. The heading icon claim is a little off. The PR text says the icon matches "the other Perf section headings". In public/perf.js only the page <h2> (line 298) and "Go Runtime" (line 368) have an icon. Most <h3>s (System Health, Disk I/O, Cache, SQLite, Server Endpoints, …, lines 384-576) are plain. The icon is harmless: it is aria-hidden, ph-chart-line exists in the sprite, and the accessible name stays "WebSocket clients". The claim is wrong, though, and the PR itself says this was a default pick. Consider dropping the icon, or giving the neighbouring sections icons in a separate change.
3. Unit test brittleness (test-perf-ws-clients.js, new CSS test).
   • It asserts exactly 3 occurrences of class="time-window-select" in analytics.js. Adding a fourth select breaks it without any regression.
   • It checks literal declaration strings (background: var(--input-bg) etc.). A reformat or shorthand change breaks it.
   • css.match(/\.time-window-select\s*\{…/) takes the first match. This works now, but it would pick the @media inner rule if that were moved above the base rule.
   • The "old class name must be gone" check is useful. The count and string checks could be relaxed to >= 1 and to the computed-style comparison already done in the E2E.
4. E2E uses a fixed 300 ms sleep (test-perf-ws-history-e2e.js, styleOf). It is meant to cover the customizer's re-apply plus the 0.15s border transition. A fixed sleep can be flaky on slow CI. Prefer waitForFunction on the computed backgroundColor changing or settling, or await two animation frames plus getAnimations() finishing. It passed 3 times locally per the PR, so this is a risk, not a failure.
5. The theme check leaves the page on #/analytics with data-theme="dark". That is fine today, because only the error assertion follows. A check appended later on the same page would inherit that state. Resetting the theme and route at the end of the check would be safer.
6. Coverage gap: touch alignment on Analytics. The PR text says the coarse rule also affects the Analytics selects, but no test covers it. The mobile check only covers #wsHistoryRange. A 375px touch assertion on #analyticsTimeWindow (height equal to the neighbouring .region-dropdown-trigger) would cover finding 1.
7. No font-family on .time-window-select (style.css:3719-3723). Other form controls set font-family: var(--font), for example .filter-bar select at style.css:1040. This select uses the UA default font in size and weight. It was already the case for Analytics and is consistent between the two pages. The E2E computed-style comparison does not include fontFamily, so it could not catch a drift. Optional.
8. Guess, not verified: the Period select's accessible name may include the selected option, e.g. "Period 24 hours". The cause is that the <select> is nested inside its <label>, which also holds the "Period" text span. This structure existed before the PR. The E2E uses getByRole('combobox', { name: 'Period' }), which matches by substring and would not notice. Not caused by this PR.

Checked and OK

• Rename completeness. git grep over the whole head tree: analytics-time-window-select appears only inside the negative assertion in test-perf-ws-clients.js. There are no leftovers in CSS, JS, HTML, docs, the customizer files or other tests. The three analytics.js call sites (lines 219, 3197, 3200) are updated. #analyticsTimeWindow is still addressed by id and data-testid.
• Theming. The shared rule uses only var(--border), var(--input-bg), var(--text) and var(--accent). --input-bg is defined for light and dark (style.css:312, 442, 497). There is no hardcoded colour in the new CSS or markup. The inline style="min-height:44px;padding:6px" is removed.
• Label. <label for="wsHistoryRange"> still wraps the select. .region-filter-label is the same class Analytics uses for "Region:". The decorative icon is aria-hidden and the heading still labels the section through aria-labelledby.
• Layout. .perf-range-control is a small inline-flex and the wrapping flex row is unchanged. The mobile 375px width and overflow checks are kept.
• Perf and XSS. The markup is static, with no interpolated node data and no new work per render. The select stays outside the 5s perfContent rebuild, as before.

[dborup]

Review feedback addressed (commit 107f173f)

1. Touch height (should-fix): the one-off 44px pointer: coarse rule is removed. .time-window-select is now in the upstream follow-up (2292): feat(observers): radio setting per observation from the observer's radio history #630-1 48px touch block, so it matches the region dropdown trigger next to it on Analytics. Unit and E2E tests now require 48px.
2. Heading icon claim: the PR description now says the icon matches the page heading and "Go Runtime", not "the other section headings".
3. Brittle unit CSS test: the declarations are checked by pattern, the fixed call-site count is replaced by a check on #analyticsTimeWindow, and the test checks the upstream follow-up (2292): feat(observers): radio setting per observation from the observer's radio history #630-1 block itself.
4. Fixed 300 ms sleep: removed. The E2E switches transitions off and waits one animation frame for the customizer's theme observer.
5. E2E leaves the page on #/analytics in dark: not changed. It is the last desktop check, and that browser context is closed right after it.
6. No Analytics touch test: added a mobile check that #analyticsTimeWindow is at least 48px high on touch.
7. font-family: the shared rule now sets font-family: var(--font), as .filter-bar select does, and the E2E compares font-family too.
8. Accessible name might include the selected option: checked in Chromium. The ARIA snapshot shows combobox "Period", and getByRole('combobox', { name: 'Period', exact: true }) finds exactly one element. The existing check now uses exact: true.

Verification:

• E2E 9/9, three runs in a row, on a local server with the migrated fixture.
• With the 48px rule removed, both the unit and the E2E test fail.
• All test-perf-*.js, test-repeater-metric-scatter.js and test-analytics-fluid-charts.js pass.
• check-xss-sinks.sh --diff origin/master exits 0.

[dborup]

Review — CS-Coordinator PR#493 — head 0ff1ec3

Verdict: APPROVE

The changes since the previous review (431e8949) are 107f173f and 0ff1ec34. Checked:

• One shared select style. analytics-time-window-select is renamed to time-window-select. Analytics (time window and scatter axes) and the Perf Period select now use it. No reference to the old class remains anywhere in public/, on this branch or on current master, and a test asserts it is gone.
• Touch size. The one-off 44 px rule is replaced by the shared 48 px touch block (min-height and height), consistent with the other filter controls.
• Overview above the history. renderPerfOverview moves the server overview row into #perfOverview above the WebSocket history, outside the 5 s #perfContent rebuild, so the Period select keeps focus. The escaping is unchanged: the same escapeHtml calls, and the slow-query count is numeric.
• The ph-chart-line icon exists in the sprite.
• Tests are updated (test-perf-ws-clients.js, test-perf-ws-history-e2e.js).
• The branch merges cleanly with master, and CI is green.
• It has run on staging since 2026-10-16 (pr493-* and the current master combo), with the select themed in light and dark and 48 px at 375 px.