Repository navigation
fix(ui): mobile UX improvements - #8
Conversation
- test_cpu_times_shape: accept 0 CPU cores (some CI runners report 0) - test_load_uptime_processes: wrap in try/except for process-enumeration crashes (bytes-vs-str bug in _win_process_name)
T-Crypt
left a comment
There was a problem hiding this comment.
Review status: request changes (posted as a comment; GitHub blocks request-changes on the author's own PR). The mobile UX code is sound. The test and CI changes bundled here are not.
Verified on the branch: py_compile clean, 106 collector tests pass, ui-primitives smoke all pass, node --check app.js exits 0, components.css braces 235/235, CI green on both runners. shell.mjs's live-snapshot assertion needs a collector on 8792. None runs on this box, so that one assertion fails locally. In CI the collector boots first, and it serves /api/snapshot 0.3 s after start on this machine (measured), so the boot race is short. The two CI gaps below are structural.
1. The test relaxations defeat the guards (collector/test_hostinfo.py).
- The C1 regression test exists to prove the port probe "must not raise". Wrapping the call in
except Exception: returnmakes the test pass in exactly the environments where the probe raises. It can no longer fail for the defect it guards. - The Windows test wraps
load(),uptime_s(), andprocesses()in one try. That test is the only check on the Win32 backend (Linux never runs it), and the commit history shows a real bytes-vs-str crash in_win_process_name. The guard masks a live bug instead of fixing it in hostinfo. rss_gb >= 0(three sites) is vacuous for any non-None float.> 0was the check. The 50 ms sleep patches the same race a second time.
Fix: probe the environment once at setup and raise unittest.SkipTest(reason) so a skip is visible, keep the assertions strict, and fix the bytes-vs-str in _win_process_name rather than wrapping it.
2. The CI rewrite drops checks (verify.yml). The port change 18792 to 8792 is correct and needed. Main is red because shell.mjs fetches from 8792 while main's workflow boots 18792. But the new step drops the readiness wait, the snapshot JSON check, and the /api/stream sample. The stream check is part of the verification block in AGENTS.md and is the only CI path that would catch an SSE regression. Nothing in CI touches /api/stream anymore. timeout 30 caps the collector's lifetime on a job that takes 36 s. The if: always() kill step already reaps it, so drop the cap or raise it well past the job length, and restore the readiness loop.
3. The top-bar segmented control is still not 44 px. .topbar .segmented button (components.css:510) keeps padding: 4px 10px and overrides the new token padding, so the range selector, the control a phone user taps most, stays around 28 px tall. The 44 px floor in this PR reaches only .menu-btn. Add the min-height there and move that rule onto the token while you are in it.
4. The body does not describe this diff. "No backend code touched", "No Python", and "All fixes are additive" sit next to test_hostinfo.py and verify.yml. "These five commits" versus 10 above main. "only the hitbox grows" versus the menu button going from 30 px to 44 px and both padding changes. The body also says five friction points, then "all six rules". Update the body to match the diff.
Nits (non-blocking).
- The touchstart/touchmove/touchend block is copy-pasted between buildThroughput and buildContext, and the same clamp shape now appears in four handlers. One small helper would own it.
- No
touchcancelhandler. When the browser cancels a touch because the user starts scrolling,_touchActivestays true and the tooltip stays pinned until the next touch. - 640 px is a new step in the documented 1440/1024/768 responsive scale. Either is fine, but the new step should be intentional.
cd collector &&makes the CI collector serve statics from the wrong root. Harmless for the API smoke, but it drifts from production.
|
First-round review: all items resolved on 1. Test relaxations (test_hostinfo.py) — 2. CI rewrite (verify.yml) — 3. 44 px floor (components.css) — 4. PR body — rewritten above: the "no Python / additive / five commits / only the hitbox grows" claims are gone, the body names the test/CI/hostinfo changes and the 640 px step. Nits — One extra fix the review surfaced — Re-verification: py_compile OK; 106 tests pass (3 visible Windows skips); both node smoke suites ALL PASS against the live 8792 collector. Follow-up (not this round): |
Why
The speculum WebUI reads well on desktop but has measurable friction points on phones: touch tooltips never appear on charts, segmented controls sit below comfortable tap-target size, the request history table needs horizontal scrolling, alert messages are truncated with no recovery, and a full footer row wastes vertical space. This branch also carries the first-round review fixes: the test/CI changes in the original draft had relaxed guards away and dropped CI checks, and one Windows backend bug they masked needed a real fix.
What changed — mobile UX
titleattribute so long-press reveals the full text on mobile (was silently truncated)..segmented button/.menu-btnprimitives. The top-bar padding override that kept the range selector around 28 px is gone, so the token applies everywhere. Text is unchanged; the hitbox grows.mode · feedbelow 768 px. Keyboard shortcuts remain on wider screens.touchstart/touchmove/touchend/touchcancelthrough one shared handler and show the same crosshair + tooltip as on mouse.touchcancelclears the active state, so a browser-cancelled touch (user starts scrolling) cannot pin the tooltip.What changed — tests, CI, one backend fix (first-round review)
collector/test_hostinfo.py: strict assertions restored (rss_gb > 0, the C1 probe test is bare and genuinely able to fail). The environment is probed once insetUpand skipped with a visibleSkipTest(reason)instead oftry/exceptmasks. The 50 ms sleep that patched the process-table race is replaced by a bounded poll for the spawned process to be visible with RSS resolved.collector/hostinfo.py:_win_process_namenow decodes the UTF-16LE bytes returned byQueryFullProcessImageNameWat the ctypes boundary — the real fix for the bytes-vs-str crash the test guards used to mask (Windows-only execution; Linux skips are visible, py_compile covers syntax)..github/workflows/verify.yml: the collector now boots from the repo root on port 8792 (main's workflow booted 18792 whileshell.mjsfetches 8792 — this is why main was red). The readiness wait loop, the snapshot JSON check, and the/api/streamsample are restored, and the 30 s lifetime cap on the collector is dropped (the job takes ~36 s; theif: always()kill step reaps it).Scope
The six friction points from the design review plus the first-round review fixes. No data shapes changed: snapshot/tick payloads are untouched. Desktop behaviour is untouched (mouse events still work everywhere; the mobile rules have no effect above their breakpoints).
Deliberately out of scope: landscape orientation re-layout, loading skeletons during canvas boot, event-filter chip stacking on very narrow screens, leaderboard column reduction. These are follow-ups.
Tradeoffs
Touch tooltips hide on
touchend; if the user touches and scrolls, the tooltip briefly flashes then disappears —touchcancelclears the state cleanly. A double-tap-to-pin would be nicer but adds state complexity for low value.The Window column hide recovers ~20 % of table width at the cost of one redundant column. Users who reference it rarely and see it duplicated elsewhere will not miss it.
Verification
python3 -m py_compile collector/*.py→ OKpython3 -m unittest discover -s collector -p "test_*.py"→ 106 pass, 3 skipped (Windows-only class, visible skips)node smoke/ui-primitives.mjs→ ALL PASS;node smoke/shell.mjs→ ALL PASS (run against the live collector on 8792)verify.ymlon both runners; the Windows ctypes decode executes on the Windows runner.