Skip to content

fix(web): stop reading anchor from undoing a pending scroll; drop duplicated settings spec - #919

Merged
tt-a1i merged 1 commit into
mainfrom
test/fix-flaky-history-provider-settings
Nov 9, 2026
Merged

tt-a1i merged 1 commit into
mainfrom
test/fix-flaky-history-provider-settings

Conversation

@tt-a1i

@tt-a1i tt-a1i commented Nov 9, 2026

Copy link
Copy Markdown
Collaborator

README PR #918 went red on two flaky checks (run 42630501387). The same code was green on main (6b18c71). This PR handles both at the root. It does not add retries, widen timeouts or rerun CI.

Verdicts

Test Verdict Evidence
session-history.e2e.ts:340 "prefetched native history loads on upward reading…" (toBeInViewport, ratio 0) Keep, fix product code This is the only real-browser check that the reading anchor holds through history prepend and streaming, so it has unique value. Under CPU load it failed locally 1/32 and 1/80. Instrumenting scrollTo and scrollTop on .conversation captured the bad run: the test's scrollTo(0), then 88 ms later useReadingAnchor.restore() (in a layout effect) called scrollTo(645) before any scroll event fired. The anchor was captured at the old position, and a render landed between the scroll and its event, so it pulled the reader back. Real wheel scrolls go through the same path.
provider-settings-page.spec.ts:1564 "saves every result display and footer control…" (5000 ms timeout on Windows; 5040 ms before) Delete It duplicates settings-parity.e2e.ts:414 step for step: the same three display selects → Full, footer style → Powerline Mono, footer switch off, then a rejected save that keeps the value. The e2e also asserts the exact patch payloads and a reload round-trip against a real browser. It was the slowest jsdom test in the file (2.2 s on Linux vs ~0.7 s for its neighbours). Its only unique assertion, that a display change never calls setup/configure, now lives in the spec below.
provider-settings-page.spec.ts:1636 "routes agent controls through canonical setup…" (expected 1 preferences call, got 2) Keep (+1 assertion) This is a cascade, not its own bug. Vitest does not stop a timed-out test body, so the previous test kept clicking display menus in this test's DOM and sent its POSTs to this test's fetch stub. Reproduced on Linux with --testTimeout=1000: the previous test timed out and this one got 3 calls. It is the only test that protects an unsaved concurrency draft across a preference save and the configure prompt text. Added toHaveBeenCalledTimes(1).

Changes

  • use-reading-anchor.ts: the anchor records scrollTop at capture time. restore() only corrects layout shifts at that same scroll position. If the scrollport has moved since capture (scroll event still pending), it leaves the position alone, and the layout effect recaptures.
  • reading-anchor.spec.ts: new test. A layout shift at the same position is still corrected, and a scroll that lands before its event survives a re-render. Mutation check: without the fix it fails with expected 370 to be 0.
  • provider-settings-page.spec.ts: deleted the duplicate and strengthened the routing test.
  • web/dist: rebuilt.

Verification

  • bun run check && bun run test: 109 files, 1647 tests passing.
  • After the fix: 96 repeats of the session-history test under 8 workers + 8 CPU spinners gave 0 toBeInViewport failures. One run hit the 30 s overall test budget under that overload; its trace shows the viewport assertion passed and the run was still progressing.
  • Related e2e files pass: session-history, streaming-reading, settings-parity, transcript-search, session-observer.

…oll event

The reading anchor restored a stale position whenever a commit ran between
a scroll and its scroll event, snapping the reader back (flaky
session-history e2e). Restore now only corrects layout shifts at the
captured scroll position.

Delete the jsdom 'saves every result display and footer control' spec: it
duplicates settings-parity.e2e.ts:414 step for step, was the slowest spec in
the file (5s timeouts on Windows), and its leaked continuation after a
timeout broke the next test. Keep the unique 'configure not called' check
in the routing spec.
@tt-a1i
tt-a1i merged commit 0afae3a into main Nov 9, 2026
21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant