diff --git a/src/lib/session-load-reconcile.test.ts b/src/lib/session-load-reconcile.test.ts new file mode 100644 index 00000000..578e08b9 --- /dev/null +++ b/src/lib/session-load-reconcile.test.ts @@ -0,0 +1,34 @@ +import { test } from "node:test" +import assert from "node:assert/strict" +import { isColdSessionLoad, isLiveEventForSession } from "./session-load-reconcile.ts" + +test("isColdSessionLoad: no session shown yet -> cold (needs the spinner)", () => { + assert.equal(isColdSessionLoad(undefined, "s1"), true) + assert.equal(isColdSessionLoad(null, "s1"), true) +}) + +test("isColdSessionLoad: switching to a different session -> cold", () => { + assert.equal(isColdSessionLoad("s1", "s2"), true) +}) + +test("isColdSessionLoad: re-selecting the session already on screen -> not cold (background refresh)", () => { + assert.equal(isColdSessionLoad("s1", "s1"), false) +}) + +test("isLiveEventForSession: matching ids -> true", () => { + assert.equal(isLiveEventForSession("s1", "s1"), true) +}) + +test("isLiveEventForSession: mismatched ids -> false", () => { + assert.equal(isLiveEventForSession("s1", "s2"), false) +}) + +test("isLiveEventForSession: missing event session id -> false", () => { + assert.equal(isLiveEventForSession(undefined, "s1"), false) + assert.equal(isLiveEventForSession(null, "s1"), false) +}) + +test("isLiveEventForSession: missing active session id -> false", () => { + assert.equal(isLiveEventForSession("s1", undefined), false) + assert.equal(isLiveEventForSession("s1", null), false) +}) diff --git a/src/lib/session-load-reconcile.ts b/src/lib/session-load-reconcile.ts new file mode 100644 index 00000000..9c9df55c --- /dev/null +++ b/src/lib/session-load-reconcile.ts @@ -0,0 +1,44 @@ +/** + * Decide whether selecting `targetSessionID` for the session detail screen + * is a "cold" load — nothing is shown yet, or we're switching to a + * different session, so the blocking spinner is appropriate — or a + * background refresh of a session that's already loaded and rendering. + * + * Why this exists (issue #150): the session detail screen re-runs + * `selectSession` on every navigation focus (see #121's useFocusEffect + * resync, which re-binds this screen to its session on every re-entry, not + * just mount — since the native stack keeps screens mounted underneath a + * pushed one). That re-fetch is a good idea (it recovers from missed SSE + * events), but unconditionally flipping `isLoading` back to `true` for it + * hides the ENTIRE conversation — messages, composer, everything — behind a + * spinner for as long as the redundant fetch takes. Meanwhile SSE keeps + * delivering `message.updated`/`message.part.updated` events the whole + * time — the store keeps them, but the screen can't show them while + * `isLoading` is blocking the message list. If that redundant fetch is slow + * or stalls (flaky mobile network), the conversation looks permanently stuck + * "loading" until the user backs out to the sessions list and re-enters — + * which works only because it's a fresh attempt that (usually) doesn't hit + * the same stall, not because anything was actually fixed. + * + * A cold load (no session shown yet, or a genuinely different session) still + * needs the spinner — there's nothing to show meanwhile. A refresh of the + * session already on screen does not: keep showing what's there (which + * live SSE updates keep current) while the refetch runs quietly in the + * background. + */ +export function isColdSessionLoad(currentSessionID: string | null | undefined, targetSessionID: string): boolean { + return currentSessionID !== targetSessionID +} + +/** + * Decide whether an incoming SSE event's session matches the session a + * screen/store is currently bound to — the shared "is this event for me" + * check used to key live updates (messages, parts) to the active session, + * instead of e.g. matching on stale or missing ids. + */ +export function isLiveEventForSession( + eventSessionID: string | null | undefined, + activeSessionID: string | null | undefined, +): boolean { + return !!eventSessionID && !!activeSessionID && eventSessionID === activeSessionID +} diff --git a/src/stores/sessions.ts b/src/stores/sessions.ts index db228346..ccd21f33 100644 --- a/src/stores/sessions.ts +++ b/src/stores/sessions.ts @@ -6,6 +6,7 @@ import { addBreadcrumb } from "../lib/sentry" import { AnalyticsEvent, track } from "../lib/analytics" import { extractPromptFromParts, type PromptFromParts } from "../lib/prompt-from-parts" import { mergeIncomingMessage } from "../lib/message-merge" +import { isColdSessionLoad, isLiveEventForSession } from "../lib/session-load-reconcile" // Helper to convert API response to our internal format function parseMessages(response: MessageWithParts[]): { messages: Message[]; parts: Record } { @@ -128,10 +129,21 @@ export const useSessions = create((set, get) => ({ const seq = ++selectSeq addBreadcrumb({ category: "session", message: "select", data: { sessionID, hasDirectory: Boolean(directory) } }) + // Re-selecting the session already shown on screen (e.g. #121's + // useFocusEffect resync firing again on re-entry) is a background + // refresh, not a cold load: the screen already has this session's + // messages, and live SSE updates keep flowing to them the whole time. + // Forcing isLoading back to true here would hide the entire + // conversation — including anything streaming in live right now — + // behind a spinner for as long as this redundant fetch takes, and if it + // stalls (flaky network), the screen looks permanently stuck "loading" + // until the user backs out and re-enters (issue #150). Only a + // genuinely new/different session needs the blocking spinner. + const isColdLoad = isColdSessionLoad(get().currentSession?.id, sessionID) try { // Reset optimistic sending — SSE sessionStatus is the source of truth set((state) => ({ - isLoading: true, + isLoading: isColdLoad ? true : state.isLoading, error: null, hasMore: false, loadingMore: false, @@ -409,9 +421,16 @@ export const useSessions = create((set, get) => ({ switch (event.type) { case "message.updated": { const message = (props.info || props.message) as Message | undefined - if (!message || message.sessionID !== currentSession.id) return + if (!message || !isLiveEventForSession(message.sessionID, currentSession.id)) return - set((state) => ({ messages: mergeIncomingMessage(state.messages, message) })) + set((state) => ({ + messages: mergeIncomingMessage(state.messages, message), + // A live update for the session on screen is proof it has content + // to show — clear any stuck spinner even if the initial (or a + // redundant re-focus) GET hasn't resolved yet, or never does + // (issue #150). Only ever clears, never sets it back to true. + isLoading: false, + })) break } @@ -431,6 +450,9 @@ export const useSessions = create((set, get) => ({ ? messageParts.map((p) => (p.id === part.id ? part : p)) : [...messageParts, part], }, + // See message.updated above — a live part update is just as + // much proof of life as a message update. + isLoading: false, } }) break @@ -453,6 +475,7 @@ export const useSessions = create((set, get) => ({ set((state) => ({ sessions: state.sessions.map((s) => (s.id === session.id ? session : s)), currentSession: state.currentSession?.id === session.id ? session : state.currentSession, + isLoading: isLiveEventForSession(session.id, state.currentSession?.id) ? false : state.isLoading, })) break }