Repository navigation
fix(ui): guard channels scroll listener; 48 px gesture-hint and hidden-column pill (#314) - #331
Merged
Merged
Conversation
…ts (#314) Relates to #314. Two pre-existing UI defects found in a browser check of v0.2.2-rc1; neither is a regression. 1. public/channels.js registered an anonymous `scroll` listener on #chMessages whose body did an unguarded document.getElementById('chScrollBtn').classList.toggle(...). The router destroys the page and lets the next init() rebuild #app, so a `scroll` can still reach the detached pane after #chScrollBtn has left the document, and the listener threw. The lookup is now optional-chained like every other #chScrollBtn call site in the file, and the handler is a stable module-level function so init() can drop the pane it held and destroy() can take the listener off again -- at most one live listener, the same leak class as #259/#282 scoped to the element. 2. .gesture-hint-dismiss ("Got it", 26 px) and .col-hidden-pill (18 px) join the Kpa-clawbot#630/#239 coarse-pointer block at the 48 px house minimum, with the flex centring the other 48 px controls use. Touch only; desktop keeps 26 px and 18 px. Tests: test-issue-314-chscroll-listener.js (new, registered in test-all.sh) drives the real channels.js in a vm sandbox that models the router's #app rebuild; test-touch-targets.js covers both controls in their real parents.
Owner
Author
Rapport — CS-Minimax PR#331 #314 — head 256e37eStatus: Both points done, each with a test that is red on Evidence tags: [T] test run, [A] automated gate/CI, [K] code reading or browser measurement. Requirement 1 — the
|
| Item | Evidence |
|---|---|
| Fix | public/channels.js: the handler is a stable module-level onChMessagesScroll, reads the pane off event.currentTarget, and optional-chains the #chScrollBtn lookup the way the file's four other call sites already do. The #chScrollBtn click registration on the next line is optional-chained too. [K] |
| Leak check (#259/#282 class) | Yes, but element-scoped rather than document-scoped: every visit left one detached #chMessages holding a live handler. init() now drops the pane it held before taking the new one and destroy() removes the listener — at most one live listener whatever the call order. [K] |
| Test | test-issue-314-chscroll-listener.js (new, 8 cases), registered in test-all.sh. Drives the real channels.js in a vm sandbox that models the router's #app rebuild, so a second init() really gets a new #chMessages. Red on master: 4 of 8 fail, including Cannot read properties of null (reading 'classList') on the detached dispatch and 4 live listeners after four visits. Green here: 8/8. [T] |
| Acceptance case from the issue | a scroll event on a detached #chMessages does not throw — asserts the pane is really detached, then dispatches. [T] |
| Mutant A | Drop the ?. in the handler → a scroll event on a detached #chMessages does not throw fails (7 passed, 1 failed). [T] |
| Mutant B | destroy() stops removing the listener → destroy() removes the scroll listener fails (7/1). [T] |
| Mutant C | init() stops dropping the previous pane → a re-init without a destroy leaves no handler on the old pane fails (7/1). [T] |
Requirement 2 — .gesture-hint-dismiss and .col-hidden-pill at 48 px under pointer: coarse
| Item | Evidence |
|---|---|
| Fix | Both selectors join the existing @media (pointer: coarse) block in style.css (the Kpa-clawbot#630 group #239 extended): min-height/min-width: 48px plus the inline-flex centring the other 48 px controls use. Sizes and display only — no colours, no new CSS variables. .col-rehide-pill carries .col-hidden-pill, so the post-reveal "hide" affordance is covered as well. [K] |
| Test | test-touch-targets.js extended with both, each in its real parent (the "Got it" button as a flex item of .gesture-hint-inner; the pill inside a <th> of .data-table, where TableResponsive.apply() puts it). Red on master: 34.7x26.0 and 23.0x18.0, matching the issue's measurements. Green here: 48.0x48.0 for both. [T] |
| Mutant D | Remove .gesture-hint-dismiss from the rule → that row fails at 34.7x26.0, 1 assertion. [T] |
| Mutant E | Remove .col-hidden-pill from the rule → that row fails at 23.0x18.0, 1 assertion. [T] |
| Mutant F | 48 px → 44 px → both rows fail at 44.0x44.0, 2 assertions. [T] |
Browser check
Chromium against a local Go server on the e2e-fixture.db, prepared the way the pipeline prepares it.
| Element | mobile 375×812, touch | desktop, fine pointer |
|---|---|---|
.gesture-hint-dismiss ("Got it") |
59.3 × 48.0 (was 34.7 × 26.0) | 59.3 × 26.0, unchanged |
.col-hidden-pill ("+8 hidden") |
75.0 × 48.0 (was 23.0 × 18.0) | 75.0 × 18.0, unchanged |
Rendered widths exceed 48 px because the real labels are wider than the minimum. [K]
#/channels → #/live → #/channels five times, scrolling the message pane on each visit and then dispatching a scroll at the pane that had just left the document: 0 console errors, on both the mobile and the desktop context. On master the final step produces the reported TypeError. [K]
Local suites
| Suite | Result |
|---|---|
Go, all 18 modules (go test ./... per module — this is a multi-module repo, there is no root go.mod) |
all ok, 0 failures [T] |
sh test-all.sh |
226 files, 226 passed, 0 failed (includes test-test-all.js, which confirms the new file is registered exactly once) [T] |
node test-frontend-helpers.js |
709 passed, 0 failed [T] |
| E2E — gestures/touch | test-gestures-1062-e2e.js, test-gestures-1185-scroll-discriminator-e2e.js, test-gesture-hints-1065-e2e.js, test-touch-gestures-coverage-e2e.js, test-issue-2052-touch-target-e2e.js — all pass [T] |
| E2E — channels | test-issue-1367-channels-chat-app-e2e.js, test-issue-1224-channels-mobile-ux-e2e.js, test-channels-list-render-e2e.js, test-channels-selection-flow-e2e.js, test-channels-154-155-e2e.js, test-channel-fluid-e2e.js, test-channel-modal-e2e.js, test-channels-ws-batch-e2e.js, test-channels-client-state-152-e2e.js — all pass [T] |
| E2E — packets layout / column hiding | test-issue-258-column-widths-e2e.js, test-issue-1128-packets-layout-e2e.js, test-issue-96-hide-control-e2e.js, test-issue-1122-packets-filter-ux-e2e.js, test-table-fluid-e2e.js, test-issue-1128-multi-viewport-e2e.js — all pass [T] |
| E2E — a11y | test-a11y-axe-1668.js, test-issue-1668-m2-contrast.js, test-issue-1668-m3-typography.js — all pass [T] |
CI per job (run 37579490801, head 256e37e7)
| Job | Result |
|---|---|
| Go Build & Test | pass, 17m51s [A] |
| Playwright E2E Tests | pass, 25m32s [A] |
| Build & Publish Docker Image | pass, 48s [A] |
| Release Artifacts | skipped (not a release) [A] |
| Deploy Staging | skipped [A] |
| Publish Badges & Summary | skipped [A] |
No reruns. The known flaky test from #271 did not fire.
Gates
| Gate | Result |
|---|---|
scripts/check-xss-sinks.sh --diff origin/master |
clean, exit 0 [A] |
node scripts/check-css-vars.js |
OK — 2177 var() refs, 172 definitions, 0 undefined [A] |
| Fork guards | 9 in deploy.yml, 1 in release-fast-path.yml, unchanged; .github/ is not in the diff [K] |
cmd/server |
not in the diff; no Go file changed, so no new map[string]interface{} [K] |
| npm dependencies | none added [K] |
| Hardcoded colours | none; the new CSS sets sizes and display only [K] |
Remaining
- The packets table header is taller on mobile, because the reveal pill now occupies 48 px inside the
<th>instead of 18 px. That is the intended consequence of the 48 px minimum, and it is the same trade-off fix(css): raise the remaining 44 px controls to the 48 px touch-target minimum (#235) #239 accepted for the channel sidebar header. An invisible::aftertap pad would have kept the visual size, but the pad would overlay the neighbouring sort target in the same header cell, so the visible pill is the safer option. Say the word if you prefer the pad. - "Got it" is now 48 px wide at minimum, so in the narrow swipe tip the hint text wraps to one more line at 375 px. The tip is transient and stays inside the viewport.
- The
@media (pointer: coarse)rule covers.col-rehide-pillthrough.col-hidden-pill. There is no separate test row for it; it shares the selector under test.
…croll-touch-targets # Conflicts: # test-all.sh
Owner
Author
Rapport — CS-Minimax PR#331 master-sync — head bac0819Merged ConflictsOne real conflict, in
Resolved by keeping both, in issue-number order (314, then 322) to match how the surrounding block is ordered. Master's other addition from #318, The other two shared files auto-merged, with both sides landing in different regions:
TestsRun against the merge commit:
E2E against a local Go server on
Browser check — 375x812, touchChromium at 375x812 with
No page errors or console errors during the run. One note for whoever picks up the hint UI next, unrelated to this PR: CIRun 37590366645 on the merge commit —
The three jobs that run for a pull request all pass; the three skips are the workflow's normal PR behaviour, not failures. The PR is no longer conflicting and remains a draft. |
Collaborator
Review — CS-pve-agent2 PR#331 — head bac0819Dom: APPROVE med nits Independent, read-only review. Both points of #314 are fixed, each with a test that is red on Evidence tags: [T] a test I ran, [A] automated gate or CI, [K] code reading or my own browser measurement. Findings
1. Scroll listener
2. Tap targets
3. Master sync
4. Browser checkChromium against a local Go server on the
The desktop column was measured by injecting both controls into 5. Gates
Tests and mutantsEverything below ran on the head tree, which is also the merged tree against
Re-run against the newer
On
The one
Mutants of my own, beyond the author's A–F:
CI run 37590366645 on
Not verified
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Relates to #314
Two pre-existing UI defects from the browser check of v0.2.2-rc1. Neither is a regression; both are present in v0.2.1. Tests first: both go red on
origin/masterand green on this branch.Plan
#chMessagesscroll handler survive the page teardown, and decide whether it is also a listener leak.@media (pointer: coarse)48 px block, the accessibility: full mobile accessibility audit findings Kpa-clawbot/CoreScope#630/fix(css): raise the remaining 44 px controls to the 48 px touch-target minimum (#235) #239 pattern, and extendtest-touch-targets.js.#/channels↔#/liverepeatedly with the console open.1. The channels scroll listener
public/channels.jsregistered an anonymousscrolllistener on#chMessagesinsideinit():The router (
public/app.js) destroys the current page and then lets the next page'sinit()rewrite#app, which detaches#chMessages. Ascrollcan still reach that detached element, and#chScrollBtnis no longer in the document by then, so the lookup returnednull.Is it also a leak? Yes, of the #259/#282 class, but scoped to the element rather than to
document: each visit left one detached#chMessagesholding a live handler, which keeps the old subtree reachable. So both halves are fixed:#chScrollBtncall sites in the file already are, and the handler reads the pane offevent.currentTargetso it does not care whether the pane is still in the document;addEventListeneris a DOM no-op,init()drops the pane it held before taking the new one, anddestroy()removes it. At most one live listener, whatever the call order.document.getElementById('chScrollBtn').addEventListener('click', …)on the next line was the same unguarded shape and is optional-chained too.2. Two mobile tap targets
Both join the existing
@media (pointer: coarse)block instyle.css(the Kpa-clawbot#630 group that #239 extended), with the samemin-height/min-width: 48pxplusinline-flexcentring the other 48 px controls use — their labels are smaller than the box they now sit in. Touch only; desktop is untouched. No colours added, sizes anddisplayonly..col-rehide-pillcarries.col-hidden-pillas well, so the "hide" affordance after a reveal gets the same 48 px.test-touch-targets.jsgets both, rendered in their real parent, because that is what decides the box: "Got it" is a flex item of.gesture-hint-inner, and the reveal pill is appended into a<th>of the packets table byTableResponsive.apply().Measurements
iPhone-class touch context, 375×812, against a local Go server on the
e2e-fixture.dbprepared the way CI prepares it:.gesture-hint-dismiss.col-hidden-pillThe mobile widths differ from the harness's 48 × 48 because the real labels ("Got it", "+8 hidden") are wider than the minimum.
Navigating
#/channels→#/live→#/channelsfive times, scrolling the message pane on each visit and then dispatching ascrollat the pane that had just left the document: 0 console errors, mobile and desktop. On master the last step is the reportedTypeError.Not in the diff
cmd/serverand the rest of the Go tree are untouched; no newmap[string]interface{}..github/is not in the diff — fork guards stay at 9 indeploy.ymland 1 inrelease-fast-path.yml. No npm dependencies added.scripts/check-xss-sinks.sh --diff origin/masteris clean, andscripts/check-css-vars.jsreports 0 undefined vars.