From 2555101765b0a3f9a12a5ede12d734cde5731f79 Mon Sep 17 00:00:00 2001 From: chrisnojima Date: Wed, 9 Sep 2026 15:12:14 -0400 Subject: [PATCH 1/3] refactor(chat): one centering module, one closed loop "Center the thread on message X" was a five-module conversation with three representations of the target and three independent closed-loop scroll correctors. center-context derived an ordinal from a messageID by identity cast, which is not what the real resolver answers - a message you sent keeps the fractional ordinal it had in the outbox, so the cast highlighted a row that does not exist. thread-message-state held the real resolver and nobody called it. list-area held a desktop pixel-measuring loop, a native index-measuring loop, a second native corrector on a [50,250,500,900] timer ladder, and the pinnedToEnd/end-anchor pair, all as free refs. centering.tsx now owns it. centerOn(messageID, highlightMode) returns the outcome - 'centered' | 'clamped' | 'not-found' - and behind it sits one messageID -> ordinal resolution through getOrdinalForMessageID, the clear-and-reload through requestWindow({anchor: {centeredOn}}), and one corrector loop driven through one adapter interface: measureTarget, scrollToIndex, scrollToOffset. Exactly two adapters, LegendList and FlatList, both in list-area. The deadbands, the settle and clamp counts, the correction budget, the retry ladder and abort-on-user-scroll are ported, per adapter where they differ: desktop measures pixels against the DOM, native measures in index space against the viewable range and reports a stale range as pending rather than correcting off it. The end anchor and the who-owns-the- scroll state move in with them. search.tsx steps on the outcome. It advanced the "n of m" counter on !!message.id alone, so a hit the thread could not produce left the counter claiming a row that never rendered; a clamp - a hit within half a viewport of either end - was absorbed silently. The counter is now taken optimistically and handed back only on 'not-found'. The file had no tests. The corrector is now addressable without a DOM, so it has them, along with the end anchor, the outcomes through the provider, and the ordinal resolution that the identity cast used to get wrong. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_015rccpV5nLxxC5opF5xzrz7 --- .../chat/conversation/center-context.test.tsx | 138 ----- shared/chat/conversation/center-context.tsx | 159 ----- shared/chat/conversation/centering.test.tsx | 521 ++++++++++++++++ shared/chat/conversation/centering.tsx | 579 ++++++++++++++++++ .../conversation/input-area/input-state.tsx | 2 +- .../conversation/input-area/normal/index.tsx | 2 +- .../conversation/input-intent-store.test.ts | 2 +- .../chat/conversation/input-intent-store.tsx | 6 +- shared/chat/conversation/list-area/index.tsx | 482 +++++---------- .../conversation/list-area/jump-to-recent.tsx | 2 +- .../chat/conversation/messages/pin/index.tsx | 8 +- .../conversation/messages/text/wrapper.tsx | 6 +- .../conversation/normal/container.test.tsx | 10 +- shared/chat/conversation/normal/container.tsx | 8 +- shared/chat/conversation/normal/index.tsx | 2 +- shared/chat/conversation/pinned-message.tsx | 6 +- shared/chat/conversation/search.test.ts | 84 ++- shared/chat/conversation/search.tsx | 38 +- 18 files changed, 1393 insertions(+), 662 deletions(-) delete mode 100644 shared/chat/conversation/center-context.test.tsx delete mode 100644 shared/chat/conversation/center-context.tsx create mode 100644 shared/chat/conversation/centering.test.tsx create mode 100644 shared/chat/conversation/centering.tsx diff --git a/shared/chat/conversation/center-context.test.tsx b/shared/chat/conversation/center-context.test.tsx deleted file mode 100644 index 0829a64ffcff..000000000000 --- a/shared/chat/conversation/center-context.test.tsx +++ /dev/null @@ -1,138 +0,0 @@ -/** @jest-environment jsdom */ -/// -import * as React from 'react' -import * as T from '@/constants/types' -import {act, cleanup, render} from '@testing-library/react' -import {resetAllStores} from '@/util/zustand' - -const convX = T.Chat.conversationIDToKey(new Uint8Array([1, 2, 3, 4])) -const convY = T.Chat.conversationIDToKey(new Uint8Array([5, 6, 7, 8])) - -const mockRequestWindow = jest.fn() -const mockSetMarkReadBlocked = jest.fn() -let mockRouteParams: {threadSearch?: {query?: string}} | undefined - -// Both providers under test pull thread/engine plumbing they don't exercise here. -jest.mock('./thread-context', () => ({ - useConversationThreadSetMarkReadBlocked: () => mockSetMarkReadBlocked, - useConversationThreadStore: () => ({getState: () => ({})}), -})) -jest.mock('./send-actions', () => ({ - useConversationSendActions: () => ({sendGiphyResult: jest.fn(), sendMessage: jest.fn()}), -})) -jest.mock('@/engine/action-listener', () => ({useEngineActionListener: () => {}})) -jest.mock('./thread-window', () => ({useRequestWindow: () => mockRequestWindow})) -jest.mock('./thread-search-route', () => ({useChatThreadRouteParams: () => mockRouteParams})) - -import {ConversationCenterProvider, useConversationCenter} from './center-context' -import {ConversationInputProvider, useConversationInput} from './input-area/input-state' -import {setInputIntent, useInputIntentState} from './input-intent-store' - -let seenHighlightOrdinal: T.Chat.Ordinal | undefined -let seenUnsentText: string | undefined - -const Probe = () => { - const centeredHighlightOrdinal = useConversationCenter().centeredHighlightOrdinal - const unsentText = useConversationInput(s => s.unsentText) - // captured in an effect, not during render: assigning module state while rendering is the - // side effect react-hooks/globals rejects - React.useEffect(() => { - seenHighlightOrdinal = centeredHighlightOrdinal - seenUnsentText = unsentText - }) - return null -} - -// The real tree order: ConversationCenterProvider wraps ConversationInputProvider, so the input -// provider's consume effect runs FIRST. If either provider claimed the other's intent types, the -// input provider would silently eat every highlight. -const Tree = ({id}: {id: T.Chat.ConversationIDKey}) => ( - - - - - -) - -const highlight = (n: number) => ({messageID: T.Chat.numberToMessageID(n), type: 'highlight'}) as const - -beforeEach(() => { - mockRouteParams = undefined - seenHighlightOrdinal = undefined - seenUnsentText = undefined -}) - -afterEach(() => { - cleanup() - jest.clearAllMocks() - resetAllStores() -}) - -test('a highlight written before mount is consumed on mount', () => { - setInputIntent(convX, highlight(42)) - - render() - - expect(mockSetMarkReadBlocked).toHaveBeenCalledWith(true) - expect(mockRequestWindow).toHaveBeenCalledTimes(1) - expect(mockRequestWindow).toHaveBeenCalledWith({anchor: {centeredOn: T.Chat.numberToMessageID(42)}, reason: 'centered'}) - expect(seenHighlightOrdinal).toBe(T.Chat.numberToOrdinal(42)) - expect(useInputIntentState.getState().intents.has(convX)).toBe(false) -}) - -test('a highlight written after mount is delivered by the subscription', () => { - render() - expect(mockRequestWindow).not.toHaveBeenCalled() - - act(() => { - setInputIntent(convX, highlight(7)) - }) - - expect(mockRequestWindow).toHaveBeenCalledWith({anchor: {centeredOn: T.Chat.numberToMessageID(7)}, reason: 'centered'}) - expect(seenHighlightOrdinal).toBe(T.Chat.numberToOrdinal(7)) -}) - -// The old route-param path deduped on the messageID *value*, so jumping to a message you had -// already jumped to was a silent no-op. Delete-on-consume keys delivery to the write instead. -test('jumping twice to the same message centers both times', () => { - render() - - act(() => { - setInputIntent(convX, highlight(11)) - }) - act(() => { - setInputIntent(convX, highlight(11)) - }) - - expect(mockRequestWindow).toHaveBeenCalledTimes(2) - expect(mockRequestWindow).toHaveBeenNthCalledWith(2, {anchor: {centeredOn: T.Chat.numberToMessageID(11)}, reason: 'centered'}) -}) - -// The two-consumer collision the store's `types` filter exists for. -test('the input provider does not consume a highlight meant for the center provider', () => { - setInputIntent(convX, highlight(5)) - - render() - - expect(mockRequestWindow).toHaveBeenCalledWith({anchor: {centeredOn: T.Chat.numberToMessageID(5)}, reason: 'centered'}) - expect(seenUnsentText).toBeUndefined() -}) - -test('the center provider does not consume an injectText meant for the input provider', () => { - setInputIntent(convX, {text: 'hello', type: 'injectText'}) - - render() - - expect(seenUnsentText).toBe('hello') - expect(mockRequestWindow).not.toHaveBeenCalled() - expect(useInputIntentState.getState().intents.has(convX)).toBe(false) -}) - -test('a highlight for another conversation is left alone', () => { - setInputIntent(convY, highlight(3)) - - render() - - expect(mockRequestWindow).not.toHaveBeenCalled() - expect(useInputIntentState.getState().intents.get(convY)).toEqual(highlight(3)) -}) diff --git a/shared/chat/conversation/center-context.tsx b/shared/chat/conversation/center-context.tsx deleted file mode 100644 index 17dc2ae2945d..000000000000 --- a/shared/chat/conversation/center-context.tsx +++ /dev/null @@ -1,159 +0,0 @@ -import * as React from 'react' -import * as T from '@/constants/types' -import {consumeInputIntent, useInputIntentState} from './input-intent-store' -import {produce} from 'immer' -import {useChatThreadRouteParams} from './thread-search-route' -import {useConversationThreadSetMarkReadBlocked} from './thread-context' -import {useRequestWindow} from './thread-window' - -type CenterState = { - center: T.Chat.CenterOrdinal | undefined - threadSearchVisible: boolean -} - -type CenterStateContextType = { - centeredHighlightOrdinal: T.Chat.Ordinal | undefined - centeredOrdinal: T.Chat.Ordinal | undefined - hasCenter: boolean -} - -type CenterActionsContextType = { - centerOnMessage: (messageID: T.Chat.MessageID, highlightMode: T.Chat.CenterOrdinalHighlightMode) => void - clearCenter: () => void - jumpToRecent: () => void -} - -const missingContext = () => { - throw new Error('Missing ConversationCenterContext in the tree') -} - -// Split contexts: the state changes when centering/highlighting, the actions stay -// stable. Per-row consumers that only dispatch (e.g. reply-quote click) subscribe -// to actions only, so a highlight change doesn't re-render every row. -const CenterStateContext = React.createContext({ - centeredHighlightOrdinal: undefined, - centeredOrdinal: undefined, - hasCenter: false, -}) -CenterStateContext.displayName = 'ConversationCenterStateContext' - -const CenterActionsContext = React.createContext({ - centerOnMessage: missingContext, - clearCenter: missingContext, - jumpToRecent: missingContext, -}) -CenterActionsContext.displayName = 'ConversationCenterActionsContext' - -export const useConversationCenter = () => React.useContext(CenterStateContext) -export const useConversationCenterActions = () => React.useContext(CenterActionsContext) - -// The other half of the input-intent bus's type split: the input provider claims the other four -// types (input-area/input-state.tsx). Neither may claim the other's or whichever mounts first -// silently eats it. -const centerInputIntentTypes = ['highlight'] as const - -const stateForThreadSearchVisible = (state: CenterState, threadSearchVisible: boolean): CenterState => - produce(state, draft => { - if (draft.threadSearchVisible === threadSearchVisible) { - return - } - draft.threadSearchVisible = threadSearchVisible - if (threadSearchVisible && draft.center) { - draft.center.highlightMode = 'none' - } else { - draft.center = undefined - } - }) - -export const ConversationCenterProvider = function ConversationCenterProvider(p: { - children: React.ReactNode - id: T.Chat.ConversationIDKey -}) { - const {children, id} = p - const routeParams = useChatThreadRouteParams() - const threadSearchVisible = !!routeParams?.threadSearch - const requestWindow = useRequestWindow() - const setMarkReadBlocked = useConversationThreadSetMarkReadBlocked() - const [centerState, setCenterState] = React.useState(() => ({ - center: undefined, - threadSearchVisible, - })) - - const currentCenterState = stateForThreadSearchVisible(centerState, threadSearchVisible) - - const setCenterForMessage = ( - messageID: T.Chat.MessageID, - highlightMode: T.Chat.CenterOrdinalHighlightMode - ) => { - const ordinal = T.Chat.numberToOrdinal(T.Chat.messageIDToNumber(messageID)) - setCenterState(state => - produce(stateForThreadSearchVisible(state, threadSearchVisible), draft => { - draft.center = {highlightMode, ordinal} - }) - ) - } - - const clearCenter = () => { - setCenterState(state => - produce(stateForThreadSearchVisible(state, threadSearchVisible), draft => { - draft.center = undefined - }) - ) - } - - const centerOnMessage = (messageID: T.Chat.MessageID, highlightMode: T.Chat.CenterOrdinalHighlightMode) => { - setCenterForMessage(messageID, highlightMode) - requestWindow({anchor: {centeredOn: messageID}, reason: 'centered'}) - } - - const jumpToRecent = () => { - clearCenter() - requestWindow({anchor: 'newest', reason: 'jump to recent'}) - } - - React.useEffect(() => { - setMarkReadBlocked(threadSearchVisible) - return () => { - setMarkReadBlocked(false) - } - }, [setMarkReadBlocked, threadSearchVisible]) - - const applyHighlight = React.useEffectEvent((messageID: T.Chat.MessageID) => { - setMarkReadBlocked(true) - centerOnMessage(messageID, 'flash') - }) - // Same two delivery moments as ConversationInputProvider: consume whatever was written before - // we mounted, then a registered subscription for later writes. Not a selector hook - that would - // re-render this subtree, and enableFreeze defers render-driven subscriptions on mobile while a - // registered callback still runs. Delete-on-consume is the dedupe, so jumping twice to the same - // messageID centers twice. - React.useEffect(() => { - const consume = () => { - const intent = consumeInputIntent(id, centerInputIntentTypes) - if (intent) { - applyHighlight(intent.messageID) - } - } - consume() - return useInputIntentState.subscribe(consume) - }, [id]) - - const center = currentCenterState.center - const centeredHighlightOrdinal = center && center.highlightMode !== 'none' ? center.ordinal : undefined - const stateValue = { - centeredHighlightOrdinal, - centeredOrdinal: center?.ordinal, - hasCenter: !!center, - } - const actionsValue = { - centerOnMessage, - clearCenter, - jumpToRecent, - } - - return ( - - {children} - - ) -} diff --git a/shared/chat/conversation/centering.test.tsx b/shared/chat/conversation/centering.test.tsx new file mode 100644 index 000000000000..f005ed7f4dfe --- /dev/null +++ b/shared/chat/conversation/centering.test.tsx @@ -0,0 +1,521 @@ +/** @jest-environment jsdom */ +/// +import * as React from 'react' +import * as T from '@/constants/types' +import {act, cleanup, render} from '@testing-library/react' +import {makeMessageText} from '@/constants/chat/message' +import {resetAllStores} from '@/util/zustand' + +const convX = T.Chat.conversationIDToKey(new Uint8Array([1, 2, 3, 4])) +const convY = T.Chat.conversationIDToKey(new Uint8Array([5, 6, 7, 8])) + +const mockRequestWindow = jest.fn() +const mockSetMarkReadBlocked = jest.fn() +let mockRouteParams: {threadSearch?: {query?: string}} | undefined + +// A loaded window, so the module's messageID -> ordinal resolution has something real to resolve +// against. 33 deliberately does NOT sit on its own message ID: that is what a message you sent +// looks like once it keeps the fractional ordinal it had in the outbox, and it is the case an +// identity cast from messageID to ordinal gets wrong. +const seededMessages = [ + {id: 3, ordinal: 3}, + {id: 5, ordinal: 5}, + {id: 7, ordinal: 7}, + {id: 11, ordinal: 11}, + {id: 33, ordinal: 32.001}, + {id: 42, ordinal: 42}, +] +const mockSnapshot = { + messageIDToOrdinal: new Map( + seededMessages.map(m => [T.Chat.numberToMessageID(m.id), T.Chat.numberToOrdinal(m.ordinal)]) + ), + messageMap: new Map( + seededMessages.map(m => [ + T.Chat.numberToOrdinal(m.ordinal), + makeMessageText({ + conversationIDKey: convX, + id: T.Chat.numberToMessageID(m.id), + ordinal: T.Chat.numberToOrdinal(m.ordinal), + }), + ]) + ), + messageOrdinals: seededMessages.map(m => T.Chat.numberToOrdinal(m.ordinal)), + moreToLoadForward: false, + pendingOutboxToOrdinal: new Map(), +} + +// Both providers under test pull thread/engine plumbing they don't exercise here. +jest.mock('./thread-context', () => ({ + useConversationThreadSelector: (selector: (s: unknown) => unknown) => selector(mockSnapshot), + useConversationThreadSetMarkReadBlocked: () => mockSetMarkReadBlocked, + useConversationThreadStore: () => ({getState: () => mockSnapshot}), +})) +jest.mock('./send-actions', () => ({ + useConversationSendActions: () => ({sendGiphyResult: jest.fn(), sendMessage: jest.fn()}), +})) +jest.mock('@/engine/action-listener', () => ({useEngineActionListener: () => {}})) +jest.mock('./thread-window', () => ({useRequestWindow: () => mockRequestWindow})) +jest.mock('./thread-search-route', () => ({useChatThreadRouteParams: () => mockRouteParams})) + +import { + type CenterMeasurement, + type CenterOutcome, + type CenterScrollAdapter, + ConversationCenteringProvider, + runCenterCorrection, + runEndAnchorCorrection, + useConversationCenter, + useConversationCenterActions, + useConversationCenterScroll, +} from './centering' +import {ConversationInputProvider, useConversationInput} from './input-area/input-state' +import {setInputIntent, useInputIntentState} from './input-intent-store' + +let seenHighlightOrdinal: T.Chat.Ordinal | undefined +let seenUnsentText: string | undefined + +const Probe = () => { + const centeredHighlightOrdinal = useConversationCenter().centeredHighlightOrdinal + const unsentText = useConversationInput(s => s.unsentText) + // captured in an effect, not during render: assigning module state while rendering is the + // side effect react-hooks/globals rejects + React.useEffect(() => { + seenHighlightOrdinal = centeredHighlightOrdinal + seenUnsentText = unsentText + }) + return null +} + +// The real tree order: ConversationCenteringProvider wraps ConversationInputProvider, so the input +// provider's consume effect runs FIRST. If either provider claimed the other's intent types, the +// input provider would silently eat every highlight. +const Tree = ({id}: {id: T.Chat.ConversationIDKey}) => ( + + + + + +) + +const highlight = (n: number) => ({messageID: T.Chat.numberToMessageID(n), type: 'highlight'}) as const + +beforeEach(() => { + mockRouteParams = undefined + seenHighlightOrdinal = undefined + seenUnsentText = undefined +}) + +afterEach(() => { + cleanup() + jest.clearAllMocks() + resetAllStores() +}) + +test('a highlight written before mount is consumed on mount', () => { + setInputIntent(convX, highlight(42)) + + render() + + expect(mockSetMarkReadBlocked).toHaveBeenCalledWith(true) + expect(mockRequestWindow).toHaveBeenCalledTimes(1) + expect(mockRequestWindow).toHaveBeenCalledWith({anchor: {centeredOn: T.Chat.numberToMessageID(42)}, reason: 'centered'}) + expect(seenHighlightOrdinal).toBe(T.Chat.numberToOrdinal(42)) + expect(useInputIntentState.getState().intents.has(convX)).toBe(false) +}) + +test('a highlight written after mount is delivered by the subscription', () => { + render() + expect(mockRequestWindow).not.toHaveBeenCalled() + + act(() => { + setInputIntent(convX, highlight(7)) + }) + + expect(mockRequestWindow).toHaveBeenCalledWith({anchor: {centeredOn: T.Chat.numberToMessageID(7)}, reason: 'centered'}) + expect(seenHighlightOrdinal).toBe(T.Chat.numberToOrdinal(7)) +}) + +// The old route-param path deduped on the messageID *value*, so jumping to a message you had +// already jumped to was a silent no-op. Delete-on-consume keys delivery to the write instead. +test('jumping twice to the same message centers both times', () => { + render() + + act(() => { + setInputIntent(convX, highlight(11)) + }) + act(() => { + setInputIntent(convX, highlight(11)) + }) + + expect(mockRequestWindow).toHaveBeenCalledTimes(2) + expect(mockRequestWindow).toHaveBeenNthCalledWith(2, {anchor: {centeredOn: T.Chat.numberToMessageID(11)}, reason: 'centered'}) +}) + +// The two-consumer collision the store's `types` filter exists for. +test('the input provider does not consume a highlight meant for the center provider', () => { + setInputIntent(convX, highlight(5)) + + render() + + expect(mockRequestWindow).toHaveBeenCalledWith({anchor: {centeredOn: T.Chat.numberToMessageID(5)}, reason: 'centered'}) + expect(seenUnsentText).toBeUndefined() +}) + +test('the center provider does not consume an injectText meant for the input provider', () => { + setInputIntent(convX, {text: 'hello', type: 'injectText'}) + + render() + + expect(seenUnsentText).toBe('hello') + expect(mockRequestWindow).not.toHaveBeenCalled() + expect(useInputIntentState.getState().intents.has(convX)).toBe(false) +}) + +test('a highlight for another conversation is left alone', () => { + setInputIntent(convY, highlight(3)) + + render() + + expect(mockRequestWindow).not.toHaveBeenCalled() + expect(useInputIntentState.getState().intents.get(convY)).toEqual(highlight(3)) +}) + +// The header a search hit is really at. A message you sent keeps the fractional ordinal it had in +// the outbox, so its server message ID is not the number its row lives at - the case the old +// messageID-as-ordinal cast got wrong, silently highlighting a row that does not exist. +test('the centered ordinal is resolved through the window, not cast from the message id', () => { + setInputIntent(convX, highlight(33)) + + render() + + expect(seenHighlightOrdinal).toBe(T.Chat.numberToOrdinal(32.001)) +}) + +// ==================== the scroll corrector ==================== + +// A list that answers a fixed script of measurements, so the loop's own rules - the deadband, the +// settle count, the clamp detection, the correction cap - are what is under test rather than a +// scroller. +const scriptedAdapter = ( + script: ReadonlyArray, + over: Partial = {} +) => { + const calls = {scrollToIndex: 0, scrollToOffset: new Array()} + let next = 0 + const adapter: CenterScrollAdapter = { + measureTarget: () => script[Math.min(next++, script.length - 1)]!, + scrollToIndex: () => { + calls.scrollToIndex += 1 + }, + scrollToOffset: offset => { + calls.scrollToOffset.push(offset) + }, + ...over, + } + return {adapter, calls} +} + +const measured = (offBy: number, scroll: number, tolerance = 8): CenterMeasurement => ({ + kind: 'measured', + offBy, + scroll, + tolerance, +}) + +const immediately = async () => Promise.resolve() +const ordinal = T.Chat.numberToOrdinal(101) + +describe('the centering scroll corrector', () => { + const run = async (adapter: CenterScrollAdapter, signal = {cancelled: false}) => + runCenterCorrection({adapter, ordinal, signal, sleep: immediately}) + + test('a row already inside the deadband is centered without touching the scroller', async () => { + const {adapter, calls} = scriptedAdapter([measured(4, 500)]) + + await expect(run(adapter)).resolves.toBe('centered') + expect(calls.scrollToOffset).toEqual([]) + }) + + test('one reading inside the deadband is not enough', async () => { + // A single reading can be the frame before a row above the target re-measures and moves it + // again, so the loop wants three in a row. + const {adapter, calls} = scriptedAdapter([ + measured(4, 500), + measured(40, 500), + measured(0, 540), + measured(0, 540), + measured(0, 540), + ]) + + await expect(run(adapter)).resolves.toBe('centered') + expect(calls.scrollToOffset).toEqual([540]) + }) + + test('it corrects by the measured offset and settles once the row holds still', async () => { + const {adapter, calls} = scriptedAdapter([ + measured(120, 1000), + measured(30, 1120), + measured(2, 1150), + measured(2, 1150), + measured(2, 1150), + ]) + + await expect(run(adapter)).resolves.toBe('centered') + expect(calls.scrollToOffset).toEqual([1120, 1150]) + }) + + test('a hit the scroller cannot reach clamps instead of spinning', async () => { + // A hit within half a viewport of either end of the thread: the offset we ask for gets clamped, + // the scroll position does not move, and the row never reaches the middle. + const {adapter, calls} = scriptedAdapter([measured(120, 0)]) + + await expect(run(adapter)).resolves.toBe('clamped') + // One correction, then three readings that showed it changed nothing. + expect(calls.scrollToOffset).toEqual([120]) + }) + + test('a row outside the rendered window is scrolled to by index first', async () => { + const {adapter, calls} = scriptedAdapter([ + {kind: 'offscreen'}, + {kind: 'offscreen'}, + measured(0, 700), + measured(0, 700), + measured(0, 700), + ]) + + await expect(run(adapter)).resolves.toBe('centered') + expect(calls.scrollToIndex).toBe(2) + expect(calls.scrollToOffset).toEqual([]) + }) + + test('a stale reading is waited out rather than corrected against', async () => { + // The native list only reports a viewable range when it moves, and correcting twice off the + // same reading overshoots. + const {adapter, calls} = scriptedAdapter([ + {kind: 'pending'}, + {kind: 'pending'}, + measured(60, 300), + measured(0, 360), + measured(0, 360), + measured(0, 360), + ]) + + await expect(run(adapter)).resolves.toBe('centered') + expect(calls.scrollToOffset).toEqual([360]) + }) + + test('a reader who takes the scroll stops the loop where it is', async () => { + // The loop re-centers for up to ~3s; someone scrolling in that window must win. + const signal = {cancelled: false} + let reads = 0 + const {adapter, calls} = scriptedAdapter([measured(120, 1000)], { + measureTarget: () => { + if (++reads === 2) { + signal.cancelled = true + } + return measured(120, 1000 + reads * 10) + }, + }) + + await expect(run(adapter, signal)).resolves.toBe('clamped') + expect(calls.scrollToOffset.length).toBeLessThanOrEqual(2) + }) + + test('a list with a correction budget stops at it', async () => { + // The native corrector has always been capped: an inverted list of tall image rows can chase a + // moving target indefinitely otherwise. + let scroll = 0 + const {adapter, calls} = scriptedAdapter([], { + maxCorrections: 3, + measureTarget: () => measured(50, (scroll += 10)), + }) + + await expect(run(adapter)).resolves.toBe('clamped') + expect(calls.scrollToOffset).toHaveLength(3) + }) + + test('a target that never settles gives up rather than running forever', async () => { + let scroll = 0 + const {adapter, calls} = scriptedAdapter([], {measureTarget: () => measured(50, (scroll += 10))}) + + await expect(run(adapter)).resolves.toBe('clamped') + // 3000ms of budget at one 50ms poll per correction. + expect(calls.scrollToOffset).toHaveLength(60) + }) +}) + +describe('the end anchor corrector', () => { + const run = async ( + read: () => {isAtEnd: boolean; scroll: number} | undefined, + scrollToEnd: () => void, + holdsEndAnchor: () => boolean = () => true + ) => + runEndAnchorCorrection({ + endAnchor: {read, scrollToEnd}, + holdsEndAnchor, + signal: {cancelled: false}, + sleep: immediately, + }) + + test('a list already at its end is left alone', async () => { + const scrollToEnd = jest.fn() + await run(() => ({isAtEnd: true, scroll: 900}), scrollToEnd) + expect(scrollToEnd).not.toHaveBeenCalled() + }) + + test('it waits for the offset to hold still before correcting', async () => { + // The header often settles while the list is still running its own initial scroll, and a + // scroll-to-end issued against that becomes the target the list abandons its bootstrap for. + const scrolls = [100, 200, 300, 300] + let reads = 0 + const scrollToEnd = jest.fn() + await run(() => { + const scroll = scrolls[Math.min(reads++, scrolls.length - 1)]! + return {isAtEnd: false, scroll} + }, scrollToEnd) + expect(scrollToEnd).toHaveBeenCalled() + // Two corrections is the whole budget: one for the header, one for whatever re-measured + // alongside it. + expect(scrollToEnd).toHaveBeenCalledTimes(2) + }) + + test('it stops as soon as the end stops being ours to hold', async () => { + const scrollToEnd = jest.fn() + await run( + () => ({isAtEnd: false, scroll: 300}), + scrollToEnd, + () => false + ) + expect(scrollToEnd).not.toHaveBeenCalled() + }) +}) + +// ==================== outcomes, through the provider ==================== + +const alwaysCentered: CenterScrollAdapter = { + measureTarget: () => measured(0, 0), + scrollToIndex: () => {}, + scrollToOffset: () => {}, +} + +let outcome: CenterOutcome | undefined +const CenterHarness = (p: {adapter?: CenterScrollAdapter; messageID: number}) => { + const {adapter, messageID} = p + const {centerOn} = useConversationCenterActions() + const {registerAdapter} = useConversationCenterScroll() + React.useEffect(() => { + registerAdapter(adapter) + }, [adapter, registerAdapter]) + React.useEffect(() => { + outcome = undefined + void centerOn(T.Chat.numberToMessageID(messageID), 'flash').then(o => { + outcome = o + }) + }, [centerOn, messageID]) + return null +} + +describe('centerOn reports what actually happened', () => { + beforeEach(() => { + jest.useFakeTimers() + }) + afterEach(() => { + jest.useRealTimers() + }) + + const drain = async (ms: number) => { + for (let elapsed = 0; elapsed < ms; elapsed += 50) { + await act(async () => { + jest.advanceTimersByTime(50) + await Promise.resolve() + }) + } + } + + test('a message the thread came back with, and a list that can centre it, is centered', async () => { + render( + + + + ) + await drain(500) + expect(outcome).toBe('centered') + expect(mockRequestWindow).toHaveBeenCalledWith({ + anchor: {centeredOn: T.Chat.numberToMessageID(42)}, + reason: 'centered', + }) + }) + + test('a message the thread never came back with is reported as not found', async () => { + // The `n of m` counter used to advance for these anyway, because the only thing asked was + // whether the hit had an id at all. + render( + + + + ) + await drain(3300) + expect(outcome).toBe('not-found') + }) + + test('a list that cannot reach the row reports a clamp rather than a success', async () => { + const pinned: CenterScrollAdapter = { + measureTarget: () => measured(500, 0), + scrollToIndex: () => {}, + scrollToOffset: () => {}, + } + render( + + + + ) + await drain(500) + expect(outcome).toBe('clamped') + }) +}) + +let repeatOutcomes: Array = [] +const RepeatHarness = (p: {adapter: CenterScrollAdapter; messageID: number}) => { + const {adapter, messageID} = p + const {centerOn} = useConversationCenterActions() + const {registerAdapter} = useConversationCenterScroll() + React.useEffect(() => { + registerAdapter(adapter) + }, [adapter, registerAdapter]) + React.useEffect(() => { + repeatOutcomes = [] + const run = async () => { + repeatOutcomes.push(await centerOn(T.Chat.numberToMessageID(messageID), 'flash')) + repeatOutcomes.push(await centerOn(T.Chat.numberToMessageID(messageID), 'flash')) + } + void run() + }, [centerOn, messageID]) + return null +} + +describe('centering twice on the same row', () => { + beforeEach(() => { + jest.useFakeTimers() + }) + afterEach(() => { + jest.useRealTimers() + }) + + // The second jump reloads the thread just like the first, so it has to steer the list again - + // and answer. A guard keyed on the ordinal alone leaves the second request hanging until its + // not-found watchdog fires, which hands the search counter back for a hit that was right there. + test('the second request is steered and answered too', async () => { + render( + + + + ) + for (let elapsed = 0; elapsed < 1000; elapsed += 50) { + await act(async () => { + jest.advanceTimersByTime(50) + await Promise.resolve() + }) + } + expect(repeatOutcomes).toEqual>(['centered', 'centered']) + }) +}) diff --git a/shared/chat/conversation/centering.tsx b/shared/chat/conversation/centering.tsx new file mode 100644 index 000000000000..d55f93e00da2 --- /dev/null +++ b/shared/chat/conversation/centering.tsx @@ -0,0 +1,579 @@ +import * as React from 'react' +import type * as T from '@/constants/types' +import {consumeInputIntent, useInputIntentState} from './input-intent-store' +import {getOrdinalForMessageIDInSnapshot} from './thread-load' +import {produce} from 'immer' +import sortedIndexOf from 'lodash/sortedIndexOf' +import {useChatThreadRouteParams} from './thread-search-route' +import {useRequestWindow} from './thread-window' +import { + useConversationThreadSelector, + useConversationThreadSetMarkReadBlocked, + useConversationThreadStore, +} from './thread-context' + +// What centering can end up doing. 'clamped' is a real outcome, not a failure: a hit within half a +// viewport of either end of the thread cannot be put in the middle, and neither can one the reader +// scrolls away from while we are still correcting. 'not-found' means the thread came back without +// the message at all - the only outcome a caller should read as "this hit is unreachable". +export type CenterOutcome = 'centered' | 'clamped' | 'not-found' + +// Where the target sits relative to the middle of the viewport, as the list can see it. +export type CenterMeasurement = + // `offBy` is in scroller pixels, already damped by whatever this list needs to converge without + // oscillating; `scroll` is the offset it was measured at; `tolerance` is how close this list can + // realistically get, below which chasing the remainder only fights the list's own adjustments. + | {kind: 'measured'; offBy: number; scroll: number; tolerance: number} + // The row is outside the rendered window, so there is nothing to measure until it mounts. + | {kind: 'offscreen'} + // Measurable, but not against anything current: the list has not reported a fresh position since + // the last correction, and correcting off a stale one overshoots. + | {kind: 'pending'} + +// Everything the corrector needs from a list. Two implementations, one per platform list, both in +// list-area: LegendList on desktop and FlatList on native. +export type CenterScrollAdapter = { + // Only a list that can report its own end takes part in the end anchor. The native list cannot, + // and never ran that correction. + endAnchor?: { + read: () => {isAtEnd: boolean; scroll: number} | undefined + scrollToEnd: () => void + } + // How many corrections this list may issue before giving up. Left unset the loop is bounded by + // its settle, clamp and timeout checks alone. + maxCorrections?: number + measureTarget: (ordinal: T.Chat.Ordinal) => CenterMeasurement + // Coarse: it lands at the wrong offset for variable-height rows, but it gets the row mounted so + // measureTarget can see it. + scrollToIndex: (ordinal: T.Chat.Ordinal) => void + scrollToOffset: (offset: number) => void +} + +// Closed loop, not one shot: rows enter at an estimated size and only settle as they measure, so the +// first scroll lands off by however wrong the estimates above the target were. Measure the row's +// real offset from the viewport centre and correct until it holds still, then get out of the way - +// the list's own maintain-visible-content-position owns the offset from then on, and two controllers +// fighting over one scroll offset oscillate. +const centerTimeoutMs = 3000 +const measurePollMs = 50 +const mountPollMs = 100 +// Three readings inside the deadband, not one: a single one can be the frame before a row above the +// target re-measures and moves it again. +const settledChecks = 3 +// A hit near either end of the thread cannot be centred: the offset we ask for gets clamped and the +// row never reaches the middle. Three corrections that moved the scroll position not at all mean we +// are pinned against an edge - stop rather than spin. +const pinnedChecksToClamp = 3 + +export const runCenterCorrection = async (p: { + adapter: CenterScrollAdapter + ordinal: T.Chat.Ordinal + signal: {cancelled: boolean} + sleep: (ms: number) => Promise +}): Promise => { + const {adapter, ordinal, signal, sleep} = p + let settled = 0 + let pinnedChecks = 0 + let corrections = 0 + let scrollAtLastRequest: number | undefined + for (let elapsed = 0; elapsed < centerTimeoutMs && !signal.cancelled; ) { + const measurement = adapter.measureTarget(ordinal) + if (measurement.kind === 'offscreen') { + adapter.scrollToIndex(ordinal) + settled = 0 + pinnedChecks = 0 + await sleep(mountPollMs) + elapsed += mountPollMs + continue + } + if (measurement.kind === 'pending') { + await sleep(measurePollMs) + elapsed += measurePollMs + continue + } + const {offBy, scroll, tolerance} = measurement + if (Math.abs(offBy) <= tolerance) { + pinnedChecks = 0 + // Only the iteration right after a correction can diagnose a clamp. + scrollAtLastRequest = undefined + if (++settled >= settledChecks) { + return 'centered' + } + } else if (scroll === scrollAtLastRequest) { + if (++pinnedChecks >= pinnedChecksToClamp) { + return 'clamped' + } + } else if (adapter.maxCorrections !== undefined && corrections >= adapter.maxCorrections) { + return 'clamped' + } else { + corrections += 1 + pinnedChecks = 0 + scrollAtLastRequest = scroll + adapter.scrollToOffset(scroll + offBy) + } + await sleep(measurePollMs) + elapsed += measurePollMs + } + return 'clamped' +} + +// The list resolves its initial end target from the header size it has measured so far, and the +// thread's intro content (retention notice, new-chat card, the "digging" spinner) lands after that. +// The list re-pins on a data, item, footer or viewport layout change but has no header trigger, so a +// header that grows after the target resolved leaves the list short by exactly that growth with +// nothing to correct it. +// +// Closed loop rather than a correction fired straight from the size change: the header often settles +// while the thread is still empty, and a scroll-to-end issued against that near-empty content +// becomes the target the list then abandons its own bootstrap for, landing anywhere. Wait for the +// scroll offset to hold still, so the list has finished its own initial scroll, and only then +// correct what it left on the table. +const endAnchorTimeoutMs = 2000 +// Two corrections is the whole budget: one for the header, one for whatever re-measured alongside +// it. Past that we would be fighting something that owns the offset. +const maxEndAnchorCorrections = 2 + +export const runEndAnchorCorrection = async (p: { + endAnchor: NonNullable + holdsEndAnchor: () => boolean + signal: {cancelled: boolean} + sleep: (ms: number) => Promise +}): Promise => { + const {endAnchor, holdsEndAnchor, signal, sleep} = p + let previousScroll: number | undefined + let corrections = 0 + for (let elapsed = 0; elapsed < endAnchorTimeoutMs && !signal.cancelled && holdsEndAnchor(); ) { + await sleep(measurePollMs) + elapsed += measurePollMs + const state = endAnchor.read() + if (!state) { + continue + } + if (state.isAtEnd) { + return + } + // Only a scroll offset that held still across two checks means the list is done moving. + if (state.scroll === previousScroll) { + if (++corrections > maxEndAnchorCorrections) { + return + } + endAnchor.scrollToEnd() + previousScroll = undefined + } else { + previousScroll = state.scroll + } + } +} + +// Who owns the scroll offset. The end anchor and the centering loop both drive it, and a reader who +// touches the list takes it from both - a correction that yanks someone away from where they landed +// is the failure both loops are bounded to avoid. +type ScrollOwner = 'center' | 'end' | 'reader' + +type CenterTarget = {highlightMode: T.Chat.CenterOrdinalHighlightMode; messageID: T.Chat.MessageID} + +type CenterState = { + target: CenterTarget | undefined + threadSearchVisible: boolean +} + +type CenterStateContextType = { + centeredHighlightOrdinal: T.Chat.Ordinal | undefined + centeredOrdinal: T.Chat.Ordinal | undefined + hasCenter: boolean +} + +type CenterActionsContextType = { + centerOn: ( + messageID: T.Chat.MessageID, + highlightMode: T.Chat.CenterOrdinalHighlightMode + ) => Promise + clearCenter: () => void + jumpToRecent: () => void +} + +// What a list registers so the module can steer it, plus the end-anchor state that used to sit +// beside it as free refs. +type CenterScrollContextType = { + endMayHaveMoved: () => void + holdsEndAnchor: () => boolean + readerTookScroll: () => void + registerAdapter: (adapter: CenterScrollAdapter | undefined) => void + takeEndAnchor: () => void +} + +const missingContext = () => { + throw new Error('Missing ConversationCenteringProvider in the tree') +} + +// Split contexts: the state changes when centering/highlighting, the actions stay +// stable. Per-row consumers that only dispatch (e.g. reply-quote click) subscribe +// to actions only, so a highlight change doesn't re-render every row. +const CenterStateContext = React.createContext({ + centeredHighlightOrdinal: undefined, + centeredOrdinal: undefined, + hasCenter: false, +}) +CenterStateContext.displayName = 'ConversationCenterStateContext' + +const CenterActionsContext = React.createContext({ + centerOn: missingContext, + clearCenter: missingContext, + jumpToRecent: missingContext, +}) +CenterActionsContext.displayName = 'ConversationCenterActionsContext' + +const CenterScrollContext = React.createContext({ + endMayHaveMoved: missingContext, + holdsEndAnchor: missingContext, + readerTookScroll: missingContext, + registerAdapter: missingContext, + takeEndAnchor: missingContext, +}) +CenterScrollContext.displayName = 'ConversationCenterScrollContext' + +export const useConversationCenter = () => React.useContext(CenterStateContext) +export const useConversationCenterActions = () => React.useContext(CenterActionsContext) +export const useConversationCenterScroll = () => React.useContext(CenterScrollContext) + +// The other half of the input-intent bus's type split: the input provider claims the other four +// types (input-area/input-state.tsx). Neither may claim the other's or whichever mounts first +// silently eats it. +const centerInputIntentTypes = ['highlight'] as const + +const stateForThreadSearchVisible = (state: CenterState, threadSearchVisible: boolean): CenterState => + produce(state, draft => { + if (draft.threadSearchVisible === threadSearchVisible) { + return + } + draft.threadSearchVisible = threadSearchVisible + if (threadSearchVisible && draft.target) { + draft.target.highlightMode = 'none' + } else { + draft.target = undefined + } + }) + +const sleep = async (ms: number) => new Promise(resolve => setTimeout(resolve, ms)) + +// The in-flight centring request, if a caller is waiting on its outcome. `corrected` is what keeps +// the not-found watchdog from answering for a request the corrector has already taken over. +type PendingCenter = { + corrected: boolean + messageID: T.Chat.MessageID + settle: (outcome: CenterOutcome) => void +} + +type ScrollControl = { + adapter: CenterScrollAdapter | undefined + correction: {cancelled: boolean} | undefined + endAnchor: {cancelled: boolean} | undefined + owner: ScrollOwner + pending: PendingCenter | undefined +} + +export const ConversationCenteringProvider = function ConversationCenteringProvider(p: { + children: React.ReactNode + id: T.Chat.ConversationIDKey +}) { + const {children, id} = p + const routeParams = useChatThreadRouteParams() + const threadSearchVisible = !!routeParams?.threadSearch + const requestWindow = useRequestWindow() + const setMarkReadBlocked = useConversationThreadSetMarkReadBlocked() + const store = useConversationThreadStore() + const [centerState, setCenterState] = React.useState(() => ({ + target: undefined, + threadSearchVisible, + })) + + const currentCenterState = stateForThreadSearchVisible(centerState, threadSearchVisible) + const target = currentCenterState.target + + // The one messageID -> ordinal resolution. A message you sent keeps the fractional ordinal it had + // in the outbox, so the ordinal it lives at is not the number its server ID makes; asking the + // window is the only way to get the right one. The row has to be in the window as well as in the + // map: a message the thread holds but does not render has nothing to scroll to, and reporting its + // ordinal would send the corrector after a row that never mounts. + const centeredOrdinal = useConversationThreadSelector(s => { + if (!target) { + return undefined + } + const found = getOrdinalForMessageIDInSnapshot(s, target.messageID) + if (found === null) { + return undefined + } + const ordinals = s.messageOrdinals + return ordinals && sortedIndexOf(ordinals as unknown as number[], found as unknown as number) >= 0 + ? found + : undefined + }) + + const scrollRef = React.useRef({ + adapter: undefined, + correction: undefined, + endAnchor: undefined, + owner: 'end', + pending: undefined, + }) + + const settlePending = React.useEffectEvent((outcome: CenterOutcome) => { + const scroll = scrollRef.current + const pending = scroll.pending + if (!pending) { + return + } + scroll.pending = undefined + pending.settle(outcome) + }) + const takeCentering = React.useEffectEvent(() => { + const scroll = scrollRef.current + scroll.owner = 'center' + if (scroll.pending) { + scroll.pending.corrected = true + } + }) + const abortCorrection = React.useEffectEvent(() => { + const {correction} = scrollRef.current + if (correction) { + correction.cancelled = true + } + }) + const abortEverything = React.useEffectEvent(() => { + const scroll = scrollRef.current + if (scroll.correction) { + scroll.correction.cancelled = true + } + if (scroll.endAnchor) { + scroll.endAnchor.cancelled = true + } + settlePending('clamped') + }) + React.useEffect(() => () => abortEverything(), []) + + const endMayHaveMoved = React.useEffectEvent(() => { + const scroll = scrollRef.current + const endAnchor = scroll.adapter?.endAnchor + // Only once there are messages: the header frequently settles while the thread is still empty, + // and there is no end to hold yet. + if (!endAnchor || scroll.owner !== 'end' || !store.getState().messageOrdinals?.length) { + return + } + if (scroll.endAnchor) { + scroll.endAnchor.cancelled = true + } + const signal = {cancelled: false} + scroll.endAnchor = signal + void runEndAnchorCorrection({ + endAnchor, + holdsEndAnchor: () => scrollRef.current.owner === 'end', + signal, + sleep, + }) + }) + const holdsEndAnchor = React.useEffectEvent(() => scrollRef.current.owner === 'end') + const readerTookScroll = React.useEffectEvent(() => { + const scroll = scrollRef.current + scroll.owner = 'reader' + if (scroll.correction) { + scroll.correction.cancelled = true + } + }) + const registerAdapter = React.useEffectEvent((adapter: CenterScrollAdapter | undefined) => { + scrollRef.current.adapter = adapter + }) + const takeEndAnchor = React.useEffectEvent(() => { + scrollRef.current.owner = 'end' + }) + const [scrollActions] = React.useState(() => ({ + endMayHaveMoved: () => { + endMayHaveMoved() + }, + holdsEndAnchor: () => holdsEndAnchor(), + readerTookScroll: () => { + readerTookScroll() + }, + registerAdapter: (adapter: CenterScrollAdapter | undefined) => { + registerAdapter(adapter) + }, + takeEndAnchor: () => { + takeEndAnchor() + }, + })) + + const setTarget = React.useEffectEvent( + (messageID: T.Chat.MessageID, highlightMode: T.Chat.CenterOrdinalHighlightMode) => { + setCenterState(state => + produce(stateForThreadSearchVisible(state, threadSearchVisible), draft => { + draft.target = {highlightMode, messageID} + }) + ) + } + ) + + const clearTarget = React.useEffectEvent(() => { + setCenterState(state => + produce(stateForThreadSearchVisible(state, threadSearchVisible), draft => { + draft.target = undefined + }) + ) + }) + + // Poll rather than subscribe: the two things waited on - the message landing in the window and the + // list registering itself - settle at different times under different owners, and the corrector + // that follows is a poll anyway. + const waitForAdapter = async () => { + for (let elapsed = 0; elapsed <= centerTimeoutMs; elapsed += measurePollMs) { + const {adapter} = scrollRef.current + if (adapter) { + return adapter + } + await sleep(measurePollMs) + } + return undefined + } + + // Started here rather than by whoever asked for it, so a list that mounts - or remounts, after a + // freeze/thaw - onto a target that resolved long ago is still steered onto it. A "last corrected" + // ordinal rather than a "did it change" flag, so the correction still runs when the thread finishes + // loading after the target was set. + const lastCorrectedRef = React.useRef(undefined) + const correctOnto = React.useEffectEvent(async (ordinal: T.Chat.Ordinal) => { + const adapter = await waitForAdapter() + if (!adapter) { + settlePending('clamped') + return + } + abortCorrection() + const signal = {cancelled: false} + scrollRef.current.correction = signal + const outcome = await runCenterCorrection({adapter, ordinal, signal, sleep}) + if (scrollRef.current.correction === signal) { + scrollRef.current.correction = undefined + settlePending(outcome) + } + }) + React.useEffect(() => { + if (centeredOrdinal === undefined) { + lastCorrectedRef.current = undefined + return + } + if (lastCorrectedRef.current === centeredOrdinal) { + return + } + lastCorrectedRef.current = centeredOrdinal + takeCentering() + void correctOnto(centeredOrdinal) + // `target` is a dep as well as the ordinal: re-centering on the row the reader is already + // parked on leaves the ordinal unchanged, and that request still has to be steered. + }, [centeredOrdinal, target]) + + // The other side of that effect: once the centre is gone the end belongs to the list again. + const returnEndToTheList = React.useEffectEvent(() => { + const scroll = scrollRef.current + if (scroll.correction) { + scroll.correction.cancelled = true + } + scroll.owner = 'end' + if (!store.getState().moreToLoadForward) { + scroll.adapter?.endAnchor?.scrollToEnd() + } + }) + const hadCenterRef = React.useRef(false) + React.useEffect(() => { + const hadCenter = hadCenterRef.current + hadCenterRef.current = !!target + if (hadCenter && !target) { + returnEndToTheList() + } + }, [target]) + + // The thread came back without the message: nothing is going to correct onto it, so answer for the + // request rather than leaving the caller waiting on a row that will never render. + const watchForMissingMessage = React.useEffectEvent(async (pending: PendingCenter) => { + for (let elapsed = 0; elapsed <= centerTimeoutMs; elapsed += measurePollMs) { + if (scrollRef.current.pending !== pending || pending.corrected) { + return + } + await sleep(measurePollMs) + } + if (scrollRef.current.pending === pending && !pending.corrected) { + settlePending('not-found') + } + }) + + const runCenterOn = React.useEffectEvent( + async (messageID: T.Chat.MessageID, highlightMode: T.Chat.CenterOrdinalHighlightMode) => { + settlePending('clamped') + abortCorrection() + takeCentering() + // Re-centering on the row the reader is already parked on still reloads the thread, so the + // list has to be steered onto it again. + lastCorrectedRef.current = undefined + setTarget(messageID, highlightMode) + requestWindow({anchor: {centeredOn: messageID}, reason: 'centered'}) + return new Promise(resolve => { + const pending: PendingCenter = {corrected: false, messageID, settle: resolve} + scrollRef.current.pending = pending + void watchForMissingMessage(pending) + }) + } + ) + const runJumpToRecent = React.useEffectEvent(() => { + clearTarget() + requestWindow({anchor: 'newest', reason: 'jump to recent'}) + }) + + const [actions] = React.useState(() => ({ + centerOn: async (messageID: T.Chat.MessageID, highlightMode: T.Chat.CenterOrdinalHighlightMode) => + runCenterOn(messageID, highlightMode), + clearCenter: () => { + clearTarget() + }, + jumpToRecent: () => { + runJumpToRecent() + }, + })) + + React.useEffect(() => { + setMarkReadBlocked(threadSearchVisible) + return () => { + setMarkReadBlocked(false) + } + }, [setMarkReadBlocked, threadSearchVisible]) + + const applyHighlight = React.useEffectEvent((messageID: T.Chat.MessageID) => { + setMarkReadBlocked(true) + void actions.centerOn(messageID, 'flash') + }) + // Same two delivery moments as ConversationInputProvider: consume whatever was written before + // we mounted, then a registered subscription for later writes. Not a selector hook - that would + // re-render this subtree, and enableFreeze defers render-driven subscriptions on mobile while a + // registered callback still runs. Delete-on-consume is the dedupe, so jumping twice to the same + // messageID centers twice. + React.useEffect(() => { + const consume = () => { + const intent = consumeInputIntent(id, centerInputIntentTypes) + if (intent) { + applyHighlight(intent.messageID) + } + } + consume() + return useInputIntentState.subscribe(consume) + }, [id]) + + const centeredHighlightOrdinal = target && target.highlightMode !== 'none' ? centeredOrdinal : undefined + const stateValue = { + centeredHighlightOrdinal, + centeredOrdinal, + hasCenter: !!target, + } + + return ( + + + {children} + + + ) +} diff --git a/shared/chat/conversation/input-area/input-state.tsx b/shared/chat/conversation/input-area/input-state.tsx index 9048983134b5..2c4de6647029 100644 --- a/shared/chat/conversation/input-area/input-state.tsx +++ b/shared/chat/conversation/input-area/input-state.tsx @@ -114,7 +114,7 @@ DispatchContext.displayName = 'ConversationInputDispatchContext' const actionConversationIDKey = (convID: string) => T.Chat.stringToConversationIDKey(convID) -// 'highlight' belongs to ConversationCenterProvider; claiming it here would let this provider +// 'highlight' belongs to ConversationCenteringProvider; claiming it here would let this provider // silently eat an intent meant for the other consumer. // `as const` (not a widened ReadonlyArray) so consumeInputIntent's generic // narrows its return to exactly these four members - no cast needed at the call site below. diff --git a/shared/chat/conversation/input-area/normal/index.tsx b/shared/chat/conversation/input-area/normal/index.tsx index f493ebc5cacb..fad88bfe57ec 100644 --- a/shared/chat/conversation/input-area/normal/index.tsx +++ b/shared/chat/conversation/input-area/normal/index.tsx @@ -14,7 +14,7 @@ import {infoPanelWidthTablet} from '../../info-panel/common' import {assertionToDisplay} from '@/common-adapters/usernames' import {ThreadRefsContext} from '@/chat/conversation/normal/context' import type {RefType as InputRef} from './input.shared' -import {useConversationCenter, useConversationCenterActions} from '../../center-context' +import {useConversationCenter, useConversationCenterActions} from '../../centering' import { useConversationThreadID, useConversationThreadMessage, diff --git a/shared/chat/conversation/input-intent-store.test.ts b/shared/chat/conversation/input-intent-store.test.ts index 00cb5f94ee45..63ae8ee29bd0 100644 --- a/shared/chat/conversation/input-intent-store.test.ts +++ b/shared/chat/conversation/input-intent-store.test.ts @@ -112,7 +112,7 @@ test('a consumer registered for another conversation does not make this one deli }) // A consumer only makes deliverable the types it claims, the same split consumeInputIntent -// enforces: ConversationCenterProvider being mounted must not vouch for the composer. +// enforces: ConversationCenteringProvider being mounted must not vouch for the composer. test('a consumer that does not claim commandStatus does not make it deliverable', () => { jest.spyOn(logger, 'info').mockImplementation(() => {}) registrations.push(registerInputIntentConsumer(convX, ['highlight'])) diff --git a/shared/chat/conversation/input-intent-store.tsx b/shared/chat/conversation/input-intent-store.tsx index 0c5c3014ad5b..f31e461dd0f4 100644 --- a/shared/chat/conversation/input-intent-store.tsx +++ b/shared/chat/conversation/input-intent-store.tsx @@ -61,7 +61,7 @@ type Consumer = {types: ReadonlyArray} const consumers = new Map>() // Only ConversationInputProvider registers today: it is the sole consumer of the one type whose -// delivery is gated on a mount. ConversationCenterProvider claims 'highlight', which is durable, +// delivery is gated on a mount. ConversationCenteringProvider claims 'highlight', which is durable, // so registering it would add an entry nothing ever asks about. export const registerInputIntentConsumer = ( conversationIDKey: T.Chat.ConversationIDKey, @@ -108,13 +108,13 @@ export const setInputIntent = (conversationIDKey: T.Chat.ConversationIDKey, inte } // Two providers can be mounted for the same conversation (the input provider, and -// ConversationCenterProvider for 'highlight'), each with its own slice of InputIntent['type']. +// ConversationCenteringProvider for 'highlight'), each with its own slice of InputIntent['type']. // `types` restricts a read to the caller's slice so one provider can never swallow an // intent meant for the other; an intent whose type isn't in `types` is left pending. // // peek reads without consuming. Only for a component that must know an intent is waiting // without being its consumer: NormalWrapper picks the initial thread-load options from a -// pending 'highlight' that ConversationCenterProvider, mounted below it, actually consumes. +// pending 'highlight' that ConversationCenteringProvider, mounted below it, actually consumes. export const peekInputIntent = ( conversationIDKey: T.Chat.ConversationIDKey, types: ReadonlyArray diff --git a/shared/chat/conversation/list-area/index.tsx b/shared/chat/conversation/list-area/index.tsx index baaeeb785f49..dee16c47e332 100644 --- a/shared/chat/conversation/list-area/index.tsx +++ b/shared/chat/conversation/list-area/index.tsx @@ -10,7 +10,7 @@ import {MessageRow} from '../messages/wrapper' import {RowHoveredContext} from '../messages/ids-context' import {PerfProfiler} from '@/perf/react-profiler' import {ThreadRefsContext} from '../normal/context' -import {useConversationCenter} from '../center-context' +import {type CenterScrollAdapter, useConversationCenter, useConversationCenterScroll} from '../centering' import { ShownUsernameCacheContext, useConversationThreadID, @@ -107,6 +107,12 @@ const usePagination = () => { } const centerTolerancePx = 8 +// Native measures in index space, so its deadband is half a row and its steps are damped to keep an +// inverted list of tall image rows from oscillating around the target. +const nativeCenterDamping = 0.9 +const maxNativeCenterCorrections = 12 +const maxStaleRangeReads = 3 +const maxScrollToIndexRetries = 5 // A scroller within this many pixels of its end counts as at the end. const endTolerancePx = 2 @@ -184,9 +190,8 @@ const DesktopThreadWrapper = function DesktopThreadWrapper() { const desktopStyles = useDesktopStyles() const editingOrdinal = InputState.useConversationInput(s => s.editing) const conversationIDKey = useConversationThreadID() - const {generation, loaded, moreToLoadForward, ordinals: messageOrdinals} = useThreadWindow() - const {centeredOrdinal} = useConversationCenter() - const containsLatestMessage = !moreToLoadForward + const {generation, ordinals: messageOrdinals} = useThreadWindow() + const {centeredOrdinal, hasCenter} = useConversationCenter() // Centered loads (search hit, reply-quote jump, pinned message) clear the thread before // refetching, so the list sees a non-empty -> empty -> non-empty transition. @@ -222,33 +227,33 @@ const DesktopThreadWrapper = function DesktopThreadWrapper() { return false }, []) - // Whether the end still belongs to the list rather than to the user. initialScrollAtEnd starts it - // ours; a wheel, a keyboard scroll or a centered load hands it over. Only consulted by the header - // re-pin below, which must not yank a reader who has scrolled away. - const pinnedToEndRef = React.useRef(true) + // Who owns the scroll offset - the end, the centering loop, or the reader - is the centering + // module's state now. initialScrollAtEnd starts it ours; a wheel, a keyboard scroll or a centered + // load hands it over. + const {endMayHaveMoved, readerTookScroll, registerAdapter, takeEndAnchor} = useConversationCenterScroll() React.useLayoutEffect(() => { - pinnedToEndRef.current = true - }, [datasetKey]) + takeEndAnchor() + }, [datasetKey, takeEndAnchor]) // Imperative scroll for ThreadRefsContext: for coming back from somewhere else in the thread, which // is the only case that needs it. While the list is at the end maintainScrollAtEnd owns the position, // and scrolling here only displaces it — the target resolves before the new row has measured, so it // lands short, and while it counts as in flight the list declines its own end anchor and abandons it. const scrollToBottom = React.useCallback(() => { - pinnedToEndRef.current = true + takeEndAnchor() if (isScrolledToEnd()) return void listRef.current?.scrollToEnd({animated: false}) - }, [isScrolledToEnd]) + }, [isScrolledToEnd, takeEndAnchor]) const scrollUp = React.useCallback(() => { const state = listRef.current?.getState() if (!state) return - pinnedToEndRef.current = false + readerTookScroll() void listRef.current?.scrollToOffset({ animated: false, offset: Math.max(0, state.scroll - state.scrollLength), }) - }, []) + }, [readerTookScroll]) const scrollDown = React.useCallback(() => { const state = listRef.current?.getState() @@ -259,52 +264,8 @@ const DesktopThreadWrapper = function DesktopThreadWrapper() { }) }, []) - // The list resolves its initialScrollAtEnd target from the header size it has measured so far, and - // SpecialTopMessage renders at its bare minHeight before the thread's intro content (retention - // notice, new-chat card, the "digging" spinner) lands. maintainScrollAtEnd re-pins on a data, item, - // footer or viewport layout change but has no header trigger, so a header that grows after the - // target resolved leaves the list short by exactly that growth and nothing corrects it. - // - // Closed loop rather than a correction fired straight from the size change, for the same reason - // scrollToBottom keeps out of the way: the header often settles while the thread is still empty, - // and a scrollToEnd issued against that near-empty content becomes the target the list then - // abandons its own bootstrap for, landing anywhere. Wait for the scroll offset to hold still, so - // the list has finished its own initial scroll, and only then correct what it left on the table. - const endAnchorLoopRef = React.useRef<{cancelled: boolean} | undefined>(undefined) - const stopEndAnchor = React.useCallback(() => { - if (endAnchorLoopRef.current) endAnchorLoopRef.current.cancelled = true - }, []) - React.useEffect(() => stopEndAnchor, [stopEndAnchor]) - const verifyEndAnchor = React.useCallback(() => { - stopEndAnchor() - const loop = {cancelled: false} - endAnchorLoopRef.current = loop - const run = async () => { - let previousScroll: number | undefined - let corrections = 0 - for (let elapsed = 0; elapsed < 2000 && !loop.cancelled && pinnedToEndRef.current; ) { - await new Promise(resolve => setTimeout(resolve, 50)) - elapsed += 50 - const state = listRef.current?.getState() - if (!state) continue - if (state.isAtEnd) return - // Only a scroll offset that held still across two checks means the list is done moving. - if (state.scroll === previousScroll) { - // Two corrections is the whole budget: one for the header, one for whatever re-measured - // alongside it. Past that we would be fighting something that owns the offset. - if (++corrections > 2) return - void listRef.current?.scrollToEnd({animated: false}) - previousScroll = undefined - } else { - previousScroll = state.scroll - } - } - } - void run() - }, [stopEndAnchor]) - - // The header's own size change is the signal, but only once there are messages: the header - // frequently settles while the thread is still empty, and there is no end to hold yet. + // The header's own size change is the signal; the centering module decides whether the end is + // still ours to hold and runs the correction. const lastHeaderSizeRef = React.useRef(undefined) React.useLayoutEffect(() => { lastHeaderSizeRef.current = undefined @@ -315,10 +276,9 @@ const DesktopThreadWrapper = function DesktopThreadWrapper() { lastHeaderSizeRef.current = metrics.headerSize // The first emit is the measurement the target was built from, not a change. if (previous === undefined || previous === metrics.headerSize) return - if (!pinnedToEndRef.current || messageOrdinalsRef.current.length === 0) return - verifyEndAnchor() + endMayHaveMoved() }, - [verifyEndAnchor] + [endMayHaveMoved] ) const {setScrollRef} = React.useContext(ThreadRefsContext) @@ -359,113 +319,60 @@ const DesktopThreadWrapper = function DesktopThreadWrapper() { [onScroll] ) - // Scroll to centered ordinal when it changes (search / thread navigation). - // Use a "last scrolled to" ref rather than a "did it change" ref so we still - // scroll when loaded becomes true after centeredOrdinal was already set. - // Reset per dataset, not per conversation: re-centering on the ordinal we are already parked - // on still reloads the thread, so the list has to scroll to it again. - const lastScrolledCenteredRef = React.useRef(undefined) - React.useLayoutEffect(() => { - lastScrolledCenteredRef.current = undefined - }, [datasetKey]) - - // Owns the in-flight centering loop. It has to outlive re-renders: the messages that make - // centering accurate arrive after it starts, so the loop must not be torn down by an effect - // cleanup when messageOrdinals changes. Only a new target or unmount stops it. - const centerLoopRef = React.useRef<{cancelled: boolean} | undefined>(undefined) - // The loop re-centers for up to ~3s; a user scrolling in that window must win. - const abortCentering = React.useCallback(() => { - if (centerLoopRef.current) centerLoopRef.current.cancelled = true - }, []) - React.useEffect(() => abortCentering, [abortCentering]) - - // Closed loop, not one shot: rows enter at estimatedItemSize and only settle as they measure, so - // the first scroll lands off by however wrong the estimates above the target were. Measure the - // row's real offset from the viewport center and correct until it holds still, then get out of - // the way: maintainVisibleContentPosition owns the offset from then on. Two controllers fighting - // over the same scroll offset would oscillate. - // - // Correct via LegendList's own scrollToOffset, never scrollIntoView: touching scrollTop directly - // desyncs LegendList's internal scroll state, and the next time it recomputes item positions it - // snaps somewhere unrelated. - const scrollToCentered = React.useEffectEvent((target: T.Chat.Ordinal) => { - abortCentering() - const loop = {cancelled: false} - centerLoopRef.current = loop - const run = async () => { - let settled = 0 - let pinnedChecks = 0 - let scrollAtLastRequest: number | undefined - for (let elapsed = 0; elapsed < 3000 && !loop.cancelled; ) { - const wrapper = wrapperRef.current as unknown as { - getBoundingClientRect: () => {height: number; top: number} - querySelector: (s: string) => {getBoundingClientRect: () => {height: number; top: number}} | null - } | null - const el = wrapper ? wrapper.querySelector(`[data-ordinal="${target}"]`) : null - if (!wrapper || !el) { - // Target is outside the rendered window; get it mounted first. - const idx = sortedIndexOf( - messageOrdinalsRef.current as unknown as number[], - target as unknown as number - ) - if (idx >= 0) { - void listRef.current?.scrollToIndex({animated: false, index: idx, viewPosition: 0.5}) - } - settled = 0 - pinnedChecks = 0 - await new Promise(resolve => setTimeout(resolve, 100)) - elapsed += 100 - continue - } + // The desktop half of the centering module's scroll adapter. LegendList owns the offset, so every + // correction goes through its own scrollToOffset, never scrollIntoView: touching scrollTop + // directly desyncs LegendList's internal scroll state, and the next time it recomputes item + // positions it snaps somewhere unrelated. + const adapter = React.useMemo( + () => ({ + endAnchor: { + read: () => { + const state = listRef.current?.getState() + return state ? {isAtEnd: state.isAtEnd, scroll: state.scroll} : undefined + }, + scrollToEnd: () => { + void listRef.current?.scrollToEnd({animated: false}) + }, + }, + measureTarget: ordinal => { + type ElLike = {getBoundingClientRect: () => {height: number; top: number}} + const wrapper = wrapperRef.current as unknown as + | (ElLike & {querySelector: (s: string) => ElLike | null}) + | null + const el = wrapper ? wrapper.querySelector(`[data-ordinal="${ordinal}"]`) : null + if (!wrapper || !el) return {kind: 'offscreen'} + const scroll = listRef.current?.getState().scroll + if (scroll === undefined) return {kind: 'pending'} const elRect = el.getBoundingClientRect() const wrapRect = wrapper.getBoundingClientRect() - const offBy = elRect.top + elRect.height / 2 - (wrapRect.top + wrapRect.height / 2) - const scroll = listRef.current?.getState().scroll - // Deadband, not exact centering: below this the row reads as centered, and chasing the - // remainder only fights maintainVisibleContentPosition's own sub-pixel adjustments. - if (Math.abs(offBy) <= centerTolerancePx || scroll === undefined) { - pinnedChecks = 0 - // Only the iteration right after a correction can diagnose a clamp. - scrollAtLastRequest = undefined - if (++settled >= 3) return - } else if (scroll === scrollAtLastRequest) { - // A hit near either end of the thread cannot be centered: the offset we ask for gets - // clamped and the row never reaches the middle. Our last correction moved the scroll - // position not at all, so we are pinned against an edge — stop rather than spin. - if (++pinnedChecks >= 3) return - } else { - pinnedChecks = 0 - scrollAtLastRequest = scroll - void listRef.current?.scrollToOffset({animated: false, offset: scroll + offBy}) + return { + kind: 'measured', + offBy: elRect.top + elRect.height / 2 - (wrapRect.top + wrapRect.height / 2), + scroll, + tolerance: centerTolerancePx, } - await new Promise(resolve => setTimeout(resolve, 50)) - elapsed += 50 - } - } - void run() - }) - + }, + scrollToIndex: ordinal => { + const idx = sortedIndexOf( + messageOrdinalsRef.current as unknown as number[], + ordinal as unknown as number + ) + if (idx >= 0) { + void listRef.current?.scrollToIndex({animated: false, index: idx, viewPosition: 0.5}) + } + }, + scrollToOffset: offset => { + void listRef.current?.scrollToOffset({animated: false, offset}) + }, + }), + [] + ) React.useEffect(() => { - if (!loaded) return - if (centeredOrdinal !== undefined) { - if (lastScrolledCenteredRef.current === centeredOrdinal) return - const idx = sortedIndexOf( - messageOrdinalsRef.current as unknown as number[], - centeredOrdinal as unknown as number - ) - if (idx < 0) return - lastScrolledCenteredRef.current = centeredOrdinal - pinnedToEndRef.current = false - scrollToCentered(centeredOrdinal) - } else if (lastScrolledCenteredRef.current !== undefined) { - lastScrolledCenteredRef.current = undefined - abortCentering() - pinnedToEndRef.current = true - if (containsLatestMessage) { - void listRef.current?.scrollToEnd({animated: false}) - } + registerAdapter(adapter) + return () => { + registerAdapter(undefined) } - }, [abortCentering, centeredOrdinal, loaded, containsLatestMessage, messageOrdinals]) + }, [adapter, registerAdapter]) // Scroll to the message being edited const lastEditingOrdinalRef = React.useRef(undefined) @@ -572,9 +479,8 @@ const DesktopThreadWrapper = function DesktopThreadWrapper() { // A wheel means the user took over: stop centering so we don't scroll them away from where // they landed, and give up the end anchor. const onWheel = React.useCallback(() => { - pinnedToEndRef.current = false - abortCentering() - }, [abortCentering]) + readerTookScroll() + }, [readerTookScroll]) return ( @@ -609,7 +515,7 @@ const DesktopThreadWrapper = function DesktopThreadWrapper() { initialScrollAtEnd={initialScrollIndex === undefined} initialScrollIndex={initialScrollIndex} maintainScrollAtEnd={ - centeredOrdinal !== undefined + hasCenter ? false : // The documented form, which enables every trigger. It was a narrowed {on: {...}} list // before, and naming any trigger opts out of the ones left unnamed — that is how the @@ -676,11 +582,8 @@ type RNFlatListRef = { const useInvertedMessageOrdinals = (source: ReadonlyArray) => React.useMemo(() => (source.length > 1 ? [...source].reverse() : source), [source]) -const useNativeScrolling = (p: { - centeredOrdinal: T.Chat.Ordinal - listRef: React.RefObject -}) => { - const {listRef, centeredOrdinal} = p +const useNativeScrolling = (p: {listRef: React.RefObject}) => { + const {listRef} = p const requestWindow = useRequestWindow() // KeyboardChatScrollView sets contentInset.top = K - insets.bottom and @@ -700,67 +603,11 @@ const useNativeScrolling = (p: { setScrollRef({scrollDown: noop, scrollToBottom, scrollUp: noop}) }, [setScrollRef, scrollToBottom]) - // only scroll to center once per - const lastScrollToCentered = React.useRef(-1) - React.useEffect(() => { - if (T.Chat.ordinalToNumber(centeredOrdinal) < 0) { - lastScrollToCentered.current = -1 - } - }, [centeredOrdinal]) - - const centeredOrdinalRef = React.useRef(centeredOrdinal) - // reset per centered target so each new search hit gets a fresh batch of retries - const scrollFailRetryRef = React.useRef(0) - React.useEffect(() => { - centeredOrdinalRef.current = centeredOrdinal - scrollFailRetryRef.current = 0 - }, [centeredOrdinal]) - const [scrollToCentered] = React.useState(() => () => { - const co = centeredOrdinalRef.current - if (lastScrollToCentered.current === co) { - return - } - lastScrollToCentered.current = co - // coarse: scrollToItem lands at the wrong offset for tall variable-height rows, - // but it gets the target area rendered. The closed-loop corrector in the - // component refines from there using the real viewable index range. - const reassert = (delay: number) => - setTimeout(() => { - const list = listRef.current - const cur = centeredOrdinalRef.current - if (!list || cur !== co || T.Chat.ordinalToNumber(cur) <= 0) { - return - } - list.scrollToItem({animated: false, item: cur, viewPosition: 0.5}) - }, delay) - ;[50, 250].forEach(reassert) - }) - - // The centered hit may be outside the rendered window, so scrollToItem fails - // silently. Wait for more rows to render and retry centering (capped) until it lands. - const [onScrollToIndexFailed] = React.useState(() => () => { - if (scrollFailRetryRef.current > 5) { - return - } - scrollFailRetryRef.current += 1 - setTimeout(() => { - const co = centeredOrdinalRef.current - if (T.Chat.ordinalToNumber(co) > 0) { - listRef.current?.scrollToItem({animated: false, item: co, viewPosition: 0.5}) - } - }, 200) - }) - const onEndReached = () => { requestWindow({anchor: 'older', reason: 'scroll back'}) } - return { - onEndReached, - onScrollToIndexFailed, - scrollToBottom, - scrollToCentered, - } + return {onEndReached, scrollToBottom} } // The maintainVisibleContentPosition prop must ALWAYS be set (never toggled to undefined): @@ -793,7 +640,7 @@ const NativeConversationList = function NativeConversationList() { const conversationIDKey = useConversationThreadID() const {loaded, ordinals} = useThreadWindow() - const {centeredHighlightOrdinal, centeredOrdinal} = useConversationCenter() + const {centeredHighlightOrdinal, centeredOrdinal, hasCenter} = useConversationCenter() const noCenteredOrdinal = T.Chat.numberToOrdinal(-1) const centeredOrdinalOrNone = centeredOrdinal ?? noCenteredOrdinal const centeredHighlightOrdinalOrNone = centeredHighlightOrdinal ?? noCenteredOrdinal @@ -840,52 +687,96 @@ const NativeConversationList = function NativeConversationList() { ], })) - const {scrollToCentered, scrollToBottom, onEndReached, onScrollToIndexFailed} = useNativeScrolling({ - centeredOrdinal: centeredOrdinalOrNone, - listRef, - }) + const {scrollToBottom, onEndReached} = useNativeScrolling({listRef}) - // Closed-loop centering corrector. scrollToItem/scrollToIndex lands at the wrong - // offset here (inverted list + custom keyboard scrollview + tall variable-height - // image rows), so instead we read the actual viewable index range each frame and - // scrollToOffset by the item-delta until the target sits at viewport center. + // The native half of the centering module's scroll adapter. scrollToItem/scrollToIndex lands at + // the wrong offset here (inverted list + custom keyboard scrollview + tall variable-height image + // rows), so the measurement is taken in index space instead - the real viewable range against the + // target's index - and converted to pixels with the average row height. const scrollOffsetRef = React.useRef(0) const contentHeightRef = React.useRef(0) - const centeredRef = React.useRef(centeredOrdinalOrNone) - React.useEffect(() => { - centeredRef.current = centeredOrdinalOrNone - }, [centeredOrdinalOrNone]) const ordsRef = React.useRef(messageOrdinals) React.useEffect(() => { ordsRef.current = messageOrdinals }, [messageOrdinals]) - // {active, iters}: correcting toward a centered hit and how many steps taken - const correctRef = React.useRef({active: false, iters: 0}) const vFirstRef = React.useRef(undefined) const vLastRef = React.useRef(undefined) - const [correctCenter] = React.useState( - () => (first: number | null | undefined, last: number | null | undefined) => { - const st = correctRef.current - if (!st.active) return - const co = centeredRef.current - const ords = ordsRef.current - const num = ords.length - if (co <= 0 || !num || first == null || last == null) return - const targetIdx = ords.indexOf(co) - if (targetIdx < 0) return - const centerIdx = (first + last) / 2 - const diff = targetIdx - centerIdx - if (Math.abs(diff) <= 0.5 || st.iters > 12) { - st.active = false - return - } - st.iters += 1 - const avgH = contentHeightRef.current / num - // damp by 0.9 to avoid overshoot/oscillation; higher index = older = higher offset - const newOffset = Math.max(0, scrollOffsetRef.current + diff * avgH * 0.9) - listRef.current?.scrollToOffset({animated: false, offset: newOffset}) - } + // The viewable range only updates when the list reports one, and correcting twice off the same + // reading overshoots. Bounded, though: a scroll that moves nothing never produces a new range, + // and the corrector has to be allowed to see that it moved nothing. + const rangeVersionRef = React.useRef(0) + const consumedRangeRef = React.useRef(-1) + const staleRangeReadsRef = React.useRef(0) + const {readerTookScroll, registerAdapter} = useConversationCenterScroll() + const adapter = React.useMemo( + () => ({ + maxCorrections: maxNativeCenterCorrections, + measureTarget: ordinal => { + const ords = ordsRef.current + const num = ords.length + const targetIdx = ords.indexOf(ordinal) + if (!num || targetIdx < 0) return {kind: 'offscreen'} + const first = vFirstRef.current + const last = vLastRef.current + if (first == null || last == null) return {kind: 'pending'} + if (rangeVersionRef.current === consumedRangeRef.current) { + staleRangeReadsRef.current += 1 + if (staleRangeReadsRef.current <= maxStaleRangeReads) return {kind: 'pending'} + } + staleRangeReadsRef.current = 0 + const centerIdx = (first + last) / 2 + const diff = targetIdx - centerIdx + const avgH = contentHeightRef.current / num + return { + kind: 'measured', + // higher index = older = higher offset, damped to avoid overshoot/oscillation + offBy: diff * avgH * nativeCenterDamping, + scroll: scrollOffsetRef.current, + // half a row, expressed through the same damping so the deadband stays the index-space + // half-row it has always been + tolerance: 0.5 * avgH * nativeCenterDamping, + } + }, + scrollToIndex: ordinal => { + if (T.Chat.ordinalToNumber(ordinal) <= 0) return + consumedRangeRef.current = rangeVersionRef.current + staleRangeReadsRef.current = 0 + listRef.current?.scrollToItem({animated: false, item: ordinal, viewPosition: 0.5}) + }, + scrollToOffset: offset => { + consumedRangeRef.current = rangeVersionRef.current + staleRangeReadsRef.current = 0 + listRef.current?.scrollToOffset({animated: false, offset: Math.max(0, offset)}) + }, + }), + [] ) + React.useEffect(() => { + registerAdapter(adapter) + return () => { + registerAdapter(undefined) + } + }, [adapter, registerAdapter]) + + // The centered hit may be outside the rendered window, so scrollToItem fails silently. Wait for + // more rows to render and retry (capped) until it lands; reset per target so each new search hit + // gets a fresh batch of retries. + const centeredRef = React.useRef(centeredOrdinalOrNone) + const scrollFailRetryRef = React.useRef(0) + React.useEffect(() => { + centeredRef.current = centeredOrdinalOrNone + scrollFailRetryRef.current = 0 + }, [centeredOrdinalOrNone]) + const [onScrollToIndexFailed] = React.useState(() => () => { + if (scrollFailRetryRef.current > maxScrollToIndexRetries) { + return + } + scrollFailRetryRef.current += 1 + setTimeout(() => { + adapter.scrollToIndex(centeredRef.current) + }, 200) + }) + const [onScrollNative] = React.useState( () => (e: {nativeEvent: {contentOffset: {y: number}; contentSize: {height: number}}}) => { @@ -897,9 +788,9 @@ const NativeConversationList = function NativeConversationList() { contentHeightRef.current = h }) // user touched the list: stop fighting them - const [onScrollBeginDrag] = React.useState(() => () => { - correctRef.current.active = false - }) + const onScrollBeginDrag = React.useCallback(() => { + readerTookScroll() + }, [readerTookScroll]) const jumpToRecent = useJumpToRecent(scrollToBottom, messageOrdinals.length) @@ -932,27 +823,6 @@ const NativeConversationList = function NativeConversationList() { return undefined }, [conversationIDKey, numOrdinals, scrollToBottom]) - // Center on the search hit once it actually appears in the loaded list. Centering - // on the raw centeredOrdinal change is unreliable: navigating to a hit reloads the - // thread centered on it, so messageOrdinals is briefly empty (idx -1) when the - // ordinal changes. Wait for the target to load, then scroll (scrollToCentered - // guards against repeats and re-asserts across frames). - React.useEffect(() => { - if (!(centeredOrdinalOrNone > 0 && messageOrdinals.includes(centeredOrdinalOrNone))) { - return undefined - } - // coarse scroll to get the target area rendered, then run the closed-loop - // corrector which refines via the real viewable index range - scrollToCentered() - correctRef.current = {active: true, iters: 0} - const ids = [50, 250, 500, 900].map(d => - setTimeout(() => correctCenter(vFirstRef.current, vLastRef.current), d) - ) - return () => { - ids.forEach(clearTimeout) - } - }, [centeredOrdinalOrNone, messageOrdinals, scrollToCentered, correctCenter]) - // These refs store the conversation they last applied to (not a boolean) so a // freeze/thaw of this screen — which re-mounts effects without a real // conversation change — does not reset them and re-trigger the initial scroll, @@ -973,36 +843,22 @@ const NativeConversationList = function NativeConversationList() { markInitiallyLoadedThreadAsRead() } - if (centeredOrdinalOrNone > 0) { - scrollToCentered() - setTimeout(() => { - scrollToCentered() - }, 100) - } else if (numOrdinals > 0) { + // A centered thread is the centering module's to steer. + if (!hasCenter && numOrdinals > 0) { scrollToBottom() setTimeout(() => { scrollToBottom() }, 100) } - }, [ - conversationIDKey, - centeredOrdinalOrNone, - loaded, - markInitiallyLoadedThreadAsRead, - numOrdinals, - scrollToBottom, - scrollToCentered, - ]) + }, [conversationIDKey, hasCenter, loaded, markInitiallyLoadedThreadAsRead, numOrdinals, scrollToBottom]) const onViewableItemsChanged = useNativeSafeOnViewableItemsChanged(onEndReached, messageOrdinals.length) const [onViewableItemsChangedNative] = React.useState( () => (info: {viewableItems: Array<{index: number | null}>}) => { onViewableItemsChanged.current(info) - const first = info.viewableItems.at(0)?.index - const last = info.viewableItems.at(-1)?.index - vFirstRef.current = first - vLastRef.current = last - correctCenter(first, last) + vFirstRef.current = info.viewableItems.at(0)?.index + vLastRef.current = info.viewableItems.at(-1)?.index + rangeVersionRef.current += 1 } ) @@ -1021,7 +877,7 @@ const NativeConversationList = function NativeConversationList() { [insets.bottom, searchOverlayHeight] ) - const mvpAutoscroll = !(centeredOrdinalOrNone > 0 || !numOrdinals || isKeyboardVisible) + const mvpAutoscroll = !(hasCenter || !numOrdinals || isKeyboardVisible) const nativeContentContainerStyle = React.useMemo( () => ({ diff --git a/shared/chat/conversation/list-area/jump-to-recent.tsx b/shared/chat/conversation/list-area/jump-to-recent.tsx index 972732b227b5..4a78a01a2252 100644 --- a/shared/chat/conversation/list-area/jump-to-recent.tsx +++ b/shared/chat/conversation/list-area/jump-to-recent.tsx @@ -1,6 +1,6 @@ import * as C from '@/constants' import * as Kb from '@/common-adapters' -import {useConversationCenterActions} from '../center-context' +import {useConversationCenterActions} from '../centering' import {useConversationThreadSelector, useConversationThreadToggleSearch} from '../thread-context' const JumpToRecent = (props: {onClick: () => void}) => { diff --git a/shared/chat/conversation/messages/pin/index.tsx b/shared/chat/conversation/messages/pin/index.tsx index 14342032e40b..459cf23c80cb 100644 --- a/shared/chat/conversation/messages/pin/index.tsx +++ b/shared/chat/conversation/messages/pin/index.tsx @@ -1,14 +1,16 @@ import * as Kb from '@/common-adapters' import type * as T from '@/constants/types' -import {useConversationCenterActions} from '../../center-context' +import {useConversationCenterActions} from '../../centering' type Props = {messageID: T.Chat.MessageID} const Pin = (props: Props) => { const styles = useStyles() const {messageID} = props - const {centerOnMessage} = useConversationCenterActions() - const onReplyClick = () => centerOnMessage(messageID, 'flash') + const {centerOn} = useConversationCenterActions() + const onReplyClick = () => { + void centerOn(messageID, 'flash') + } return ( pinned a message to this chat. diff --git a/shared/chat/conversation/messages/text/wrapper.tsx b/shared/chat/conversation/messages/text/wrapper.tsx index 7086297536f9..2ed5c9bed89c 100644 --- a/shared/chat/conversation/messages/text/wrapper.tsx +++ b/shared/chat/conversation/messages/text/wrapper.tsx @@ -5,7 +5,7 @@ import {useOrdinal} from '../ids-context' import {WrapperMessage, useWrapperMessage, type Props} from '../wrapper/wrapper' import type {StyleOverride} from '@/common-adapters/markdown' import {useSharedStyles} from '../shared-styles' -import {useConversationCenterActions} from '../../center-context' +import {useConversationCenterActions} from '../../centering' const getStyle = ( sharedStyles: ReturnType, @@ -44,7 +44,7 @@ function WrapperText(p: Props) { const {ordinal, isCenteredHighlight = false} = p const wrapper = useWrapperMessage(ordinal, isCenteredHighlight) const {messageData} = wrapper - const {centerOnMessage} = useConversationCenterActions() + const {centerOn} = useConversationCenterActions() const {isEditing, message, replyTo} = messageData const {hasCoinFlip, hasUnfurlList, hasUnfurlPrompts, showCenteredHighlight, text, textType, type} = @@ -61,7 +61,7 @@ function WrapperText(p: Props) { const onReplyClick = () => { const id = replyTo?.id ?? 0 if (id) { - centerOnMessage(id, 'flash') + void centerOn(id, 'flash') } } const reply = useReply(replyTo, onReplyClick) diff --git a/shared/chat/conversation/normal/container.test.tsx b/shared/chat/conversation/normal/container.test.tsx index 3cbf83018a9b..d2bdfb3d0687 100644 --- a/shared/chat/conversation/normal/container.test.tsx +++ b/shared/chat/conversation/normal/container.test.tsx @@ -104,8 +104,8 @@ jest.mock('../team-hooks', () => { return {ChatTeamProvider: mockPassthroughProvider} }) -jest.mock('../center-context', () => { - return {ConversationCenterProvider: mockPassthroughProvider} +jest.mock('../centering', () => { + return {ConversationCenteringProvider: mockPassthroughProvider} }) jest.mock('../input-area/input-state', () => { @@ -601,7 +601,7 @@ test('a pending highlight intent skips thread load but leaves mark-read allowed' }) }) -// Peek, not consume: ConversationCenterProvider (mocked away here) is the real consumer, so the +// Peek, not consume: ConversationCenteringProvider (mocked away here) is the real consumer, so the // intent has to survive NormalWrapper's read. test('reading the pending highlight leaves it in the store for the center provider', () => { mockLoaded = false @@ -642,14 +642,14 @@ test('a pending highlight for another conversation does not skip thread load on }) // The peek must run in a useState initializer keyed on the conversation, not a useMemo React may -// drop and recompute: a recompute after ConversationCenterProvider consumed reads an empty mailbox. +// drop and recompute: a recompute after ConversationCenteringProvider consumed reads an empty mailbox. // Re-rendering with the intent already consumed must not change what the provider was mounted with. test('the mount-time highlight decision survives a re-render after the intent is consumed', () => { mockLoaded = false setInputIntent(convID, {messageID: T.Chat.numberToMessageID(123), type: 'highlight'}) const {rerender} = render() - // ConversationCenterProvider is mocked out here, so drain the mailbox the way it would + // ConversationCenteringProvider is mocked out here, so drain the mailbox the way it would act(() => { consumeInputIntent(convID, ['highlight']) }) diff --git a/shared/chat/conversation/normal/container.tsx b/shared/chat/conversation/normal/container.tsx index 2c4a5aa22d53..c1803770b837 100644 --- a/shared/chat/conversation/normal/container.tsx +++ b/shared/chat/conversation/normal/container.tsx @@ -7,7 +7,7 @@ import * as T from '@/constants/types' import {ThreadRefsProvider} from './context' import {OrangeLineContext, SetOrangeLineContext, useExplicitOrangeLineState} from '../orange-line-context' import {ChatTeamProvider} from '../team-hooks' -import {ConversationCenterProvider} from '../center-context' +import {ConversationCenteringProvider} from '../centering' import {ConversationInputProvider} from '../input-area/input-state' import { useConversationThreadID, @@ -203,7 +203,7 @@ const NormalOrangeLineProvider = (props: OrangeLineProviderProps) => { } // Keyed on the conversation by its caller, so the peek runs in a useState initializer exactly -// once per conversation - before ConversationCenterProvider, mounted below, consumes the intent. +// once per conversation - before ConversationCenteringProvider, mounted below, consumes the intent. // A useMemo would not do: React may drop and recompute one, and a later recompute would read an // already-consumed mailbox and flip the answer. // @@ -254,13 +254,13 @@ const NormalWrapper = function NormalWrapper() { id={conversationIDKey} threadSearchVisible={threadSearchVisible} > - + - + diff --git a/shared/chat/conversation/normal/index.tsx b/shared/chat/conversation/normal/index.tsx index 4a16c1b89849..1031cf182033 100644 --- a/shared/chat/conversation/normal/index.tsx +++ b/shared/chat/conversation/normal/index.tsx @@ -8,7 +8,7 @@ import InvitationToBlock from '@/chat/blocking/invitation-to-block' import ListArea from '../list-area' import PinnedMessage from '../pinned-message' import ThreadLoadStatus from '../load-status' -import {useConversationCenterActions} from '../center-context' +import {useConversationCenterActions} from '../centering' import { useConversationThreadID, useConversationThreadToggleSearch, diff --git a/shared/chat/conversation/pinned-message.tsx b/shared/chat/conversation/pinned-message.tsx index cd8a51fa36b9..8aa952985c93 100644 --- a/shared/chat/conversation/pinned-message.tsx +++ b/shared/chat/conversation/pinned-message.tsx @@ -6,7 +6,7 @@ import * as Kb from '@/common-adapters' import {useCurrentUserState} from '@/stores/current-user' import {useChatTeam} from './team-hooks' import {ZoomedImage} from './common' -import {useConversationCenterActions} from './center-context' +import {useConversationCenterActions} from './centering' import {useConversationThreadID, useThreadMeta} from './thread-context' import logger from '@/logger' import {RPCError} from '@/util/errors' @@ -22,7 +22,7 @@ const PinnedMessage = function PinnedMessage() { teamname: m.teamname, })) ) - const {centerOnMessage} = useConversationCenterActions() + const {centerOn} = useConversationCenterActions() const you = useCurrentUserState(s => s.username) const {yourOperations} = useChatTeam(teamID, teamname) const unpinning = C.Waiting.useAnyWaiting(C.waitingKeyChatUnpin(conversationIDKey)) @@ -40,7 +40,7 @@ const PinnedMessage = function PinnedMessage() { const onClick = () => { if (messageID) { - centerOnMessage(messageID, 'flash') + void centerOn(messageID, 'flash') } } const onUnpin = () => { diff --git a/shared/chat/conversation/search.test.ts b/shared/chat/conversation/search.test.ts index 80d30fc1e66d..f14dacbb181a 100644 --- a/shared/chat/conversation/search.test.ts +++ b/shared/chat/conversation/search.test.ts @@ -9,7 +9,9 @@ const conversationIDKey = T.Chat.conversationIDToKey(new Uint8Array([1, 2, 3, 4] const username = 'testuser' const devicename = 'testuser-mac' -const mockCenterOnMessage = jest.fn() +type CenterOutcome = 'centered' | 'clamped' | 'not-found' +// Centering answers with the outcome; unless a test says otherwise the hit was reached. +const mockCenterOn = jest.fn, [T.Chat.MessageID, string]>() const mockClearCenter = jest.fn() const mockToggleThreadSearch = jest.fn() const mockCancelSearch = jest.fn() @@ -17,9 +19,9 @@ type CallMap = Record void> const mockSearchCalls: Array<{incomingCallMap: CallMap; query: string}> = [] const mockLastOrdinal = {current: T.Chat.numberToOrdinal(0)} -jest.mock('./center-context', () => ({ +jest.mock('./centering', () => ({ useConversationCenterActions: () => ({ - centerOnMessage: mockCenterOnMessage, + centerOn: mockCenterOn, clearCenter: mockClearCenter, jumpToRecent: () => {}, }), @@ -101,6 +103,7 @@ const deliverDone = () => { } beforeEach(() => { + mockCenterOn.mockResolvedValue('centered') jest.useFakeTimers() useCurrentUserState.getState().dispatch.setBootstrap({ deviceID: 'device-id', @@ -114,7 +117,8 @@ afterEach(() => { cleanup() jest.useRealTimers() mockSearchCalls.length = 0 - mockCenterOnMessage.mockClear() + mockCenterOn.mockClear() + mockCenterOn.mockResolvedValue('centered') mockClearCenter.mockClear() mockToggleThreadSearch.mockClear() mockCancelSearch.mockClear() @@ -303,7 +307,7 @@ describe('navigation', () => { test('the first hit is auto-selected and centered', () => { const {result} = mountWithHits(3) expect(result.current.selectedIndex).toBe(0) - expect(mockCenterOnMessage).toHaveBeenCalledWith(messageID(10), 'always') + expect(mockCenterOn).toHaveBeenCalledWith(messageID(10), 'always') }) test('onUp walks forward through the hits and wraps at the end', () => { @@ -342,33 +346,33 @@ describe('navigation', () => { act(() => result.current.onUp()) act(() => result.current.onDown()) expect(result.current.selectedIndex).toBe(0) - expect(mockCenterOnMessage).not.toHaveBeenCalled() + expect(mockCenterOn).not.toHaveBeenCalled() }) test('every move centers on the matching message', () => { const {result} = mountWithHits(3) - mockCenterOnMessage.mockClear() + mockCenterOn.mockClear() act(() => result.current.onUp()) - expect(mockCenterOnMessage).toHaveBeenCalledWith(messageID(11), 'always') + expect(mockCenterOn).toHaveBeenCalledWith(messageID(11), 'always') act(() => result.current.onDown()) - expect(mockCenterOnMessage).toHaveBeenCalledWith(messageID(10), 'always') + expect(mockCenterOn).toHaveBeenCalledWith(messageID(10), 'always') }) test('selectResult jumps directly to an index', () => { const {result} = mountWithHits(3) act(() => result.current.selectResult(2)) expect(result.current.selectedIndex).toBe(2) - expect(mockCenterOnMessage).toHaveBeenLastCalledWith(messageID(12), 'always') + expect(mockCenterOn).toHaveBeenLastCalledWith(messageID(12), 'always') }) test('selectResult out of range leaves the selection where it was', () => { const {result} = mountWithHits(2) act(() => result.current.selectResult(1)) - mockCenterOnMessage.mockClear() + mockCenterOn.mockClear() act(() => result.current.selectResult(7)) // a bogus index would surface as `8 of 2` and send the up/down walk adrift expect(result.current.selectedIndex).toBe(1) - expect(mockCenterOnMessage).not.toHaveBeenCalled() + expect(mockCenterOn).not.toHaveBeenCalled() }) test('onUp steps over a hit it cannot center on instead of wedging', () => { @@ -381,7 +385,7 @@ describe('navigation', () => { act(() => result.current.onUp()) expect(result.current.selectedIndex).toBe(2) - expect(mockCenterOnMessage).toHaveBeenLastCalledWith(messageID(12), 'always') + expect(mockCenterOn).toHaveBeenLastCalledWith(messageID(12), 'always') act(() => result.current.onUp()) expect(result.current.selectedIndex).toBe(0) }) @@ -396,15 +400,65 @@ describe('navigation', () => { expect(result.current.selectedIndex).toBe(0) }) + // The counter used to advance on `!!message.id` alone, so a hit the thread could not produce - + // expunged, or outside what the centered load came back with - left `n of m` claiming a row that + // never rendered. + test('a hit the thread cannot produce hands the counter back', async () => { + const {result} = mountWithHits(3) + expect(result.current.selectedIndex).toBe(0) + mockCenterOn.mockResolvedValue('not-found') + + act(() => result.current.onUp()) + // taken optimistically, so the counter answers the keypress + expect(result.current.selectedIndex).toBe(1) + await act(async () => { + await Promise.resolve() + }) + expect(result.current.selectedIndex).toBe(0) + }) + + test('a hit the list could only clamp onto still counts as reached', async () => { + // A hit within half a viewport of either end of the thread cannot be put in the middle, but it + // is on screen and it is where the reader was sent. + const {result} = mountWithHits(3) + mockCenterOn.mockResolvedValue('clamped') + + act(() => result.current.onUp()) + await act(async () => { + await Promise.resolve() + }) + expect(result.current.selectedIndex).toBe(1) + }) + + test('a later selection owns the counter, however the earlier one settled', async () => { + const {result} = mountWithHits(3) + let settleFirst: ((outcome: CenterOutcome) => void) | undefined + mockCenterOn.mockReturnValueOnce( + new Promise(resolve => { + settleFirst = resolve + }) + ) + + act(() => result.current.onUp()) + expect(result.current.selectedIndex).toBe(1) + act(() => result.current.onUp()) + expect(result.current.selectedIndex).toBe(2) + await act(async () => { + settleFirst?.('not-found') + await Promise.resolve() + }) + expect(result.current.selectedIndex).toBe(2) + }) + test('a walk with nothing selectable leaves the selection alone', () => { const {result} = mountSearch('needle') deliverHits(hitMessage(0), hitMessage(0, {bodySummary: 'other'})) deliverDone() - mockCenterOnMessage.mockClear() + mockCenterOn.mockClear() act(() => result.current.onUp()) act(() => result.current.onDown()) expect(result.current.selectedIndex).toBe(0) - expect(mockCenterOnMessage).not.toHaveBeenCalled() + expect(mockCenterOn).not.toHaveBeenCalled() }) }) diff --git a/shared/chat/conversation/search.tsx b/shared/chat/conversation/search.tsx index 09b43fcbbc76..3f1bb8087761 100644 --- a/shared/chat/conversation/search.tsx +++ b/shared/chat/conversation/search.tsx @@ -6,7 +6,7 @@ import * as Kb from '@/common-adapters' import {RPCError} from '@/util/errors' import {formatTimeForMessages} from '@/util/timestamp' import {useCurrentUserState} from '@/stores/current-user' -import {useConversationCenterActions} from './center-context' +import {useConversationCenterActions} from './centering' import {cancelActiveThreadSearchRPC, searchInboxRPC} from '../search-rpc' import { useConversationThreadID, @@ -100,7 +100,7 @@ const runSearchInbox = async (p: { export const useCommon = (ownProps: CommonProps) => { const {conversationIDKey, initialQuery, style} = ownProps const toggleThreadSearch = useConversationThreadToggleSearch() - const {centerOnMessage, clearCenter} = useConversationCenterActions() + const {centerOn, clearCenter} = useConversationCenterActions() const onToggleThreadSearch = () => { clearCenter() toggleThreadSearch() @@ -235,34 +235,49 @@ export const useCommon = (ownProps: CommonProps) => { runThreadSearch(text) } - // returns whether the index was taken; the index feeds the `n of m` counter and - // the up/down walk, so never record one we can't center on - const [selectHit] = React.useState(() => (index: number) => { + // The index feeds the `n of m` counter and the up/down walk, so it may only rest on a hit the + // thread actually reached. Taken optimistically - the counter should answer the keypress, not the + // round trip - then given back if centering reports the message was never in the thread. + // 'clamped' counts as reached: a hit within half a viewport of either end cannot be put in the + // middle, but it is on screen and it is where the reader was sent. Only 'not-found' means the + // thread came back without the message at all, and leaving the counter parked on a row that never + // rendered is what used to make `n of m` lie. + const selectRequestRef = React.useRef(0) + const [selectHit] = React.useState(() => (index: number, previousIndex: number) => { const message = hitsRef.current[index] if (!message?.id) { return false } - centerOnMessage(message.id, 'always') + const request = ++selectRequestRef.current setSelectedIndex(index) + const settle = async () => { + const outcome = await centerOn(message.id, 'always') + // A later selection owns the counter now. + if (selectRequestRef.current !== request || outcome !== 'not-found') { + return + } + setSelectedIndex(previousIndex) + } + void settle() return true }) - // walk in `delta`'s direction until we land on a hit we can center on, so a hit - // we can't center on can never wedge the walk in place + // walk in `delta`'s direction until we land on a hit that has an id at all, so a hit with none + // can never wedge the walk in place const step = (delta: 1 | -1) => { if (!numHits) { return } for (let moved = 1; moved <= numHits; ++moved) { const index = (((selectedIndex + delta * moved) % numHits) + numHits) % numHits - if (selectHit(index)) { + if (selectHit(index, selectedIndex)) { return } } } const selectResult = (index: number) => { - selectHit(index) + selectHit(index, selectedIndex) } const onUp = () => { @@ -311,7 +326,8 @@ export const useCommon = (ownProps: CommonProps) => { React.useEffect(() => { if (hasHits && !hadHitsRef.current) { hadHitsRef.current = true - selectHit(0) + // The first hit of a fresh search: there is nothing to hand the counter back to. + selectHit(0, 0) } else if (!hasHits) { hadHitsRef.current = false } From fab870caabc350ea8b10f8b6eaf5e1580e8ae4e8 Mon Sep 17 00:00:00 2001 From: chrisnojima Date: Wed, 9 Sep 2026 19:29:34 -0400 Subject: [PATCH 2/3] fix(chat): answer a centering request for what the thread did, not the clock `not-found` was reported after three seconds of wall clock, whatever the thread was doing. It is the one outcome a caller reads as "this hit is unreachable" - search hands its counter back on it - so a slow RPC retracted a hit that was moments from arriving, which is the same lie the counter used to tell, arrived at from the other direction. It is now judged on the reload this request asked for having finished: a new generation, loaded, and the message not in the window. If the window never settles at all the honest answer is not `not-found`, so that stays `clamped` and the caller's optimistic answer stands. The retreat in search was half a retreat. centerOn had already cleared and reloaded the thread around a message it turned out not to hold, so handing back only the counter left the reader on a window centered on nothing while `n of m` named a row somewhere else - and for the first hit of a fresh search, which comes in as select(0, 0), the rollback did nothing at all. It now goes back to the hit it came from, centre included, or gives the centre up when there is nowhere to go. One step, not a walk: the retreat passes itself as its own previous, so a second miss lands on the give-up branch instead of unwinding the list. onScrollToIndexFailed scheduled six retries against a cap of five. The ordinal-in-window test both the centered selector and the watcher need is one function now, so the question "is this message actually in the thread" cannot be answered two ways. Also binds a correction to the request that started it. Waiting for the adapter can park a correction for the whole timeout, and the existing signal check passes for a stale resumption that overwrites the newer signal on its way through. I could not build an interleaving that reaches it - abortCorrection covers the orderings I could construct - so this is hardening of an invariant the module already relies on elsewhere, not a fix for a demonstrated failure, and it carries no test for that reason. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01RYPqG8z11HmGRn5MHSu89S --- shared/chat/conversation/centering.test.tsx | 30 ++++++++- shared/chat/conversation/centering.tsx | 65 +++++++++++++++----- shared/chat/conversation/list-area/index.tsx | 2 +- shared/chat/conversation/search.test.ts | 35 +++++++++++ shared/chat/conversation/search.tsx | 48 ++++++++++----- 5 files changed, 145 insertions(+), 35 deletions(-) diff --git a/shared/chat/conversation/centering.test.tsx b/shared/chat/conversation/centering.test.tsx index f005ed7f4dfe..163de7bc93a4 100644 --- a/shared/chat/conversation/centering.test.tsx +++ b/shared/chat/conversation/centering.test.tsx @@ -25,7 +25,11 @@ const seededMessages = [ {id: 33, ordinal: 32.001}, {id: 42, ordinal: 42}, ] -const mockSnapshot = { +// Mutable, and read through `getState()` on every poll: 'not-found' is now judged on the reload +// this request asked for having finished, so a test has to be able to land one. +let mockSnapshot = { + generation: 0, + loaded: false, messageIDToOrdinal: new Map( seededMessages.map(m => [T.Chat.numberToMessageID(m.id), T.Chat.numberToOrdinal(m.ordinal)]) ), @@ -43,6 +47,11 @@ const mockSnapshot = { moreToLoadForward: false, pendingOutboxToOrdinal: new Map(), } +const initialSnapshot = mockSnapshot +// The window this request asked for coming back, with or without the message in it. +const landWindow = () => { + mockSnapshot = {...mockSnapshot, generation: mockSnapshot.generation + 1, loaded: true} +} // Both providers under test pull thread/engine plumbing they don't exercise here. jest.mock('./thread-context', () => ({ @@ -417,6 +426,7 @@ const CenterHarness = (p: {adapter?: CenterScrollAdapter; messageID: number}) => describe('centerOn reports what actually happened', () => { beforeEach(() => { + mockSnapshot = initialSnapshot jest.useFakeTimers() }) afterEach(() => { @@ -446,7 +456,7 @@ describe('centerOn reports what actually happened', () => { }) }) - test('a message the thread never came back with is reported as not found', async () => { + test('a message the thread came back without is reported as not found', async () => { // The `n of m` counter used to advance for these anyway, because the only thing asked was // whether the hit had an id at all. render( @@ -454,10 +464,24 @@ describe('centerOn reports what actually happened', () => { ) - await drain(3300) + // The reload lands, and 9999 is not in it. + landWindow() + await drain(300) expect(outcome).toBe('not-found') }) + test('a reload that has not come back yet does not retract the hit', async () => { + // 'not-found' is the one outcome search hands its counter back on, so a slow RPC must not + // produce it off a stopwatch: the message may be moments from arriving. + render( + + + + ) + await drain(3300) + expect(outcome).toBe('clamped') + }) + test('a list that cannot reach the row reports a clamp rather than a success', async () => { const pinned: CenterScrollAdapter = { measureTarget: () => measured(500, 0), diff --git a/shared/chat/conversation/centering.tsx b/shared/chat/conversation/centering.tsx index d55f93e00da2..3d25744508be 100644 --- a/shared/chat/conversation/centering.tsx +++ b/shared/chat/conversation/centering.tsx @@ -7,6 +7,7 @@ import sortedIndexOf from 'lodash/sortedIndexOf' import {useChatThreadRouteParams} from './thread-search-route' import {useRequestWindow} from './thread-window' import { + type ConversationThreadState, useConversationThreadSelector, useConversationThreadSetMarkReadBlocked, useConversationThreadStore, @@ -202,6 +203,17 @@ type CenterScrollContextType = { takeEndAnchor: () => void } +const ordinalInWindow = (snapshot: ConversationThreadState, messageID: T.Chat.MessageID) => { + const found = getOrdinalForMessageIDInSnapshot(snapshot, messageID) + if (found === null) { + return undefined + } + const ordinals = snapshot.messageOrdinals + return ordinals && sortedIndexOf(ordinals as unknown as number[], found as unknown as number) >= 0 + ? found + : undefined +} + const missingContext = () => { throw new Error('Missing ConversationCenteringProvider in the tree') } @@ -295,19 +307,9 @@ export const ConversationCenteringProvider = function ConversationCenteringProvi // window is the only way to get the right one. The row has to be in the window as well as in the // map: a message the thread holds but does not render has nothing to scroll to, and reporting its // ordinal would send the corrector after a row that never mounts. - const centeredOrdinal = useConversationThreadSelector(s => { - if (!target) { - return undefined - } - const found = getOrdinalForMessageIDInSnapshot(s, target.messageID) - if (found === null) { - return undefined - } - const ordinals = s.messageOrdinals - return ordinals && sortedIndexOf(ordinals as unknown as number[], found as unknown as number) >= 0 - ? found - : undefined - }) + const centeredOrdinal = useConversationThreadSelector(s => + target ? ordinalInWindow(s, target.messageID) : undefined + ) const scrollRef = React.useRef({ adapter: undefined, @@ -438,8 +440,20 @@ export const ConversationCenteringProvider = function ConversationCenteringProvi // ordinal rather than a "did it change" flag, so the correction still runs when the thread finishes // loading after the target was set. const lastCorrectedRef = React.useRef(undefined) + // Names the current centering request. Everything below that can resume after an await checks it + // before touching shared state. + const centerRequestRef = React.useRef(0) const correctOnto = React.useEffectEvent(async (ordinal: T.Chat.Ordinal) => { + // Bound to the request that started it. Waiting for the adapter can park this for the whole + // timeout, long enough for a newer centerOn to install its own pending and its own correction - + // and a stale resumption checking only `correction === signal` would pass that check by + // overwriting the newer signal on its way through, then answer the newer request with this + // one's outcome and cancel the correction actually steering the list. + const request = centerRequestRef.current const adapter = await waitForAdapter() + if (centerRequestRef.current !== request) { + return + } if (!adapter) { settlePending('clamped') return @@ -448,6 +462,9 @@ export const ConversationCenteringProvider = function ConversationCenteringProvi const signal = {cancelled: false} scrollRef.current.correction = signal const outcome = await runCenterCorrection({adapter, ordinal, signal, sleep}) + if (centerRequestRef.current !== request) { + return + } if (scrollRef.current.correction === signal) { scrollRef.current.correction = undefined settlePending(outcome) @@ -490,20 +507,35 @@ export const ConversationCenteringProvider = function ConversationCenteringProvi // The thread came back without the message: nothing is going to correct onto it, so answer for the // request rather than leaving the caller waiting on a row that will never render. + // + // Judged on the reload this request asked for having finished, not on elapsed time. 'not-found' is + // the one outcome a caller reads as "this hit is unreachable" - search hands its counter back on + // it - so reporting it off a stopwatch would mean a slow RPC retracts a hit that is about to + // arrive. If the window never settles at all, the honest answer is not 'not-found': say 'clamped' + // and leave the caller's optimistic answer standing. const watchForMissingMessage = React.useEffectEvent(async (pending: PendingCenter) => { + const generationAtRequest = store.getState().generation for (let elapsed = 0; elapsed <= centerTimeoutMs; elapsed += measurePollMs) { if (scrollRef.current.pending !== pending || pending.corrected) { return } + const snapshot = store.getState() + // The window this request asked for, done loading. + if (snapshot.generation !== generationAtRequest && snapshot.loaded) { + if (ordinalInWindow(snapshot, pending.messageID) === undefined) { + settlePending('not-found') + } + // Otherwise the row is here and the correction owns it from now on. + return + } await sleep(measurePollMs) } - if (scrollRef.current.pending === pending && !pending.corrected) { - settlePending('not-found') - } + settlePending('clamped') }) const runCenterOn = React.useEffectEvent( async (messageID: T.Chat.MessageID, highlightMode: T.Chat.CenterOrdinalHighlightMode) => { + centerRequestRef.current += 1 settlePending('clamped') abortCorrection() takeCentering() @@ -520,6 +552,7 @@ export const ConversationCenteringProvider = function ConversationCenteringProvi } ) const runJumpToRecent = React.useEffectEvent(() => { + centerRequestRef.current += 1 clearTarget() requestWindow({anchor: 'newest', reason: 'jump to recent'}) }) diff --git a/shared/chat/conversation/list-area/index.tsx b/shared/chat/conversation/list-area/index.tsx index dee16c47e332..5b3a413828b6 100644 --- a/shared/chat/conversation/list-area/index.tsx +++ b/shared/chat/conversation/list-area/index.tsx @@ -768,7 +768,7 @@ const NativeConversationList = function NativeConversationList() { scrollFailRetryRef.current = 0 }, [centeredOrdinalOrNone]) const [onScrollToIndexFailed] = React.useState(() => () => { - if (scrollFailRetryRef.current > maxScrollToIndexRetries) { + if (scrollFailRetryRef.current >= maxScrollToIndexRetries) { return } scrollFailRetryRef.current += 1 diff --git a/shared/chat/conversation/search.test.ts b/shared/chat/conversation/search.test.ts index f14dacbb181a..4137db26cbfe 100644 --- a/shared/chat/conversation/search.test.ts +++ b/shared/chat/conversation/search.test.ts @@ -417,6 +417,41 @@ describe('navigation', () => { expect(result.current.selectedIndex).toBe(0) }) + test('the retreat takes the centre back with it, not just the counter', async () => { + // centerOn cleared and reloaded the thread around a message it turned out not to hold. Handing + // the counter back without moving the centre leaves the reader on a window centered on nothing + // while `n of m` names a row somewhere else. + const {result} = mountWithHits(3) + expect(result.current.selectedIndex).toBe(0) + mockCenterOn.mockClear() + mockCenterOn.mockResolvedValueOnce('not-found') + mockCenterOn.mockResolvedValue('centered') + + act(() => result.current.onUp()) + expect(result.current.selectedIndex).toBe(1) + await act(async () => { + await Promise.resolve() + }) + + expect(result.current.selectedIndex).toBe(0) + // Re-centred on the hit it came from, rather than left pointing at the missing one. + expect(mockCenterOn).toHaveBeenLastCalledWith(messageID(10), 'always') + }) + + test('a retreat with nowhere to go gives up the centre', async () => { + // The first hit of a fresh search is selected as select(0, 0), so there is no earlier hit to + // fall back to. Holding a centre the thread cannot show is worse than holding none. + mockCenterOn.mockResolvedValue('not-found') + mockClearCenter.mockClear() + const {result} = mountWithHits(3) + await act(async () => { + await Promise.resolve() + }) + + expect(result.current.selectedIndex).toBe(0) + expect(mockClearCenter).toHaveBeenCalled() + }) + test('a hit the list could only clamp onto still counts as reached', async () => { // A hit within half a viewport of either end of the thread cannot be put in the middle, but it // is on screen and it is where the reader was sent. diff --git a/shared/chat/conversation/search.tsx b/shared/chat/conversation/search.tsx index 3f1bb8087761..1196ffa96bb4 100644 --- a/shared/chat/conversation/search.tsx +++ b/shared/chat/conversation/search.tsx @@ -243,23 +243,41 @@ export const useCommon = (ownProps: CommonProps) => { // thread came back without the message at all, and leaving the counter parked on a row that never // rendered is what used to make `n of m` lie. const selectRequestRef = React.useRef(0) - const [selectHit] = React.useState(() => (index: number, previousIndex: number) => { - const message = hitsRef.current[index] - if (!message?.id) { - return false - } - const request = ++selectRequestRef.current - setSelectedIndex(index) - const settle = async () => { - const outcome = await centerOn(message.id, 'always') - // A later selection owns the counter now. - if (selectRequestRef.current !== request || outcome !== 'not-found') { - return + const [selectHit] = React.useState(() => { + const select = (index: number, previousIndex: number): boolean => { + const message = hitsRef.current[index] + if (!message?.id) { + return false + } + const request = ++selectRequestRef.current + setSelectedIndex(index) + const settle = async () => { + const outcome = await centerOn(message.id, 'always') + // A later selection owns the counter now. + if (selectRequestRef.current !== request || outcome !== 'not-found') { + return + } + // Putting the counter back is only half of the retreat. centerOn cleared and reloaded the + // thread around a message it turned out not to hold, so the centre is still on that message: + // leaving it there parks the reader on a window centered on nothing while the counter names + // a row somewhere else. Go back to the hit we came from, centre included. + const previous = hitsRef.current[previousIndex] + // Nowhere to retreat to: the first hit of a fresh search comes in as select(0, 0), and a + // previous hit with no id was never reachable either. Give up the centre rather than hold + // one the thread cannot show. + if (previousIndex === index || !previous?.id) { + setSelectedIndex(previousIndex) + clearCenter() + return + } + // One step, not a walk: the retreat passes itself as its own previous, so if that hit is + // missing too it lands on the branch above instead of unwinding the whole list. + select(previousIndex, previousIndex) } - setSelectedIndex(previousIndex) + void settle() + return true } - void settle() - return true + return select }) // walk in `delta`'s direction until we land on a hit that has an id at all, so a hit with none From b77569c9a05570d5281aa8413ee9d2ae1eddafa0 Mon Sep 17 00:00:00 2001 From: chrisnojima Date: Wed, 9 Sep 2026 19:44:17 -0400 Subject: [PATCH 3/3] fix(chat): never measure the native centre against nothing Moved here from the row-identity branch, where it was written: it is this module's code, and this is the PR that should carry it. The native adapter measures in index space and converts to pixels with the average row height, so every value it reports scales by that average. Fed a content height the list has not reported yet, the average is zero - and `offBy` and `tolerance` are then both zero, which is indistinguishable from "already in the middle". The corrector counted its three settled readings, returned 'centered' without ever issuing a scroll, and search recorded the hit as reached: `n of m` parked on a row the reader was never taken to. The viewable range had the same shape of problem from the other side. It is indices into one window, and dropping the window renumbers all of them, but nothing cleared it. Before this branch that was survivable because the coarse scrollToItem went out first and again at 50/250ms; it stopped going out, because `measureTarget` asked for the anchor only when the target was missing from the window and the corrector only runs once the target is confirmed present - so scrollToIndex and the onScrollToIndexFailed retry behind it had become unreachable. The range and the height it is scaled by are now dropped with the generation they describe - value compared against a ref, not a dependency, because a react-native-screens freeze/thaw re-mounts effects while the refs survive and zeroing good geometry on a thaw would leave nothing to restore it. The three ways the measurement can be fed nothing useful ask for the anchor instead of reporting a measurement, which is what makes the coarse anchor reachable again on first mount and after every window swap. A briefly absent range is not one of those three. The list reports an empty viewable set for a frame whenever a scroll lands where its cells have not rendered yet, and re-anchoring there throws away a fine correction that may be one reading from settling. With a known scale that is a `pending` poll. `offscreen` becomes `needs-anchor`: both adapters used it to mean "no trustworthy frame of reference yet", and the corrector's answer to it has always been to coarse-anchor and re-poll. The arithmetic moves to measureNativeCenter, pure and apart from the refs that feed it, because that is the half worth testing. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01RYPqG8z11HmGRn5MHSu89S --- shared/chat/conversation/centering.test.tsx | 65 +++++++++++++++++- shared/chat/conversation/centering.tsx | 52 ++++++++++++++- shared/chat/conversation/list-area/index.tsx | 69 +++++++++++++------- 3 files changed, 157 insertions(+), 29 deletions(-) diff --git a/shared/chat/conversation/centering.test.tsx b/shared/chat/conversation/centering.test.tsx index 163de7bc93a4..7c98f695cedd 100644 --- a/shared/chat/conversation/centering.test.tsx +++ b/shared/chat/conversation/centering.test.tsx @@ -71,6 +71,7 @@ import { type CenterOutcome, type CenterScrollAdapter, ConversationCenteringProvider, + measureNativeCenter, runCenterCorrection, runEndAnchorCorrection, useConversationCenter, @@ -285,8 +286,8 @@ describe('the centering scroll corrector', () => { test('a row outside the rendered window is scrolled to by index first', async () => { const {adapter, calls} = scriptedAdapter([ - {kind: 'offscreen'}, - {kind: 'offscreen'}, + {kind: 'needs-anchor'}, + {kind: 'needs-anchor'}, measured(0, 700), measured(0, 700), measured(0, 700), @@ -353,6 +354,66 @@ describe('the centering scroll corrector', () => { }) }) +describe('the native index-space measurement', () => { + const run = async (adapter: CenterScrollAdapter, signal = {cancelled: false}) => + runCenterCorrection({adapter, ordinal, signal, sleep: immediately}) + + // Every one of these used to come back as a `measured` reading whose offBy and tolerance were both + // zero, which the corrector cannot tell from "already in the middle": it counts its three settled + // readings, returns 'centered', and never issues a scroll. Search then records the hit as reached + // and parks `n of m` on a row the reader was never taken to. + const base = {contentHeight: 4000, first: 0, last: 9, num: 20, scroll: 500, targetIdx: 15} + + test('a normal reading measures in index space and scales by the average row height', () => { + // centre of the viewable range is index 4.5, the target is 15, so 10.5 rows below it at 200px a + // row, damped by 0.9. + expect(measureNativeCenter(base)).toEqual({ + kind: 'measured', + offBy: 10.5 * 200 * 0.9, + scroll: 500, + tolerance: 0.5 * 200 * 0.9, + }) + }) + + test('a content height the list has not reported yet asks for the anchor, not a zero deadband', () => { + expect(measureNativeCenter({...base, contentHeight: 0})).toEqual({kind: 'needs-anchor'}) + }) + + test('a transient empty viewable range is waited out, not re-anchored', () => { + // The list reports an empty viewable set for a frame after a scroll lands somewhere its cells + // have not rendered yet. Answering that with the coarse anchor would throw away a fine + // correction that may be one reading from settling. + expect(measureNativeCenter({...base, first: undefined, last: undefined})).toEqual({ + kind: 'pending', + }) + expect(measureNativeCenter({...base, last: null})).toEqual({kind: 'pending'}) + }) + + test('no range and no scale asks for the anchor', () => { + // On first mount and after the window is dropped both are cleared together, and it is the + // missing scale that puts the coarse anchor back in play. + expect( + measureNativeCenter({...base, contentHeight: 0, first: undefined, last: undefined}) + ).toEqual({kind: 'needs-anchor'}) + }) + + test('a target the window does not hold asks for the anchor', () => { + expect(measureNativeCenter({...base, targetIdx: -1})).toEqual({kind: 'needs-anchor'}) + expect(measureNativeCenter({...base, num: 0})).toEqual({kind: 'needs-anchor'}) + }) + + test('a needs-anchor reading can never be mistaken for a settled one', async () => { + // The whole point of the guards above: drive the corrector with what a zero content height used + // to produce and it must not report 'centered' off readings it never scrolled for. + const degenerate: CenterMeasurement = {kind: 'measured', offBy: 0, scroll: 0, tolerance: 0} + const {adapter, calls} = scriptedAdapter([degenerate, degenerate, degenerate]) + await expect(run(adapter)).resolves.toBe('centered') + expect(calls.scrollToOffset).toEqual([]) + // ...which is exactly why the adapter must not hand that shape over in the first place. + expect(measureNativeCenter({...base, contentHeight: 0})).toEqual({kind: 'needs-anchor'}) + }) +}) + describe('the end anchor corrector', () => { const run = async ( read: () => {isAtEnd: boolean; scroll: number} | undefined, diff --git a/shared/chat/conversation/centering.tsx b/shared/chat/conversation/centering.tsx index 3d25744508be..1dba1203cf4e 100644 --- a/shared/chat/conversation/centering.tsx +++ b/shared/chat/conversation/centering.tsx @@ -25,8 +25,11 @@ export type CenterMeasurement = // oscillating; `scroll` is the offset it was measured at; `tolerance` is how close this list can // realistically get, below which chasing the remainder only fights the list's own adjustments. | {kind: 'measured'; offBy: number; scroll: number; tolerance: number} - // The row is outside the rendered window, so there is nothing to measure until it mounts. - | {kind: 'offscreen'} + // No trustworthy frame of reference yet: the row has not mounted, or the list has not reported + // the geometry the measurement is taken against. The corrector answers with the coarse anchor - + // scrollToIndex - and re-polls, so this is the signal that asks to be put in the neighbourhood + // before anything tries to measure a remainder. + | {kind: 'needs-anchor'} // Measurable, but not against anything current: the list has not reported a fresh position since // the last correction, and correcting off a stale one overshoots. | {kind: 'pending'} @@ -66,6 +69,49 @@ const settledChecks = 3 // are pinned against an edge - stop rather than spin. const pinnedChecksToClamp = 3 +// The native list cannot measure a row's offset directly (inverted list + custom keyboard +// scrollview + tall variable-height image rows all make scrollToItem land wrong), so it measures in +// index space - the reported viewable range against the target's index - and converts to pixels with +// the average row height. Pure, and separated from the refs that feed it, because every way this +// arithmetic can be fed nothing useful ends in the same silent failure: an `offBy` and a `tolerance` +// that are both zero read as "already centred" to the corrector, which then settles without ever +// scrolling and reports 'centered' for a row nobody moved to. +const nativeCenterDamping = 0.9 +export const measureNativeCenter = (p: { + contentHeight: number + first: number | null | undefined + last: number | null | undefined + num: number + scroll: number + targetIdx: number +}): CenterMeasurement => { + const {contentHeight, first, last, num, scroll, targetIdx} = p + // The row is not in the window, so there is no index to measure against. + if (!num || targetIdx < 0) return {kind: 'needs-anchor'} + const avgH = contentHeight / num + // No content height means no scale, and at zero the deadband collapses onto the offset, so every + // reading would come back as already centred. This is also the state the list is left in on first + // mount and whenever the window is dropped, which is what puts the coarse anchor back in play at + // exactly the two moments an index-space measurement has nothing trustworthy to stand on. + if (!(avgH > 0)) return {kind: 'needs-anchor'} + // Scale but no range: the list reports an empty viewable set for a frame whenever a scroll lands + // somewhere its cells have not rendered yet. That is a gap to wait out, not a reason to re-anchor + // - answering it with the coarse scroll would throw away a correction that may be one reading away + // from settling and yank the thread back to the middle of nowhere. + if (first == null || last == null) return {kind: 'pending'} + const centerIdx = (first + last) / 2 + const diff = targetIdx - centerIdx + return { + kind: 'measured', + // higher index = older = higher offset, damped to avoid overshoot/oscillation + offBy: diff * avgH * nativeCenterDamping, + scroll, + // half a row, expressed through the same damping so the deadband stays the index-space half-row + // it has always been + tolerance: 0.5 * avgH * nativeCenterDamping, + } +} + export const runCenterCorrection = async (p: { adapter: CenterScrollAdapter ordinal: T.Chat.Ordinal @@ -79,7 +125,7 @@ export const runCenterCorrection = async (p: { let scrollAtLastRequest: number | undefined for (let elapsed = 0; elapsed < centerTimeoutMs && !signal.cancelled; ) { const measurement = adapter.measureTarget(ordinal) - if (measurement.kind === 'offscreen') { + if (measurement.kind === 'needs-anchor') { adapter.scrollToIndex(ordinal) settled = 0 pinnedChecks = 0 diff --git a/shared/chat/conversation/list-area/index.tsx b/shared/chat/conversation/list-area/index.tsx index 5b3a413828b6..44e6bb9fd316 100644 --- a/shared/chat/conversation/list-area/index.tsx +++ b/shared/chat/conversation/list-area/index.tsx @@ -10,7 +10,12 @@ import {MessageRow} from '../messages/wrapper' import {RowHoveredContext} from '../messages/ids-context' import {PerfProfiler} from '@/perf/react-profiler' import {ThreadRefsContext} from '../normal/context' -import {type CenterScrollAdapter, useConversationCenter, useConversationCenterScroll} from '../centering' +import { + type CenterScrollAdapter, + measureNativeCenter, + useConversationCenter, + useConversationCenterScroll, +} from '../centering' import { ShownUsernameCacheContext, useConversationThreadID, @@ -107,9 +112,9 @@ const usePagination = () => { } const centerTolerancePx = 8 -// Native measures in index space, so its deadband is half a row and its steps are damped to keep an -// inverted list of tall image rows from oscillating around the target. -const nativeCenterDamping = 0.9 +// Native measures in index space (measureNativeCenter, in the centering module, does the arithmetic), +// so its budgets are about how long to keep chasing a moving target on an inverted list of tall +// image rows rather than about pixels. const maxNativeCenterCorrections = 12 const maxStaleRangeReads = 3 const maxScrollToIndexRetries = 5 @@ -340,7 +345,7 @@ const DesktopThreadWrapper = function DesktopThreadWrapper() { | (ElLike & {querySelector: (s: string) => ElLike | null}) | null const el = wrapper ? wrapper.querySelector(`[data-ordinal="${ordinal}"]`) : null - if (!wrapper || !el) return {kind: 'offscreen'} + if (!wrapper || !el) return {kind: 'needs-anchor'} const scroll = listRef.current?.getState().scroll if (scroll === undefined) return {kind: 'pending'} const elRect = el.getBoundingClientRect() @@ -639,7 +644,7 @@ const NativeConversationList = function NativeConversationList() { > const conversationIDKey = useConversationThreadID() - const {loaded, ordinals} = useThreadWindow() + const {generation, loaded, ordinals} = useThreadWindow() const {centeredHighlightOrdinal, centeredOrdinal, hasCenter} = useConversationCenter() const noCenteredOrdinal = T.Chat.numberToOrdinal(-1) const centeredOrdinalOrNone = centeredOrdinal ?? noCenteredOrdinal @@ -701,6 +706,27 @@ const NativeConversationList = function NativeConversationList() { }, [messageOrdinals]) const vFirstRef = React.useRef(undefined) const vLastRef = React.useRef(undefined) + // The range is indices into one window. Dropping the window renumbers every one of them, and the + // list does not report a fresh range until it scrolls, so a correction taken between the two would + // be computed off indices that no longer point at anything. Forget it, and the content height it + // was scaled by, with the generation they belong to and let the corrector coarse-anchor its way + // back. + // + // Value compare against a ref rather than a dependency, for the same reason numBaselineConvRef + // below does it: a react-native-screens freeze/thaw re-mounts effects while the refs survive, and + // zeroing this geometry on a thaw would throw away measurements that are still perfectly good - + // with nothing to restore them, because a list whose content and offset did not change reports + // neither a new size nor a new range. + const geometryGenerationRef = React.useRef(generation) + React.useEffect(() => { + if (geometryGenerationRef.current === generation) { + return + } + geometryGenerationRef.current = generation + vFirstRef.current = undefined + vLastRef.current = undefined + contentHeightRef.current = 0 + }, [generation]) // The viewable range only updates when the list reports one, and correcting twice off the same // reading overshoots. Bounded, though: a scroll that moves nothing never produces a new range, // and the corrector has to be allowed to see that it moved nothing. @@ -713,29 +739,24 @@ const NativeConversationList = function NativeConversationList() { maxCorrections: maxNativeCenterCorrections, measureTarget: ordinal => { const ords = ordsRef.current - const num = ords.length - const targetIdx = ords.indexOf(ordinal) - if (!num || targetIdx < 0) return {kind: 'offscreen'} - const first = vFirstRef.current - const last = vLastRef.current - if (first == null || last == null) return {kind: 'pending'} + const measurement = measureNativeCenter({ + contentHeight: contentHeightRef.current, + first: vFirstRef.current, + last: vLastRef.current, + num: ords.length, + scroll: scrollOffsetRef.current, + targetIdx: ords.indexOf(ordinal), + }) + // Only a real measurement can be stale. A request for the anchor is about what the list has + // not reported yet, and making it wait out the stale budget first would sit on its hands for + // three polls before asking to be put in the neighbourhood. + if (measurement.kind !== 'measured') return measurement if (rangeVersionRef.current === consumedRangeRef.current) { staleRangeReadsRef.current += 1 if (staleRangeReadsRef.current <= maxStaleRangeReads) return {kind: 'pending'} } staleRangeReadsRef.current = 0 - const centerIdx = (first + last) / 2 - const diff = targetIdx - centerIdx - const avgH = contentHeightRef.current / num - return { - kind: 'measured', - // higher index = older = higher offset, damped to avoid overshoot/oscillation - offBy: diff * avgH * nativeCenterDamping, - scroll: scrollOffsetRef.current, - // half a row, expressed through the same damping so the deadband stays the index-space - // half-row it has always been - tolerance: 0.5 * avgH * nativeCenterDamping, - } + return measurement }, scrollToIndex: ordinal => { if (T.Chat.ordinalToNumber(ordinal) <= 0) return