Skip to content

test(desktop): settle transcript scroll extremes - #4150

Closed
orangeCatDeveloper wants to merge 1 commit into
apache:mainfrom
orangeCatDeveloper:fix/prompt-rail-scroll-settle
Closed

test(desktop): settle transcript scroll extremes#4150
orangeCatDeveloper wants to merge 1 commit into
apache:mainfrom
orangeCatDeveloper:fix/prompt-rail-scroll-settle

Conversation

@orangeCatDeveloper

@orangeCatDeveloper orangeCatDeveloper commented Aug 29, 2026

Copy link
Copy Markdown
Contributor

Summary

prompt-rail.spec.ts intermittently fails on CI at evicting a turn-owned sibling interaction hands focus back to the transcript: turn-prompt-rail-120 is expected to be mounted after scrolling to the bottom, and the locator resolves to zero for the full ten-second poll. It has failed on main — most recently at 01369b082, the commit this branch is based on.

scrollTranscriptTo assigned scrollTop once. Measured on the runner, that single assignment can leave the scroller 2982px above the bottom while every turn is already loaded, so the virtual window ends seven turns short of the tail and the awaited turn never mounts. The failing state is identical on every occurrence:

tail=false gap=2982 client=780 mounted=14 last=turn-prompt-rail-113 source=120

#3863 added a one-shot scroll dispatch plus two painted frames here, which cannot help when the layout keeps growing past them.

The helper now re-scrolls until a painted frame agrees the scroller is at the requested extreme. It is split rather than changed in place, because its callers want opposite things: settleTranscriptAtExtreme is used where a test needs to actually be at an extreme, while scrollTranscriptOnce keeps the single jump for the focus-handback test, which deliberately jumps once so a late scroll-anchor restore stays observable — a retrying helper would fight that restore and turn the #3121 regression into a false pass.

The assertion also reports the scroll offset, mounted window and loaded turn count on failure, so a future CI-only failure names its own cause instead of saying only that a turn is missing.

Test-only. No renderer or virtualizer behavior changes.

Verification

Thirty full-suite runs per side on GitHub-hosted ubuntu-latest, the same runner class that produces the failure. Both sides carry the same probe workflow; apart from it the two trees differ only by this commit.

before — 01369b082
  26 rounds  79 passed
   3 rounds  78 passed, 1 failed
   1 round   77 passed, 2 failed
  prompt-rail.spec.ts:288          3 / 30
  partial-history-notice.spec.ts:59 2 / 30

after — this branch
  30 rounds  79 passed
  prompt-rail.spec.ts:288          0 / 30
  partial-history-notice.spec.ts:59 0 / 30

All three before failures reported the same state, quoted under Summary.

Two limits worth stating. Three-of-thirty against zero-of-thirty is not statistically significant on its own (Fisher exact, p ≈ 0.23); the argument rests on the measured mechanism, with the rates as corroboration. And partial-history-notice.spec.ts:59 is an unrelated flake that this change does not address — its zero on the after side is sampling, not a fix.

$ npm exec -w @maka/desktop -- playwright test --config e2e/playwright.config.ts prompt-rail.spec.ts
7 passed (19.7s)

$ ./node_modules/.bin/biome check apps/desktop/e2e/prompt-rail.spec.ts
Checked 1 file. No fixes applied.

AI use

Select exactly one:

  • No generative tool made a substantive contribution
  • Generative tooling made a substantive contribution

Tool(s) and scope: Claude Code reproduced the failure, took the measurements above, wrote the change, and prepared this PR description.

Checklist

  • Tests cover the change and fail without it
  • Lint, format, typecheck and the affected suites pass locally

The first box is deliberately unchecked. The change is itself a test, and the failure it removes is a race that appears in roughly one CI run in ten rather than on demand; the before/after measurement above stands in for a deterministic red run.

Does this PR entail a change in behavior?

  • Yes — described under Summary above
  • No

@github-actions github-actions Bot added the effort/S Under 100 readable lines label Aug 29, 2026
@orangeCatDeveloper
orangeCatDeveloper force-pushed the fix/prompt-rail-scroll-settle branch 2 times, most recently from bc02dfb to fb21b8a Compare August 29, 2026 09:55
@orangeCatDeveloper orangeCatDeveloper changed the title test(desktop): settle transcript scroll extremes test(desktop): make the scroll flake self-report Aug 29, 2026
@orangeCatDeveloper
orangeCatDeveloper force-pushed the fix/prompt-rail-scroll-settle branch from fb21b8a to 4a56b44 Compare August 29, 2026 19:02
@orangeCatDeveloper orangeCatDeveloper changed the title test(desktop): make the scroll flake self-report test(desktop): settle transcript scroll extremes Aug 29, 2026
A single scrollTop assignment can leave the scroller 2982px above the
bottom, so the virtual window ends seven turns short and the tail turn
never mounts. Re-scroll until a painted frame agrees it is at the end.

Generated-by: Claude Code
@orangeCatDeveloper
orangeCatDeveloper force-pushed the fix/prompt-rail-scroll-settle branch from 4a56b44 to 04f692b Compare August 29, 2026 19:07
@orangeCatDeveloper

Copy link
Copy Markdown
Contributor Author

Closing: the mechanism this change works around was replaced on main, and the failure no longer reproduces.

The review is right that the base was stale, and it turns out to matter more than a rebase. refactor(ui): make the transcript the only writer of its scroll position deleted chat-scroll-anchor.ts and left a single writer for scrollTop. The failing state measured here is exactly what several competing writers produce, so the fix in this PR patches over a mechanism that no longer exists — and the split helper's justification, keeping one jump observable so a late scroll-anchor restore stays visible, went away with the anchor module itself.

Measurements, all on GitHub-hosted ubuntu-latest, thirty full-suite runs per configuration:

base of this PR
  26 rounds  79 passed
   3 rounds  78 passed, 1 failed
   1 round   77 passed, 2 failed
  prompt-rail.spec.ts:288           3 / 30
  partial-history-notice.spec.ts:59 2 / 30

current main
  30 rounds  90 passed, 1 skipped
  prompt-rail.spec.ts:288           0 / 30
  partial-history-notice.spec.ts:59 0 / 30
  any spec                          0 / 30

All three failures on the old base reported the same state, from an observation-only diagnostic on the assertion:

tail=false gap=2982 client=780 mounted=14 last=turn-prompt-rail-113 source=120

Every turn was loaded; the scroller sat 2982px above the bottom after a single scrollTop assignment, so the virtual window ended seven turns short and the awaited turn never mounted.

Worth stating plainly: three-of-thirty against zero-of-thirty is not significant on its own (Fisher exact, p ≈ 0.23). It is the same evidence strength this PR was claiming for its own fix, so it does not prove the refactor fixed the flake either. What decides it is that the code this PR reasons about is gone, and the reworked transcript shows no failure of any spec in thirty runs.

If the assertion flakes again, the fastest way back to a cause is a temporary diagnostic in place of the bare count, reporting mounted window, distance from the bottom, and loaded turn count. That is not worth carrying for a failure that is not currently occurring, so it is not being split out.

On the other two points: inline issue references would have been fixed — thanks for catching that. The Generated-by trailer is required rather than prohibited; CONTRIBUTING.md says to add one to each affected commit and keep it through squash or amend when AI authors a material part of a contribution, and most recent commits on main carry one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/S Under 100 readable lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant