fix(#745): performance.spec.ts asserted nothing — T167 never paginated - #750
Merged
Conversation
Ten tests, NINE of which asserted only `expectConversationLoaded` — "the thread and the input
are visible" — then waited a few animation frames and closed the page.
T167: Pagination loads next 50 messages never scrolled, never paginated
T166: Performance with 1000 messages - FPS 150-message fixture, no timings taken
Virtual scrolling activates at 100 messages never counted a rendered node
Jump to bottom button with smooth scroll wrapped in `if (jumpVisible)`, so absent
button = zero assertions = green
Keyboard navigation through messages pressed keys, asserted nothing
Scroll position maintained during pagination did not paginate or check position
Auto-scroll to bottom on new message sent no message
They passed whether the features worked, were broken, or were deleted — the same family as
the regex guard that predicted #723 and stayed green through it, and the
`if (await jumpButton.isVisible())` wrapper that made T009 vacuous.
EACH ASSERTION IS NOW GROUNDED IN AN OBSERVABLE CONSEQUENCE:
virtualization rendered `message-bubble` nodes < 100 for a 150-message history, and > 0
(a bare "fewer than 150" passes on an empty thread)
pagination `pagination-loader` appears — the component saying it is fetching — AND
scrollHeight grows. Under virtualization the loaded count is not countable
in the DOM, so total height is the honest signal
jump-to-bottom `data-show-scroll-button` flips true, the button is asserted VISIBLE rather
than tested for, and the landing position is POLLED because the scroll is
smooth (the #300 lesson)
restoration after a page loads, scrollTop > 0 — you were not teleported to the oldest
message in the conversation
keyboard focus stays in the composer through arrow keys; Tab lands on something
focusable WITH an accessible name, not `<body>`
FOUR TESTS DELETED RATHER THAN FIXED. Two "60fps" tests, "Performance monitoring logs", and a
duplicate load check made claims this harness cannot substantiate: Playwright cannot measure
frame rate meaningfully through a WebDriver session, and there is no logging contract to
assert against. A test that cannot check its own name is worse than no test — it occupies the
space where a real one would go. Frame budget needs a tracing-based measurement; that is its
own work, not a rename.
VERIFICATION, STATED HONESTLY. type-check and lint clean. I could NOT run these locally: the
isolated-conversation fixture times out in `openConversationAs` waiting 60s for
`[data-testid="message-thread"]`. That is not caused by this change — I stashed the rewrite
and ran MAIN's version as a control, and it fails identically. Something in that fixture path
does not work against my local stack, and it is worth its own investigation.
So the verification for this change is the PR's own local E2E lane, which runs these specs in
the conditions they are written for. Not merging until it is green.
Noticed while grepping, not fixed here: `tests/e2e/mobile-check.spec.ts:14` hardcodes
`http://localhost:3000/status`, the same bug #749 just fixed in messaging-scroll.
Closes #745
The lane caught this, which is the point of it. All four new assertions failed on all three
browsers with the same message — the one I had added as a guard:
Error: no messages rendered at all
Expected: > 0
Received: 0
Zero `message-bubble` nodes. Not a selector problem: all three MessageBubble branches
(deleted, decryption-error, normal) carry `data-testid="message-bubble"`, so zero means the
thread really had rendered nothing when I counted.
THE CAUSE IS THE MISTAKE THIS FILE EXISTS TO REMOVE. I counted after `settleFrames` — three
animation frames — and above 100 messages the virtualizer renders nothing until it has
measured the scroll box. "The thread element is visible" is true long before any message is
fetched, decrypted and laid out. So I measured an empty list, and every assertion that
depends on content — how many nodes virtualization renders, whether scrolling to the top
paginates, whether the jump button appears — failed together.
Same lesson as #300 and #739, and I walked into it while fixing exactly that class of bug.
`expectConversationLoaded` now waits for the first bubble to be VISIBLE (auto-retrying, 45s)
before any test measures anything. The count assertion also proves non-empty BEFORE checking
the upper bound, because "fewer than 100 bubbles" is trivially true of zero — that ordering
is what turned a silent vacuous pass into a loud failure in the first place.
Two tests already passed in that run — the keyboard ones — which is what established the
fixture and `openConversationAs` work fine in CI, and that the failure was mine.
Refs #745
… still loading
Both remaining failures on this branch are fixed, and only one of them was the bug I
said it was.
RESTORATION WAS NEVER BROKEN — MY TEST MEASURED THE GAP. `loading older history does
not teleport you to the top` failed with scrollTop 0 every run on firefox, and I
reported it as a product defect. It is not. Traced in the browser at 60ms intervals:
0ms top=0 h=5278 kids=2
94ms top=2722 h=8216 kids=8
155ms top=2962 h=8600 kids=14 <- stable for the next 1.4s
The component restores correctly. The test polled `scrollHeight > before.scrollHeight`
where `before` was captured at 5216 while the settled pre-pagination height was 5278 —
so 62px of ordinary layout drift satisfied the poll before a single older message had
arrived, and the one-shot read that followed caught scrollTop still at 0. It now polls
the value it is actually claiming, and separately asserts the thread really grew, so
it cannot be satisfied by a stray scroll with no pagination behind it.
That is the third time today this exact shape has cost real time, in the file written
to remove it.
THE JUMP IS AN INTENT, NOT A COORDINATE. `jump-to-bottom returns you to the newest
message` landed 1934px short in 2 of 3 full-spec runs. Reaching the top is what
triggers loading older messages, so "scroll to top, then press jump" is the ordinary
case — and the click resolved to `scrollTop = scrollHeight` at that instant, a number
already stale by the time the scroll finished. Nothing re-aimed afterwards, because
prepending older messages does not change the newest message id, so the auto-scroll
effect never fires.
So the jump now sets an intent that outlives the trip: while it is pending, the
ResizeObserver re-aims at the bottom whenever the content grows, and the reader taking
over — wheel, touch, keydown, or a scrollbar mousedown — cancels it. Deliberately not
the `scroll` event, which the jump's own animation emits and which would cancel the
intent the instant it started acting on it. It converges because scrolling changes
position, not size.
The observer also watched `firstElementChild`, which during pagination is the LOADER,
not the message list — so it would have missed exactly the growth this depends on. It
now observes every child and re-subscribes when the content changes.
Verified where it actually fails, which is the full spec: firefox 6/6 clean, was 2 of
3 failing. chromium 7/7. webkit cannot launch in this container (missing libxslt,
libx264) so CI covers it. Reverting the re-aim reproduces the failure.
Deterministic coverage added, because the E2E is timing-dependent by nature: one test
grows the content and drives the observer by hand, asserting the jump re-aims at the
new height; one asserts a wheel event stops it, so it cannot fight the reader. Each
fails under its own mutation, and the first fails first if the click never reached the
container at all.
Also filed: #757, a `/game/cod-skeleton` horizontal-overflow failure seen once in CI
and 5/5 clean locally — unreproduced rather than dismissed, and not blocking this.
Closes #756
Closes #745
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
TortoiseWolfe
force-pushed
the
fix/745-performance-spec-assertions
branch
from
August 15, 2026 18:59
29759f0 to
86ccbed
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
performance.spec.tshad ten tests, nine of which asserted only that the thread and inputwere visible, then waited a few animation frames and closed the page.
T167: Pagination loads next 50 messagesnever paginated. Two tests claimed 60fps while doing no scrolling and takingno timings. They passed whether the features worked, were broken, or were deleted.
Rewritten to six tests that each name an observable consequence. They immediately found
three defects, which is why this PR sat red — and all three are now resolved.
What the new tests found
1. The jump button was decided against an empty thread — fixed in #754/#755, already on
main.
2. The virtualized jump asked for a scroll TanStack cannot perform — also #754/#755.
3. The jump resolves to a coordinate while the page is still loading. Reaching the top is
what triggers loading older messages, so "scroll to top, then press jump" is the ordinary
case. The click resolved to
scrollTop = scrollHeightat that instant — a number alreadystale by the time the scroll finished — and nothing re-aimed, because prepending older
messages does not change the newest message id, so the auto-scroll effect never fires. Landed
1934px short in 2 of 3 full-spec runs.
Fixed by making the jump an intent that outlives the trip: while it is pending the
ResizeObserverre-aims at the bottom as content grows, and the reader taking over — wheel,touch, keydown, scrollbar mousedown — cancels it. Deliberately not the
scrollevent, whichthe jump's own animation emits and which would cancel the intent the moment it started acting
on it.
That observer also watched
firstElementChild, which during pagination is the loader, notthe message list — so it would have missed exactly the growth this depends on. It now observes
every child and re-subscribes when content changes.
One correction
I reported
loading older history does not teleport you to the topas a product defect. Itwas my test measuring the gap. Traced in the browser:
Restoration works. The test polled
scrollHeight > before.scrollHeightwherebeforewas5216 while the settled height was 5278, so 62px of layout drift satisfied the poll before any
older message arrived, and the one-shot read that followed caught scrollTop still at 0. It now
polls the value it claims, and separately asserts the thread really grew — so it cannot be
satisfied by a stray scroll with no pagination behind it.
Verification
Measured where it actually fails, which is the full spec — running the test alone passes
regardless, which is how the first mutation attempt fooled me:
libxslt,libx264) — CI covers itReverting the re-aim reproduces the failure. Two deterministic unit tests were added because
the E2E is timing-dependent by nature: one grows the content and drives the observer by hand,
asserting the jump re-aims at the new height; one asserts a wheel event stops it so it cannot
fight the reader. Each fails under its own mutation, and the first fails first if the click
never reached the container.
MessageThreadunits: 35/35.Also filed
#757 —
/game/cod-skeletonhorizontal overflow failed once in CI and is 5/5 cleanlocally. Recorded as unreproduced rather than dismissed as a flake, with the dev-server vs
production-build caveat stated. Not blocking.
Closes #745
Closes #756