diff --git a/src/components/molecular/MessageThread/MessageThread.test.tsx b/src/components/molecular/MessageThread/MessageThread.test.tsx index 4522bc82..65c2985c 100755 --- a/src/components/molecular/MessageThread/MessageThread.test.tsx +++ b/src/components/molecular/MessageThread/MessageThread.test.tsx @@ -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(); + 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(); + 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. * diff --git a/src/components/molecular/MessageThread/MessageThread.tsx b/src/components/molecular/MessageThread/MessageThread.tsx index 8b099440..40bcb34c 100755 --- a/src/components/molecular/MessageThread/MessageThread.tsx +++ b/src/components/molecular/MessageThread/MessageThread.tsx @@ -88,6 +88,15 @@ export default function MessageThread({ const previousScrollHeight = useRef(0); const shouldAutoScroll = useRef(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(false); + // Determine whether to use virtual scrolling const useVirtualScrolling = messages.length >= VIRTUAL_SCROLL_THRESHOLD; @@ -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 @@ -435,7 +490,11 @@ export default function MessageThread({ {showScrollButton && (