diff --git a/backend/app/routes/chats.py b/backend/app/routes/chats.py index 4c575cf16..175fac937 100644 --- a/backend/app/routes/chats.py +++ b/backend/app/routes/chats.py @@ -11,7 +11,7 @@ from fastapi import APIRouter, Depends, HTTPException from fastapi.responses import PlainTextResponse, Response from pydantic import BaseModel, Field, field_validator -from sqlalchemy import func +from sqlalchemy import Text, case, cast, func from sqlalchemy.orm import Session from app import activity, auth, models, providers, questions @@ -262,6 +262,28 @@ def _owner_chat_summary(chat: models.Chat) -> dict: } +def _owner_chat_summary_projection(chat) -> dict: + """Serialize the lightweight row projection used by the drawer list. + + ``GET /api/chats`` used to hydrate every complete ``Chat`` ORM object merely + to return eight summary fields. On a long-lived instance that decoded tens of + megabytes of transcript JSON on every drawer open and chat switch, contending + with the selected chat's small detail read. Keep transcript inspection inside + the database (``message_count``) and never materialize ``messages`` here. + """ + return { + "id": chat.id, + "title": chat.title, + "updated_at": chat.updated_at.isoformat(), + "activity_at": chat.activity_at.isoformat() if chat.activity_at else None, + "pinned_at": chat.pinned_at.isoformat() if chat.pinned_at else None, + "has_messages": bool(chat.message_count), + "created_by_app_id": chat.created_by_app_id, + "run_status": chat.run_status, + "running": chat.run_status == "running" or is_chat_running(chat.id), + } + + def _chat_detail_response( chat: models.Chat, *, @@ -419,7 +441,33 @@ def list_chats( # a `desc()` on a nullable column would put NULL last under our # SQLite collation, but making the boolean explicit is clearer and # portable. - q = db.query(models.Chat).filter(models.Chat.deleted_at.is_(None)) + # Drawer projection only. Selecting the Chat entity here hydrates its full + # ``messages`` JSON column even though the response needs only a boolean; on + # this owner's history that was ~47 MB of JSON decoding per refresh. Compare + # the canonical serialized empty-array value in SQL instead of parsing every + # JSON array with json_array_length: Chat.messages is a non-null list written + # by SQLAlchemy's canonical serializer, and CAST(... AS TEXT) is portable + # across SQLite and PostgreSQL. A future raw-import path must normalize JSON + # text first (PostgreSQL's json type preserves whitespace such as ``[ ]``). + # The database can reject non-empty values from their stored length without + # walking every transcript. + q = db.query( + models.Chat.id, + models.Chat.title, + models.Chat.updated_at, + models.Chat.activity_at, + models.Chat.pinned_at, + models.Chat.created_by_app_id, + # App-created owner-visible chats carry their visibility bit here. Owner + # chats normally keep this NULL, so this remains a tiny projection rather + # than pulling transcript/runtime JSON into the hot path. + models.Chat.agent_settings_json, + models.Chat.run_status, + case( + (cast(models.Chat.messages, Text) != "[]", 1), + else_=0, + ).label("message_count"), + ).filter(models.Chat.deleted_at.is_(None)) chats = ( q.order_by( models.Chat.pinned_at.is_(None), @@ -433,7 +481,7 @@ def list_chats( # embedded app panels and stay hidden; an app can opt a spawned, first-class # owner conversation into the drawer by setting owner_visible at creation. chats = [c for c in chats if _visible_in_owner_drawer(c)] - return [_owner_chat_summary(c) for c in chats] + return [_owner_chat_summary_projection(c) for c in chats] @router.get("/session-links") diff --git a/backend/tests/test_chats.py b/backend/tests/test_chats.py index c9fbaae96..2d6c32994 100644 --- a/backend/tests/test_chats.py +++ b/backend/tests/test_chats.py @@ -3,8 +3,9 @@ import asyncio from uuid import uuid4 -from app import memory, questions +from app import memory, models, questions from app.pending_questions import PendingQuestion +from sqlalchemy import event def _make_pending() -> PendingQuestion: @@ -116,6 +117,38 @@ def test_create_chat_returns_canonical_owner_drawer_summary(client, auth): assert body["detail"] == detail_body +def test_chat_list_projects_summaries_without_hydrating_transcripts( + client, auth, +): + created = client.post( + "/api/chats", + json={ + "title": "Projected row", + "messages": [{"role": "user", "content": "large history sentinel"}], + }, + headers=auth, + ) + assert created.status_code == 200 + + hydrated_chat_ids = [] + + def on_load(chat, _context): + hydrated_chat_ids.append(chat.id) + + event.listen(models.Chat, "load", on_load) + try: + listed = client.get("/api/chats", headers=auth) + finally: + event.remove(models.Chat, "load", on_load) + + assert listed.status_code == 200 + row = next(item for item in listed.json() if item["id"] == created.json()["id"]) + assert row["has_messages"] is True + assert hydrated_chat_ids == [], ( + "the drawer list must not instantiate Chat objects and decode messages" + ) + + def test_update_chat_rejects_cross_site_request(client, auth, chat): cross = client.put( f"/api/chats/{chat.id}", diff --git a/frontend/src/api/client.js b/frontend/src/api/client.js index 1dc6c6307..d087b37bb 100644 --- a/frontend/src/api/client.js +++ b/frontend/src/api/client.js @@ -338,7 +338,7 @@ export const api = { }, }, chats: { - list: () => apiFetch('/chats'), + list: (options = {}) => apiFetch('/chats', options), create: (payload) => apiFetch('/chats', { method: 'POST', body: JSON.stringify(payload), diff --git a/frontend/src/components/Shell/Shell.jsx b/frontend/src/components/Shell/Shell.jsx index 607a1e375..b051ed297 100644 --- a/frontend/src/components/Shell/Shell.jsx +++ b/frontend/src/components/Shell/Shell.jsx @@ -59,8 +59,11 @@ import { addCreatedChatToList, createdChatDetailCache, currentReusableEmptyChat, - detailIsUntouchedEmptyChat, enteredEmptySingleScreen, + mergeChatListWithCreatedGuards, + reconcileCreatedChatGuard, + rememberCreatedChat, + reusableChatDetailVerdict, } from './newChatPolicy.js' import { reloadWhenWorkerTakesOver, @@ -469,7 +472,20 @@ export default function Shell() { const { loadTheme } = useTheme() const queryClient = useQueryClient() const appsQuery = appQueries.list.useQuery() - const chatsQuery = chatQueries.list.useQuery() + // Create responses are authoritative even when the next NetworkFirst list + // request has to fall back to a just-stale service-worker copy. Reconcile at + // the query function boundary so the protected row never disappears from + // cache/render between fetch settlement and an after-the-fact patch. + const recentlyCreatedChatsRef = useRef(new Map()) + const reconcileCreatedChats = useCallback( + rows => mergeChatListWithCreatedGuards( + rows, recentlyCreatedChatsRef.current, + ), + [], + ) + const chatsQuery = chatQueries.list.useQuery({ + reconcile: reconcileCreatedChats, + }) const apps = appsQuery.data ?? [] const chats = chatsQuery.data ?? [] // Warm the model registry as soon as a chat is open so the composer's @@ -2596,14 +2612,52 @@ export default function Shell() { }) if (empty && online) { try { + const staleEmptyId = empty.id const res = await apiFetch( `/chats/${encodeURIComponent(empty.id)}?limit=1`, { timeoutMs: 5000 }, ) - const detail = res.ok ? await res.json() : null - if (!detailIsUntouchedEmptyChat(detail)) { + let detail = null + if (res.ok) detail = await res.json() + const verdict = reusableChatDetailVerdict({ + ok: res.ok, + status: res.status, + detail, + }) + if (verdict !== 'empty') { empty = null - void refreshChats() + reconcileCreatedChatGuard( + recentlyCreatedChatsRef.current, + staleEmptyId, + verdict, + ) + if (verdict === 'missing') { + // A 404 is authoritative deletion, not evidence of content. + knownExistingOffListChatIdsRef.current.delete(String(staleEmptyId)) + queryClient.setQueryData(chatQueries.keys.all, current => { + if (!Array.isArray(current)) return current + const next = current.filter( + chat => String(chat.id) !== String(staleEmptyId), + ) + chatsRef.current = next + return next + }) + } else if (verdict === 'occupied') { + // The complete successful detail read has given us the only fact + // New Chat needs: this row is no longer reusable. Publish that + // narrow correction instead of launching a drawer list beside the + // create request. Uncertain/malformed responses leave it unchanged. + queryClient.setQueryData(chatQueries.keys.all, current => { + if (!Array.isArray(current)) return current + const next = current.map(chat => ( + String(chat.id) === String(staleEmptyId) + ? { ...chat, has_messages: true } + : chat + )) + chatsRef.current = next + return next + }) + } } } catch { empty = null @@ -2620,8 +2674,18 @@ export default function Shell() { if (creatingChatRef.current) return { chatId: null, reason: 'inflight' } creatingChatRef.current = true try { + // Opening the drawer may already have started a list read whose snapshot + // predates this POST. Cancel it before creation so it cannot land later + // and overwrite the optimistic row with a stale list. fetchChats consumes + // TanStack's AbortSignal, making this a real network cancellation rather + // than merely ignoring the query result. + await queryClient.cancelQueries({ + queryKey: chatQueries.keys.all, + exact: true, + }) const res = await api.chats.create({ title: 'New chat' }) const chat = await jsonOrThrow(res, 'Chat creation failed') + rememberCreatedChat(recentlyCreatedChatsRef.current, chat) const detailCache = createdChatDetailCache(chat) if (detailCache) { queryClient.setQueryData(chatMessagesQueryKey(chat.id), detailCache) @@ -2631,9 +2695,9 @@ export default function Shell() { chatsRef.current = next return next }) - // Navigation and first paint can start from the authoritative create response. - // Both wider caches revalidate off the opening path. - void refreshChats() + // Navigation, drawer membership, and first paint all come from the + // authoritative create response. Do not immediately replace it with a + // second list read; ordinary drawer/run events revalidate later. return { chatId: chat.id, reason: null } } catch { return { chatId: null, reason: 'error' } @@ -2802,7 +2866,6 @@ export default function Shell() { function selectChat(id) { clearChatAttention(id) navTo('chat', { chatId: id }) - refreshChats() } async function deleteChat(id) { @@ -2830,6 +2893,7 @@ export default function Shell() { } // A 404 means the server row is already gone; remove the local phantom. } + recentlyCreatedChatsRef.current.delete(String(id)) try { sessionStorage.removeItem(`draft:${id}`) } catch {} // Evict the cached messages so a future chat-ID collision (e.g. // recovery) can't surface stale content. diff --git a/frontend/src/components/Shell/__tests__/newChatPolicy.test.js b/frontend/src/components/Shell/__tests__/newChatPolicy.test.js index 29d1d01bf..de1822319 100644 --- a/frontend/src/components/Shell/__tests__/newChatPolicy.test.js +++ b/frontend/src/components/Shell/__tests__/newChatPolicy.test.js @@ -1,5 +1,6 @@ import { test } from 'node:test' import assert from 'node:assert/strict' +import { readFileSync } from 'node:fs' import { addCreatedChatToList, @@ -7,7 +8,16 @@ import { currentReusableEmptyChat, detailIsUntouchedEmptyChat, enteredEmptySingleScreen, + mergeChatListWithCreatedGuards, + reconcileCreatedChatGuard, + rememberCreatedChat, + reusableChatDetailVerdict, } from '../newChatPolicy.js' +import { chatQueries } from '../../../hooks/queries.js' + +const shellSource = readFileSync(new URL('../Shell.jsx', import.meta.url), 'utf8') +const queriesSource = readFileSync(new URL('../../../hooks/queries.js', import.meta.url), 'utf8') +const clientSource = readFileSync(new URL('../../../api/client.js', import.meta.url), 'utf8') const empty = (id, extra = {}) => ({ id, @@ -123,6 +133,24 @@ test('fresh detail fails closed on partial or malformed responses', () => { assert.equal(detailIsUntouchedEmptyChat(untouchedDetail({ total: '0' })), false) }) +test('fresh detail probe separates occupied, missing, and uncertain rows', () => { + assert.equal(reusableChatDetailVerdict({ + ok: true, status: 200, detail: untouchedDetail(), + }), 'empty') + assert.equal(reusableChatDetailVerdict({ + ok: true, status: 200, detail: untouchedDetail({ total: 1, messages: [{}] }), + }), 'occupied') + assert.equal(reusableChatDetailVerdict({ + ok: false, status: 404, detail: null, + }), 'missing') + assert.equal(reusableChatDetailVerdict({ + ok: false, status: 503, detail: null, + }), 'uncertain') + assert.equal(reusableChatDetailVerdict({ + ok: true, status: 200, detail: { messages: [], pending_messages: [] }, + }), 'uncertain') +}) + test('a canonical create response becomes an authoritative empty detail cache', () => { const cache = createdChatDetailCache({ id: 'new', @@ -186,6 +214,55 @@ test('a created chat enters the cache without displacing pinned chats', () => { assert.equal('detail' in result[1], false) }) +test('ordinary chat selection does not launch a competing drawer refresh', () => { + const selectChat = shellSource.match( + /function selectChat\(id\) \{([\s\S]*?)\n \}/, + )?.[1] || '' + assert.match(selectChat, /navTo\('chat', \{ chatId: id \}\)/) + assert.doesNotMatch(selectChat, /refreshChats/) +}) + +test('new-chat creation cancels stale list reads through a real AbortSignal', () => { + const cancelAt = shellSource.indexOf('await queryClient.cancelQueries({') + const createAt = shellSource.indexOf("api.chats.create({ title: 'New chat' })") + assert.ok(cancelAt >= 0 && cancelAt < createAt, + 'the stale drawer read must be cancelled before the create request') + assert.match(queriesSource, /async function fetchChats\(\{ signal \} = \{\}\)/) + assert.match(queriesSource, /api\.chats\.list\(\{ signal \}\)/) + assert.match(clientSource, /list: \(options = \{\}\) => apiFetch\('\/chats', options\)/) +}) + +test('the drawer transport aborts before creation can continue', async () => { + const originalFetch = globalThis.fetch + const sequence = [] + globalThis.fetch = (_url, options = {}) => new Promise(resolve => { + sequence.push('list-started') + options.signal?.addEventListener('abort', () => { + sequence.push('list-aborted') + // Resolve a non-success response instead of rejecting so apiFetch's + // connectivity verifier does not start unrelated background work. + resolve(new Response('[]', { + status: 499, + headers: { 'Content-Type': 'application/json' }, + })) + }, { once: true }) + }) + + try { + const controller = new AbortController() + const list = chatQueries.list.fetch({ signal: controller.signal }) + await Promise.resolve() + controller.abort() + await assert.rejects(list, /chats fetch failed: 499/) + sequence.push('create-allowed') + assert.deepEqual(sequence, [ + 'list-started', 'list-aborted', 'create-allowed', + ]) + } finally { + globalThis.fetch = originalFetch + } +}) + test('a created chat replaces a duplicate cache row', () => { const result = addCreatedChatToList([ { id: 'same', title: 'stale', pinned_at: null }, @@ -197,3 +274,96 @@ test('a created chat replaces a duplicate cache row', () => { assert.equal(result[0].title, 'Fresh') assert.equal(result[0].has_messages, true) }) + +test('a stale post-create list cannot hide the protected chat row', () => { + const guards = new Map() + const created = { + id: 'new', title: 'New chat', pinned_at: null, has_messages: false, + } + rememberCreatedChat(guards, created, { now: 1000, guardMs: 30_000 }) + + const stale = mergeChatListWithCreatedGuards([ + { id: 'older', title: 'Older', pinned_at: null }, + ], guards, { now: 2000 }) + assert.deepEqual(stale.map(chat => chat.id), ['new', 'older']) + + const confirmed = mergeChatListWithCreatedGuards([ + { id: 'new', title: 'Server title', pinned_at: null, has_messages: true }, + { id: 'older', title: 'Older', pinned_at: null }, + ], guards, { now: 3000 }) + assert.equal(confirmed[0].title, 'Server title') + + const secondFallback = mergeChatListWithCreatedGuards([ + { id: 'older', title: 'Older', pinned_at: null }, + ], guards, { now: 4000 }) + assert.equal(secondFallback[0].title, 'Server title') + + const expired = mergeChatListWithCreatedGuards([ + { id: 'older', title: 'Older', pinned_at: null }, + ], guards, { now: 31_001 }) + assert.deepEqual(expired.map(chat => chat.id), ['older']) + assert.equal(guards.size, 0) +}) + +test('detail verdicts update or retire the protected create row', () => { + const guards = new Map() + const created = { + id: 'new', title: 'New chat', pinned_at: null, has_messages: false, + } + rememberCreatedChat(guards, created, { now: 1000, guardMs: 30_000 }) + + reconcileCreatedChatGuard(guards, 'new', 'occupied') + const occupiedFallback = mergeChatListWithCreatedGuards([], guards, { + now: 2000, + }) + assert.equal(occupiedFallback[0].has_messages, true) + + reconcileCreatedChatGuard(guards, 'new', 'missing') + const missingFallback = mergeChatListWithCreatedGuards([], guards, { + now: 3000, + }) + assert.deepEqual(missingFallback, []) + assert.equal(guards.size, 0) +}) + +test('stale-present rows cannot downgrade an occupied or newer guard', () => { + const guards = new Map() + rememberCreatedChat(guards, { + id: 'new', + title: 'Created title', + pinned_at: null, + has_messages: false, + updated_at: '2026-07-22T00:00:02Z', + }, { now: 1000, guardMs: 30_000 }) + reconcileCreatedChatGuard(guards, 'new', 'occupied') + + const stalePresent = mergeChatListWithCreatedGuards([{ + id: 'new', + title: 'Older cached title', + pinned_at: null, + has_messages: false, + updated_at: '2026-07-22T00:00:01Z', + }], guards, { now: 2000 }) + assert.equal(stalePresent[0].title, 'Created title') + assert.equal(stalePresent[0].has_messages, true) + + const newerConfirmed = mergeChatListWithCreatedGuards([{ + id: 'new', + title: 'Server title', + pinned_at: null, + has_messages: true, + updated_at: '2026-07-22T00:00:03Z', + }], guards, { now: 3000 }) + assert.equal(newerConfirmed[0].title, 'Server title') + assert.equal(newerConfirmed[0].has_messages, true) + + const secondStalePresent = mergeChatListWithCreatedGuards([{ + id: 'new', + title: 'Older cached title', + pinned_at: null, + has_messages: false, + updated_at: '2026-07-22T00:00:01Z', + }], guards, { now: 4000 }) + assert.equal(secondStalePresent[0].title, 'Server title') + assert.equal(secondStalePresent[0].has_messages, true) +}) diff --git a/frontend/src/components/Shell/newChatPolicy.js b/frontend/src/components/Shell/newChatPolicy.js index 5c6c895cd..076f5e3d8 100644 --- a/frontend/src/components/Shell/newChatPolicy.js +++ b/frontend/src/components/Shell/newChatPolicy.js @@ -69,6 +69,24 @@ export function detailIsUntouchedEmptyChat(detail) { return true } +/** Classify a fresh detail probe without turning uncertainty into fake data. */ +export function reusableChatDetailVerdict({ ok, status, detail }) { + if (status === 404) return 'missing' + if (!ok) return 'uncertain' + if (detailIsUntouchedEmptyChat(detail)) return 'empty' + // A successful response is safe to call occupied only when its runtime + // shape is complete. Malformed/partial JSON is uncertainty, not evidence + // that the row has messages. + if (!detail || typeof detail !== 'object') return 'uncertain' + if (!Number.isInteger(detail.total)) return 'uncertain' + if (!Array.isArray(detail.messages)) return 'uncertain' + if (!Array.isArray(detail.pending_messages)) return 'uncertain' + if (typeof detail.running !== 'boolean') return 'uncertain' + if (!Object.hasOwn(detail, 'pending_question_id')) return 'uncertain' + if (!Object.hasOwn(detail, 'session_id')) return 'uncertain' + return 'occupied' +} + /** Convert a complete create response into ChatView's persisted cache shape. * Older/local backends that return only the historical summary fail closed and * keep the existing detail fetch path. */ @@ -126,3 +144,73 @@ export function addCreatedChatToList( ...existing.slice(insertAt), ] } + +// A NetworkFirst drawer read can fall back to the service worker's previous +// list just after POST /chats succeeds. Keep the create response protected for +// one bounded handoff window so that fallback cannot erase the new row. The +// guard is Shell-owned (not global state); an explicit delete removes it. +export const CREATED_CHAT_LIST_GUARD_MS = 30_000 + +export function rememberCreatedChat(guards, created, { + now = Date.now(), + guardMs = CREATED_CHAT_LIST_GUARD_MS, +} = {}) { + if (!guards || !created?.id) return + const row = addCreatedChatToList([], created)[0] + guards.set(String(created.id), { + row, + expiresAt: now + guardMs, + }) +} + +/** Keep the bounded create guard aligned with an authoritative detail probe. */ +export function reconcileCreatedChatGuard(guards, chatId, verdict) { + const id = String(chatId || '') + if (!id || !guards) return + if (verdict === 'missing') { + guards.delete(id) + return + } + if (verdict !== 'occupied') return + const guard = guards.get(id) + if (!guard?.row) return + guard.row = { ...guard.row, has_messages: true } +} + +export function mergeChatListWithCreatedGuards(incoming, guards, { + now = Date.now(), +} = {}) { + let merged = Array.isArray(incoming) ? incoming : [] + if (!guards?.size) return merged + for (const [id, guard] of guards) { + if (!guard || guard.expiresAt <= now) { + guards.delete(id) + continue + } + const confirmedIndex = merged.findIndex(row => String(row?.id) === id) + if (confirmedIndex >= 0) { + const confirmed = merged[confirmedIndex] + const guardedAt = Date.parse(guard.row?.updated_at || '') + const confirmedAt = Date.parse(confirmed?.updated_at || '') + const guardedIsNewer = Number.isFinite(guardedAt) + && Number.isFinite(confirmedAt) + && guardedAt > confirmedAt + const preferred = guardedIsNewer ? guard.row : confirmed + // During this short post-create window, a chat cannot become untouched + // again. Keep has_messages monotonic even when a stale-present SW row + // arrives after an authoritative detail probe or newer list response. + const reconciled = { + ...preferred, + has_messages: !!( + guard.row?.has_messages || confirmed?.has_messages + ), + } + guard.row = reconciled + merged = [...merged] + merged[confirmedIndex] = reconciled + continue + } + merged = addCreatedChatToList(merged, guard.row) + } + return merged +} diff --git a/frontend/src/hooks/queries.js b/frontend/src/hooks/queries.js index e3091e8be..dbf7383d5 100644 --- a/frontend/src/hooks/queries.js +++ b/frontend/src/hooks/queries.js @@ -98,16 +98,19 @@ function useAppsQuery() { }) } -async function fetchChats() { - const res = await api.chats.list() +async function fetchChats({ signal } = {}) { + const res = await api.chats.list({ signal }) const data = await jsonOrThrow(res, 'chats fetch failed:') return Array.isArray(data) ? data : [] } -function useChatsQuery() { +function useChatsQuery({ reconcile } = {}) { return useQuery({ queryKey: chatsKey, - queryFn: fetchChats, + queryFn: async (context) => { + const rows = await fetchChats(context) + return reconcile ? reconcile(rows) : rows + }, }) }