diff --git a/.claude/hooks/pre-commit-check.sh b/.claude/hooks/pre-commit-check.sh
index c7e0895f9857..6423b763332f 100755
--- a/.claude/hooks/pre-commit-check.sh
+++ b/.claude/hooks/pre-commit-check.sh
@@ -12,6 +12,13 @@ if ! (cd "$REPO_ROOT/shared" && yarn lint 2>&1); then
exit 2
fi
+# Bailouts are invisible to eslint: no react-compiler rule is wired into
+# eslint.config.mjs, so this is the only check that catches them.
+if ! (cd "$REPO_ROOT/shared" && yarn lint:bailouts 2>&1); then
+ echo "React-compiler bailout check failed — commit blocked." >&2
+ exit 2
+fi
+
if ! (cd "$REPO_ROOT/shared" && yarn tsc 2>&1); then
echo "TypeScript check failed — commit blocked." >&2
exit 2
diff --git a/CLAUDE.md b/CLAUDE.md
index fdddff874632..64f8e241ab61 100644
--- a/CLAUDE.md
+++ b/CLAUDE.md
@@ -26,3 +26,5 @@ Repo root is `client/`. TS source lives in `shared/`. Always use absolute paths
## Validation
After TS changes (from `shared/`): `yarn lint:all` (= `yarn lint` && `yarn lint:bailouts` && `yarn tsc`). Plain `yarn lint` is eslint only and does NOT catch react-compiler bailouts — no compiler rule is wired into `eslint.config.mjs`, so bailouts only surface via `lint:bailouts`. `lint:bailouts` also flags components the compiler cannot name (an `isMobile ? arrow : arrow` ternary is never compiled at all, so nothing in it is memoized — name both branches instead), and memo scopes keyed on the whole props object (a `props.x` read inside a callback, or a destructure below one, makes the compiler key on `props` itself, so the cache never hits — read every prop through one destructure at the top, above every callback). Repo baseline is 0 bailouts and 0 whole-props deps; keep it there. When debugging visually, skip until fix is confirmed. Never delete the ESLint cache.
+
+Before reporting any TS change complete: run `yarn lint:all`, then run `/code-review high` against your own diff and fix what it finds. Report done only after both are clean — do not hand unvalidated work to the user for review. If a finding is wrong, say why instead of applying it.
diff --git a/shared/chat/conversation/attachment-actions.tsx b/shared/chat/conversation/attachment-actions.tsx
index a859e6c4f514..3125261ceb5f 100644
--- a/shared/chat/conversation/attachment-actions.tsx
+++ b/shared/chat/conversation/attachment-actions.tsx
@@ -455,47 +455,8 @@ export const useConversationAttachmentActions = () => {
ignorePromise(f())
}
- const loadNextAttachment = async (from: T.Chat.Ordinal, backInTime: boolean) => {
- const fromMsg = threadStore.getState().messageMap.get(from)
- if (!fromMsg) {
- return Promise.reject(new Error('Incorrect from'))
- }
- const {deviceName, username} = useCurrentUserState.getState()
- const getLastOrdinal = () => threadStore.getState().messageOrdinals?.at(-1) ?? T.Chat.numberToOrdinal(0)
- const result = await T.RPCChat.localGetNextAttachmentMessageLocalRpcPromise({
- assetTypes: [T.RPCChat.AssetMetadataType.image, T.RPCChat.AssetMetadataType.video],
- backInTime,
- convID: T.Chat.keyToConversationID(conversationIDKey),
- identifyBehavior: T.RPCGen.TLFIdentifyBehavior.chatGui,
- messageID: fromMsg.id,
- })
-
- if (result.message) {
- const goodMessage = Message.uiMessageToMessage(
- conversationIDKey,
- result.message,
- username,
- getLastOrdinal,
- deviceName
- )
- if (goodMessage?.type === 'attachment') {
- actions.addMessages([goodMessage])
- let ordinal = goodMessage.ordinal
- if (goodMessage.outboxID && !threadStore.getState().messageMap.get(ordinal)) {
- const pendingOrdinal = threadStore.getState().pendingOutboxToOrdinal.get(goodMessage.outboxID)
- if (pendingOrdinal) {
- ordinal = pendingOrdinal
- }
- }
- return ordinal
- }
- }
- return Promise.reject(new Error('No more results'))
- }
-
return {
attachmentDownload,
- loadNextAttachment,
messageAttachmentNativeSave,
messageAttachmentNativeShare,
showAttachmentPreview: (ordinal: T.Chat.Ordinal, message?: T.Chat.MessageAttachment) => {
diff --git a/shared/chat/conversation/normal/container.test.tsx b/shared/chat/conversation/normal/container.test.tsx
index 1d8c11f6ee20..7a2622a60607 100644
--- a/shared/chat/conversation/normal/container.test.tsx
+++ b/shared/chat/conversation/normal/container.test.tsx
@@ -290,6 +290,45 @@ test('initial load uses the read message ID from mount even if meta changes befo
expectOrangeLine(T.Chat.numberToOrdinal(15))
})
+test('the read position is latched when localization lands, not when the load finishes', async () => {
+ // The DB-nuke order: the conversation is unlocalized and the thread is still loading, so the
+ // loaded gate skips the fetch when localization lands. The load then finishing flips `loaded`
+ // and issues mark-read in the same breath, so by the next render the meta can already carry the
+ // advanced read position. Asking the service about that one puts the unreadline at the newest
+ // message and the thread shows no divider at all; the position from the moment localization
+ // landed is the only one that means anything here.
+ const unreadlineRpc = getUnreadlineRpc().mockResolvedValue({
+ offline: false,
+ unreadlineID: T.Chat.numberToMessageID(6),
+ })
+ mockLoaded = false
+ mockMeta = makeMeta(convID, -1)
+
+ render()
+ await flushOrangeLine()
+ expect(unreadlineRpc).not.toHaveBeenCalled()
+
+ // Localization lands while the thread load is still in flight.
+ mockMeta = makeMeta(convID, 5, 9)
+ act(() => {
+ useShellState.setState({mobileAppState: 'background'})
+ })
+ await flushOrangeLine()
+ expect(unreadlineRpc).not.toHaveBeenCalled()
+
+ // The load finishes, and the mark-read it issues has already moved the read position.
+ mockMeta = makeMeta(convID, 9, 9)
+ act(() => {
+ mockLoaded = true
+ useShellState.setState({mobileAppState: 'active'})
+ })
+ await flushOrangeLine()
+
+ expect(unreadlineRpc).toHaveBeenCalledTimes(1)
+ expectUnreadlineRpcReadMsgID(unreadlineRpc, 5)
+ expectOrangeLine(T.Chat.numberToOrdinal(6))
+})
+
test('a thread reload does not refetch the orange line against the stale mount read position', async () => {
const unreadlineRpc = getUnreadlineRpc().mockResolvedValue({
offline: false,
@@ -322,7 +361,12 @@ test('a thread reload does not refetch the orange line against the stale mount r
expectOrangeLine(noOrangeLine)
})
-test('negative read message IDs are clamped before fetching the orange line', async () => {
+test('an unknown read position draws no orange line rather than one above everything', async () => {
+ // A negative read position means the conversation's meta has not landed yet
+ // (emptyConversationMeta reads -1), which a DB nuke makes the norm. The old code clamped it to 0,
+ // and the service answers 0 with "everything is unread", pinning the line above the oldest
+ // message - and since the state is set once, that answer used to stick for the life of the mount.
+ // 0 itself is a real read position and is still asked about; see the zero-value test below.
const unreadlineRpc = getUnreadlineRpc().mockResolvedValue({
offline: false,
unreadlineID: T.Chat.numberToMessageID(8),
@@ -332,9 +376,58 @@ test('negative read message IDs are clamped before fetching the orange line', as
render()
await flushOrangeLine()
- expect(unreadlineRpc).toHaveBeenCalledTimes(1)
- expectUnreadlineRpcReadMsgID(unreadlineRpc, 0)
- expectOrangeLine(T.Chat.numberToOrdinal(8))
+ expect(unreadlineRpc).not.toHaveBeenCalled()
+ expectOrangeLine(noOrangeLine)
+})
+
+test('an unknown read position is not asked about', async () => {
+ // -1 is emptyConversationMeta's "not localized yet", the norm right after a DB nuke. The old code
+ // clamped it to 0, so the service answered "everything is unread" and pinned the line above the
+ // oldest message - and since the state is set once, that answer stuck.
+ const unreadlineRpc = getUnreadlineRpc().mockResolvedValue({
+ offline: false,
+ unreadlineID: T.Chat.numberToMessageID(8),
+ })
+ mockMeta = makeMeta(convID, -1)
+
+ render()
+ await flushOrangeLine()
+
+ expect(unreadlineRpc).not.toHaveBeenCalled()
+ expectOrangeLine(noOrangeLine)
+})
+
+test('an inactive conversation with an unknown read position is not asked about either', async () => {
+ // The inactive refresh passes the live readMsgID rather than the mount-time one, so it reaches
+ // loadOrangeLine with -1 directly and needs its own guard.
+ const unreadlineRpc = getUnreadlineRpc().mockResolvedValue({
+ offline: false,
+ unreadlineID: T.Chat.numberToMessageID(8),
+ })
+ mockMeta = makeMeta(convID, -1)
+ useShellState.setState({active: false})
+
+ render()
+ await flushOrangeLine()
+
+ expect(unreadlineRpc).not.toHaveBeenCalled()
+})
+
+test('a zero read position is a real answer and is still asked about', async () => {
+ // ReaderInfo reports 0 for a conversation you have genuinely never read - every first open of a
+ // new channel or DM. "Everything is unread" is the correct answer there, so suppressing the
+ // request would silently drop the orange line for exactly those conversations. Only a negative
+ // read position means "not known yet".
+ const unreadlineRpc = getUnreadlineRpc().mockResolvedValue({
+ offline: false,
+ unreadlineID: T.Chat.numberToMessageID(8),
+ })
+ mockMeta = makeMeta(convID, 0)
+
+ render()
+ await flushOrangeLine()
+
+ expect(unreadlineRpc).toHaveBeenCalledWith(expect.objectContaining({readMsgID: 0}))
})
test('zero unreadline responses render as no orange line', async () => {
diff --git a/shared/chat/conversation/normal/container.tsx b/shared/chat/conversation/normal/container.tsx
index 2c9b4a8f28f9..c1d97028da76 100644
--- a/shared/chat/conversation/normal/container.tsx
+++ b/shared/chat/conversation/normal/container.tsx
@@ -58,18 +58,43 @@ const useOrangeLine = (
const {maxVisibleMsgID, readMsgID} = useThreadMeta(
C.useShallow(m => ({maxVisibleMsgID: m.maxVisibleMsgID, readMsgID: m.readMsgID}))
)
- // Keep the read position from when this conversation mounted. Mark-as-read updates
- // readMsgID shortly after navigation, but the open thread should retain its orange line.
- const [initialReadMsgID] = React.useState(() => readMsgID)
+ // Keep the read position from when this conversation mounted. Mark-as-read updates readMsgID
+ // shortly after navigation, but the open thread should retain its orange line.
+ //
+ // An unlocalized conversation reads -1 ("not known yet"), which a DB nuke makes the norm, so
+ // freezing on mount would pin that and the thread would never get an orange line for the life of
+ // the mount. Latch the first real value instead, on the commit it arrives in, rather than reading
+ // the live one where it is used. The two differ exactly when it matters: after a nuke the thread
+ // is still loading when localization lands, so the load below is skipped, and the load then
+ // finishing flips `loaded` and issues mark-read in the same breath. A live read on the next
+ // commit can already see the advanced position, and the thread then shows no unread divider at
+ // all - the failure this latch exists to prevent.
+ const latchedReadMsgIDRef = React.useRef(readMsgID)
+ React.useEffect(() => {
+ if (latchedReadMsgIDRef.current < 0 && readMsgID >= 0) {
+ latchedReadMsgIDRef.current = readMsgID
+ }
+ }, [readMsgID])
const loadOrangeLine = React.useEffectEvent(
(conversationIDKey: T.Chat.ConversationIDKey, readMsgID: T.Chat.MessageID) => {
+ // Negative means we do not know the read position yet: an unlocalized conversation reads -1
+ // from emptyConversationMeta, which a DB nuke makes the norm, and the old code turned that
+ // into 0 - so the service answered "everything is unread" and put the line above the oldest
+ // message. Since the state is set once and only refreshed while the conversation is
+ // inactive, that answer stuck.
+ //
+ // Zero is different and must still be asked: ReaderInfo reports 0 for a conversation you
+ // have genuinely never read, where "everything is unread" is the right answer.
+ if (readMsgID < 0) {
+ return
+ }
const f = async () => {
const convID = T.Chat.keyToConversationID(conversationIDKey)
const unreadlineRes = await T.RPCChat.localGetUnreadlineRpcPromise({
convID,
identifyBehavior: T.RPCGen.TLFIdentifyBehavior.chatGui,
- readMsgID: readMsgID < 0 ? 0 : readMsgID,
+ readMsgID,
})
const nextOrangeLine = T.Chat.numberToOrdinal(
unreadlineRes.unreadlineID ? unreadlineRes.unreadlineID : 0
@@ -100,11 +125,16 @@ const useOrangeLine = (
// messages we sent ourselves.
const initialOrangeLineLoadedRef = React.useRef(false)
React.useEffect(() => {
- if (loaded && !initialOrangeLineLoadedRef.current) {
+ // Only claim the latch once there is a read position to ask about, so an unlocalized
+ // conversation gets its orange line when localization lands rather than never. readMsgID is a
+ // dep so that landing wakes this effect; the value asked about is the latched one, and the
+ // effect that sets it is declared above so it has already run for this commit.
+ const readMsgIDAtLocalization = latchedReadMsgIDRef.current
+ if (loaded && !initialOrangeLineLoadedRef.current && readMsgIDAtLocalization >= 0) {
initialOrangeLineLoadedRef.current = true
- loadOrangeLine(id, initialReadMsgID)
+ loadOrangeLine(id, readMsgIDAtLocalization)
}
- }, [id, loaded, initialReadMsgID])
+ }, [id, loaded, readMsgID])
// just use the rpc for orange line if we're not active
// if we are active we want to keep whatever state we had so it is maintained
diff --git a/shared/chat/conversation/thread-context.test.tsx b/shared/chat/conversation/thread-context.test.tsx
index b2dc1803d231..abcf188d74e0 100644
--- a/shared/chat/conversation/thread-context.test.tsx
+++ b/shared/chat/conversation/thread-context.test.tsx
@@ -2,11 +2,12 @@
///
import * as Common from '@/constants/chat/common'
import * as Message from '@/constants/chat/message'
+import * as Meta from '@/constants/chat/meta'
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 {participantInfoReceived} from '@/chat/inbox/metadata'
+import {metasReceived, participantInfoReceived} from '@/chat/inbox/metadata'
import {notifyEngineActionListeners} from '@/engine/action-listener'
import {useConfigState} from '@/stores/config'
import {useCurrentUserState} from '@/stores/current-user'
@@ -38,6 +39,16 @@ const flushPromises = async () => {
}
}
+const textAt = (n: number) =>
+ Message.makeMessageText({
+ author: 'alice',
+ conversationIDKey: convID,
+ id: T.Chat.numberToMessageID(n),
+ ordinal: T.Chat.numberToOrdinal(n),
+ text: new HiddenString(`message ${n}`),
+ timestamp: 100,
+ })
+
const makeTextMessage = () =>
Message.makeMessageText({
author: 'alice',
@@ -62,7 +73,11 @@ const makeAttachmentMessage = (override?: Partial) =>
...override,
})
-const makeValidTextUIMessage = (serverMsgID: T.Chat.MessageID, text: string): T.RPCChat.UIMessage => ({
+const makeValidTextUIMessage = (
+ serverMsgID: T.Chat.MessageID,
+ text: string,
+ outboxID = ''
+): T.RPCChat.UIMessage => ({
state: T.RPCChat.MessageUnboxedState.valid,
valid: {
atMentions: null,
@@ -92,7 +107,7 @@ const makeValidTextUIMessage = (serverMsgID: T.Chat.MessageID, text: string): T.
},
},
messageID: T.Chat.messageIDToNumber(serverMsgID),
- outboxID: '',
+ outboxID,
paymentInfos: null,
pinnedMessageID: null,
reactions: {},
@@ -237,6 +252,20 @@ beforeEach(() => {
uid: 'uid',
username: 'alice',
})
+ // A conversation the reader has open is localized. readMsgID -1 - the makeConversationMeta
+ // default - means "not known yet", which suppresses mark-read so the true read position survives
+ // long enough for the orange line to be resolved against it.
+ metasReceived(
+ [
+ {
+ ...Meta.makeConversationMeta(),
+ conversationIDKey: convID,
+ readMsgID: T.Chat.numberToMessageID(0),
+ },
+ ],
+ undefined,
+ {force: true}
+ )
})
afterEach(() => {
@@ -712,6 +741,66 @@ test('mounted thread listener applies incoming messages while inactive without m
expect(markAsRead).not.toHaveBeenCalled()
})
+test('an unlocalized conversation defers mark read until localization lands', async () => {
+ // The post-nuke case. Mark-read reads the newest message out of the window and needs no meta, so
+ // without this it wins the race against localization and destroys the read position before
+ // useOrangeLine can ask for the unreadline against it - the thread then shows no unread divider.
+ useConfigState.setState({loggedIn: true})
+ useShellState.getState().dispatch.setActive(true)
+ jest.spyOn(Common, 'isUserActivelyLookingAtThisThread').mockImplementation(() => true)
+ metasReceived(
+ [{...Meta.makeConversationMeta(), conversationIDKey: convID}],
+ undefined,
+ {force: true}
+ )
+ const markAsRead = jest
+ .spyOn(T.RPCChat, 'localMarkAsReadLocalRpcPromise')
+ .mockResolvedValue({offline: false})
+ const msgID = T.Chat.numberToMessageID(605)
+ jest.spyOn(T.RPCChat, 'localGetThreadNonblockRpcListener').mockImplementation(async p => {
+ p.incomingCallMap['chat.1.chatUi.chatThreadFull']?.({
+ thread: JSON.stringify({
+ messages: [makeValidTextUIMessage(msgID, 'loaded unlocalized')],
+ pagination: {last: true, next: '', num: 100, previous: ''},
+ }),
+ })
+ await Promise.resolve()
+ return {offline: false}
+ })
+ const {result} = renderHook(() => useConversationThreadLoadMoreMessages(), {wrapper})
+
+ act(() => {
+ result.current({reason: 'tab selected'})
+ })
+ await act(async () => {
+ await flushPromises()
+ })
+ expect(markAsRead).not.toHaveBeenCalled()
+
+ // Localization lands carrying the read position that was there all along.
+ act(() => {
+ metasReceived(
+ [
+ {
+ ...Meta.makeConversationMeta(),
+ conversationIDKey: convID,
+ readMsgID: T.Chat.numberToMessageID(600),
+ },
+ ],
+ undefined,
+ {force: true}
+ )
+ })
+ await act(async () => {
+ await flushPromises()
+ })
+ expect(markAsRead).toHaveBeenCalledWith({
+ conversationID: T.Chat.keyToConversationID(convID),
+ forceUnread: false,
+ msgID,
+ })
+})
+
test('active change marks read after an eligible mounted thread load', async () => {
useConfigState.setState({loggedIn: true})
useShellState.getState().dispatch.setActive(false)
@@ -1376,3 +1465,633 @@ test('mounted thread listener applies attachment download and upload progress',
: undefined
).toBeUndefined()
})
+
+test('a warm-cache load prunes against both passes, not either one alone', async () => {
+ // Regression: once the service has sent a cached thread it switches the full response to
+ // INCREMENTAL, so the full pass only carries what changed. Treating either pass on its own as
+ // authoritative deleted real messages that were still in the thread. The two together are the
+ // window - INCREMENTAL walks it and omits only what the cached pass already carried - so the
+ // range spans both, and everything inside it that either pass carried survives.
+ useConfigState.setState({loggedIn: true})
+ jest.spyOn(Common, 'isUserActivelyLookingAtThisThread').mockReturnValue(true)
+ jest.spyOn(T.RPCChat, 'localMarkAsReadLocalRpcPromise').mockResolvedValue({offline: false})
+ const ids = [301, 302, 303, 304].map(T.Chat.numberToMessageID)
+ const threadJSON = (msgIDs: ReadonlyArray) =>
+ JSON.stringify({
+ messages: msgIDs.map(id => makeValidTextUIMessage(id, `m${id}`)),
+ pagination: {last: true, next: '', num: 100, previous: ''},
+ })
+
+ // The cache holds the older three; only 304 changed, so that is all the full pass carries. The
+ // span is what makes this the dangerous shape: a range of [301..304] computed from the full pass
+ // alone covers 302 and 303, which are absent from it and would be pruned.
+ jest.spyOn(T.RPCChat, 'localGetThreadNonblockRpcListener').mockImplementation(async p => {
+ p.incomingCallMap['chat.1.chatUi.chatThreadCached']?.({thread: threadJSON(ids.slice(0, 3))})
+ await Promise.resolve()
+ p.incomingCallMap['chat.1.chatUi.chatThreadFull']?.({thread: threadJSON([ids[3]!])})
+ await Promise.resolve()
+ return {offline: false}
+ })
+ const {result} = renderHook(
+ () => ({
+ actions: useConversationThreadActions(),
+ loadMoreMessages: useConversationThreadLoadMoreMessages(),
+ ordinals: useConversationThreadSelector(s => s.messageOrdinals),
+ }),
+ {wrapper}
+ )
+
+ // Seed a settled four-message window the way a whole-window full pass would.
+ act(() => {
+ result.current.actions.applyThreadLoad({
+ centered: false,
+ enableActiveMarkRead: false,
+ messages: ids.map(id =>
+ Message.makeMessageText({
+ author: 'alice',
+ conversationIDKey: convID,
+ id,
+ ordinal: T.Chat.numberToOrdinal(T.Chat.messageIDToNumber(id)),
+ outboxID: undefined,
+ text: new HiddenString(`m${id}`),
+ timestamp: 100,
+ })
+ ),
+ moreToLoad: false,
+ scrollDirection: 'none',
+ })
+ })
+ expect(result.current.ordinals).toEqual([301, 302, 303, 304])
+
+ act(() => {
+ result.current.loadMoreMessages({reason: 'test'})
+ })
+ await act(async () => {
+ await flushPromises()
+ })
+
+ expect(result.current.ordinals).toEqual([301, 302, 303, 304])
+})
+
+test('a warm-cache load does not prune a message sitting on its outbox ordinal', async () => {
+ // A message you sent keeps the fractional ordinal it had in the outbox, so the ordinal it parses
+ // with - its server one - is not the ordinal it occupies. The prune walks the window, so what
+ // the passes delivered has to be recorded in the window's terms too; recording the parsed
+ // ordinal deletes the row it was meant to protect.
+ useConfigState.setState({loggedIn: true})
+ jest.spyOn(Common, 'isUserActivelyLookingAtThisThread').mockReturnValue(true)
+ jest.spyOn(T.RPCChat, 'localMarkAsReadLocalRpcPromise').mockResolvedValue({offline: false})
+ const outboxID = T.Chat.stringToOutboxID('sent-1')
+ const sentOrdinal = T.Chat.numberToOrdinal(302.001)
+
+ jest.spyOn(T.RPCChat, 'localGetThreadNonblockRpcListener').mockImplementation(async p => {
+ p.incomingCallMap['chat.1.chatUi.chatThreadCached']?.({
+ thread: JSON.stringify({
+ messages: [
+ makeValidTextUIMessage(T.Chat.numberToMessageID(301), 'm301'),
+ makeValidTextUIMessage(T.Chat.numberToMessageID(302), 'm302'),
+ makeValidTextUIMessage(T.Chat.numberToMessageID(303), 'mine', 'sent-1'),
+ ],
+ pagination: {last: true, next: '', num: 100, previous: ''},
+ }),
+ })
+ await Promise.resolve()
+ p.incomingCallMap['chat.1.chatUi.chatThreadFull']?.({
+ thread: JSON.stringify({
+ messages: [makeValidTextUIMessage(T.Chat.numberToMessageID(301), 'm301 edited')],
+ pagination: {last: true, next: '', num: 100, previous: ''},
+ }),
+ })
+ await Promise.resolve()
+ return {offline: false}
+ })
+ const {result} = renderHook(
+ () => ({
+ actions: useConversationThreadActions(),
+ loadMoreMessages: useConversationThreadLoadMoreMessages(),
+ ordinals: useConversationThreadSelector(s => s.messageOrdinals),
+ }),
+ {wrapper}
+ )
+
+ // The window as it stands after the send settled: the message is at its outbox ordinal, indexed
+ // under the server ID the service will send it back as.
+ act(() => {
+ result.current.actions.applyThreadLoad({
+ centered: false,
+ enableActiveMarkRead: false,
+ messages: [
+ Message.makeMessageText({
+ author: 'alice',
+ conversationIDKey: convID,
+ id: T.Chat.numberToMessageID(301),
+ ordinal: T.Chat.numberToOrdinal(301),
+ outboxID: undefined,
+ text: new HiddenString('m301'),
+ timestamp: 100,
+ }),
+ Message.makeMessageText({
+ author: 'alice',
+ conversationIDKey: convID,
+ id: T.Chat.numberToMessageID(302),
+ ordinal: T.Chat.numberToOrdinal(302),
+ outboxID: undefined,
+ text: new HiddenString('m302'),
+ timestamp: 100,
+ }),
+ Message.makeMessageText({
+ author: 'testuser',
+ conversationIDKey: convID,
+ id: T.Chat.numberToMessageID(303),
+ ordinal: sentOrdinal,
+ outboxID,
+ text: new HiddenString('mine'),
+ timestamp: 100,
+ }),
+ ],
+ moreToLoad: false,
+ scrollDirection: 'none',
+ })
+ })
+ expect(result.current.ordinals).toEqual([301, 302, sentOrdinal])
+
+ act(() => {
+ result.current.loadMoreMessages({reason: 'test'})
+ })
+ await act(async () => {
+ await flushPromises()
+ })
+
+ expect(result.current.ordinals).toEqual([301, 302, sentOrdinal])
+})
+
+test('a full pass that changed nothing still reconciles the window', async () => {
+ // The ordinary warm reload: the cached pass is the window and the INCREMENTAL full pass behind it
+ // carries nothing at all, because nothing changed. That is still an authoritative answer about
+ // the span, so a row the service no longer has is still a ghost - skipping the prune for want of
+ // messages to add leaves it on screen until the conversation is reopened.
+ useConfigState.setState({loggedIn: true})
+ jest.spyOn(Common, 'isUserActivelyLookingAtThisThread').mockReturnValue(true)
+ jest.spyOn(T.RPCChat, 'localMarkAsReadLocalRpcPromise').mockResolvedValue({offline: false})
+ const ids = [301, 302, 303].map(T.Chat.numberToMessageID)
+
+ jest.spyOn(T.RPCChat, 'localGetThreadNonblockRpcListener').mockImplementation(async p => {
+ p.incomingCallMap['chat.1.chatUi.chatThreadCached']?.({
+ thread: JSON.stringify({
+ messages: [ids[0]!, ids[2]!].map(id => makeValidTextUIMessage(id, `m${id}`)),
+ pagination: {last: true, next: '', num: 100, previous: ''},
+ }),
+ })
+ await Promise.resolve()
+ p.incomingCallMap['chat.1.chatUi.chatThreadFull']?.({
+ thread: JSON.stringify({messages: null, pagination: {last: true, next: '', num: 100, previous: ''}}),
+ })
+ await Promise.resolve()
+ return {offline: false}
+ })
+ const {result} = renderHook(
+ () => ({
+ actions: useConversationThreadActions(),
+ loadMoreMessages: useConversationThreadLoadMoreMessages(),
+ ordinals: useConversationThreadSelector(s => s.messageOrdinals),
+ }),
+ {wrapper}
+ )
+
+ act(() => {
+ result.current.actions.applyThreadLoad({
+ centered: false,
+ enableActiveMarkRead: false,
+ messages: ids.map(id =>
+ Message.makeMessageText({
+ author: 'alice',
+ conversationIDKey: convID,
+ id,
+ ordinal: T.Chat.numberToOrdinal(T.Chat.messageIDToNumber(id)),
+ outboxID: undefined,
+ text: new HiddenString(`m${id}`),
+ timestamp: 100,
+ })
+ ),
+ moreToLoad: false,
+ scrollDirection: 'none',
+ })
+ })
+ expect(result.current.ordinals).toEqual([301, 302, 303])
+
+ act(() => {
+ result.current.loadMoreMessages({reason: 'test'})
+ })
+ await act(async () => {
+ await flushPromises()
+ })
+
+ expect(result.current.ordinals).toEqual([301, 303])
+})
+
+test('a warm-cache load still prunes a row neither pass carries', async () => {
+ // The other half of the same rule: a row inside the range that neither pass returned is a ghost -
+ // a cache repair left it behind, or it was deleted while we were away - and reconciling it away
+ // is what the range is for. Gating on a full pass with no cached one before it would have given
+ // this up for every conversation the cache is warm for.
+ useConfigState.setState({loggedIn: true})
+ jest.spyOn(Common, 'isUserActivelyLookingAtThisThread').mockReturnValue(true)
+ jest.spyOn(T.RPCChat, 'localMarkAsReadLocalRpcPromise').mockResolvedValue({offline: false})
+ const ids = [301, 302, 303, 304].map(T.Chat.numberToMessageID)
+ const threadJSON = (msgIDs: ReadonlyArray) =>
+ JSON.stringify({
+ messages: msgIDs.map(id => makeValidTextUIMessage(id, `m${id}`)),
+ pagination: {last: true, next: '', num: 100, previous: ''},
+ })
+
+ // 303 is in neither pass, and it sits inside the span the two of them cover.
+ jest.spyOn(T.RPCChat, 'localGetThreadNonblockRpcListener').mockImplementation(async p => {
+ p.incomingCallMap['chat.1.chatUi.chatThreadCached']?.({thread: threadJSON([ids[0]!, ids[1]!])})
+ await Promise.resolve()
+ p.incomingCallMap['chat.1.chatUi.chatThreadFull']?.({thread: threadJSON([ids[3]!])})
+ await Promise.resolve()
+ return {offline: false}
+ })
+ const {result} = renderHook(
+ () => ({
+ actions: useConversationThreadActions(),
+ loadMoreMessages: useConversationThreadLoadMoreMessages(),
+ ordinals: useConversationThreadSelector(s => s.messageOrdinals),
+ }),
+ {wrapper}
+ )
+
+ act(() => {
+ result.current.actions.applyThreadLoad({
+ centered: false,
+ enableActiveMarkRead: false,
+ messages: ids.map(id =>
+ Message.makeMessageText({
+ author: 'alice',
+ conversationIDKey: convID,
+ id,
+ ordinal: T.Chat.numberToOrdinal(T.Chat.messageIDToNumber(id)),
+ outboxID: undefined,
+ text: new HiddenString(`m${id}`),
+ timestamp: 100,
+ })
+ ),
+ moreToLoad: false,
+ scrollDirection: 'none',
+ })
+ })
+ expect(result.current.ordinals).toEqual([301, 302, 303, 304])
+
+ act(() => {
+ result.current.loadMoreMessages({reason: 'test'})
+ })
+ await act(async () => {
+ await flushPromises()
+ })
+
+ expect(result.current.ordinals).toEqual([301, 302, 304])
+})
+
+// The window invariant, at the callsite that enforces it. The four unit tests in
+// thread-message-state.test.tsx pass `dropNewBelowWindow` themselves; only this proves addMessages
+// actually sets it, and that thread loads are still allowed to extend the window downward.
+test('a notification may not strand a new ordinal below the loaded window', () => {
+ const {result} = renderHook(() => ({actions: useConversationThreadActions()}), {wrapper})
+
+ act(() => {
+ result.current.actions.applyThreadLoad({
+ centered: false,
+ enableActiveMarkRead: false,
+ messages: [textAt(7152), textAt(7153)],
+ moreToLoad: true,
+ scrollDirection: 'none',
+ })
+ })
+
+ // A MessagesUpdated push re-unboxing the ancient METADATA message. It must not become the new
+ // floor: messageOrdinals is the list's data array, so an item at index 0 far below the window
+ // makes back-paging stop being a prepend and kills scrollback.
+ act(() => {
+ result.current.actions.addMessages([textAt(1)], {liveUpdate: true})
+ })
+ expect(result.current.actions.getSnapshot().messageOrdinals).toEqual([
+ T.Chat.numberToOrdinal(7152),
+ T.Chat.numberToOrdinal(7153),
+ ])
+
+ // A notification updating something inside the window still applies.
+ act(() => {
+ result.current.actions.addMessages([textAt(7152)], {liveUpdate: true})
+ })
+ expect(result.current.actions.getSnapshot().messageOrdinals).toEqual([
+ T.Chat.numberToOrdinal(7152),
+ T.Chat.numberToOrdinal(7153),
+ ])
+
+ // And a thread load - the only thing allowed to - still extends the window downward.
+ act(() => {
+ result.current.actions.applyThreadLoad({
+ centered: false,
+ enableActiveMarkRead: false,
+ messages: [textAt(7052)],
+ moreToLoad: true,
+ scrollDirection: 'back',
+ })
+ })
+ expect(result.current.actions.getSnapshot().messageOrdinals?.[0]).toEqual(
+ T.Chat.numberToOrdinal(7052)
+ )
+})
+
+test('jumpToRecent drops the old window instead of merging a disjoint one into it', async () => {
+ // The newest window has nothing to do with wherever the reader had scrolled back to, so merging
+ // the two leaves a hole in messageOrdinals between them - which is the same stranded-index-0
+ // shape that kills scrollback. A centered jump already clears first; this must too.
+ useConfigState.setState({loggedIn: true})
+ jest.spyOn(Common, 'isUserActivelyLookingAtThisThread').mockReturnValue(true)
+ jest.spyOn(T.RPCChat, 'localMarkAsReadLocalRpcPromise').mockResolvedValue({offline: false})
+ const recent = T.Chat.numberToMessageID(9001)
+ jest.spyOn(T.RPCChat, 'localGetThreadNonblockRpcListener').mockImplementation(async p => {
+ p.incomingCallMap['chat.1.chatUi.chatThreadFull']?.({
+ thread: JSON.stringify({
+ messages: [makeValidTextUIMessage(recent, 'newest')],
+ pagination: {last: true, next: '', num: 100, previous: ''},
+ }),
+ })
+ await Promise.resolve()
+ return {offline: false}
+ })
+ const {result} = renderHook(
+ () => ({
+ actions: useConversationThreadActions(),
+ jumpToRecent: useConversationThreadJumpToRecent(),
+ ordinals: useConversationThreadSelector(s => s.messageOrdinals),
+ }),
+ {wrapper}
+ )
+
+ // The reader is deep in old history.
+ act(() => {
+ result.current.actions.applyThreadLoad({
+ centered: false,
+ enableActiveMarkRead: false,
+ messages: [textAt(101), textAt(102)],
+ moreToLoad: true,
+ scrollDirection: 'none',
+ })
+ })
+
+ act(() => {
+ result.current.jumpToRecent()
+ })
+ await act(async () => {
+ await flushPromises()
+ })
+
+ // Only the newest window survives. If the old one were merged in, ordinals would read
+ // [101, 102, 9001] with a 8899-wide hole.
+ expect(result.current.ordinals).toEqual([T.Chat.numberToOrdinal(9001)])
+})
+
+test('only the load that claimed the window gate may drop it', () => {
+ // clearVersion cannot separate two loads of the same conversation - the load generation only
+ // moves on a conversation change or unmount, so both call themselves current. The reader taps a
+ // search result, messagesClear issues the centered reload, and a ChatThreadsStale notification
+ // then fires a second load at the same generation. If that one settles first - no thread, an
+ // error - it would take the gate down while the reload is still in flight, and a push landing in
+ // what is left of the gap strands exactly as it did before the gate existed.
+ const {result} = renderHook(() => ({actions: useConversationThreadActions()}), {wrapper})
+
+ act(() => {
+ result.current.actions.applyThreadLoad({
+ centered: false,
+ enableActiveMarkRead: false,
+ messages: [textAt(7152), textAt(7153)],
+ moreToLoad: true,
+ scrollDirection: 'none',
+ })
+ })
+ act(() => {
+ result.current.actions.messagesClear()
+ })
+
+ // The reload the clear issued claims the gate; the stale-thread load that follows loses the race.
+ act(() => {
+ result.current.actions.claimWindowGate(1)
+ result.current.actions.claimWindowGate(2)
+ })
+ act(() => {
+ result.current.actions.clearWindowGate(2)
+ })
+ expect(result.current.actions.getSnapshot().windowCleared).toBe(true)
+
+ act(() => {
+ result.current.actions.clearWindowGate(1)
+ })
+ expect(result.current.actions.getSnapshot().windowCleared).toBe(false)
+})
+
+test('a stale reload does not merge the newest page into a centered window', () => {
+ // The reader taps a search result and sits on the window around it, with more to load forward.
+ // A ChatThreadsStale reload fetches the newest page, which is nowhere near that window: merging
+ // the two leaves ordinals with a hole through the middle and then calls the result the latest
+ // message, which is the gap this invariant is about.
+ const {result} = renderHook(
+ () => ({
+ actions: useConversationThreadActions(),
+ ordinals: useConversationThreadSelector(s => s.messageOrdinals),
+ }),
+ {wrapper}
+ )
+
+ const textAt = (ord: number) =>
+ Message.makeMessageText({
+ author: 'alice',
+ conversationIDKey: convID,
+ id: T.Chat.numberToMessageID(ord),
+ ordinal: T.Chat.numberToOrdinal(ord),
+ outboxID: undefined,
+ text: new HiddenString(`m${ord}`),
+ timestamp: 100,
+ })
+
+ act(() => {
+ result.current.actions.applyThreadLoad({
+ centered: true,
+ enableActiveMarkRead: false,
+ messages: [textAt(7000), textAt(7001)],
+ moreToLoad: true,
+ scrollDirection: 'none',
+ })
+ })
+ expect(result.current.actions.getSnapshot().moreToLoadForward).toBe(true)
+
+ act(() => {
+ result.current.actions.applyThreadLoad({
+ centered: false,
+ enableActiveMarkRead: false,
+ messages: [textAt(9900), textAt(9901)],
+ moreToLoad: true,
+ scrollDirection: 'none',
+ })
+ })
+
+ expect(result.current.ordinals).toEqual([7000, 7001])
+ // ...and the window still knows it has not reached the latest message.
+ expect(result.current.actions.getSnapshot().moreToLoadForward).toBe(true)
+})
+
+test('a newest page that reaches the window is still merged', () => {
+ // The other side of the rule. A reader near the bottom gets a page that overlaps what they hold,
+ // so there is no hole to open and the refresh must land.
+ const {result} = renderHook(
+ () => ({
+ actions: useConversationThreadActions(),
+ ordinals: useConversationThreadSelector(s => s.messageOrdinals),
+ }),
+ {wrapper}
+ )
+
+ const textAt = (ord: number) =>
+ Message.makeMessageText({
+ author: 'alice',
+ conversationIDKey: convID,
+ id: T.Chat.numberToMessageID(ord),
+ ordinal: T.Chat.numberToOrdinal(ord),
+ outboxID: undefined,
+ text: new HiddenString(`m${ord}`),
+ timestamp: 100,
+ })
+
+ act(() => {
+ result.current.actions.applyThreadLoad({
+ centered: true,
+ enableActiveMarkRead: false,
+ messages: [textAt(9900), textAt(9901)],
+ moreToLoad: true,
+ scrollDirection: 'none',
+ })
+ })
+
+ act(() => {
+ result.current.actions.applyThreadLoad({
+ centered: false,
+ enableActiveMarkRead: false,
+ messages: [textAt(9901), textAt(9902)],
+ moreToLoad: true,
+ scrollDirection: 'none',
+ })
+ })
+
+ expect(result.current.ordinals).toEqual([9900, 9901, 9902])
+})
+
+test('an empty pass leaves the thread unloaded rather than loaded and empty', () => {
+ // addMessagesToThreadState always leaves a messageOrdinals array behind, and the top-of-thread
+ // block reads `messageOrdinals !== undefined` as "this conversation has loaded at least once".
+ // A cold open answers with an empty cached pass first, so handing that pass to the store renders
+ // the top of the conversation against an empty thread, which then swaps when the page lands.
+ const {result} = renderHook(() => ({actions: useConversationThreadActions()}), {wrapper})
+
+ act(() => {
+ result.current.actions.applyThreadLoad({
+ centered: false,
+ enableActiveMarkRead: false,
+ messages: [],
+ moreToLoad: true,
+ reconcile: {carried: new Set(), prune: false},
+ scrollDirection: 'none',
+ })
+ })
+ expect(result.current.actions.getSnapshot().messageOrdinals).toBeUndefined()
+})
+
+test('an empty pass during a jump-to-recent gap leaves the gate up', () => {
+ // A cold cache sends a cached pass carrying no messages ahead of the full response, and it
+ // reaches applyThreadLoad like any other. Dropping the gate on it reopens the gap: a
+ // notification landing before the real page becomes the sole ordinal, and the page that follows
+ // is disjoint from it.
+ const {result} = renderHook(() => ({actions: useConversationThreadActions()}), {wrapper})
+
+ act(() => {
+ result.current.actions.applyThreadLoad({
+ centered: false,
+ enableActiveMarkRead: false,
+ messages: [textAt(7152), textAt(7153)],
+ moreToLoad: true,
+ scrollDirection: 'none',
+ })
+ })
+ act(() => {
+ result.current.actions.messagesClear()
+ })
+ act(() => {
+ result.current.actions.applyThreadLoad({
+ centered: false,
+ enableActiveMarkRead: false,
+ messages: [],
+ moreToLoad: true,
+ scrollDirection: 'none',
+ })
+ })
+ act(() => {
+ result.current.actions.addMessages([textAt(7155)], {liveUpdate: true})
+ })
+ act(() => {
+ result.current.actions.applyThreadLoad({
+ centered: false,
+ enableActiveMarkRead: false,
+ messages: [textAt(9001)],
+ moreToLoad: true,
+ scrollDirection: 'none',
+ })
+ })
+
+ expect(result.current.actions.getSnapshot().messageOrdinals).toEqual([T.Chat.numberToOrdinal(9001)])
+})
+
+test('a notification during a jump-to-recent gap cannot become the new window', () => {
+ // jumpToRecent and a centered jump both clear before reloading, so for one RPC round trip the
+ // window is empty. The notification most likely to land in that gap is the post-send
+ // ResolveSkippedUnboxeds push from the load already in flight - the very one carrying the ancient
+ // setChannelname at ID 1. Without the gap being marked it installs itself at index 0 and the
+ // thread is stranded again, which is the bug this branch exists to fix.
+ //
+ // A push between the old window and the page that is coming strands the same way: jump-to-recent
+ // reloads the newest page, which for a reader parked far back starts thousands of ordinals above
+ // where they were, so "newer than what we dropped" says nothing about whether it belongs.
+ const {result} = renderHook(
+ () => ({actions: useConversationThreadActions()}),
+ {wrapper}
+ )
+
+ act(() => {
+ result.current.actions.applyThreadLoad({
+ centered: false,
+ enableActiveMarkRead: false,
+ messages: [textAt(7152), textAt(7153)],
+ moreToLoad: true,
+ scrollDirection: 'none',
+ })
+ })
+
+ act(() => {
+ result.current.actions.messagesClear()
+ })
+ act(() => {
+ result.current.actions.addMessages([textAt(1), textAt(7155)], {liveUpdate: true})
+ })
+ act(() => {
+ result.current.actions.applyThreadLoad({
+ centered: false,
+ enableActiveMarkRead: false,
+ messages: [textAt(9001)],
+ moreToLoad: true,
+ scrollDirection: 'none',
+ })
+ })
+
+ expect(result.current.actions.getSnapshot().messageOrdinals).toEqual([
+ T.Chat.numberToOrdinal(9001),
+ ])
+})
diff --git a/shared/chat/conversation/thread-context.tsx b/shared/chat/conversation/thread-context.tsx
index 237100b19655..0a7abbe52b08 100644
--- a/shared/chat/conversation/thread-context.tsx
+++ b/shared/chat/conversation/thread-context.tsx
@@ -20,6 +20,7 @@ import {useStore} from 'zustand'
import {createStore, type StoreApi} from 'zustand/vanilla'
import {useIsFocused} from '@react-navigation/core'
import {
+ type ThreadLoadReconcile,
addMessagesToThreadState,
applyOptimisticReactionsToMessage,
completeAttachmentDownloadInThreadState,
@@ -107,6 +108,16 @@ export type ConversationThreadState = {
messageIDToOrdinal: Map
messageMap: Map
messageOrdinals?: ReadonlyArray
+ // Set between a messagesClear and the reload that refills the window, so a notification arriving
+ // in that gap cannot install itself as the new window. Cleared once that load settles, however it
+ // settles - see clearWindowGate.
+ windowCleared?: boolean
+ // The load that owns the gate above: the first one to claim it after the clear, which is the
+ // reload the clear issued. Only that load may drop the gate. clearVersion alone cannot tell two
+ // loads of the same conversation apart, and a second load at the same generation - a
+ // ChatThreadsStale reload, say - would otherwise settle first and take down a gate the reload is
+ // still relying on.
+ windowGateOwner?: number
messageTypeMap: Map
moreToLoadBack: boolean
moreToLoadForward: boolean
@@ -115,7 +126,6 @@ export type ConversationThreadState = {
pendingOutboxToOrdinal: Map
typing: Set
unfurlPrompt: Map>
- validatedOrdinalRange?: {from: T.Chat.Ordinal; to: T.Chat.Ordinal}
}
type ConversationThreadStore = StoreApi
@@ -150,7 +160,6 @@ const makeEmptyThreadState = (): ConversationThreadState =>
pendingOutboxToOrdinal: new Map(),
typing: new Set(),
unfurlPrompt: new Map>(),
- validatedOrdinalRange: undefined as {from: T.Chat.Ordinal; to: T.Chat.Ordinal} | undefined,
},
() => {}
)
@@ -187,6 +196,12 @@ export type LoadMoreMessagesParams = ThreadLoadStatusOptions & {
messageIDControl?: T.RPCChat.MessageIDControl | null
numberOfMessagesToLoad?: number
reason: string
+ // Internal: set only by the empty-back-page reload in thread-load.tsx, carrying the oldest
+ // message ID the previous attempt saw. Each reload must reach strictly further back than that,
+ // which is what stops it looping. Callers leave it unset.
+ retryBelowMessageID?: T.Chat.MessageID
+ // How many times the back-page reload has already chained. See maxBackPageReloads.
+ retryCount?: number
scrollDirection?: ScrollDirection
}
type LoadMoreMessages = ((p: LoadMoreMessagesParams) => void) & {cancel: () => void}
@@ -212,7 +227,6 @@ export type ConversationThreadActions = {
opt?: {
liveUpdate?: boolean
markAsRead?: boolean
- validatedRange?: {from: T.Chat.Ordinal; to: T.Chat.Ordinal}
}
) => void
applyThreadLoad: (p: {
@@ -222,10 +236,9 @@ export type ConversationThreadActions = {
forceContainsLatestCalc?: boolean
messages: ReadonlyArray
moreToLoad: boolean
+ reconcile?: ThreadLoadReconcile
scrollDirection: ScrollDirection
- validatedRange?: {from: T.Chat.Ordinal; to: T.Chat.Ordinal}
}) => void
- clearValidatedOrdinalRange: () => void
clearUnfurlPrompt: (messageID: T.Chat.MessageID, domain: string) => void
deleteMessages: (p: {
messageIDs?: ReadonlyArray
@@ -239,6 +252,8 @@ export type ConversationThreadActions = {
explodedBy?: string,
liveUpdate?: boolean
) => void
+ claimWindowGate: (loadID: number) => void
+ clearWindowGate: (loadID: number) => void
getSnapshot: () => ConversationThreadState
loadMoreMessages: LoadMoreMessages
markThreadAsRead: () => void
@@ -428,6 +443,16 @@ const ConversationThreadProviderInner = (p: ConversationThreadProviderProps) =>
logger.info('mark read bail on unloaded thread')
return
}
+ // Marking read overwrites the read position, so it must not run before we know what that
+ // position was. An unlocalized conversation reads -1, which a db nuke makes the norm, and
+ // localization races the thread load - mark-read needs neither, it reads the newest message
+ // out of the window. If it wins that race the true read position is gone before useOrangeLine
+ // ever sees it, localization then lands already advanced, and the thread shows no unread
+ // divider at all. Wait for localization; the effect below runs this again once it lands.
+ if ((getInboxConversationMeta(id)?.readMsgID ?? T.Chat.numberToMessageID(-1)) < 0) {
+ logger.info('mark read bail on unlocalized conversation')
+ return
+ }
if (snapshot.moreToLoadForward) {
logger.info('mark read bail on not containing latest message')
return
@@ -458,20 +483,40 @@ const ConversationThreadProviderInner = (p: ConversationThreadProviderProps) =>
markThreadAsRead()
}
}, [lookingAtThread])
+ // The other half of the unlocalized bail above: whatever mark-read attempt was refused for want of
+ // a read position, run it again now that there is one. Only on the transition, so an ordinary
+ // mark-read moving readMsgID does not bounce back through here.
+ //
+ // Safe against the orange line: useOrangeLine latches the read position into state on the commit
+ // localization lands in, so the position it later asks the unreadline about does not depend on
+ // beating this mark-read to it.
+ const metaReadMsgID = useInboxMetadataState(
+ s => (s.metas.get(id) ?? emptyConversationMeta).readMsgID
+ )
+ const wasUnlocalizedRef = React.useRef(metaReadMsgID < 0)
+ React.useEffect(() => {
+ const wasUnlocalized = wasUnlocalizedRef.current
+ wasUnlocalizedRef.current = metaReadMsgID < 0
+ if (wasUnlocalized && metaReadMsgID >= 0) {
+ markThreadAsRead()
+ }
+ }, [metaReadMsgID])
const addMessages = React.useEffectEvent(
(
messages: ReadonlyArray,
opt: {
liveUpdate?: boolean
markAsRead?: boolean
- validatedRange?: {from: T.Chat.Ordinal; to: T.Chat.Ordinal}
} = {}
) => {
updateThreadState(s => {
if (opt.liveUpdate) {
s.liveUpdateVersion += 1
}
- addMessagesToThreadState(s, messages, {validatedRange: opt.validatedRange})
+ addMessagesToThreadState(s, messages, {
+ // Only thread loads may extend the window downward; a notification must not.
+ dropNewBelowWindow: true,
+ })
clearOptimisticReactionsForMessagesInThreadState(s, messages)
})
if (opt.markAsRead) {
@@ -493,15 +538,66 @@ const ConversationThreadProviderInner = (p: ConversationThreadProviderProps) =>
forceContainsLatestCalc?: boolean
messages: ReadonlyArray
moreToLoad: boolean
+ reconcile?: ThreadLoadReconcile
scrollDirection: ScrollDirection
- validatedRange?: {from: T.Chat.Ordinal; to: T.Chat.Ordinal}
}) => {
+ const rendered = p.messages.filter(m => m.conversationMessage !== false && m.type !== 'deleted')
+ // Judged on what this pass carried rather than on the state of the window, so the gate turns
+ // on the one thing that decides it: whether this pass put a row on screen.
+ const carriedRenderedMessage = rendered.length > 0
+ // A 'none' load fetches the newest page, and a window with more to load forward does not
+ // reach it. Merging the two leaves ordinals with a hole through the middle, and the branch
+ // below then reports that window as containing the latest message - which is the gap this
+ // whole invariant is about, arriving through a ChatThreadsStale reload while the reader sits
+ // on a search result. Both conditions are needed: a window that already reaches the newest
+ // message merges fine, and so does a page that overlaps what we hold, however far back the
+ // reader is. Neither holds here, so the page is left alone rather than applied - the reader
+ // keeps their window, and jumping to recent (which empties it first) is what replaces it.
+ const beforeApply = threadStore.getState()
+ const windowOrdinals = beforeApply.messageOrdinals
+ const floor = windowOrdinals?.[0]
+ const ceiling = windowOrdinals?.[windowOrdinals.length - 1]
+ if (
+ p.scrollDirection === 'none' &&
+ rendered.length &&
+ beforeApply.moreToLoadForward &&
+ floor !== undefined &&
+ ceiling !== undefined
+ ) {
+ let lowest = Number.MAX_SAFE_INTEGER
+ let highest = Number.MIN_SAFE_INTEGER
+ for (const m of rendered) {
+ lowest = Math.min(lowest, m.ordinal)
+ highest = Math.max(highest, m.ordinal)
+ }
+ if (lowest > ceiling || highest < floor) {
+ logger.info(
+ `applyThreadLoad: page ${lowest}-${highest} does not reach window ${floor}-${ceiling}, ignoring`
+ )
+ return
+ }
+ }
updateThreadState(s => {
s.loaded = true
- if (p.messages.length) {
- addMessagesToThreadState(s, p.messages, {validatedRange: p.validatedRange})
+ // The reconciling pass runs even with nothing to add: the warm reload where nothing changed
+ // answers with an empty full pass, and the span its earlier pass covered is authoritative
+ // all the same - the stale rows inside it are exactly what the prune is for. A pass with
+ // neither is skipped rather than passed through: addMessagesToThreadState always leaves a
+ // messageOrdinals array behind, and an empty one reads as a loaded, empty thread - the top
+ // of the conversation renders against it and then swaps when the real page arrives.
+ if (p.messages.length || p.reconcile?.prune) {
+ addMessagesToThreadState(s, p.messages, {reconcile: p.reconcile})
clearOptimisticReactionsForMessagesInThreadState(s, p.messages)
}
+ // Only a pass that actually rendered something drops the gate. A cold cache sends an empty
+ // cached pass ahead of the full response, and a page can be all tombstones: dropping the
+ // gate on either would let a notification arriving before the real page install itself as
+ // the whole window and strand once that page lands. A load that ends without ever producing
+ // an ordinal releases the gate in its own finally instead - see clearWindowGate.
+ if (carriedRenderedMessage) {
+ s.windowCleared = false
+ s.windowGateOwner = undefined
+ }
switch (p.scrollDirection) {
case 'forward':
s.moreToLoadForward = p.moreToLoad
@@ -866,9 +962,34 @@ const ConversationThreadProviderInner = (p: ConversationThreadProviderProps) =>
markThreadAsRead()
}
)
- const clearValidatedOrdinalRange = React.useEffectEvent(() => {
- updateThreadState(s => {
- s.validatedOrdinalRange = undefined
+ // The reload a clear issues claims the gate, so a load that merely happens to be running at the
+ // same clear generation cannot drop it out from under that reload. First claim wins: the clear
+ // issues its reload synchronously, so that reload is the first to get here.
+ const claimWindowGate = React.useEffectEvent((loadID: number) => {
+ const s = threadStore.getState()
+ if (!s.windowCleared || s.windowGateOwner !== undefined) {
+ return
+ }
+ updateThreadState(d => {
+ d.windowGateOwner = loadID
+ })
+ })
+ // applyThreadLoad drops the gate when a load refills the window, but a load can end without ever
+ // applying: offline, scchatnotinteam, or a response that carries no thread. Left alone the gate
+ // would keep dropping notifications for the life of the provider, with no window to correct it.
+ const clearWindowGate = React.useEffectEvent((loadID: number) => {
+ const s = threadStore.getState()
+ if (!s.windowCleared) {
+ return
+ }
+ // An unclaimed gate is released by whoever settles first: nothing claimed it, so there is no
+ // reload in flight to protect, and leaving it up would strand the thread.
+ if (s.windowGateOwner !== undefined && s.windowGateOwner !== loadID) {
+ return
+ }
+ updateThreadState(d => {
+ d.windowCleared = false
+ d.windowGateOwner = undefined
})
})
const messagesClear = React.useEffectEvent(() => {
@@ -878,12 +999,18 @@ const ConversationThreadProviderInner = (p: ConversationThreadProviderProps) =>
s.clearVersion += 1
s.pendingOutboxToOrdinal.clear()
s.loaded = false
+ // Mark the gap. A notification landing between here and the reload would otherwise face an
+ // empty window, install itself as the whole of it, and strand once the load response arrives.
+ // Both callers reload a region disjoint from the one being dropped - a centered jump an
+ // arbitrary one, jumpToRecent the newest page - so nothing arriving first can be placed
+ // against what is coming.
+ s.windowCleared = true
+ s.windowGateOwner = undefined
s.messageIDToOrdinal.clear()
s.messageMap.clear()
s.messageOrdinals = undefined
s.messageTypeMap.clear()
s.optimisticReactionMap.clear()
- s.validatedOrdinalRange = undefined
})
})
const setTyping = React.useEffectEvent((typing: ReadonlySet) => {
@@ -1001,7 +1128,8 @@ const ConversationThreadProviderInner = (p: ConversationThreadProviderProps) =>
addOptimisticReaction,
applyThreadLoad,
clearUnfurlPrompt,
- clearValidatedOrdinalRange,
+ claimWindowGate,
+ clearWindowGate,
completeAttachmentDownload,
deleteMessages,
explodeMessages,
@@ -1184,12 +1312,15 @@ export const useConversationThreadLoadMessagesCentered = () => {
}
export const useConversationThreadJumpToRecent = () => {
- const {clearValidatedOrdinalRange, setMarkReadBlocked} = useConversationThreadActions()
+ const {setMarkReadBlocked} = useConversationThreadActions()
const loadMoreMessages = useConversationThreadLoadMoreMessages()
+ const messagesClear = useConversationThreadMessagesClear()
const jumpToRecent: JumpToRecent = options => {
setMarkReadBlocked(false)
- clearValidatedOrdinalRange()
+ // The newest window is disjoint from wherever the reader was, so merging the two would leave a
+ // gap in the ordinals. Drop the old window first, the way a centered jump does.
+ messagesClear()
loadMoreMessages({...(options ?? {}), reason: 'jump to recent'})
}
return jumpToRecent
diff --git a/shared/chat/conversation/thread-load.test.tsx b/shared/chat/conversation/thread-load.test.tsx
index 0d5ad57c0e3b..993a38961ce2 100644
--- a/shared/chat/conversation/thread-load.test.tsx
+++ b/shared/chat/conversation/thread-load.test.tsx
@@ -6,9 +6,20 @@ import {
getExplodingModeFromGregorItems,
getLastOrdinalFromSnapshot,
getOrdinalForMessageIDInSnapshot,
+ loadConversationThreadMessages,
+ maxBackPageReloads,
+ numMessagesOnScrollback,
scrollDirectionToPagination,
} from './thread-load'
-import type {ConversationThreadState} from './thread-context'
+import * as ThreadRpc from './thread-rpc'
+import {resetAllStores} from '@/util/zustand'
+import {useCurrentUserState} from '@/stores/current-user'
+import type {ThreadLoadReconcile} from './thread-message-state'
+import type {
+ ConversationThreadActions,
+ ConversationThreadState,
+ LoadMoreMessagesParams,
+} from './thread-context'
const conversationIDKey = T.Chat.stringToConversationIDKey('conv1')
const otherConversationIDKey = T.Chat.stringToConversationIDKey('conv2')
@@ -155,3 +166,626 @@ describe('snapshot helpers', () => {
expect(getOrdinalForMessageIDInSnapshot(snapshot, messageID(7))).toBeNull()
})
})
+
+describe('a back page that adds no ordinals reloads itself', () => {
+ const flushPromises = async () => {
+ for (let i = 0; i < 200; i++) {
+ await Promise.resolve()
+ }
+ }
+
+ // A stand-in for the real store that keeps the one behaviour under test: applying a load adds an
+ // ordinal per message EXCEPT the ones addMessagesToThreadState drops, which is `deleted`. A fake
+ // that grows unconditionally would make every page look productive and hide the bug; one that
+ // never grows would make every page look empty and hide the opposite bug.
+ const trackingActions = () => {
+ const ordinals = new Set([
+ T.Chat.numberToOrdinal(7152),
+ T.Chat.numberToOrdinal(7153),
+ ])
+ const actions = {
+ applyThreadLoad: jest.fn((p: {messages: ReadonlyArray}) => {
+ for (const m of p.messages) {
+ if (m.type !== 'deleted') {
+ ordinals.add(m.ordinal)
+ }
+ }
+ }),
+ claimWindowGate: jest.fn(),
+ clearWindowGate: jest.fn(),
+ getSnapshot: () =>
+ ({
+ liveUpdateVersion: 0,
+ loaded: true,
+ messageIDToOrdinal: new Map(),
+ messageMap: new Map(),
+ messageOrdinals: [...ordinals].sort((a, b) => a - b),
+ pendingOutboxToOrdinal: new Map(),
+ }) as unknown as ConversationThreadState,
+ // The reload goes through the action, which in the store is the throttled loadMoreMessages.
+ // Standing in the unthrottled call here keeps these tests about the reload chain rather than
+ // about lodash timers; the throttle itself is store wiring.
+ loadMoreMessages: jest.fn((p: LoadMoreMessagesParams) => {
+ loadConversationThreadMessages(conversationIDKey, p, actions)
+ }),
+ markThreadAsRead: jest.fn(),
+ } as unknown as ConversationThreadActions
+ return actions
+ }
+
+ // Hidden placeholders are what a DELETE-superseded message arrives as, and what becomes `deleted`
+ // on this side. They carry real message IDs, which is what bounds the reload.
+ const tombstones = (from: number, to: number) =>
+ Array.from({length: from - to + 1}, (_, i) => ({
+ placeholder: {hidden: true, messageID: T.Chat.numberToMessageID(from - i)},
+ state: T.RPCChat.MessageUnboxedState.placeholder,
+ }))
+
+ // hidden: false parses to a `placeholder`, which the thread does render and keep an ordinal for.
+ const visible = (from: number, to: number) =>
+ Array.from({length: from - to + 1}, (_, i) => ({
+ placeholder: {hidden: false, messageID: T.Chat.numberToMessageID(from - i)},
+ state: T.RPCChat.MessageUnboxedState.placeholder,
+ }))
+
+ // Each call walks one page further back, exactly as the service does, until it runs out.
+ const mockWalkingBack = (oldestOverall: number) => {
+ let next = 7151
+ return jest.spyOn(ThreadRpc, 'loadThreadNonblock').mockImplementation(async p => {
+ const from = next
+ const to = Math.max(oldestOverall, from - numMessagesOnScrollback + 1)
+ next = to - 1
+ await Promise.resolve()
+ p.onFullThread?.(
+ JSON.stringify({messages: tombstones(from, to), pagination: {last: to <= oldestOverall, num: 100}})
+ )
+ return undefined as never
+ })
+ }
+
+ const loadBack = (actions: ConversationThreadActions) =>
+ loadConversationThreadMessages(
+ conversationIDKey,
+ {numberOfMessagesToLoad: numMessagesOnScrollback, reason: 'scroll back', scrollDirection: 'back'},
+ actions
+ )
+
+ beforeEach(() => {
+ useCurrentUserState.getState().dispatch.setBootstrap({
+ deviceID: 'device-id',
+ deviceName: 'testuser-mac',
+ uid: 'uid',
+ username: 'testuser',
+ })
+ })
+
+ afterEach(() => {
+ jest.restoreAllMocks()
+ resetAllStores()
+ })
+
+ test('keeps paging through a run of tombstones until the pager says it is done', async () => {
+ // 7151 down to 6952 is two pages of 100, so one reload after the first call.
+ const rpc = mockWalkingBack(6952)
+ loadBack(trackingActions())
+ await flushPromises()
+ expect(rpc).toHaveBeenCalledTimes(2)
+ })
+
+ test('walks a run of tombstones that ends before the cap', async () => {
+ const oldest = 6752
+ const rpc = mockWalkingBack(oldest)
+ loadBack(trackingActions())
+ await flushPromises()
+ expect(rpc).toHaveBeenCalledTimes(Math.ceil((7151 - oldest + 1) / numMessagesOnScrollback))
+ })
+
+ test('stops at the reload cap rather than walking an expunged history', async () => {
+ // A channel whose history was largely expunged has far more tombstones than the chain should
+ // walk off one gesture. It stops at the cap and hands the thread back; scrolling away and back
+ // fires onStartReached again and starts a fresh chain from where this one stopped.
+ const rpc = mockWalkingBack(1)
+ loadBack(trackingActions())
+ await flushPromises()
+ expect(rpc).toHaveBeenCalledTimes(maxBackPageReloads + 1)
+ })
+
+ test('stops if a page fails to reach further back', async () => {
+ // A service that keeps handing back the same window must not spin us forever. Progress in
+ // message ID is the only thing permitting another attempt.
+ const rpc = jest.spyOn(ThreadRpc, 'loadThreadNonblock').mockImplementation(async p => {
+ await Promise.resolve()
+ p.onFullThread?.(
+ JSON.stringify({messages: tombstones(7151, 7052), pagination: {last: false, num: 100}})
+ )
+ return undefined as never
+ })
+ loadBack(trackingActions())
+ await flushPromises()
+ expect(rpc).toHaveBeenCalledTimes(2)
+ })
+
+ test('does not reload when the page actually added ordinals', async () => {
+ // Renderable messages, so the store grows and the list will ask for the next page itself.
+ const rpc = jest.spyOn(ThreadRpc, 'loadThreadNonblock').mockImplementation(async p => {
+ await Promise.resolve()
+ p.onFullThread?.(
+ JSON.stringify({messages: visible(7151, 7052), pagination: {last: false, num: 100}})
+ )
+ return undefined as never
+ })
+ loadBack(trackingActions())
+ await flushPromises()
+ expect(rpc).toHaveBeenCalledTimes(1)
+ })
+
+ test('reloads when a warm cache delivers the tombstones', async () => {
+ // The reported bug's own shape: the conversation is already in local storage, so PullLocalOnly
+ // wins and the cached pass carries the page - which is entirely tombstones. Judging only the
+ // full pass, or refusing to judge at all once a cached pass arrived, leaves this inert.
+ let next = 7151
+ const rpc = jest.spyOn(ThreadRpc, 'loadThreadNonblock').mockImplementation(async p => {
+ const from = next
+ const to = Math.max(6952, from - numMessagesOnScrollback + 1)
+ next = to - 1
+ await Promise.resolve()
+ const body = JSON.stringify({
+ messages: tombstones(from, to),
+ pagination: {last: to <= 6952, num: 100},
+ })
+ p.onCachedThread?.(body)
+ // The full pass is INCREMENTAL once a cached thread has been sent.
+ p.onFullThread?.(JSON.stringify({messages: tombstones(to, to), pagination: {last: to <= 6952, num: 100}}))
+ return undefined as never
+ })
+ loadBack(trackingActions())
+ await flushPromises()
+ expect(rpc).toHaveBeenCalledTimes(2)
+ })
+
+ test('does not reload after a cached pass already delivered the page', async () => {
+ // The normal warm-cache sequence: PullLocalOnly wins, the cached pass carries the whole page,
+ // and the full pass that follows is INCREMENTAL - only the messages that changed, every one of
+ // them already in the window. On ordinal count alone that is indistinguishable from a page of
+ // tombstones, and reloading on it walks the client back through the entire conversation.
+ const rpc = jest.spyOn(ThreadRpc, 'loadThreadNonblock').mockImplementation(async p => {
+ await Promise.resolve()
+ p.onCachedThread?.(
+ JSON.stringify({messages: visible(7151, 7052), pagination: {last: false, num: 100}})
+ )
+ p.onFullThread?.(
+ JSON.stringify({messages: visible(7052, 7052), pagination: {last: false, num: 100}})
+ )
+ return undefined as never
+ })
+ loadBack(trackingActions())
+ await flushPromises()
+ expect(rpc).toHaveBeenCalledTimes(1)
+ })
+
+ test('stops when the window is cleared under it', async () => {
+ // jump to recent and a centered jump both clear then reload. A chain still walking backwards
+ // would prepend pages into a window the reader has just left, producing the disjoint ordinals
+ // this whole branch exists to prevent.
+ let calls = 0
+ const ordinals = new Set([T.Chat.numberToOrdinal(7152)])
+ let clearVersion = 0
+ const actions = {
+ applyThreadLoad: jest.fn(),
+ claimWindowGate: jest.fn(),
+ getSnapshot: () =>
+ ({
+ clearVersion,
+ liveUpdateVersion: 0,
+ loaded: true,
+ messageIDToOrdinal: new Map(),
+ messageMap: new Map(),
+ messageOrdinals: [...ordinals].sort((a, b) => a - b),
+ pendingOutboxToOrdinal: new Map(),
+ }) as unknown as ConversationThreadState,
+ markThreadAsRead: jest.fn(),
+ } as unknown as ConversationThreadActions
+ const rpc = jest.spyOn(ThreadRpc, 'loadThreadNonblock').mockImplementation(async p => {
+ calls++
+ await Promise.resolve()
+ // Someone hits jump-to-recent while the first page is in flight.
+ if (calls === 1) {
+ clearVersion = 1
+ }
+ p.onFullThread?.(
+ JSON.stringify({messages: tombstones(7151, 7052), pagination: {last: false, num: 100}})
+ )
+ return undefined as never
+ })
+ loadBack(actions)
+ await flushPromises()
+ expect(rpc).toHaveBeenCalledTimes(1)
+ })
+
+ test('does not reload an initial load', async () => {
+ const rpc = mockWalkingBack(6152)
+ loadConversationThreadMessages(conversationIDKey, {reason: 'focused'}, trackingActions())
+ await flushPromises()
+ expect(rpc).toHaveBeenCalledTimes(1)
+ })
+})
+
+describe('a load releases the window gate it was issued under', () => {
+ const flushPromises = async () => {
+ for (let i = 0; i < 200; i++) {
+ await Promise.resolve()
+ }
+ }
+
+ // clearVersion is the only thing that separates two loads of the same conversation:
+ // isThreadLoadCurrent is keyed on a generation that moves only when the conversation changes or
+ // the thread unmounts, so both loads call themselves current.
+ const gateActions = (clearVersion: () => number) =>
+ ({
+ applyThreadLoad: jest.fn(),
+ claimWindowGate: jest.fn(),
+ clearWindowGate: jest.fn(),
+ getSnapshot: () =>
+ ({
+ clearVersion: clearVersion(),
+ liveUpdateVersion: 0,
+ loaded: true,
+ messageIDToOrdinal: new Map(),
+ messageMap: new Map(),
+ messageOrdinals: undefined,
+ pendingOutboxToOrdinal: new Map(),
+ }) as unknown as ConversationThreadState,
+ loadMoreMessages: jest.fn(),
+ markThreadAsRead: jest.fn(),
+ }) as unknown as ConversationThreadActions
+
+ beforeEach(() => {
+ useCurrentUserState.getState().dispatch.setBootstrap({
+ deviceID: 'device-id',
+ deviceName: 'testuser-mac',
+ uid: 'uid',
+ username: 'testuser',
+ })
+ })
+
+ afterEach(() => {
+ jest.restoreAllMocks()
+ resetAllStores()
+ })
+
+ test('releases it when the load bails before the rpc is even made', async () => {
+ // The clear issues its reload synchronously, so if that reload is the one bailing there is
+ // nothing else coming to take the gate down and the thread stops receiving messages for good.
+ const rpc = jest.spyOn(ThreadRpc, 'loadThreadNonblock')
+ const actions = gateActions(() => 3)
+ loadConversationThreadMessages(
+ conversationIDKey,
+ {isThreadLoadCurrent: () => false, reason: 'focused'},
+ actions
+ )
+ await flushPromises()
+
+ expect(rpc).not.toHaveBeenCalled()
+ expect(actions.clearWindowGate).toHaveBeenCalledTimes(1)
+ })
+
+ test('releases it when the load ends without ever applying', async () => {
+ // A response that carries no thread: applyThreadLoad never runs, so nothing else would take the
+ // gate down. Left up it drops every notification for the life of the provider.
+ jest.spyOn(ThreadRpc, 'loadThreadNonblock').mockImplementation(async () => {
+ await Promise.resolve()
+ return undefined as never
+ })
+ const actions = gateActions(() => 3)
+ loadConversationThreadMessages(conversationIDKey, {reason: 'focused'}, actions)
+ await flushPromises()
+
+ expect(actions.clearWindowGate).toHaveBeenCalledTimes(1)
+ })
+
+ test('does not apply a response that arrives after a clear', async () => {
+ // The back page is in flight when the reader taps jump-to-recent: messagesClear empties the
+ // window and starts its own load. Applying this one anyway repopulates the window the clear
+ // dropped and lowers the gate the new load is relying on, and the two disjoint pages then
+ // merge - the stranded-row bug the gate exists to prevent.
+ let clearVersion = 3
+ jest.spyOn(ThreadRpc, 'loadThreadNonblock').mockImplementation(async p => {
+ await Promise.resolve()
+ clearVersion = 4
+ p.onFullThread?.(
+ JSON.stringify({
+ messages: [
+ {
+ placeholder: {hidden: false, messageID: T.Chat.numberToMessageID(7152)},
+ state: T.RPCChat.MessageUnboxedState.placeholder,
+ },
+ ],
+ pagination: {last: false, num: 100},
+ })
+ )
+ return undefined as never
+ })
+ const actions = gateActions(() => clearVersion)
+ loadConversationThreadMessages(conversationIDKey, {reason: 'focused'}, actions)
+ await flushPromises()
+
+ expect(actions.applyThreadLoad).not.toHaveBeenCalled()
+ expect(actions.clearWindowGate).not.toHaveBeenCalled()
+ })
+
+ test('does not apply a response while another load owns the gate', async () => {
+ // Two loads issued after the same clear: the reload the clear started owns the gate, and a
+ // ChatThreadsStale reload fired behind it answers first. clearVersion cannot tell them apart -
+ // it moved once, for the clear both of them started after. Applying this one would fill the
+ // cleared window with the newest page while the owner is still fetching a disjoint region, and
+ // the owner's page would then merge into it.
+ let claimed = -1
+ const actions = {
+ applyThreadLoad: jest.fn(),
+ claimWindowGate: jest.fn((loadID: number) => {
+ claimed = loadID
+ }),
+ clearWindowGate: jest.fn(),
+ getSnapshot: () =>
+ ({
+ clearVersion: 3,
+ liveUpdateVersion: 0,
+ loaded: true,
+ messageIDToOrdinal: new Map(),
+ messageMap: new Map(),
+ messageOrdinals: undefined,
+ pendingOutboxToOrdinal: new Map(),
+ // Someone else got here first.
+ windowCleared: true,
+ windowGateOwner: claimed + 1,
+ }) as unknown as ConversationThreadState,
+ loadMoreMessages: jest.fn(),
+ markThreadAsRead: jest.fn(),
+ } as unknown as ConversationThreadActions
+ jest.spyOn(ThreadRpc, 'loadThreadNonblock').mockImplementation(async p => {
+ await Promise.resolve()
+ p.onFullThread?.(
+ JSON.stringify({
+ messages: [
+ {
+ placeholder: {hidden: false, messageID: T.Chat.numberToMessageID(7152)},
+ state: T.RPCChat.MessageUnboxedState.placeholder,
+ },
+ ],
+ pagination: {last: false, num: 100},
+ })
+ )
+ return undefined as never
+ })
+ loadConversationThreadMessages(conversationIDKey, {reason: 'focused'}, actions)
+ await flushPromises()
+
+ expect(actions.applyThreadLoad).not.toHaveBeenCalled()
+ })
+
+ test('applies the response of the load that owns the gate', async () => {
+ // The other half: the gate is up and this is the reload that claimed it, so it is the one
+ // allowed to refill the window.
+ let claimed = -1
+ const actions = {
+ applyThreadLoad: jest.fn(),
+ claimWindowGate: jest.fn((loadID: number) => {
+ claimed = loadID
+ }),
+ clearWindowGate: jest.fn(),
+ getSnapshot: () =>
+ ({
+ clearVersion: 3,
+ liveUpdateVersion: 0,
+ loaded: true,
+ messageIDToOrdinal: new Map(),
+ messageMap: new Map(),
+ messageOrdinals: undefined,
+ pendingOutboxToOrdinal: new Map(),
+ windowCleared: true,
+ windowGateOwner: claimed,
+ }) as unknown as ConversationThreadState,
+ loadMoreMessages: jest.fn(),
+ markThreadAsRead: jest.fn(),
+ } as unknown as ConversationThreadActions
+ jest.spyOn(ThreadRpc, 'loadThreadNonblock').mockImplementation(async p => {
+ await Promise.resolve()
+ p.onFullThread?.(
+ JSON.stringify({
+ messages: [
+ {
+ placeholder: {hidden: false, messageID: T.Chat.numberToMessageID(7152)},
+ state: T.RPCChat.MessageUnboxedState.placeholder,
+ },
+ ],
+ pagination: {last: false, num: 100},
+ })
+ )
+ return undefined as never
+ })
+ loadConversationThreadMessages(conversationIDKey, {reason: 'focused'}, actions)
+ await flushPromises()
+
+ expect(actions.applyThreadLoad).toHaveBeenCalled()
+ })
+
+ test('leaves a newer clear’s gate alone', async () => {
+ // The load is in flight when the user taps a search result: messagesClear bumps clearVersion and
+ // starts its own load. This one must not pull down the gate that one is relying on - the load
+ // generation does not move between two loads of the same conversation, so it cannot tell them
+ // apart on its own.
+ let clearVersion = 3
+ jest.spyOn(ThreadRpc, 'loadThreadNonblock').mockImplementation(async () => {
+ await Promise.resolve()
+ clearVersion = 4
+ return undefined as never
+ })
+ const actions = gateActions(() => clearVersion)
+ loadConversationThreadMessages(conversationIDKey, {reason: 'focused'}, actions)
+ await flushPromises()
+
+ expect(actions.clearWindowGate).not.toHaveBeenCalled()
+ })
+})
+describe('only a pass that can account for a whole window reconciles', () => {
+ const flushPromises = async () => {
+ for (let i = 0; i < 200; i++) {
+ await Promise.resolve()
+ }
+ }
+
+ const page = (from: number, to: number) =>
+ Array.from({length: from - to + 1}, (_, i) => ({
+ placeholder: {hidden: false, messageID: T.Chat.numberToMessageID(from - i)},
+ state: T.RPCChat.MessageUnboxedState.placeholder,
+ }))
+
+ const recordingActions = () =>
+ ({
+ applyThreadLoad: jest.fn(),
+ claimWindowGate: jest.fn(),
+ clearWindowGate: jest.fn(),
+ getSnapshot: () =>
+ ({
+ clearVersion: 0,
+ liveUpdateVersion: 0,
+ loaded: true,
+ messageIDToOrdinal: new Map(),
+ messageMap: new Map(),
+ messageOrdinals: undefined,
+ pendingOutboxToOrdinal: new Map(),
+ }) as unknown as ConversationThreadState,
+ loadMoreMessages: jest.fn(),
+ markThreadAsRead: jest.fn(),
+ }) as unknown as ConversationThreadActions
+
+ const mockPasses = (cached: string, full: string) =>
+ jest.spyOn(ThreadRpc, 'loadThreadNonblock').mockImplementation(async p => {
+ await Promise.resolve()
+ p.onCachedThread?.(cached)
+ p.onFullThread?.(full)
+ return undefined as never
+ })
+
+ // Whether the last pass of a load was the one that reconciles. What gets pruned is the store's
+ // business - addMessagesToThreadState fills the carried set itself - so these tests only check
+ // which passes are allowed to ask for it; the thread-context suite covers the pruning.
+ const prunedOnLastPass = (actions: ConversationThreadActions) => {
+ const calls = (actions.applyThreadLoad as unknown as jest.Mock).mock.calls
+ return (calls.at(-1)?.[0] as {reconcile?: ThreadLoadReconcile} | undefined)?.reconcile?.prune
+ }
+
+ beforeEach(() => {
+ useCurrentUserState.getState().dispatch.setBootstrap({
+ deviceID: 'device-id',
+ deviceName: 'testuser-mac',
+ uid: 'uid',
+ username: 'testuser',
+ })
+ })
+
+ afterEach(() => {
+ jest.restoreAllMocks()
+ resetAllStores()
+ })
+
+ test('reconciles on a full pass that followed an empty cached one', async () => {
+ // First open after a db nuke: PullLocalOnly finds nothing, but its collector suppresses the miss
+ // and a cached pass is sent anyway, carrying no messages. INCREMENTAL against an empty local
+ // thread filters nothing out, so the full pass really is the whole window - and only a whole
+ // window may prune the stale ordinals a cache repair left behind.
+ const actions = recordingActions()
+ mockPasses(
+ JSON.stringify({messages: null, pagination: {last: false, num: 100}}),
+ JSON.stringify({messages: page(7153, 7152), pagination: {last: false, num: 100}})
+ )
+ loadConversationThreadMessages(conversationIDKey, {reason: 'focused'}, actions)
+ await flushPromises()
+
+ expect(prunedOnLastPass(actions)).toBe(true)
+ })
+
+ test('does not reconcile when a cached pass arrived but another load owned the window', async () => {
+ // The gate-owner guard is the one guard that can turn a cached pass away and still let the full
+ // pass behind it through: the owner drops the gate in between. The service counts that cached
+ // pass as sent either way, so the full pass is INCREMENTAL - a handful of changed messages - and
+ // a span built from those alone covers every row between them with nothing recorded as present.
+ // That is not a stale-row cleanup, it is deleting the thread.
+ let claimed = -1
+ let ownedByAnother = true
+ const actions = {
+ applyThreadLoad: jest.fn(),
+ claimWindowGate: jest.fn((loadID: number) => {
+ claimed = loadID
+ }),
+ clearWindowGate: jest.fn(),
+ getSnapshot: () =>
+ ({
+ clearVersion: 0,
+ liveUpdateVersion: 0,
+ loaded: true,
+ messageIDToOrdinal: new Map(),
+ messageMap: new Map(),
+ messageOrdinals: undefined,
+ pendingOutboxToOrdinal: new Map(),
+ windowCleared: ownedByAnother,
+ windowGateOwner: claimed + 1,
+ }) as unknown as ConversationThreadState,
+ loadMoreMessages: jest.fn(),
+ markThreadAsRead: jest.fn(),
+ } as unknown as ConversationThreadActions
+ jest.spyOn(ThreadRpc, 'loadThreadNonblock').mockImplementation(async p => {
+ await Promise.resolve()
+ p.onCachedThread?.(
+ JSON.stringify({messages: page(7153, 7052), pagination: {last: false, num: 100}})
+ )
+ // The load that owned the gate settles here, so the full pass is no longer refused.
+ ownedByAnother = false
+ p.onFullThread?.(
+ JSON.stringify({messages: page(7153, 7150), pagination: {last: false, num: 100}})
+ )
+ return undefined as never
+ })
+ loadConversationThreadMessages(conversationIDKey, {reason: 'focused'}, actions)
+ await flushPromises()
+
+ expect(actions.applyThreadLoad).not.toHaveBeenCalled()
+ })
+
+ test('does not reconcile when the service never reported a cached pass', async () => {
+ // The service records the cached thread as sent before it marshals it, so a failure there
+ // leaves the full pass INCREMENTAL against a pass we were never shown. The cached callback
+ // firing - with a thread, or with the nil a cold cache sends - is the only sign we get that
+ // this did not happen.
+ const actions = recordingActions()
+ jest.spyOn(ThreadRpc, 'loadThreadNonblock').mockImplementation(async p => {
+ await Promise.resolve()
+ p.onFullThread?.(
+ JSON.stringify({messages: page(7153, 7150), pagination: {last: false, num: 100}})
+ )
+ return undefined as never
+ })
+ loadConversationThreadMessages(conversationIDKey, {reason: 'focused'}, actions)
+ await flushPromises()
+
+ expect(prunedOnLastPass(actions)).toBe(false)
+ })
+
+ test('reconciles on the full pass of a warm-cache load, against both passes', async () => {
+ // The warm-cache sequence: the cached pass carries the page and the full pass behind it is
+ // INCREMENTAL, only what changed. Neither is a window on its own - but INCREMENTAL walks the
+ // authoritative window and omits only what the cached pass already carried unchanged, so the
+ // two together are that window, and the range spans both. Judging the full pass alone would
+ // give up pruning on every conversation the cache is warm for, which is all of them after the
+ // first open.
+ const actions = recordingActions()
+ mockPasses(
+ JSON.stringify({messages: page(7153, 7052), pagination: {last: false, num: 100}}),
+ JSON.stringify({messages: page(7153, 7153), pagination: {last: false, num: 100}})
+ )
+ loadConversationThreadMessages(conversationIDKey, {reason: 'focused'}, actions)
+ await flushPromises()
+
+ expect(prunedOnLastPass(actions)).toBe(true)
+ })
+})
diff --git a/shared/chat/conversation/thread-load.tsx b/shared/chat/conversation/thread-load.tsx
index de4121253cad..1798882eb82f 100644
--- a/shared/chat/conversation/thread-load.tsx
+++ b/shared/chat/conversation/thread-load.tsx
@@ -12,7 +12,7 @@ import {persistRoute} from '@/util/storeless-actions'
import {uint8ArrayToString} from '@/util/uint8array'
import {useCurrentUserState} from '@/stores/current-user'
import {useConfigState} from '@/stores/config'
-import {getOrdinalForMessageID} from './thread-message-state'
+import {type ThreadLoadReconcile, getOrdinalForMessageID} from './thread-message-state'
import {getInboxConversationMeta, updateInboxConversationMeta} from '@/chat/inbox/metadata'
import {loadThreadNonblock, threadLoadReasonToRPCReason} from './thread-rpc'
import type {
@@ -22,7 +22,14 @@ import type {
ScrollDirection,
} from './thread-context'
+// Identifies one load, so the window gate can tell two loads of the same conversation apart.
+// Only ever compared for equality, never ordered.
+let nextLoadID = 0
+
export const numMessagesOnInitialLoad = isMobile ? 20 : 100
+// How far the no-new-ordinals back-page chain will walk on its own before handing the thread back to
+// the reader. See the reload block in loadConversationThreadMessages.
+export const maxBackPageReloads = 10
export const numMessagesOnScrollback = 100
const ignoreErrors = [
@@ -162,7 +169,12 @@ export const loadConversationThreadMessages = (
if (!T.Chat.isValidConversationIDKey(conversationIDKey)) {
return
}
- const {scrollDirection = 'none', numberOfMessagesToLoad = numMessagesOnInitialLoad} = p
+ const {
+ scrollDirection = 'none',
+ numberOfMessagesToLoad = numMessagesOnInitialLoad,
+ retryBelowMessageID,
+ retryCount = 0,
+ } = p
const {
allowMarkAsRead = true,
reason,
@@ -176,20 +188,50 @@ export const loadConversationThreadMessages = (
const isCurrentThreadLoad = () => isThreadLoadCurrent?.() ?? true
const f = async () => {
+ const loadStartedSnapshot = actions.getSnapshot()
+ const clearVersionAtLoadStart = loadStartedSnapshot.clearVersion
+ // applyThreadLoad drops the window gate when a load refills the window, but a load can end
+ // without ever applying: offline, scchatnotinteam, a response carrying no thread, or a bail
+ // before the RPC is even made. Left alone the gate would keep dropping notifications for the
+ // life of the provider, which is a thread that silently stops receiving messages.
+ //
+ // Keyed on clearVersion, not on isThreadLoadCurrent: the load generation only moves when the
+ // conversation changes or the thread unmounts, so two loads of the same conversation both call
+ // themselves current. A load that started before the clear would otherwise pull down the gate
+ // belonging to the load that started after it, while that one is still in flight.
+ //
+ // clearVersion alone still cannot separate two loads issued after the same clear, so the gate
+ // is also owned: first claim wins, and only the owner may drop it. Claimed here, before the
+ // first await, rather than when a response arrives - both clear paths bypass the load throttle
+ // (see loadMoreMessages in thread-context) and call in synchronously, so the reload the clear
+ // issued is always the first to get here, and a load that ends without ever applying still has
+ // to be the one that releases.
+ const loadID = nextLoadID++
+ actions.claimWindowGate(loadID)
+ const releaseWindowGate = () => {
+ if (actions.getSnapshot().clearVersion === clearVersionAtLoadStart) {
+ actions.clearWindowGate(loadID)
+ }
+ }
+ // Every bail from here on releases, including the two that used to sit above the claim: the
+ // clear issues its reload synchronously, so if that reload is the one bailing there is nothing
+ // else coming to take the gate down, and the thread stops receiving messages for good.
if (!isCurrentThreadLoad()) {
logger.info('loadMoreMessages: bail: stale mounted thread load')
+ releaseWindowGate()
return
}
if (!conversationIDKey || !T.Chat.isValidConversationIDKey(conversationIDKey)) {
logger.info('loadMoreMessages: bail: no conversationIDKey')
+ releaseWindowGate()
return
}
- const loadStartedSnapshot = actions.getSnapshot()
const currentMeta = getMeta(conversationIDKey)
if (currentMeta.membershipType === 'youAreReset' || currentMeta.rekeyers.size > 0) {
logger.info('loadMoreMessages: bail: we are reset')
+ releaseWindowGate()
return
}
const loadStartedLiveUpdateVersion = loadStartedSnapshot.liveUpdateVersion
@@ -204,20 +246,78 @@ export const loadConversationThreadMessages = (
)
const loadingKey = Strings.waitingKeyChatThreadLoad(conversationIDKey)
- let reconciled = false
+ // What this load has put in the window, filled in by addMessagesToThreadState as each pass
+ // applies. Once the service has sent a cached thread it switches the full response to
+ // INCREMENTAL, which walks the authoritative window and sends only the messages that cached
+ // pass did not already carry unchanged (mergeLocalRemoteThread in go/chat/uithreadloader.go,
+ // where localSentThread is that exact pass). Neither pass is a whole window on its own, so the
+ // two are gathered here and the last one reconciles against the both of them.
+ const carried = new Set()
+ // A load is all or nothing. Once one of its passes is turned away, the rest of them are too:
+ // the service filters each pass against what it has already sent this load, so the ones that
+ // follow a refused pass are a subset of a window we never took, and both ways of using them
+ // are wrong. Merging one into whatever refilled the window in the meantime is the disjoint
+ // window this whole invariant exists to prevent; reconciling against one takes out every row
+ // between the few messages it happens to carry.
+ let refusedAPass = false
+ // Whether the service's cached goroutine reported at all - with a thread, or with the nil it
+ // sends when the local cache had nothing. It is the only evidence the client gets that the
+ // full pass was not filtered behind our back: the service records the cached thread as sent
+ // before it marshals it, so a marshal failure there leaves us with an INCREMENTAL full pass
+ // and no sign of the pass it was filtered against (LoadNonblock in
+ // go/chat/uithreadloader.go). No report, no reconciling.
+ let sawCachedReport = false
+ // The reload below is judged against the whole load, not one pass of it. A warm-cache load
+ // delivers the page on the cached pass and then an INCREMENTAL full pass carrying only what
+ // changed, so measuring the full pass alone says "added nothing" for a perfectly good page.
+ // Measuring from before either pass tells the two apart: a page of real messages moves this,
+ // a page of tombstones does not, wherever it arrived.
+ const floorAtLoadStart = loadStartedSnapshot.messageOrdinals?.[0]
+ let oldestSeenThisLoad = Number.MAX_SAFE_INTEGER as T.Chat.MessageID
const onGotThread = (thread: string, why: string) => {
if (!thread) {
return
}
+ if (refusedAPass) {
+ logger.info(`loadMoreMessages: pass ignored, an earlier one of this load was: ${why}`)
+ return
+ }
+ const refuse = (msg: string) => {
+ refusedAPass = true
+ logger.info(msg)
+ }
if (!isCurrentThreadLoad()) {
- logger.info(`loadMoreMessages: stale response ignored: ${why}`)
+ refuse(`loadMoreMessages: stale response ignored: ${why}`)
+ return
+ }
+ // A clear under us - jump to recent, a centered jump - dropped the window this load was
+ // paging against, and the reload that follows fetches a disjoint region. isCurrentThreadLoad
+ // does not catch it: the load generation only moves when the conversation changes or the
+ // thread unmounts, so a load that started before the clear still calls itself current.
+ // Applying it anyway would repopulate the cleared window and lower the gate belonging to the
+ // reload, which then merges its own page into the leftovers.
+ const snapshotAtResponse = actions.getSnapshot()
+ if (snapshotAtResponse.clearVersion !== clearVersionAtLoadStart) {
+ refuse(`loadMoreMessages: response ignored after clear: ${why}`)
return
}
+ // clearVersion cannot separate two loads issued after the same clear, and the second one is
+ // not hypothetical: a ChatThreadsStale or ChatInboxSynced reload fires with scrollDirection
+ // 'none' and fetches the newest page, not the region the clear asked for. If it answers
+ // first it would fill the cleared window with that disjoint page and drop the gate, and the
+ // reload the clear issued would then merge its own page into the leftovers - exactly the
+ // ordinal gap the gate exists to prevent. While the gate is up only its owner may refill the
+ // window; once the owner settles the gate is down and everyone applies normally again.
if (
- protectLoadedFocusRefresh &&
- actions.getSnapshot().liveUpdateVersion !== loadStartedLiveUpdateVersion
+ snapshotAtResponse.windowCleared &&
+ snapshotAtResponse.windowGateOwner !== undefined &&
+ snapshotAtResponse.windowGateOwner !== loadID
) {
- logger.info(
+ refuse(`loadMoreMessages: response ignored, another load owns the window: ${why}`)
+ return
+ }
+ if (protectLoadedFocusRefresh && snapshotAtResponse.liveUpdateVersion !== loadStartedLiveUpdateVersion) {
+ refuse(
`loadMoreMessages: stale response ignored after live update: ${why} reason=${reason} convID=${conversationIDKey}`
)
return
@@ -239,19 +339,17 @@ export const loadConversationThreadMessages = (
scrollDirection !== 'back' &&
reason !== 'findNewestConversation' &&
reason !== 'findNewestConversationFromLayout'
- let validatedRange: {from: T.Chat.Ordinal; to: T.Chat.Ordinal} | undefined
- if (messages.length) {
- if (scrollDirection === 'none' && !reconciled) {
- const ords = messages
- .filter(m => m.conversationMessage !== false && m.type !== 'deleted')
- .map(m => m.ordinal)
- if (ords.length > 0) {
- validatedRange = {
- from: Math.min(...ords) as T.Chat.Ordinal,
- to: Math.max(...ords) as T.Chat.Ordinal,
- }
- }
- reconciled = true
+ // Reconciling is only safe against a whole window, and a single pass is not one: the cached
+ // pass is whatever the local cache holds, gaps included, and the full pass behind it carries
+ // only what changed. The full pass is the last one, so it is the one that prunes - against
+ // everything both passes delivered. Waiting instead for a pass with no cached one before it
+ // would leave the stale-row cleanup running on cold caches only, which is where ghost rows
+ // are least likely to be: a reopened conversation is warm every time.
+ const reconcile: ThreadLoadReconcile | undefined =
+ scrollDirection === 'none' ? {carried, prune: why === 'full' && sawCachedReport} : undefined
+ for (const m of messages) {
+ if (m.id > 0 && m.id < oldestSeenThisLoad) {
+ oldestSeenThisLoad = m.id
}
}
actions.applyThreadLoad({
@@ -261,9 +359,64 @@ export const loadConversationThreadMessages = (
forceContainsLatestCalc,
messages,
moreToLoad,
+ reconcile,
scrollDirection,
- validatedRange,
})
+ const after = actions.getSnapshot()
+ // A back page can be composed entirely of messages the thread will never render: a message
+ // superseded by a DELETE arrives as a hidden placeholder, becomes `deleted`, and addMessages
+ // drops it. The ordinal list is then identical to what it was, so the list never fires
+ // onStartReached again and scrollback stops even though the pager says there is more. Ask for
+ // the next page ourselves.
+ //
+ // The tombstones still carry message IDs, and each page reaches further back than the last,
+ // so requiring strict progress terminates: message IDs are finite and only ever decrease
+ // here. Strict progress alone is a weak bound though - a channel whose history was largely
+ // expunged has tens of thousands of them, which is minutes of paging off one gesture - so the
+ // chain also stops after maxBackPageReloads. Stopping is safe: the reader is still pinned at
+ // the top with an unchanged list, and scrolling away and back fires onStartReached again,
+ // which starts a fresh chain from wherever this one left off.
+ const floorAfter = after.messageOrdinals?.[0]
+ const windowGrewDownward =
+ floorAfter !== undefined && (floorAtLoadStart === undefined || floorAfter < floorAtLoadStart)
+ if (
+ scrollDirection === 'back' &&
+ // The full pass is the last one of a load, so by here the whole load has been applied.
+ why === 'full' &&
+ moreToLoad &&
+ // The floor, not the count: a page can add real messages while its `deleted` entries
+ // remove more from the window, which nets negative on a count but is real progress.
+ !windowGrewDownward &&
+ oldestSeenThisLoad < (retryBelowMessageID ?? Number.MAX_SAFE_INTEGER) &&
+ retryCount < maxBackPageReloads
+ ) {
+ logger.info(
+ `loadMoreMessages: back page added no ordinals, reloading below ${oldestSeenThisLoad} (${
+ retryCount + 1
+ }/${maxBackPageReloads}): convID: ${conversationIDKey}`
+ )
+ // Through the action, not loadConversationThreadMessages directly: the action carries the
+ // 500ms throttle and the unmount cancel(), and a long run of tombstones would otherwise
+ // issue these back to back with no pacing. The throttle only ever drops a call that a
+ // later load supersedes, and that load extends the window or retries in turn.
+ //
+ // The delay has a cost: the next page comes from a cursor the daemon holds, not one we
+ // send. pgmode is SERVER (see thread-rpc), so `next` resolves against convPageStatus in the
+ // service, and any first-page request resets it (applyPagerModeOutgoing in
+ // go/chat/uithreadloader.go) - which every scrollDirection 'none' load is, stale and focus
+ // reloads included. One landing inside the throttle window makes this retry fetch near the
+ // top of the thread instead of the next page back. It fails closed rather than looping:
+ // oldestSeenThisLoad is then no lower than retryBelowMessageID, so the chain stops and the
+ // reader is left where another scroll gesture starts a fresh one.
+ //
+ // Sizing, for the same reason the chain is bounded at all: a full run is 11 sequential
+ // 100-message RPCs off one gesture, several seconds of paging with nothing visible moving.
+ actions.loadMoreMessages({
+ ...p,
+ retryBelowMessageID: oldestSeenThisLoad,
+ retryCount: retryCount + 1,
+ })
+ }
if (canMarkReadForThreadWindow) {
actions.markThreadAsRead()
@@ -278,7 +431,10 @@ export const loadConversationThreadMessages = (
conversationIDKey,
knownRemotes,
messageIDControl,
- onCachedThread: thread => onGotThread(thread, 'cached'),
+ onCachedThread: thread => {
+ sawCachedReport = true
+ onGotThread(thread, 'cached')
+ },
onFullThread: thread => onGotThread(thread, 'full'),
onThreadStatus: status => {
logger.info(
@@ -313,6 +469,8 @@ export const loadConversationThreadMessages = (
throw error
}
}
+ } finally {
+ releaseWindowGate()
}
}
diff --git a/shared/chat/conversation/thread-message-state.test.tsx b/shared/chat/conversation/thread-message-state.test.tsx
index c883899caf91..8ba2bfb62629 100644
--- a/shared/chat/conversation/thread-message-state.test.tsx
+++ b/shared/chat/conversation/thread-message-state.test.tsx
@@ -80,6 +80,11 @@ const makeThreadState = (
messageMap,
messageOrdinals,
messageTypeMap,
+ // A thread that has loaded at least once, so the two flags below mean what they say.
+ loaded: true,
+ // A partial window by default: the drop rules only bound an edge that still has more past it.
+ moreToLoadBack: true,
+ moreToLoadForward: false,
pendingOutboxToOrdinal,
...extra,
}
@@ -374,28 +379,107 @@ describe('addMessagesToThreadState', () => {
expect(state.messageMap.has(T.Chat.numberToOrdinal(20))).toBe(true)
})
- test('a validated range prunes local ordinals the service did not send back', () => {
+ test('a reconciling pass prunes local ordinals the service did not send back', () => {
const state = makeThreadState([])
addMessagesToThreadState(state, [textAt(10), textAt(20), textAt(30)], {})
addMessagesToThreadState(state, [textAt(10), textAt(30)], {
- validatedRange: {from: T.Chat.numberToOrdinal(10), to: T.Chat.numberToOrdinal(30)},
+ reconcile: {carried: new Set(), prune: true},
})
expect(state.messageOrdinals).toEqual([10, 30])
expect(state.messageMap.has(T.Chat.numberToOrdinal(20))).toBe(false)
- expect(state.validatedOrdinalRange).toEqual({from: 10, to: 30})
})
- test('a validated range leaves ordinals outside of it alone and widens the known range', () => {
+ test('a reconciling pass counts what an earlier pass of the same load carried', () => {
+ // The warm shape, at this level: the cached pass delivers the page, the full pass behind it
+ // only what changed. The span covers both, and a row the first pass delivered is present even
+ // though the second never mentions it.
const state = makeThreadState([])
- addMessagesToThreadState(state, [textAt(10), textAt(50)], {})
- addMessagesToThreadState(state, [textAt(50)], {
- validatedRange: {from: T.Chat.numberToOrdinal(40), to: T.Chat.numberToOrdinal(60)},
+ const carried = new Set()
+ addMessagesToThreadState(state, [textAt(10), textAt(20), textAt(30)], {
+ reconcile: {carried, prune: false},
})
- expect(state.messageOrdinals).toEqual([10, 50])
- addMessagesToThreadState(state, [textAt(10)], {
- validatedRange: {from: T.Chat.numberToOrdinal(5), to: T.Chat.numberToOrdinal(15)},
+ addMessagesToThreadState(state, [textAt(30)], {reconcile: {carried, prune: true}})
+ expect(state.messageOrdinals).toEqual([10, 20, 30])
+
+ // ...and a row neither pass carried is stale, so it goes.
+ const withGhost = makeThreadState([textAt(10), textAt(20), textAt(30)])
+ const carriedAgain = new Set()
+ addMessagesToThreadState(withGhost, [textAt(10)], {reconcile: {carried: carriedAgain, prune: false}})
+ addMessagesToThreadState(withGhost, [textAt(30)], {reconcile: {carried: carriedAgain, prune: true}})
+ expect(withGhost.messageOrdinals).toEqual([10, 30])
+ })
+
+ test('a reconciling pass leaves ordinals outside its span alone', () => {
+ const state = makeThreadState([])
+ addMessagesToThreadState(state, [textAt(10), textAt(50), textAt(60)], {})
+ addMessagesToThreadState(state, [textAt(50), textAt(60)], {
+ reconcile: {carried: new Set(), prune: true},
})
- expect(state.validatedOrdinalRange).toEqual({from: 5, to: 60})
+ // 10 is below everything the load covered, so nothing is known about it.
+ expect(state.messageOrdinals).toEqual([10, 50, 60])
+ })
+
+ test('a notification may not strand a new ordinal below the loaded window', () => {
+ // The post-load ResolveSkippedUnboxeds push can carry the channel-name message at ID 1 long
+ // after the window has moved on. Adding it puts an orphan row at index 0 and breaks scrollback.
+ const state = makeThreadState([])
+ addMessagesToThreadState(state, [textAt(7152), textAt(7153)], {})
+ addMessagesToThreadState(state, [textAt(1)], {dropNewBelowWindow: true})
+ expect(state.messageOrdinals).toEqual([7152, 7153])
+ })
+
+ test('a notification still updates a message already inside the window', () => {
+ const state = makeThreadState([])
+ addMessagesToThreadState(state, [textAt(10), textAt(20)], {})
+ addMessagesToThreadState(state, [textAt(10, {text: 'edited'})], {dropNewBelowWindow: true})
+ expect(state.messageOrdinals).toEqual([10, 20])
+ const m = state.messageMap.get(T.Chat.numberToOrdinal(10))
+ expect(m?.type === 'text' ? m.text.stringValue() : undefined).toBe('edited')
+ })
+
+ test('a notification may still append a new ordinal above the window', () => {
+ const state = makeThreadState([])
+ addMessagesToThreadState(state, [textAt(10), textAt(20)], {})
+ addMessagesToThreadState(state, [textAt(21)], {dropNewBelowWindow: true})
+ expect(state.messageOrdinals).toEqual([10, 20, 21])
+ })
+
+ test('a thread load may still extend the window downward', () => {
+ const state = makeThreadState([])
+ addMessagesToThreadState(state, [textAt(7152), textAt(7153)], {})
+ addMessagesToThreadState(state, [textAt(7038)], {})
+ expect(state.messageOrdinals).toEqual([7038, 7152, 7153])
+ })
+
+ test('a superseded placeholder does not strand its own ordinal in the list', () => {
+ // A sent message keeps the fractional ordinal it had in the outbox, so a later placeholder for
+ // the same message ID maps onto that fractional ordinal, not its own integer one.
+ const state = makeThreadState([])
+ addMessagesToThreadState(
+ state,
+ [
+ makeTextMessage({
+ id: T.Chat.numberToMessageID(100),
+ ordinal: T.Chat.numberToOrdinal(100.001),
+ outboxID: undefined,
+ }),
+ ],
+ {}
+ )
+ expect(state.messageOrdinals).toEqual([100.001])
+ addMessagesToThreadState(
+ state,
+ [
+ Message.makeMessagePlaceholder({
+ conversationIDKey: convID,
+ id: T.Chat.numberToMessageID(100),
+ ordinal: T.Chat.numberToOrdinal(100),
+ }),
+ ],
+ {}
+ )
+ expect(state.messageOrdinals).toEqual([100.001])
+ expect(state.messageMap.has(T.Chat.numberToOrdinal(100))).toBe(false)
})
test('the render type index only tracks non text messages', () => {
@@ -405,4 +489,187 @@ describe('addMessagesToThreadState', () => {
expect(state.messageTypeMap.has(T.Chat.numberToOrdinal(10))).toBe(false)
expect(state.messageTypeMap.get(T.Chat.numberToOrdinal(20))).toBe('attachment:file')
})
+
+ test('a placeholder for a message we already hold does not get it pruned', () => {
+ // Regression: the placeholder bailed out of incomingOrdinals bookkeeping, so the prune saw its
+ // ordinal as absent from the response and deleted the real message underneath.
+ // A quick-mode Pull returns a placeholder for anything it could not unbox, so this is the
+ // ordinary shape of a focused refresh, not an edge case.
+ const state = makeThreadState([textAt(49), textAt(50), textAt(51)])
+ addMessagesToThreadState(
+ state,
+ [textAt(49), Message.makeMessagePlaceholder({ordinal: T.Chat.numberToOrdinal(50)}), textAt(51)],
+ {reconcile: {carried: new Set(), prune: true}}
+ )
+ expect(state.messageOrdinals).toEqual([
+ T.Chat.numberToOrdinal(49),
+ T.Chat.numberToOrdinal(50),
+ T.Chat.numberToOrdinal(51),
+ ])
+ expect(state.messageMap.get(T.Chat.numberToOrdinal(50))?.type).toEqual('text')
+ })
+
+ test('a message dropped below the window leaves nothing behind in the maps', () => {
+ const state = makeThreadState([textAt(7152), textAt(7153)])
+ addMessagesToThreadState(state, [textAt(1)], {dropNewBelowWindow: true})
+
+ expect(state.messageOrdinals).toEqual([
+ T.Chat.numberToOrdinal(7152),
+ T.Chat.numberToOrdinal(7153),
+ ])
+ // The ordinal is not in the list, so nothing may still point at it. An entry left in these maps
+ // makes getOrdinalForMessageID hand out an ordinal with no row, and callers act on a message the
+ // thread is not showing.
+ expect(state.messageMap.has(T.Chat.numberToOrdinal(1))).toBe(false)
+ expect(state.messageIDToOrdinal.has(T.Chat.numberToMessageID(1))).toBe(false)
+ expect(state.messageTypeMap.has(T.Chat.numberToOrdinal(1))).toBe(false)
+ })
+
+ test('a message dropped below the window does not disturb one already in the window', () => {
+ const state = makeThreadState([textAt(7152), textAt(7153)])
+ addMessagesToThreadState(state, [textAt(1), textAt(7152)], {dropNewBelowWindow: true})
+
+ expect(state.messageOrdinals).toEqual([
+ T.Chat.numberToOrdinal(7152),
+ T.Chat.numberToOrdinal(7153),
+ ])
+ expect(state.messageMap.has(T.Chat.numberToOrdinal(1))).toBe(false)
+ expect(state.messageMap.has(T.Chat.numberToOrdinal(7152))).toBe(true)
+ expect(state.messageIDToOrdinal.get(T.Chat.numberToMessageID(7152))).toEqual(
+ T.Chat.numberToOrdinal(7152)
+ )
+ })
+
+ test('a message newer than the window is dropped while there is more to load forward', () => {
+ // A centered jump - a search result - leaves a contiguous window with more on both sides of it.
+ // A push newer than the ceiling has a hole under it just as a below-floor one has a hole over
+ // it, and paging forward is what fills that hole.
+ const state = makeThreadState([textAt(7152), textAt(7153)], {moreToLoadForward: true})
+ addMessagesToThreadState(state, [textAt(9001)], {dropNewBelowWindow: true})
+
+ expect(state.messageOrdinals).toEqual([T.Chat.numberToOrdinal(7152), T.Chat.numberToOrdinal(7153)])
+ expect(state.messageMap.has(T.Chat.numberToOrdinal(9001))).toBe(false)
+ })
+
+ test('a message older than the window prepends once the window reaches the oldest message', () => {
+ // The mirror of the ceiling rule. A small channel pages back to its start, but message 1 - the
+ // setChannelname system message - came back as a hidden placeholder and was dropped, so the
+ // floor is 2. The ResolveSkippedUnboxeds push then delivers the real message 1. With no more to
+ // load back there is no hole under the floor for it to strand against, and nothing else will
+ // ever fetch it: loadOlderMessagesDueToScroll bails outright once moreToLoadBack is false.
+ const state = makeThreadState([textAt(2), textAt(3)], {moreToLoadBack: false})
+ addMessagesToThreadState(state, [textAt(1)], {dropNewBelowWindow: true})
+
+ expect(state.messageOrdinals).toEqual([
+ T.Chat.numberToOrdinal(1),
+ T.Chat.numberToOrdinal(2),
+ T.Chat.numberToOrdinal(3),
+ ])
+ })
+
+ test('a message newer than the window appends once the window reaches the newest message', () => {
+ // The ordinary live path: the window contains the latest message, so there is no hole to open
+ // above it and an incoming message must land.
+ const state = makeThreadState([textAt(7152), textAt(7153)], {moreToLoadForward: false})
+ addMessagesToThreadState(state, [textAt(9001)], {dropNewBelowWindow: true})
+
+ expect(state.messageOrdinals).toEqual([
+ T.Chat.numberToOrdinal(7152),
+ T.Chat.numberToOrdinal(7153),
+ T.Chat.numberToOrdinal(9001),
+ ])
+ })
+
+ test('a cleared window refuses a notification in either direction', () => {
+ // messagesClear wipes messageOrdinals and marks the gap, because jumpToRecent and a centered
+ // jump both clear then reload. A push landing in that gap would otherwise face an empty window,
+ // install itself as the whole of it, and strand once the load response arrives. Neither reload
+ // can be predicted from the old window - a centered jump lands on an arbitrary region, and
+ // jump-to-recent on the newest page, which is nowhere near a reader parked far back - so above
+ // strands as surely as below.
+ const state = makeThreadState([])
+ state.windowCleared = true
+ addMessagesToThreadState(state, [textAt(1), textAt(7155), textAt(9001)], {dropNewBelowWindow: true})
+ expect(state.messageOrdinals ?? []).toEqual([])
+
+ // ...and the load that follows populates it normally.
+ addMessagesToThreadState(state, [textAt(8900), textAt(8901)], {})
+ expect(state.messageOrdinals).toEqual([T.Chat.numberToOrdinal(8900), T.Chat.numberToOrdinal(8901)])
+ })
+
+ test('with no window and nothing cleared a notification still applies', () => {
+ // A conversation that has never held a window - a first load, or one that is genuinely empty.
+ // There is no floor to be below, so nothing is dropped and a new message appears at once.
+ const state = makeThreadState([])
+ addMessagesToThreadState(state, [textAt(1)], {dropNewBelowWindow: true})
+ expect(state.messageOrdinals).toEqual([T.Chat.numberToOrdinal(1)])
+ })
+
+ test('the floor is still bound while no load has landed to say otherwise', () => {
+ // moreToLoadBack is initialized false and only a thread load ever sets it. Pushes reach the
+ // window before the first load answers, so a second push older than the one they installed
+ // would read that false as "the window reaches the oldest message" and prepend. The load then
+ // fills the region between, and the pushed row is left stranded over the hole.
+ const state = makeThreadState([textAt(7153)], {loaded: false, moreToLoadBack: false})
+ addMessagesToThreadState(state, [textAt(1)], {dropNewBelowWindow: true})
+ expect(state.messageOrdinals).toEqual([T.Chat.numberToOrdinal(7153)])
+
+ // The ceiling is deliberately not bound the same way. A clear whose reload never applies leaves
+ // `loaded` false with no load coming, and dropping everything newer than the window would then
+ // be permanent - the thread would stop receiving messages for good.
+ addMessagesToThreadState(state, [textAt(9001)], {dropNewBelowWindow: true})
+ expect(state.messageOrdinals).toEqual([T.Chat.numberToOrdinal(7153), T.Chat.numberToOrdinal(9001)])
+ })
+
+ test('a reconciling pass leaves a send that is still in the outbox alone', () => {
+ // The span covers both passes now, so it reaches above the window the first one carried: send a
+ // message between the passes and its fractional ordinal falls inside a span that neither pass
+ // could have carried. The service has never been told about that row, so its absence says
+ // nothing - deleting it takes the row out from under a send in flight.
+ const state = makeThreadState([textAt(6900), textAt(7000)])
+ const carried = new Set()
+ addMessagesToThreadState(state, [textAt(6900), textAt(7000)], {reconcile: {carried, prune: false}})
+
+ const pending = makeTextMessage({
+ id: T.Chat.numberToMessageID(0),
+ ordinal: T.Chat.numberToOrdinal(7000.001),
+ outboxID: T.Chat.stringToOutboxID('sending-1'),
+ submitState: 'pending',
+ })
+ addMessagesToThreadState(state, [pending], {})
+ expect(state.messageOrdinals).toEqual([6900, 7000, 7000.001])
+
+ addMessagesToThreadState(state, [textAt(7001)], {reconcile: {carried, prune: true}})
+ expect(state.messageOrdinals).toEqual([6900, 7000, 7000.001, 7001])
+ })
+
+ test('a message remapped out of the window is dropped, not stranded', () => {
+ // The window is judged on the ordinal the message will occupy, which an outbox or messageID
+ // match can move. Here messageIDToOrdinal still points at an ancient ordinal the thread no
+ // longer renders, so a push whose own ordinal sits inside the window remaps outside it.
+ const state = makeThreadState([textAt(7152), textAt(7153)], {moreToLoadForward: true})
+ const strandedOrdinal = T.Chat.numberToOrdinal(1)
+ const messageID = T.Chat.numberToMessageID(7153)
+ // An entry the thread no longer renders: gone from the ordinal list, still in the map and index.
+ state.messageMap.set(strandedOrdinal, T.castDraft(textAt(1, {id: messageID})))
+ state.messageIDToOrdinal.set(messageID, strandedOrdinal)
+
+ addMessagesToThreadState(state, [textAt(7153)], {dropNewBelowWindow: true})
+
+ expect(state.messageOrdinals).toEqual([T.Chat.numberToOrdinal(7152), T.Chat.numberToOrdinal(7153)])
+ })
+
+ test('a message remapped onto a row already in the window still merges', () => {
+ // The mirror of the case above: the raw ordinal is outside the window but the row it maps onto
+ // is one the thread is already showing, so this is an update to that row, not a new one.
+ const sent = textAt(7153, {outboxID: T.Chat.stringToOutboxID('sent-1')})
+ const state = makeThreadState([textAt(7152), sent], {moreToLoadForward: true})
+ const resent = textAt(1, {outboxID: T.Chat.stringToOutboxID('sent-1'), text: 'edited'})
+
+ addMessagesToThreadState(state, [resent], {dropNewBelowWindow: true})
+
+ expect(state.messageOrdinals).toEqual([T.Chat.numberToOrdinal(7152), T.Chat.numberToOrdinal(7153)])
+ const merged = state.messageMap.get(T.Chat.numberToOrdinal(7153))
+ expect(merged?.type === 'text' && merged.text.stringValue()).toBe('edited')
+ })
})
diff --git a/shared/chat/conversation/thread-message-state.tsx b/shared/chat/conversation/thread-message-state.tsx
index dd81ff9cb2cb..a2c575bb253b 100644
--- a/shared/chat/conversation/thread-message-state.tsx
+++ b/shared/chat/conversation/thread-message-state.tsx
@@ -5,13 +5,40 @@ import type {WritableDraft} from '@/util/zustand'
type MessageLookup = Pick
+// How a thread load reconciles the window against what the service returned.
+//
+// A load answers in passes - a cached one off the local database, then a full one the service has
+// filtered down to what changed - and only the passes together are a whole window. So each pass
+// adds what it delivered to `carried`, and the last one prunes: rows inside the span of `carried`
+// that no pass delivered are stale, and go.
+//
+// The set is filled in here rather than by the caller, and that is the point of it: it holds the
+// ordinals the messages actually occupy, after the outbox and message-ID remaps below. A message
+// you sent keeps the fractional ordinal it had in the outbox, so the ordinal it arrives under is
+// not the one it lives at, and a set built from the response would leave that row unprotected.
+export type ThreadLoadReconcile = {
+ carried: Set
+ prune: boolean
+}
+
type WritableConversationThreadMessageState = {
messageIDToOrdinal: Map
messageMap: Map>
messageOrdinals?: ReadonlyArray
+ // Set by messagesClear, cleared once the reload that refills the window settles. While it is set
+ // there is no window to place an arriving message against. See the drop rules in
+ // addMessagesToThreadState.
+ windowCleared?: boolean
messageTypeMap: Map
+ // Set by a thread load, cleared by messagesClear: whether either flag below means anything yet.
+ loaded: boolean
+ // False once the window reaches the oldest message, which is what makes a push older than the
+ // floor a prepend rather than a stranded row.
+ moreToLoadBack: boolean
+ // False once the window reaches the newest message, which is what makes a push newer than the
+ // ceiling an append rather than a stranded row.
+ moreToLoadForward: boolean
pendingOutboxToOrdinal: Map
- validatedOrdinalRange?: {from: T.Chat.Ordinal; to: T.Chat.Ordinal}
}
type ThreadMessagesDeleteParams = {
@@ -152,9 +179,15 @@ const mergeMessage = (
export const addMessagesToThreadState = (
state: WritableConversationThreadMessageState,
messages: ReadonlyArray,
- opt: {validatedRange?: {from: T.Chat.Ordinal; to: T.Chat.Ordinal}}
+ opt: {
+ dropNewBelowWindow?: boolean
+ reconcile?: ThreadLoadReconcile
+ }
) => {
- const {validatedRange} = opt
+ const {dropNewBelowWindow, reconcile} = opt
+ // The bounds of the loaded window before this batch is merged in.
+ const ords = state.messageOrdinals
+ const windowFloor = ords?.[0]
const incomingOrdinals = new Set()
for (const m of messages) {
if (m.conversationMessage !== false && m.type !== 'deleted') {
@@ -179,10 +212,67 @@ export const addMessagesToThreadState = (
return mapOrdinal
}
+ const existing = new Set(state.messageOrdinals ?? [])
+
+ // A notification (the post-load ResolveSkippedUnboxeds push, say) can carry a message from far
+ // outside the loaded window - the channel-name message at ID 1 is the usual one. Adding it
+ // strands a row against a hole, and the list then pages against that row instead of the real
+ // edge of the thread, so scrolling that way stops working. Only a thread load may extend the
+ // window; a push may land inside it, or extend an edge that is already the end of the thread.
+ //
+ // Both edges matter. A centered jump - a search result - leaves a contiguous window with more to
+ // load above and below it, and the reader can page either way from there, so a push newer than
+ // the ceiling strands exactly as one older than the floor does. Each edge is only a bound while
+ // there is still more to load past it: once the window reaches the end of the thread on that
+ // side there is no hole to open, and the message must simply join the window. A fully paged-back
+ // thread is the case that matters below - the ResolveSkippedUnboxeds push carrying the real
+ // message 1 has nowhere else to come from, and paging cannot fetch it again.
+ //
+ // Decided before anything is written for the message, so it is skipped whole. Dropping only the
+ // ordinal later would leave messageMap and messageIDToOrdinal holding a message the thread does
+ // not render, and getOrdinalForMessageID would then hand out an ordinal with no row. Nothing is
+ // lost either way: paging to it loads it in the ordinary way.
+ const windowCeiling = ords?.[ords.length - 1]
+ const isOutsideWindow = (o: T.Chat.Ordinal) => {
+ if (!dropNewBelowWindow || existing.has(o)) {
+ return false
+ }
+ // A clear is always followed by a reload that replaces the window wholesale, so until that
+ // lands there is nothing to place an arriving message against: a centered jump reloads an
+ // arbitrary region, and jump-to-recent the newest page, which is disjoint from wherever the
+ // reader was. A message landing in the gap that the reload does not carry waits for the next
+ // load or push; a stranded ordinal, by contrast, breaks paging for the life of the thread.
+ if (state.windowCleared) {
+ return true
+ }
+ // moreToLoadBack starts false and only a thread load ever sets it, so until one has landed a
+ // false reads as "the window reaches the oldest message" when it only means "nothing has said
+ // yet" - and pushes do reach the window before the first load answers. An older push admitted
+ // on that reading lands under a floor the load is about to fill, stranded over the hole.
+ //
+ // Only this edge. The same reasoning would wedge the other one: after a clear whose reload
+ // never applies there is no load coming at all, and bounding the ceiling on a flag that can no
+ // longer change would drop every incoming message for the life of the thread. Below is the
+ // edge with a backstop - the load that fills the hole is what makes the drop temporary, and
+ // paging back reaches those messages again in the ordinary way.
+ const below =
+ windowFloor !== undefined && o < windowFloor && (!state.loaded || state.moreToLoadBack)
+ const above = windowCeiling !== undefined && o > windowCeiling && state.moreToLoadForward
+ return below || above
+ }
+
const deletedOrdinals = new Set()
for (const _m of messages) {
const regularMessage = _m.conversationMessage !== false
const mapOrdinal = getMapOrdinal(_m, regularMessage)
+ // Judged on mapOrdinal, the ordinal the message will actually occupy: an outbox or messageID
+ // match can move it out of the window, or onto a row already inside it. Deletions and
+ // non-conversation messages are not rows, so the window does not bound them.
+ if (regularMessage && _m.type !== 'deleted' && isOutsideWindow(mapOrdinal)) {
+ incomingOrdinals.delete(_m.ordinal)
+ incomingOrdinals.delete(mapOrdinal)
+ continue
+ }
const getIncomingMessage = (): WritableDraft =>
messageForThreadState(_m, mapOrdinal)
@@ -195,6 +285,16 @@ export const addMessagesToThreadState = (
if (_m.type === 'placeholder') {
const old = state.messageMap.get(mapOrdinal)
if (old && old.type !== 'placeholder') {
+ // The real message already sits under mapOrdinal, which is not always _m.ordinal: a sent
+ // message keeps the fractional ordinal it had in the outbox. Bailing out before the remap
+ // below would strand _m.ordinal in the list with nothing stored under it.
+ //
+ // Do the remap anyway rather than just forgetting _m.ordinal. `incomingOrdinals` is what
+ // the prune treats as "still present", so an ordinal missing from it inside the span
+ // gets the real message deleted - including when mapOrdinal and _m.ordinal are the same,
+ // where the delete below would otherwise be a plain loss.
+ incomingOrdinals.delete(_m.ordinal)
+ incomingOrdinals.add(mapOrdinal)
continue
}
}
@@ -240,7 +340,6 @@ export const addMessagesToThreadState = (
}
}
- const existing = new Set(state.messageOrdinals ?? [])
let changed = false
for (const o of incomingOrdinals) {
if (!existing.has(o)) {
@@ -254,24 +353,33 @@ export const addMessagesToThreadState = (
changed = true
}
}
- if (validatedRange) {
- // The service response is authoritative within this range; prune stale local ordinals.
- for (const o of existing) {
- if (o >= validatedRange.from && o <= validatedRange.to && !incomingOrdinals.has(o)) {
- clearMessageIDIndexForOrdinal(state, o)
- existing.delete(o)
- state.messageMap.delete(o)
- state.messageTypeMap.delete(o)
- changed = true
- }
+ if (reconcile) {
+ for (const o of incomingOrdinals) {
+ reconcile.carried.add(o)
}
- const prev = state.validatedOrdinalRange
- state.validatedOrdinalRange = prev
- ? {
- from: Math.min(prev.from, validatedRange.from) as T.Chat.Ordinal,
- to: Math.max(prev.to, validatedRange.to) as T.Chat.Ordinal,
+ if (reconcile.prune) {
+ // The load is authoritative over the span it covered, so a row inside it that no pass of the
+ // load delivered is stale. Outside the span nothing is known and nothing is touched.
+ let from = Number.MAX_SAFE_INTEGER as T.Chat.Ordinal
+ let to = Number.MIN_SAFE_INTEGER as T.Chat.Ordinal
+ for (const o of reconcile.carried) {
+ from = Math.min(from, o) as T.Chat.Ordinal
+ to = Math.max(to, o) as T.Chat.Ordinal
+ }
+ for (const o of existing) {
+ // A row with no server ID is one of ours, still in the outbox: the service cannot have
+ // failed to return what it has never been told about. It sits on a fractional ordinal just
+ // above the message it was composed after, so the span reaches it as soon as anything
+ // newer arrives - and deleting it takes the row out from under a send in flight.
+ if (o >= from && o <= to && !reconcile.carried.has(o) && state.messageMap.get(o)?.id) {
+ clearMessageIDIndexForOrdinal(state, o)
+ existing.delete(o)
+ state.messageMap.delete(o)
+ state.messageTypeMap.delete(o)
+ changed = true
}
- : validatedRange
+ }
+ }
}
if (changed || !state.messageOrdinals) {
state.messageOrdinals = [...existing].sort((a, b) => a - b)
diff --git a/shared/tests/e2e/electron/helpers/connect.ts b/shared/tests/e2e/electron/helpers/connect.ts
index a0225f35d3f8..02f9f23bebf1 100644
--- a/shared/tests/e2e/electron/helpers/connect.ts
+++ b/shared/tests/e2e/electron/helpers/connect.ts
@@ -19,7 +19,7 @@ export async function connectToElectron(): Promise<{browser: Browser; page: Page
// KB_E2E_TEST=1 suppresses the menubar widget and devtools windows, so
// pages()[0] is always the main app. Keep the URL check as a safety net.
const allPages = browser.contexts().flatMap(ctx => ctx.pages())
- const mainPage = allPages.find(p => p.url().includes('main.dev.html')) ?? allPages[0]
+ const mainPage = allPages.find(p => p.url().includes('main.html')) ?? allPages[0]
if (!mainPage) {
throw new Error('Could not find main app page. Is the app running with KB_ENABLE_REMOTE_DEBUG=1 KB_E2E_TEST=1?')
diff --git a/skill/playwright-cli/SKILL.md b/skill/playwright-cli/SKILL.md
index 0818f8d3aa03..2659ff077d07 100644
--- a/skill/playwright-cli/SKILL.md
+++ b/skill/playwright-cli/SKILL.md
@@ -283,17 +283,33 @@ PLAYWRIGHT_MCP_CDP_ENDPOINT=http://localhost:9222 playwright-cli open --persiste
### Tab layout
-After connecting, you'll see tabs like:
-- Tab 0: Menubar (`menubar.dev.html`)
-- Tab 1: Main app (`main.dev.html`) — this is the one you want
-- Tab 2+: Other windows
+Tab order is NOT stable. It changes when the app restarts, and a DevTools window can take slot 0:
+- Seen: `0: menubar, 1: main app`
+- Also seen (after an app restart with DevTools open): `0: DevTools, 1: main app, 2: menubar`
-### Identifying the main app tab
+### Identifying the main app tab — do this EVERY time
+
+Never reuse a remembered index. Re-run `tab-list` and pick the row whose URL contains `main.html`
+after every restart, reload, or reconnect, then confirm with one cheap eval before trusting anything:
+
+```bash
+playwright-cli tab-list # find the row whose URL has main.html
+playwright-cli tab-select
+playwright-cli eval "document.title + ' ' + location.href" # assert before proceeding
+```
+
+**A wrong tab fails silently.** DevTools is a real page: it answers every `eval` plausibly and its
+console is nearly empty, so readings look like findings rather than mistakes. Treat a suspiciously
+empty console or a tiny `document.body.innerText` as "wrong tab" first, not as evidence.
+
+Also note `console` only captures output from the moment playwright attached to that page. Anything
+logged before the attach lives in the user's own DevTools and cannot be retrieved through
+playwright — ask them to paste it, or have them re-trigger while you are attached.
Use the page URL, not the title — the title stays `"Keybase DEV"` until the router navigates and can't be relied on:
-- Main app: URL contains `main.dev.html`
-- Menubar: URL contains `menubar.dev.html`
+- Main app: URL contains `main.html`
+- Menubar: URL contains `remote.html?component=menubar` (the menubar is a remote window, not its own shell)
- Avoid: `devtools://` pages and the `"Keybase DEV"` standalone DevTools window
```js
@@ -301,7 +317,7 @@ Use the page URL, not the title — the title stays `"Keybase DEV"` until the ro
let mainPage
for (const ctx of browser.contexts()) {
for (const p of ctx.pages()) {
- if (p.url().includes('main.dev.html')) { mainPage = p; break }
+ if (p.url().includes('main.html')) { mainPage = p; break }
}
if (mainPage) break
}