From 61fe64f8a962c5cf761f6bce7f9b9c988703d3d4 Mon Sep 17 00:00:00 2001 From: chrisnojima Date: Wed, 9 Sep 2026 15:19:41 -0400 Subject: [PATCH 1/3] refactor(chat): give a row one render identity "Does this row paint an author header, and therefore which recycling pool does it join" was computed twice per row per render, in two modules, from a mutable Map passed through a context - and a third time in the separator, which wrote to that same Map on the way past. getMessageShowUsername had a pure signature and a hidden write, so whichever caller ran first decided what the others saw: getItemType read the live store, the row read its own selector's snapshot, and a row typed headerless while it renders a header lands in the headerless pool and poisons that pool's height average. The code documented the hazard rather than removing it. row-identity.tsx owns it. useRowIdentity(ordinal) for a row, useRowPoolKey() for getItemType, and one derivation behind both, memoized per store snapshot - so the two callers cannot be told different things about the same row, whoever asks first. The sticky header record is private to the module, per conversation, and reset with the window generation rather than by a hand- placed clear inside messagesClear. getMessageShowUsername keeps its pure signature and loses the write: it now answers what the row shows and whether that answer is firm enough to remember (`provisional`), which is the half that was always pure. list-area no longer reaches past the thread-context interface to the store, the shown-username cache, the current user and getMessageRowType just to answer getItemType, and ShownUsernameCacheContext is gone from thread-context. row-metadata.test.ts keeps the pure half, now asserting `provisional` directly. The sticky half moves to row-identity.test.tsx, along with the failure mode that was unreachable while the cache was shared: the row and the list must be told the same thing, whichever asks first. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_015rccpV5nLxxC5opF5xzrz7 --- shared/chat/conversation/list-area/index.tsx | 54 +--- .../messages/row-identity.test.tsx | 199 ++++++++++++++ .../conversation/messages/row-identity.tsx | 132 ++++++++++ .../messages/row-metadata.test.ts | 246 +++++------------- .../conversation/messages/row-metadata.tsx | 28 +- .../chat/conversation/messages/separator.tsx | 18 +- .../conversation/messages/wrapper/wrapper.tsx | 15 +- shared/chat/conversation/thread-context.tsx | 26 +- 8 files changed, 428 insertions(+), 290 deletions(-) create mode 100644 shared/chat/conversation/messages/row-identity.test.tsx create mode 100644 shared/chat/conversation/messages/row-identity.tsx 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..b5df998ba17c --- /dev/null +++ b/shared/chat/conversation/messages/row-identity.test.tsx @@ -0,0 +1,199 @@ +/** @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, 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))).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)) + ).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)) + ).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)) + ).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)) + ).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)) + ).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)) + ).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)) + + expect( + getRowIdentity(store, snapshotOf([textAt(701, 'bob', 100), message], {generation: 1}), ord(702)) + ).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))).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('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') +}) diff --git a/shared/chat/conversation/messages/row-identity.tsx b/shared/chat/conversation/messages/row-identity.tsx new file mode 100644 index 000000000000..3900f00c4d2a --- /dev/null +++ b/shared/chat/conversation/messages/row-identity.tsx @@ -0,0 +1,132 @@ +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' + +export 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: ''} +} + +// 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 +): RowIdentity => { + const you = useCurrentUserState.getState().username + let cache = caches.get(store) + // The sticky record describes one window; a clear or a conversation change replaces that window, + // and a different signed-in user changes every answer in it. + if (cache?.generation !== snapshot.generation || cache.you !== you) { + cache = makeCache(snapshot.generation, you) + caches.set(store, cache) + } + 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 +} + +// For a row rendering itself. +export const useRowIdentity = (ordinal: T.Chat.Ordinal): RowIdentity => { + const store = useConversationThreadStore() + return useConversationThreadSelector(useShallow(s => getRowIdentity(store, s, ordinal))) +} + +// For getItemType, which the list calls outside React and which only wants the pool. +export const useRowPoolKey = () => { + const store = useConversationThreadStore() + return React.useCallback( + (ordinal: T.Chat.Ordinal) => getRowIdentity(store, store.getState(), ordinal).poolKey, + [store] + ) +} 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..c773ebb4df89 100644 --- a/shared/chat/conversation/messages/separator.tsx +++ b/shared/chat/conversation/messages/separator.tsx @@ -5,9 +5,9 @@ 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 {useCurrentUserState} from '@/stores/current-user' -import {ShownUsernameCacheContext, useConversationThreadSelector} from '../thread-context' +import {useConversationThreadSelector, useConversationThreadStore} from '../thread-context' const missingMessage = Chat.makeMessageDeleted({}) const noOrdinal = T.Chat.numberToOrdinal(0) @@ -16,8 +16,7 @@ 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 you = useCurrentUserState(s => s.username) - const shownCache = React.useContext(ShownUsernameCacheContext) + const store = useConversationThreadStore() return useConversationThreadSelector( C.useShallow(s => { @@ -35,14 +34,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) 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} ) From 20fa849d5a00976978b83296d93cf9dd0f9fca0c Mon Sep 17 00:00:00 2001 From: chrisnojima Date: Wed, 9 Sep 2026 17:39:32 -0400 Subject: [PATCH 2/3] docs(chat): point the chat readme at the modules that exist The architecture doc still routed thread loads through `loadMoreMessages` -> `loadConversationThreadMessages` in thread-load.tsx and described `ShownUsernameCacheContext` as a sibling of the thread store. Both are gone: the loads go through requestWindow/runThreadWindowLoad in thread-window.tsx, and the sticky header record is a module-level cache in row-identity.tsx keyed on the store and thrown away with the window's generation. thread-load.tsx is still there and still worth naming, but it no longer holds the RPC - what is left is what both the window and the row derivation draw on: meta and current-user lookups, exploding-mode-from-gregor, snapshot accessors, pagination sizing. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01RYPqG8z11HmGRn5MHSu89S --- shared/chat/readme.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 From 76e9376d1c4dfacf6abb15f591f2b378bc57b1e2 Mon Sep 17 00:00:00 2001 From: chrisnojima Date: Wed, 9 Sep 2026 19:17:36 -0400 Subject: [PATCH 3/3] fix(chat): let a change of signed-in user reach the rows it answers for `you` was read off the store inside the derivation, which put it outside every subscription that depends on it. The wrapper re-renders through its own username subscription, but the separator's thread selector does not re-run and getItemType keeps the same callback - so a change while the thread stayed mounted put the painted header, the separator and the recycling pool back into exactly the three-way disagreement this module exists to make impossible. It is passed in now, and all three entry points subscribe to it. The cache was thrown away wholesale on that change, which is the other half of the same mistake: it reads as "this is handled" while collapsing every reserved header in the thread at once. `you` reaches almost nothing - one row type suppresses its header when the invitee is you - so only the rows that answer differently under the new name give up their reserved height. Which ones those are is asked of the derivation rather than spelled out against the one case that reads `you` today, so a second one cannot quietly go stale there. They do give it up rather than keeping the gap: unlike the scroll-back load this record exists to smooth over, a different signed-in user is a real change in what the row is, and holding space for a header that is never coming back is not a kindness. The ordering test did not test ordering. `act` flushes renders, so a single hook holding both callers has always already computed the row by the time the pool is asked. The real shape - and the one getItemType actually runs in - is a row the list types while nothing renders it, so the pool consumer and the row consumer mount separately, with the sticky record written by the list's question and the row arriving afterwards to agree with it. RowIdentity goes file-local; nothing outside the module names it. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01RYPqG8z11HmGRn5MHSu89S --- .../messages/row-identity.test.tsx | 145 ++++++++++++++++-- .../conversation/messages/row-identity.tsx | 62 ++++++-- .../chat/conversation/messages/separator.tsx | 7 +- 3 files changed, 192 insertions(+), 22 deletions(-) diff --git a/shared/chat/conversation/messages/row-identity.test.tsx b/shared/chat/conversation/messages/row-identity.test.tsx index b5df998ba17c..7c3a5cfd4ec9 100644 --- a/shared/chat/conversation/messages/row-identity.test.tsx +++ b/shared/chat/conversation/messages/row-identity.test.tsx @@ -3,7 +3,7 @@ import * as Message from '@/constants/chat/message' import * as T from '@/constants/types' import HiddenString from '@/util/hidden-string' -import {act, cleanup, renderHook} from '@testing-library/react' +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' @@ -72,7 +72,7 @@ test('a header hides but keeps its space once a real previous message groups the 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))).toEqual({ + expect(getRowIdentity(store, snapshotOf([message]), ord(702), 'alice')).toEqual({ poolKey: 'text:hdr', reserveHeader: false, showUsername: 'bob', @@ -80,13 +80,13 @@ test('a header hides but keeps its space once a real previous message groups the // An unboxing placeholder above is not an answer, so the header stays. expect( - getRowIdentity(store, snapshotOf([placeholderAt(701), message]), ord(702)) + 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)) + getRowIdentity(store, snapshotOf([textAt(701, 'bob', 100), message]), ord(702), 'alice') ).toEqual({poolKey: 'text:hdr', reserveHeader: true, showUsername: ''}) }) @@ -95,13 +95,13 @@ test('a header forced by an unresolved previous is not remembered', () => { const message = textAt(802, 'bob', 101) expect( - getRowIdentity(store, snapshotOf([placeholderAt(801), message]), ord(802)) + 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)) + getRowIdentity(store, snapshotOf([textAt(801, 'bob', 100), message]), ord(802), 'alice') ).toEqual({poolKey: 'text', reserveHeader: false, showUsername: ''}) }) @@ -113,12 +113,12 @@ test('a header shown for an ordinal outside the loaded window is not remembered' const live = textAt(902, 'bob', 100) expect( - getRowIdentity(store, snapshotOf([stale, live], {messageOrdinals: [ord(902)]}), ord(901)) + 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)) + getRowIdentity(store, snapshotOf([textAt(900, 'bob', 100), stale]), ord(901), 'alice') ).toEqual({poolKey: 'text', reserveHeader: false, showUsername: ''}) }) @@ -126,10 +126,10 @@ 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)) + getRowIdentity(store, snapshotOf([message]), ord(702), 'alice') expect( - getRowIdentity(store, snapshotOf([textAt(701, 'bob', 100), message], {generation: 1}), ord(702)) + getRowIdentity(store, snapshotOf([textAt(701, 'bob', 100), message], {generation: 1}), ord(702), 'alice') ).toEqual({poolKey: 'text', reserveHeader: false, showUsername: ''}) }) @@ -138,7 +138,7 @@ test('a row whose message the window no longer holds pools by its type map', () const snapshot = snapshotOf([], {messageOrdinals: [ord(500)]}) snapshot.messageTypeMap.set(ord(500), 'attachment') - expect(getRowIdentity(store, snapshot, ord(500))).toEqual({ + expect(getRowIdentity(store, snapshot, ord(500), 'alice')).toEqual({ poolKey: 'attachment', reserveHeader: false, showUsername: '', @@ -149,6 +149,74 @@ test('a row whose message the window no longer holds pools by its type map', () // 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} @@ -197,3 +265,58 @@ test('the row and the list are told the same thing, whichever asks first', () => }) 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 index 3900f00c4d2a..7e440e7629e1 100644 --- a/shared/chat/conversation/messages/row-identity.tsx +++ b/shared/chat/conversation/messages/row-identity.tsx @@ -10,7 +10,7 @@ import { useConversationThreadStore, } from '../thread-context' -export type RowIdentity = { +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 @@ -87,20 +87,53 @@ const computeRowIdentity = ( 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 + ordinal: T.Chat.Ordinal, + you: string ): RowIdentity => { - const you = useCurrentUserState.getState().username let cache = caches.get(store) - // The sticky record describes one window; a clear or a conversation change replaces that window, - // and a different signed-in user changes every answer in it. - if (cache?.generation !== snapshot.generation || cache.you !== you) { + // 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) { @@ -116,17 +149,26 @@ export const getRowIdentity = ( 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() - return useConversationThreadSelector(useShallow(s => getRowIdentity(store, s, ordinal))) + 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. +// 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).poolKey, - [store] + (ordinal: T.Chat.Ordinal) => getRowIdentity(store, store.getState(), ordinal, you).poolKey, + [store, you] ) } diff --git a/shared/chat/conversation/messages/separator.tsx b/shared/chat/conversation/messages/separator.tsx index c773ebb4df89..7612ea6bebdd 100644 --- a/shared/chat/conversation/messages/separator.tsx +++ b/shared/chat/conversation/messages/separator.tsx @@ -8,6 +8,7 @@ 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' const missingMessage = Chat.makeMessageDeleted({}) const noOrdinal = T.Chat.numberToOrdinal(0) @@ -17,6 +18,10 @@ const noOrdinal = T.Chat.numberToOrdinal(0) 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) return useConversationThreadSelector( C.useShallow(s => { @@ -36,7 +41,7 @@ const useSeparatorData = (trailingItem: T.Chat.Ordinal) => { if (orangeLineAbove && !isMobile) { // 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) + 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) {