From e00b49bbe5a141b5b999437c56cc6521d5d7cbab Mon Sep 17 00:00:00 2001 From: Adrian Bonpin Date: Sun, 4 Oct 2026 15:34:09 +0800 Subject: [PATCH] =?UTF-8?q?fix:=20stop=20the=20chat=20view=20hanging=20on?= =?UTF-8?q?=20"Loading=20conversations=E2=80=A6"=20after=20a=20snip?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The hand-off launch deadlocked its own startup. `resumed` was initialised to `Boolean(initialSession)`, which is false for a snip, and the idle-resume effect returned early whenever there was a hand-off, so nothing ever set it. The render gate needed both `resumed` and `prepared`, so a successful snip left the view spinning and there was no way to reach the composer. Two effects guarding two booleans had already produced one bug here, so this fixes the shape rather than the value: - One startup effect and one `startup` flag, cleared in a `finally`. Every branch ends in the same place, including a thrown one, so no path can leave the view loading. - The decision tree moves to `lib/startup.ts` as a pure `resolveStartup`, which takes its storage and file reads as injected ports. It is now covered by 16 tests, including the case that broke: a hand-off must not fall through to the idle resume, and must still finish startup. - `startup.ts` joins the guard that keeps the logic layer free of Vicinae imports. - ChatView is prettier-clean; it had been carrying pre-existing drift and has been rewritten twice this session anyway. Verified: 273 tests, tsc, eslint, prettier, and vici build. --- src/__tests__/agent.test.ts | 12 +- src/__tests__/startup.test.ts | 203 ++++++++++++++++++++++++++++++++ src/lib/components/ChatView.tsx | 107 ++++++----------- src/lib/startup.ts | 96 +++++++++++++++ 4 files changed, 345 insertions(+), 73 deletions(-) create mode 100644 src/__tests__/startup.test.ts create mode 100644 src/lib/startup.ts diff --git a/src/__tests__/agent.test.ts b/src/__tests__/agent.test.ts index 22a4fb5..a79064f 100644 --- a/src/__tests__/agent.test.ts +++ b/src/__tests__/agent.test.ts @@ -165,7 +165,17 @@ describe("module wiring", () => { // These modules must stay renderer-free: they are pure logic, they are what // the unit tests cover, and nothing in them should need a Vicinae runtime. // (`sessions.ts` is deliberately excluded: it legitimately needs LocalStorage.) - for (const file of ["agent.ts", "ollama.ts", "tools.ts", "location.ts", "history.ts", "markdown.ts"]) { + for (const file of [ + "agent.ts", + "ollama.ts", + "tools.ts", + "location.ts", + "history.ts", + "markdown.ts", + "capture.ts", + "attachments.ts", + "startup.ts", + ]) { const source = readFileSync(path.resolve(__dirname, `../lib/${file}`), "utf8"); expect(source, `${file} should not import @vicinae/api`).not.toMatch(/from\s+"@vicinae\/api"/); } diff --git a/src/__tests__/startup.test.ts b/src/__tests__/startup.test.ts new file mode 100644 index 0000000..2caba86 --- /dev/null +++ b/src/__tests__/startup.test.ts @@ -0,0 +1,203 @@ +import { describe, expect, it, vi } from "vitest"; +import { isHandoff, resolveStartup, type StartupPorts } from "../lib/startup"; +import type { PendingAttachment, Session, SessionSummary } from "../lib/types"; + +/** + * The chat view's startup is the part that kept breaking: three asynchronous + * paths (a pushed session, a handed-over capture, an idle resume) that all have + * to end in a rendered chat. Two of those bugs shipped, so the decision tree is + * tested here rather than discovered from a screenshot of a spinner. + */ + +const NOW = 1_000_000; +const HOUR = 3_600_000; + +function session(id: string, updatedAt: number): Session { + return { id, title: id, model: "m", createdAt: updatedAt, updatedAt, turns: [] }; +} + +function summary(id: string, updatedAt: number): SessionSummary { + return { id, title: id, model: "m", createdAt: updatedAt, updatedAt, turnCount: 0, preview: "" }; +} + +function attachment(path: string): PendingAttachment { + return { path, mime: "image/png", base64: "AAAA" }; +} + +/** Ports that record what was asked for, so "did not resume" is checkable. */ +function ports(overrides: Partial = {}) { + const loadSession = vi.fn(async (id: string) => session(id, NOW)); + const listSessions = vi.fn(async () => [summary("latest", NOW)]); + const readAttachment = vi.fn(async (path: string) => attachment(path)); + + return { + loadSession, + listSessions, + readAttachment, + ...overrides, + } satisfies StartupPorts; +} + +function input(overrides: Partial[0]> = {}) { + return { + hasInitialSession: false, + now: NOW, + ...overrides, + }; +} + +describe("isHandoff", () => { + it("is true when a file was handed over", () => { + expect(isHandoff({ initialAttachmentPath: "/tmp/snip.png" })).toBe(true); + }); + + it("is true when a conversation was named", () => { + expect(isHandoff({ initialSessionId: "abc" })).toBe(true); + }); + + it("is false on an ordinary open", () => { + expect(isHandoff({})).toBe(false); + }); +}); + +describe("resolveStartup", () => { + describe("a snip hand-off", () => { + it("reads the attachment and restores the named conversation", async () => { + const p = ports(); + + const result = await resolveStartup( + input({ initialSessionId: "abc", initialAttachmentPath: "/tmp/snip.png" }), + p, + ); + + expect(result.session?.id).toBe("abc"); + expect(result.attachment?.path).toBe("/tmp/snip.png"); + }); + + it("does not fall through to the idle resume", async () => { + // The bug this file exists for: a hand-off has to finish startup even + // though it took the resume path off the table. Guarding the resume with + // "and there was no hand-off" while forgetting to clear the readiness flag + // left the view on its loading row forever. + const p = ports(); + + await resolveStartup(input({ initialAttachmentPath: "/tmp/snip.png" }), p); + + expect(p.listSessions).not.toHaveBeenCalled(); + }); + + it("does not attach the most recent conversation's session", async () => { + const p = ports(); + + const result = await resolveStartup(input({ initialAttachmentPath: "/tmp/snip.png" }), p); + + expect(result.session).toBeUndefined(); + }); + + it("still resolves when the named conversation has been deleted", async () => { + // A snip started from a conversation someone has since deleted should mean + // a fresh chat with the image, not a spinner or a crash. + const p = ports({ loadSession: vi.fn(async () => undefined) }); + + const result = await resolveStartup( + input({ initialSessionId: "gone", initialAttachmentPath: "/tmp/snip.png" }), + p, + ); + + expect(result.session).toBeUndefined(); + expect(result.attachment?.path).toBe("/tmp/snip.png"); + }); + + it("reports an unreadable attachment without refusing to open", async () => { + const p = ports({ + readAttachment: vi.fn(async () => { + throw new Error("ENOENT: no such file"); + }), + }); + + const result = await resolveStartup(input({ initialAttachmentPath: "/tmp/gone.png" }), p); + + expect(result.attachment).toBeUndefined(); + expect(result.attachmentError).toContain("ENOENT"); + }); + + it("survives a storage failure while loading the named conversation", async () => { + const p = ports({ + loadSession: vi.fn(async () => { + throw new Error("storage exploded"); + }), + }); + + const result = await resolveStartup(input({ initialSessionId: "abc" }), p); + + expect(result.session).toBeUndefined(); + }); + }); + + describe("an ordinary open", () => { + it("resumes the most recent conversation when it is fresh", async () => { + const p = ports({ listSessions: vi.fn(async () => [summary("latest", NOW - 60_000)]) }); + + const result = await resolveStartup(input({ resumeTimeoutMs: HOUR }), p); + + expect(result.session?.id).toBe("latest"); + expect(p.loadSession).toHaveBeenCalledWith("latest"); + }); + + it("starts fresh when the newest conversation is older than the window", async () => { + const p = ports({ listSessions: vi.fn(async () => [summary("latest", NOW - 2 * HOUR)]) }); + + const result = await resolveStartup(input({ resumeTimeoutMs: HOUR }), p); + + expect(result.session).toBeUndefined(); + expect(p.loadSession).not.toHaveBeenCalled(); + }); + + it("treats exactly the window as fresh", async () => { + const p = ports({ listSessions: vi.fn(async () => [summary("latest", NOW - HOUR)]) }); + + const result = await resolveStartup(input({ resumeTimeoutMs: HOUR }), p); + + expect(result.session?.id).toBe("latest"); + }); + + it("always resumes when the preference is 'never time out'", async () => { + const p = ports({ listSessions: vi.fn(async () => [summary("latest", NOW - 400 * HOUR)]) }); + + const result = await resolveStartup(input({ resumeTimeoutMs: undefined }), p); + + expect(result.session?.id).toBe("latest"); + }); + + it("starts fresh when there are no conversations at all", async () => { + const p = ports({ listSessions: vi.fn(async () => []) }); + + const result = await resolveStartup(input({ resumeTimeoutMs: HOUR }), p); + + expect(result).toEqual({}); + expect(p.loadSession).not.toHaveBeenCalled(); + }); + + it("starts fresh rather than failing when the index cannot be read", async () => { + const p = ports({ + listSessions: vi.fn(async () => { + throw new Error("corrupt index"); + }), + }); + + const result = await resolveStartup(input({ resumeTimeoutMs: HOUR }), p); + + expect(result).toEqual({}); + }); + + it("does not resume when a session was pushed in by hand", async () => { + // The sessions command already knows which conversation to show. + const p = ports(); + + await resolveStartup(input({ hasInitialSession: true }), p); + + expect(p.listSessions).not.toHaveBeenCalled(); + expect(p.loadSession).not.toHaveBeenCalled(); + }); + }); +}); diff --git a/src/lib/components/ChatView.tsx b/src/lib/components/ChatView.tsx index efc14dd..522d4e1 100644 --- a/src/lib/components/ChatView.tsx +++ b/src/lib/components/ChatView.tsx @@ -6,9 +6,10 @@ import { useVisionSupport } from "../hooks/useVision"; import { buildThreadMarkdown, type TranscriptOptions } from "../markdown"; import { getLamPreferences } from "../preferences"; import { deleteSession, listSessions, loadSession, summarizeSession } from "../sessions"; +import { resolveStartup } from "../startup"; import { buildThread } from "../thread"; import type { PendingAttachment, Session, SessionSummary } from "../types"; -import { errorMessage, recencyGroup, truncate } from "../utils"; +import { recencyGroup, truncate } from "../utils"; /** * The chat surface: conversations on the left, the open conversation on the right. @@ -92,13 +93,10 @@ export function ChatView({ const { replaceSession } = chat; const { canSeeImages } = useVisionSupport(preferences); - /** A launch that hands something over has to wait for it before it can render. */ - const needsPreparation = Boolean(initialSessionId) || Boolean(initialAttachmentPath); - const [input, setInput] = useState(initialPrompt ?? ""); const [summaries, setSummaries] = useState(undefined); - const [prepared, setPrepared] = useState(!needsPreparation); - const [resumed, setResumed] = useState(Boolean(initialSession)); + // One flag, not two. See the startup effect below for why that matters. + const [startup, setStartup] = useState<"loading" | "ready">(initialSession ? "ready" : "loading"); // A captured image is held here until the user says what they want to know // about it. Nothing is sent to the model on capture alone. const [pending, setPending] = useState(undefined); @@ -118,76 +116,47 @@ export function ChatView({ }, [reload, chat.session.updatedAt]); /** - * Restores whatever the launching command handed over. + * Everything that has to happen before the first render, in one place. * - * This is the second half of the snip handoff: the capture command closed the - * window to take the shot, so it comes back here with a file to read and, when - * the snip was started from a conversation, the id of the conversation to - * return to. Reloading by id rather than trusting "most recent" matters - * because the snip itself is not a turn, so nothing was saved in between. + * This used to be two effects guarding two booleans, and a hand-off launch + * satisfied the guard of *both* while setting neither, so `resumed` stayed + * false and the view sat on "Loading conversations…" forever. Anything that + * blocks the first render belongs in one effect with one flag, and the flag is + * cleared in a `finally` so that no branch, including a thrown one, can leave + * the view spinning. */ useEffect(() => { - if (!needsPreparation) return; - let cancelled = false; - void (async () => { - if (initialSessionId) { - const session = await loadSession(initialSessionId); - if (!cancelled && session) replaceSession(session); - } - - if (initialAttachmentPath) { - try { - const attachment = await readAttachment(initialAttachmentPath); - if (!cancelled) setPending(attachment); - } catch (thrown) { - if (!cancelled) setSnipNote(`**Could not open the screenshot:** ${errorMessage(thrown)}`); - } - } - - if (!cancelled) setPrepared(true); - })(); - - return () => { - cancelled = true; - }; - }, [needsPreparation, initialSessionId, initialAttachmentPath, replaceSession]); - - /** - * Reopening the command continues the most recent conversation, but only if it - * was used recently. Past the configured idle window it starts a fresh chat - * instead. The previous conversation is never removed, it just drops down the - * sidebar. Setting the preference to "Never" keeps the last chat open forever. - * - * A hand-off launch opts out: it either names the conversation to return to or - * deliberately starts a new one, and quietly continuing some other chat would - * bury the screenshot in an unrelated thread. - */ - useEffect(() => { - if (initialSession || needsPreparation) return; - - let cancelled = false; void (async () => { try { - const [latest] = await listSessions(); - if (!latest) return; + const outcome = await resolveStartup( + { + hasInitialSession: Boolean(initialSession), + initialSessionId, + initialAttachmentPath, + resumeTimeoutMs: preferences.resumeTimeoutMs, + now: Date.now(), + }, + { loadSession, listSessions, readAttachment }, + ); - const idleMs = Date.now() - latest.updatedAt; - const { resumeTimeoutMs } = preferences; - if (resumeTimeoutMs !== undefined && idleMs > resumeTimeoutMs) return; + if (cancelled) return; - const session = await loadSession(latest.id); - if (!cancelled && session) replaceSession(session); + if (outcome.session) replaceSession(outcome.session); + if (outcome.attachment) setPending(outcome.attachment); + if (outcome.attachmentError) { + setSnipNote(`**Could not open the screenshot:** ${outcome.attachmentError}`); + } } finally { - if (!cancelled) setResumed(true); + if (!cancelled) setStartup("ready"); } })(); return () => { cancelled = true; }; - }, [initialSession, needsPreparation, preferences, replaceSession]); + }, [initialSession, initialSessionId, initialAttachmentPath, preferences.resumeTimeoutMs, replaceSession]); const thread = useMemo( () => @@ -213,8 +182,8 @@ export function ChatView({ * already looking. */ const threadMarkdown = useMemo(() => { - const notes = [launchNote, snipNote, pending ? PENDING_NOTE : undefined].filter( - (note): note is string => Boolean(note), + const notes = [launchNote, snipNote, pending ? PENDING_NOTE : undefined].filter((note): note is string => + Boolean(note), ); const transcript = @@ -237,10 +206,7 @@ export function ChatView({ if (base.some((summary) => summary.id === activeId)) return mapped; const active = summarizeSession(chat.session); - return [ - { id: active.id, title: active.title, updatedAt: active.updatedAt, isActive: true }, - ...mapped, - ]; + return [{ id: active.id, title: active.title, updatedAt: active.updatedAt, isActive: true }, ...mapped]; }, [summaries, activeId, chat.session]); /** @@ -282,8 +248,7 @@ export function ChatView({ /** The snip a "show me the screenshot" action would open: the pending one, else the newest. */ const latestAttachment = - pending ?? - [...chat.session.turns].reverse().find((turn) => turn.attachments.length > 0)?.attachments[0]; + pending ?? [...chat.session.turns].reverse().find((turn) => turn.attachments.length > 0)?.attachments[0]; const send = useCallback( (text: string) => { @@ -382,13 +347,11 @@ export function ChatView({ ); }; - if (!resumed || !prepared || summaries === undefined) { + if (startup === "loading" || summaries === undefined) { return ; } - const placeholder = chat.status - ? `${chat.status}. Press ⏎ to stop` - : "Lam is thinking. Press ⏎ to stop"; + const placeholder = chat.status ? `${chat.status}. Press ⏎ to stop` : "Lam is thinking. Press ⏎ to stop"; const idlePlaceholder = pending ? "Ask about the screenshot…" : "Ask Lam anything…"; diff --git a/src/lib/startup.ts b/src/lib/startup.ts new file mode 100644 index 0000000..dd11776 --- /dev/null +++ b/src/lib/startup.ts @@ -0,0 +1,96 @@ +import type { PendingAttachment, Session, SessionSummary } from "./types"; +import { errorMessage } from "./utils"; + +/** + * Everything the chat view resolves before its first render. + * + * Pulled out of the component and made pure because this is the part that keeps + * going wrong. Three things can happen on open, and all of them are + * asynchronous: a launch hands over a captured file, it may also name the + * conversation to return to, and an ordinary open resumes the most recent + * conversation if it is fresh enough. Every path has to end in a rendered chat. + * + * As a plain function it is testable without mounting anything, which matters + * because the alternative is finding out from a screenshot that a view is + * spinning forever. + * + * Imports nothing from Vicinae, so it belongs to the logic layer. + */ + +export interface StartupInput { + /** A conversation already in hand, which is how the sessions command pushes this view. */ + hasInitialSession: boolean; + /** A conversation to reopen by id, handed over by the snip command. */ + initialSessionId?: string; + /** A captured image to read in. */ + initialAttachmentPath?: string; + /** Idle time past which an ordinary open starts fresh. Undefined means never time out. */ + resumeTimeoutMs?: number; + /** Injected so the resume window is deterministic under test. */ + now: number; +} + +/** The side effects, injected so nothing here touches storage or the file system directly. */ +export interface StartupPorts { + loadSession: (id: string) => Promise; + listSessions: () => Promise; + readAttachment: (path: string) => Promise; +} + +export interface StartupResult { + /** The conversation to show, when one was restored. */ + session?: Session; + /** The image that was handed over, when one could be read. */ + attachment?: PendingAttachment; + /** Why a handed-over image could not be read, so the view can say so. */ + attachmentError?: string; +} + +/** True when the launch handed something over, which also means "do not resume". */ +export function isHandoff(input: Pick): boolean { + return Boolean(input.initialSessionId || input.initialAttachmentPath); +} + +export async function resolveStartup(input: StartupInput, ports: StartupPorts): Promise { + const result: StartupResult = {}; + + if (input.initialSessionId) { + // A hand-off names the conversation to return to, so that one is restored + // rather than whichever chat happens to be most recent. By id matters + // because taking a snip is not a turn, so nothing was saved in between. + // A session that has since been deleted just means starting fresh. + result.session = await ports.loadSession(input.initialSessionId).catch(() => undefined); + } + + if (input.initialAttachmentPath) { + try { + result.attachment = await ports.readAttachment(input.initialAttachmentPath); + } catch (thrown) { + // A missing file leaves an empty chat with an explanation, which beats + // refusing to open at all. + result.attachmentError = errorMessage(thrown); + } + } + + // Nothing was handed over, so the idle-resume rule applies: reopening the + // command continues the most recent conversation, but only if it was used + // recently. The previous conversation is never removed either way, it just + // drops down the sidebar. + if (!input.hasInitialSession && !isHandoff(input)) { + const latest = await ports + .listSessions() + .then((sessions) => sessions[0]) + .catch(() => undefined); + + if (latest) { + const idleMs = input.now - latest.updatedAt; + const fresh = input.resumeTimeoutMs === undefined || idleMs <= input.resumeTimeoutMs; + + if (fresh) { + result.session = await ports.loadSession(latest.id).catch(() => undefined); + } + } + } + + return result; +}