Skip to content

Loading older history dumps the reader at the top, and jump-to-bottom lands 1934px short on the non-virtualized path #756

Description

@TortoiseWolfe

Two defects in MessageThread's pagination path, both reproduced against a local Supabase
stack on firefox — 2 of 2 runs, deterministic, not flaky — with the same numbers CI
reports on PR #750.

They were found by #745's rewritten performance.spec.ts and noted as out of scope in #754,
which is now closed. This is their ticket.

1. Loading older history dumps the reader at the very top

after older messages loaded the view stayed pinned at the very top, so the reader lost their place
expect(received).toBeGreaterThan(expected)
Expected: > 0
Received:   0

Scroll to the top of a conversation, a page of older messages loads, and scrollTop is still
0 — so instead of staying with the message they were reading, the reader is looking at the
oldest message in the newly-loaded page. Every subsequent scroll-up repeats it.

MessageThread.tsx intends to prevent exactly this. It captures the pre-pagination height and
adds the difference back:

useEffect(() => {                       // capture
  if (loading && hasMore) previousScrollHeight.current = parent.scrollHeight;
}, [loading, hasMore]);

useEffect(() => {                       // restore
  if (!parent || !previousScrollHeight.current) return;
  const heightDifference = parent.scrollHeight - previousScrollHeight.current;
  if (heightDifference > 0) parent.scrollTop = parent.scrollTop + heightDifference;
}, [messages]);

Suspected mechanism, to be confirmed by measurement before anything is changed: the
capture is an effect keyed on [loading, hasMore] while the restore is keyed on [messages].
When loading and messages arrive in the same React commit, the capture runs after the
new rows are already in the DOM, so it records the NEW height, heightDifference is 0, and
the restore does nothing. The if (!previousScrollHeight.current) return guard produces the
same outcome if the capture never ran at all.

Two effects racing to describe one DOM mutation is the shape; the fix should remove the race
rather than add a third effect — capture the height synchronously at the point the load is
requested, or anchor to a known element instead of arithmetic on heights.

2. Jump-to-bottom lands 1934px short on the non-virtualized path

jump-to-bottom did not land at the newest message
expect(received).toBeLessThan(expected)
Expected: < 100
Received:   1934

Distinct from the virtualized defect fixed in #754. Here the loaded message count is below
the 100-message threshold, so scrollToBottom takes the plain branch:

parent.scrollTo({ top: parent.scrollHeight, behavior: 'smooth' });

Suspects, in order, none yet confirmed:

  • the restoration effect above writes parent.scrollTop while the smooth animation is in
    flight, and a programmatic scrollTop write cancels a smooth scroll;
  • top: parent.scrollHeight is read once when the click happens, while pagination is still
    prepending content, so the target is stale by the time the animation ends.

Both are consistent with landing short and staying short through a 15-second poll.

Why these surfaced now

Before #745 these tests ended on a wait rather than an assertion, so both defects were
invisible. The earlier #750 commit failed msg-iso on all three browsers; after a partial
fix only firefox fails, which means the remaining defect is real and engine-sensitive rather
than gone.

Definition of done

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't working

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions