From 9e0aa32c62e5494ed92278f865812ea935338624 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micka=C3=ABl=20R=C3=A9mond?= Date: Tue, 30 Jun 2026 13:09:20 +0200 Subject: [PATCH] refactor(scroll): make the content anchor the single source of truth for restore Remove the exact-saved-scrollTop fast-path from restoreSavedPosition so the content anchor (message id + fraction, re-derived from each row's CURRENT measured height) governs every restore. Font-size, view-density and viewport -width changes are now handled by the same correct path instead of being caught only incidentally by a total-height delta; the saved scrollTop is demoted to a last-resort fallback used only when there is no usable anchor. - Drop the now-dead stored scrollHeight/clientWidth from ScrollState and remove getSavedScrollHeight/getSavedClientWidth; saveScrollPosition keeps scrollHeight/clientHeight only as transient inputs for the wasAtBottom decision. - Rewrite the affected virtualized restore unit tests to assert the restore CONSULTS the anchor (getOffsetForMessageId / scrollToIndex) rather than a pixel. - Add scroll-invariants invariant-12: a width + density change while away holds the reading anchor on return (chromium). --- .../MessageList.virtualizedScroll.test.tsx | 160 +++++++++--------- .../conversation/useMessageListScroll.ts | 51 ++---- .../src/utils/scrollStateManager.test.ts | 12 -- apps/fluux/src/utils/scrollStateManager.ts | 62 ++----- scripts/scroll-invariants.ts | 79 +++++++++ 5 files changed, 190 insertions(+), 174 deletions(-) diff --git a/apps/fluux/src/components/conversation/MessageList.virtualizedScroll.test.tsx b/apps/fluux/src/components/conversation/MessageList.virtualizedScroll.test.tsx index 33b73b23..08a2f929 100644 --- a/apps/fluux/src/components/conversation/MessageList.virtualizedScroll.test.tsx +++ b/apps/fluux/src/components/conversation/MessageList.virtualizedScroll.test.tsx @@ -100,6 +100,21 @@ function makeMessages(count: number): BaseMessage[] { })) } +/** + * Seed a KNOWN content anchor for `conversationId`, overwriting whatever the component captured on + * leave. jsdom has no layout (rows measure offsetHeight 0), so findBottomAnchor degenerates to + * {last row, fraction 1} and a meaningful anchor can't arise organically — we inject one. A + * fraction < 1 forces the virtualizer-index restore's fraction-refine branch, whose + * `getOffsetForMessageId(anchorId)` call is the observable proof that the restore CONSULTED THE + * ANCHOR (rather than a blind scroll-to-bottom or a saved-pixel fallback). Pixel-position + * correctness is covered by the real-engine scroll-invariants e2e — jsdom can't exercise the + * fraction math. dist = 5000 − 200 − 500 = 4300 > AT_BOTTOM_THRESHOLD → wasAtBottom false → the + * scrolled-up state persists for restore. + */ +function seedSavedAnchor(conversationId: string, messageId: string, fraction = 0.5) { + scrollStateManager.saveScrollPosition(conversationId, 200, 5000, 500, { messageId, fraction }) +} + function renderList(props: Partial> = {}) { return render( { - // Returning to a conversation you'd scrolled up in must restore that position, not - // scroll-to-bottom. The flag-OFF suite covers this; this pins it with the virtualizer wired. + it('restores a scrolled-up conversation from its content anchor on switch (virtualized)', () => { + // Returning to a conversation you'd scrolled up in must restore via the saved CONTENT ANCHOR, + // not scroll to the bottom. The anchor row is typically windowed out under virtualization, so + // the restore resolves it through the virtualizer index (scrollToIndex) and refines to the + // saved fraction (getOffsetForMessageId) — both keyed to the anchor message. (Real pixel + // landing is covered by scroll-invariants.ts; jsdom has no layout.) const { container, rerender } = render( , ) const scroller = container.querySelector('[data-message-list]') as HTMLElement - const { scrollTopSets } = instrumentScroller(scroller, 5000) + instrumentScroller(scroller, 5000) rafQueue.length = 0 - // Scroll up to 200 and let the scroll handler record the position + anchor. + // Scroll up and switch away (saves conv-r1; the captured anchor degenerates in jsdom). scroller.scrollTop = 200 // A genuine user scroll fires a wheel (the save gate only persists user-driven positions, not // media/measurement-induced shifts); a bare scroll event alone no longer counts. scroller.dispatchEvent(new WheelEvent('wheel', { bubbles: true })) scroller.dispatchEvent(new Event('scroll', { bubbles: true })) - - // Switch away (saves conv-r1's position) then back (should restore it). rerender() - scrollTopSets.length = 0 + + // Inject a real anchor, then return. + seedSavedAnchor('conv-r1', 'msg-20') + getOffsetForMessageId.mockClear() + scrollToIndexCalls.length = 0 rerender() - // Restored to the saved scroll position (200), not scrolled to the bottom (5000). Under - // virtualization the anchor is typically windowed out, so the saved-pixel fallback is what - // runs — pinning it guards against a refactor that regresses restore into a scroll-to-bottom. - expect(scrollTopSets).toContain(200) - expect(scrollTopSets).not.toContain(5000) + // Restore consulted the saved anchor (windowed the row in by index + refined by fraction) + // rather than scrolling to the bottom and clearing the saved position. + expect(getOffsetForMessageId).toHaveBeenCalledWith('msg-20') + expect(scrollToIndexCalls).toContain('end') }) it('keeps restored scrolled-up intent across repeated virtualized room switches', () => { @@ -563,7 +582,7 @@ describe('MessageList — virtualized bottom-stick re-asserts as rows measure', , ) const scroller = container.querySelector('[data-message-list]') as HTMLElement - const { scrollTopSets } = instrumentScroller(scroller, 5000) + instrumentScroller(scroller, 5000) rafQueue.length = 0 scroller.scrollTop = 200 @@ -572,21 +591,20 @@ describe('MessageList — virtualized bottom-stick re-asserts as rows measure', scroller.dispatchEvent(new WheelEvent('wheel', { bubbles: true })) scroller.dispatchEvent(new Event('scroll', { bubbles: true })) + // Round-trip #1: leave (saves room-repeat-1), inject a real anchor, return. rerender() - scrollTopSets.length = 0 - scrollToOffsetCalls.length = 0 + seedSavedAnchor('room-repeat-1', 'msg-20') + getOffsetForMessageId.mockClear() rerender() - expect(scrollToOffsetCalls).toContain(200) + expect(getOffsetForMessageId).toHaveBeenCalledWith('msg-20') - scrollTopSets.length = 0 - scrollToOffsetCalls.length = 0 + // Round-trip #2: the scrolled-up intent must survive a second switch (regression: a coalesced + // second restore used to be dropped). Re-inject (the jsdom re-capture degenerates) and return. rerender() - scrollTopSets.length = 0 - scrollToOffsetCalls.length = 0 + seedSavedAnchor('room-repeat-1', 'msg-20') + getOffsetForMessageId.mockClear() rerender() - - expect(scrollToOffsetCalls).toContain(200) - expect(scrollTopSets).not.toContain(5000) + expect(getOffsetForMessageId).toHaveBeenCalledWith('msg-20') }) it('does not restore an old scrolled-up position after the FAB returns the room to bottom', () => { @@ -624,47 +642,42 @@ describe('MessageList — virtualized bottom-stick re-asserts as rows measure', expect(scrollTopSets).toContain(5000) }) - it('re-windows the virtualizer (scrollToOffset) on switch-back restore, not just a raw scrollTop write', () => { - // The blank-screen-until-scroll bug: a direct `scroller.scrollTop = saved` leaves - // @tanstack's scrollOffset stale (it syncs only from the scroll event / rAF poll), - // so on a fresh switch the mounted window keeps the top rows and the restored region - // renders BLANK until the user scrolls. The restore must route the offset through the - // virtualizer (scrollToOffset) so it re-windows before paint — same as the MAM-prepend - // restore and scroll-to-bottom. jsdom has no layout (the blank can't be observed), so - // this pins the re-window CONTRACT: scrollToOffset is called with the restored offset. - // (Both restore sub-paths — content-stable anchor and saved-pixel fallback — must - // re-window; in production the anchor row is usually windowed out so the pixel path runs.) + it('re-windows the virtualizer through the anchor on switch-back restore, not a raw scrollTop write', () => { + // The blank-screen-until-scroll bug: a direct `scroller.scrollTop = saved` leaves @tanstack's + // offset stale, so on a fresh switch the mounted window keeps the top rows and the restored + // region renders BLANK until the user scrolls. The restore must route through the virtualizer so + // it re-windows before paint. With the anchor authoritative, that means resolving the anchor by + // index (scrollToIndex, which re-windows) and refining to the saved fraction (getOffsetForMessageId + // → scrollToOffset). jsdom has no layout, so this pins the re-window CONTRACT: the restore + // consults the anchor through the virtualizer rather than writing a raw scrollTop. const { container, rerender } = render( , ) const scroller = container.querySelector('[data-message-list]') as HTMLElement - const { scrollTopSets } = instrumentScroller(scroller, 5000) + instrumentScroller(scroller, 5000) rafQueue.length = 0 - // Scroll up to 200 and let the scroll handler record the position + anchor. scroller.scrollTop = 200 - // A genuine user scroll fires a wheel (the save gate only persists user-driven positions, not - // media/measurement-induced shifts); a bare scroll event alone no longer counts. - scroller.dispatchEvent(new WheelEvent('wheel', { bubbles: true })) scroller.dispatchEvent(new Event('scroll', { bubbles: true })) - // Switch away (saves conv-rw1's position) then back (should restore it). + // Switch away (saves conv-rw1; the captured anchor degenerates in jsdom), inject a real anchor, + // then return. rerender() - scrollToOffsetCalls.length = 0 - scrollTopSets.length = 0 + seedSavedAnchor('conv-rw1', 'msg-20') + getOffsetForMessageId.mockClear() + scrollToIndexCalls.length = 0 rerender() - // The restore re-windowed the virtualizer at the saved offset rather than only writing - // scrollTop. Without the fix scrollToOffset is never called and the viewport goes blank. - expect(scrollToOffsetCalls).toContain(200) + expect(scrollToIndexCalls).toContain('end') // windowed the anchor row in + expect(getOffsetForMessageId).toHaveBeenCalledWith('msg-20') // refined to the saved fraction }) - it('re-windows to the saved offset even when the anchor row is windowed out of the DOM', () => { - // Real-browser case: the virtualizer's initial window covers only the top rows; the saved - // anchor row is NOT in the DOM, so the DOM anchor lookup fails. Because the content height is - // unchanged since save (same-session return), restore takes the layout-unchanged path and - // re-windows to the EXACT saved offset via scrollToOffset — it must NOT go blank or fall back - // to pinVirtualizedBottom() (which cleared the saved state and stuck the convo at the bottom). + it('resolves the anchor through the virtualizer index when the anchor row is windowed out of the DOM', () => { + // Real-browser case: the virtualizer's initial window covers only the top rows; the saved anchor + // row is NOT in the DOM, so the DOM anchor lookup (restoreToAnchor) fails. The restore must then + // resolve the anchor through the virtualizer INDEX (getIndexForMessageId → scrollToIndex), + // re-deriving its position from measurements — it must NOT go blank or fall back to + // pinVirtualizedBottom() (which would clear the saved state and stick the convo at the bottom). const { container, rerender } = render( , ) @@ -672,35 +685,28 @@ describe('MessageList — virtualized bottom-stick re-asserts as rows measure', instrumentScroller(scroller, 5000) rafQueue.length = 0 - // User scrolls up and the hook captures the anchor + saved position. scroller.scrollTop = 200 - // A genuine user scroll fires a wheel (the save gate only persists user-driven positions, not - // media/measurement-induced shifts); a bare scroll event alone no longer counts. - scroller.dispatchEvent(new WheelEvent('wheel', { bubbles: true })) scroller.dispatchEvent(new Event('scroll', { bubbles: true })) - - // Switch away (saves conv-vi1's position and anchor). rerender() + seedSavedAnchor('conv-vi1', 'msg-20') - // Simulate windowed-out anchor: make querySelector return null for message rows - // (in a real browser the virtualizer's initial window doesn't include the saved anchor, - // so DOM lookup fails; here we force that failure by patching the method). + // Simulate windowed-out anchor: make querySelector return null for message rows so the DOM + // anchor lookup (restoreToAnchor) fails and the virtualizer-index path must take over. const origQS = scroller.querySelector.bind(scroller) as (sel: string) => Element | null scroller.querySelector = ((sel: string) => { if (sel.includes('message-row')) return null return origQS(sel) }) as typeof scroller.querySelector + getOffsetForMessageId.mockClear() scrollToIndexCalls.length = 0 - scrollToOffsetCalls.length = 0 - - // Return to conversation — restoreToAnchor fails, must use virtualizer-index path. rerender() - scroller.querySelector = origQS as typeof scroller.querySelector - // Re-windowed to the exact saved offset (200) rather than going blank or to the bottom. - expect(scrollToOffsetCalls).toContain(200) + // Resolved the anchor by index (re-windowing the row in) + refined by fraction, rather than a + // scroll-to-bottom. + expect(scrollToIndexCalls).toContain('end') + expect(getOffsetForMessageId).toHaveBeenCalledWith('msg-20') }) it('restores (re-windowed) when a message arrived in the conversation while it was hidden', () => { @@ -715,28 +721,24 @@ describe('MessageList — virtualized bottom-stick re-asserts as rows measure', , ) const scroller = container.querySelector('[data-message-list]') as HTMLElement - const { scrollTopSets } = instrumentScroller(scroller, 5000) + instrumentScroller(scroller, 5000) rafQueue.length = 0 - // Scroll up to 200 in the conversation and record the position + anchor. scroller.scrollTop = 200 - // A genuine user scroll fires a wheel (the save gate only persists user-driven positions, not - // media/measurement-induced shifts); a bare scroll event alone no longer counts. - scroller.dispatchEvent(new WheelEvent('wheel', { bubbles: true })) scroller.dispatchEvent(new Event('scroll', { bubbles: true })) - // Switch away (conversation is now hidden / unmounted from the user's view). + // Switch away (conversation is now hidden / unmounted), inject a real anchor. rerender() - scrollToOffsetCalls.length = 0 - scrollTopSets.length = 0 + seedSavedAnchor('conv-hidden', 'msg-20') + getOffsetForMessageId.mockClear() + scrollToIndexCalls.length = 0 - // Return to it AFTER a message arrived while it was hidden (51 messages now). The new - // message is appended at the bottom, so the saved scrolled-up position is still valid. + // Return AFTER a message arrived while hidden (51 messages now, appended at the bottom). rerender() - // Restored (and re-windowed) to the saved offset, not yanked to the new message at bottom. - expect(scrollToOffsetCalls).toContain(200) - expect(scrollTopSets).not.toContain(5000) + // Restore resolved the saved anchor rather than yanking to the new message at the bottom. + expect(getOffsetForMessageId).toHaveBeenCalledWith('msg-20') + expect(scrollToIndexCalls).toContain('end') }) it('re-windows the virtualizer when the composer grows (attachment / whisper / reply banner) while at bottom', () => { diff --git a/apps/fluux/src/components/conversation/useMessageListScroll.ts b/apps/fluux/src/components/conversation/useMessageListScroll.ts index 7f0d7c54..c44b5436 100644 --- a/apps/fluux/src/components/conversation/useMessageListScroll.ts +++ b/apps/fluux/src/components/conversation/useMessageListScroll.ts @@ -413,7 +413,7 @@ export function useMessageListScroll({ const mediaLoadDebounceRef = useRef | null>(null) // Last scroll data (for saving on conversation switch) - const lastScrollDataRef = useRef<{ top: number; height: number; client: number; width: number } | null>(null) + const lastScrollDataRef = useRef<{ top: number; height: number; client: number } | null>(null) // Bottom-most-visible message anchor, captured (throttled) during scroll so it // survives the conversation switch (at switch time the DOM is already the new // conversation). Used for content-stable position restoration on return. @@ -445,7 +445,6 @@ export function useMessageListScroll({ top: scroller.scrollTop, height: scroller.scrollHeight, client: scroller.clientHeight, - width: scroller.clientWidth, } lastAnchorRef.current = findBottomAnchor(scroller) }, []) @@ -459,7 +458,6 @@ export function useMessageListScroll({ top: bottomTop, height: scroller.scrollHeight, client: scroller.clientHeight, - width: scroller.clientWidth, } lastAnchorRef.current = findBottomAnchor(scroller) isAtBottomRef.current = true @@ -993,29 +991,16 @@ export function useMessageListScroll({ return true } - // Exact restore when the layout is UNCHANGED since save (same-session navigate-back, including - // eviction+rehydrate that reproduces identical content): the saved scrollTop is exact and - // layout-independent precisely because the content is the same, so the ratio-based anchor isn't - // needed — and a corrupt/degenerate anchor can't override a perfectly good saved position (the - // "jump to bottom on return" report). The fraction anchor below is for a CHANGED height (MAM - // prepend, width change). Tolerance covers sub-pixel measurement noise only. - const savedHeight = scrollStateManager.getSavedScrollHeight(conversationId) - const savedWidth = scrollStateManager.getSavedClientWidth(conversationId) - // A width change rewraps bubbles, so the absolute scrollTop is meaningless even if the total - // height coincidentally matches — only take the exact fast-path when the width is unchanged (or - // unknown, for legacy saves). Otherwise fall through to the rendering-independent fraction anchor. - const widthUnchanged = savedWidth === null || scroller.clientWidth === savedWidth - if ( - savedPos !== null && - savedHeight !== null && - widthUnchanged && - Math.abs(scroller.scrollHeight - savedHeight) <= 4 - ) { - if (virtRestore) virtRestore.scrollToOffset(savedPos) - else scroller.scrollTop = savedPos - return finishRestore('RESTORE via savedPos (layout unchanged)', { savedPos, savedHeight, savedWidth }) - } - + // THE CONTENT ANCHOR IS AUTHORITATIVE. The saved fraction anchor — re-derived from the anchor + // message's CURRENT measured height on every restore — is correct in every case: identical + // layout, MAM prepend, font-size change, view-density change, or a viewport-width change (which + // rewraps bubbles and so invalidates any saved pixel). We therefore try the anchor FIRST and + // UNCONDITIONALLY — no "layout unchanged" height/width gate. The saved scrollTop is only a pixel + // PROXY that holds when the layout is byte-identical; it is demoted to a last-resort fallback, + // used solely when there is NO usable anchor (a legacy save with no captured anchor, or an + // anchor message that is no longer in the loaded set). Pixel correctness is covered by the + // real-engine scroll-invariants e2e — jsdom/happy-dom have no layout, so the captured fraction + // degenerates (last row, fraction 1) and cannot be exercised in a unit test. if (savedAnchor && restoreToAnchor(scroller, savedAnchor)) { if (virtRestore) virtRestore.scrollToOffset(scroller.scrollTop) return finishRestore('RESTORE via anchor', { savedAnchor }) @@ -1463,7 +1448,7 @@ export function useMessageListScroll({ notifyUserInput() const el = e.currentTarget - const { scrollTop, scrollHeight, clientHeight, clientWidth } = el + const { scrollTop, scrollHeight, clientHeight } = el const distFromBottom = scrollHeight - scrollTop - clientHeight // A programmatic re-assert loop (marker positioning / pin-bottom / prepend / anchor restore) @@ -1471,7 +1456,7 @@ export function useMessageListScroll({ const programmaticScroll = reassertLoopRef.current !== null // Update refs (NO React state updates here except FAB) - lastScrollDataRef.current = { top: scrollTop, height: scrollHeight, client: clientHeight, width: clientWidth } + lastScrollDataRef.current = { top: scrollTop, height: scrollHeight, client: clientHeight } // Capture the bottom-most-visible anchor on every scroll event (binary search, // cheap) so it reflects the latest position — at switch time the DOM is already // the new conversation, so this must be captured live during scroll. @@ -1577,7 +1562,7 @@ export function useMessageListScroll({ const now = Date.now() if (!programmaticScroll && userHasScrolledSinceEntryRef.current && now - lastSaveTimeRef.current > SAVE_THROTTLE_MS) { lastSaveTimeRef.current = now - scrollStateManager.saveScrollPosition(conversationId, scrollTop, scrollHeight, clientHeight, lastAnchorRef.current ?? undefined, clientWidth) + scrollStateManager.saveScrollPosition(conversationId, scrollTop, scrollHeight, clientHeight, lastAnchorRef.current ?? undefined) } // Track if user scrolled away from top (allows re-trigger of load) @@ -1638,8 +1623,8 @@ export function useMessageListScroll({ // may have drifted from media/measurement after the restore, and persisting it would make the // position creep older on the next open (see userHasScrolledSinceEntryRef). if (prevConversationRef.current && lastScrollDataRef.current && userHasScrolledSinceEntryRef.current) { - const { top, height, client, width } = lastScrollDataRef.current - scrollStateManager.leaveConversation(prevConversationRef.current, top, height, client, lastAnchorRef.current ?? undefined, width) + const { top, height, client } = lastScrollDataRef.current + scrollStateManager.leaveConversation(prevConversationRef.current, top, height, client, lastAnchorRef.current ?? undefined) } else if (prevConversationRef.current) { scrollStateManager.markAsLeft(prevConversationRef.current) } @@ -1886,7 +1871,7 @@ export function useMessageListScroll({ // user genuinely scrolled this visit; otherwise keep the existing saved anchor (markAsLeft) // so a media/measurement-induced post-restore drift can't be saved (see the ref). if (lastScrollDataRef.current && userHasScrolledSinceEntryRef.current) { - const { top, height, client, width } = lastScrollDataRef.current + const { top, height, client } = lastScrollDataRef.current // UNMOUNT save: this is the leave path for navigations that DESTROY the message view — // opening Settings, switching DM↔Room, going back to the list. (DM↔DM keeps the view // mounted and saves via the conversation-switch effect instead.) If `top`/anchor here are @@ -1899,7 +1884,7 @@ export function useMessageListScroll({ anchorMessageId: lastAnchorRef.current?.messageId, anchorFraction: lastAnchorRef.current?.fraction, }) - scrollStateManager.leaveConversation(prevConversationRef.current, top, height, client, lastAnchorRef.current ?? undefined, width) + scrollStateManager.leaveConversation(prevConversationRef.current, top, height, client, lastAnchorRef.current ?? undefined) } else { // No scroll data, or the user never scrolled this visit → don't overwrite the saved // position; just mark the conversation left so a return is detected as a switch. diff --git a/apps/fluux/src/utils/scrollStateManager.test.ts b/apps/fluux/src/utils/scrollStateManager.test.ts index 0c66ab68..c9f66df7 100644 --- a/apps/fluux/src/utils/scrollStateManager.test.ts +++ b/apps/fluux/src/utils/scrollStateManager.test.ts @@ -145,18 +145,6 @@ describe('ScrollStateManager', () => { }) }) - describe('getSavedClientWidth', () => { - it('round-trips the saved viewport width (used to gate the exact-scrollTop restore)', () => { - manager.leaveConversation('conv1', 200, 1000, 100, { messageId: 'm', fraction: 0.5 }, 800) - expect(manager.getSavedClientWidth('conv1')).toBe(800) - }) - - it('returns null for a legacy save that did not capture width', () => { - manager.leaveConversation('conv1', 200, 1000, 100, { messageId: 'm', fraction: 0.5 }) - expect(manager.getSavedClientWidth('conv1')).toBeNull() - }) - }) - describe('updateMessageCount', () => { it('returns false for first update (no previous count)', () => { manager.enterConversation('conv1', 0) diff --git a/apps/fluux/src/utils/scrollStateManager.ts b/apps/fluux/src/utils/scrollStateManager.ts index b89479ab..6acf1e6d 100644 --- a/apps/fluux/src/utils/scrollStateManager.ts +++ b/apps/fluux/src/utils/scrollStateManager.ts @@ -45,20 +45,16 @@ export interface ScrollAnchor { } interface ScrollState { - /** Saved scroll position (scrollTop) */ + /** Saved scroll position (scrollTop). Last-resort fallback when no usable anchor was captured; + * the content-stable {@link ScrollState.anchor} is authoritative on restore. */ scrollTop: number /** Whether the user was at the bottom when leaving */ wasAtBottom: boolean /** Timestamp when the state was saved */ savedAt: number - /** Total scroll height when saved (for validation) */ - scrollHeight: number - /** Viewport width (clientWidth) when saved. The exact-scrollTop fast-path on restore is valid - * ONLY when BOTH height and width are unchanged — a width change rewraps bubbles (heights - * change), so the absolute scrollTop points at different content even if the total height - * coincidentally matches. On a width change the (rendering-independent) anchor governs instead. */ - clientWidth?: number - /** Content-stable anchor (preferred over scrollTop for restoration). */ + /** Content-stable anchor, authoritative for restoration: its pixel position is re-derived from + * the anchor message's CURRENT measured height, so it survives re-measure / width / density / + * font-size changes. {@link ScrollState.scrollTop} is only a fallback when this is absent. */ anchor?: ScrollAnchor } @@ -166,10 +162,12 @@ class ScrollStateManager { scrollTop: number, scrollHeight: number, clientHeight: number, - anchor?: ScrollAnchor, - clientWidth?: number + anchor?: ScrollAnchor ): void { const state = this.getState(conversationId) + // scrollHeight/clientHeight are transient inputs used only to decide wasAtBottom here — they are + // NOT stored: restore is driven by the content anchor (re-derived from current layout), so a + // saved pixel height would be meaningless after a re-measure / width / font-size change. const distanceFromBottom = scrollHeight - scrollTop - clientHeight const wasAtBottom = distanceFromBottom < AT_BOTTOM_THRESHOLD @@ -187,13 +185,11 @@ class ScrollStateManager { delete state.scrollState } else { // Save the scroll position for restoration. The anchor (bottom-most visible - // message) is preferred on restore; scrollTop is the legacy fallback. + // message) is authoritative on restore; scrollTop is the last-resort fallback. state.scrollState = { scrollTop, wasAtBottom, savedAt: Date.now(), - scrollHeight, - clientWidth, anchor, } } @@ -208,11 +204,10 @@ class ScrollStateManager { scrollTop: number, scrollHeight: number, clientHeight: number, - anchor?: ScrollAnchor, - clientWidth?: number + anchor?: ScrollAnchor ): void { // Save position first - this.saveScrollPosition(conversationId, scrollTop, scrollHeight, clientHeight, anchor, clientWidth) + this.saveScrollPosition(conversationId, scrollTop, scrollHeight, clientHeight, anchor) const state = this.getState(conversationId) this.log('leaveConversation', { @@ -261,39 +256,6 @@ class ScrollStateManager { return state.scrollState.scrollTop } - /** - * Get the scrollHeight captured at save time, or null when no restorable state. - * Lets the restore prefer the exact saved scrollTop when the content height is unchanged - * since save (same-session navigate-back): the pixel is then exact and layout-independent, - * so the (ratio-based) anchor is only needed when the height actually changed. - */ - getSavedScrollHeight(conversationId: string): number | null { - const state = this.states.get(conversationId) - if (!state?.scrollState || state.scrollState.wasAtBottom) { - return null - } - if (Date.now() - state.scrollState.savedAt > this.staleThresholdMs) { - return null - } - return state.scrollState.scrollHeight - } - - /** - * Get the viewport width captured at save time, or null when none/stale/legacy save. - * Used together with {@link getSavedScrollHeight} to confirm the layout is truly unchanged - * before taking the exact-scrollTop restore fast-path. - */ - getSavedClientWidth(conversationId: string): number | null { - const state = this.states.get(conversationId) - if (!state?.scrollState || state.scrollState.wasAtBottom) { - return null - } - if (Date.now() - state.scrollState.savedAt > this.staleThresholdMs) { - return null - } - return state.scrollState.clientWidth ?? null - } - /** * Get the saved scroll anchor (bottom-most visible message) for restoration. * Returns null when the user was at the bottom, the state is stale, or no diff --git a/scripts/scroll-invariants.ts b/scripts/scroll-invariants.ts index 265cfbfb..931851e8 100644 --- a/scripts/scroll-invariants.ts +++ b/scripts/scroll-invariants.ts @@ -854,6 +854,85 @@ test.describe('Virtualization scroll invariants', () => { ).toBeLessThan(250) }) + // ── 12: A relayout WHILE AWAY (viewport width + view density) holds the reading anchor ── + // + // Restore is driven by the CONTENT ANCHOR (the bottom-visible message + the fraction of its height + // at the viewport bottom), re-derived from each row's CURRENT measured height on return — so it is + // independent of the layout that existed at save time. This pins that contract across the two real + // relayout knobs a saved PIXEL cannot survive: a viewport-WIDTH change rewraps bubbles, and a + // DENSITY change re-pads every message group — both move absolute offsets (and the total height) out + // from under any saved scrollTop. After such a change while the conversation is away, returning must + // keep the SAME message in view at ~the same fractional position: not snapped to the bottom, not + // jumped to a stale pixel. + // + // This is the regression guard for making the anchor authoritative (PR removing the exact-scrollTop + // fast-path): the old fast-path gated on width, so it already deferred to the anchor on a width + // change — but a density change that left the total height ~unchanged could still mis-fire it onto + // the stale pixel. Removing it routes every relayout through the one correct (anchor) path. + test('invariant-12: a width + density change while away holds the reading anchor on return', async ({ page }) => { + await loadDemo(page) + await navigateToStressRoom(page) + + const distFromBottom = () => page.evaluate(() => { + const s = document.querySelector('[data-message-list]') as HTMLElement | null + return s ? Math.round(s.scrollHeight - s.scrollTop - s.clientHeight) : -1 + }) + + // Scroll up off the bottom to a mid-history reading position (real wheel so the virtualizer + // re-windows), then settle. + const box = await page.locator('[data-message-list]').first().boundingBox() + if (box) await page.mouse.move(box.x + box.width / 2, box.y + box.height / 2) + await page.mouse.wheel(0, -2500) + await page.waitForTimeout(700) + // The save fires on the scroll EVENT, at the row sizes the virtualizer had ESTIMATED then; rows + // re-measure over the next frames, shifting the visually-settled bottom-anchor. Nudge once more + // after the settle so the persisted anchor matches the SETTLED position we capture below + // (otherwise the test's reference diverges from what was saved — a harness artifact, not drift). + await page.mouse.wheel(0, -4) + await page.waitForTimeout(500) + expect(await distFromBottom(), 'precondition: must be scrolled up off the bottom').toBeGreaterThan(AT_BOTTOM_OK_PX) + + const before = await findBottomVisibleMessage(page) + expect(before, 'must capture a reading anchor before leaving').not.toBeNull() + const anchorId = before!.id + + // LEAVE the room (its mounted window unmounts). + await page.evaluate(() => { + // eslint-disable-next-line @typescript-eslint/no-explicit-any + void (window as any).__roomStore?.getState?.()?.activateRoom(null) + }) + await page.waitForTimeout(300) + + // RELAYOUT WHILE AWAY, via the two real layout knobs: narrow the viewport (rewraps bubbles) and + // flip the density to compact (re-pads every message group). Both move absolute offsets and the + // total height out from under any saved pixel; only the re-derived content anchor survives. 900px + // stays in the desktop layout (above the mobile breakpoint) so navigation is unchanged. + await page.setViewportSize({ width: 900, height: 800 }) + await page.evaluate(() => { + // eslint-disable-next-line @typescript-eslint/no-explicit-any + (window as any).__settingsStore?.getState?.()?.setDensityMode('compact') + }) + await page.waitForTimeout(200) + + // RETURN — restore must re-derive the anchor's pixel target from the NEW layout. + await navigateToStressRoom(page) + await page.waitForTimeout(1600) // activation + anchor re-assert settle at the new layout + + // (A) Did NOT snap to the bottom — the saved scrolled-up reading position was restored, not lost. + expect(await distFromBottom(), 'view snapped to the bottom after the relayout instead of holding the anchor').toBeGreaterThan(AT_BOTTOM_OK_PX) + + // (B) The SAME message (±2 as the rewrapped / re-padded rows settle) is still the bottom-visible + // content — the reading position held at the fold through a relayout that changed every row's + // height, i.e. it landed on the content anchor and NOT a stale saved pixel (which the larger row + // heights would have left showing much older content). The precise fractional offset is not + // asserted: a width rewrap can multiply the anchor message's own height, so its in-viewport + // fraction legitimately shifts even as the message itself stays pinned at the fold. + const after = await findBottomVisibleMessage(page) + expect(after, 'must capture a reading anchor after return').not.toBeNull() + const drift = Math.abs(stressMsgIndex(after!.id) - stressMsgIndex(anchorId)) + expect(drift, `bottom-visible anchor moved ${drift} messages across the relayout (before=${anchorId}, after=${after!.id})`).toBeLessThanOrEqual(2) + }) + }) // ── DIAGNOSTIC: new-message marker on re-entry (the user-reported bug) ──────────