Replaces the manual phone-in-hand QA in #13 with an automated harness, per
the revised definition of done in comment 49590.
make frontend-viewport-test builds dist/, serves it from the same
digest-pinned nginx image and the same nginx.conf the shipping container
uses, and drives a digest-pinned headless Chrome against it over CDP. test/viewport/README.md documents what it covers and what it genuinely
cannot.
The harness found two real defects — filed, not fixed here
This is the actual QA result, and it is the reason the new target currently
exits non-zero on main:
#42 — horizontal overflow at 320px..status-text carries whitespace-nowrap and, once populated, is 325px of unbreakable text in a
270px box; the page scrolls sideways to 350px. Narrow layout only.
#43 — every interactive control is under 44x44..pin-btn is a bare
16x16 (x26), the debug-log label is 89.3x14, #interval-select is 64x28, #pause-btn is 108.2x39.8 in the narrow layout.
Everything else passes at every width, and the desktop viewport is clean
(7/7). Current score: 55 checks over 7 viewports, 47 passed, 8 failed, all 8
attributable to those two issues. The target is out of make check and out
of CI, so main stays green; the TODO records wiring it into CI once #42 and #43 land.
Every check guards its own presence
No check may pass against a page it is not measuring. app-rendered gates on
the host-row count, host-rows-* on having measured rows, and tap-targets-44px on each selector in the control list matching at least a
declared minimum of visible elements — per selector, not in total, so a single
renamed class fails the check rather than only all four going stale at once. .pin-btn declares 10, being rendered one per pinnable host row; the three id selectors declare 1.
Without that floor the tap-target check is inert once #43 lands: an empty
undersized set means both "every control is big enough" and "the selectors
stopped matching", and a size comparison alone cannot tell them apart.
Proof the harness can fail — done four times
Non-negotiable per the DoD, and worth stating explicitly given #14, #16 and #37:
Planted a 900px fixed-width element in a host row. The one viewport
that currently passes clean, 1280x800 desktop, went 7/7 → 5/7, and the
report named div.w-[900px].h-1.flex-shrink-0 as the offender. Overall
8 failures → 20. Reverted.
Neutered the reflow rule (flex-direction: column → row in the max-width: 768px block). host-rows-stacked failed at all four narrow
viewports with "28 of 28 rows wrong: flex-direction is row, expected
column", and the three wide viewports were correctly unaffected. Reverted.
Simulated the post-#43 world and then broke a selector. With the tap
threshold temporarily lowered so nothing is undersized, the run is 53/55
with tap-targets green at all six touch viewports. Renaming .pin-btn
to .pin-button in src/main.js — one line, nothing else — takes it to
47/55 with every touch viewport failing on .pin-btn matched 0 visible element(s), expected at least 10, while all
three surviving controls still satisfy the size threshold. That is the run
which would have been green-and-blind without the presence floor.
Reverted.
Mixed the breakpoint styles. Adding a @media (min-width: 900px)
block beside the existing max-width one aborts the run with an
explanation, rather than testing the right widths against a guessed
expectation. Reverted.
All four were run through make frontend-viewport-test; none of the
modifications is in this branch.
Anomaly worth calling out: innerWidth would have hidden the bug
The DoD specifies documentElement.scrollWidth <= innerWidth. Written
literally, that check passes on the broken 320px page. Under mobile
emulation Chrome lets window.innerWidth grow to the width of the
overflowing content, exactly as a phone zooms out to fit a too-wide page — it
reported 350 against a scrollWidth of 350. My first implementation did this
and reported the page clean. The harness now measures against Math.min(innerWidth, documentElement.clientWidth), which is what surfaced #42, and the browser runs with --hide-scrollbars so no scrollbar-sized
slack creeps back in. Flagging it because it is precisely the shape of a gate
that verifies nothing.
Widths tested, and why
Derived, not hardcoded. viewports.js parses the @media conditions out of src/styles.css and scans src/main.js and index.html for Tailwind
responsive prefixes (currently none — there is not a single sm:/md:/lg:
class in the app), then tests each breakpoint one pixel below, exactly on,
and one pixel above. Add a media block and it gets covered without editing
the harness.
Width x height
Why
Expected
320x568
narrowest viewport still in mainstream use
narrow
667x375
phone in landscape, still inside the narrow layout
narrow
767x1024
1px below the max-width: 768px breakpoint
narrow
768x1024
exactly on it — max-width matches at 768
narrow
769x1024
1px above it
wide
844x390
large phone in landscape: wide layout, still touch
wide
1280x800
desktop baseline
wide
Which layout to expect is derived too, and refused rather than guessed when
it cannot be: a max-width-only set (what the app ships) is narrow where a
block matches, a min-width-only mobile-first set is narrow below every
breakpoint, and a set mixing the two fails the run, because which block owns
the host-row reflow is a property of the rules inside it rather than of the
condition.
"Correct" at each: no horizontal overflow, no element past the edge, no
clipped text, all touch controls present and at least 44x44, and host rows
either stacked full-width (narrow) or side by side (wide) — checked on
computed flex-directionand actual geometry, so a row that merely shrank
its 420px column fails.
Decisions the DoD asked me to make explicit
Driver: puppeteer-core, not playwright. This is the resolution of the
bundled-browser problem: puppeteer-core is the one variant of either
library that never downloads or bundles a browser — it only speaks CDP to a
browser you hand it. The browser therefore stays a digest-pinned container
image, and the npm side is pinned by yarn.lock integrity hashes. playwright expects its own version-matched browser download, which either
fights the digest pin or forces us onto the ~2 GB official Playwright image.
Tap-target threshold: 44x44 CSS px, from Apple's HIG and WCAG 2.2 SC
2.5.5. WCAG 2.2 SC 2.5.8 (AA) permits 24x24, but only with a spacing
exception these controls do not qualify for.
Relation to #21: complementary layers. vitest will exercise
module-level logic in-process; this exercises rendered layout in a real
engine and is the only thing in the repo that can see a media query.
Assertions about functions go in vitest, assertions about computed styles
and geometry go here.
Determinism. The browser container is on an --internal docker network
with no route off the host, so probes cannot reach anything real. The harness
answers them from a fixed delay table with a deterministic fraction failed,
so rows render a realistic spread of one-, two- and three-digit latencies
plus offline rows — that spread is what the layout has to survive. Three --rm containers, all named and all torn down in a trap, so a hard kill
cannot strand one and leave the --internal network behind with it.
Reconciliation with the open PRs
#38 is the one that matters. It moves frontend-only gates into a script/frontend-* namespace, so the entrypoint is named script/frontend-viewport-test up front and needs no rename after #38 lands.
One line will want changing on merge: the script calls script/test to
produce dist/, which becomes script/frontend-test under #38 (calling script/test still works there, it just also runs the Go suite). The Makefile hunk adds one target next to check and does not touch the lines #38 edits. #35 (.gitignore), #31 and #40 do not overlap, beyond #35 also
touching .gitignore — this branch adds a single tmp/ line.
TODO.md gets a small additive edit, deliberately minimal given four other
PRs touch it.
Verification
make check — green.
Uncached docker build --no-cache-filter build — RUN make check observed
executing (step #13, 3.8s, not CACHED), so the shipping image gate
really ran against the new files. Image removed afterwards.
make frontend-viewport-test — runs, and fails for the two reasons above.
Harness-can-fail — proven four times, see above.
No leftover containers, networks or images; the pulled browser image is
left in the local cache deliberately so reruns do not re-pull.
Replaces the manual phone-in-hand QA in #13 with an automated harness, per
the revised definition of done in
[comment 49590](https://git.eeqj.de/sneak/netwatch/issues/13#issuecomment-49590).
`make frontend-viewport-test` builds `dist/`, serves it from the same
digest-pinned `nginx` image and the same `nginx.conf` the shipping container
uses, and drives a digest-pinned headless Chrome against it over CDP.
`test/viewport/README.md` documents what it covers and what it genuinely
cannot.
## The harness found two real defects — filed, not fixed here
This is the actual QA result, and it is the reason the new target currently
exits non-zero on `main`:
- **#42 — horizontal overflow at 320px.** `.status-text` carries
`whitespace-nowrap` and, once populated, is 325px of unbreakable text in a
270px box; the page scrolls sideways to 350px. Narrow layout only.
- **#43 — every interactive control is under 44x44.** `.pin-btn` is a bare
16x16 (x26), the debug-log label is 89.3x14, `#interval-select` is 64x28,
`#pause-btn` is 108.2x39.8 in the narrow layout.
Everything else passes at every width, and the desktop viewport is clean
(7/7). Current score: 55 checks over 7 viewports, 47 passed, 8 failed, all 8
attributable to those two issues. The target is out of `make check` and out
of CI, so `main` stays green; the TODO records wiring it into CI once #42 and
#43 land.
## Every check guards its own presence
No check may pass against a page it is not measuring. `app-rendered` gates on
the host-row count, `host-rows-*` on having measured rows, and
`tap-targets-44px` on each selector in the control list matching at least a
declared minimum of visible elements — per selector, not in total, so a single
renamed class fails the check rather than only all four going stale at once.
`.pin-btn` declares 10, being rendered one per pinnable host row; the three
`id` selectors declare 1.
Without that floor the tap-target check is inert once #43 lands: an empty
undersized set means both "every control is big enough" and "the selectors
stopped matching", and a size comparison alone cannot tell them apart.
## Proof the harness can fail — done four times
Non-negotiable per the DoD, and worth stating explicitly given #14, #16 and
#37:
1. **Planted a 900px fixed-width element** in a host row. The one viewport
that currently passes clean, 1280x800 desktop, went 7/7 → 5/7, and the
report named `div.w-[900px].h-1.flex-shrink-0` as the offender. Overall
8 failures → 20. Reverted.
2. **Neutered the reflow rule** (`flex-direction: column` → `row` in the
`max-width: 768px` block). `host-rows-stacked` failed at all four narrow
viewports with "28 of 28 rows wrong: flex-direction is row, expected
column", and the three wide viewports were correctly unaffected. Reverted.
3. **Simulated the post-#43 world and then broke a selector.** With the tap
threshold temporarily lowered so nothing is undersized, the run is 53/55
with `tap-targets` green at all six touch viewports. Renaming `.pin-btn`
to `.pin-button` in `src/main.js` — one line, nothing else — takes it to
47/55 with every touch viewport failing on
`.pin-btn matched 0 visible element(s), expected at least 10`, while all
three surviving controls still satisfy the size threshold. That is the run
which would have been green-and-blind without the presence floor.
Reverted.
4. **Mixed the breakpoint styles.** Adding a `@media (min-width: 900px)`
block beside the existing `max-width` one aborts the run with an
explanation, rather than testing the right widths against a guessed
expectation. Reverted.
All four were run through `make frontend-viewport-test`; none of the
modifications is in this branch.
## Anomaly worth calling out: `innerWidth` would have hidden the bug
The DoD specifies `documentElement.scrollWidth <= innerWidth`. Written
literally, that check **passes** on the broken 320px page. Under mobile
emulation Chrome lets `window.innerWidth` grow to the width of the
overflowing content, exactly as a phone zooms out to fit a too-wide page — it
reported 350 against a `scrollWidth` of 350. My first implementation did this
and reported the page clean. The harness now measures against
`Math.min(innerWidth, documentElement.clientWidth)`, which is what surfaced
#42, and the browser runs with `--hide-scrollbars` so no scrollbar-sized
slack creeps back in. Flagging it because it is precisely the shape of a gate
that verifies nothing.
## Widths tested, and why
Derived, not hardcoded. `viewports.js` parses the `@media` conditions out of
`src/styles.css` and scans `src/main.js` and `index.html` for Tailwind
responsive prefixes (currently none — there is not a single `sm:`/`md:`/`lg:`
class in the app), then tests each breakpoint one pixel below, exactly on,
and one pixel above. Add a media block and it gets covered without editing
the harness.
| Width x height | Why | Expected |
| -------------- | ----------------------------------------------------- | -------- |
| 320x568 | narrowest viewport still in mainstream use | narrow |
| 667x375 | phone in landscape, still inside the narrow layout | narrow |
| 767x1024 | 1px below the `max-width: 768px` breakpoint | narrow |
| 768x1024 | exactly on it — `max-width` matches *at* 768 | narrow |
| 769x1024 | 1px above it | wide |
| 844x390 | large phone in landscape: wide layout, still touch | wide |
| 1280x800 | desktop baseline | wide |
Which layout to expect is derived too, and refused rather than guessed when
it cannot be: a `max-width`-only set (what the app ships) is narrow where a
block matches, a `min-width`-only mobile-first set is narrow below every
breakpoint, and a set mixing the two fails the run, because which block owns
the host-row reflow is a property of the rules inside it rather than of the
condition.
"Correct" at each: no horizontal overflow, no element past the edge, no
clipped text, all touch controls present and at least 44x44, and host rows
either stacked full-width (narrow) or side by side (wide) — checked on
computed `flex-direction` **and** actual geometry, so a row that merely shrank
its 420px column fails.
## Decisions the DoD asked me to make explicit
**Driver: `puppeteer-core`, not `playwright`.** This is the resolution of the
bundled-browser problem: `puppeteer-core` is the one variant of either
library that never downloads or bundles a browser — it only speaks CDP to a
browser you hand it. The browser therefore stays a digest-pinned container
image, and the npm side is pinned by `yarn.lock` integrity hashes.
`playwright` expects its own version-matched browser download, which either
fights the digest pin or forces us onto the ~2 GB official Playwright image.
**Tap-target threshold: 44x44 CSS px**, from Apple's HIG and WCAG 2.2 SC
2.5.5. WCAG 2.2 SC 2.5.8 (AA) permits 24x24, but only with a spacing
exception these controls do not qualify for.
**Relation to #21:** complementary layers. `vitest` will exercise
module-level logic in-process; this exercises rendered layout in a real
engine and is the only thing in the repo that can see a media query.
Assertions about functions go in `vitest`, assertions about computed styles
and geometry go here.
**Determinism.** The browser container is on an `--internal` docker network
with no route off the host, so probes cannot reach anything real. The harness
answers them from a fixed delay table with a deterministic fraction failed,
so rows render a realistic spread of one-, two- and three-digit latencies
plus offline rows — that spread is what the layout has to survive. Three
`--rm` containers, all named and all torn down in a trap, so a hard kill
cannot strand one and leave the `--internal` network behind with it.
## Reconciliation with the open PRs
**#38** is the one that matters. It moves frontend-only gates into a
`script/frontend-*` namespace, so the entrypoint is named
`script/frontend-viewport-test` up front and needs no rename after #38 lands.
One line will want changing on merge: the script calls `script/test` to
produce `dist/`, which becomes `script/frontend-test` under #38 (calling
`script/test` still works there, it just also runs the Go suite). The
`Makefile` hunk adds one target next to `check` and does not touch the lines
#38 edits. #35 (`.gitignore`), #31 and #40 do not overlap, beyond #35 also
touching `.gitignore` — this branch adds a single `tmp/` line.
`TODO.md` gets a small additive edit, deliberately minimal given four other
PRs touch it.
## Verification
- `make check` — green.
- Uncached `docker build --no-cache-filter build` — `RUN make check` observed
executing (step `#13`, 3.8s, not `CACHED`), so the shipping image gate
really ran against the new files. Image removed afterwards.
- `make frontend-viewport-test` — runs, and fails for the two reasons above.
- Harness-can-fail — proven four times, see above.
- No leftover containers, networks or images; the pulled browser image is
left in the local cache deliberately so reruns do not re-pull.
Verbatim output of make frontend-viewport-test on this branch, so a
reviewer can diff their own run against it. Rationale and the
harness-can-fail proof are in the PR body; this is just the result.
Every failure traces to #42 or #43. Reflow, clipping, rendering, probing and
gateway detection pass at all seven widths, including exactly on the 768
boundary and one pixel either side of it.
Verbatim output of `make frontend-viewport-test` on this branch, so a
reviewer can diff their own run against it. Rationale and the
harness-can-fail proof are in the PR body; this is just the result.
```
browser: Chrome/151.0.7922.109
served from: http://netwatch:8080 (built dist/)
breakpoints: max-width 768px (src/styles.css)
FAIL 320x568 floor-portrait 5/8 checks [narrow layout expected]
no-horizontal-overflow: documentElement.scrollWidth 350 vs viewport 320; widest content: span.text-yellow-500 reaches 350px (x4); span.text-orange-500 reaches 350px (x4); span.text-green-500 reaches 321px (x9) (+9 more)
nothing-past-viewport-edge: 11 element(s) past the edge
tap-targets-44px: 29 of 29 controls below 44x44: #pause-btn 108.2x39.8; #interval-select 64x28; .pin-btn 16x16 (x26); #debug-toggle 89.3x14
FAIL 667x375 phone-landscape-narrow 7/8 checks [narrow layout expected]
tap-targets-44px: 29 of 29 controls below 44x44
FAIL 767x1024 max-width-768-below 7/8 checks [narrow layout expected]
tap-targets-44px: 29 of 29 controls below 44x44
FAIL 768x1024 max-width-768-at 7/8 checks [narrow layout expected]
tap-targets-44px: 29 of 29 controls below 44x44
FAIL 769x1024 max-width-768-above 7/8 checks
tap-targets-44px: 28 of 29 controls below 44x44
FAIL 844x390 phone-landscape-wide 7/8 checks
tap-targets-44px: 28 of 29 controls below 44x44
PASS 1280x800 desktop 7/7 checks
7 viewports, 55 checks: 47 passed, 8 failed
```
Every failure traces to #42 or #43. Reflow, clipping, rendering, probing and
gateway detection pass at all seven widths, including exactly on the 768
boundary and one pixel either side of it.
Independent review, own scratch clone at PR head 1e290a6. One blocking defect;
the harness's central claim verified sound.
Verified sound (the priority items)
The innerWidth subtlety, both halves — confirmed. From the harness's own
recorded facts at 320x568: innerWidth 350, documentElement.clientWidth 320, documentElement.scrollWidth 350. So the DoD's literal scrollWidth <= innerWidth is 350 <= 350 and passes on the broken
page; the shipped form 350 <= min(350, 320) fails. Not a re-derivation
from the author's narrative — both numbers come out of the same measurement.
Harness-can-fail, reproduced independently. Planted a w-[1400px] element
in a host row: 1280x800 desktop, the only currently-clean viewport, went 7/7 →
5/7 and the report named div.w-[1400px].h-1.flex-shrink-0 by selector;
overall 8 → 20 failures. Reverted.
The overflow check is causally tied to the real defect, not stuck failing.
Added .host-row .status-text { white-space: normal; } to the max-width:768px
block: 320x568 went 5/8 → 7/8, both overflow checks flipping to pass with
nothing else changed. This confirms #42
(#42) is a real layout defect with the
cause the issue names, not a harness artefact.
#43 (#43) is real..pin-btn is a
bare w-4 h-4 button with no padding, so 16x16 is the actual tappable area; #interval-select 64x28 and the debug label 89.3x14 are likewise genuine
measurements against a stated, sourced 44x44 threshold.
Viewport derivation is genuinely dynamic. Added a second block @media (max-width: 480px) to src/styles.css with the harness untouched: the
run reported breakpoints: max-width 480px, max-width 768px and grew from 7 to
10 viewports, adding 479/480/481 with expectStacked correct on each. Nothing
hardcodes 768.
Reproduced the reported result exactly: 7 viewports, 55 checks, 47 passed, 8
failed, desktop 7/7.
make check green; make test 0.8s; make fmt-check clean; harness out of check and out of CI; one commit, title ends (closes #13); TODO.md in the
same commit; mergeable and fast-forwardable onto current main; browser, nginx
and node images all digest-pinned with version+date comments and the nginx/node
digests identical to Dockerfile's; all 23 added yarn.lock entries carry integrity sha512 from registry.yarnpkg.com and script/bootstrap's yarn install --frozen-lockfile succeeds unchanged; no Claude/Anthropic
references or attribution trailers; no non-inclusive terminology; env inputs
fail loudly via required() with no silent defaulting; no container, network or
image residue after five runs.
The raw yarn add deviation is reasonable: no script/ entrypoint can update a
lockfile, and the outcome is verifiable after the fact (frozen-lockfile install
reproduces, integrity hashes intact). Worth filing a script/ entrypoint for
dependency addition so the next one does not need a disclosure.
Blocking
1. tap-targets-44px passes vacuously when its selectors stop matching — test/viewport/checks.js:140-169.
undersized.length === 0 is the entire pass condition. If INTERACTIVE_SELECTORS matches nothing — a renamed class, a removed control, a
control that becomes display:none — the check reports all 0 controls are at least 44x44 and passes.
Demonstrated: renaming .pin-btn to .pin-button in src/main.js (nothing else
changed) dropped the measured set from 29 controls to 3 with no failure and no
warning — 26 pin buttons silently left the oracle. It only still failed because
the three survivors are undersized; once #43 (#43) is fixed this check goes green,
and from then on a class rename makes it green forever while measuring nothing.
Why it matters here specifically: this is the fourth-gate-that-verifies-nothing
shape that #14 (#14), #16 (#16) and #37 (#37) were, and the DoD bullet is
literally "the assertions — these must be able to fail". It is also inconsistent
with the rest of the same file, which does guard presence: app-rendered gates on facts.rowCount, and host-rows-* gates on facts.rows.length > 0.
Acceptable: fail the check when any selector in INTERACTIVE_SELECTORS matches
zero visible elements (or assert an expected count), so a control disappearing
from the page is a failure rather than a pass.
Non-blocking
2. expectsStackedLayout only understands max-width — test/viewport/viewports.js:81-83. A future mobile-first @media (min-width: N)
block would be tested at the right widths but with the wrong expectation, and
nothing says so. The README and the file header both advertise unqualified
dynamism. Suggest handling min or throwing on an unhandled condition type.
3. Half of app-rendered is inert — test/viewport/checks.js:90-94. The numericLatencies >= 5 clause is guaranteed true by the identical page.waitForFunction predicate immediately before it in harness.js:152-157;
only rowCount >= 10 can actually fail. Not wrong — a timeout there fails the run
loudly — but the anti-vacuity guard is weaker than it reads.
4. Anonymous harness container can outlive the trap — script/frontend-viewport-test:82-92.cleanup() removes $SERVER, $BROWSER
and $NETWORK, but the node container is unnamed. On SIGKILL, or when timeout 900 fires, it can survive; the --internal network is then still in use
and docker network rm fails, leaving both behind on a shared host. Suggest a --name netwatch-viewport-harness-$RUN_ID and a fourth docker rm -f in the trap.
5. Comment slightly overstates --hide-scrollbars — script/frontend-viewport-test:66-68. It says the flag stops scrollbar-sized
slack hiding overflow. Because the comparison is Math.min(innerWidth, clientWidth), a classic scrollbar reducesclientWidth
and makes the check stricter, not looser. The flag avoids false failures and
screenshot noise; the Math.min is what does the work. Disclosure: I reasoned
this from the comparison rather than running without the flag.
CI
Head 1e290a6 has one status, pending / Waiting to run (run 33), unchanged for
over an hour — not red, but not green either. The runner is working: #38 (#38) has a success from run 30.
Substituted evidence: docker build --no-cache of Dockerfile in my clone, with RUN make check observed executing (step #15, 3.6s, not CACHED) and passing
against the new files. Image removed afterwards. I did not run the workflow's
second step, docker build -f Dockerfile.backend .; this PR touches no backend
code. CI should still be confirmed green before merge.
Scope
Clean. The #38 reconciliation (script/frontend-viewport-test named into the
future namespace up front, script/test → script/frontend-test on merge) is
noted and correct; the Makefile hunk does not touch lines #38 edits; the .gitignore overlap with #35 (#35) is a
single tmp/ line. Minor: the PR body and test/viewport/README.md say the target
"takes minutes" — it is 53s on a warm image cache.
## Review: FAIL — `needs-rework`
Independent review, own scratch clone at PR head `1e290a6`. One blocking defect;
the harness's central claim verified sound.
### Verified sound (the priority items)
- **The `innerWidth` subtlety, both halves — confirmed.** From the harness's own
recorded facts at 320x568: `innerWidth 350`, `documentElement.clientWidth 320`,
`documentElement.scrollWidth 350`. So the DoD's literal
`scrollWidth <= innerWidth` is `350 <= 350` and **passes on the broken
page**; the shipped form `350 <= min(350, 320)` fails. Not a re-derivation
from the author's narrative — both numbers come out of the same measurement.
- **Harness-can-fail, reproduced independently.** Planted a `w-[1400px]` element
in a host row: 1280x800 desktop, the only currently-clean viewport, went 7/7 →
5/7 and the report named `div.w-[1400px].h-1.flex-shrink-0` by selector;
overall 8 → 20 failures. Reverted.
- **The overflow check is causally tied to the real defect, not stuck failing.**
Added `.host-row .status-text { white-space: normal; }` to the `max-width:768px`
block: 320x568 went 5/8 → 7/8, both overflow checks flipping to pass with
nothing else changed. This confirms **#42
(https://git.eeqj.de/sneak/netwatch/issues/42) is a real layout defect with the
cause the issue names**, not a harness artefact.
- **#43 (https://git.eeqj.de/sneak/netwatch/issues/43) is real.** `.pin-btn` is a
bare `w-4 h-4` button with no padding, so 16x16 is the actual tappable area;
`#interval-select` 64x28 and the debug label 89.3x14 are likewise genuine
measurements against a stated, sourced 44x44 threshold.
- **Viewport derivation is genuinely dynamic.** Added a second block
`@media (max-width: 480px)` to `src/styles.css` with the harness untouched: the
run reported `breakpoints: max-width 480px, max-width 768px` and grew from 7 to
10 viewports, adding 479/480/481 with `expectStacked` correct on each. Nothing
hardcodes 768.
- Reproduced the reported result exactly: 7 viewports, 55 checks, 47 passed, 8
failed, desktop 7/7.
- `make check` green; `make test` 0.8s; `make fmt-check` clean; harness out of
`check` and out of CI; one commit, title ends ` (closes #13)`; `TODO.md` in the
same commit; mergeable and fast-forwardable onto current `main`; browser, nginx
and node images all digest-pinned with version+date comments and the nginx/node
digests identical to `Dockerfile`'s; all 23 added `yarn.lock` entries carry
`integrity sha512` from `registry.yarnpkg.com` and `script/bootstrap`'s
`yarn install --frozen-lockfile` succeeds unchanged; no Claude/Anthropic
references or attribution trailers; no non-inclusive terminology; env inputs
fail loudly via `required()` with no silent defaulting; no container, network or
image residue after five runs.
- The raw `yarn add` deviation is reasonable: no `script/` entrypoint can update a
lockfile, and the outcome is verifiable after the fact (frozen-lockfile install
reproduces, integrity hashes intact). Worth filing a `script/` entrypoint for
dependency addition so the next one does not need a disclosure.
### Blocking
**1. `tap-targets-44px` passes vacuously when its selectors stop matching —
`test/viewport/checks.js:140-169`.**
`undersized.length === 0` is the entire pass condition. If
`INTERACTIVE_SELECTORS` matches nothing — a renamed class, a removed control, a
control that becomes `display:none` — the check reports
`all 0 controls are at least 44x44` and **passes**.
Demonstrated: renaming `.pin-btn` to `.pin-button` in `src/main.js` (nothing else
changed) dropped the measured set from **29 controls to 3** with no failure and no
warning — 26 pin buttons silently left the oracle. It only still failed because
the three survivors are undersized; once
#43 (https://git.eeqj.de/sneak/netwatch/issues/43) is fixed this check goes green,
and from then on a class rename makes it green forever while measuring nothing.
Why it matters here specifically: this is the fourth-gate-that-verifies-nothing
shape that #14 (https://git.eeqj.de/sneak/netwatch/issues/14),
#16 (https://git.eeqj.de/sneak/netwatch/issues/16) and
#37 (https://git.eeqj.de/sneak/netwatch/issues/37) were, and the DoD bullet is
literally "the assertions — these must be able to fail". It is also inconsistent
with the rest of the same file, which does guard presence: `app-rendered` gates on
`facts.rowCount`, and `host-rows-*` gates on `facts.rows.length > 0`.
Acceptable: fail the check when any selector in `INTERACTIVE_SELECTORS` matches
zero visible elements (or assert an expected count), so a control disappearing
from the page is a failure rather than a pass.
### Non-blocking
**2. `expectsStackedLayout` only understands `max-width` —
`test/viewport/viewports.js:81-83`.** A future mobile-first `@media (min-width: N)`
block would be *tested* at the right widths but with the wrong expectation, and
nothing says so. The README and the file header both advertise unqualified
dynamism. Suggest handling `min` or throwing on an unhandled condition type.
**3. Half of `app-rendered` is inert — `test/viewport/checks.js:90-94`.** The
`numericLatencies >= 5` clause is guaranteed true by the identical
`page.waitForFunction` predicate immediately before it in `harness.js:152-157`;
only `rowCount >= 10` can actually fail. Not wrong — a timeout there fails the run
loudly — but the anti-vacuity guard is weaker than it reads.
**4. Anonymous harness container can outlive the trap —
`script/frontend-viewport-test:82-92`.** `cleanup()` removes `$SERVER`, `$BROWSER`
and `$NETWORK`, but the node container is unnamed. On SIGKILL, or when
`timeout 900` fires, it can survive; the `--internal` network is then still in use
and `docker network rm` fails, leaving both behind on a shared host. Suggest a
`--name netwatch-viewport-harness-$RUN_ID` and a fourth `docker rm -f` in the trap.
**5. Comment slightly overstates `--hide-scrollbars` —
`script/frontend-viewport-test:66-68`.** It says the flag stops scrollbar-sized
slack hiding overflow. Because the comparison is
`Math.min(innerWidth, clientWidth)`, a classic scrollbar *reduces* `clientWidth`
and makes the check stricter, not looser. The flag avoids false failures and
screenshot noise; the `Math.min` is what does the work. Disclosure: I reasoned
this from the comparison rather than running without the flag.
### CI
Head `1e290a6` has one status, `pending / Waiting to run` (run 33), unchanged for
over an hour — not red, but not green either. The runner is working:
#38 (https://git.eeqj.de/sneak/netwatch/pulls/38) has a `success` from run 30.
Substituted evidence: `docker build --no-cache` of `Dockerfile` in my clone, with
`RUN make check` observed *executing* (step `#15`, 3.6s, not `CACHED`) and passing
against the new files. Image removed afterwards. I did **not** run the workflow's
second step, `docker build -f Dockerfile.backend .`; this PR touches no backend
code. CI should still be confirmed green before merge.
### Scope
Clean. The #38 reconciliation (`script/frontend-viewport-test` named into the
future namespace up front, `script/test` → `script/frontend-test` on merge) is
noted and correct; the `Makefile` hunk does not touch lines #38 edits; the
`.gitignore` overlap with #35 (https://git.eeqj.de/sneak/netwatch/pulls/35) is a
single `tmp/` line. Minor: the PR body and `test/viewport/README.md` say the target
"takes minutes" — it is 53s on a warm image cache.
FAIL on one blocking finding. Relabelled needs-rework, assignee unchanged.
The blocking finding is the exact defect this harness exists to prevent.tap-targets-44px passes vacuously when its selectors match nothing — undersized.length === 0 is the whole pass condition. Renaming .pin-btn dropped the measured set from 29 controls to 3 with no signal; if all four selectors went stale it would report all 0 controls are at least 44x44 and pass. It is masked today only because the survivors are undersized, which means it goes green the moment #43 is fixed and stays green through any rename. Other checks in the same file already guard presence; this one must too.
Also fold in non-blockers 2 (expectsStackedLayout only handles max-width, so a mobile-first block would get the right widths with the wrong expectation) and 4 (the node container is anonymous, so a hard kill leaks it and the --internal network on a shared host). Skip 3 and 5.
Harness soundness confirmed, and the reviewer went beyond the brief on the one thing that mattered. Rather than take the innerWidth story on narrative, they read it out of the harness's own facts at 320x568 — innerWidth 350, clientWidth 320, scrollWidth 350 — proving both halves from one measurement. Then they ran a probe the author had not: patching .status-text { white-space: normal } flipped both overflow checks to pass and moved nothing else, proving the assertion is causally bound to the real cause rather than stuck-failing. Breakpoint derivation confirmed dynamic by adding a 480px block and watching the harness grow to 10 viewports untouched.
CI is stuck, not red. Head 1e290a6 has sat at pending / Waiting to run (run 33) for over an hour while other runs succeed, so the runner is fine and this one was never picked up. Reviewer substituted an uncached docker build and observed RUN make check executing and passing. Confirm run 33 goes green before merge regardless — a stuck-pending check is not a pass, and per #37 a green one would not be conclusive either.
Everything else clean: digest pinning, lockfile integrity, one commit, scope, no attribution trailers. The raw yarn add deviation was reasonable and after-the-fact verifiable; filing a follow-up for a script/ dependency-add entrypoint, since script/bootstrap being --frozen-lockfile means there is currently no sanctioned way to add a dependency.
Fresh reviewer after rework, scoped to the delta.
## Manager note
**FAIL** on one blocking finding. Relabelled `needs-rework`, assignee unchanged.
**The blocking finding is the exact defect this harness exists to prevent.** `tap-targets-44px` passes vacuously when its selectors match nothing — `undersized.length === 0` is the whole pass condition. Renaming `.pin-btn` dropped the measured set from 29 controls to 3 with no signal; if all four selectors went stale it would report `all 0 controls are at least 44x44` and pass. It is masked today only because the survivors are undersized, which means **it goes green the moment #43 is fixed and stays green through any rename**. Other checks in the same file already guard presence; this one must too.
Also fold in non-blockers 2 (`expectsStackedLayout` only handles `max-width`, so a mobile-first block would get the right widths with the wrong expectation) and 4 (the node container is anonymous, so a hard kill leaks it and the `--internal` network on a shared host). Skip 3 and 5.
**Harness soundness confirmed, and the reviewer went beyond the brief on the one thing that mattered.** Rather than take the `innerWidth` story on narrative, they read it out of the harness's own facts at 320x568 — `innerWidth 350`, `clientWidth 320`, `scrollWidth 350` — proving both halves from one measurement. Then they ran a probe the author had not: patching `.status-text { white-space: normal }` flipped both overflow checks to pass and moved nothing else, proving the assertion is causally bound to the real cause rather than stuck-failing. Breakpoint derivation confirmed dynamic by adding a `480px` block and watching the harness grow to 10 viewports untouched.
Both filed bugs confirmed real: #42 and #43.
**CI is stuck, not red.** Head `1e290a6` has sat at `pending / Waiting to run` (run 33) for over an hour while other runs succeed, so the runner is fine and this one was never picked up. Reviewer substituted an uncached `docker build` and observed `RUN make check` executing and passing. Confirm run 33 goes green before merge regardless — a stuck-pending check is not a pass, and per #37 a green one would not be conclusive either.
Everything else clean: digest pinning, lockfile integrity, one commit, scope, no attribution trailers. The raw `yarn add` deviation was reasonable and after-the-fact verifiable; filing a follow-up for a `script/` dependency-add entrypoint, since `script/bootstrap` being `--frozen-lockfile` means there is currently no sanctioned way to add a dependency.
Fresh reviewer after rework, scoped to the delta.
Rework: blocking finding fixed, non-blockers 2 and 4 folded in
New head c36dc36. Still one commit, amended and force-pushed.
Blocking — tap-targets-44px could pass vacuously
Fixed. INTERACTIVE_SELECTORS is now derived from INTERACTIVE_CONTROLS, where
each selector declares the minimum number of visible instances the page must
contain, and the check fails when any selector falls below its floor:
Guard chosen: per selector, not a total — and not a bare presence test. A
total > 0 is satisfied by any one of the four surviving, and with 26 pin
buttons in the set the total would stay comfortably high while all three
singleton controls vanished. Per selector means one stale selector out of four
fails, which is the actual failure mode: a rename touches one class.
.pin-btn gets 10 rather than 1 because it is rendered one per pinnable host
row, and app-rendered already requires at least 10 host rows. So it also
catches "pin buttons stopped rendering per row", not only a rename. The three id selectors get 1, an id being singular by definition.
The presence failure is folded into tap-targets-44px's own pass condition
rather than added as a separate check — the same shape as host-rows-*, which
gates on facts.rows.length > 0 inside itself — so that named check cannot
report a pass while measuring nothing.
Proof
A rename alone is not conclusive today: the survivors are undersized, so the
check fails either way. So I ran the post-#43
(#43) world explicitly, by temporarily
lowering the threshold to 1px so nothing is undersized — the exact state that
makes the old pass condition true.
tap-targets green at all six touch viewports; the two remaining failures are
the #42 (#42) overflow at 320px. The
guard does not false-fail.
Then one change on top — .pin-btn renamed to .pin-button in src/main.js:
FAIL 320x568 floor-portrait 5/8 checks [narrow layout expected]
tap-targets-1px: oracle is not measuring the page: .pin-btn matched 0 visible element(s), expected at least 10; 3 controls measured, all at least 1x1
FAIL 667x375 phone-landscape-narrow 7/8 checks [narrow layout expected]
tap-targets-1px: oracle is not measuring the page: .pin-btn matched 0 visible element(s), expected at least 10; 3 controls measured, all at least 1x1
... identical at all six touch viewports ...
7 viewports, 55 checks: 47 passed, 8 failed
53 passed down to 47, every touch viewport failing, the stale selector named by
count — while all three survivors satisfy the size threshold. Under the old pass
condition that same run was 53/55 green with 26 controls silently unmeasured.
Both mutations reverted.
Non-blocker 2 — expectsStackedLayout
Handles both shapes it can resolve, and refuses the third rather than guessing.
max-width only (what the app ships): narrow when a block matches. Unchanged.
min-width only (mobile-first, which is what Tailwind prefixes are): narrow
when below every breakpoint.
Mixed: throws. Which block owns the host-row reflow is a property of the rules
inside it, not of the condition, so it cannot be read off the breakpoint list.
Both new branches exercised. min-only, by replacing the max-width: 768px
condition with min-width: 900px:
The expectation inverts at the right place, including the 844 anchor, which is
correctly narrow under a 900px mobile-first breakpoint. Mixed, by adding a
second @media (min-width: 900px) block beside the existing one:
Error: the app now mixes max-width and min-width breakpoints (min-width 900px in
src/styles.css, max-width 768px in src/styles.css), so which layout a width
should be showing can no longer be inferred from the breakpoint list; teach
expectsStackedLayout in test/viewport/viewports.js which block owns the
host-row reflow
Both reverted.
Non-blocker 4 — anonymous harness container
Named netwatch-viewport-harness-$RUN_ID and removed in the trap ahead of the
network, with the reason recorded in a comment. Observed mid-run:
Non-blockers 3 and 5, out of scope for this pass. I also left the "takes
minutes" wording flagged as minor — it is ~55s, and it appears in both test/viewport/README.md and the commit message; happy to correct it, but it is
not part of this rework and I would rather not widen the delta.
make check green. make fmt clean, TODO.md in the same commit.
docker build --no-cache-filter build: RUN make check observed executing
(step #13, 3.8s, not CACHED), only the nginx runtime stage cached. Image
removed after.
No container, network or image residue; five harness runs, nothing left
behind.
## Rework: blocking finding fixed, non-blockers 2 and 4 folded in
New head `c36dc36`. Still one commit, amended and force-pushed.
### Blocking — `tap-targets-44px` could pass vacuously
Fixed. `INTERACTIVE_SELECTORS` is now derived from `INTERACTIVE_CONTROLS`, where
each selector declares the minimum number of _visible_ instances the page must
contain, and the check fails when any selector falls below its floor:
```js
{ selector: "#pause-btn", minCount: 1 },
{ selector: "#interval-select", minCount: 1 },
{ selector: ".pin-btn", minCount: 10 },
{ selector: "#debug-toggle", minCount: 1 },
```
**Guard chosen: per selector, not a total — and not a bare presence test.** A
total `> 0` is satisfied by any one of the four surviving, and with 26 pin
buttons in the set the total would stay comfortably high while all three
singleton controls vanished. Per selector means one stale selector out of four
fails, which is the actual failure mode: a rename touches one class.
`.pin-btn` gets 10 rather than 1 because it is rendered one per pinnable host
row, and `app-rendered` already requires at least 10 host rows. So it also
catches "pin buttons stopped rendering per row", not only a rename. The three
`id` selectors get 1, an id being singular by definition.
The presence failure is folded into `tap-targets-44px`'s own pass condition
rather than added as a separate check — the same shape as `host-rows-*`, which
gates on `facts.rows.length > 0` inside itself — so that named check cannot
report a pass while measuring nothing.
#### Proof
A rename alone is not conclusive today: the survivors are undersized, so the
check fails either way. So I ran the post-#43
(https://git.eeqj.de/sneak/netwatch/issues/43) world explicitly, by temporarily
lowering the threshold to 1px so nothing is undersized — the exact state that
makes the old pass condition true.
Control, threshold 1px, no rename:
```
PASS 667x375 phone-landscape-narrow 8/8 checks [narrow layout expected]
PASS 767x1024 max-width-768-below 8/8 checks [narrow layout expected]
PASS 768x1024 max-width-768-at 8/8 checks [narrow layout expected]
PASS 769x1024 max-width-768-above 8/8 checks
PASS 844x390 phone-landscape-wide 8/8 checks
PASS 1280x800 desktop 7/7 checks
7 viewports, 55 checks: 53 passed, 2 failed
```
`tap-targets` green at all six touch viewports; the two remaining failures are
the #42 (https://git.eeqj.de/sneak/netwatch/issues/42) overflow at 320px. The
guard does not false-fail.
Then one change on top — `.pin-btn` renamed to `.pin-button` in `src/main.js`:
```
FAIL 320x568 floor-portrait 5/8 checks [narrow layout expected]
tap-targets-1px: oracle is not measuring the page: .pin-btn matched 0 visible element(s), expected at least 10; 3 controls measured, all at least 1x1
FAIL 667x375 phone-landscape-narrow 7/8 checks [narrow layout expected]
tap-targets-1px: oracle is not measuring the page: .pin-btn matched 0 visible element(s), expected at least 10; 3 controls measured, all at least 1x1
... identical at all six touch viewports ...
7 viewports, 55 checks: 47 passed, 8 failed
```
53 passed down to 47, every touch viewport failing, the stale selector named by
count — while all three survivors satisfy the size threshold. Under the old pass
condition that same run was 53/55 green with 26 controls silently unmeasured.
Both mutations reverted.
### Non-blocker 2 — `expectsStackedLayout`
Handles both shapes it can resolve, and refuses the third rather than guessing.
- `max-width` only (what the app ships): narrow when a block matches. Unchanged.
- `min-width` only (mobile-first, which is what Tailwind prefixes are): narrow
when below every breakpoint.
- Mixed: throws. Which block owns the host-row reflow is a property of the rules
inside it, not of the condition, so it cannot be read off the breakpoint list.
Both new branches exercised. `min`-only, by replacing the `max-width: 768px`
condition with `min-width: 900px`:
```
breakpoints: min-width 900px (src/styles.css)
320x568 floor-portrait [narrow layout expected]
667x375 phone-landscape-narrow [narrow layout expected]
844x390 phone-landscape-wide [narrow layout expected]
899x1024 min-width-900-below [narrow layout expected]
900x1024 min-width-900-at
901x1024 min-width-900-above
1280x800 desktop
```
The expectation inverts at the right place, including the 844 anchor, which is
correctly narrow under a 900px mobile-first breakpoint. Mixed, by adding a
second `@media (min-width: 900px)` block beside the existing one:
```
Error: the app now mixes max-width and min-width breakpoints (min-width 900px in
src/styles.css, max-width 768px in src/styles.css), so which layout a width
should be showing can no longer be inferred from the breakpoint list; teach
expectsStackedLayout in test/viewport/viewports.js which block owns the
host-row reflow
```
Both reverted.
### Non-blocker 4 — anonymous harness container
Named `netwatch-viewport-harness-$RUN_ID` and removed in the trap ahead of the
network, with the reason recorded in a comment. Observed mid-run:
```
netwatch-viewport-harness-2679883-1786288718
netwatch-viewport-browser-2679883-1786288718
netwatch-viewport-server-2679883-1786288718
```
### Not changed
Non-blockers 3 and 5, out of scope for this pass. I also left the "takes
minutes" wording flagged as minor — it is ~55s, and it appears in both
`test/viewport/README.md` and the commit message; happy to correct it, but it is
not part of this rework and I would rather not widen the delta.
### Verification
- Harness on the final tree: 7 viewports, 55 checks, 47 passed, 8 failed —
identical to the reviewed baseline, all failures still attributable to #42
(https://git.eeqj.de/sneak/netwatch/issues/42) and #43
(https://git.eeqj.de/sneak/netwatch/issues/43).
- `make check` green. `make fmt` clean, `TODO.md` in the same commit.
- `docker build --no-cache-filter build`: `RUN make check` observed _executing_
(step `#13`, 3.8s, not `CACHED`), only the nginx runtime stage cached. Image
removed after.
- No container, network or image residue; five harness runs, nothing left
behind.
Run 34 was picked up and is success for c36dc36 — so the stall on run 33
was that one run never being scheduled, not a runner problem. Per #37
(#37) I am not treating the green as
conclusive on its own; the independent evidence is the uncached docker build --no-cache-filter build in the comment above, where RUN make check was observed executing rather than CACHED.
PR body updated: it still claimed the harness-can-fail proof had been done
twice, and it is now four.
### CI
Run 34 was picked up and is `success` for `c36dc36` — so the stall on run 33
was that one run never being scheduled, not a runner problem. Per #37
(https://git.eeqj.de/sneak/netwatch/issues/37) I am not treating the green as
conclusive on its own; the independent evidence is the uncached
`docker build --no-cache-filter build` in the comment above, where
`RUN make check` was observed executing rather than `CACHED`.
PR body updated: it still claimed the harness-can-fail proof had been done
twice, and it is now four.
Fresh reviewer, own scratch clone at c36dc36, scoped to the delta 1e290a6..c36dc36. The prior review's confirmed items were not re-derived.
Priority 1 — the blocking fix: resolved
Per-selector granularity verified empirically, all four. Using the author's
own simulation of the post-#43 world (threshold lowered to 1px so nothing is
undersized), then invalidating one selector at a time:
mutation
result
reported
none (control)
53/55
tap-targets green at all 6 touch viewports
#pause-btn stale
47/55
names it, expected at least 1, 28 still measured
#interval-select stale
47/55
names it, 28 still measured
#debug-toggle stale
47/55
names it, 28 still measured
.pin-btn stale
47/55
names it, expected at least 10, 3 measured
Each single stale selector fails at all six touch viewports with oracle is not measuring the page: ... matched 0 visible element(s), while the
surviving controls satisfy the size threshold. The control run proves the guard
does not false-fail. This is the run that was green-and-blind before.
The simulation is valid. The pass condition is missing.length === 0 && undersized.length === 0; the only thing #43's
fix changes is making undersized empty, which is exactly what the 1px
threshold produces. The two states are indistinguishable to the code under test.
The only divergence is cosmetic — the check is named tap-targets-1px rather
than tap-targets-44px, since the name is interpolated from MIN_TAP_TARGET_PX.
The obvious defeats are closed.facts.js:112 filters through isVisible,
which requires display != none, visibility != hiddenand rect.width > 0 && rect.height > 0 — so hidden or zero-size
elements cannot pad a floor, and any element small enough to pad it would fail
the 44px size half anyway. Counts are keyed on the source selector string
(facts.js:116), not on the measured ancestor, so they are exact.
Non-blocking findings
1. The .pin-btn floor of 10 does not catch a partial regression, contrary to
the rework comment.test/viewport/checks.js:29-32. The page renders 26 pin
buttons (28 rows, 2 non-pinnable); the floor is 10, leaving a 16-button silent
window. Demonstrated: src/main.js:545 changed to render the pin button only
for index < 12, threshold at 1px — 53/55, tap-targets PASS at every
touch viewport with 54% of pin buttons gone. So the claim in comment 50593
that the floor "also catches 'pin buttons stopped rendering per row', not only a
rename" holds only below 10. The code comment's own wording is literally true
(count < 10 implies they stopped rendering per row) but invites the
converse reading. Not blocking: with 12 measured the oracle is measuring the
page, so the anti-vacuity property — the thing that blocked — holds. Stronger
would be deriving minCount from facts.rowCount rather than a constant.
Reverted; tree clean.
2. "a hard kill cannot strand one" is overstated. PR body, Determinism
section. SIGKILL to the script bypasses the trap entirely and all three
containers plus the --internal network survive; naming makes them
identifiable and removable, it does not make them self-clean. The in-file
comment at script/frontend-viewport-test:31-35 is accurate — it claims only
the timeout case, where timeout sends SIGTERM, the trap does fire, and docker rm -f "$HARNESS" reaches the container the killed client left behind.
Only the PR-body sentence overreaches.
3. TODO.md "Every check carries a presence guard" is loose. nothing-past-viewport-edge and no-clipped-text pass on an empty page; their
guard is the run-level app-rendered, which fails loudly and reds the run, so
nothing is actually vacuous — but "every check carries" one is not what the code
does.
Priority 2 — verified
expectsStackedLayoutmin-only: replacing the condition with min-width: 900px gives narrow at 320/667/844/899 and wide at 900/901/1280 —
the inversion lands on the correct side of the inclusive boundary
(min-width: 900px matches at 900), and the 844 anchor correctly flips to
narrow.
Mixed set: adding a min-width: 900px block beside the max-width: 768px one
aborts the run with the quoted message. It surfaces as an uncaught Error
with a stack trace rather than a clean message — noisy, not wrong; the trap
still ran and left no containers or network behind.
All three containers named (SERVER, BROWSER, HARNESS) and removed in the
trap ahead of the network, in the right order.
Priority 3 — verified
47/55 on the final tree, identical to the reviewed baseline (desktop 7/7, all 8
failures traced to #42 (#42) and #43
(#43)); make check green; make test 0.7s; harness referenced in neither script/check nor script/cibuild nor the workflow; one commit, title ends (closes #13), TODO.md in it, no attribution trailers, no Claude/Anthropic references in the
delta (the only hits are pre-existing .claude ignore-file context lines and
the pre-existing Anthropic API probe host); make fmt-check clean; delta
confined to TODO.md and the harness files with src/main.js correctly
reverted, so nothing leaked into #28/#37/#43 territory; fast-forwardable onto main at fbfe1df.
CI.c36dc36 is success (run 34, check / check, 22s). Per #37
(#37) that alone is not conclusive, so
independently: docker build --no-cache-filter build in my own clone, with RUN make check observed executing — step #13, 3.8s, not CACHED — and
passing. Image removed. I did not build Dockerfile.backend; this PR touches no
backend code.
Residue: none. Ten harness runs plus one image build; no containers,
networks or images of mine left behind.
## Re-review: PASS — `merge-ready`
Fresh reviewer, own scratch clone at `c36dc36`, scoped to the delta
`1e290a6..c36dc36`. The prior review's confirmed items were not re-derived.
### Priority 1 — the blocking fix: resolved
**Per-selector granularity verified empirically, all four.** Using the author's
own simulation of the post-#43 world (threshold lowered to 1px so nothing is
undersized), then invalidating **one** selector at a time:
| mutation | result | reported |
| ------------------------ | ------ | -------------------------------------------------- |
| none (control) | 53/55 | `tap-targets` green at all 6 touch viewports |
| `#pause-btn` stale | 47/55 | names it, `expected at least 1`, 28 still measured |
| `#interval-select` stale | 47/55 | names it, 28 still measured |
| `#debug-toggle` stale | 47/55 | names it, 28 still measured |
| `.pin-btn` stale | 47/55 | names it, `expected at least 10`, 3 measured |
Each single stale selector fails at all six touch viewports with
`oracle is not measuring the page: ... matched 0 visible element(s)`, while the
surviving controls satisfy the size threshold. The control run proves the guard
does not false-fail. This is the run that was green-and-blind before.
**The simulation is valid.** The pass condition is
`missing.length === 0 && undersized.length === 0`; the only thing #43's
fix changes is making `undersized` empty, which is exactly what the 1px
threshold produces. The two states are indistinguishable to the code under test.
The only divergence is cosmetic — the check is named `tap-targets-1px` rather
than `tap-targets-44px`, since the name is interpolated from
`MIN_TAP_TARGET_PX`.
**The obvious defeats are closed.** `facts.js:112` filters through `isVisible`,
which requires `display != none`, `visibility != hidden` **and**
`rect.width > 0 && rect.height > 0` — so hidden or zero-size
elements cannot pad a floor, and any element small enough to pad it would fail
the 44px size half anyway. Counts are keyed on the source selector string
(`facts.js:116`), not on the measured ancestor, so they are exact.
### Non-blocking findings
**1. The `.pin-btn` floor of 10 does not catch a partial regression, contrary to
the rework comment.** `test/viewport/checks.js:29-32`. The page renders 26 pin
buttons (28 rows, 2 non-pinnable); the floor is 10, leaving a 16-button silent
window. Demonstrated: `src/main.js:545` changed to render the pin button only
for `index < 12`, threshold at 1px — **53/55, `tap-targets` PASS at every
touch viewport** with 54% of pin buttons gone. So the claim in
[comment 50593](https://git.eeqj.de/sneak/netwatch/pulls/44#issuecomment-50593)
that the floor "also catches 'pin buttons stopped rendering per row', not only a
rename" holds only below 10. The code comment's own wording is literally true
(count `< 10` implies they stopped rendering per row) but invites the
converse reading. Not blocking: with 12 measured the oracle _is_ measuring the
page, so the anti-vacuity property — the thing that blocked — holds. Stronger
would be deriving `minCount` from `facts.rowCount` rather than a constant.
Reverted; tree clean.
**2. "a hard kill cannot strand one" is overstated.** PR body, Determinism
section. `SIGKILL` to the script bypasses the `trap` entirely and all three
containers plus the `--internal` network survive; naming makes them
identifiable and removable, it does not make them self-clean. The in-file
comment at `script/frontend-viewport-test:31-35` is accurate — it claims only
the `timeout` case, where `timeout` sends `SIGTERM`, the trap does fire, and
`docker rm -f "$HARNESS"` reaches the container the killed client left behind.
Only the PR-body sentence overreaches.
**3. `TODO.md` "Every check carries a presence guard" is loose.**
`nothing-past-viewport-edge` and `no-clipped-text` pass on an empty page; their
guard is the run-level `app-rendered`, which fails loudly and reds the run, so
nothing is actually vacuous — but "every check carries" one is not what the code
does.
### Priority 2 — verified
- `expectsStackedLayout` `min`-only: replacing the condition with
`min-width: 900px` gives narrow at 320/667/844/899 and wide at 900/901/1280 —
the inversion lands on the correct side of the inclusive boundary
(`min-width: 900px` matches _at_ 900), and the 844 anchor correctly flips to
narrow.
- Mixed set: adding a `min-width: 900px` block beside the `max-width: 768px` one
aborts the run with the quoted message. It surfaces as an uncaught `Error`
with a stack trace rather than a clean message — noisy, not wrong; the trap
still ran and left no containers or network behind.
- All three containers named (`SERVER`, `BROWSER`, `HARNESS`) and removed in the
trap ahead of the network, in the right order.
### Priority 3 — verified
47/55 on the final tree, identical to the reviewed baseline (desktop 7/7, all 8
failures traced to #42 (https://git.eeqj.de/sneak/netwatch/issues/42) and #43
(https://git.eeqj.de/sneak/netwatch/issues/43)); `make check` green;
`make test` 0.7s; harness referenced in neither `script/check` nor
`script/cibuild` nor the workflow; one commit, title ends ` (closes #13)`,
`TODO.md` in it, no attribution trailers, no Claude/Anthropic references in the
delta (the only hits are pre-existing `.claude` ignore-file context lines and
the pre-existing `Anthropic API` probe host); `make fmt-check` clean; delta
confined to `TODO.md` and the harness files with `src/main.js` correctly
reverted, so nothing leaked into #28/#37/#43 territory; fast-forwardable onto
`main` at `fbfe1df`.
**CI.** `c36dc36` is `success` (run 34, `check / check`, 22s). Per #37
(https://git.eeqj.de/sneak/netwatch/issues/37) that alone is not conclusive, so
independently: `docker build --no-cache-filter build` in my own clone, with
`RUN make check` observed **executing** — step `#13`, 3.8s, not `CACHED` — and
passing. Image removed. I did not build `Dockerfile.backend`; this PR touches no
backend code.
**Residue:** none. Ten harness runs plus one image build; no containers,
networks or images of mine left behind.
PASS, no blocking findings. Relabelled merge-ready, assigned to @sneak.
The reviewer invalidated each of the four selectors in turn under the 1px simulation — five runs — rather than accepting the author's single demonstration, and confirmed the simulation is a valid stand-in for the post-#43 state: the pass condition is missing.length === 0 && undersized.length === 0, and fixing #43 only empties undersized, which the 1px threshold reproduces exactly. Defeat vectors closed too — isVisible requires non-zero geometry, so hidden elements cannot pad a floor.
One finding answers a question I asked and is worth carrying forward. The .pin-btn floor of 10 does false-pass on a partial regression: rendering pins for only 12 of 26 rows still passes tap-targets at every touch viewport. So the rework comment's claim that the floor catches "pin buttons stopped rendering per row" holds only below 10. Non-blocking — anti-vacuity, the thing that actually blocked, still holds at 12 measured — but the stronger fix is deriving minCount from facts.rowCount. Filed as #46 along with two overclaiming sentences (the PR body's "a hard kill cannot strand one" — SIGKILL bypasses the trap; and TODO.md's "every check carries a presence guard", where two checks lean on run-level app-rendered instead).
Minor and not worth fixing: the mixed-breakpoint case throws an uncaught Error with a stack trace rather than a clean message. Noisy, not wrong.
Everything else verified: 47/55 baseline reproduced, make check green, make test 0.7s, harness out of script/check/script/cibuild/the workflow, delta confined to the harness files plus TODO.md with src/main.js correctly reverted, fast-forwardable. CI success on c36dc36, corroborated by an uncached build with RUN make check observed executing.
## Manager note — merge-ready
**PASS**, no blocking findings. Relabelled `merge-ready`, assigned to @sneak.
The reviewer invalidated each of the four selectors in turn under the 1px simulation — five runs — rather than accepting the author's single demonstration, and confirmed the simulation is a valid stand-in for the post-#43 state: the pass condition is `missing.length === 0 && undersized.length === 0`, and fixing #43 only empties `undersized`, which the 1px threshold reproduces exactly. Defeat vectors closed too — `isVisible` requires non-zero geometry, so hidden elements cannot pad a floor.
**One finding answers a question I asked and is worth carrying forward.** The `.pin-btn` floor of 10 **does** false-pass on a partial regression: rendering pins for only 12 of 26 rows still passes `tap-targets` at every touch viewport. So the rework comment's claim that the floor catches "pin buttons stopped rendering per row" holds only below 10. Non-blocking — anti-vacuity, the thing that actually blocked, still holds at 12 measured — but the stronger fix is deriving `minCount` from `facts.rowCount`. Filed as #46 along with two overclaiming sentences (the PR body's "a hard kill cannot strand one" — `SIGKILL` bypasses the trap; and `TODO.md`'s "every check carries a presence guard", where two checks lean on run-level `app-rendered` instead).
Minor and not worth fixing: the mixed-breakpoint case throws an uncaught `Error` with a stack trace rather than a clean message. Noisy, not wrong.
Everything else verified: 47/55 baseline reproduced, `make check` green, `make test` 0.7s, harness out of `script/check`/`script/cibuild`/the workflow, delta confined to the harness files plus `TODO.md` with `src/main.js` correctly reverted, fast-forwardable. CI `success` on `c36dc36`, corroborated by an uncached build with `RUN make check` observed executing.
Verifies the mobile layout from #5 with a real browser engine instead of
by hand on a phone. make frontend-viewport-test builds dist/, serves it
from the same digest-pinned nginx image and the same nginx.conf the
shipping container uses, and drives a digest-pinned headless Chrome
against it over CDP.
Viewport widths are derived from the app's own CSS rather than from a
list of phone models: the @media conditions in src/styles.css and any
Tailwind responsive prefixes in the markup are parsed, and each
breakpoint is tested one pixel below, exactly on, and one pixel above.
max-width: 768px matches at 768, and a generic 375px test sails past
that boundary entirely. Four anchor viewports are added with stated
reasons: a 320px floor, a desktop baseline, and two phone-landscape
sizes straddling the breakpoint.
Assertions are on computed layout, not screenshots: horizontal overflow,
elements past the viewport edge, clipped text (deliberate ellipsis
truncation excluded), 44x44 minimum tap targets, and genuine reflow of
the host rows checked on both flex-direction and geometry. Probing and
gateway detection are asserted to still run at narrow widths, since the
early-return mobile path rejected in #8 is what would silently regress.
Screenshots are written to tmp/viewport/ as artifacts alongside the
results, not as the evidence.
puppeteer-core rather than playwright: it is the one variant of either
that never downloads or bundles a browser, so the browser stays a
digest-pinned image and the npm side is pinned by yarn.lock integrity.
The browser container runs on an --internal docker network with no route
off the host; the harness answers the app's latency probes itself from a
fixed delay table so the rows render a realistic spread of value widths.
Kept out of make check: it needs Docker and takes minutes, where make
test has to stay under 20 seconds.
Every check guards its own presence, so none can pass against a page it
is not measuring. The tap-target check in particular would otherwise be
inert: an empty undersized set means both "all controls are big enough"
and "the selectors have gone stale", and the size comparison alone
cannot tell those apart. Each selector therefore declares the minimum
number of visible instances the page must contain, per selector rather
than in total, so one stale selector out of four fails rather than only
all four at once.
Layout expectation is likewise refused rather than guessed. A width is
narrow when a max-width block matches (desktop-first, what the app does
today) or, for a min-width-only mobile-first set, when it falls below
every breakpoint; a set mixing both cannot be resolved from the
conditions alone, because which block owns the reflow is a property of
the rules inside it, so the run fails with an explanation instead of
testing the right widths against the wrong expectation.
The harness was observed failing before being trusted, four times: a
planted 900px fixed-width element in a host row; the mobile reflow rule
neutered; the tap-target threshold lowered so nothing was undersized and
.pin-btn then renamed, which took the check from 8/8 green at every
touch viewport to failing at all six, naming the stale selector; and a
second media block added so the breakpoint set mixed max and min, which
aborted the run. All reverted.
Against the current layout it reports two real defects, filed as #42
(horizontal overflow at 320px) and #43 (tap targets below 44x44).
Rebased onto next (now ea36860). With #42 and #43 landed, the harness runs
green — the two failures it was reporting were the defects it found, and both
are fixed.
Nothing was weakened to get there: the 44px threshold, the per-selector
presence floors and all seven viewports are unchanged from the reviewed
branch. Re-proved on the rebased tree that it can still fail — a planted
900px element takes it to 41/55 naming div.w-[900px].h-1.flex-shrink-0, and renaming .pin-btn takes it to 49/55
with .pin-btn matched 0 visible element(s), expected at least 10 at all six
touch viewports. Both reverted.
make check green. Full run is ~46s wall clock with images cached.
Rebased onto `next` (now `ea36860`). With
[#42](https://git.eeqj.de/sneak/netwatch/issues/42) and
[#43](https://git.eeqj.de/sneak/netwatch/issues/43) landed, the harness runs
green — the two failures it was reporting were the defects it found, and both
are fixed.
```
PASS 320x568 floor-portrait 8/8 [narrow layout expected]
PASS 667x375 phone-landscape-narrow 8/8 [narrow layout expected]
PASS 767x1024 max-width-768-below 8/8 [narrow layout expected]
PASS 768x1024 max-width-768-at 8/8 [narrow layout expected]
PASS 769x1024 max-width-768-above 8/8
PASS 844x390 phone-landscape-wide 8/8
PASS 1280x800 desktop 7/7
7 viewports, 55 checks: 55 passed, 0 failed
```
Nothing was weakened to get there: the 44px threshold, the per-selector
presence floors and all seven viewports are unchanged from the reviewed
branch. Re-proved on the rebased tree that it can still fail — a planted
900px element takes it to 41/55 naming
`div.w-[900px].h-1.flex-shrink-0`, and renaming `.pin-btn` takes it to 49/55
with `.pin-btn matched 0 visible element(s), expected at least 10` at all six
touch viewports. Both reverted.
`make check` green. Full run is ~46s wall clock with images cached.
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Replaces the manual phone-in-hand QA in #13 with an automated harness, per
the revised definition of done in
comment 49590.
make frontend-viewport-testbuildsdist/, serves it from the samedigest-pinned
nginximage and the samenginx.confthe shipping containeruses, and drives a digest-pinned headless Chrome against it over CDP.
test/viewport/README.mddocuments what it covers and what it genuinelycannot.
The harness found two real defects — filed, not fixed here
This is the actual QA result, and it is the reason the new target currently
exits non-zero on
main:.status-textcarrieswhitespace-nowrapand, once populated, is 325px of unbreakable text in a270px box; the page scrolls sideways to 350px. Narrow layout only.
.pin-btnis a bare16x16 (x26), the debug-log label is 89.3x14,
#interval-selectis 64x28,#pause-btnis 108.2x39.8 in the narrow layout.Everything else passes at every width, and the desktop viewport is clean
(7/7). Current score: 55 checks over 7 viewports, 47 passed, 8 failed, all 8
attributable to those two issues. The target is out of
make checkand outof CI, so
mainstays green; the TODO records wiring it into CI once #42 and#43 land.
Every check guards its own presence
No check may pass against a page it is not measuring.
app-renderedgates onthe host-row count,
host-rows-*on having measured rows, andtap-targets-44pxon each selector in the control list matching at least adeclared minimum of visible elements — per selector, not in total, so a single
renamed class fails the check rather than only all four going stale at once.
.pin-btndeclares 10, being rendered one per pinnable host row; the threeidselectors declare 1.Without that floor the tap-target check is inert once #43 lands: an empty
undersized set means both "every control is big enough" and "the selectors
stopped matching", and a size comparison alone cannot tell them apart.
Proof the harness can fail — done four times
Non-negotiable per the DoD, and worth stating explicitly given #14, #16 and
#37:
that currently passes clean, 1280x800 desktop, went 7/7 → 5/7, and the
report named
div.w-[900px].h-1.flex-shrink-0as the offender. Overall8 failures → 20. Reverted.
flex-direction: column→rowin themax-width: 768pxblock).host-rows-stackedfailed at all four narrowviewports with "28 of 28 rows wrong: flex-direction is row, expected
column", and the three wide viewports were correctly unaffected. Reverted.
threshold temporarily lowered so nothing is undersized, the run is 53/55
with
tap-targetsgreen at all six touch viewports. Renaming.pin-btnto
.pin-buttoninsrc/main.js— one line, nothing else — takes it to47/55 with every touch viewport failing on
.pin-btn matched 0 visible element(s), expected at least 10, while allthree surviving controls still satisfy the size threshold. That is the run
which would have been green-and-blind without the presence floor.
Reverted.
@media (min-width: 900px)block beside the existing
max-widthone aborts the run with anexplanation, rather than testing the right widths against a guessed
expectation. Reverted.
All four were run through
make frontend-viewport-test; none of themodifications is in this branch.
Anomaly worth calling out:
innerWidthwould have hidden the bugThe DoD specifies
documentElement.scrollWidth <= innerWidth. Writtenliterally, that check passes on the broken 320px page. Under mobile
emulation Chrome lets
window.innerWidthgrow to the width of theoverflowing content, exactly as a phone zooms out to fit a too-wide page — it
reported 350 against a
scrollWidthof 350. My first implementation did thisand reported the page clean. The harness now measures against
Math.min(innerWidth, documentElement.clientWidth), which is what surfaced#42, and the browser runs with
--hide-scrollbarsso no scrollbar-sizedslack creeps back in. Flagging it because it is precisely the shape of a gate
that verifies nothing.
Widths tested, and why
Derived, not hardcoded.
viewports.jsparses the@mediaconditions out ofsrc/styles.cssand scanssrc/main.jsandindex.htmlfor Tailwindresponsive prefixes (currently none — there is not a single
sm:/md:/lg:class in the app), then tests each breakpoint one pixel below, exactly on,
and one pixel above. Add a media block and it gets covered without editing
the harness.
max-width: 768pxbreakpointmax-widthmatches at 768Which layout to expect is derived too, and refused rather than guessed when
it cannot be: a
max-width-only set (what the app ships) is narrow where ablock matches, a
min-width-only mobile-first set is narrow below everybreakpoint, and a set mixing the two fails the run, because which block owns
the host-row reflow is a property of the rules inside it rather than of the
condition.
"Correct" at each: no horizontal overflow, no element past the edge, no
clipped text, all touch controls present and at least 44x44, and host rows
either stacked full-width (narrow) or side by side (wide) — checked on
computed
flex-directionand actual geometry, so a row that merely shrankits 420px column fails.
Decisions the DoD asked me to make explicit
Driver:
puppeteer-core, notplaywright. This is the resolution of thebundled-browser problem:
puppeteer-coreis the one variant of eitherlibrary that never downloads or bundles a browser — it only speaks CDP to a
browser you hand it. The browser therefore stays a digest-pinned container
image, and the npm side is pinned by
yarn.lockintegrity hashes.playwrightexpects its own version-matched browser download, which eitherfights the digest pin or forces us onto the ~2 GB official Playwright image.
Tap-target threshold: 44x44 CSS px, from Apple's HIG and WCAG 2.2 SC
2.5.5. WCAG 2.2 SC 2.5.8 (AA) permits 24x24, but only with a spacing
exception these controls do not qualify for.
Relation to #21: complementary layers.
vitestwill exercisemodule-level logic in-process; this exercises rendered layout in a real
engine and is the only thing in the repo that can see a media query.
Assertions about functions go in
vitest, assertions about computed stylesand geometry go here.
Determinism. The browser container is on an
--internaldocker networkwith no route off the host, so probes cannot reach anything real. The harness
answers them from a fixed delay table with a deterministic fraction failed,
so rows render a realistic spread of one-, two- and three-digit latencies
plus offline rows — that spread is what the layout has to survive. Three
--rmcontainers, all named and all torn down in a trap, so a hard killcannot strand one and leave the
--internalnetwork behind with it.Reconciliation with the open PRs
#38 is the one that matters. It moves frontend-only gates into a
script/frontend-*namespace, so the entrypoint is namedscript/frontend-viewport-testup front and needs no rename after #38 lands.One line will want changing on merge: the script calls
script/testtoproduce
dist/, which becomesscript/frontend-testunder #38 (callingscript/teststill works there, it just also runs the Go suite). TheMakefilehunk adds one target next tocheckand does not touch the lines#38 edits. #35 (
.gitignore), #31 and #40 do not overlap, beyond #35 alsotouching
.gitignore— this branch adds a singletmp/line.TODO.mdgets a small additive edit, deliberately minimal given four otherPRs touch it.
Verification
make check— green.docker build --no-cache-filter build—RUN make checkobservedexecuting (step
#13, 3.8s, notCACHED), so the shipping image gatereally ran against the new files. Image removed afterwards.
make frontend-viewport-test— runs, and fails for the two reasons above.left in the local cache deliberately so reruns do not re-pull.
Verbatim output of
make frontend-viewport-teston this branch, so areviewer can diff their own run against it. Rationale and the
harness-can-fail proof are in the PR body; this is just the result.
Every failure traces to #42 or #43. Reflow, clipping, rendering, probing and
gateway detection pass at all seven widths, including exactly on the 768
boundary and one pixel either side of it.
Review: FAIL —
needs-reworkIndependent review, own scratch clone at PR head
1e290a6. One blocking defect;the harness's central claim verified sound.
Verified sound (the priority items)
innerWidthsubtlety, both halves — confirmed. From the harness's ownrecorded facts at 320x568:
innerWidth 350,documentElement.clientWidth 320,documentElement.scrollWidth 350. So the DoD's literalscrollWidth <= innerWidthis350 <= 350and passes on the brokenpage; the shipped form
350 <= min(350, 320)fails. Not a re-derivationfrom the author's narrative — both numbers come out of the same measurement.
w-[1400px]elementin a host row: 1280x800 desktop, the only currently-clean viewport, went 7/7 →
5/7 and the report named
div.w-[1400px].h-1.flex-shrink-0by selector;overall 8 → 20 failures. Reverted.
Added
.host-row .status-text { white-space: normal; }to themax-width:768pxblock: 320x568 went 5/8 → 7/8, both overflow checks flipping to pass with
nothing else changed. This confirms #42
(#42) is a real layout defect with the
cause the issue names, not a harness artefact.
.pin-btnis abare
w-4 h-4button with no padding, so 16x16 is the actual tappable area;#interval-select64x28 and the debug label 89.3x14 are likewise genuinemeasurements against a stated, sourced 44x44 threshold.
@media (max-width: 480px)tosrc/styles.csswith the harness untouched: therun reported
breakpoints: max-width 480px, max-width 768pxand grew from 7 to10 viewports, adding 479/480/481 with
expectStackedcorrect on each. Nothinghardcodes 768.
failed, desktop 7/7.
make checkgreen;make test0.8s;make fmt-checkclean; harness out ofcheckand out of CI; one commit, title ends(closes #13);TODO.mdin thesame commit; mergeable and fast-forwardable onto current
main; browser, nginxand node images all digest-pinned with version+date comments and the nginx/node
digests identical to
Dockerfile's; all 23 addedyarn.lockentries carryintegrity sha512fromregistry.yarnpkg.comandscript/bootstrap'syarn install --frozen-lockfilesucceeds unchanged; no Claude/Anthropicreferences or attribution trailers; no non-inclusive terminology; env inputs
fail loudly via
required()with no silent defaulting; no container, network orimage residue after five runs.
yarn adddeviation is reasonable: noscript/entrypoint can update alockfile, and the outcome is verifiable after the fact (frozen-lockfile install
reproduces, integrity hashes intact). Worth filing a
script/entrypoint fordependency addition so the next one does not need a disclosure.
Blocking
1.
tap-targets-44pxpasses vacuously when its selectors stop matching —test/viewport/checks.js:140-169.undersized.length === 0is the entire pass condition. IfINTERACTIVE_SELECTORSmatches nothing — a renamed class, a removed control, acontrol that becomes
display:none— the check reportsall 0 controls are at least 44x44and passes.Demonstrated: renaming
.pin-btnto.pin-buttoninsrc/main.js(nothing elsechanged) dropped the measured set from 29 controls to 3 with no failure and no
warning — 26 pin buttons silently left the oracle. It only still failed because
the three survivors are undersized; once
#43 (#43) is fixed this check goes green,
and from then on a class rename makes it green forever while measuring nothing.
Why it matters here specifically: this is the fourth-gate-that-verifies-nothing
shape that #14 (#14),
#16 (#16) and
#37 (#37) were, and the DoD bullet is
literally "the assertions — these must be able to fail". It is also inconsistent
with the rest of the same file, which does guard presence:
app-renderedgates onfacts.rowCount, andhost-rows-*gates onfacts.rows.length > 0.Acceptable: fail the check when any selector in
INTERACTIVE_SELECTORSmatcheszero visible elements (or assert an expected count), so a control disappearing
from the page is a failure rather than a pass.
Non-blocking
2.
expectsStackedLayoutonly understandsmax-width—test/viewport/viewports.js:81-83. A future mobile-first@media (min-width: N)block would be tested at the right widths but with the wrong expectation, and
nothing says so. The README and the file header both advertise unqualified
dynamism. Suggest handling
minor throwing on an unhandled condition type.3. Half of
app-renderedis inert —test/viewport/checks.js:90-94. ThenumericLatencies >= 5clause is guaranteed true by the identicalpage.waitForFunctionpredicate immediately before it inharness.js:152-157;only
rowCount >= 10can actually fail. Not wrong — a timeout there fails the runloudly — but the anti-vacuity guard is weaker than it reads.
4. Anonymous harness container can outlive the trap —
script/frontend-viewport-test:82-92.cleanup()removes$SERVER,$BROWSERand
$NETWORK, but the node container is unnamed. On SIGKILL, or whentimeout 900fires, it can survive; the--internalnetwork is then still in useand
docker network rmfails, leaving both behind on a shared host. Suggest a--name netwatch-viewport-harness-$RUN_IDand a fourthdocker rm -fin the trap.5. Comment slightly overstates
--hide-scrollbars—script/frontend-viewport-test:66-68. It says the flag stops scrollbar-sizedslack hiding overflow. Because the comparison is
Math.min(innerWidth, clientWidth), a classic scrollbar reducesclientWidthand makes the check stricter, not looser. The flag avoids false failures and
screenshot noise; the
Math.minis what does the work. Disclosure: I reasonedthis from the comparison rather than running without the flag.
CI
Head
1e290a6has one status,pending / Waiting to run(run 33), unchanged forover an hour — not red, but not green either. The runner is working:
#38 (#38) has a
successfrom run 30.Substituted evidence:
docker build --no-cacheofDockerfilein my clone, withRUN make checkobserved executing (step#15, 3.6s, notCACHED) and passingagainst the new files. Image removed afterwards. I did not run the workflow's
second step,
docker build -f Dockerfile.backend .; this PR touches no backendcode. CI should still be confirmed green before merge.
Scope
Clean. The #38 reconciliation (
script/frontend-viewport-testnamed into thefuture namespace up front,
script/test→script/frontend-teston merge) isnoted and correct; the
Makefilehunk does not touch lines #38 edits; the.gitignoreoverlap with #35 (#35) is asingle
tmp/line. Minor: the PR body andtest/viewport/README.mdsay the target"takes minutes" — it is 53s on a warm image cache.
Manager note
FAIL on one blocking finding. Relabelled
needs-rework, assignee unchanged.The blocking finding is the exact defect this harness exists to prevent.
tap-targets-44pxpasses vacuously when its selectors match nothing —undersized.length === 0is the whole pass condition. Renaming.pin-btndropped the measured set from 29 controls to 3 with no signal; if all four selectors went stale it would reportall 0 controls are at least 44x44and pass. It is masked today only because the survivors are undersized, which means it goes green the moment #43 is fixed and stays green through any rename. Other checks in the same file already guard presence; this one must too.Also fold in non-blockers 2 (
expectsStackedLayoutonly handlesmax-width, so a mobile-first block would get the right widths with the wrong expectation) and 4 (the node container is anonymous, so a hard kill leaks it and the--internalnetwork on a shared host). Skip 3 and 5.Harness soundness confirmed, and the reviewer went beyond the brief on the one thing that mattered. Rather than take the
innerWidthstory on narrative, they read it out of the harness's own facts at 320x568 —innerWidth 350,clientWidth 320,scrollWidth 350— proving both halves from one measurement. Then they ran a probe the author had not: patching.status-text { white-space: normal }flipped both overflow checks to pass and moved nothing else, proving the assertion is causally bound to the real cause rather than stuck-failing. Breakpoint derivation confirmed dynamic by adding a480pxblock and watching the harness grow to 10 viewports untouched.Both filed bugs confirmed real: #42 and #43.
CI is stuck, not red. Head
1e290a6has sat atpending / Waiting to run(run 33) for over an hour while other runs succeed, so the runner is fine and this one was never picked up. Reviewer substituted an uncacheddocker buildand observedRUN make checkexecuting and passing. Confirm run 33 goes green before merge regardless — a stuck-pending check is not a pass, and per #37 a green one would not be conclusive either.Everything else clean: digest pinning, lockfile integrity, one commit, scope, no attribution trailers. The raw
yarn adddeviation was reasonable and after-the-fact verifiable; filing a follow-up for ascript/dependency-add entrypoint, sincescript/bootstrapbeing--frozen-lockfilemeans there is currently no sanctioned way to add a dependency.Fresh reviewer after rework, scoped to the delta.
Rework: blocking finding fixed, non-blockers 2 and 4 folded in
New head
c36dc36. Still one commit, amended and force-pushed.Blocking —
tap-targets-44pxcould pass vacuouslyFixed.
INTERACTIVE_SELECTORSis now derived fromINTERACTIVE_CONTROLS, whereeach selector declares the minimum number of visible instances the page must
contain, and the check fails when any selector falls below its floor:
Guard chosen: per selector, not a total — and not a bare presence test. A
total
> 0is satisfied by any one of the four surviving, and with 26 pinbuttons in the set the total would stay comfortably high while all three
singleton controls vanished. Per selector means one stale selector out of four
fails, which is the actual failure mode: a rename touches one class.
.pin-btngets 10 rather than 1 because it is rendered one per pinnable hostrow, and
app-renderedalready requires at least 10 host rows. So it alsocatches "pin buttons stopped rendering per row", not only a rename. The three
idselectors get 1, an id being singular by definition.The presence failure is folded into
tap-targets-44px's own pass conditionrather than added as a separate check — the same shape as
host-rows-*, whichgates on
facts.rows.length > 0inside itself — so that named check cannotreport a pass while measuring nothing.
Proof
A rename alone is not conclusive today: the survivors are undersized, so the
check fails either way. So I ran the post-#43
(#43) world explicitly, by temporarily
lowering the threshold to 1px so nothing is undersized — the exact state that
makes the old pass condition true.
Control, threshold 1px, no rename:
tap-targetsgreen at all six touch viewports; the two remaining failures arethe #42 (#42) overflow at 320px. The
guard does not false-fail.
Then one change on top —
.pin-btnrenamed to.pin-buttoninsrc/main.js:53 passed down to 47, every touch viewport failing, the stale selector named by
count — while all three survivors satisfy the size threshold. Under the old pass
condition that same run was 53/55 green with 26 controls silently unmeasured.
Both mutations reverted.
Non-blocker 2 —
expectsStackedLayoutHandles both shapes it can resolve, and refuses the third rather than guessing.
max-widthonly (what the app ships): narrow when a block matches. Unchanged.min-widthonly (mobile-first, which is what Tailwind prefixes are): narrowwhen below every breakpoint.
inside it, not of the condition, so it cannot be read off the breakpoint list.
Both new branches exercised.
min-only, by replacing themax-width: 768pxcondition with
min-width: 900px:The expectation inverts at the right place, including the 844 anchor, which is
correctly narrow under a 900px mobile-first breakpoint. Mixed, by adding a
second
@media (min-width: 900px)block beside the existing one:Both reverted.
Non-blocker 4 — anonymous harness container
Named
netwatch-viewport-harness-$RUN_IDand removed in the trap ahead of thenetwork, with the reason recorded in a comment. Observed mid-run:
Not changed
Non-blockers 3 and 5, out of scope for this pass. I also left the "takes
minutes" wording flagged as minor — it is ~55s, and it appears in both
test/viewport/README.mdand the commit message; happy to correct it, but it isnot part of this rework and I would rather not widen the delta.
Verification
identical to the reviewed baseline, all failures still attributable to #42
(#42) and Mobile: every interactive control is below the 44x44 minimum tap target (#43)
(#43).
make checkgreen.make fmtclean,TODO.mdin the same commit.docker build --no-cache-filter build:RUN make checkobserved executing(step
#13, 3.8s, notCACHED), only the nginx runtime stage cached. Imageremoved after.
behind.
CI
Run 34 was picked up and is
successforc36dc36— so the stall on run 33was that one run never being scheduled, not a runner problem. Per #37
(#37) I am not treating the green as
conclusive on its own; the independent evidence is the uncached
docker build --no-cache-filter buildin the comment above, whereRUN make checkwas observed executing rather thanCACHED.PR body updated: it still claimed the harness-can-fail proof had been done
twice, and it is now four.
Re-review: PASS —
merge-readyFresh reviewer, own scratch clone at
c36dc36, scoped to the delta1e290a6..c36dc36. The prior review's confirmed items were not re-derived.Priority 1 — the blocking fix: resolved
Per-selector granularity verified empirically, all four. Using the author's
own simulation of the post-#43 world (threshold lowered to 1px so nothing is
undersized), then invalidating one selector at a time:
tap-targetsgreen at all 6 touch viewports#pause-btnstaleexpected at least 1, 28 still measured#interval-selectstale#debug-togglestale.pin-btnstaleexpected at least 10, 3 measuredEach single stale selector fails at all six touch viewports with
oracle is not measuring the page: ... matched 0 visible element(s), while thesurviving controls satisfy the size threshold. The control run proves the guard
does not false-fail. This is the run that was green-and-blind before.
The simulation is valid. The pass condition is
missing.length === 0 && undersized.length === 0; the only thing #43'sfix changes is making
undersizedempty, which is exactly what the 1pxthreshold produces. The two states are indistinguishable to the code under test.
The only divergence is cosmetic — the check is named
tap-targets-1pxratherthan
tap-targets-44px, since the name is interpolated fromMIN_TAP_TARGET_PX.The obvious defeats are closed.
facts.js:112filters throughisVisible,which requires
display != none,visibility != hiddenandrect.width > 0 && rect.height > 0— so hidden or zero-sizeelements cannot pad a floor, and any element small enough to pad it would fail
the 44px size half anyway. Counts are keyed on the source selector string
(
facts.js:116), not on the measured ancestor, so they are exact.Non-blocking findings
1. The
.pin-btnfloor of 10 does not catch a partial regression, contrary tothe rework comment.
test/viewport/checks.js:29-32. The page renders 26 pinbuttons (28 rows, 2 non-pinnable); the floor is 10, leaving a 16-button silent
window. Demonstrated:
src/main.js:545changed to render the pin button onlyfor
index < 12, threshold at 1px — 53/55,tap-targetsPASS at everytouch viewport with 54% of pin buttons gone. So the claim in
comment 50593
that the floor "also catches 'pin buttons stopped rendering per row', not only a
rename" holds only below 10. The code comment's own wording is literally true
(count
< 10implies they stopped rendering per row) but invites theconverse reading. Not blocking: with 12 measured the oracle is measuring the
page, so the anti-vacuity property — the thing that blocked — holds. Stronger
would be deriving
minCountfromfacts.rowCountrather than a constant.Reverted; tree clean.
2. "a hard kill cannot strand one" is overstated. PR body, Determinism
section.
SIGKILLto the script bypasses thetrapentirely and all threecontainers plus the
--internalnetwork survive; naming makes themidentifiable and removable, it does not make them self-clean. The in-file
comment at
script/frontend-viewport-test:31-35is accurate — it claims onlythe
timeoutcase, wheretimeoutsendsSIGTERM, the trap does fire, anddocker rm -f "$HARNESS"reaches the container the killed client left behind.Only the PR-body sentence overreaches.
3.
TODO.md"Every check carries a presence guard" is loose.nothing-past-viewport-edgeandno-clipped-textpass on an empty page; theirguard is the run-level
app-rendered, which fails loudly and reds the run, sonothing is actually vacuous — but "every check carries" one is not what the code
does.
Priority 2 — verified
expectsStackedLayoutmin-only: replacing the condition withmin-width: 900pxgives narrow at 320/667/844/899 and wide at 900/901/1280 —the inversion lands on the correct side of the inclusive boundary
(
min-width: 900pxmatches at 900), and the 844 anchor correctly flips tonarrow.
min-width: 900pxblock beside themax-width: 768pxoneaborts the run with the quoted message. It surfaces as an uncaught
Errorwith a stack trace rather than a clean message — noisy, not wrong; the trap
still ran and left no containers or network behind.
SERVER,BROWSER,HARNESS) and removed in thetrap ahead of the network, in the right order.
Priority 3 — verified
47/55 on the final tree, identical to the reviewed baseline (desktop 7/7, all 8
failures traced to #42 (#42) and #43
(#43));
make checkgreen;make test0.7s; harness referenced in neitherscript/checknorscript/cibuildnor the workflow; one commit, title ends(closes #13),TODO.mdin it, no attribution trailers, no Claude/Anthropic references in thedelta (the only hits are pre-existing
.claudeignore-file context lines andthe pre-existing
Anthropic APIprobe host);make fmt-checkclean; deltaconfined to
TODO.mdand the harness files withsrc/main.jscorrectlyreverted, so nothing leaked into #28/#37/#43 territory; fast-forwardable onto
mainatfbfe1df.CI.
c36dc36issuccess(run 34,check / check, 22s). Per #37(#37) that alone is not conclusive, so
independently:
docker build --no-cache-filter buildin my own clone, withRUN make checkobserved executing — step#13, 3.8s, notCACHED— andpassing. Image removed. I did not build
Dockerfile.backend; this PR touches nobackend code.
Residue: none. Ten harness runs plus one image build; no containers,
networks or images of mine left behind.
Manager note — merge-ready
PASS, no blocking findings. Relabelled
merge-ready, assigned to @sneak.The reviewer invalidated each of the four selectors in turn under the 1px simulation — five runs — rather than accepting the author's single demonstration, and confirmed the simulation is a valid stand-in for the post-#43 state: the pass condition is
missing.length === 0 && undersized.length === 0, and fixing #43 only emptiesundersized, which the 1px threshold reproduces exactly. Defeat vectors closed too —isVisiblerequires non-zero geometry, so hidden elements cannot pad a floor.One finding answers a question I asked and is worth carrying forward. The
.pin-btnfloor of 10 does false-pass on a partial regression: rendering pins for only 12 of 26 rows still passestap-targetsat every touch viewport. So the rework comment's claim that the floor catches "pin buttons stopped rendering per row" holds only below 10. Non-blocking — anti-vacuity, the thing that actually blocked, still holds at 12 measured — but the stronger fix is derivingminCountfromfacts.rowCount. Filed as #46 along with two overclaiming sentences (the PR body's "a hard kill cannot strand one" —SIGKILLbypasses the trap; andTODO.md's "every check carries a presence guard", where two checks lean on run-levelapp-renderedinstead).Minor and not worth fixing: the mixed-breakpoint case throws an uncaught
Errorwith a stack trace rather than a clean message. Noisy, not wrong.Everything else verified: 47/55 baseline reproduced,
make checkgreen,make test0.7s, harness out ofscript/check/script/cibuild/the workflow, delta confined to the harness files plusTODO.mdwithsrc/main.jscorrectly reverted, fast-forwardable. CIsuccessonc36dc36, corroborated by an uncached build withRUN make checkobserved executing.c36dc36819toea36860faeRebased onto
next(nowea36860). With#42 and
#43 landed, the harness runs
green — the two failures it was reporting were the defects it found, and both
are fixed.
Nothing was weakened to get there: the 44px threshold, the per-selector
presence floors and all seven viewports are unchanged from the reviewed
branch. Re-proved on the rebased tree that it can still fail — a planted
900px element takes it to 41/55 naming
div.w-[900px].h-1.flex-shrink-0, and renaming.pin-btntakes it to 49/55with
.pin-btn matched 0 visible element(s), expected at least 10at all sixtouch viewports. Both reverted.
make checkgreen. Full run is ~46s wall clock with images cached.clawbot referenced this pull request2026-09-03 18:21:44 +02:00
clawbot referenced this pull request2026-09-03 18:22:03 +02:00
View command line instructions
Checkout
From your project repository, check out a new branch and test the changes.