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
Original file line number Diff line number Diff line change
Expand Up @@ -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(
<MessageList messages={makeMessages(50)} conversationId="conv-mam-repaint" isLoadingOlder={false} {...props} />,
)
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(<MessageList messages={makeMessages(51)} conversationId="conv-mam-repaint" isLoadingOlder={false} {...props} />)
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(<MessageList messages={makeMessages(51)} conversationId="conv-mam-repaint" isLoadingOlder={true} {...props} />)
rerender(<MessageList messages={makeMessages(52)} conversationId="conv-mam-repaint" isLoadingOlder={true} {...props} />)
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(
<MessageList messages={makeMessages(50)} conversationId="conv-mam-done" isLoadingOlder={true} {...props} />,
)
const scroller = container.querySelector('[data-message-list]') as HTMLElement
instrumentScroller(scroller, 2000)
rafQueue.length = 0
scrollToIndexCalls.length = 0

rerender(<MessageList messages={makeMessages(50)} conversationId="conv-mam-done" isLoadingOlder={false} {...props} />)

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(
<MessageList messages={makeMessages(50)} conversationId="conv-mam-scrolled" isLoadingOlder={true} {...props} />,
)
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(<MessageList messages={makeMessages(50)} conversationId="conv-mam-scrolled" isLoadingOlder={false} {...props} />)

expect(scrollToIndexCalls).not.toContain('end')
})
})

/**
Expand Down
16 changes: 16 additions & 0 deletions apps/fluux/src/components/conversation/pinBottomRun.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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', () => {
Expand Down
13 changes: 11 additions & 2 deletions apps/fluux/src/components/conversation/pinBottomRun.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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'

Expand All @@ -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. */
Expand Down
37 changes: 34 additions & 3 deletions apps/fluux/src/components/conversation/useMessageListScroll.ts
Original file line number Diff line number Diff line change
Expand Up @@ -372,6 +372,13 @@ export function useMessageListScroll({
const virtualizerRef = useRef<MessageVirtualizer | undefined>(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<string | null>(null)
const prevMessageCountRef = useRef(0)
Expand Down Expand Up @@ -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
}

Expand Down Expand Up @@ -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()
Expand Down Expand Up @@ -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
// ==========================================================================
Expand Down
Loading