Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
156 changes: 156 additions & 0 deletions src/components/molecular/MessageThread/MessageThread.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -280,6 +280,162 @@ describe('MessageThread', () => {
}
});

/**
* The jump is an INTENT, and content is still arriving when it is made (#756).
*
* Reaching the top is what triggers loading older messages, so "scroll to the top,
* then press jump" is the ordinary case, not a corner. The old code resolved the jump
* to a coordinate — `scrollTop = scrollHeight` at the instant of the click — and that
* number is stale before the scroll finishes. Prepending older messages does not
* change the newest message id, so the auto-scroll effect never re-aims either, and
* the reader is left short. Measured at 1934px in `performance.spec.ts`, failing 2 of
* 3 full-spec runs on firefox.
*
* Deterministic here, where the E2E is inherently timing-dependent: grow the content
* and drive the observer by hand.
*/
it('re-aims at the bottom when content arrives after the jump was requested', async () => {
const user = userEvent.setup();
const callbacks: ResizeObserverCallback[] = [];
const RealRO = global.ResizeObserver;
vi.stubGlobal(
'ResizeObserver',
class {
constructor(cb: ResizeObserverCallback) {
callbacks.push(cb);
}
observe() {}
unobserve() {}
disconnect() {}
}
);

try {
// Below the virtualization threshold, which is the path this defect lives on.
const messages = Array.from({ length: 50 }, (_, i) =>
createMockMessage(`msg-${i}`, `Message ${i}`, i)
);
render(<MessageThread messages={messages} />);
const container = screen.getByTestId('message-thread');

const scrollTo = vi.fn();
Object.defineProperty(container, 'scrollTo', {
value: scrollTo,
writable: true,
});
Object.defineProperty(container, 'scrollTop', {
value: 0,
writable: true,
});
Object.defineProperty(container, 'scrollHeight', {
value: 5278,
writable: true,
});
Object.defineProperty(container, 'clientHeight', {
value: 258,
writable: true,
});
container.dispatchEvent(new Event('scroll'));

const button = await screen.findByTestId('jump-to-bottom');
await user.click(button);
expect(
scrollTo,
'the jump never asked the container to scroll, so nothing below is meaningful'
).toHaveBeenCalledWith(expect.objectContaining({ top: 5278 }));

// A page of older messages lands: the thread is now much taller, and the target
// the click resolved to is no longer the bottom.
scrollTo.mockClear();
Object.defineProperty(container, 'scrollHeight', {
value: 8600,
writable: true,
});
callbacks.forEach((cb) =>
cb([] as unknown as ResizeObserverEntry[], {} as ResizeObserver)
);

await waitFor(() => {
expect(scrollTo).toHaveBeenCalledWith(
expect.objectContaining({ top: 8600 })
);
});
} finally {
vi.stubGlobal('ResizeObserver', RealRO);
}
});

/**
* ...and stops as soon as the reader takes over, or it would fight them.
*/
it('stops re-aiming once the reader scrolls for themselves', async () => {
const user = userEvent.setup();
const callbacks: ResizeObserverCallback[] = [];
const RealRO = global.ResizeObserver;
vi.stubGlobal(
'ResizeObserver',
class {
constructor(cb: ResizeObserverCallback) {
callbacks.push(cb);
}
observe() {}
unobserve() {}
disconnect() {}
}
);

try {
const messages = Array.from({ length: 50 }, (_, i) =>
createMockMessage(`msg-${i}`, `Message ${i}`, i)
);
render(<MessageThread messages={messages} />);
const container = screen.getByTestId('message-thread');

const scrollTo = vi.fn();
Object.defineProperty(container, 'scrollTo', {
value: scrollTo,
writable: true,
});
Object.defineProperty(container, 'scrollTop', {
value: 0,
writable: true,
});
Object.defineProperty(container, 'scrollHeight', {
value: 5278,
writable: true,
});
Object.defineProperty(container, 'clientHeight', {
value: 258,
writable: true,
});
container.dispatchEvent(new Event('scroll'));

await user.click(await screen.findByTestId('jump-to-bottom'));

// The reader grabs the wheel. That cancels the pending jump.
container.dispatchEvent(new WheelEvent('wheel', { bubbles: true }));

scrollTo.mockClear();
Object.defineProperty(container, 'scrollHeight', {
value: 8600,
writable: true,
});
callbacks.forEach((cb) =>
cb([] as unknown as ResizeObserverEntry[], {} as ResizeObserver)
);

await waitFor(() => {
expect(callbacks.length).toBeGreaterThan(0);
});
expect(
scrollTo,
'the component kept dragging the reader back to the bottom after they scrolled away'
).not.toHaveBeenCalled();
} finally {
vi.stubGlobal('ResizeObserver', RealRO);
}
});

