From 6cceb81c46074fedf17b0a0d577ab65ce5076990 Mon Sep 17 00:00:00 2001 From: Arnas Donauskas Date: Mon, 20 Apr 2026 20:01:59 +0300 Subject: [PATCH] fix(inbox): don't drop messages when ?c= URL param updates MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reports: clicking a conversation sometimes left the message thread stuck on "No messages yet" until the user hit browser refresh. Root cause — a three-way race kicked off by any click on a conversation in the list: 1. handleSelectConversation runs: setActiveConversation(B) setMessages([]) router.replace("/inbox?c=B") 2. The URL change flips `deepLinkConvId` (from useSearchParams). handleConversationsLoaded is a useCallback with [deepLinkConvId], so its identity changes. That cascades into ConversationList, whose fetchConversations useCallback has [onConversationsLoaded], whose useEffect has [fetchConversations] — so the list refetches conversations purely because the URL changed. 3. When the refetch lands, handleConversationsLoaded runs its deep-link auto-select branch. autoSelectedForDeepLinkRef still points at its previous value (null, or the previous deep-link), so the guard `ref !== deepLinkConvId` passes and the branch re-applies B: setActiveConversation(B) // no-op, same id setMessages([]) // BLOWS AWAY the messages // MessageThread just fetched 4. MessageThread's fetch effect only re-runs when conversationId changes. B is still B. It does not refetch. Messages stuck at []. A hard reload unsticks things because state starts from scratch and the initial deep-link flow is the only path that runs the auto-select. Three defense-in-depth fixes: - handleSelectConversation now writes the clicked id into autoSelectedForDeepLinkRef BEFORE the router.replace, so by the time any downstream refetch lands, the auto-select branch sees ref === deepLinkConvId and skips. - handleConversationsLoaded early-returns if the deep-linked conversation is already the active one. This is redundant with the ref update above but protects against any other code path we don't foresee triggering a refetch. - ConversationList's fetch is now driven by a plain useEffect with [] deps, using a ref for the latest onConversationsLoaded callback. URL changes no longer cause a conversations refetch. Matches the same stabilisation pattern used in src/hooks/use-auth.tsx and src/components/inbox/message-thread.tsx. Verified: typecheck passes; lint count unchanged (37 pre-existing problems, zero added). Couldn't exercise the rendered /inbox from the preview sandbox — it's auth-gated and no test credentials are available. Please confirm in the authenticated inbox: - Click back and forth between two conversations with different message histories; messages should populate every time. - Open /inbox?c= directly; thread opens with messages. - Switch conversations after a deep-link landing; still works. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/app/(dashboard)/inbox/page.tsx | 20 ++++++- src/components/inbox/conversation-list.tsx | 67 ++++++++++++++-------- 2 files changed, 61 insertions(+), 26 deletions(-) diff --git a/src/app/(dashboard)/inbox/page.tsx b/src/app/(dashboard)/inbox/page.tsx index f7e531d..a5f9501 100644 --- a/src/app/(dashboard)/inbox/page.tsx +++ b/src/app/(dashboard)/inbox/page.tsx @@ -163,6 +163,16 @@ export default function InboxPage() { loaded.length > 0 ) { autoSelectedForDeepLinkRef.current = deepLinkConvId; + // If the deep-linked conversation is already the active one + // (e.g. because the user clicked it in the list and we + // router.replace()'d the URL, which made the ConversationList + // refetch and land us back here), do NOT re-apply it. Doing so + // would setMessages([]) on a thread whose messages have + // already been loaded by MessageThread — and because + // conversationId didn't change, MessageThread wouldn't + // refetch. The thread would read "No messages yet" until a + // full page reload rehydrated state from scratch. + if (activeConversation?.id === deepLinkConvId) return; const match = loaded.find((c) => c.id === deepLinkConvId); if (match) { setActiveConversation(match); @@ -171,7 +181,7 @@ export default function InboxPage() { } } }, - [deepLinkConvId] + [deepLinkConvId, activeConversation?.id] ); const handleSelectConversation = useCallback( @@ -184,6 +194,14 @@ export default function InboxPage() { setActiveConversation(conv); setActiveContact(conv.contact ?? null); setMessages([]); + // Record the selection on the deep-link ref BEFORE we change the + // URL. The router.replace below flips `deepLinkConvId`, which can + // in turn cause ConversationList to refetch and eventually call + // handleConversationsLoaded again. Without this line, the ref + // still points at the previous value, the auto-select block + // sees `ref !== deepLinkConvId`, fires a second time, and + // clobbers the messages MessageThread just fetched. + autoSelectedForDeepLinkRef.current = conv.id; // Reflect the selection in the URL so a refresh lands the user // back in the same thread, and so copy-paste links work. Use // replace() to avoid polluting browser history with every click. diff --git a/src/components/inbox/conversation-list.tsx b/src/components/inbox/conversation-list.tsx index fe9ae48..b582a53 100644 --- a/src/components/inbox/conversation-list.tsx +++ b/src/components/inbox/conversation-list.tsx @@ -1,6 +1,6 @@ "use client"; -import { useState, useEffect, useCallback, useMemo } from "react"; +import { useState, useEffect, useCallback, useMemo, useRef } from "react"; import { createClient } from "@/lib/supabase/client"; import { cn } from "@/lib/utils"; import type { Conversation, ConversationStatus } from "@/types"; @@ -46,32 +46,49 @@ export function ConversationList({ const [filter, setFilter] = useState("all"); const [loading, setLoading] = useState(true); - const fetchConversations = useCallback(async () => { - const supabase = createClient(); - const { data, error } = await supabase - .from("conversations") - .select("*, contact:contacts(*)") - .order("last_message_at", { ascending: false }); - - if (error) { - // Supabase errors have non-enumerable properties — log fields explicitly - console.error("Failed to fetch conversations:", { - message: error.message, - details: error.details, - hint: error.hint, - code: error.code, - }); - setLoading(false); - return; - } - - onConversationsLoaded(data ?? []); - setLoading(false); - }, [onConversationsLoaded]); + // Keep the latest callback in a ref so the fetch effect below can + // have a stable, empty-dep identity. Previously the fetch useCallback + // depended on `onConversationsLoaded`, which depends on the parent's + // `deepLinkConvId` — so every URL change (including one the parent + // triggered via router.replace after a click) caused a fresh + // conversations fetch. That extra refetch was the trigger for the + // deep-link auto-select running a second time and wiping the active + // thread's messages. + const onConversationsLoadedRef = useRef(onConversationsLoaded); + onConversationsLoadedRef.current = onConversationsLoaded; useEffect(() => { - fetchConversations(); - }, [fetchConversations]); + const supabase = createClient(); + let cancelled = false; + + (async () => { + const { data, error } = await supabase + .from("conversations") + .select("*, contact:contacts(*)") + .order("last_message_at", { ascending: false }); + + if (cancelled) return; + + if (error) { + // Supabase errors have non-enumerable properties — log fields explicitly + console.error("Failed to fetch conversations:", { + message: error.message, + details: error.details, + hint: error.hint, + code: error.code, + }); + setLoading(false); + return; + } + + onConversationsLoadedRef.current(data ?? []); + setLoading(false); + })(); + + return () => { + cancelled = true; + }; + }, []); const filtered = useMemo(() => { let result = conversations;