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; +}