/**
* Under virtualization the jump must NOT ask for a smooth scroll.
*
Expand Down
71 changes: 65 additions & 6 deletions src/components/molecular/MessageThread/MessageThread.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,15 @@ export default function MessageThread({
const previousScrollHeight = useRef<number>(0);
const shouldAutoScroll = useRef<boolean>(true);

/**
* An explicit "take me to the newest message" that has not been satisfied yet (#756).
*
* Separate from `shouldAutoScroll` on purpose: that one is recomputed from position on
* every scroll event, including the jump's own animation frames, so it is false for
* most of the journey and cannot represent an intent that has to outlive the trip.
*/
const pendingJumpRef = useRef<boolean>(false);

// Determine whether to use virtual scrolling
const useVirtualScrolling = messages.length >= VIRTUAL_SCROLL_THRESHOLD;

Expand Down Expand Up @@ -247,15 +256,61 @@ export default function MessageThread({
const parent = parentRef.current;
if (!parent || typeof ResizeObserver === 'undefined') return;

const observer = new ResizeObserver(() => syncJumpButton());
const observer = new ResizeObserver(() => {
syncJumpButton();

// HOLD THE BOTTOM WHILE THE CONTENT IS STILL ARRIVING (#756).
//
// "Jump to the bottom" is an intent, not a coordinate, and the old code treated it
// as a coordinate: it scrolled to whatever `scrollHeight` happened to be at the
// instant of the click. Click it while a page of older messages is still loading —
// which is the normal case, because reaching the top is what STARTED that load —
// and the target is stale before the animation ends. Measured: the reader is left
// 1934px short, and nothing re-aims, because prepending older messages does not
// change the newest message id so the auto-scroll effect never fires.
//
// Re-aiming here converges: scrolling changes position, not size, so this cannot
// feed itself. It stops as soon as the reader takes over, below.
if (pendingJumpRef.current) scrollToBottomRef.current?.(false);
});
observer.observe(parent);
// The container's own box rarely changes; the CONTENT's height is what moves when
// messages land, so observe both.
const content = parent.firstElementChild;
if (content) observer.observe(content);
// messages land, so observe the children too.
//
// ALL of them, not `firstElementChild`: while a page is loading the first child is
// the pagination loader, so watching only that one would miss the very growth this
// exists to notice. Re-subscribed when the content changes, because the children are
// replaced on render.
Array.from(parent.children).forEach((child) => observer.observe(child));

return () => observer.disconnect();
}, [syncJumpButton]);
}, [syncJumpButton, messages.length, loading]);

/**
* The reader taking over cancels the pending jump.
*
* Deliberately NOT the `scroll` event: the jump's own animation emits those, so using
* them would cancel the intent the moment it started acting on it. These three are the
* ways a person actually moves a thread themselves.
*/
useEffect(() => {
const parent = parentRef.current;
if (!parent) return;
const release = () => {
pendingJumpRef.current = false;
};
parent.addEventListener('wheel', release, { passive: true });
parent.addEventListener('touchstart', release, { passive: true });
parent.addEventListener('keydown', release);
// Dragging the scrollbar emits neither wheel nor touch.
parent.addEventListener('mousedown', release);
return () => {
parent.removeEventListener('wheel', release);
parent.removeEventListener('touchstart', release);
parent.removeEventListener('keydown', release);
parent.removeEventListener('mousedown', release);
};
}, []);

// Bind handleScroll as a native DOM event listener instead of via React's
// `onScroll` JSX prop. Reason: React's synthetic onScroll does not reliably
Expand Down Expand Up @@ -435,7 +490,11 @@ export default function MessageThread({
{showScrollButton && (
<button
type="button"
onClick={() => scrollToBottom(true)}
onClick={() => {
// The intent outlives the scroll: content may still be arriving.
pendingJumpRef.current = true;
scrollToBottom(true);
}}
className="btn btn-circle btn-primary absolute right-4 bottom-4 z-10 min-h-11 min-w-11 shadow-lg"
aria-label="Jump to bottom"
data-testid="jump-to-bottom"
Expand Down
Loading
Loading