From a56bc0879096439225df2a2d3b624e67ac91b45e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micka=C3=ABl=20R=C3=A9mond?= Date: Wed, 8 Jul 2026 18:10:40 +0200 Subject: [PATCH] fix(scroll): suppress pin-bottom repaint during MAM catch-up, settle once on completion Entering a room that needs forward MAM catch-up pages in history over ~1-2s; each merge grew scrollHeight and forced a WebKit repaint (~12ms) on every re-pin, so the bottom-follow loop never converged within its frame budget and the view visibly jumped once catch-up finally settled. The repaint is now suppressed while isLoadingOlder is true (WebKit never painted those intermediate positions anyway), and a single clean settle pin fires once catch-up completes. --- .../MessageList.virtualizedScroll.test.tsx | 65 +++++++++++++++++++ .../conversation/pinBottomRun.test.ts | 16 +++++ .../components/conversation/pinBottomRun.ts | 13 +++- .../conversation/useMessageListScroll.ts | 37 ++++++++++- 4 files changed, 126 insertions(+), 5 deletions(-) diff --git a/apps/fluux/src/components/conversation/MessageList.virtualizedScroll.test.tsx b/apps/fluux/src/components/conversation/MessageList.virtualizedScroll.test.tsx index b65b83987..f217ebf48 100644 --- a/apps/fluux/src/components/conversation/MessageList.virtualizedScroll.test.tsx +++ b/apps/fluux/src/components/conversation/MessageList.virtualizedScroll.test.tsx @@ -981,6 +981,71 @@ describe('MessageList — virtualized bottom-stick re-asserts as rows measure', expect(scrollToOffsetCalls.length).toBeGreaterThan(0) // the prepend restore actually ran expect(scrollToIndexCalls).not.toContain('end') // ...and did not jump to the bottom }) + + it('suppresses the forced repaint on re-pins while a MAM catch-up is loading', () => { + // Baseline: NOT loading -> the stale-paint fix forces a repaint (offsetHeight read) on a + // write that actually moved scrollTop, same as every other re-pin test in this file. + const { container, rerender } = render( + , + ) + const scroller = container.querySelector('[data-message-list]') as HTMLElement + const { grow } = instrumentScroller(scroller, 2000) + let offsetHeightReads = 0 + Object.defineProperty(scroller, 'offsetHeight', { get: () => { offsetHeightReads++; return 500 }, configurable: true }) + rafQueue.length = 0 + + offsetHeightReads = 0 + rerender() + flush(1) + grow(3000) + flush(14) + expect(offsetHeightReads).toBeGreaterThan(0) + + // A MAM page lands (message count grows again) while catch-up is in flight: the re-pin still + // writes scrollTop, but the expensive forced repaint must be skipped. + offsetHeightReads = 0 + rerender() + rerender() + flush(1) + grow(4000) + flush(14) + expect(offsetHeightReads).toBe(0) + }) + + it('fires one clean settle pin when a MAM catch-up completes with no further message growth', () => { + // The catch-up's last page can land (messageCount already reflects it) with NOTHING further + // changing except isLoadingOlder flipping false — the "new message" effect sees no count/id + // change and stays silent, so the completion must be its own trigger. + const { container, rerender } = render( + , + ) + const scroller = container.querySelector('[data-message-list]') as HTMLElement + instrumentScroller(scroller, 2000) + rafQueue.length = 0 + scrollToIndexCalls.length = 0 + + rerender() + + expect(scrollToIndexCalls).toContain('end') + }) + + it('does NOT fire the settle pin on catch-up completion when the reader is scrolled away from the bottom', () => { + const { container, rerender } = render( + , + ) + const scroller = container.querySelector('[data-message-list]') as HTMLElement + instrumentScroller(scroller, 5000) + rafQueue.length = 0 + + // Reader scrolls up, away from the bottom -> isAtBottom flips false. + scroller.scrollTop = 1000 + scroller.dispatchEvent(new Event('scroll', { bubbles: true })) + scrollToIndexCalls.length = 0 + + rerender() + + expect(scrollToIndexCalls).not.toContain('end') + }) }) /** diff --git a/apps/fluux/src/components/conversation/pinBottomRun.test.ts b/apps/fluux/src/components/conversation/pinBottomRun.test.ts index d0d3bcbea..25221f20a 100644 --- a/apps/fluux/src/components/conversation/pinBottomRun.test.ts +++ b/apps/fluux/src/components/conversation/pinBottomRun.test.ts @@ -81,6 +81,22 @@ describe('shouldForceRepaint — repaint gating policy', () => { expect(shouldForceRepaint(true, 'off')).toBe(false) expect(shouldForceRepaint(false, 'off')).toBe(false) }) + + it('on-write: suppresses the repaint when a background load (MAM catch-up) is in flight', () => { + expect(shouldForceRepaint(true, 'on-write', true)).toBe(false) + }) + + it('on-write: repaints as before when no background load is in flight', () => { + expect(shouldForceRepaint(true, 'on-write', false)).toBe(true) + }) + + it('always: the debug escape hatch still forces a repaint during a background load', () => { + expect(shouldForceRepaint(true, 'always', true)).toBe(true) + }) + + it('off: stays suppressed during a background load (already off)', () => { + expect(shouldForceRepaint(true, 'off', true)).toBe(false) + }) }) describe('readPinRepaintMode — localStorage override parsing', () => { diff --git a/apps/fluux/src/components/conversation/pinBottomRun.ts b/apps/fluux/src/components/conversation/pinBottomRun.ts index 220cc843a..8842c5370 100644 --- a/apps/fluux/src/components/conversation/pinBottomRun.ts +++ b/apps/fluux/src/components/conversation/pinBottomRun.ts @@ -28,6 +28,14 @@ const PIN_SETTLED_FRAMES = 8 * repaint and skipping it removes the most expensive step on WebKitGTK. * - 'always' / 'off': on-device A/B escape hatches (localStorage * `fluux:pin-repaint`) to validate the gating on the real Linux build. + * + * `suppressForBackgroundLoad` additionally skips the repaint while a MAM catch-up (or + * "load older") query is in flight for the conversation. A catch-up can page in dozens of + * merges over 1-2s, each moving scrollTop; forcing a repaint on every single one is the + * forced-layout/repaint storm PR #860 already had to cut down for the measurement-settle + * case. WebKit isn't painting those intermediate positions anyway without the forced + * toggle, so nothing visible is lost — the caller is responsible for forcing one final + * repaint once the load completes. 'always' still wins (on-device A/B must stay unconditional). */ export type PinRepaintMode = 'on-write' | 'always' | 'off' @@ -45,11 +53,12 @@ export function readPinRepaintMode( export function shouldForceRepaint( scrollTopMoved: boolean, - mode: PinRepaintMode + mode: PinRepaintMode, + suppressForBackgroundLoad = false ): boolean { if (mode === 'always') return true if (mode === 'off') return false - return scrollTopMoved + return scrollTopMoved && !suppressForBackgroundLoad } /** Forced-work categories a pin run accumulates for the probe line. */ diff --git a/apps/fluux/src/components/conversation/useMessageListScroll.ts b/apps/fluux/src/components/conversation/useMessageListScroll.ts index 7b088884c..08c6c6230 100644 --- a/apps/fluux/src/components/conversation/useMessageListScroll.ts +++ b/apps/fluux/src/components/conversation/useMessageListScroll.ts @@ -372,6 +372,13 @@ export function useMessageListScroll({ const virtualizerRef = useRef(undefined) virtualizerRef.current = virtualizer + // Latest MAM-loading state (forward catch-up on entry, or backward "load older" pagination) for + // the active conversation, read imperatively inside pinVirtualizedBottom's stable useCallback — + // see the repaint-suppression note in writePin below. Updated synchronously in the render body + // (same pattern as virtualizerRef) so it is never stale when the pin loop reads it mid-run. + const isLoadingOlderRef = useRef(isLoadingOlder) + isLoadingOlderRef.current = isLoadingOlder + // Track conversation const prevConversationRef = useRef(null) const prevMessageCountRef = useRef(0) @@ -841,7 +848,7 @@ export function useMessageListScroll({ run.addMs('scroll', performance.now() - started) const moved = ss.scrollTop !== before if (moved) wroteAny = true - if (shouldForceRepaint(moved, repaintMode)) forceRepaint() + if (shouldForceRepaint(moved, repaintMode, isLoadingOlderRef.current)) forceRepaint() return moved } @@ -875,12 +882,14 @@ export function useMessageListScroll({ if (framesLeft-- <= 0) { // Loop ran its full budget without converging. Re-derive isAtBottom from geometry (accurate — // the position is correct even when WebKit withheld the paint) and force one final repaint — - // if anything was written — so the settled position is actually drawn. + // if anything was written — so the settled position is actually drawn. Still suppressed while + // a MAM catch-up is in flight: more content is coming, so this isn't the settled position yet — + // the catch-up-complete effect below forces the real final repaint once it finishes. if (s) { flushTailLayout() const dist = s.scrollHeight - s.scrollTop - s.clientHeight isAtBottomRef.current = dist < AT_BOTTOM_THRESHOLD - if (shouldForceRepaint(wroteAny, repaintMode)) forceRepaint() + if (shouldForceRepaint(wroteAny, repaintMode, isLoadingOlderRef.current)) forceRepaint() debugLog('PIN settled (frames exhausted)', { distFromBottom: dist }) } finish() @@ -2698,6 +2707,28 @@ export function useMessageListScroll({ prevLastMessageIdRef.current = lastMessageId }, [conversationId, messageCount, lastMessageId, isAtBottomRef, staticMode, lastMessageIsOutgoing, reassertBottom]) + // ========================================================================== + // EFFECT: Clean settle pin once a MAM catch-up completes + // ========================================================================== + + // writePin above suppresses the forced repaint while isLoadingOlder is true (see pinBottomRun's + // shouldForceRepaint doc) — a catch-up pages in merges every ~50-300ms, each moving scrollTop, and + // WebKit isn't painting those intermediate positions anyway without the forced toggle. But once the + // LAST merge lands, something has to force the final repaint or the view is stuck showing a stale + // frame at the (geometrically correct) suppressed position. This fires exactly once on the + // isLoadingOlder true -> false transition, while the reader is following the bottom, mirroring the + // "new message" effect above but keyed on load completion rather than message-count growth (needed + // because the last MAM page can land with no further count change once the switch effect's own + // initial cache load already brought it in). + const prevIsLoadingOlderRef = useRef(isLoadingOlder) + useEffect(() => { + const wasLoading = prevIsLoadingOlderRef.current + prevIsLoadingOlderRef.current = isLoadingOlder + if (wasLoading && !isLoadingOlder && isAtBottomRef.current && !staticMode) { + reassertBottom('mam-catchup-complete') + } + }, [isLoadingOlder, isAtBottomRef, staticMode, reassertBottom]) + // ========================================================================== // EFFECT: Reset marker scroll tracking when firstNewMessageId changes // ==========================================================================