diff --git a/shared/chat/conversation/list-area/index.tsx b/shared/chat/conversation/list-area/index.tsx index 44e6bb9fd316..7f013ca15ed8 100644 --- a/shared/chat/conversation/list-area/index.tsx +++ b/shared/chat/conversation/list-area/index.tsx @@ -16,16 +16,10 @@ import { useConversationCenter, useConversationCenterScroll, } from '../centering' -import { - ShownUsernameCacheContext, - useConversationThreadID, - useConversationThreadMarkThreadAsRead, - useConversationThreadStore, -} from '../thread-context' +import {useConversationThreadID, useConversationThreadMarkThreadAsRead} from '../thread-context' import {useJumpToRecent} from './jump-to-recent' import {useRequestWindow, useThreadWindow} from '../thread-window' -import {getMessageRowType, getMessageShowUsername} from '../messages/row-metadata' -import {useCurrentUserState} from '@/stores/current-user' +import {useRowPoolKey} from '../messages/row-identity' import * as InputState from '../input-area/input-state' import sortedIndexOf from 'lodash/sortedIndexOf' import {copyToClipboard} from '@/util/storeless-actions' @@ -45,48 +39,8 @@ import {ThreadSearchOverlayContext} from '../thread-search-overlay-context' import {useSafeAreaInsets} from 'react-native-safe-area-context' type ItemType = T.Chat.Ordinal -const noOrdinals: ReadonlyArray = [] - const keyExtractor = (ordinal: ItemType) => String(ordinal) -// Item type for list recycling pool separation. A message that leads its author group renders an -// avatar + username header (~40px taller) than a grouped follow-on of the same render type. Without -// splitting the pool, recycleItems reuses one container across both heights, so a recycled view -// paints at the wrong height for a frame before re-measure — visible as rows overlapping during -// scroll. Append ':hdr' so header and grouped rows pool separately. A row that reserves header -// space after a scroll-back load is as tall as a headered one, so it belongs in the same pool. -const useGetItemType = () => { - const threadStore = useConversationThreadStore() - const you = useCurrentUserState(s => s.username) - // Must be the same sticky cache the rows render with (wrapper.tsx): without it, a row that keeps - // its sticky header after a scroll-back load would be typed headerless here, mixing tall headered - // rows into the headerless pool and poisoning that pool's height average. - const shownCache = React.useContext(ShownUsernameCacheContext) - return React.useCallback( - (ordinal: T.Chat.Ordinal) => { - if (!ordinal) { - return 'null' - } - const {messageMap, messageTypeMap, messageOrdinals} = threadStore.getState() - const message = messageMap.get(ordinal) - if (!message) { - return messageTypeMap.get(ordinal) ?? 'text' - } - const base = getMessageRowType(message, messageTypeMap.get(ordinal)) - const {reserveHeader, showUsername} = getMessageShowUsername({ - message, - messageMap, - messageOrdinals: messageOrdinals ?? noOrdinals, - ordinal, - shownCache, - you, - }) - return showUsername || reserveHeader ? `${base}:hdr` : base - }, - [threadStore, you, shownCache] - ) -} - // ==================== SHARED ==================== // Pagination: load older at the top of the list, newer at the bottom. Whether either edge has more @@ -215,7 +169,7 @@ const DesktopThreadWrapper = function DesktopThreadWrapper() { messageOrdinalsRef.current = messageOrdinals }, [messageOrdinals]) - const getItemType = useGetItemType() + const getItemType = useRowPoolKey() // Asks the scroller, not the list's own isAtEnd: that flag comes from the content size and viewport // the list has recorded, and both lag a composer collapse, so it reads not-at-end while the scroller @@ -669,7 +623,7 @@ const NativeConversationList = function NativeConversationList() { const numOrdinals = messageOrdinals.length - const getItemType = useGetItemType() + const getItemType = useRowPoolKey() const insets = useSafeAreaInsets() const isKeyboardVisible = useKeyboardState((s: {isVisible: boolean}) => s.isVisible) diff --git a/shared/chat/conversation/messages/row-identity.test.tsx b/shared/chat/conversation/messages/row-identity.test.tsx new file mode 100644 index 000000000000..7c3a5cfd4ec9 --- /dev/null +++ b/shared/chat/conversation/messages/row-identity.test.tsx @@ -0,0 +1,322 @@ +/** @jest-environment jsdom */ +/// +import * as Message from '@/constants/chat/message' +import * as T from '@/constants/types' +import HiddenString from '@/util/hidden-string' +import {act, cleanup, render, renderHook} from '@testing-library/react' +import type * as React from 'react' +import {getRowIdentity, useRowIdentity, useRowPoolKey} from './row-identity' +import {resetAllStores} from '@/util/zustand' +import {useCurrentUserState} from '@/stores/current-user' +import { + ConversationThreadProvider, + type ConversationThreadState, + type ConversationThreadStore, + useConversationThreadActions, +} from '../thread-context' + +const convID = T.Chat.conversationIDToKey(new Uint8Array([1, 2, 3, 4])) +const ord = (n: number) => T.Chat.numberToOrdinal(n) + +const textAt = (n: number, author: string, timestamp: number) => + Message.makeMessageText({ + author, + conversationIDKey: convID, + id: T.Chat.numberToMessageID(n), + ordinal: ord(n), + text: new HiddenString(`m${n}`), + timestamp, + }) + +const placeholderAt = (n: number) => + Message.makeMessagePlaceholder({ + conversationIDKey: convID, + id: T.Chat.numberToMessageID(n), + ordinal: ord(n), + }) + +// Immer hands the store a fresh state object on every change, and the derivation is memoized per +// state object, so a test that mutates one in place would read its own stale answer back. Each of +// these is a new commit. +const snapshotOf = ( + messages: ReadonlyArray, + over: {generation?: number; messageOrdinals?: ReadonlyArray} = {} +) => + ({ + generation: over.generation ?? 0, + messageMap: new Map(messages.map(m => [m.ordinal, m])), + messageOrdinals: over.messageOrdinals ?? messages.map(m => m.ordinal), + messageTypeMap: new Map(), + }) as unknown as ConversationThreadState + +// The cache is per conversation, keyed on the store that owns the window; any stable object stands +// in for one here. +const stubStore = () => ({}) as unknown as ConversationThreadStore + +beforeEach(() => { + useCurrentUserState.getState().dispatch.setBootstrap({ + deviceID: 'device-id', + deviceName: 'testuser-mac', + uid: 'uid', + username: 'alice', + }) +}) + +afterEach(() => { + cleanup() + resetAllStores() +}) + +test('a header hides but keeps its space once a real previous message groups the row', () => { + const store = stubStore() + const message = textAt(702, 'bob', 101) + + // The oldest row of the loaded window: nothing above it yet, so it leads a group. + expect(getRowIdentity(store, snapshotOf([message]), ord(702), 'alice')).toEqual({ + poolKey: 'text:hdr', + reserveHeader: false, + showUsername: 'bob', + }) + + // An unboxing placeholder above is not an answer, so the header stays. + expect( + getRowIdentity(store, snapshotOf([placeholderAt(701), message]), ord(702), 'alice') + ).toEqual({poolKey: 'text:hdr', reserveHeader: false, showUsername: 'bob'}) + + // It resolves to a same-author message close in time: the row groups, so the header stops showing + // but keeps its space - and its pool - so the row height does not change under the load. + expect( + getRowIdentity(store, snapshotOf([textAt(701, 'bob', 100), message]), ord(702), 'alice') + ).toEqual({poolKey: 'text:hdr', reserveHeader: true, showUsername: ''}) +}) + +test('a header forced by an unresolved previous is not remembered', () => { + const store = stubStore() + const message = textAt(802, 'bob', 101) + + expect( + getRowIdentity(store, snapshotOf([placeholderAt(801), message]), ord(802), 'alice') + ).toEqual({poolKey: 'text:hdr', reserveHeader: false, showUsername: 'bob'}) + + // The placeholder unboxes into a same-author message: no header, and no space held for one - the + // neighbour's own height was about to change anyway, so there is nothing to keep stable. + expect( + getRowIdentity(store, snapshotOf([textAt(801, 'bob', 100), message]), ord(802), 'alice') + ).toEqual({poolKey: 'text', reserveHeader: false, showUsername: ''}) +}) + +test('a header shown for an ordinal outside the loaded window is not remembered', () => { + // List churn can ask about an ordinal the window no longer holds. That looks the same as "oldest + // row, nothing above it" from the previous ordinal alone, but it is not a real gap. + const store = stubStore() + const stale = textAt(901, 'bob', 101) + const live = textAt(902, 'bob', 100) + + expect( + getRowIdentity(store, snapshotOf([stale, live], {messageOrdinals: [ord(902)]}), ord(901), 'alice') + ).toEqual({poolKey: 'text:hdr', reserveHeader: false, showUsername: 'bob'}) + + // Nothing was recorded, so once it is back in the window and grouped it reserves no space. + expect( + getRowIdentity(store, snapshotOf([textAt(900, 'bob', 100), stale]), ord(901), 'alice') + ).toEqual({poolKey: 'text', reserveHeader: false, showUsername: ''}) +}) + +test('dropping the window forgets the headers it painted', () => { + // messagesClear bumps the generation, and the sticky record describes the window that is gone. + const store = stubStore() + const message = textAt(702, 'bob', 101) + getRowIdentity(store, snapshotOf([message]), ord(702), 'alice') + + expect( + getRowIdentity(store, snapshotOf([textAt(701, 'bob', 100), message], {generation: 1}), ord(702), 'alice') + ).toEqual({poolKey: 'text', reserveHeader: false, showUsername: ''}) +}) + +test('a row whose message the window no longer holds pools by its type map', () => { + const store = stubStore() + const snapshot = snapshotOf([], {messageOrdinals: [ord(500)]}) + snapshot.messageTypeMap.set(ord(500), 'attachment') + + expect(getRowIdentity(store, snapshot, ord(500), 'alice')).toEqual({ + poolKey: 'attachment', + reserveHeader: false, + showUsername: '', + }) +}) + +// The failure mode that was unreachable while the sticky map was a mutable Map handed to two +// callers: getItemType read it through the live store, the row read it through its own selector's +// snapshot, and whichever ran first decided what the other saw. A row typed headerless while it +// renders a header lands in the headerless recycling pool and poisons that pool's height average. +test('a row the list typed before it mounted paints into the pool it was given', () => { + // getItemType is asked about rows that have not rendered - that is the whole point of it - so the + // answer it gets has to be the one the row will agree with when it finally mounts. `act` flushes + // renders, so a single hook holding both callers can never show this: the row has always already + // computed by the time the pool is asked. The row consumer mounts on the second render instead. + const seen: { + actions?: ReturnType + identity?: ReturnType + poolKeyFor?: (o: T.Chat.Ordinal) => string + } = {} + // These exist to be mounted at different times and hand their hook's answer back out; writing to + // an enclosing scope is the whole job, and it is not something the compiler will compile. + const Actions = () => { + 'use no memo' + seen.actions = useConversationThreadActions() + return null + } + const Pool = () => { + 'use no memo' + seen.poolKeyFor = useRowPoolKey() + return null + } + const Row = () => { + 'use no memo' + seen.identity = useRowIdentity(ord(702)) + return null + } + const tree = (withRow: boolean) => ( + + + + {withRow ? : null} + + ) + + const {rerender} = render(tree(false)) + act(() => { + seen.actions?.applyThreadLoad({ + centered: false, + enableActiveMarkRead: false, + messages: [textAt(702, 'bob', 101)], + moreToLoad: true, + scrollDirection: 'none', + }) + }) + // Typed with nothing rendering it. + const typed = seen.poolKeyFor?.(ord(702)) + expect(typed).toBe('text:hdr') + + // A scroll-back load groups it under a same-author previous, still with nothing rendering it, so + // the sticky record is written by the list's question rather than the row's. + act(() => { + seen.actions?.applyThreadLoad({ + centered: false, + enableActiveMarkRead: false, + messages: [textAt(701, 'bob', 100)], + moreToLoad: true, + scrollDirection: 'back', + }) + }) + expect(seen.poolKeyFor?.(ord(702))).toBe('text:hdr') + + // Now it mounts: it must reserve the header height the list already pooled it for, rather than + // deciding for itself that a grouped row is headerless. + rerender(tree(true)) + expect(seen.identity).toEqual({poolKey: 'text:hdr', reserveHeader: true, showUsername: ''}) +}) + +test('the row and the list are told the same thing, whichever asks first', () => { + const wrapper = ({children}: {children: React.ReactNode}) => ( + {children} + ) + const {result} = renderHook( + () => ({ + actions: useConversationThreadActions(), + identity: useRowIdentity(ord(702)), + poolKeyFor: useRowPoolKey(), + }), + {wrapper} + ) + + // The oldest row of the window paints a header, and the list asks first. + act(() => { + result.current.actions.applyThreadLoad({ + centered: false, + enableActiveMarkRead: false, + messages: [textAt(702, 'bob', 101)], + moreToLoad: true, + scrollDirection: 'none', + }) + }) + expect(result.current.poolKeyFor(ord(702))).toBe('text:hdr') + expect(result.current.identity).toEqual({ + poolKey: 'text:hdr', + reserveHeader: false, + showUsername: 'bob', + }) + + // A scroll-back load hands it a same-author previous: the header goes, the space stays, and both + // callers still say so. + act(() => { + result.current.actions.applyThreadLoad({ + centered: false, + enableActiveMarkRead: false, + messages: [textAt(701, 'bob', 100)], + moreToLoad: true, + scrollDirection: 'back', + }) + }) + expect(result.current.identity).toEqual({ + poolKey: 'text:hdr', + reserveHeader: true, + showUsername: '', + }) + expect(result.current.poolKeyFor(ord(702))).toBe('text:hdr') +}) + +test('signing in as someone else reaches only the rows that answer differently for them', () => { + const store = stubStore() + // An invite this row announces. getUsernameToShow suppresses the header when the invitee is the + // signed-in user, so this is the one answer in the whole derivation that `you` reaches. + const invite = { + adder: '', + author: '[Keybase]', + conversationIDKey: convID, + id: T.Chat.numberToMessageID(901), + inviteType: 'none', + invitee: 'bob', + inviter: '', + ordinal: ord(901), + role: 'none', + team: '', + timestamp: 100, + type: 'systemInviteAccepted', + } as unknown as T.Chat.Message + // ...and a plain row that groups under a same-author previous, so it is holding reserved header + // space that has nothing to do with who is signed in. + const grouped = textAt(903, 'carol', 201) + const messages = [invite, textAt(902, 'carol', 200), grouped] + + // As alice: the invite is addressed to someone else, so it paints bob's header and records it. + expect(getRowIdentity(store, snapshotOf(messages), ord(901), 'alice').showUsername).toBe('bob') + // And the grouped row leads its group first, so it records a header of its own... + expect(getRowIdentity(store, snapshotOf([grouped]), ord(903), 'alice')).toEqual({ + poolKey: 'text:hdr', + reserveHeader: false, + showUsername: 'carol', + }) + // ...which it then keeps the space for once its same-author previous arrives. + expect(getRowIdentity(store, snapshotOf(messages), ord(903), 'alice')).toEqual({ + poolKey: 'text:hdr', + reserveHeader: true, + showUsername: '', + }) + + // Now ask the same derivation as bob. + // The invite is now addressed to the signed-in user: no header, and no held-open gap for one, + // because this is a real change in what the row is rather than a load about to fill it back in. + expect(getRowIdentity(store, snapshotOf(messages), ord(901), 'bob')).toEqual({ + poolKey: 'systemInviteAccepted', + reserveHeader: false, + showUsername: '', + }) + // The grouped row answers the same for anyone, so it keeps the space it reserved. Dropping the + // record wholesale would have collapsed this row by a header's height for no reason. + expect(getRowIdentity(store, snapshotOf(messages), ord(903), 'bob')).toEqual({ + poolKey: 'text:hdr', + reserveHeader: true, + showUsername: '', + }) +}) diff --git a/shared/chat/conversation/messages/row-identity.tsx b/shared/chat/conversation/messages/row-identity.tsx new file mode 100644 index 000000000000..7e440e7629e1 --- /dev/null +++ b/shared/chat/conversation/messages/row-identity.tsx @@ -0,0 +1,174 @@ +import * as React from 'react' +import type * as T from '@/constants/types' +import {getMessageRowType, getMessageShowUsername} from './row-metadata' +import {useCurrentUserState} from '@/stores/current-user' +import {useShallow} from '@/util/zustand' +import { + type ConversationThreadState, + type ConversationThreadStore, + useConversationThreadSelector, + useConversationThreadStore, +} from '../thread-context' + +type RowIdentity = { + // The recycling pool this row joins. A message that leads its author group renders an avatar + + // username header (~40px taller) than a grouped follow-on of the same render type. Without + // splitting the pool, recycleItems reuses one container across both heights, so a recycled view + // paints at the wrong height for a frame before re-measure - visible as rows overlapping during + // scroll. A row that reserves header space after a scroll-back load is as tall as a headered one, + // so it belongs in the same pool as one that paints it. + poolKey: string + // A header was already painted here, so the row keeps the SPACE and loses the CONTENT rather than + // shrinking ~40px mid-load and jumping the thread. + reserveHeader: boolean + showUsername: string +} + +const noOrdinals: ReadonlyArray = [] +const nullIdentity: RowIdentity = {poolKey: 'null', reserveHeader: false, showUsername: ''} + +type RowIdentityCache = { + // One answer per (snapshot, ordinal). Two callers ask about the same row - the list, deciding + // which recycling pool to put it in, and the row itself, deciding whether to draw a header - and + // they must not be able to disagree. Whichever asks first computes; the other reads what it got. + bySnapshot: WeakMap> + generation: number + // Which rows have painted an author header, and the name they painted. Sticky on purpose: it + // outlives snapshots, and that is what lets a row keep its header's height once a scroll-back load + // hands it a same-author previous. Reset with the window it describes. + shown: Map + you: string +} + +// Per conversation, because the sticky record is: it lives and dies with the window it describes, +// and one conversation's headers say nothing about another's. +const caches = new WeakMap() + +const makeCache = (generation: number, you: string): RowIdentityCache => ({ + bySnapshot: new WeakMap(), + generation, + shown: new Map(), + you, +}) + +const computeRowIdentity = ( + cache: RowIdentityCache, + snapshot: ConversationThreadState, + ordinal: T.Chat.Ordinal +): RowIdentity => { + if (!ordinal) { + return nullIdentity + } + const {messageMap, messageOrdinals, messageTypeMap} = snapshot + const message = messageMap.get(ordinal) + if (!message) { + // A row whose message the window no longer holds: it renders nothing, so it draws no header and + // reserves no space, and the type map is all that is left to pool it by. + return {poolKey: messageTypeMap.get(ordinal) ?? 'text', reserveHeader: false, showUsername: ''} + } + const base = getMessageRowType(message, messageTypeMap.get(ordinal)) + const {provisional, showUsername} = getMessageShowUsername({ + message, + messageMap, + messageOrdinals: messageOrdinals ?? noOrdinals, + ordinal, + you: cache.you, + }) + if (showUsername) { + // Only non-provisional decisions are recorded: a row that shows a header because its neighbour + // has not unboxed yet is about to lose it, and reserving space for it would leave a permanent + // blank gap where an avatar never belonged. + if (!provisional) { + cache.shown.set(ordinal, showUsername) + } + return {poolKey: `${base}:hdr`, reserveHeader: false, showUsername} + } + const reserveHeader = cache.shown.has(ordinal) + return {poolKey: reserveHeader ? `${base}:hdr` : base, reserveHeader, showUsername: ''} +} + +// What one row would show as its author header under a given signed-in user, or '' for none. Only +// used to ask which rows a change of that user can actually reach. +const showUsernameFor = (snapshot: ConversationThreadState, ordinal: T.Chat.Ordinal, you: string) => { + const message = snapshot.messageMap.get(ordinal) + if (!message) { + return '' + } + return getMessageShowUsername({ + message, + messageMap: snapshot.messageMap, + messageOrdinals: snapshot.messageOrdinals ?? noOrdinals, + ordinal, + you, + }).showUsername +} + +// The one derivation. Both entry points below come through here, so the list and the row can only +// ever be told the same thing about a row. +export const getRowIdentity = ( + store: ConversationThreadStore, + snapshot: ConversationThreadState, + ordinal: T.Chat.Ordinal, + you: string +): RowIdentity => { + let cache = caches.get(store) + // The sticky record describes one window: a clear or a conversation change replaces that window, + // and every ordinal in the record was numbered against the old one. + if (cache?.generation !== snapshot.generation) { + cache = makeCache(snapshot.generation, you) + caches.set(store, cache) + } else if (cache.you !== you) { + const was = cache.you + cache.you = you + // Every memoized answer was computed with the old name, so the memo goes. + cache.bySnapshot = new WeakMap() + // The sticky record does not. `you` reaches almost nothing - one row type suppresses its header + // when the invitee is you - and dropping the whole record would collapse every reserved header + // in the thread at once over a change that cannot reach any of them. Which rows it does reach is + // asked of the derivation rather than spelled out here, so a second `you`-dependent answer + // cannot quietly go stale in this loop. Unlike the load that this record exists to smooth over, + // a different signed-in user is a real change in what the row is, so the ones it does reach give + // up their reserved height rather than keeping a gap that nothing will ever fill again. + for (const ordinal of [...cache.shown.keys()]) { + if (showUsernameFor(snapshot, ordinal, was) !== showUsernameFor(snapshot, ordinal, you)) { + cache.shown.delete(ordinal) + } + } + } + let byOrdinal = cache.bySnapshot.get(snapshot) + if (!byOrdinal) { + byOrdinal = new Map() + cache.bySnapshot.set(snapshot, byOrdinal) + } + const memoized = byOrdinal.get(ordinal) + if (memoized) { + return memoized + } + const identity = computeRowIdentity(cache, snapshot, ordinal) + byOrdinal.set(ordinal, identity) + return identity +} + +// `you` is passed in rather than read off the store inside the derivation, and every entry point +// below subscribes to it. Reading it imperatively would have left the row, the separator and the +// recycling pool free to disagree again the moment it changed: the wrapper re-renders through its +// own subscription while the separator's thread selector does not re-run and getItemType keeps the +// same callback, which is exactly the three-way disagreement this module exists to make impossible. + +// For a row rendering itself. +export const useRowIdentity = (ordinal: T.Chat.Ordinal): RowIdentity => { + const store = useConversationThreadStore() + const you = useCurrentUserState(s => s.username) + return useConversationThreadSelector(useShallow(s => getRowIdentity(store, s, ordinal, you))) +} + +// For getItemType, which the list calls outside React and which only wants the pool. `you` is in the +// callback identity so the list re-reads its pools when it changes. +export const useRowPoolKey = () => { + const store = useConversationThreadStore() + const you = useCurrentUserState(s => s.username) + return React.useCallback( + (ordinal: T.Chat.Ordinal) => getRowIdentity(store, store.getState(), ordinal, you).poolKey, + [store, you] + ) +} diff --git a/shared/chat/conversation/messages/row-metadata.test.ts b/shared/chat/conversation/messages/row-metadata.test.ts index f6dd83536447..45a5c326c3b1 100644 --- a/shared/chat/conversation/messages/row-metadata.test.ts +++ b/shared/chat/conversation/messages/row-metadata.test.ts @@ -10,6 +10,10 @@ import { const showUsernameFor = (p: Parameters[0]) => getMessageHeader(p).showUsername +// `provisional` is what row-identity keys the sticky header record on, so which answers are firm +// and which are about to change is this module's half of that decision. +const provisionalFor = (p: Parameters[0]) => getMessageHeader(p).provisional + const convID = T.Chat.conversationIDToKey(new Uint8Array([1, 2, 3, 4])) const outboxID = T.Chat.stringToOutboxID('outbox-1') @@ -112,181 +116,6 @@ test('showUsername is derived from the previous ordinal and current message data ).toBe('bob') }) -test('a header hides but keeps its space once a real previous message groups the row', () => { - const olderOrdinal = T.Chat.numberToOrdinal(701) - const ordinal = T.Chat.numberToOrdinal(702) - const message = makeTextMessage({ - author: 'bob', - id: T.Chat.numberToMessageID(702), - ordinal, - outboxID: T.Chat.stringToOutboxID('current'), - timestamp: 101, - }) - const messageMap = new Map([[ordinal, message]]) - const shownCache = new Map() - - // oldest row of the loaded window: nothing above it yet, so it leads a group - expect( - getMessageHeader({message, messageMap, messageOrdinals: [ordinal], ordinal, shownCache, you: 'alice'}) - ).toEqual({reserveHeader: false, showUsername: 'bob'}) - expect(shownCache.get(ordinal)).toBe('bob') - - // an unboxing placeholder above is not an answer, so the header stays - messageMap.set( - olderOrdinal, - Message.makeMessagePlaceholder({ - conversationIDKey: convID, - id: T.Chat.numberToMessageID(701), - ordinal: olderOrdinal, - }) - ) - expect( - getMessageHeader({ - message, - messageMap, - messageOrdinals: [olderOrdinal, ordinal], - ordinal, - shownCache, - you: 'alice', - }) - ).toEqual({reserveHeader: false, showUsername: 'bob'}) - - // it resolves to a same-author message close in time: the row groups, so the header stops showing - // but keeps its space so the row height doesn't change under the load - messageMap.set( - olderOrdinal, - makeTextMessage({ - author: 'bob', - id: T.Chat.numberToMessageID(701), - ordinal: olderOrdinal, - outboxID: T.Chat.stringToOutboxID('older'), - timestamp: 100, - }) - ) - expect( - getMessageHeader({ - message, - messageMap, - messageOrdinals: [olderOrdinal, ordinal], - ordinal, - shownCache, - you: 'alice', - }) - ).toEqual({reserveHeader: true, showUsername: ''}) -}) - -test('a header forced by an unresolved previous is not remembered', () => { - const olderOrdinal = T.Chat.numberToOrdinal(801) - const ordinal = T.Chat.numberToOrdinal(802) - const message = makeTextMessage({ - author: 'bob', - id: T.Chat.numberToMessageID(802), - ordinal, - outboxID: T.Chat.stringToOutboxID('current'), - timestamp: 101, - }) - const messageOrdinals = [olderOrdinal, ordinal] - const messageMap = new Map([ - [ - olderOrdinal, - Message.makeMessagePlaceholder({ - conversationIDKey: convID, - id: T.Chat.numberToMessageID(801), - ordinal: olderOrdinal, - }), - ], - [ordinal, message], - ]) - const shownCache = new Map() - - expect( - getMessageHeader({message, messageMap, messageOrdinals, ordinal, shownCache, you: 'alice'}) - ).toEqual({reserveHeader: false, showUsername: 'bob'}) - expect(shownCache.has(ordinal)).toBe(false) - - // the placeholder unboxes into a same-author message: no header, and no space held for one — the - // neighbor's own height was about to change anyway, so there is nothing to keep stable - messageMap.set( - olderOrdinal, - makeTextMessage({ - author: 'bob', - id: T.Chat.numberToMessageID(801), - ordinal: olderOrdinal, - outboxID: T.Chat.stringToOutboxID('older'), - timestamp: 100, - }) - ) - expect( - getMessageHeader({message, messageMap, messageOrdinals, ordinal, shownCache, you: 'alice'}) - ).toEqual({reserveHeader: false, showUsername: ''}) -}) - -test('a header shown for an ordinal outside the loaded window is not remembered', () => { - // list churn can ask about an ordinal the window no longer holds. That looks the same as "oldest - // row, nothing above it" from the previous ordinal alone, but it is not a real gap, so nothing - // about it should be recorded and reserved later. - const staleOrdinal = T.Chat.numberToOrdinal(901) - const liveOrdinal = T.Chat.numberToOrdinal(902) - const message = makeTextMessage({ - author: 'bob', - id: T.Chat.numberToMessageID(901), - ordinal: staleOrdinal, - outboxID: T.Chat.stringToOutboxID('stale'), - timestamp: 101, - }) - const messageMap = new Map([[staleOrdinal, message]]) - const shownCache = new Map() - - expect( - getMessageHeader({ - message, - messageMap, - messageOrdinals: [liveOrdinal], - ordinal: staleOrdinal, - shownCache, - you: 'alice', - }) - ).toEqual({reserveHeader: false, showUsername: 'bob'}) - expect(shownCache.has(staleOrdinal)).toBe(false) -}) - -test('without a cache nothing is recorded and no space is ever reserved', () => { - const olderOrdinal = T.Chat.numberToOrdinal(1001) - const ordinal = T.Chat.numberToOrdinal(1002) - const message = makeTextMessage({ - author: 'bob', - id: T.Chat.numberToMessageID(1002), - ordinal, - outboxID: T.Chat.stringToOutboxID('current'), - timestamp: 101, - }) - const messageMap = new Map([[ordinal, message]]) - - expect( - getMessageHeader({message, messageMap, messageOrdinals: [ordinal], ordinal, you: 'alice'}) - ).toEqual({reserveHeader: false, showUsername: 'bob'}) - - messageMap.set( - olderOrdinal, - makeTextMessage({ - author: 'bob', - id: T.Chat.numberToMessageID(1001), - ordinal: olderOrdinal, - outboxID: T.Chat.stringToOutboxID('older'), - timestamp: 100, - }) - ) - expect( - getMessageHeader({ - message, - messageMap, - messageOrdinals: [olderOrdinal, ordinal], - ordinal, - you: 'alice', - }) - ).toEqual({reserveHeader: false, showUsername: ''}) -}) - test('row type only uses suffixes that are stable for the message lifetime', () => { // pending flips to confirmed after every send; reactions toggle. Both would leave stale // recycling-pool labels behind, so they must NOT affect the row type. @@ -432,3 +261,70 @@ test('row type combines stable suffixes and is unchanged by send confirmation', expect(getMessageRowType(confirmed)).toBe('text:reply') }) + +test('an answer resting on an unresolved neighbour is provisional', () => { + // A row whose previous ordinal is in the window but whose message is missing or still an unboxing + // placeholder reads as a different author and shows a header it will lose a moment later. That + // neighbour's own height is about to change anyway, so nothing about this row is worth + // remembering yet. + const olderOrdinal = T.Chat.numberToOrdinal(801) + const ordinal = T.Chat.numberToOrdinal(802) + const message = makeTextMessage({ + author: 'bob', + id: T.Chat.numberToMessageID(802), + ordinal, + outboxID: T.Chat.stringToOutboxID('current'), + timestamp: 101, + }) + const messageOrdinals = [olderOrdinal, ordinal] + const placeholder = Message.makeMessagePlaceholder({ + conversationIDKey: convID, + id: T.Chat.numberToMessageID(801), + ordinal: olderOrdinal, + }) + const resolved = makeTextMessage({ + author: 'bob', + id: T.Chat.numberToMessageID(801), + ordinal: olderOrdinal, + outboxID: T.Chat.stringToOutboxID('older'), + timestamp: 100, + }) + + const withPrevious = (previous: T.Chat.Message) => + getMessageHeader({ + message, + messageMap: new Map([ + [olderOrdinal, previous], + [ordinal, message], + ]), + messageOrdinals, + ordinal, + you: 'alice', + }) + + expect(withPrevious(placeholder)).toEqual({provisional: true, showUsername: 'bob'}) + expect(withPrevious(resolved)).toEqual({provisional: false, showUsername: ''}) +}) + +test('the oldest row of the window is a firm answer, a row outside it is not', () => { + // Nothing above the oldest loaded row is a real gap a scroll-back load can fill, so its header is + // a firm answer. An ordinal the window does not hold at all - list churn asking about a stale + // item id - looks identical from the previous ordinal alone, and is not. + const ordinal = T.Chat.numberToOrdinal(901) + const liveOrdinal = T.Chat.numberToOrdinal(902) + const message = makeTextMessage({ + author: 'bob', + id: T.Chat.numberToMessageID(901), + ordinal, + outboxID: T.Chat.stringToOutboxID('stale'), + timestamp: 101, + }) + const messageMap = new Map([[ordinal, message]]) + + expect(provisionalFor({message, messageMap, messageOrdinals: [ordinal], ordinal, you: 'alice'})).toBe( + false + ) + expect( + provisionalFor({message, messageMap, messageOrdinals: [liveOrdinal], ordinal, you: 'alice'}) + ).toBe(true) +}) diff --git a/shared/chat/conversation/messages/row-metadata.tsx b/shared/chat/conversation/messages/row-metadata.tsx index 75e9d4331024..7481ed14dde1 100644 --- a/shared/chat/conversation/messages/row-metadata.tsx +++ b/shared/chat/conversation/messages/row-metadata.tsx @@ -39,38 +39,28 @@ export const getPreviousOrdinal = ( // (author grouping), so the oldest row of the loaded window has no previous and must assume it // leads a group. A scroll-back load then hands it a same-author previous and the header has to go — // but dropping the header outright shrinks the row ~40px mid-load and the thread jumps. So the row -// keeps the SPACE and loses the CONTENT: `showUsername` is always the currently correct answer -// (empty once the row groups) while `reserveHeader` says a header was already painted here, so the -// row renders it invisibly and its height never changes. shownCache records which rows painted one. +// keeps the SPACE and loses the CONTENT, which is row-identity.tsx's business: this answers only +// what the row should show right now, and whether that answer is firm enough to be remembered. // -// Only non-provisional decisions are recorded. A row whose previous ordinal is in the window but +// `provisional` marks the answers that are not. A row whose previous ordinal is in the window but // whose message is missing or still an unboxing placeholder reads as a different author and shows a // header it will lose a moment later; that neighbor's own height is about to change anyway, so // reserving space for it would leave a permanent blank gap where an avatar never belonged. Ditto a -// stale ordinal that isn't in the window at all. The cache is owned per-conversation by the thread -// provider (ShownUsernameCacheContext) and passed in; omitting it (e.g. in tests) disables both the -// recording and the reservation. +// stale ordinal that isn't in the window at all. export const getMessageShowUsername = (p: { message: T.Chat.Message messageMap: ReadonlyMap messageOrdinals: ReadonlyArray ordinal: T.Chat.Ordinal you: string - shownCache?: Map -}): {reserveHeader: boolean; showUsername: string} => { - const {message, messageMap, messageOrdinals, ordinal, you, shownCache} = p +}): {provisional: boolean; showUsername: string} => { + const {message, messageMap, messageOrdinals, ordinal, you} = p const {inWindow, previous} = getPreviousOrdinalInfo(messageOrdinals, ordinal) const previousMessage = previous ? messageMap.get(previous) : undefined - const showUsername = getUsernameToShow(message, previousMessage, you) - if (!shownCache) return {reserveHeader: false, showUsername} - const provisional = !inWindow || (!!previous && (!previousMessage || previousMessage.type === 'placeholder')) - if (showUsername) { - if (!provisional) { - shownCache.set(ordinal, showUsername) - } - return {reserveHeader: false, showUsername} + return { + provisional: !inWindow || (!!previous && (!previousMessage || previousMessage.type === 'placeholder')), + showUsername: getUsernameToShow(message, previousMessage, you), } - return {reserveHeader: shownCache.has(ordinal), showUsername} } export const getMessageRowRecycleType = ( diff --git a/shared/chat/conversation/messages/separator.tsx b/shared/chat/conversation/messages/separator.tsx index 56315c22b81a..7612ea6bebdd 100644 --- a/shared/chat/conversation/messages/separator.tsx +++ b/shared/chat/conversation/messages/separator.tsx @@ -5,9 +5,10 @@ import * as React from 'react' import * as RowMetadata from './row-metadata' import * as T from '@/constants/types' import {formatTimeForConversationList} from '@/util/timestamp' +import {getRowIdentity} from './row-identity' import {OrangeLineContext} from '../orange-line-context' +import {useConversationThreadSelector, useConversationThreadStore} from '../thread-context' import {useCurrentUserState} from '@/stores/current-user' -import {ShownUsernameCacheContext, useConversationThreadSelector} from '../thread-context' const missingMessage = Chat.makeMessageDeleted({}) const noOrdinal = T.Chat.numberToOrdinal(0) @@ -16,8 +17,11 @@ const noOrdinal = T.Chat.numberToOrdinal(0) // `trailingItem` on both platforms, so the orange line sits above that ordinal's message. const useSeparatorData = (trailingItem: T.Chat.Ordinal) => { const orangeOrdinal = React.useContext(OrangeLineContext) + const store = useConversationThreadStore() + // Subscribed, not read off the store inside the selector: the answer depends on it, so this + // selector has to re-run when it changes or the separator would keep drawing a time label for a + // header the row has stopped painting. const you = useCurrentUserState(s => s.username) - const shownCache = React.useContext(ShownUsernameCacheContext) return useConversationThreadSelector( C.useShallow(s => { @@ -35,14 +39,9 @@ const useSeparatorData = (trailingItem: T.Chat.Ordinal) => { // only pay for the time label when an orange line will actually render let orangeTime = '' if (orangeLineAbove && !isMobile) { - const {showUsername} = RowMetadata.getMessageShowUsername({ - message: m, - messageMap: s.messageMap, - messageOrdinals, - ordinal, - you, - shownCache, - }) + // Through the same derivation the row and the list use, so all three agree about whether + // this row carries an author header. + const {showUsername} = getRowIdentity(store, s, ordinal, you) const tooSoon = !m.timestamp || Date.now() - m.timestamp < 1000 * 60 * 60 * 2 const isJoinLeave = m.type === 'systemJoined' if (!showUsername && !tooSoon && !isJoinLeave) { diff --git a/shared/chat/conversation/messages/wrapper/wrapper.tsx b/shared/chat/conversation/messages/wrapper/wrapper.tsx index 42cb9dd9ef89..93ec371895bb 100644 --- a/shared/chat/conversation/messages/wrapper/wrapper.tsx +++ b/shared/chat/conversation/messages/wrapper/wrapper.tsx @@ -4,7 +4,6 @@ import * as Chat from '@/constants/chat' import * as Kb from '@/common-adapters' import * as React from 'react' import * as InputState from '../../input-area/input-state' -import * as RowMetadata from '../row-metadata' import {MessageContext, RowHoveredContext, useOrdinal} from '../ids-context' import EmojiRow from '../emoji-row' import ExplodingHeightRetainer from './exploding-height-retainer' @@ -23,7 +22,6 @@ import {navToProfile} from '@/constants/router' import {formatTimeForChat} from '@/util/timestamp' import { getConversationThreadDisplayMessage, - ShownUsernameCacheContext, useConversationThreadActions, useConversationThreadID, useConversationThreadMessageActions, @@ -31,6 +29,7 @@ import { useThreadMeta, } from '../../thread-context' import {emptyParticipantInfo} from '../../data-hooks' +import {useRowIdentity} from '../row-identity' import {useInboxMetadataState} from '@/chat/inbox/metadata' import type {ConversationInputState} from '../../input-area/input-state' import {useChatTeamMemberRole} from '../../team-hooks' @@ -390,7 +389,9 @@ export const useMessageData = (ordinal: T.Chat.Ordinal, isCenteredHighlight?: bo ) const {retryMessage} = useConversationThreadActions() const messageActions = useConversationThreadMessageActions() - const shownCache = React.useContext(ShownUsernameCacheContext) + // The same derivation the list types this row's recycling pool from, so the two cannot disagree + // about whether this row paints an author header. + const {reserveHeader, showUsername} = useRowIdentity(ordinal) const conversationIDKey = useConversationThreadID() // Reload-free read: avoid useConversationParticipants' per-mount unboxRows + engine // listener registration, which is too expensive to pay per message row. @@ -416,14 +417,6 @@ export const useMessageData = (ordinal: T.Chat.Ordinal, isCenteredHighlight?: bo unfurlPrompt: s.unfurlPrompt, you, }) - const {reserveHeader, showUsername} = RowMetadata.getMessageShowUsername({ - message, - messageMap: s.messageMap, - messageOrdinals: s.messageOrdinals ?? [], - ordinal, - you, - shownCache, - }) return { ...commonData, ...getEditCancelRetryData(commonData.ecrType, message), diff --git a/shared/chat/conversation/thread-context.tsx b/shared/chat/conversation/thread-context.tsx index 259c11638072..71e430885713 100644 --- a/shared/chat/conversation/thread-context.tsx +++ b/shared/chat/conversation/thread-context.tsx @@ -106,18 +106,10 @@ export type ConversationThreadState = { unfurlPrompt: Map> } -type ConversationThreadStore = StoreApi +export type ConversationThreadStore = StoreApi const ConversationThreadStoreContext = React.createContext(undefined) ConversationThreadStoreContext.displayName = 'ConversationThreadStoreContext' -// Per-conversation sticky username-header cache (see getMessageShowUsername). Owned by the provider -// as a ref, so it lives and dies with the conversation rather than as a module global; cleared on -// messagesClear (thread reload). Maps ordinal -> the author username it has shown. -export const ShownUsernameCacheContext = React.createContext | undefined>( - undefined -) -ShownUsernameCacheContext.displayName = 'ShownUsernameCacheContext' - const makeEmptyThreadState = (): ConversationThreadState => produce( { @@ -279,14 +271,11 @@ const ConversationThreadContextProvider = (p: { actions: ConversationThreadActions children: React.ReactNode id: T.Chat.ConversationIDKey - shownUsernameCache: Map store: ConversationThreadStore }) => ( - - {p.children} - + {p.children} ) @@ -294,9 +283,6 @@ const ConversationThreadContextProvider = (p: { const ConversationThreadProviderInner = (p: ConversationThreadProviderProps) => { const {children, id} = p const [threadStore] = React.useState(() => makeThreadStore(id)) - // sticky username-header cache, owned here so it's scoped to this conversation (see - // getMessageShowUsername / ShownUsernameCacheContext); reset on messagesClear (thread reload). - const [shownUsernameCache] = React.useState(() => new Map()) const active = useShellState(s => s.active) const appFocused = useShellState(s => s.appFocused) const routeFocused = useIsFocused() @@ -833,7 +819,6 @@ const ConversationThreadProviderInner = (p: ConversationThreadProviderProps) => }) const messagesClear = React.useEffectEvent(() => { activeMarkReadEnabledRef.current = false - shownUsernameCache.clear() updateThreadState(s => { s.generation += 1 s.pendingOutboxToOrdinal.clear() @@ -985,12 +970,7 @@ const ConversationThreadProviderInner = (p: ConversationThreadProviderProps) => useThreadEngineListeners(id, threadActions) return ( - + {children} ) diff --git a/shared/chat/readme.md b/shared/chat/readme.md index df507deff25e..8e3370bbc8f7 100644 --- a/shared/chat/readme.md +++ b/shared/chat/readme.md @@ -16,7 +16,7 @@ Engine notifications land in `shared/constants/init/shared.tsx`'s `_onEngineInco ## Lifecycle -The thread store and its sibling `ShownUsernameCacheContext` are created in `ConversationThreadProviderInner` and torn down by unmounting; the screen mounts a fresh provider (via a React `key` on the conversationIDKey) when you switch conversations, so there's no manual "clear old thread" step — the old store and its listeners just go away. `ConversationThreadProvider` special-cases the case where the requested id matches the currently-provided one, reusing the existing store/actions instead of remounting (e.g. nested same-thread wrappers). On logout, `Z.resetAllStores()` (`util/zustand.tsx`) resets every store created via `Z.createZustand` — inbox metadata, badge, typing, layout, orange-line, etc. — back to its initial state; it's invoked from `stores/config.tsx` when `loggedIn` flips to false. +The thread store is created in `ConversationThreadProviderInner` and torn down by unmounting; the screen mounts a fresh provider (via a React `key` on the conversationIDKey) when you switch conversations, so there's no manual "clear old thread" step — the old store and its listeners just go away. `ConversationThreadProvider` special-cases the case where the requested id matches the currently-provided one, reusing the existing store/actions instead of remounting (e.g. nested same-thread wrappers). The sticky record of which rows have painted an author header is no longer a sibling context: it lives in a module-level cache in `messages/row-identity.tsx`, keyed on the thread store and thrown away when the window's `generation` moves, so the list and the row are answered from one derivation. On logout, `Z.resetAllStores()` (`util/zustand.tsx`) resets every store created via `Z.createZustand` — inbox metadata, badge, typing, layout, orange-line, etc. — back to its initial state; it's invoked from `stores/config.tsx` when `loggedIn` flips to false. ## Intentional dualities