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..7c98f695cedd --- /dev/null +++ b/shared/chat/conversation/centering.test.tsx @@ -0,0 +1,606 @@ +/** @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}, +] +// 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)]) + ), + 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(), +} +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', () => ({ + 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, + measureNativeCenter, + 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: 'needs-anchor'}, + {kind: 'needs-anchor'}, + 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 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, + 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(() => { + mockSnapshot = initialSnapshot + 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 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( + + + + ) + // 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), + 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..1dba1203cf4e --- /dev/null +++ b/shared/chat/conversation/centering.tsx @@ -0,0 +1,658 @@ +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 { + type ConversationThreadState, + 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} + // 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'} + +// 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 + +// 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 + 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 === 'needs-anchor') { + 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 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') +} + +// 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 => + target ? ordinalInWindow(s, target.messageID) : 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) + // 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 + } + abortCorrection() + 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) + } + }) + 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. + // + // 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) + } + settlePending('clamped') + }) + + const runCenterOn = React.useEffectEvent( + async (messageID: T.Chat.MessageID, highlightMode: T.Chat.CenterOrdinalHighlightMode) => { + centerRequestRef.current += 1 + 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(() => { + centerRequestRef.current += 1 + 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..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 {useConversationCenter} from '../center-context' +import { + type CenterScrollAdapter, + measureNativeCenter, + useConversationCenter, + useConversationCenterScroll, +} from '../centering' import { ShownUsernameCacheContext, useConversationThreadID, @@ -107,6 +112,12 @@ const usePagination = () => { } const centerTolerancePx = 8 +// 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 // A scroller within this many pixels of its end counts as at the end. const endTolerancePx = 2 @@ -184,9 +195,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 +232,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 +269,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 +281,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 +324,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: 'needs-anchor'} + 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 +484,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 +520,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 +587,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 +608,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): @@ -792,8 +644,8 @@ const NativeConversationList = function NativeConversationList() { > const conversationIDKey = useConversationThreadID() - const {loaded, ordinals} = useThreadWindow() - const {centeredHighlightOrdinal, centeredOrdinal} = useConversationCenter() + const {generation, loaded, ordinals} = useThreadWindow() + const {centeredHighlightOrdinal, centeredOrdinal, hasCenter} = useConversationCenter() const noCenteredOrdinal = T.Chat.numberToOrdinal(-1) const centeredOrdinalOrNone = centeredOrdinal ?? noCenteredOrdinal const centeredHighlightOrdinalOrNone = centeredHighlightOrdinal ?? noCenteredOrdinal @@ -840,52 +692,112 @@ 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 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. + 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 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 + return measurement + }, + 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 +809,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 +844,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 +864,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 +898,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..4137db26cbfe 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,100 @@ 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('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. + 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..1196ffa96bb4 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,67 @@ 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) => { - const message = hitsRef.current[index] - if (!message?.id) { - return false + // 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(() => { + 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) + } + void settle() + return true } - centerOnMessage(message.id, 'always') - setSelectedIndex(index) - return true + return select }) - // 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 +344,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 }