From fab331e67e998ea5a9422ad287e938ea4d3aadcb Mon Sep 17 00:00:00 2001 From: Ryan Gregg Date: Sat, 25 Jul 2026 00:40:47 -0700 Subject: [PATCH 1/6] Virtualize the message list A conversation rendered every message it had loaded. Nothing bounded that work, so cost grew with history: at 2000 messages the list mounted ~20k DOM nodes, took ~285ms to mount, and ~212ms to re-render on any update. While that runs, nothing else on the main thread gets a turn, typing included. Only the visible window is mounted now, measured in Chromium with the real component tree: history mount re-render DOM nodes 2000 285ms -> 4ms 212ms -> 2.2ms 22038 -> 225 Rendered rows stay at ~17 regardless of how much history is loaded. Row heights are measured rather than assumed, since messages vary from one line to a wrapped paragraph with path badges, so the scroll behaviors that used to do height arithmetic now anchor by index instead, which is exact even when off-screen rows still carry estimated heights: - opening a conversation, and following new messages, anchor to the last row - loading older messages anchors to the message that was on top - jump-to-message and jump-to-unread scroll by index, since the target row may not be mounted yet Two degenerate cases are handled explicitly: a viewport that measures zero (before first layout, hidden tab) falls back to a nominal height instead of collapsing the window to nothing, and a row that measures zero keeps its estimate instead of ballooning the window. Most of the diff is re-indentation from the new wrapper element; the real change is 112 added / 36 removed lines (git diff -w). --- frontend/package-lock.json | 62 ++- frontend/package.json | 1 + frontend/src/components/MessageList.tsx | 632 +++++++++++++----------- frontend/src/test/messageList.test.tsx | 18 + 4 files changed, 403 insertions(+), 310 deletions(-) diff --git a/frontend/package-lock.json b/frontend/package-lock.json index c5bd629..7c4c231 100644 --- a/frontend/package-lock.json +++ b/frontend/package-lock.json @@ -1,12 +1,12 @@ { "name": "remoteterm-meshcore-frontend", - "version": "3.16.0", + "version": "3.17.0", "lockfileVersion": 3, "requires": true, "packages": { "": { "name": "remoteterm-meshcore-frontend", - "version": "3.16.0", + "version": "3.17.0", "dependencies": { "@codemirror/lang-python": "^6.2.1", "@codemirror/theme-one-dark": "^6.1.3", @@ -17,6 +17,7 @@ "@radix-ui/react-separator": "^1.1.8", "@radix-ui/react-slot": "^1.2.4", "@radix-ui/react-tabs": "^1.1.13", + "@tanstack/react-virtual": "^3.14.8", "@uiw/react-codemirror": "^4.25.4", "class-variance-authority": "^0.7.1", "clsx": "^2.1.1", @@ -1543,9 +1544,6 @@ "arm64" ], "dev": true, - "libc": [ - "glibc" - ], "license": "MIT", "optional": true, "os": [ @@ -1563,9 +1561,6 @@ "arm64" ], "dev": true, - "libc": [ - "musl" - ], "license": "MIT", "optional": true, "os": [ @@ -1583,9 +1578,6 @@ "ppc64" ], "dev": true, - "libc": [ - "glibc" - ], "license": "MIT", "optional": true, "os": [ @@ -1603,9 +1595,6 @@ "s390x" ], "dev": true, - "libc": [ - "glibc" - ], "license": "MIT", "optional": true, "os": [ @@ -1623,9 +1612,6 @@ "x64" ], "dev": true, - "libc": [ - "glibc" - ], "license": "MIT", "optional": true, "os": [ @@ -1643,9 +1629,6 @@ "x64" ], "dev": true, - "libc": [ - "musl" - ], "license": "MIT", "optional": true, "os": [ @@ -1744,6 +1727,33 @@ "integrity": "sha512-e7Mew686owMaPJVNNLs55PUvgz371nKgwsc4vxE49zsODpJEnxgxRo2y/OKrqueavXgZNMDVj3DdHFlaSAeU8g==", "license": "MIT" }, + "node_modules/@tanstack/react-virtual": { + "version": "3.14.8", + "resolved": "https://registry.npmjs.org/@tanstack/react-virtual/-/react-virtual-3.14.8.tgz", + "integrity": "sha512-O39GJQpAYEJcIu3uN1//YtmhjSEOyw75vg9CKCatBDPiD5hKtZQoJHfferyrB/LdOD3UWaoMLWtdEjarwIwdDw==", + "license": "MIT", + "dependencies": { + "@tanstack/virtual-core": "3.17.6" + }, + "funding": { + "type": "github", + "url": "https://github.com/sponsors/tannerlinsley" + }, + "peerDependencies": { + "react": "^16.8.0 || ^17.0.0 || ^18.0.0 || ^19.0.0", + "react-dom": "^16.8.0 || ^17.0.0 || ^18.0.0 || ^19.0.0" + } + }, + "node_modules/@tanstack/virtual-core": { + "version": "3.17.6", + "resolved": "https://registry.npmjs.org/@tanstack/virtual-core/-/virtual-core-3.17.6.tgz", + "integrity": "sha512-h0/Ebo18CkOrChlQIhNtQkM5ySUnh/GumQ/D1st3hG2HWUPEF+ILUc2k29UtivCi/9G7w7G3/f7Xyd5cCFbKBw==", + "license": "MIT", + "funding": { + "type": "github", + "url": "https://github.com/sponsors/tannerlinsley" + } + }, "node_modules/@testing-library/dom": { "version": "10.4.1", "resolved": "https://registry.npmjs.org/@testing-library/dom/-/dom-10.4.1.tgz", @@ -4504,9 +4514,6 @@ "arm64" ], "dev": true, - "libc": [ - "glibc" - ], "license": "MPL-2.0", "optional": true, "os": [ @@ -4528,9 +4535,6 @@ "arm64" ], "dev": true, - "libc": [ - "musl" - ], "license": "MPL-2.0", "optional": true, "os": [ @@ -4552,9 +4556,6 @@ "x64" ], "dev": true, - "libc": [ - "glibc" - ], "license": "MPL-2.0", "optional": true, "os": [ @@ -4576,9 +4577,6 @@ "x64" ], "dev": true, - "libc": [ - "musl" - ], "license": "MPL-2.0", "optional": true, "os": [ diff --git a/frontend/package.json b/frontend/package.json index 777492f..fa07c99 100644 --- a/frontend/package.json +++ b/frontend/package.json @@ -25,6 +25,7 @@ "@radix-ui/react-separator": "^1.1.8", "@radix-ui/react-slot": "^1.2.4", "@radix-ui/react-tabs": "^1.1.13", + "@tanstack/react-virtual": "^3.14.8", "@uiw/react-codemirror": "^4.25.4", "class-variance-authority": "^0.7.1", "clsx": "^2.1.1", diff --git a/frontend/src/components/MessageList.tsx b/frontend/src/components/MessageList.tsx index 4b2c63d..415777f 100644 --- a/frontend/src/components/MessageList.tsx +++ b/frontend/src/components/MessageList.tsx @@ -1,5 +1,4 @@ import { - Fragment, useEffect, useLayoutEffect, useRef, @@ -31,6 +30,7 @@ import { PathModal } from './PathModal'; import { RawPacketInspectorDialog } from './RawPacketDetailModal'; import { toast } from './ui/sonner'; import { handleKeyboardActivate } from '../utils/a11y'; +import { useVirtualizer } from '@tanstack/react-virtual'; import { cn } from '@/lib/utils'; interface MessageListProps { @@ -138,6 +138,15 @@ function renderMeshcoreOpenPayload( return null; } +/** + * Starting guess for an unmeasured row: a single-line message with its header. + * Rows are measured for real once they scroll into view. + */ +const ESTIMATED_MESSAGE_HEIGHT = 64; + +/** Stand-in viewport height for when the scroll container cannot be measured. */ +const FALLBACK_VIEWPORT_HEIGHT = 800; + // URL regex for linkifying plain text const URL_PATTERN = /https?:\/\/(www\.)?[-a-zA-Z0-9@:%._+~#=]{1,256}\.[a-zA-Z0-9()]{1,6}\b([-a-zA-Z0-9()@:%_+.~#?&//=]*)/g; @@ -474,11 +483,65 @@ export function MessageList({ } }, []); + // Sort messages by received_at ascending (oldest first) + // Note: Deduplication is handled by useConversationMessages.observeMessage() + // and the database UNIQUE constraint on (type, conversation_key, text, sender_timestamp) + const sortedMessages = useMemo( + () => + preSorted + ? messages + : [...messages].sort((a, b) => a.received_at - b.received_at || a.id - b.id), + [messages, preSorted] + ); + /** + * Only the visible window of messages is mounted. A long channel history otherwise + * costs a full render of every message on any update — hundreds of milliseconds once + * a conversation has a few thousand messages, which stalls everything else on the + * main thread, typing included. + * + * Heights are measured, not assumed: messages vary wildly (one line, a wrapped + * paragraph, path badges, the unread divider), so `estimateSize` is only the starting + * guess for rows that have not been on screen yet. + */ + const virtualizer = useVirtualizer({ + count: sortedMessages.length, + getScrollElement: () => listRef.current, + estimateSize: () => ESTIMATED_MESSAGE_HEIGHT, + getItemKey: (index) => sortedMessages[index]?.id ?? index, + overscan: 8, + // A row that measures zero has not really been laid out yet (hidden pane, images + // still loading). Keep the estimate instead, or the window balloons to compensate. + measureElement: (element) => element.getBoundingClientRect().height || ESTIMATED_MESSAGE_HEIGHT, + // A viewport that measures zero (before first layout, a hidden tab, jsdom) would + // otherwise collapse the window to nothing and render an empty list. Fall back to a + // nominal height so we always mount a plausible screenful. + observeElementRect: (instance, cb) => { + const element = instance.scrollElement; + if (!element) return; + const report = () => { + const rect = element.getBoundingClientRect(); + cb({ width: rect.width, height: rect.height || FALLBACK_VIEWPORT_HEIGHT }); + }; + report(); + const observer = new ResizeObserver(report); + observer.observe(element); + return () => observer.disconnect(); + }, + }); + const virtualRows = virtualizer.getVirtualItems(); + + const scrollToIndex = useCallback( + (index: number, align: 'start' | 'center' | 'end') => { + if (index < 0) return; + virtualizer.scrollToIndex(index, { align }); + }, + [virtualizer] + ); + // Handle scroll position AFTER render useLayoutEffect(() => { if (!listRef.current) return; - const list = listRef.current; const messagesAdded = messages.length - prevMessagesLengthRef.current; // Detect if messages are from a different conversation (handles the case where @@ -489,40 +552,40 @@ export function MessageList({ if (convKey !== null) prevConvKeyRef.current = convKey; if ((isInitialLoadRef.current || conversationChanged) && messages.length > 0) { - // Initial load or conversation switch - scroll to bottom - list.scrollTop = list.scrollHeight; + // Initial load or conversation switch - scroll to bottom. + // Anchored to the last row rather than scrollHeight: rows that have never been + // on screen still carry estimated heights, so the total is not yet exact. + scrollToIndex(sortedMessages.length - 1, 'end'); isInitialLoadRef.current = false; } else if (messagesAdded > 0 && prevMessagesLengthRef.current > 0) { - // Messages were added - use scroll state captured before the update - const scrollHeightDiff = list.scrollHeight - scrollStateRef.current.scrollHeight; - - if (scrollStateRef.current.wasNearTop && scrollHeightDiff > 0) { - // User was near top (loading older) - preserve position by adding the height diff - list.scrollTop = scrollStateRef.current.scrollTop + scrollHeightDiff; + if (scrollStateRef.current.wasNearTop) { + // User was near top (loading older) - keep the message that was on top in place. + // Prepended rows are unmeasured, so anchoring by index beats height arithmetic. + scrollToIndex(messagesAdded, 'start'); } else if (scrollStateRef.current.wasNearBottom && !hasNewerMessagesRef.current) { // User was near bottom - scroll to bottom for new messages (including sent). // Skip when browsing mid-history (hasNewerMessages) so that forward-pagination // appends in place instead of chasing the bottom in an infinite load loop. - list.scrollTop = list.scrollHeight; + scrollToIndex(sortedMessages.length - 1, 'end'); } } prevMessagesLengthRef.current = messages.length; - }, [messages]); + }, [messages, sortedMessages.length, scrollToIndex]); // Scroll to target message and highlight it useLayoutEffect(() => { if (!targetMessageId || targetScrolledRef.current || messages.length === 0) return; - const el = listRef.current?.querySelector(`[data-message-id="${targetMessageId}"]`); - if (!el) return; + const targetIndex = sortedMessages.findIndex((msg) => msg.id === targetMessageId); + if (targetIndex === -1) return; // Prevent the initial-load layout effect from overriding our scroll isInitialLoadRef.current = false; - el.scrollIntoView({ block: 'center' }); + scrollToIndex(targetIndex, 'center'); setHighlightedMessageId(targetMessageId); targetScrolledRef.current = true; onTargetReached?.(); - }, [messages, targetMessageId, onTargetReached]); + }, [messages, sortedMessages, targetMessageId, onTargetReached, scrollToIndex]); // Reset target scroll tracking when targetMessageId changes useEffect(() => { @@ -597,16 +660,6 @@ export function MessageList({ }; }, [messages, onResendChannelMessage]); - // Sort messages by received_at ascending (oldest first) - // Note: Deduplication is handled by useConversationMessages.observeMessage() - // and the database UNIQUE constraint on (type, conversation_key, text, sender_timestamp) - const sortedMessages = useMemo( - () => - preSorted - ? messages - : [...messages].sort((a, b) => a.received_at - b.received_at || a.id - b.id), - [messages, preSorted] - ); const unreadMarkerIndex = useMemo(() => { if (unreadMarkerLastReadAt === undefined) { return -1; @@ -722,10 +775,8 @@ export function MessageList({ onJumpToBottom(); return; } - if (listRef.current) { - listRef.current.scrollTop = listRef.current.scrollHeight; - } - }, [hasNewerMessages, onJumpToBottom]); + scrollToIndex(sortedMessages.length - 1, 'end'); + }, [hasNewerMessages, onJumpToBottom, scrollToIndex, sortedMessages.length]); // Sender info for outgoing messages (used by path modal on own messages) const selfSenderInfo = useMemo( @@ -894,7 +945,7 @@ export function MessageList({ return (
@@ -908,231 +959,215 @@ export function MessageList({ Scroll up for older messages
)} - {sortedMessages.map((msg, index) => { - // For DMs, look up contact; for channel messages, use parsed sender - const contact = msg.type === 'PRIV' ? getContact(msg.conversation_key) : null; - const isRoomServer = contact?.type === CONTACT_TYPE_ROOM; +
+ {virtualRows.map((virtualRow) => { + const index = virtualRow.index; + const msg = sortedMessages[index]; + // For DMs, look up contact; for channel messages, use parsed sender + const contact = msg.type === 'PRIV' ? getContact(msg.conversation_key) : null; + const isRoomServer = contact?.type === CONTACT_TYPE_ROOM; - // Only parse "sender: text" prefix for channel messages — DMs never carry - // an in-text sender prefix, so parsing them would incorrectly strip - // user text that happens to contain a colon (e.g. "TEST1: TEST2"). - const { sender, content } = - msg.type === 'PRIV' - ? { sender: null, content: msg.text } - : parseSenderFromText(msg.text); - const directSenderName = - msg.type === 'PRIV' && isRoomServer ? msg.sender_name || null : null; - const channelSenderName = msg.type === 'CHAN' ? msg.sender_name || sender : null; - const channelSenderContact = - msg.type === 'CHAN' && channelSenderName ? getContactByName(channelSenderName) : null; - const isCorruptChannelMessage = isCorruptUnnamedChannelMessage(msg, sender); - const displaySender = msg.outgoing - ? 'You' - : directSenderName || - (isRoomServer && msg.sender_key ? msg.sender_key.slice(0, 8) : null) || - contact?.name || - channelSenderName || - (isCorruptChannelMessage - ? CORRUPT_SENDER_LABEL - : msg.conversation_key?.slice(0, 8) || 'Unknown'); + // Only parse "sender: text" prefix for channel messages — DMs never carry + // an in-text sender prefix, so parsing them would incorrectly strip + // user text that happens to contain a colon (e.g. "TEST1: TEST2"). + const { sender, content } = + msg.type === 'PRIV' + ? { sender: null, content: msg.text } + : parseSenderFromText(msg.text); + const directSenderName = + msg.type === 'PRIV' && isRoomServer ? msg.sender_name || null : null; + const channelSenderName = msg.type === 'CHAN' ? msg.sender_name || sender : null; + const channelSenderContact = + msg.type === 'CHAN' && channelSenderName ? getContactByName(channelSenderName) : null; + const isCorruptChannelMessage = isCorruptUnnamedChannelMessage(msg, sender); + const displaySender = msg.outgoing + ? 'You' + : directSenderName || + (isRoomServer && msg.sender_key ? msg.sender_key.slice(0, 8) : null) || + contact?.name || + channelSenderName || + (isCorruptChannelMessage + ? CORRUPT_SENDER_LABEL + : msg.conversation_key?.slice(0, 8) || 'Unknown'); - const canClickSender = - !msg.outgoing && - onSenderClick && - displaySender !== 'Unknown' && - displaySender !== CORRUPT_SENDER_LABEL; + const canClickSender = + !msg.outgoing && + onSenderClick && + displaySender !== 'Unknown' && + displaySender !== CORRUPT_SENDER_LABEL; - // Determine if we should show avatar (first message in a chunk from same sender) - const currentSenderKey = getSenderKey( - msg, - directSenderName || channelSenderName, - isCorruptChannelMessage - ); - const prevMsg = sortedMessages[index - 1]; - const prevParsedSender = - prevMsg && prevMsg.type === 'CHAN' ? parseSenderFromText(prevMsg.text).sender : null; - const prevSenderKey = prevMsg - ? getSenderKey( - prevMsg, - prevMsg.type === 'PRIV' && - getContact(prevMsg.conversation_key)?.type === CONTACT_TYPE_ROOM - ? prevMsg.sender_name - : prevMsg.type === 'CHAN' - ? prevMsg.sender_name || prevParsedSender - : prevParsedSender, - isCorruptUnnamedChannelMessage(prevMsg, prevParsedSender) - ) - : null; - const isFirstInGroup = currentSenderKey !== prevSenderKey; - const showAvatar = !msg.outgoing && isFirstInGroup; - const isFirstMessage = index === 0; + // Determine if we should show avatar (first message in a chunk from same sender) + const currentSenderKey = getSenderKey( + msg, + directSenderName || channelSenderName, + isCorruptChannelMessage + ); + const prevMsg = sortedMessages[index - 1]; + const prevParsedSender = + prevMsg && prevMsg.type === 'CHAN' ? parseSenderFromText(prevMsg.text).sender : null; + const prevSenderKey = prevMsg + ? getSenderKey( + prevMsg, + prevMsg.type === 'PRIV' && + getContact(prevMsg.conversation_key)?.type === CONTACT_TYPE_ROOM + ? prevMsg.sender_name + : prevMsg.type === 'CHAN' + ? prevMsg.sender_name || prevParsedSender + : prevParsedSender, + isCorruptUnnamedChannelMessage(prevMsg, prevParsedSender) + ) + : null; + const isFirstInGroup = currentSenderKey !== prevSenderKey; + const showAvatar = !msg.outgoing && isFirstInGroup; + const isFirstMessage = index === 0; - // Get avatar info for incoming messages - let avatarName: string | null = null; - let avatarKey: string = ''; - let avatarVariant: 'default' | 'corrupt' = 'default'; - if (!msg.outgoing) { - if (msg.type === 'PRIV' && msg.conversation_key) { - if (isRoomServer) { - avatarName = directSenderName; - avatarKey = - msg.sender_key || (avatarName ? `name:${avatarName}` : msg.conversation_key); + // Get avatar info for incoming messages + let avatarName: string | null = null; + let avatarKey: string = ''; + let avatarVariant: 'default' | 'corrupt' = 'default'; + if (!msg.outgoing) { + if (msg.type === 'PRIV' && msg.conversation_key) { + if (isRoomServer) { + avatarName = directSenderName; + avatarKey = + msg.sender_key || (avatarName ? `name:${avatarName}` : msg.conversation_key); + } else { + avatarName = contact?.name || null; + avatarKey = msg.conversation_key; + } + } else if (isCorruptChannelMessage) { + avatarName = CORRUPT_SENDER_LABEL; + avatarKey = `corrupt:${msg.id}`; + avatarVariant = 'corrupt'; } else { - avatarName = contact?.name || null; - avatarKey = msg.conversation_key; + // Channel message: use stored sender identity first, then parsed/fallback display name + avatarName = + channelSenderName || (displaySender !== 'Unknown' ? displaySender : null); + avatarKey = + msg.sender_key || + channelSenderContact?.public_key || + (avatarName ? `name:${avatarName}` : `message:${msg.id}`); } - } else if (isCorruptChannelMessage) { - avatarName = CORRUPT_SENDER_LABEL; - avatarKey = `corrupt:${msg.id}`; - avatarVariant = 'corrupt'; - } else { - // Channel message: use stored sender identity first, then parsed/fallback display name - avatarName = - channelSenderName || (displaySender !== 'Unknown' ? displaySender : null); - avatarKey = - msg.sender_key || - channelSenderContact?.public_key || - (avatarName ? `name:${avatarName}` : `message:${msg.id}`); } - } - const avatarActionLabel = - avatarName && avatarName !== 'Unknown' - ? `View info for ${avatarName}` - : `View info for ${avatarKey.slice(0, 12)}`; + const avatarActionLabel = + avatarName && avatarName !== 'Unknown' + ? `View info for ${avatarName}` + : `View info for ${avatarKey.slice(0, 12)}`; - return ( - - {unreadMarkerIndex === index && - (onDismissUnreadMarker ? ( - - ) : ( -
- - - Unread messages - - -
- ))} + return ( + // Absolutely positioned so the scroll container keeps a stable total height + // while only the visible window is mounted. `flex flex-col` matters: it makes + // child margins (group spacing, the unread divider) part of the measured height.
- {!msg.outgoing && ( -
- {showAvatar && - avatarKey && - (onOpenContactInfo ? ( - - ) : ( - - - - ))} -
- )} + {unreadMarkerIndex === index && + (onDismissUnreadMarker ? ( + + ) : ( +
+ + + Unread messages + + +
+ ))}
- {showAvatar && ( -
- {canClickSender ? ( - onSenderClick(displaySender)} - title={`Mention ${displaySender}`} - > - {displaySender} - - ) : ( - displaySender - )} - - {formatTime(msg.received_at)} - - {!msg.outgoing && msg.paths && msg.paths.length > 0 && ( - - setSelectedPath({ - paths: msg.paths!, - senderInfo: getSenderInfo(msg, contact, directSenderName || sender), - messageId: msg.id, - packetId: msg.packet_id, - }) - } - /> - )} - {msg.region && } + {!msg.outgoing && ( +
+ {showAvatar && + avatarKey && + (onOpenContactInfo ? ( + + ) : ( + + + + ))}
)} -
- {(renderRichPayloads && - renderMeshcoreOpenPayload(content, radioName, onChannelReferenceClick)) || - content.split('\n').map((line, i, arr) => ( - - {renderTextWithMentions(line, radioName, onChannelReferenceClick)} - {i < arr.length - 1 &&
} - - ))} - {!showAvatar && ( - <> - +
+ {showAvatar && ( +
+ {canClickSender ? ( + onSenderClick(displaySender)} + title={`Mention ${displaySender}`} + > + {displaySender} + + ) : ( + displaySender + )} + {formatTime(msg.received_at)} {!msg.outgoing && msg.paths && msg.paths.length > 0 && ( setSelectedPath({ paths: msg.paths!, @@ -1144,11 +1179,68 @@ export function MessageList({ /> )} {msg.region && } - +
)} - {msg.outgoing && - (msg.acked > 0 ? ( - msg.paths && msg.paths.length > 0 ? ( +
+ {(renderRichPayloads && + renderMeshcoreOpenPayload(content, radioName, onChannelReferenceClick)) || + content.split('\n').map((line, i, arr) => ( + + {renderTextWithMentions(line, radioName, onChannelReferenceClick)} + {i < arr.length - 1 &&
} + + ))} + {!showAvatar && ( + <> + + {formatTime(msg.received_at)} + + {!msg.outgoing && msg.paths && msg.paths.length > 0 && ( + + setSelectedPath({ + paths: msg.paths!, + senderInfo: getSenderInfo( + msg, + contact, + directSenderName || sender + ), + messageId: msg.id, + packetId: msg.packet_id, + }) + } + /> + )} + {msg.region && } + + )} + {msg.outgoing && + (msg.acked > 0 ? ( + msg.paths && msg.paths.length > 0 ? ( + { + e.stopPropagation(); + setSelectedPath({ + paths: msg.paths!, + senderInfo: selfSenderInfo, + messageId: msg.id, + packetId: msg.packet_id, + isOutgoingChan: msg.type === 'CHAN' && !!onResendChannelMessage, + }); + }} + title="View echo paths" + aria-label={`Acknowledged, ${msg.acked} echo${msg.acked !== 1 ? 's' : ''} — view paths`} + >{` ✓${msg.acked > 1 ? msg.acked : ''}`} + ) : ( + {` ✓${msg.acked > 1 ? msg.acked : ''}`} + ) + ) : onResendChannelMessage && msg.type === 'CHAN' ? ( { e.stopPropagation(); setSelectedPath({ - paths: msg.paths!, + paths: [], senderInfo: selfSenderInfo, messageId: msg.id, packetId: msg.packet_id, - isOutgoingChan: msg.type === 'CHAN' && !!onResendChannelMessage, + isOutgoingChan: true, }); }} - title="View echo paths" - aria-label={`Acknowledged, ${msg.acked} echo${msg.acked !== 1 ? 's' : ''} — view paths`} - >{` ✓${msg.acked > 1 ? msg.acked : ''}`} + title="Message status" + aria-label="No echoes yet — view message status" + > + {' '} + ? + ) : ( - {` ✓${msg.acked > 1 ? msg.acked : ''}`} - ) - ) : onResendChannelMessage && msg.type === 'CHAN' ? ( - { - e.stopPropagation(); - setSelectedPath({ - paths: [], - senderInfo: selfSenderInfo, - messageId: msg.id, - packetId: msg.packet_id, - isOutgoingChan: true, - }); - }} - title="Message status" - aria-label="No echoes yet — view message status" - > - {' '} - ? - - ) : ( - - {' '} - ? - - ))} + + {' '} + ? + + ))} +
- - ); - })} + ); + })} +
{loadingNewer && (
Loading newer messages... @@ -1223,7 +1294,12 @@ export function MessageList({
)}
@@ -1146,7 +1185,10 @@ export function MessageList({ data-index={index} ref={virtualizer.measureElement} className="absolute left-0 top-0 flex w-full flex-col pb-0.5" - style={{ transform: `translateY(${virtualRow.start}px)` }} + // start is measured from the scroll container's origin, which + // scrollMargin accounts for; the spacer already sits that far + // down, so subtract it back out when positioning within it. + style={{ transform: `translateY(${virtualRow.start - scrollMargin}px)` }} > {unreadMarkerIndex === index && (onDismissUnreadMarker ? ( diff --git a/frontend/src/hooks/useUnreadCounts.ts b/frontend/src/hooks/useUnreadCounts.ts index fafd655..df1d46d 100644 --- a/frontend/src/hooks/useUnreadCounts.ts +++ b/frontend/src/hooks/useUnreadCounts.ts @@ -164,18 +164,29 @@ export function useUnreadCounts( } }, [activeConversation]); - const incrementUnread = useCallback((stateKey: string, hasMention?: boolean) => { - setUnreadCounts((prev) => ({ - ...prev, - [stateKey]: (prev[stateKey] || 0) + 1, - })); - if (hasMention) { - setMentions((prev) => ({ + const incrementUnread = useCallback( + (stateKey: string, messageId: number, hasMention?: boolean) => { + setUnreadCounts((prev) => ({ ...prev, - [stateKey]: true, + [stateKey]: (prev[stateKey] || 0) + 1, })); - } - }, []); + // Counts move live over the socket, but first_unread_ids only arrives with a + // full /unreads fetch. Without seeding it here, a conversation that goes from + // read to unread while the app is open has a count but no boundary, and the + // divider silently never renders. Only the transition matters: once a + // boundary exists, later messages are not the *first* unread. + setFirstUnreadIds((prev) => + prev[stateKey] != null ? prev : { ...prev, [stateKey]: messageId } + ); + if (hasMention) { + setMentions((prev) => ({ + ...prev, + [stateKey]: true, + })); + } + }, + [] + ); const recordMessageEvent = useCallback( ({ @@ -205,7 +216,7 @@ export function useUnreadCounts( setLastMessageTimes(updated); if (!isActiveConversation && !msg.outgoing && isNewMessage) { - incrementUnread(stateKey, hasMention); + incrementUnread(stateKey, msg.id, hasMention); } }, [incrementUnread] @@ -230,6 +241,14 @@ export function useUnreadCounts( return next; }); + setFirstUnreadIds((prev) => { + if (!(oldStateKey in prev)) return prev; + const next = { ...prev }; + next[newStateKey] = next[newStateKey] ?? next[oldStateKey]; + delete next[oldStateKey]; + return next; + }); + setLastMessageTimes(renameConversationTimeKey(oldStateKey, newStateKey)); }, []); @@ -246,6 +265,12 @@ export function useUnreadCounts( delete next[stateKey]; return next; }); + setFirstUnreadIds((prev) => { + if (!(stateKey in prev)) return prev; + const next = { ...prev }; + delete next[stateKey]; + return next; + }); setUnreadLastReadAts((prev) => { if (!(stateKey in prev)) return prev; const next = { ...prev }; @@ -261,6 +286,7 @@ export function useUnreadCounts( setUnreadCounts({}); setMentions({}); setUnreadLastReadAts({}); + setFirstUnreadIds({}); // Persist to server with single bulk request api.markAllRead().catch((err) => { diff --git a/frontend/src/test/appFavorites.test.tsx b/frontend/src/test/appFavorites.test.tsx index b8746b6..ad7c545 100644 --- a/frontend/src/test/appFavorites.test.tsx +++ b/frontend/src/test/appFavorites.test.tsx @@ -104,6 +104,7 @@ vi.mock('../hooks', async (importOriginal) => { mentions: {}, lastMessageTimes: {}, unreadLastReadAts: {}, + firstUnreadIds: {}, recordMessageEvent: mocks.hookFns.recordMessageEvent, renameConversationState: vi.fn(), markAllRead: mocks.hookFns.markAllRead, diff --git a/frontend/src/test/appSearchJump.test.tsx b/frontend/src/test/appSearchJump.test.tsx index 9345005..259799a 100644 --- a/frontend/src/test/appSearchJump.test.tsx +++ b/frontend/src/test/appSearchJump.test.tsx @@ -53,6 +53,7 @@ vi.mock('../hooks', async (importOriginal) => { mentions: {}, lastMessageTimes: {}, unreadLastReadAts: {}, + firstUnreadIds: {}, recordMessageEvent: vi.fn(), renameConversationState: vi.fn(), markAllRead: vi.fn(), diff --git a/frontend/src/test/appStartupHash.test.tsx b/frontend/src/test/appStartupHash.test.tsx index 8a9a1bd..12d2a3a 100644 --- a/frontend/src/test/appStartupHash.test.tsx +++ b/frontend/src/test/appStartupHash.test.tsx @@ -47,6 +47,7 @@ vi.mock('../hooks', async (importOriginal) => { mentions: {}, lastMessageTimes: {}, unreadLastReadAts: {}, + firstUnreadIds: {}, recordMessageEvent: vi.fn(), renameConversationState: vi.fn(), markAllRead: vi.fn(), diff --git a/frontend/src/test/unreadMarkerResolution.test.ts b/frontend/src/test/unreadMarkerResolution.test.ts new file mode 100644 index 0000000..6baaae7 --- /dev/null +++ b/frontend/src/test/unreadMarkerResolution.test.ts @@ -0,0 +1,56 @@ +/** + * The unread divider is anchored to a message id from the server. These cover the + * one case where that id is deliberately overridden: a channel that has never + * been read, whose true boundary is the start of history and therefore a useless + * jump target. + */ +import { describe, expect, it } from 'vitest'; + +import { resolveUnreadMarkerId } from '../App'; +import type { Message } from '../types'; + +function msg(id: number, receivedAt: number): Message { + return { + id, + type: 'CHAN', + conversation_key: 'CHAN1', + text: `Alice: m${id}`, + sender_timestamp: receivedAt, + received_at: receivedAt, + paths: null, + txt_type: 0, + signature: null, + sender_key: null, + outgoing: false, + acked: 0, + sender_name: 'Alice', + }; +} + +describe('resolveUnreadMarkerId', () => { + const loaded = [msg(50, 1700000050), msg(51, 1700000051), msg(52, 1700000052)]; + + it('uses the server boundary when the channel has been read before', () => { + expect(resolveUnreadMarkerId(9, 1700000000, loaded)).toBe(9); + }); + + it('uses the server boundary when it is inside the loaded window', () => { + expect(resolveUnreadMarkerId(51, null, loaded)).toBe(51); + }); + + it('anchors a never-read channel to the top of the loaded window', () => { + // Boundary 1 is the first message ever sent; jumping there would dump the + // reader at the start of history. Everything loaded is unread, so the top of + // the window is both true and useful. + expect(resolveUnreadMarkerId(1, null, loaded)).toBe(50); + }); + + it('picks the oldest loaded message regardless of array order', () => { + expect(resolveUnreadMarkerId(1, null, [loaded[2], loaded[0], loaded[1]])).toBe(50); + }); + + it('passes through when there is no boundary or no messages', () => { + expect(resolveUnreadMarkerId(null, null, loaded)).toBeNull(); + expect(resolveUnreadMarkerId(7, null, [])).toBe(7); + }); +}); diff --git a/frontend/src/test/useUnreadCounts.test.ts b/frontend/src/test/useUnreadCounts.test.ts index ab9745f..13f2ed4 100644 --- a/frontend/src/test/useUnreadCounts.test.ts +++ b/frontend/src/test/useUnreadCounts.test.ts @@ -479,4 +479,74 @@ describe('useUnreadCounts', () => { expect(result.current.lastMessageTimes[getStateKey('contact', CONTACT_KEY)]).toBe(1700002000); expect(result.current.lastMessageTimes[getStateKey('channel', CHANNEL_KEY)]).toBe(1700002001); }); + + it('seeds the first-unread boundary when a conversation goes unread over the socket', async () => { + // Counts move live over WS but first_unread_ids only arrives with a full + // fetch. Without seeding here, a channel that goes unread while the app is + // open has a count but no boundary, so the divider never renders. + const mocks = await getMockedApi(); + mocks.getUnreads.mockResolvedValue({ + counts: {}, + mentions: {}, + last_message_times: {}, + first_unread_ids: {}, + last_read_ats: {}, + }); + + const { result } = renderWith({ channels: [makeChannel(CHANNEL_KEY, 'Test')] }); + await act(async () => { + await vi.waitFor(() => expect(mocks.getUnreads).toHaveBeenCalled()); + }); + + const key = getStateKey('channel', CHANNEL_KEY); + act(() => { + result.current.recordMessageEvent({ + msg: makeMessage({ id: 4711, type: 'CHAN', conversation_key: CHANNEL_KEY }), + activeConversation: false, + isNewMessage: true, + }); + }); + expect(result.current.firstUnreadIds[key]).toBe(4711); + + // A later message must not move the boundary — it is not the *first* unread. + act(() => { + result.current.recordMessageEvent({ + msg: makeMessage({ id: 4712, type: 'CHAN', conversation_key: CHANNEL_KEY }), + activeConversation: false, + isNewMessage: true, + }); + }); + expect(result.current.firstUnreadIds[key]).toBe(4711); + expect(result.current.unreadCounts[key]).toBe(2); + }); + + it('drops first-unread boundaries on mark-all-read', async () => { + const mocks = await getMockedApi(); + mocks.getUnreads.mockResolvedValue({ + counts: {}, + mentions: {}, + last_message_times: {}, + first_unread_ids: {}, + last_read_ats: {}, + }); + + const { result } = renderWith({ channels: [makeChannel(CHANNEL_KEY, 'Test')] }); + await act(async () => { + await vi.waitFor(() => expect(mocks.getUnreads).toHaveBeenCalled()); + }); + + act(() => { + result.current.recordMessageEvent({ + msg: makeMessage({ id: 99, type: 'CHAN', conversation_key: CHANNEL_KEY }), + activeConversation: false, + isNewMessage: true, + }); + }); + expect(result.current.firstUnreadIds[getStateKey('channel', CHANNEL_KEY)]).toBe(99); + + await act(async () => { + await result.current.markAllRead(); + }); + expect(result.current.firstUnreadIds).toEqual({}); + }); }); diff --git a/tests/test_api.py b/tests/test_api.py index 6090163..c1cc6b2 100644 --- a/tests/test_api.py +++ b/tests/test_api.py @@ -1022,6 +1022,54 @@ class TestReadStateEndpoints: ) assert result["first_unread_ids"][f"contact-{contact_key}"] == first_dm.id + @pytest.mark.asyncio + async def test_first_unread_id_breaks_same_second_ties_by_id(self, test_db): + """Sender timestamps are whole seconds, so the oldest unread second is + routinely shared. The boundary must be the first of those messages, not + an arbitrary one.""" + chan_key = "BBBBBBBBBBBBBBBBBBBBBBBBBBBBBBB2" + await ChannelRepository.upsert(key=chan_key, name="Busy") + await ChannelRepository.update_last_read_at(chan_key, 1000) + + # Three unread messages all landing in the same second. + ids = [] + for text in ("Bob: first", "Bob: second", "Bob: third"): + ids.append( + await MessageRepository.create( + msg_type="CHAN", + text=text, + received_at=1001, + conversation_key=chan_key, + sender_timestamp=1001, + ) + ) + + result = await MessageRepository.get_unread_counts(None) + + assert result["counts"][f"channel-{chan_key}"] == 3 + assert result["first_unread_ids"][f"channel-{chan_key}"] == min(ids) + + @pytest.mark.asyncio + async def test_first_unread_id_ignores_muted_and_outgoing(self, test_db): + """Muted channels are excluded from unread counts, so they must not + report a boundary either.""" + chan_key = "CCCCCCCCCCCCCCCCCCCCCCCCCCCCCCC3" + await ChannelRepository.upsert(key=chan_key, name="Muted") + await ChannelRepository.update_last_read_at(chan_key, 1000) + await ChannelRepository.set_muted(chan_key, True) + await MessageRepository.create( + msg_type="CHAN", + text="Bob: unread but muted", + received_at=1001, + conversation_key=chan_key, + sender_timestamp=1001, + ) + + result = await MessageRepository.get_unread_counts(None) + + assert f"channel-{chan_key}" not in result["counts"] + assert result["first_unread_ids"].get(f"channel-{chan_key}") is None + @pytest.mark.asyncio async def test_get_unreads_no_name_skips_mentions(self, test_db): """Unreads without a radio name returns counts but no mention flags.""" From fc8dec3417e2fbcf8b0b3e5f279a3a2e286ba13a Mon Sep 17 00:00:00 2001 From: Jack Kingsman Date: Sat, 25 Jul 2026 21:02:53 -0700 Subject: [PATCH 5/6] Fix busted tests --- frontend/src/components/MessageList.tsx | 11 ++++++++++ .../e2e/specs/channel-info-and-search.spec.ts | 6 ++++-- tests/e2e/specs/pagination.spec.ts | 21 ++++++++++++------- 3 files changed, 29 insertions(+), 9 deletions(-) diff --git a/frontend/src/components/MessageList.tsx b/frontend/src/components/MessageList.tsx index 59a7733..a1b2338 100644 --- a/frontend/src/components/MessageList.tsx +++ b/frontend/src/components/MessageList.tsx @@ -602,11 +602,22 @@ export function MessageList({ let frames = 0; let raf = 0; + let lastAppliedTop: number | null = null; const step = () => { const list = listRef.current; if (!pendingBottomScrollRef.current || !list) return; + // Something other than us moved the scroll (a programmatic scrollTop, a + // restored position, assistive tech). Gesture handlers only catch a human + // at the wheel, so also bail when the position we last set has been moved + // upward — the pin must never fight another writer. + if (lastAppliedTop !== null && list.scrollTop < lastAppliedTop - 1) { + pendingBottomScrollRef.current = false; + return; + } + scrollToIndex(sortedMessages.length - 1, 'end'); + lastAppliedTop = list.scrollTop; const atBottom = list.scrollHeight - list.scrollTop - list.clientHeight <= 1; if (atBottom || (frames += 1) >= BOTTOM_SCROLL_FRAME_BUDGET) { diff --git a/tests/e2e/specs/channel-info-and-search.spec.ts b/tests/e2e/specs/channel-info-and-search.spec.ts index 36c98fc..3d3a9ad 100644 --- a/tests/e2e/specs/channel-info-and-search.spec.ts +++ b/tests/e2e/specs/channel-info-and-search.spec.ts @@ -37,8 +37,10 @@ test.describe('Channel info pane', () => { await page.goto(`/#channel/${channelKey}/flightless`); await expect(page.getByRole('status', { name: 'Radio OK' })).toBeVisible(); - // Wait for messages to load - await expect(page.getByText('seed-0')).toBeVisible({ timeout: 15_000 }); + // Wait for messages to load. Assert on the newest seeded message, not the + // oldest: the list is virtualized, so it opens at the bottom and only the + // visible window is in the DOM — seed-0 is 30 rows up and legitimately absent. + await expect(page.getByText(`seed-${SEED_COUNT - 1}`)).toBeVisible({ timeout: 15_000 }); // Click the channel name in the header to open info pane const headerTitle = page.locator('h2').filter({ hasText: '#flightless' }); diff --git a/tests/e2e/specs/pagination.spec.ts b/tests/e2e/specs/pagination.spec.ts index fb6cf69..5ac010a 100644 --- a/tests/e2e/specs/pagination.spec.ts +++ b/tests/e2e/specs/pagination.spec.ts @@ -25,13 +25,20 @@ test.describe('Message pagination ordering/dedup', () => { const list = page.locator('div.h-full.overflow-y-auto').first(); - // Scroll to top to trigger older fetch - await list.evaluate((el) => { - el.scrollTop = 0; - }); - - // Wait for oldest message to appear after pagination - await expect(page.getByText('seed-0')).toBeVisible({ timeout: 15_000 }); + // Scroll to top repeatedly until the oldest message renders. + // + // One scroll is not enough with a virtualized list: reaching the top fetches + // the older page, and the list then anchors to the message that was on top so + // the reader keeps their place, which leaves seed-0 fifty rows further up and + // unmounted. A reader scrolls again to get there; so does this. (Before + // virtualization every loaded row was in the DOM, so a single scroll made + // seed-0 findable regardless of where the viewport actually was.) + await expect(async () => { + await list.evaluate((el) => { + el.scrollTop = 0; + }); + await expect(page.getByText('seed-0')).toBeVisible({ timeout: 1_000 }); + }).toPass({ timeout: 20_000 }); // Spot-check ordering: seed-249 appears above seed-200; seed-50 above seed-10 // Fetch from API to validate ordering and dedup From 4f783f4f59890337b1ce205c4ea56c5901cccc91 Mon Sep 17 00:00:00 2001 From: Jack Kingsman Date: Sat, 25 Jul 2026 21:57:48 -0700 Subject: [PATCH 6/6] Fix ANOTHER failing test. God I'm tired tonight. --- frontend/src/test/appPacketIsolation.test.tsx | 1 + 1 file changed, 1 insertion(+) diff --git a/frontend/src/test/appPacketIsolation.test.tsx b/frontend/src/test/appPacketIsolation.test.tsx index 320cdaa..fd49dd2 100644 --- a/frontend/src/test/appPacketIsolation.test.tsx +++ b/frontend/src/test/appPacketIsolation.test.tsx @@ -81,6 +81,7 @@ vi.mock('../hooks', async (importOriginal) => { mentions: {}, lastMessageTimes: {}, unreadLastReadAts: {}, + firstUnreadIds: {}, recordMessageEvent: vi.fn(), renameConversationState: vi.fn(), removeConversationState: vi.fn(),