test(desktop): settle the prompt rail across two consecutive reads - #4685
test(desktop): settle the prompt rail across two consecutive reads#4685Adarsh-Me wants to merge 3 commits into
Conversation
hqhq1025
left a comment
hqhq1025
left a comment
There was a problem hiding this comment.
Reviewed exact head d46eee4ca8aaa7e2dddb76acd8145325432d2ebc. This change replaces the post-poll verification with two immediately consecutive snapshots in the prompt-rail E2E helper. I found one P2: those reads do not establish cross-frame stability, and the targeted test still fails under repeated CI-like concurrency. I inspected the complete diff, the production prompt-rail scheduling path, the reported historical failures, and existing review threads. Desktop build/typecheck, the full prompt-rail spec once, Biome, ASF headers, and diff checks passed; the targeted 4-worker stress run failed 2/40. Current main equals the PR base (b0255edcb35588b2f24efd386447d9a494c1e395), so there is no untested main delta. GitHub currently reports only the label check; no hosted test result is available. I could not validate the macOS-only overlay-scrollbar coverage from this Linux host.
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
| && lastSnapshot.currentIds[0] === lastSnapshot.expectedId; | ||
| }, { message: 'the one current tick maps from the Turn being read' }).toBe(true); | ||
| const first = await activePromptRailSnapshot(page); | ||
| const second = await activePromptRailSnapshot(page); |
There was a problem hiding this comment.
[P2] Require stability across a rendered frame, not two immediate protocol reads
These two page.evaluate snapshots do not establish a settle boundary. On this exact head, a frame counter in the real Electron fixture showed that 91 of 100 snapshot pairs ran in the same animation frame, and a queued requestAnimationFrame mutation could run only after both snapshots had returned the matching turn-prompt-rail-120 state. That is the same scheduling mechanism used by the production rail resolver in packages/ui/src/prompt-anchor-rail.tsx:580-614.
The practical failure remains reproducible: --workers=4 --repeat-each=40 failed this test 2/40 times at the subsequent last-tick assertion because the final tick no longer had aria-current. The helper can therefore accept a mid-settle state, and this PR has not stabilized the target test. Please require agreement across an actual painted-frame/quiescence window, then rerun the concurrent stress case.
There was a problem hiding this comment.
Addressed on add2b7c. The two reads are now separated by waitForPaintedFrames(page, 6) — the same re-resolve window the production rail uses after a mutation (membershipFramesLeft = 6 in packages/ui/src/prompt-anchor-rail.tsx), so agreement now spans a real painted-frame boundary instead of two same-frame protocol reads. The poll timeout is raised to 15s to leave room for the six-frame windows across retries. The post-poll re-read stays deleted, so nothing is read after the poll that the poll did not see.
One honest limitation: I could not run the --workers=4 --repeat-each stress case locally — the Desktop E2E suite needs Linux/Xvfb Electron and this dev machine is Windows. The concurrent stress run on this PR's CI job is the confirmation; if it still trips, the next suspect is the poll interval vs. frame cadence interaction.
There was a problem hiding this comment.
Addressed on d4c6677, and the diagnosis was the key input: in every stress failure the helper had already returned, which means the state that flipped was stable — no fixed frame count between two reads can certify it. The mechanism behind the flip: the transcript keeps remeasuring turn content after scrollTranscriptTo('bottom'), scrollHeight grows while scrollTop stays put, the snapshot's atEnd branch (scrollHeight - scrollTop - clientHeight <= 2) turns false, sourceTurn falls back to the reading-band turn, and the rail moves current off the last prompt for good — exactly the aria-current-never-returns failure you saw at line 394.
The helper is now a single in-page settle loop instead of protocol-side reads: every painted frame it re-reads the tick mapping together with scrollTop/scrollHeight/clientHeight (the inputs whose drift drives the resolver), and it resolves only when that full state is byte-identical for six consecutive frames while one current tick maps from the Turn being read. That is the observable quiet state you asked for: every input the rail's resolver reacts to — scroll, mutation, geometry — is exactly what produced the asserted state, so reads after the helper see the same rail. The loop hands the verified state back and the helper asserts it; nothing is read after settle. In-page timeout is 10s, inside the test's 60s budget.
Same limitation as before: no local Linux/Xvfb here, so please rerun the --workers=4 --repeat-each=40 case against d4c6677 — if the flip still happens, the next place to look would be a resolver input the state key does not cover.
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Reviewed current head d46eee4ca8aaa7e2dddb76acd8145325432d2ebc (OPEN, MERGEABLE). This change does not fix the flakiness it targets — NO-GO, one P2-grade test-conclusion problem; no product code touched.
P2 — the two snapshots do not cross a paint frame, so the settle is not real
The change replaces "one snapshot inside the poll plus a re-read after the poll" with "two consecutive snapshots inside the poll" and deletes the post-poll re-read. But the two page.evaluate calls do not cross a paint frame: in a real Electron probe, 91 out of 100 consecutive snapshot pairs land in the same frame, and an already-queued requestAnimationFrame update can land only after both reads have matched. The production prompt rail itself updates via rAF (packages/ui/src/prompt-anchor-rail.tsx:580-614). More directly, this head still fails 2 out of 40 runs under --workers=4 --repeat-each=40, losing aria-current on the last tick after scrolling to the bottom.
So the change establishes no cross-frame stability condition and does not remove the target test's instability. Suggested fix: make agreement span an actually painted frame or an explicit quiet window, then re-run the same 4-worker stress topology.
What was checked on this head
Clean install and desktop build:with-deps, typecheck, full prompt-rail spec single round 11/11, Biome, ASF headers, git diff --check — all pass. Current main and the PR base are the same commit, so there is no extra main delta. What I could not judge: the macOS overlay-scrollbar-only path noted in the file cannot be covered from Linux; hosted tests have not reported on this head yet.
Automated review notice: This comment was posted by an automated review agent operated by Astro-Han. It is not an independent human review and does not replace one.
简体中文
本条结论全部来自 @Haoqing_Reviewer 的审查。我自己没有读这份 diff；我核的是当前 head 有没有漂移、以及 exact-head 的 CI 状态。当前 head 是 d46eee4，可合并，线上只有 label 检查。这次改动没建立跨帧稳定条件，目标测试在 4 worker 下仍失败，修好再合。
9196604 to
add2b7c
Compare
) `expectPromptRailMatchesReadingPosition` polled until one snapshot agreed with the reading position, then took a fresh snapshot a round trip later and asserted it again. After `scrollTranscriptTo(page, 'bottom')` the transcript can still be settling, so the poll could pass on a frame where rail and reading position agreed and the second read could then see the rail one tick further — the exact failure CI reported at prompt-rail.spec.ts:206 (`turn-prompt-rail-119` vs `turn-prompt-rail-120`). The second read cannot fail on a settled page, so it only added a way to fail on an unsettled one. Requiring two agreeing snapshots is not enough on its own: both protocol reads can execute inside one rendered frame, so an update the rail queued on requestAnimationFrame lands only after they agreed. The rail resolves on the frame after a scroll and keeps re-resolving for six frames after a mutation (packages/ui/src/prompt-anchor-rail.tsx), so the poll now requires two snapshots agreeing across that whole painted-frame window, and the post-poll re-read is gone: nothing is read after the poll that the poll did not see. Closes apache#4675
add2b7c to
009fd2c
Compare
|
This PR is complete and ready for review — could a maintainer add the |
hqhq1025
left a comment
hqhq1025
left a comment
There was a problem hiding this comment.
Reviewed exact head 009fd2c13f9c42fb25c9b5902a7ecd2edfdfc716. The prior P2 remains on the new implementation at apps/desktop/e2e/prompt-rail.spec.ts:208-219.
The new six-frame delay does make the two snapshots cross painted frames, but it still does not establish that the transcript and rail are quiescent. Under the same CI-like command used for the previous head (--workers=4 --repeat-each=40), this exact head failed 4 of 40 runs. In every failure, expectPromptRailMatchesReadingPosition had already returned; the following assertion at line 394 then waited the full 10 seconds without the final tick regaining aria-current. The target instability therefore remains reproducible, so mirroring the production six-frame retry count is not a sufficient settle condition. Please investigate the state change that occurs after the helper returns, or gate on an observable quiet/stable state rather than another fixed frame count.
The complete prompt-rail spec passed once (11/11). Clean install, Desktop build:with-deps, Desktop typecheck, Biome, ASF headers, and git diff --check also passed. Current main equals the PR base (b0255edcb35588b2f24efd386447d9a494c1e395), so there is no additional main delta. GitHub currently reports no hosted checks on this head. I could not validate the macOS-only overlay-scrollbar path from this Linux host.
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
…he#4675) The two-consecutive-reads poll still flaked 4/40 under 4-worker stress: it proved agreement at a moment, not quiescence, and the rail state changed after the helper returned — the test then failed at the last-tick aria-current assertion because the transcript kept remeasuring after the scroll, flipped the snapshot's atEnd branch, and moved the current tick off the last prompt for good. No fixed frame count can rule that out. The settle loop now runs in the page: each painted frame re-reads the tick mapping together with the scroll metrics that feed it, and the helper returns only once that full state is unchanged for six consecutive frames while one current tick maps from the Turn being read. Every input the rail's resolver reacts to (scroll, mutation, geometry) is then exactly what produced the asserted state, so later reads see the same rail. The helper asserts the state the loop verified and reads nothing after it. Closes apache#4675
hqhq1025
left a comment
hqhq1025
left a comment
There was a problem hiding this comment.
Reviewed exact head d4c66770150c6e7658a61e615c0cd8c84e888468. The new commit replaces the protocol-side six-frame poll with an in-page quiet-state loop over the rail mapping and scroll metrics. I found one P2: the loop source is passed to Playwright as a string, so it is evaluated but never invoked with the supplied argument; the helper receives undefined and the target test now fails deterministically.
On this head, the targeted --workers=4 --repeat-each=40 run failed 40/40 at prompt-rail.spec.ts:251, and the full prompt-rail spec failed the same test while the other 10 passed. Clean install, Desktop build:with-deps, Desktop typecheck, Biome, ASF headers, and git diff --check passed. Current main is 74a20f60c9a7bb6947e2c428f07ca2e83fad92a1; its eight changed files since the PR base do not overlap this PR, and a three-way merge tree is clean. GitHub currently reports no hosted checks. I could not validate the macOS-only overlay-scrollbar path from this Linux host.
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
| }, { message: 'the one current tick maps from the Turn being read' }).toBe(true); | ||
| } catch { | ||
| throw new Error(`the prompt rail did not settle on the reading position: ${JSON.stringify(lastSnapshot)}`); | ||
| const outcome = await page.evaluate(SETTLE_PROMPT_RAIL_SOURCE, { |
There was a problem hiding this comment.
[P2] Invoke the settle function instead of evaluating its source string
SETTLE_PROMPT_RAIL_SOURCE is a string, so Playwright sends it with isFunction: false: the expression produces an arrow function object, but Playwright does not call it or bind the second argument. The function result is not serializable and comes back as undefined, so the next line throws before any quiet-state frame is observed. This exact head fails the targeted 4-worker stress run 40/40 and the full spec fails 1/11 at this path. Pass an actual function callback to page.evaluate, or explicitly invoke the generated source with the serialized arguments, then rerun the same stress topology.
There was a problem hiding this comment.
Confirmed, and thank you for catching it — the finding is correct and the 40/40 result is consistent with the mechanism. I verified against playwright-core 1.62.1's injected evaluate: with isFunction: false the expression result is returned as-is (else if (isFunction === false) { result = result; }), so the generated arrow function was never invoked with the argument object and serialization reduced it to undefined; the helper then threw on outcome.settled before the first settle frame. The string-source form was an unjustified complication on my part — the old snapshot helper had already demonstrated the right pattern (a real function with the argument carrying promptCount).
Fixed on ab2484d: the settle loop is now passed to page.evaluate as a function, taking { promptCount, quietFrames, timeoutMs } — the same shape waitForStableTurnAtScrollerStart uses in transcript-scroll.spec.ts. The quiescence semantics from the previous commit are unchanged: full rail state (tick mapping plus scrollTop/scrollHeight/clientHeight) byte-identical across six consecutive painted frames while exactly one current tick maps from the Turn being read, the helper asserts the state the loop verified, and nothing is read after settle. The function-form also removes the duplication the string form created between source generation and the typed read.
Stress rerun request stands: --workers=4 --repeat-each=40 against ab2484d. This time the expected failure mode, if any remains, would be inside the helper's 10s in-page window rather than after it returns.
M4n5ter
left a comment
M4n5ter
left a comment
There was a problem hiding this comment.
Blocking on current head d4c66770150c6e7658a61e615c0cd8c84e888468.
SETTLE_PROMPT_RAIL_SOURCE is passed to page.evaluate as a string. Playwright treats a string as an expression; it does not invoke the function produced by that expression or pass the second argument into it. The call therefore returns undefined, and outcome.settled fails before any settling logic runs. The hosted CI reproduces this exactly at prompt-rail.spec.ts:251.
Please pass an actual callable page function (or explicitly invoke the string expression), then rerun the targeted CI-like concurrency stress because the preceding implementations still reproduced the original flake.
Automated review notice: This review was posted by OpenAI Codex operating for @M4n5ter. It does not replace independent human judgment.
github-actions
Bot
added
effort/M
…pache#4675) `page.evaluate(SETTLE_PROMPT_RAIL_SOURCE, arg)` evaluated the generated source as an expression: Playwright sends string page functions with `isFunction: false`, so the arrow function came back uncalled and serialization reduced it to `undefined` — the helper threw on `outcome.settled` before the first settle frame ran, failing the stress run 40/40 and the spec at this path (review on apache#4685). The settle loop is now a real function passed to `page.evaluate`, with `promptCount`, `quietFrames`, and `timeoutMs` as the argument — the pattern this file already used for its snapshot helper and that `waitForStableTurnAtScrollerStart` uses in transcript-scroll.spec.ts. The quiescence semantics are unchanged: full rail state (tick mapping plus scroll metrics) identical across six consecutive painted frames while one current tick maps from the Turn being read, with the helper asserting the state the loop verified.
|
Confirmed on both counts, and fixed on the current head The string-evaluate bug: independently verified the same way - Playwright's client sends a string Per your request, the settle loop is now passed to On the stress rerun: note that |
4 participants
Summary
The Desktop E2E test
manual transcript scrolling keeps exactly the visible prompt current(apps/desktop/e2e/prompt-rail.spec.ts:370) fails intermittently on CI at the firstexpectPromptRailMatchesReadingPosition(page)call, before the scroll it is about has happened.The helper polled until one snapshot agreed with the reading position, then took a fresh snapshot a round trip later and asserted it again. After
scrollTranscriptTo(page, 'bottom')the transcript can still be settling, so the poll could pass on a frame where rail and reading position agreed and the second read could then see the rail one tick further — the exact failure CI reported (turn-prompt-rail-119vsturn-prompt-rail-120atprompt-rail.spec.ts:206). The second read cannot fail on a settled page, so it only added a way to fail on an unsettled one.The poll now gates on an observable quiet state, and the post-poll re-read is gone: nothing is read after the settle that the settle did not verify. Neither a single agreeing snapshot nor two reads separated by a fixed frame count proves quiescence — agreement at a moment can precede a stable flip. After the scroll, turn content keeps remeasuring,
scrollHeightgrows whilescrollTopstays put, the snapshot'satEndbranch turns false, and the rail legitimately moves current off the last prompt after the helper returned. The helper therefore runs one settle loop in the page: each painted frame re-reads the tick mapping together with the scroll metrics that feed it, and it resolves only when that full state is unchanged for six consecutive frames while one current tick maps from the Turn being read. Every input the rail's resolver reacts to (scroll, mutation, geometry) is then exactly what produced the asserted state.Fixes #4675
Verification
biome checkon the spec: clean.tsc --noEmitpass over the changed helper region: no errors (a full repo typecheck was not possible locally —node_modulesis not installed; the remaining errors in the run were missing-dependency artifacts infixtures.ts, none in this file).Desktop e2eCI job on this PR is the confirmation of the flake fix. The concurrent stress case from the review (--workers=4 --repeat-each) could not be reproduced locally for the same reason.AI use
Select exactly one:
Tool(s) and scope:
Checklist
Does this PR entail a change in behavior?