Skip to content

fix(css): 48 px touch targets for the add-channel dialog buttons (#249) - #264

Merged
dborup merged 2 commits into
masterfrom
codex/issue-249-add-channel-buttons-48
Oct 5, 2026
Merged

dborup merged 2 commits into
masterfrom
codex/issue-249-add-channel-buttons-48

Conversation

@dborup-agent

Copy link
Copy Markdown
Collaborator

Relates to #249

The add-channel dialog's three primary buttons were 32 px tall on touch, below the 48 px house minimum (Kpa-clawbot#2052). This raises them to 48 px under (pointer: coarse), the same scope as the Kpa-clawbot#630 coarse-pointer block that #239 raised. Tests are written first: they go red on master, and the fix turns them green.

  • Production code: one CSS rule in public/style.css, placed after the dialog close button's ≤640px block from fix(css): raise the remaining 44 px controls to the 48 px touch-target minimum (#235) #239. It sets only min-height: 48px. No colours, tokens, JS or markup change.
  • No npm dependencies added. .github/ is not in the diff; fork guards unchanged (9 in deploy.yml, 1 in release-fast-path.yml). scripts/check-xss-sinks.sh --diff origin/master is clean.

Plan

  1. Tests first (4df9ecc9): add the three buttons to test-touch-targets.js in their real dialog rows, and to test-issue-2052-touch-target-e2e.js in the real DOM at 320, 390, 640 and 768 px with touch, plus a desktop check at 1440.
  2. Fix (e2ac1b5b): @media (pointer: coarse) { #chGenerateBtn, #chPskAddBtn, #chHashtagBtn { min-height: 48px; } }.
  3. Check the dialog at 320/390/640/768 touch and 1440 desktop: heights, label centring, no overflow, no other style change. Then take before/after screenshots at 390 px in light and dark, and run mutants.
  4. There are no configurable values, so nothing for the customizer.

Measurements

Real DOM, add-channel dialog open, master → this branch:

Viewport #chGenerateBtn #chPskAddBtn #chHashtagBtn
320×640 touch 180.8×32 → 180.8×48 67.2×32 → 67.2×48 91.3×32 → 91.3×48
390×844 touch 180.8×32 → 180.8×48 67.2×32 → 67.2×48 91.3×32 → 91.3×48
640×900 touch 180.8×32 → 180.8×48 67.2×32 → 67.2×48 91.3×32 → 91.3×48
768×1024 touch 180.8×32 → 180.8×48 67.2×32 → 67.2×48 91.3×32 → 91.3×48
1440×900 desktop 180.8×32 (unchanged) 67.2×32 (unchanged) 91.3×32 (unchanged)

The inputs next to the buttons, and Scan QR, were already 48 px on master (global rules), so each dialog row stays 48 px tall. The dialog's height doesn't change at any width, and neither does its width or any button's width. Labels stay centred (offset 0.0/0.0 px), and the dialog and page have no horizontal scroll at any width. Background, text colour, padding, font size and radius are the same as on master.

Why touch-only: the issue asks for the touch minimum, and #239 kept desktop as it was. Under (pointer: coarse), a 768 px touch tablet is covered too, which a max-width: 640px rule would miss. On desktop the buttons stay 32 px next to 48 px inputs, as on master. If you'd rather align desktop too, removing the media wrapper does it, and the desktop pin in the E2E would then need updating.

Screenshots

390×844 touch, before = master 0572e7f9, after = this branch. They're on an evidence-only branch that isn't meant to be merged: https://github.com/dborup/CoreScope/tree/codex/issue-249-screenshots

before after
light
dark

Tests

  • test-touch-targets.js (CI E2E step, no server, iPhone 13): renders the three buttons with their real ids and labels in their real .ch-modal .ch-modal-row markup, each next to its input (the hashtag row has its # prefix too). It asserts ≥48×48, a label centred within 1 px, and the button inside the dialog. On master all 3 fail.
  • test-issue-2052-touch-target-e2e.js, real DOM: at 320, 390, 640 and 768 px with touch, each button is ≥48×48, visible, unclipped and hit at its centre. Each has a centred label and sits inside the dialog, and neither the dialog nor the page scrolls sideways. At 1440 px without touch, the buttons keep their 32 px height, which pins the touch-only scope. On master the 4 touch steps fail.

Mutants, each must fail a test:

Mutant Caught by
#chPskAddBtn dropped from the rule (back to 32 px) harness and all 4 touch E2E steps
rule moved out of the touch block (global) E2E desktop pin (48 px on desktop)
rule moved into @media (max-width: 640px) E2E 768 px touch step
min-height: 44px harness and all 4 touch E2E steps
label pushed off centre (display: flex; align-items: flex-start) harness and all 4 touch E2E steps (offset −8 px)

Not changed here

#chSuggestBtn, the optional Suggest button in the same dialog, only shows when public channel suggestions are enabled. It is also 32 px on touch (95.2×32 at 390 px), but #249 doesn't list it, so it is left for a follow-up.

Perf

No hot path: one static CSS rule, no JS.

🤖 Generated with Claude Code

dborup and others added 2 commits October 5, 2026 14:24
test-touch-targets.js renders #chGenerateBtn, #chPskAddBtn and
#chHashtagBtn in their real dialog rows and requires 48x48, a centred
label and no overflow out of the dialog. The Kpa-clawbot#2052 E2E checks the same in
the real DOM at 320, 390, 640 and 768 px with touch, and pins the desktop
height at 32 px, since the fix is touch-only.

Red on master: all three buttons render 32 px tall.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
#chGenerateBtn, #chPskAddBtn and #chHashtagBtn were 32 px tall next to
their 48 px inputs. Under (pointer: coarse) they now get min-height: 48px,
like the Kpa-clawbot#630 coarse-pointer block. The labels stay centred, the rows stay
48 px tall, and desktop is unchanged.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@dborup-agent

Copy link
Copy Markdown
Collaborator Author

Rapport — CS-pve-agent3 PR#264 #249 — head e2ac1b5

Status: All three add-channel dialog buttons are 48 px tall on touch at 320–768 px, and desktop is unchanged. Tests went red first; 5 of 5 mutants are killed; CI is green on the first run. Still a draft.

Evidence: [T] test run here · [A] ad-hoc measurement or screenshot here · [K] from the issue or another PR, not re-run.

Setup [T]: worktree from origin/master 0572e7f9. The Go server, migrate tool and ingestor were built from this branch. A copy of e2e-fixture.db was prepared as CI does: freshen, the Kpa-clawbot#1486/Kpa-clawbot#1791 seed SQL, corescope-migrate, then seeds 2073 and 199. Two servers ran on that DB: one served this branch's public/, the other master's public/ (for before). Both were stopped via their pid files, and the ports were checked free afterwards.

Acceptance criteria (#249)

# Criterion Result
1 All three buttons ≥48 px tall at mobile width, no overflow in the dialog Met at 320, 390, 640 and 768 px touch. The dialog and page have no horizontal scroll. Desktop 1440 is unchanged. [T][A]
2 A test fails if one goes back to 32 px test-touch-targets.js and test-issue-2052-touch-target-e2e.js. Mutant M1 is killed by both. [T]

Measurements, before → after (real DOM, dialog open) [A]

Viewport #chGenerateBtn #chPskAddBtn #chHashtagBtn
320×640 touch 180.8×32 → 180.8×48 67.2×32 → 67.2×48 91.3×32 → 91.3×48
390×844 touch 180.8×32 → 180.8×48 67.2×32 → 67.2×48 91.3×32 → 91.3×48
640×900 touch 180.8×32 → 180.8×48 67.2×32 → 67.2×48 91.3×32 → 91.3×48
768×1024 touch 180.8×32 → 180.8×48 67.2×32 → 67.2×48 91.3×32 → 91.3×48
1440×900 desktop 180.8×32 → 180.8×32 67.2×32 → 67.2×32 91.3×32 → 91.3×32

Same probe, before and after, at every viewport [A]:

  • The label offset from the button centre is 0.0/0.0 px.
  • Each dialog row is 48 px tall, because the inputs and Scan QR were already 48 px on master.
  • The dialog's scroll height is unchanged: 823 / 756 / 662 / 662 / 662 px.
  • The dialog has scrollWidth = clientWidth, and the page has no horizontal overflow.
  • Button background, text colour, padding, font size and radius are identical to master.

The only computed change is the button height.

Tests [T]

# Mutant Killed by
M1 #chPskAddBtn dropped from the rule (32 px) harness, and E2E at 320/390/640/768 touch
M2 rule moved out of the touch block (global) E2E desktop pin (48 px on desktop)
M3 rule moved into @media (max-width: 640px) E2E 768 px touch step
M4 min-height: 44px harness, and E2E at all 4 touch widths
M5 label pushed off centre (display: flex; align-items: flex-start) harness, and E2E at all 4 touch widths (offset −8 px)

5 of 5 killed. M2 and M3 are caught only by the real-DOM E2E, because the harness runs a single phone context.

Screenshots [A]

Taken at 390×844 touch in light and dark, before (master 0572e7f9) and after (this branch). They are on the evidence-only branch codex/issue-249-screenshots and are embedded in the PR description.

  • Before: the three blue buttons are visibly shorter than the inputs next to them and than Scan QR.
  • After: all three are as tall as their inputs, with the labels centred. Everything else is identical in both themes: the callout, section titles, hints, inputs, Scan QR, the footer and the close button.
  • The whole dialog still fits in one view at 844 px height.

Rules [T]

  • One CSS rule; no colours and no new tokens.
  • scripts/check-xss-sinks.sh --diff origin/master exits 0 (no public/**/*.{js,html} changes).
  • Fork guards: 9 in deploy.yml and 1 in release-fast-path.yml. .github/ is not in the diff.
  • No closing keywords: closingIssuesReferences is empty, and the title, body and commits contain none.
  • Both commits (4df9ecc9 tests, e2ac1b5b fix) have author and committer dborup <kontakt@meshview.dk>.
  • No npm dependencies were added.

CI, run 37325849948 [T]

Job Result
Go Build & Test pass (22m)
Playwright E2E Tests pass (20m). The log shows the 5 new #249 steps in the Kpa-clawbot#2052 E2E (19 passed, 0 failed) and the 3 new harness checks (OK).
Build & Publish Docker Image pass
Release Artifacts, Deploy Staging, Publish Badges & Summary skipped (PR / fork guard)

No re-runs were needed; the known flaky test #256 did not fail.

Remainders

@dborup-agent

Copy link
Copy Markdown
Collaborator Author

Review — CS-pve-agent1 PR#264 — head e2ac1b5

Dom: APPROVE

Evidence: [T] test run here · [A] ad-hoc measurement, screenshot or code reading here · [K] from the issue, the PR or CI, not re-run.

Setup [T]:

Findings

# Severity Finding Evidence
1 Info, pre-existing The Kpa-clawbot#2052 E2E failed once in 4 local runs, in the #235 avatar step, not in a #249 step: back button centre hits "gesture-hint-text", not the control. The edge-drawer gesture hint (gesture-hints.js, top-left, shown 800 ms after load, fades after 8 s) can sit over the channel back button. The test never marks the hints as seen. The 3 reruns were 19/0, and CI was 19/0. This PR does not touch that step or gesture-hints.js. A follow-up could mark the hints seen in the test's addInitScript. [T]
2 Info, out of scope #chSuggestBtn (the optional Suggest button, channel-proposals.js:263) is also a .btn-primary in the same dialog. The rule uses three ids, so it does not cover that button. This matches #249's scope, and the author already lists it as a follow-up. [A] code; size [K] from the author
3 Info The E2E pins the desktop height at exactly 32 px. That is a deliberate scope pin (touch-only, like #239), and it is documented in the PR. If desktop is aligned later, that step must change with it. [A]

There are no blocking findings.

The review points

  1. Button size [T][A]. The add-channel modal has no tabs. It is one scrolling dialog with three sections (Generate, PSK, Hashtag), and I measured every section with the modal open in the real app. My own probe ran on both servers, with the dialog opened via #chAddChannelBtn and each button scrolled into view.

    Viewport #chGenerateBtn #chPskAddBtn #chHashtagBtn
    320×640 touch 180.8×32 → 180.8×48 67.2×32 → 67.2×48 91.3×32 → 91.3×48
    390×844 touch 180.8×32 → 180.8×48 67.2×32 → 67.2×48 91.3×32 → 91.3×48
    640×900 touch 180.8×32 → 180.8×48 67.2×32 → 67.2×48 91.3×32 → 91.3×48
    768×1024 touch 180.8×32 → 180.8×48 67.2×32 → 67.2×48 91.3×32 → 91.3×48
    1440×900 desktop 32 → 32 32 → 32 32 → 32

    matchMedia('(pointer: coarse)') is true in every touch context and false at 1440. elementFromPoint at each button's centre hits the button.

  2. No overflow, centred labels, desktop [T][A].

  3. Tests catch a return to 32 px [T]. I ran the PR's test files against master's CSS: the harness reports 3 failures, and the style.css declares .nav-btn and .ch-icon-btn at both 48px and 44px Kpa-clawbot/CoreScope#2052 E2E is 15/4, with all 4 touch steps red. On the merged tree, the harness is OK and the E2E is 19/0. The E2E measures in the real context (the live app, modal opened by its button), and the harness renders the real ids, labels and row markup. See also the mutants below.

  4. No regression in fix(css): raise the remaining 44 px controls to the 48 px touch-target minimum (#235) #239's targets [T][A].

  5. Screenshots at 390×844 touch [A] (the dialog element, before = master public/, after = merged):

    • Light, after: Generate & Show QR, Add and Monitor are solid blue and exactly as tall as the inputs beside them and as Scan QR. Each label is centred, and the rows line up flush.
    • Dark, after: the same geometry. Blue buttons with light text sit on the dark card, and the inputs have a dark fill.
    • Before (both themes): the three blue buttons are visibly shorter than their inputs, which leaves a gap above and below them.
    • Everything else is identical before and after in both themes: the warning callout, section titles and hints, inputs, Scan QR, the # prefix, the case-sensitivity note, the footer and the close button.
    • At 390 the name input placeholder is truncated ("Channel name (e…"), the same as on master.

Tests and mutants [T]

On the merged tree:

  • cmd/server go test -timeout 30m ./...: ok (1528 s).
  • cmd/ingestor go test -timeout 30m ./...: ok (1071 s).
  • Both suites first ran together with Go's default 10 min timeout. On this 4-core box they hit that limit while still running (Test1690_BackgroundLoadHonesty and TestConcurrentWrites), with no failing test. CI uses -timeout 20m. This PR changes no Go code.
  • sh test-all.sh: 219 of 219 files pass.
  • node test-frontend-helpers.js: 707 passed, 0 failed.

E2E against the local server, all exit 0:

Own mutants: each was applied to the rule in public/style.css and run against the harness and the Kpa-clawbot#2052 E2E. The file was restored and checked with cmp afterwards.

# Mutant Harness Kpa-clawbot#2052 E2E
R1 min-height: 47px (boundary) killed (3 ❌) killed (4 touch steps)
R2 #chHashtagBtn dropped from the rule killed killed (4 touch steps)
R3 extra #chGenerateBtn { min-width: 400px } (overflow) killed (not inside the dialog) killed at 320 and 390 (clipped by 92.8 / 25.6 px)
R4 rule cancelled at min-width: 700px survives (phone context only) killed (768 touch step)

4 of 4 killed by at least one test. R4 is caught only by the real-DOM E2E, the same as the author's M3.

CI, run 37325849948 on e2ac1b5b [K, checked per job]:

Neither known flake (#256, #267) fired.

Rules [T][A]

  • Production diff: one 7-line CSS rule. No JS, no markup, no colours and no new tokens. The only hex-like text added is #249 in a comment.
  • Behaviour: the only change is min-height on three buttons under (pointer: coarse).
  • Go: the merged tree has no cmd/, internal/ or .github/ diff against master. So cmd/server stays read-only, and no map[string]interface{} is added (0 in the merged diff).
  • XSS check: scripts/check-xss-sinks.sh --diff origin/master on the head, in a scratch clone, exits 0 ("no public/**/*.{js,html} changes to scan").
  • Fork guards: the guard string counts 9 in deploy.yml and 1 in release-fast-path.yml, unchanged.
  • Closing keywords: none in the title, body or commits, and closingIssuesReferences is empty.
  • Commits: 4df9ecc9 (tests) and e2ac1b5b (fix). Both have author and committer dborup <kontakt@meshview.dk>. The tests come first and are red before the fix.

Not verified

  • A real touch device. All touch measurements use Chromium emulation (hasTouch + isMobile).
  • WebKit/Safari and Firefox.
  • #chSuggestBtn with public suggestions enabled. Its 32 px figure is the author's [K].
  • The live-filter hit-area in the real #/live DOM with the toggles panel open. I covered it only through the harness and diff scope.
  • The -race variant of the Go suites. CI ran it and it passed [K].
  • The PR's screenshot branch images. I took my own instead.

🤖 Generated with Claude Code

@dborup
dborup marked this pull request as ready for review October 5, 2026 18:04
@dborup
dborup merged commit a771ca4 into master Oct 5, 2026
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants