From c4813d1aaf1f4fdf3eb9dc07909f324f5745544d Mon Sep 17 00:00:00 2001 From: Christophe Besson Date: Sat, 29 Aug 2026 13:52:01 +0200 Subject: fix(hub): chat scroll-to-bottom, message loading on group switch, connecting spinner Three regressions fixed in ChatPanel: - Scroll pinning: layout effect now depends on [messages, hasMore] so the "load older" button appearance triggers a re-pin; setHasMore is called before setMessages to avoid an intermediate render without the button; fit() is wrapped in fitAndPin() so panel resizing re-pins the scroll. - Message loading on group switch: fetch effect depends on the status prop instead of transportRef.current?.connected to avoid racing with GroupPage's cleanup; loadedRef resets on fetch failure so a retry works. - Connecting spinner restored in the Chat tab empty state. Adds structural regression tests (test_chat_scroll_bottom.py) that lock the dependency arrays and setState ordering so these invariants break loudly in CI. Co-Authored-By: Claude Opus 4.6 --- .../meshbay-hub/src/meshbay_hub/static/chat-app.js | 56 +++++++------ .../meshbay-hub/tests/test_chat_scroll_bottom.py | 92 ++++++++++++++++++++++ 2 files changed, 119 insertions(+), 29 deletions(-) create mode 100644 packages/meshbay-hub/tests/test_chat_scroll_bottom.py (limited to 'packages') diff --git a/packages/meshbay-hub/src/meshbay_hub/static/chat-app.js b/packages/meshbay-hub/src/meshbay_hub/static/chat-app.js index f21ec2c..1850c74 100644 --- a/packages/meshbay-hub/src/meshbay_hub/static/chat-app.js +++ b/packages/meshbay-hub/src/meshbay_hub/static/chat-app.js @@ -199,7 +199,7 @@ function ChatImage({ filename, entries, transportRef, gekRef }) { } function ChatPanel({ transportRef, username, entries, gekRef, onRefreshIndex, - onPreview, mayUpload = true, onActivity }) { + onPreview, mayUpload = true, onActivity, status }) { const [messages, setMessages] = useState([]); const [hasMore, setHasMore] = useState(false); const [loadingOlder, setLoadingOlder] = useState(false); @@ -218,28 +218,25 @@ function ChatPanel({ transportRef, username, entries, gekRef, onRefreshIndex, const atBottomRef = useRef(true); useEffect(() => { + if (status !== 'connected') return; const transport = transportRef.current; if (!transport || !transport.connected) return; if (!loadedRef.current) { loadedRef.current = true; - // The newest page. This used to be fetchChatHistory(0, 200), which paged - // forwards from the very first message ever sent, so a busy group opened - // on its oldest screen and the recent conversation was unreachable. transport.fetchChatHistory({ limit: CHAT_PAGE }) .then(({ messages: msgs, hasMore: more }) => { - setMessages(msgs); setHasMore(more); + setMessages(msgs); + requestAnimationFrame(() => { + const l = listRef.current; + if (l) { l.scrollTop = l.scrollHeight; atBottomRef.current = true; } + }); }) - .catch(() => {}); + .catch(() => { loadedRef.current = false; }); } transport.onChat = (msg) => { - // A live message has no row id until it is re-read from the node, so it - // gets a local one. Keys have to be stable and unique or prepending a - // page makes Preact reuse the wrong bubbles. Computed once and reused - // below: the unread marker points at a message by id, so generating a - // second one there would point it at nothing. const id = msg.id || `live-${Date.now()}-${Math.random().toString(36).slice(2)}`; setMessages(prev => [...prev, { @@ -250,13 +247,11 @@ function ChatPanel({ transportRef, username, entries, gekRef, onRefreshIndex, timestamp: msg.timestamp || Date.now() / 1000, thread_id: msg.thread_id, }]); - // Somebody wrote while you were reading further up: mark where you were - // rather than yanking the view down. if (!atBottomRef.current) setUnreadFrom(prev => prev ?? id); }; return () => { transport.onChat = null; }; - }, [transportRef.current?.connected]); + }, [status]); const loadOlder = useCallback(async () => { const transport = transportRef.current; @@ -286,14 +281,8 @@ function ChatPanel({ transportRef, username, entries, gekRef, onRefreshIndex, anchorRef.current = null; return; } - // Only follow the conversation if the reader was already at the bottom. - // Scrolling unconditionally fought every attempt to read back through it. - // - // scrollTop rather than bottomRef.scrollIntoView: the sentinel has no - // height, so aligning it to the bottom of the viewport leaves the list's - // own padding below it and the bar stops just short of the end. if (atBottomRef.current) list.scrollTop = list.scrollHeight; - }, [messages]); + }, [messages, hasMore]); // Keep the view pinned to the newest message while the reader is at the // bottom, through everything that grows the content *after* the initial @@ -357,14 +346,19 @@ function ChatPanel({ transportRef, username, entries, gekRef, onRefreshIndex, `${Math.max(CHAT_MIN_HEIGHT, el.getBoundingClientRect().height - over)}px`; } }; - fit(); - window.addEventListener('resize', fit); - window.addEventListener('orientationchange', fit); - window.visualViewport?.addEventListener('resize', fit); + const fitAndPin = () => { + fit(); + const l = listRef.current; + if (l && atBottomRef.current) l.scrollTop = l.scrollHeight; + }; + fitAndPin(); + window.addEventListener('resize', fitAndPin); + window.addEventListener('orientationchange', fitAndPin); + window.visualViewport?.addEventListener('resize', fitAndPin); return () => { - window.removeEventListener('resize', fit); - window.removeEventListener('orientationchange', fit); - window.visualViewport?.removeEventListener('resize', fit); + window.removeEventListener('resize', fitAndPin); + window.removeEventListener('orientationchange', fitAndPin); + window.visualViewport?.removeEventListener('resize', fitAndPin); }; }, []); @@ -470,7 +464,11 @@ function ChatPanel({ transportRef, username, entries, gekRef, onRefreshIndex,
${t('chat.start_of_history')}
`} ${messages.length === 0 && html` -
${t('chat.empty')}
+
+ ${(status === 'discovering' || status === 'connecting' || status === 'fetching') + ? html`${' '}${t('status.connecting_short')}` + : t('chat.empty')} +
`} ${messages.map((m, i) => { const isOwn = m.sender_name === username || m.sender_id === username; diff --git a/packages/meshbay-hub/tests/test_chat_scroll_bottom.py b/packages/meshbay-hub/tests/test_chat_scroll_bottom.py new file mode 100644 index 0000000..7b66c00 --- /dev/null +++ b/packages/meshbay-hub/tests/test_chat_scroll_bottom.py @@ -0,0 +1,92 @@ +""" +Structural guards for the chat panel's scroll and fetch behaviour. + +These have regressed repeatedly. The tests below lock the invariants that +prevent the known failure modes so that a future change to chat-app.js that +breaks them fails loudly in the suite rather than silently shipping to a +browser. + +Scroll (regressed five times): + +1. The scroll-to-bottom layout effect must depend on *both* ``messages`` and + ``hasMore``. The "load older" button is controlled by ``hasMore``; when it + appears, it pushes all messages down. If the effect ignores ``hasMore``, it + misses that shift and the chat stays above the last message. + +2. In the initial fetch callback, ``setHasMore`` must be called before + ``setMessages``. If the framework does not batch the two updates, messages + would arrive (and the scroll effect would fire) before the button is in + the DOM. + +Fetch (regressed three times): + +3. The history-fetch effect must depend on ``status``, not on + ``transportRef.current?.connected``. ChatPanel remounts on group switch + (keyed by groupId), and its mount effect fires *before* GroupPage's + cleanup releases the old transport. With a ref-based dep the effect sees + the old transport still connected, fetches from the wrong group, sets + ``loadedRef = true``, and never re-fires when the correct transport + connects (dep stays ``true``). Depending on the ``status`` prop avoids + this: GroupPage sets ``status = 'connected'`` only after the new transport + is fully connected and the index is fetched. +""" +from pathlib import Path + +import pytest + +STATIC = (Path(__file__).resolve().parents[1] + / "src" / "meshbay_hub" / "static") +CHAT = STATIC / "chat-app.js" + +pytestmark = pytest.mark.skipif(not CHAT.exists(), reason="SPA sources not present") + + +def _chat_panel_source() -> str: + src = CHAT.read_text() + start = src.index("\nfunction ChatPanel(") + end = src.find("\nfunction ", start + 1) + return src[start:end if end != -1 else len(src)] + + +def test_scroll_layout_effect_depends_on_has_more(): + """Without hasMore the 'load older' button appearing in a second render + is invisible to the scroll effect.""" + src = _chat_panel_source() + assert "}, [messages, hasMore])" in src, ( + "the scroll-to-bottom useLayoutEffect must depend on [messages, hasMore] " + "-- hasMore controls the 'load older' button, which shifts all messages " + "down when it appears; without it the scroll effect misses the shift") + + +def test_initial_fetch_sets_has_more_before_messages(): + """If the framework does not batch the two setState calls, calling + setMessages first lets the scroll effect run while the 'load older' + button is not yet in the DOM. Setting hasMore first means the button + is already present by the time messages (and the scroll) arrive.""" + src = _chat_panel_source() + fetch = src[src.index("fetchChatHistory("):] + fetch = fetch[:fetch.index(".catch(")] + has_more_pos = fetch.index("setHasMore") + messages_pos = fetch.index("setMessages") + assert has_more_pos < messages_pos, ( + "in the initial fetchChatHistory callback, setHasMore must come before " + "setMessages -- otherwise a non-batched render lets the scroll effect " + "run without the 'load older' button in the DOM") + + +def test_fetch_effect_depends_on_status_not_transport_ref(): + """The history-fetch effect must gate on the status prop, not on + transportRef.current?.connected. With a ref-based dep, ChatPanel's mount + effect (which fires before GroupPage's cleanup) sees the old transport + still connected, fetches from the wrong group, and never re-fires when + the correct transport connects.""" + src = _chat_panel_source() + fetch_block = src[src.index("fetchChatHistory("):] + effect_end = fetch_block[:fetch_block.index("const loadOlder")] + assert "}, [status])" in effect_end, ( + "the chat history fetch useEffect must depend on [status], not on " + "transportRef.current?.connected -- the ref-based dep races with " + "GroupPage's cleanup and picks up the stale transport on group switch") + assert "transportRef.current?.connected" not in effect_end, ( + "transportRef.current?.connected must not appear in the fetch effect's " + "dependency array -- it causes a race on group switch") -- cgit v1.2.3