diff --git a/frontend/src/components/MessageList.tsx b/frontend/src/components/MessageList.tsx index 6603106..3290cd5 100644 --- a/frontend/src/components/MessageList.tsx +++ b/frontend/src/components/MessageList.tsx @@ -204,11 +204,9 @@ export function MessageList({ const resendTimersRef = useRef>>(new Map()); const [highlightedMessageId, setHighlightedMessageId] = useState(null); const [showJumpToUnread, setShowJumpToUnread] = useState(false); + const [jumpToUnreadDismissed, setJumpToUnreadDismissed] = useState(false); const targetScrolledRef = useRef(false); const unreadMarkerRef = useRef(null); - const setUnreadMarkerElement = useCallback((node: HTMLButtonElement | HTMLDivElement | null) => { - unreadMarkerRef.current = node; - }, []); // Capture scroll state in the scroll handler BEFORE any state updates const scrollStateRef = useRef({ @@ -330,68 +328,6 @@ export function MessageList({ }; }, [messages, onResendChannelMessage]); - // Refs for scroll handler to read without causing callback recreation - const onLoadOlderRef = useRef(onLoadOlder); - const loadingOlderRef = useRef(loadingOlder); - const hasOlderMessagesRef = useRef(hasOlderMessages); - const onLoadNewerRef = useRef(onLoadNewer); - const loadingNewerRef = useRef(loadingNewer); - const hasNewerMessagesRef = useRef(hasNewerMessages); - onLoadOlderRef.current = onLoadOlder; - loadingOlderRef.current = loadingOlder; - hasOlderMessagesRef.current = hasOlderMessages; - onLoadNewerRef.current = onLoadNewer; - loadingNewerRef.current = loadingNewer; - hasNewerMessagesRef.current = hasNewerMessages; - - // Handle scroll - capture state and detect when user is near top/bottom - // Stable callback: reads changing values from refs, never recreated. - const handleScroll = useCallback(() => { - if (!listRef.current) return; - - const { scrollTop, scrollHeight, clientHeight } = listRef.current; - const distanceFromBottom = scrollHeight - scrollTop - clientHeight; - - // Always capture current scroll state (needed for scroll preservation) - scrollStateRef.current = { - scrollTop, - scrollHeight, - clientHeight, - wasNearTop: scrollTop < 150, - wasNearBottom: distanceFromBottom < 100, - }; - - // Show scroll-to-bottom button when not near the bottom (more than 100px away) - setShowScrollToBottom(distanceFromBottom > 100); - - if (!onLoadOlderRef.current || loadingOlderRef.current || !hasOlderMessagesRef.current) { - // skip older load - } else if (scrollTop < 100) { - onLoadOlderRef.current(); - } - - // Trigger load newer when within 100px of bottom - if ( - onLoadNewerRef.current && - !loadingNewerRef.current && - hasNewerMessagesRef.current && - distanceFromBottom < 100 - ) { - onLoadNewerRef.current(); - } - }, []); - - // Scroll to bottom handler (or jump to bottom if viewing historical messages) - const scrollToBottom = useCallback(() => { - if (hasNewerMessages && onJumpToBottom) { - onJumpToBottom(); - return; - } - if (listRef.current) { - listRef.current.scrollTop = listRef.current.scrollHeight; - } - }, [hasNewerMessages, onJumpToBottom]); - // Sort messages by received_at ascending (oldest first) // Note: Deduplication is handled by useConversationMessages.addMessageIfNew() // and the database UNIQUE constraint on (type, conversation_key, text, sender_timestamp) @@ -408,10 +344,117 @@ export function MessageList({ return sortedMessages.findIndex((msg) => !msg.outgoing && msg.received_at > boundary); }, [sortedMessages, unreadMarkerLastReadAt]); + const syncJumpToUnreadVisibility = useCallback(() => { + if (unreadMarkerIndex === -1 || jumpToUnreadDismissed) { + setShowJumpToUnread(false); + return; + } + + const marker = unreadMarkerRef.current; + const list = listRef.current; + if (!marker || !list) { + setShowJumpToUnread(true); + return; + } + + const markerRect = marker.getBoundingClientRect(); + const listRect = list.getBoundingClientRect(); + + if ( + markerRect.width === 0 || + markerRect.height === 0 || + listRect.width === 0 || + listRect.height === 0 + ) { + setShowJumpToUnread(true); + return; + } + + const markerVisible = + markerRect.top >= listRect.top && + markerRect.bottom <= listRect.bottom && + markerRect.left >= listRect.left && + markerRect.right <= listRect.right; + + setShowJumpToUnread(!markerVisible); + }, [jumpToUnreadDismissed, unreadMarkerIndex]); + + // Refs for scroll handler to read without causing callback recreation + const onLoadOlderRef = useRef(onLoadOlder); + const loadingOlderRef = useRef(loadingOlder); + const hasOlderMessagesRef = useRef(hasOlderMessages); + const onLoadNewerRef = useRef(onLoadNewer); + const loadingNewerRef = useRef(loadingNewer); + const hasNewerMessagesRef = useRef(hasNewerMessages); + onLoadOlderRef.current = onLoadOlder; + loadingOlderRef.current = loadingOlder; + hasOlderMessagesRef.current = hasOlderMessages; + onLoadNewerRef.current = onLoadNewer; + loadingNewerRef.current = loadingNewer; + hasNewerMessagesRef.current = hasNewerMessages; + + const setUnreadMarkerElement = useCallback( + (node: HTMLButtonElement | HTMLDivElement | null) => { + unreadMarkerRef.current = node; + syncJumpToUnreadVisibility(); + }, + [syncJumpToUnreadVisibility] + ); + useEffect(() => { - setShowJumpToUnread(unreadMarkerIndex !== -1); + setJumpToUnreadDismissed(false); }, [unreadMarkerIndex]); + useLayoutEffect(() => { + syncJumpToUnreadVisibility(); + }, [messages, syncJumpToUnreadVisibility]); + + // Handle scroll - capture state and detect when user is near top/bottom + // Stable callback: reads changing values from refs, never recreated. + const handleScroll = useCallback(() => { + if (!listRef.current) return; + + const { scrollTop, scrollHeight, clientHeight } = listRef.current; + const distanceFromBottom = scrollHeight - scrollTop - clientHeight; + + scrollStateRef.current = { + scrollTop, + scrollHeight, + clientHeight, + wasNearTop: scrollTop < 150, + wasNearBottom: distanceFromBottom < 100, + }; + + setShowScrollToBottom(distanceFromBottom > 100); + + if (!onLoadOlderRef.current || loadingOlderRef.current || !hasOlderMessagesRef.current) { + // skip older load + } else if (scrollTop < 100) { + onLoadOlderRef.current(); + } + + if ( + onLoadNewerRef.current && + !loadingNewerRef.current && + hasNewerMessagesRef.current && + distanceFromBottom < 100 + ) { + onLoadNewerRef.current(); + } + syncJumpToUnreadVisibility(); + }, [syncJumpToUnreadVisibility]); + + // Scroll to bottom handler (or jump to bottom if viewing historical messages) + const scrollToBottom = useCallback(() => { + if (hasNewerMessages && onJumpToBottom) { + onJumpToBottom(); + return; + } + if (listRef.current) { + listRef.current.scrollTop = listRef.current.scrollHeight; + } + }, [hasNewerMessages, onJumpToBottom]); + // Sender info for outgoing messages (used by path modal on own messages) const selfSenderInfo = useMemo( () => ({ @@ -841,6 +884,7 @@ export function MessageList({ type="button" onClick={() => { unreadMarkerRef.current?.scrollIntoView?.({ block: 'center' }); + setJumpToUnreadDismissed(true); setShowJumpToUnread(false); }} className="pointer-events-auto h-9 rounded-full bg-card hover:bg-accent border border-border px-3 text-sm font-medium shadow-lg transition-all hover:scale-105 focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-ring" diff --git a/frontend/src/hooks/useConversationMessages.ts b/frontend/src/hooks/useConversationMessages.ts index 7deefcd..85e6e98 100644 --- a/frontend/src/hooks/useConversationMessages.ts +++ b/frontend/src/hooks/useConversationMessages.ts @@ -86,6 +86,30 @@ function isMessageConversation(conversation: Conversation | null): conversation return !!conversation && !['raw', 'map', 'visualizer', 'search'].includes(conversation.type); } +function appendUniqueMessages(current: Message[], incoming: Message[]): Message[] { + if (incoming.length === 0) return current; + + const seenIds = new Set(current.map((msg) => msg.id)); + const seenContent = new Set(current.map((msg) => getMessageContentKey(msg))); + const additions: Message[] = []; + + for (const msg of incoming) { + const contentKey = getMessageContentKey(msg); + if (seenIds.has(msg.id) || seenContent.has(contentKey)) { + continue; + } + seenIds.add(msg.id); + seenContent.add(contentKey); + additions.push(msg); + } + + if (additions.length === 0) { + return current; + } + + return [...current, ...additions]; +} + export function useConversationMessages( activeConversation: Conversation | null, targetMessageId?: number | null @@ -139,6 +163,7 @@ export function useConversationMessages( const fetchingConversationIdRef = useRef(null); const latestReconcileRequestIdRef = useRef(0); const messagesRef = useRef([]); + const loadingOlderRef = useRef(false); const hasOlderMessagesRef = useRef(false); const hasNewerMessagesRef = useRef(false); const prevConversationIdRef = useRef(null); @@ -147,6 +172,10 @@ export function useConversationMessages( messagesRef.current = messages; }, [messages]); + useEffect(() => { + loadingOlderRef.current = loadingOlder; + }, [loadingOlder]); + useEffect(() => { hasOlderMessagesRef.current = hasOlderMessages; }, [hasOlderMessages]); @@ -252,7 +281,13 @@ export function useConversationMessages( ); const fetchOlderMessages = useCallback(async () => { - if (!isMessageConversation(activeConversation) || loadingOlder || !hasOlderMessages) return; + if ( + !isMessageConversation(activeConversation) || + loadingOlderRef.current || + !hasOlderMessagesRef.current + ) { + return; + } const conversationId = activeConversation.id; const oldestMessage = messages.reduce( @@ -266,6 +301,7 @@ export function useConversationMessages( ); if (!oldestMessage) return; + loadingOlderRef.current = true; setLoadingOlder(true); try { const data = await api.getMessages({ @@ -281,9 +317,17 @@ export function useConversationMessages( const dataWithPendingAck = data.map((msg) => applyPendingAck(msg)); if (dataWithPendingAck.length > 0) { - setMessages((prev) => [...prev, ...dataWithPendingAck]); - for (const msg of dataWithPendingAck) { - seenMessageContent.current.add(getMessageContentKey(msg)); + let nextMessages: Message[] | null = null; + setMessages((prev) => { + const merged = appendUniqueMessages(prev, dataWithPendingAck); + if (merged !== prev) { + nextMessages = merged; + } + return merged; + }); + if (nextMessages) { + messagesRef.current = nextMessages; + syncSeenContent(nextMessages); } } setHasOlderMessages(dataWithPendingAck.length >= MESSAGE_PAGE_SIZE); @@ -293,9 +337,10 @@ export function useConversationMessages( description: err instanceof Error ? err.message : 'Check your connection', }); } finally { + loadingOlderRef.current = false; setLoadingOlder(false); } - }, [activeConversation, applyPendingAck, hasOlderMessages, loadingOlder, messages]); + }, [activeConversation, applyPendingAck, messages, syncSeenContent]); const fetchNewerMessages = useCallback(async () => { if (!isMessageConversation(activeConversation) || loadingNewer || !hasNewerMessages) return; @@ -379,6 +424,7 @@ export function useConversationMessages( } setLoadingOlder(false); + loadingOlderRef.current = false; setLoadingNewer(false); if (conversationChanged) { setHasNewerMessages(false); diff --git a/frontend/src/test/messageList.test.tsx b/frontend/src/test/messageList.test.tsx index 63a4b93..d887747 100644 --- a/frontend/src/test/messageList.test.tsx +++ b/frontend/src/test/messageList.test.tsx @@ -7,6 +7,7 @@ import { MessageList } from '../components/MessageList'; import type { Message } from '../types'; const scrollIntoViewMock = vi.fn(); +const originalGetBoundingClientRect = HTMLElement.prototype.getBoundingClientRect; function createMessage(overrides: Partial = {}): Message { return { @@ -35,6 +36,11 @@ describe('MessageList channel sender rendering', () => { value: scrollIntoViewMock, writable: true, }); + Object.defineProperty(HTMLElement.prototype, 'getBoundingClientRect', { + configurable: true, + value: originalGetBoundingClientRect, + writable: true, + }); }); it('renders explicit corrupt placeholder and warning avatar for unnamed corrupt channel packets', () => { @@ -155,4 +161,68 @@ describe('MessageList channel sender rendering', () => { expect(screen.getByText('Unread messages')).toBeInTheDocument(); expect(scrollIntoViewMock).toHaveBeenCalled(); }); + + it('hides the jump-to-unread button when the unread marker is already visible', () => { + Object.defineProperty(HTMLElement.prototype, 'getBoundingClientRect', { + configurable: true, + writable: true, + value: function () { + const element = this as HTMLElement; + if (element.textContent?.includes('Unread messages')) { + return { + top: 200, + bottom: 240, + left: 0, + right: 300, + width: 300, + height: 40, + x: 0, + y: 200, + toJSON: () => '', + }; + } + if (element.className.includes('overflow-y-auto')) { + return { + top: 100, + bottom: 500, + left: 0, + right: 400, + width: 400, + height: 400, + x: 0, + y: 100, + toJSON: () => '', + }; + } + return { + top: 0, + bottom: 0, + left: 0, + right: 0, + width: 0, + height: 0, + x: 0, + y: 0, + toJSON: () => '', + }; + }, + }); + + const messages = [ + createMessage({ id: 1, received_at: 1700000001, text: 'Alice: older' }), + createMessage({ id: 2, received_at: 1700000010, text: 'Alice: newer' }), + ]; + + render( + + ); + + expect(screen.getByText('Unread messages')).toBeInTheDocument(); + expect(screen.queryByRole('button', { name: 'Jump to unread' })).not.toBeInTheDocument(); + }); }); diff --git a/frontend/src/test/useConversationMessages.race.test.ts b/frontend/src/test/useConversationMessages.race.test.ts index a0d49e5..ef0accd 100644 --- a/frontend/src/test/useConversationMessages.race.test.ts +++ b/frontend/src/test/useConversationMessages.race.test.ts @@ -323,6 +323,102 @@ describe('useConversationMessages background reconcile ordering', () => { }); }); +describe('useConversationMessages older-page dedup and reentry', () => { + beforeEach(() => { + mockGetMessages.mockReset(); + messageCache.clear(); + }); + + it('prevents duplicate overlapping older-page fetches in the same tick', async () => { + const conv: Conversation = { type: 'contact', id: 'conv_a', name: 'Contact A' }; + + const fullPage = Array.from({ length: 200 }, (_, i) => + createMessage({ + id: i + 1, + conversation_key: 'conv_a', + text: `msg-${i + 1}`, + sender_timestamp: 1700000000 + i, + received_at: 1700000000 + i, + }) + ); + mockGetMessages.mockResolvedValueOnce(fullPage); + + const olderDeferred = createDeferred(); + mockGetMessages.mockReturnValueOnce(olderDeferred.promise); + + const { result } = renderHook(() => useConversationMessages(conv)); + + await waitFor(() => expect(result.current.messagesLoading).toBe(false)); + expect(result.current.messages).toHaveLength(200); + expect(result.current.hasOlderMessages).toBe(true); + + act(() => { + void result.current.fetchOlderMessages(); + void result.current.fetchOlderMessages(); + }); + + expect(mockGetMessages).toHaveBeenCalledTimes(2); // initial page + one older fetch + + olderDeferred.resolve([ + createMessage({ + id: 0, + conversation_key: 'conv_a', + text: 'older-msg', + sender_timestamp: 1699999999, + received_at: 1699999999, + }), + ]); + + await waitFor(() => expect(result.current.loadingOlder).toBe(false)); + expect(result.current.messages).toHaveLength(201); + expect(result.current.messages.filter((msg) => msg.id === 0)).toHaveLength(1); + }); + + it('does not append duplicate messages from an overlapping older page', async () => { + const conv: Conversation = { type: 'contact', id: 'conv_a', name: 'Contact A' }; + + const fullPage = Array.from({ length: 200 }, (_, i) => + createMessage({ + id: i + 1, + conversation_key: 'conv_a', + text: `msg-${i + 1}`, + sender_timestamp: 1700000000 + i, + received_at: 1700000000 + i, + }) + ); + mockGetMessages.mockResolvedValueOnce(fullPage); + mockGetMessages.mockResolvedValueOnce([ + createMessage({ + id: 1, + conversation_key: 'conv_a', + text: 'msg-1', + sender_timestamp: 1700000000, + received_at: 1700000000, + }), + createMessage({ + id: 0, + conversation_key: 'conv_a', + text: 'older-msg', + sender_timestamp: 1699999999, + received_at: 1699999999, + }), + ]); + + const { result } = renderHook(() => useConversationMessages(conv)); + + await waitFor(() => expect(result.current.messagesLoading).toBe(false)); + expect(result.current.messages).toHaveLength(200); + + await act(async () => { + await result.current.fetchOlderMessages(); + }); + + expect(result.current.messages.filter((msg) => msg.id === 1)).toHaveLength(1); + expect(result.current.messages.filter((msg) => msg.id === 0)).toHaveLength(1); + expect(result.current.messages).toHaveLength(201); + }); +}); + describe('useConversationMessages forward pagination', () => { beforeEach(() => { mockGetMessages.mockReset();