Repository navigation
test(core): the WebKit row release video proof judges rest on the exit curve - #10011
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
meta-cla
Bot
added
the
CLA Signed
|
AI review status for this pull request.
|
PR Analysis Report🔎 Preview ·
|
| Component | Size | LoC | Visual · A11y · RTL |
|---|---|---|---|
| List | 5.4 kB (0) · 469 B CSS (0) | — | ✓ · ✓ · — |
| ListItem | 58.7 kB (0) · 10.7 kB CSS (0) | — | ✓ · ✓ · ✓ |
Package size: no published package changed; no new dependencies.
API changes: none in this pull request
Committed API report behind this build: cli 9 rows, core 4 rows; run pnpm api:report.
Touched components checked; add ci:run-visual, ci:run-a11y or ci:run-rtl to run the full suites.
Generated by PR Enrichment workflow | View full report
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
Changes requested. The new rest threshold matches the exit curve, but the changed WebKit case has not run anywhere yet.
Blocking
- The description says this makes
maingreen and that CI's WebKit interaction job proves it. Butpr-interaction-webkitnever selectspackages/core/src/List/**: its scope regex and plan list in.github/workflows/ci.ymlleave it out, both legs printed "No WebKit interaction proofs for this change", and Chromium skips the case. Please run it in Linux WebKit at this head, for example withinternal/interaction-harness/scripts/webkit-container.mjsand--repeat-each=10, and put the pass count in the description, or say that no CI lane runs it.
Worth a look (non-blocking)
pressedis the minimum over every frame sincenewPage(), Storybook boot included, andnearRestnow scales with it. Taking it from the hold's frames keeps a dark frame during load from failing line 702.- Over 400 ms, the first two recorded frames move the row by about 0.4 and 1.2 levels. The strict "lighter on every distinct frame" check therefore needs a noise-free decode there, and the
distinctarrays from your WebKit run will show whether it gets one.
[Automated review]
| // Judge the release up to the frame it reaches the row's resting color. | ||
| const before = samples.filter(s => s.beacon === 'before').map(s => s.value); | ||
| const rest = Math.max(...before); | ||
| const pressed = Math.min(...before); |
There was a problem hiding this comment.
Non-blocking: before holds every frame recorded since newPage() (line 539), Storybook boot included, so pressed is the darkest probe value of the whole load rather than of the hold. nearRest now scales with it, so a darker frame during load lowers the threshold. A black frame would put it near 220, under the first blocked frame (about 235), and line 702 would fail. Taking pressed from the hold (for example the last few before frames before the lift) removes that dependency.
| expect(blocked[0].value, values).toBeLessThan(nearRest); | ||
| expect(restIndex, values).toBeGreaterThan(0); | ||
| for (let i = 1; i < distinct.length; i++) { | ||
| expect(distinct[i], values).toBeGreaterThan(distinct[i - 1]); |
There was a problem hiding this comment.
Non-blocking: over 400 ms on cubic-bezier(0.3, 0, 0.6, 0.6), the first two 40 ms frames move the row 2.1% and 5.8% of the press contrast, about 0.4 and 1.2 levels on the white Storybook surface. This strict check over those frames needs a decode with no ±1 noise. The distinct arrays from a WebKit run will show whether it holds.
170e30f to
bb770b3
Compare
There was a problem hiding this comment.
Changes requested. The new threshold fixes most of the flake: in Linux WebKit the case passed 29 of 30 runs at this head, against 1 of 10 for the version on main.
Blocking
- Miss: the 50 ms end margin in
nearRest(lines 663-666) is narrower than the video's frame gaps, so one run failed at line 708 when its last release frame read 250, about 68 ms before the end, against a threshold of about 250.4; allowing two recording frames (for examplestrengthAt(exit, exit.ms - 80), about 248) clears every run observed here, and a release held on the main thread still fails.
In 6 of the 20 passing runs of the second batch the qualifying frame also cleared the threshold by less than one level, so a 10-run check can pass with the flake still present. Minor: the description says the 400 ms exit follows "the Button and Switch blocked-thread proofs", but only the Switch proof sets one.
[Automated review]
| const nearRest = | ||
| rest - | ||
| (rest - pressed) * strengthAt(exit, exit.ms - RECORDING_FRAME_MS) - |
There was a problem hiding this comment.
Miss: one recording frame of margin is not enough. In Linux WebKit at this head, 1 of 30 runs failed at line 708: its blocked frames read 230, 230, 234, 237, 240, 243, 244, 248, 250 and then the hand-back to 228, so the last release frame landed about 68 ms before the end, 0.4 levels under this threshold (about 250.4). At least six passing runs cleared it with a qualifying frame of only 251. Allowing two frames, for example strengthAt(exit, exit.ms - 80) (about 248), clears every run observed, and a release held on the main thread (about 229) still fails.
bb770b3 to
3f51978
Compare
There was a problem hiding this comment.
Approved pending CI: the heavy suites (build, test, docsite-test) were deferred on this push and run on the pre-merge re-run.
The 80 ms rest margin fixes the flake: in Linux WebKit the case passed 30 of 30 runs at this head, against 0 of 10 for the version on main, and a release held on the main thread still cannot pass it. No CI lane selects this case yet, so those runs are its only evidence.
- Non-blocking: the commit message still says the case judges rest "one recording frame before the end", but the code allows two (80 ms), and the squash would carry that sentence onto
main.
[Automated review]
cixzhang
added
the
ci:run
…t curve After #9890 the row's touch release plays the theme's exit tokens, whose curve leaves fastest at its end. The video's 40 ms frames rarely land in the last few milliseconds before Linux WebKit's end-of-animation hand-back, so the "within 1 of rest" check failed most runs. The case now sets a 400 ms exit on the row, reads the clock and curve from the page, and judges rest as the curve's value one recording frame before the end.
bc00bdd to
9a24488
Compare
There was a problem hiding this comment.
Approved pending CI: this pull request is a draft, so the heavy suites and the WebKit interaction lane have not run on this head yet; they run when it is marked ready.
The 80 ms rest margin fixes the flake: in Linux WebKit the case passed 20 of 20 runs at this head, against 1 of 10 for the version on main, and a release held on the main thread still fails it at the restIndex check.
- Non-blocking: the commit message still says the case judges rest "one recording frame before the end", but the code allows two (80 ms); reword it if the squash keeps that message.
[Automated review]
There was a problem hiding this comment.
Changes requested: the new rest threshold works, but the changed case cannot pass in this pull request's own WebKit CI lane, so the required test check fails.
- Found by CI:
pr-interaction-webkit (2/2)fails this case atListRowPress.interaction.browser.spec.ts:596with "Executable doesn't exist" for Chromium's headless shell, because the case launches Chromium to decode its video while the WebKit lane installs only WebKit (.github/workflows/ci.yml:2750); install Chromium in that lane (for examplenpx playwright install --with-deps webkit chromium), here or in a pull request that lands first, or decode the recording without a second browser.
Locally in Linux WebKit the case passed 20 of 20 runs at this head, against 1 of 10 for the version on main, and a release held on the main thread still fails it at the restIndex check.
- Non-blocking: the commit message still says the case judges rest "one recording frame before the end", but the code allows two (80 ms).
[Automated review]
There was a problem hiding this comment.
Approved. The required test check is still red at this head, from a CheckboxList unit test (packages/core/src/CheckboxList/CheckboxList.test.tsx:1700) that fails on main's tip too and that this change cannot reach, so main needs that fix before this merges.
The WebKit video proof for a List row's release no longer depends on where the recording's frames land: it plays the release over 400 ms and counts it at rest at the exit curve's value 80 ms before the end. With main merged in, the WebKit lane installs Chromium's headless shell, so the case now runs in this pull request's own pr-interaction-webkit run and passes there, while a release held on the main thread still fails it.
- Non-blocking: the commit message still says the case judges rest "one recording frame before the end", but the code allows two (80 ms); reword it if the squash keeps that message.
[Automated review]
There was a problem hiding this comment.
Approved.
The WebKit video proof for a List row's release no longer depends on where the recording's frames land. It plays the release over 400 ms and counts it at rest at the exit curve's value 80 ms before the end, and it passed in this head's own pr-interaction-webkit run, while a release held on the main thread still cannot pass it.
- Non-blocking: the description says this makes
maingreen and that the flake turns unrelated pull requests red, but CI runs this case only for a pull request that changes this spec, so "fails about half of Linux WebKit runs ofmain's version" would match the evidence. - Non-blocking: the commit message says rest is judged "one recording frame before the end", but
REST_MARGIN_MSis 80 ms, two frames; reword it if the squash keeps that message.
[Automated review]
There was a problem hiding this comment.
Approved.
The WebKit video proof for a List row's release no longer depends on where the recording's frames land: it plays the release over 400 ms and counts it at rest at the exit curve's value 80 ms before the end. It passed in this pull request's own pr-interaction-webkit run, and a release held on the main thread still cannot pass it.
- Non-blocking: the description says this makes
maingreen and that the flake turns unrelated pull requests red, but CI runs this case only for a pull request that changes this spec, so "fails about half of Linux WebKit runs ofmain's version" would match the evidence. - Non-blocking: the commit message says rest is judged "one recording frame before the end", but
REST_MARGIN_MSis 80 ms, two frames; reword it if the squash keeps that message.
[Automated review]
User impact
None for people using Astryx: this changes one test. It makes
maingreen again. Since #9890, the WebKit busy-main-thread video proof for a list row fails most runs onmain, which turns unrelated pull requests red.Problem and solution fit
exitOf, as its sibling cases do).Verification
promise-function-async, line 261) is onmainalready.main; the test file is byte-identical here): 20 of 20 pass;main: 9 of 20.pr-interaction-webkitrun covers the case.pressed,nearRestand the exit timing beside the frames.Scope