From d1ebe0767c264d395c8bc504c0957ccc3af90103 Mon Sep 17 00:00:00 2001 From: Adam <2363879+adamdotdevin@users.noreply.github.com> Date: Sun, 8 Feb 2026 05:02:19 -0600 Subject: [PATCH] chore: refactoring and tests (#12629) --- packages/app/playwright.config.ts | 2 +- packages/app/script/e2e-local.ts | 120 +++++--- .../context/global-sync/event-reducer.test.ts | 283 +++++++++++++++++- .../app/src/context/layout-scroll.test.ts | 66 ++-- packages/app/src/pages/directory-layout.tsx | 5 +- packages/app/src/pages/layout/deep-links.ts | 10 +- packages/app/src/pages/layout/helpers.test.ts | 29 ++ packages/app/src/pages/layout/helpers.ts | 7 +- packages/app/src/pages/session.tsx | 10 +- .../app/src/pages/session/helpers.test.ts | 12 +- packages/app/src/pages/session/helpers.ts | 7 + packages/app/src/utils/persist.test.ts | 102 +++++++ packages/app/src/utils/persist.ts | 48 +-- packages/app/src/utils/server-health.test.ts | 74 ++++- packages/app/src/utils/server-health.ts | 72 ++++- packages/opencode/src/file/ripgrep.ts | 10 +- 16 files changed, 744 insertions(+), 113 deletions(-) create mode 100644 packages/app/src/utils/persist.test.ts diff --git a/packages/app/playwright.config.ts b/packages/app/playwright.config.ts index 10819e69f..ea85829e0 100644 --- a/packages/app/playwright.config.ts +++ b/packages/app/playwright.config.ts @@ -14,7 +14,7 @@ export default defineConfig({ expect: { timeout: 10_000, }, - fullyParallel: true, + fullyParallel: process.env.PLAYWRIGHT_FULLY_PARALLEL === "1", forbidOnly: !!process.env.CI, retries: process.env.CI ? 2 : 0, reporter: [["html", { outputFolder: "e2e/playwright-report", open: "never" }], ["line"]], diff --git a/packages/app/script/e2e-local.ts b/packages/app/script/e2e-local.ts index df2107f76..96b35cfb7 100644 --- a/packages/app/script/e2e-local.ts +++ b/packages/app/script/e2e-local.ts @@ -55,6 +55,7 @@ const extraArgs = (() => { const [serverPort, webPort] = await Promise.all([freePort(), freePort()]) const sandbox = await fs.mkdtemp(path.join(os.tmpdir(), "opencode-e2e-")) +const keepSandbox = process.env.OPENCODE_E2E_KEEP_SANDBOX === "1" const serverEnv = { ...process.env, @@ -83,58 +84,99 @@ const runnerEnv = { PLAYWRIGHT_PORT: String(webPort), } satisfies Record -const seed = Bun.spawn(["bun", "script/seed-e2e.ts"], { - cwd: opencodeDir, - env: serverEnv, - stdout: "inherit", - stderr: "inherit", -}) +let seed: ReturnType | undefined +let runner: ReturnType | undefined +let server: { stop: () => Promise | void } | undefined +let inst: { Instance: { disposeAll: () => Promise | void } } | undefined +let cleaned = false +let internalError = false -const seedExit = await seed.exited -if (seedExit !== 0) { - process.exit(seedExit) +const cleanup = async () => { + if (cleaned) return + cleaned = true + + if (seed && seed.exitCode === null) seed.kill("SIGTERM") + if (runner && runner.exitCode === null) runner.kill("SIGTERM") + + const jobs = [ + inst?.Instance.disposeAll(), + server?.stop(), + keepSandbox ? undefined : fs.rm(sandbox, { recursive: true, force: true }), + ].filter(Boolean) + await Promise.allSettled(jobs) } -Object.assign(process.env, serverEnv) -process.env.AGENT = "1" -process.env.OPENCODE = "1" +const shutdown = (code: number, reason: string) => { + process.exitCode = code + void cleanup().finally(() => { + console.error(`e2e-local shutdown: ${reason}`) + process.exit(code) + }) +} -const log = await import("../../opencode/src/util/log") -const install = await import("../../opencode/src/installation") -await log.Log.init({ - print: true, - dev: install.Installation.isLocal(), - level: "WARN", +const reportInternalError = (reason: string, error: unknown) => { + internalError = true + console.error(`e2e-local internal error: ${reason}`) + console.error(error) +} + +process.once("SIGINT", () => shutdown(130, "SIGINT")) +process.once("SIGTERM", () => shutdown(143, "SIGTERM")) +process.once("SIGHUP", () => shutdown(129, "SIGHUP")) +process.once("uncaughtException", (error) => { + reportInternalError("uncaughtException", error) +}) +process.once("unhandledRejection", (error) => { + reportInternalError("unhandledRejection", error) }) -const servermod = await import("../../opencode/src/server/server") -const inst = await import("../../opencode/src/project/instance") -const server = servermod.Server.listen({ port: serverPort, hostname: "127.0.0.1" }) -console.log(`opencode server listening on http://127.0.0.1:${serverPort}`) +let code = 1 + +try { + seed = Bun.spawn(["bun", "script/seed-e2e.ts"], { + cwd: opencodeDir, + env: serverEnv, + stdout: "inherit", + stderr: "inherit", + }) + + const seedExit = await seed.exited + if (seedExit !== 0) { + code = seedExit + } else { + Object.assign(process.env, serverEnv) + process.env.AGENT = "1" + process.env.OPENCODE = "1" + + const log = await import("../../opencode/src/util/log") + const install = await import("../../opencode/src/installation") + await log.Log.init({ + print: true, + dev: install.Installation.isLocal(), + level: "WARN", + }) + + const servermod = await import("../../opencode/src/server/server") + inst = await import("../../opencode/src/project/instance") + server = servermod.Server.listen({ port: serverPort, hostname: "127.0.0.1" }) + console.log(`opencode server listening on http://127.0.0.1:${serverPort}`) -const result = await (async () => { - try { await waitForHealth(`http://127.0.0.1:${serverPort}/global/health`) - - const runner = Bun.spawn(["bun", "test:e2e", ...extraArgs], { + runner = Bun.spawn(["bun", "test:e2e", ...extraArgs], { cwd: appDir, env: runnerEnv, stdout: "inherit", stderr: "inherit", }) - - return { code: await runner.exited } - } catch (error) { - return { error } - } finally { - await inst.Instance.disposeAll() - await server.stop() + code = await runner.exited } -})() - -if ("error" in result) { - console.error(result.error) - process.exit(1) +} catch (error) { + console.error(error) + code = 1 +} finally { + await cleanup() } -process.exit(result.code) +if (code === 0 && internalError) code = 1 + +process.exit(code) diff --git a/packages/app/src/context/global-sync/event-reducer.test.ts b/packages/app/src/context/global-sync/event-reducer.test.ts index f79b9fc95..ad63f3c20 100644 --- a/packages/app/src/context/global-sync/event-reducer.test.ts +++ b/packages/app/src/context/global-sync/event-reducer.test.ts @@ -1,5 +1,5 @@ import { describe, expect, test } from "bun:test" -import type { Message, Part, Project, Session } from "@opencode-ai/sdk/v2/client" +import type { Message, Part, PermissionRequest, Project, QuestionRequest, Session } from "@opencode-ai/sdk/v2/client" import { createStore } from "solid-js/store" import type { State } from "./types" import { applyDirectoryEvent, applyGlobalEvent } from "./event-reducer" @@ -34,6 +34,29 @@ const textPart = (id: string, sessionID: string, messageID: string) => text: id, }) as Part +const permissionRequest = (id: string, sessionID: string, title = id) => + ({ + id, + sessionID, + permission: title, + patterns: ["*"], + metadata: {}, + always: [], + }) as PermissionRequest + +const questionRequest = (id: string, sessionID: string, title = id) => + ({ + id, + sessionID, + questions: [ + { + question: title, + header: title, + options: [{ label: title, description: title }], + }, + ], + }) as QuestionRequest + const baseState = (input: Partial = {}) => ({ status: "complete", @@ -164,6 +187,264 @@ describe("applyDirectoryEvent", () => { expect(store.session_status.ses_1).toBeUndefined() }) + test("cleans session caches when deleted and decrements only root totals", () => { + const cases = [ + { info: rootSession({ id: "ses_1" }), expectedTotal: 1 }, + { info: rootSession({ id: "ses_2", parentID: "ses_1" }), expectedTotal: 2 }, + ] + + for (const item of cases) { + const message = userMessage("msg_1", item.info.id) + const [store, setStore] = createStore( + baseState({ + session: [ + rootSession({ id: "ses_1" }), + rootSession({ id: "ses_2", parentID: "ses_1" }), + rootSession({ id: "ses_3" }), + ], + sessionTotal: 2, + message: { [item.info.id]: [message] }, + part: { [message.id]: [textPart("prt_1", item.info.id, message.id)] }, + session_diff: { [item.info.id]: [] }, + todo: { [item.info.id]: [] }, + permission: { [item.info.id]: [] }, + question: { [item.info.id]: [] }, + session_status: { [item.info.id]: { type: "busy" } }, + }), + ) + + applyDirectoryEvent({ + event: { type: "session.deleted", properties: { info: item.info } }, + store, + setStore, + push() {}, + directory: "/tmp", + loadLsp() {}, + }) + + expect(store.session.find((x) => x.id === item.info.id)).toBeUndefined() + expect(store.sessionTotal).toBe(item.expectedTotal) + expect(store.message[item.info.id]).toBeUndefined() + expect(store.part[message.id]).toBeUndefined() + expect(store.session_diff[item.info.id]).toBeUndefined() + expect(store.todo[item.info.id]).toBeUndefined() + expect(store.permission[item.info.id]).toBeUndefined() + expect(store.question[item.info.id]).toBeUndefined() + expect(store.session_status[item.info.id]).toBeUndefined() + } + }) + + test("upserts and removes messages while clearing orphaned parts", () => { + const sessionID = "ses_1" + const [store, setStore] = createStore( + baseState({ + message: { [sessionID]: [userMessage("msg_1", sessionID), userMessage("msg_3", sessionID)] }, + part: { msg_2: [textPart("prt_1", sessionID, "msg_2")] }, + }), + ) + + applyDirectoryEvent({ + event: { type: "message.updated", properties: { info: userMessage("msg_2", sessionID) } }, + store, + setStore, + push() {}, + directory: "/tmp", + loadLsp() {}, + }) + + expect(store.message[sessionID]?.map((x) => x.id)).toEqual(["msg_1", "msg_2", "msg_3"]) + + applyDirectoryEvent({ + event: { + type: "message.updated", + properties: { + info: { + ...userMessage("msg_2", sessionID), + role: "assistant", + } as Message, + }, + }, + store, + setStore, + push() {}, + directory: "/tmp", + loadLsp() {}, + }) + + expect(store.message[sessionID]?.find((x) => x.id === "msg_2")?.role).toBe("assistant") + + applyDirectoryEvent({ + event: { type: "message.removed", properties: { sessionID, messageID: "msg_2" } }, + store, + setStore, + push() {}, + directory: "/tmp", + loadLsp() {}, + }) + + expect(store.message[sessionID]?.map((x) => x.id)).toEqual(["msg_1", "msg_3"]) + expect(store.part.msg_2).toBeUndefined() + }) + + test("upserts and prunes message parts", () => { + const sessionID = "ses_1" + const messageID = "msg_1" + const [store, setStore] = createStore( + baseState({ + part: { [messageID]: [textPart("prt_1", sessionID, messageID), textPart("prt_3", sessionID, messageID)] }, + }), + ) + + applyDirectoryEvent({ + event: { type: "message.part.updated", properties: { part: textPart("prt_2", sessionID, messageID) } }, + store, + setStore, + push() {}, + directory: "/tmp", + loadLsp() {}, + }) + expect(store.part[messageID]?.map((x) => x.id)).toEqual(["prt_1", "prt_2", "prt_3"]) + + applyDirectoryEvent({ + event: { + type: "message.part.updated", + properties: { + part: { + ...textPart("prt_2", sessionID, messageID), + text: "changed", + } as Part, + }, + }, + store, + setStore, + push() {}, + directory: "/tmp", + loadLsp() {}, + }) + const updated = store.part[messageID]?.find((x) => x.id === "prt_2") + expect(updated?.type).toBe("text") + if (updated?.type === "text") expect(updated.text).toBe("changed") + + applyDirectoryEvent({ + event: { type: "message.part.removed", properties: { messageID, partID: "prt_1" } }, + store, + setStore, + push() {}, + directory: "/tmp", + loadLsp() {}, + }) + applyDirectoryEvent({ + event: { type: "message.part.removed", properties: { messageID, partID: "prt_2" } }, + store, + setStore, + push() {}, + directory: "/tmp", + loadLsp() {}, + }) + applyDirectoryEvent({ + event: { type: "message.part.removed", properties: { messageID, partID: "prt_3" } }, + store, + setStore, + push() {}, + directory: "/tmp", + loadLsp() {}, + }) + + expect(store.part[messageID]).toBeUndefined() + }) + + test("tracks permission and question request lifecycles", () => { + const sessionID = "ses_1" + const [store, setStore] = createStore( + baseState({ + permission: { [sessionID]: [permissionRequest("perm_1", sessionID), permissionRequest("perm_3", sessionID)] }, + question: { [sessionID]: [questionRequest("q_1", sessionID), questionRequest("q_3", sessionID)] }, + }), + ) + + applyDirectoryEvent({ + event: { type: "permission.asked", properties: permissionRequest("perm_2", sessionID) }, + store, + setStore, + push() {}, + directory: "/tmp", + loadLsp() {}, + }) + expect(store.permission[sessionID]?.map((x) => x.id)).toEqual(["perm_1", "perm_2", "perm_3"]) + + applyDirectoryEvent({ + event: { type: "permission.asked", properties: permissionRequest("perm_2", sessionID, "updated") }, + store, + setStore, + push() {}, + directory: "/tmp", + loadLsp() {}, + }) + expect(store.permission[sessionID]?.find((x) => x.id === "perm_2")?.permission).toBe("updated") + + applyDirectoryEvent({ + event: { type: "permission.replied", properties: { sessionID, requestID: "perm_2" } }, + store, + setStore, + push() {}, + directory: "/tmp", + loadLsp() {}, + }) + expect(store.permission[sessionID]?.map((x) => x.id)).toEqual(["perm_1", "perm_3"]) + + applyDirectoryEvent({ + event: { type: "question.asked", properties: questionRequest("q_2", sessionID) }, + store, + setStore, + push() {}, + directory: "/tmp", + loadLsp() {}, + }) + expect(store.question[sessionID]?.map((x) => x.id)).toEqual(["q_1", "q_2", "q_3"]) + + applyDirectoryEvent({ + event: { type: "question.asked", properties: questionRequest("q_2", sessionID, "updated") }, + store, + setStore, + push() {}, + directory: "/tmp", + loadLsp() {}, + }) + expect(store.question[sessionID]?.find((x) => x.id === "q_2")?.questions[0]?.header).toBe("updated") + + applyDirectoryEvent({ + event: { type: "question.rejected", properties: { sessionID, requestID: "q_2" } }, + store, + setStore, + push() {}, + directory: "/tmp", + loadLsp() {}, + }) + expect(store.question[sessionID]?.map((x) => x.id)).toEqual(["q_1", "q_3"]) + }) + + test("updates vcs branch in store and cache", () => { + const [store, setStore] = createStore(baseState()) + const [cacheStore, setCacheStore] = createStore({ value: undefined as State["vcs"] }) + + applyDirectoryEvent({ + event: { type: "vcs.branch.updated", properties: { branch: "feature/test" } }, + store, + setStore, + push() {}, + directory: "/tmp", + loadLsp() {}, + vcsCache: { + store: cacheStore, + setStore: setCacheStore, + ready: () => true, + }, + }) + + expect(store.vcs).toEqual({ branch: "feature/test" }) + expect(cacheStore.value).toEqual({ branch: "feature/test" }) + }) + test("routes disposal and lsp events to side-effect handlers", () => { const [store, setStore] = createStore(baseState()) const pushes: string[] = [] diff --git a/packages/app/src/context/layout-scroll.test.ts b/packages/app/src/context/layout-scroll.test.ts index c421a58b6..2a13e4020 100644 --- a/packages/app/src/context/layout-scroll.test.ts +++ b/packages/app/src/context/layout-scroll.test.ts @@ -1,36 +1,44 @@ -import { describe, expect, test } from "bun:test" +import { describe, expect, test, vi } from "bun:test" import { createScrollPersistence } from "./layout-scroll" describe("createScrollPersistence", () => { - test("debounces persisted scroll writes", async () => { - const snapshot = { - session: { - review: { x: 0, y: 0 }, - }, - } as Record> - const writes: Array> = [] - const scroll = createScrollPersistence({ - debounceMs: 10, - getSnapshot: (sessionKey) => snapshot[sessionKey], - onFlush: (sessionKey, next) => { - snapshot[sessionKey] = next - writes.push(next) - }, - }) + test("debounces persisted scroll writes", () => { + vi.useFakeTimers() + try { + const snapshot = { + session: { + review: { x: 0, y: 0 }, + }, + } as Record> + const writes: Array> = [] + const scroll = createScrollPersistence({ + debounceMs: 10, + getSnapshot: (sessionKey) => snapshot[sessionKey], + onFlush: (sessionKey, next) => { + snapshot[sessionKey] = next + writes.push(next) + }, + }) - for (const i of Array.from({ length: 30 }, (_, n) => n + 1)) { - scroll.setScroll("session", "review", { x: 0, y: i }) + for (const i of Array.from({ length: 30 }, (_, n) => n + 1)) { + scroll.setScroll("session", "review", { x: 0, y: i }) + } + + vi.advanceTimersByTime(9) + expect(writes).toHaveLength(0) + + vi.advanceTimersByTime(1) + + expect(writes).toHaveLength(1) + expect(writes[0]?.review).toEqual({ x: 0, y: 30 }) + + scroll.setScroll("session", "review", { x: 0, y: 30 }) + vi.advanceTimersByTime(20) + + expect(writes).toHaveLength(1) + scroll.dispose() + } finally { + vi.useRealTimers() } - - await new Promise((resolve) => setTimeout(resolve, 40)) - - expect(writes).toHaveLength(1) - expect(writes[0]?.review).toEqual({ x: 0, y: 30 }) - - scroll.setScroll("session", "review", { x: 0, y: 30 }) - await new Promise((resolve) => setTimeout(resolve, 20)) - - expect(writes).toHaveLength(1) - scroll.dispose() }) }) diff --git a/packages/app/src/pages/directory-layout.tsx b/packages/app/src/pages/directory-layout.tsx index 2f4db8564..b2a17b96b 100644 --- a/packages/app/src/pages/directory-layout.tsx +++ b/packages/app/src/pages/directory-layout.tsx @@ -15,6 +15,7 @@ export default function Layout(props: ParentProps) { const params = useParams() const navigate = useNavigate() const language = useLanguage() + let invalid = "" const directory = createMemo(() => { return decode64(params.dir) ?? "" }) @@ -22,12 +23,14 @@ export default function Layout(props: ParentProps) { createEffect(() => { if (!params.dir) return if (directory()) return + if (invalid === params.dir) return + invalid = params.dir showToast({ variant: "error", title: language.t("common.requestFailed"), description: language.t("directory.error.invalidUrl"), }) - navigate("/") + navigate("/", { replace: true }) }) return ( diff --git a/packages/app/src/pages/layout/deep-links.ts b/packages/app/src/pages/layout/deep-links.ts index 772e6ece6..7bdb002a3 100644 --- a/packages/app/src/pages/layout/deep-links.ts +++ b/packages/app/src/pages/layout/deep-links.ts @@ -2,7 +2,15 @@ export const deepLinkEvent = "opencode:deep-link" export const parseDeepLink = (input: string) => { if (!input.startsWith("opencode://")) return - const url = new URL(input) + if (typeof URL.canParse === "function" && !URL.canParse(input)) return + const url = (() => { + try { + return new URL(input) + } catch { + return undefined + } + })() + if (!url) return if (url.hostname !== "open-project") return const directory = url.searchParams.get("directory") if (!directory) return diff --git a/packages/app/src/pages/layout/helpers.test.ts b/packages/app/src/pages/layout/helpers.test.ts index 8a8ea78c7..83d8f4748 100644 --- a/packages/app/src/pages/layout/helpers.test.ts +++ b/packages/app/src/pages/layout/helpers.test.ts @@ -12,6 +12,27 @@ describe("layout deep links", () => { expect(parseDeepLink("https://example.com")).toBeUndefined() }) + test("ignores malformed deep links safely", () => { + expect(() => parseDeepLink("opencode://open-project/%E0%A4%A%")).not.toThrow() + expect(parseDeepLink("opencode://open-project/%E0%A4%A%")).toBeUndefined() + }) + + test("parses links when URL.canParse is unavailable", () => { + const original = Object.getOwnPropertyDescriptor(URL, "canParse") + Object.defineProperty(URL, "canParse", { configurable: true, value: undefined }) + try { + expect(parseDeepLink("opencode://open-project?directory=/tmp/demo")).toBe("/tmp/demo") + } finally { + if (original) Object.defineProperty(URL, "canParse", original) + if (!original) Reflect.deleteProperty(URL, "canParse") + } + }) + + test("ignores open-project deep links without directory", () => { + expect(parseDeepLink("opencode://open-project")).toBeUndefined() + expect(parseDeepLink("opencode://open-project?directory=")).toBeUndefined() + }) + test("collects only valid open-project directories", () => { const result = collectOpenProjectDeepLinks([ "opencode://open-project?directory=/a", @@ -39,6 +60,14 @@ describe("layout workspace helpers", () => { expect(workspaceKey("C:\\tmp\\demo\\\\")).toBe("C:\\tmp\\demo") }) + test("preserves posix and drive roots in workspace key", () => { + expect(workspaceKey("/")).toBe("/") + expect(workspaceKey("///")).toBe("/") + expect(workspaceKey("C:\\")).toBe("C:\\") + expect(workspaceKey("C:\\\\\\")).toBe("C:\\") + expect(workspaceKey("C:///")).toBe("C:/") + }) + test("keeps local first while preserving known order", () => { const result = syncWorkspaceOrder("/root", ["/root", "/b", "/c"], ["/root", "/c", "/a", "/b"]) expect(result).toEqual(["/root", "/c", "/b"]) diff --git a/packages/app/src/pages/layout/helpers.ts b/packages/app/src/pages/layout/helpers.ts index 4d144f34e..6ecccb95c 100644 --- a/packages/app/src/pages/layout/helpers.ts +++ b/packages/app/src/pages/layout/helpers.ts @@ -1,7 +1,12 @@ import { getFilename } from "@opencode-ai/util/path" import { type Session } from "@opencode-ai/sdk/v2/client" -export const workspaceKey = (directory: string) => directory.replace(/[\\/]+$/, "") +export const workspaceKey = (directory: string) => { + const drive = directory.match(/^([A-Za-z]:)[\\/]+$/) + if (drive) return `${drive[1]}${directory.includes("\\") ? "\\" : "/"}` + if (/^[\\/]+$/.test(directory)) return directory.includes("\\") ? "\\" : "/" + return directory.replace(/[\\/]+$/, "") +} export function sortSessions(now: number) { const oneMinuteAgo = now - 60 * 1000 diff --git a/packages/app/src/pages/session.tsx b/packages/app/src/pages/session.tsx index 98f8bdbcd..7678ea6a8 100644 --- a/packages/app/src/pages/session.tsx +++ b/packages/app/src/pages/session.tsx @@ -40,7 +40,7 @@ import { showToast } from "@opencode-ai/ui/toast" import { SessionHeader, SessionContextTab, SortableTab, FileVisual, NewSessionView } from "@/components/session" import { navMark, navParams } from "@/utils/perf" import { same } from "@/utils/same" -import { createOpenReviewFile, focusTerminalById } from "@/pages/session/helpers" +import { createOpenReviewFile, focusTerminalById, getTabReorderIndex } from "@/pages/session/helpers" import { createScrollSpy } from "@/pages/session/scroll-spy" import { createFileTabListSync } from "@/pages/session/file-tab-scroll" import { FileTabContent } from "@/pages/session/file-tabs" @@ -844,11 +844,9 @@ export default function Page() { const { draggable, droppable } = event if (draggable && droppable) { const currentTabs = tabs().all() - const fromIndex = currentTabs?.indexOf(draggable.id.toString()) - const toIndex = currentTabs?.indexOf(droppable.id.toString()) - if (fromIndex !== toIndex && toIndex !== undefined) { - tabs().move(draggable.id.toString(), toIndex) - } + const toIndex = getTabReorderIndex(currentTabs, draggable.id.toString(), droppable.id.toString()) + if (toIndex === undefined) return + tabs().move(draggable.id.toString(), toIndex) } } diff --git a/packages/app/src/pages/session/helpers.test.ts b/packages/app/src/pages/session/helpers.test.ts index 0afc7eb6a..d877d5b2e 100644 --- a/packages/app/src/pages/session/helpers.test.ts +++ b/packages/app/src/pages/session/helpers.test.ts @@ -1,5 +1,5 @@ import { describe, expect, test } from "bun:test" -import { combineCommandSections, createOpenReviewFile, focusTerminalById } from "./helpers" +import { combineCommandSections, createOpenReviewFile, focusTerminalById, getTabReorderIndex } from "./helpers" describe("createOpenReviewFile", () => { test("opens and loads selected review file", () => { @@ -59,3 +59,13 @@ describe("combineCommandSections", () => { expect(result.map((item) => item.id)).toEqual(["a", "b", "c"]) }) }) + +describe("getTabReorderIndex", () => { + test("returns target index for valid drag reorder", () => { + expect(getTabReorderIndex(["a", "b", "c"], "a", "c")).toBe(2) + }) + + test("returns undefined for unknown droppable id", () => { + expect(getTabReorderIndex(["a", "b", "c"], "a", "missing")).toBeUndefined() + }) +}) diff --git a/packages/app/src/pages/session/helpers.ts b/packages/app/src/pages/session/helpers.ts index d9ce90793..dcf2c8784 100644 --- a/packages/app/src/pages/session/helpers.ts +++ b/packages/app/src/pages/session/helpers.ts @@ -36,3 +36,10 @@ export const createOpenReviewFile = (input: { export const combineCommandSections = (sections: readonly (readonly CommandOption[])[]) => { return sections.flatMap((section) => section) } + +export const getTabReorderIndex = (tabs: readonly string[], from: string, to: string) => { + const fromIndex = tabs.indexOf(from) + const toIndex = tabs.indexOf(to) + if (fromIndex === -1 || toIndex === -1 || fromIndex === toIndex) return undefined + return toIndex +} diff --git a/packages/app/src/utils/persist.test.ts b/packages/app/src/utils/persist.test.ts new file mode 100644 index 000000000..5be68f3b0 --- /dev/null +++ b/packages/app/src/utils/persist.test.ts @@ -0,0 +1,102 @@ +import { beforeAll, beforeEach, describe, expect, mock, test } from "bun:test" + +type PersistTestingType = typeof import("./persist").PersistTesting + +class MemoryStorage implements Storage { + private values = new Map() + readonly events: string[] = [] + readonly calls = { get: 0, set: 0, remove: 0 } + + clear() { + this.values.clear() + } + + get length() { + return this.values.size + } + + key(index: number) { + return Array.from(this.values.keys())[index] ?? null + } + + getItem(key: string) { + this.calls.get += 1 + this.events.push(`get:${key}`) + if (key.startsWith("opencode.throw")) throw new Error("storage get failed") + return this.values.get(key) ?? null + } + + setItem(key: string, value: string) { + this.calls.set += 1 + this.events.push(`set:${key}`) + if (key.startsWith("opencode.quota")) throw new DOMException("quota", "QuotaExceededError") + if (key.startsWith("opencode.throw")) throw new Error("storage set failed") + this.values.set(key, value) + } + + removeItem(key: string) { + this.calls.remove += 1 + this.events.push(`remove:${key}`) + if (key.startsWith("opencode.throw")) throw new Error("storage remove failed") + this.values.delete(key) + } +} + +const storage = new MemoryStorage() + +let persistTesting: PersistTestingType + +beforeAll(async () => { + mock.module("@/context/platform", () => ({ + usePlatform: () => ({ platform: "web" }), + })) + + const mod = await import("./persist") + persistTesting = mod.PersistTesting +}) + +beforeEach(() => { + storage.clear() + storage.events.length = 0 + storage.calls.get = 0 + storage.calls.set = 0 + storage.calls.remove = 0 + Object.defineProperty(globalThis, "localStorage", { + value: storage, + configurable: true, + }) +}) + +describe("persist localStorage resilience", () => { + test("does not cache values as persisted when quota write and eviction fail", () => { + const storageApi = persistTesting.localStorageWithPrefix("opencode.quota.scope") + storageApi.setItem("value", '{"value":1}') + + expect(storage.getItem("opencode.quota.scope:value")).toBeNull() + expect(storageApi.getItem("value")).toBeNull() + }) + + test("disables only the failing scope when storage throws", () => { + const bad = persistTesting.localStorageWithPrefix("opencode.throw.scope") + bad.setItem("value", '{"value":1}') + + const before = storage.calls.set + bad.setItem("value", '{"value":2}') + expect(storage.calls.set).toBe(before) + expect(bad.getItem("value")).toBeNull() + + const healthy = persistTesting.localStorageWithPrefix("opencode.safe.scope") + healthy.setItem("value", '{"value":3}') + expect(storage.getItem("opencode.safe.scope:value")).toBe('{"value":3}') + }) + + test("failing fallback scope does not poison direct storage scope", () => { + const broken = persistTesting.localStorageWithPrefix("opencode.throw.scope2") + broken.setItem("value", '{"value":1}') + + const direct = persistTesting.localStorageDirect() + direct.setItem("direct-value", '{"value":5}') + + expect(storage.getItem("direct-value")).toBe('{"value":5}') + }) +}) diff --git a/packages/app/src/utils/persist.ts b/packages/app/src/utils/persist.ts index 0ca3abad0..329a406dd 100644 --- a/packages/app/src/utils/persist.ts +++ b/packages/app/src/utils/persist.ts @@ -17,7 +17,7 @@ type PersistTarget = { const LEGACY_STORAGE = "default.dat" const GLOBAL_STORAGE = "opencode.global.dat" const LOCAL_PREFIX = "opencode." -const fallback = { disabled: false } +const fallback = new Map() const CACHE_MAX_ENTRIES = 500 const CACHE_MAX_BYTES = 8 * 1024 * 1024 @@ -65,6 +65,14 @@ function cacheGet(key: string) { return entry.value } +function fallbackDisabled(scope: string) { + return fallback.get(scope) === true +} + +function fallbackSet(scope: string) { + fallback.set(scope, true) +} + function quota(error: unknown) { if (error instanceof DOMException) { if (error.name === "QuotaExceededError") return true @@ -142,7 +150,6 @@ function write(storage: Storage, key: string, value: string) { } const ok = evict(storage, key, value) - if (!ok) cacheSet(key, value) return ok } @@ -196,18 +203,19 @@ function workspaceStorage(dir: string) { function localStorageWithPrefix(prefix: string): SyncStorage { const base = `${prefix}:` + const scope = `prefix:${prefix}` const item = (key: string) => base + key return { getItem: (key) => { const name = item(key) const cached = cacheGet(name) - if (fallback.disabled && cached !== undefined) return cached + if (fallbackDisabled(scope)) return cached ?? null const stored = (() => { try { return localStorage.getItem(name) } catch { - fallback.disabled = true + fallbackSet(scope) return null } })() @@ -217,40 +225,40 @@ function localStorageWithPrefix(prefix: string): SyncStorage { }, setItem: (key, value) => { const name = item(key) - cacheSet(name, value) - if (fallback.disabled) return + if (fallbackDisabled(scope)) return try { if (write(localStorage, name, value)) return } catch { - fallback.disabled = true + fallbackSet(scope) return } - fallback.disabled = true + fallbackSet(scope) }, removeItem: (key) => { const name = item(key) cacheDelete(name) - if (fallback.disabled) return + if (fallbackDisabled(scope)) return try { localStorage.removeItem(name) } catch { - fallback.disabled = true + fallbackSet(scope) } }, } } function localStorageDirect(): SyncStorage { + const scope = "direct" return { getItem: (key) => { const cached = cacheGet(key) - if (fallback.disabled && cached !== undefined) return cached + if (fallbackDisabled(scope)) return cached ?? null const stored = (() => { try { return localStorage.getItem(key) } catch { - fallback.disabled = true + fallbackSet(scope) return null } })() @@ -259,28 +267,32 @@ function localStorageDirect(): SyncStorage { return stored }, setItem: (key, value) => { - cacheSet(key, value) - if (fallback.disabled) return + if (fallbackDisabled(scope)) return try { if (write(localStorage, key, value)) return } catch { - fallback.disabled = true + fallbackSet(scope) return } - fallback.disabled = true + fallbackSet(scope) }, removeItem: (key) => { cacheDelete(key) - if (fallback.disabled) return + if (fallbackDisabled(scope)) return try { localStorage.removeItem(key) } catch { - fallback.disabled = true + fallbackSet(scope) } }, } } +export const PersistTesting = { + localStorageDirect, + localStorageWithPrefix, +} + export const Persist = { global(key: string, legacy?: string[]): PersistTarget { return { storage: GLOBAL_STORAGE, key, legacy } diff --git a/packages/app/src/utils/server-health.test.ts b/packages/app/src/utils/server-health.test.ts index 34c86685a..26bda070a 100644 --- a/packages/app/src/utils/server-health.test.ts +++ b/packages/app/src/utils/server-health.test.ts @@ -1,6 +1,12 @@ import { describe, expect, test } from "bun:test" import { checkServerHealth } from "./server-health" +function abortFromInput(input: RequestInfo | URL, init?: RequestInit) { + if (init?.signal) return init.signal + if (input instanceof Request) return input.signal + return undefined +} + describe("checkServerHealth", () => { test("returns healthy response with version", async () => { const fetch = (async () => @@ -24,10 +30,40 @@ describe("checkServerHealth", () => { expect(result).toEqual({ healthy: false }) }) + test("uses timeout fallback when AbortSignal.timeout is unavailable", async () => { + const timeout = Object.getOwnPropertyDescriptor(AbortSignal, "timeout") + Object.defineProperty(AbortSignal, "timeout", { + configurable: true, + value: undefined, + }) + + let aborted = false + const fetch = ((input: RequestInfo | URL, init?: RequestInit) => + new Promise((_resolve, reject) => { + const signal = abortFromInput(input, init) + signal?.addEventListener( + "abort", + () => { + aborted = true + reject(new DOMException("Aborted", "AbortError")) + }, + { once: true }, + ) + })) as unknown as typeof globalThis.fetch + + const result = await checkServerHealth("http://localhost:4096", fetch, { timeoutMs: 10 }).finally(() => { + if (timeout) Object.defineProperty(AbortSignal, "timeout", timeout) + if (!timeout) Reflect.deleteProperty(AbortSignal, "timeout") + }) + + expect(aborted).toBe(true) + expect(result).toEqual({ healthy: false }) + }) + test("uses provided abort signal", async () => { let signal: AbortSignal | undefined const fetch = (async (input: RequestInfo | URL, init?: RequestInit) => { - signal = init?.signal ?? (input instanceof Request ? input.signal : undefined) + signal = abortFromInput(input, init) return new Response(JSON.stringify({ healthy: true, version: "1.2.3" }), { status: 200, headers: { "content-type": "application/json" }, @@ -39,4 +75,40 @@ describe("checkServerHealth", () => { expect(signal).toBe(abort.signal) }) + + test("retries transient failures and eventually succeeds", async () => { + let count = 0 + const fetch = (async () => { + count += 1 + if (count < 3) throw new TypeError("network") + return new Response(JSON.stringify({ healthy: true, version: "1.2.3" }), { + status: 200, + headers: { "content-type": "application/json" }, + }) + }) as unknown as typeof globalThis.fetch + + const result = await checkServerHealth("http://localhost:4096", fetch, { + retryCount: 2, + retryDelayMs: 1, + }) + + expect(count).toBe(3) + expect(result).toEqual({ healthy: true, version: "1.2.3" }) + }) + + test("returns unhealthy when retries are exhausted", async () => { + let count = 0 + const fetch = (async () => { + count += 1 + throw new TypeError("network") + }) as unknown as typeof globalThis.fetch + + const result = await checkServerHealth("http://localhost:4096", fetch, { + retryCount: 2, + retryDelayMs: 1, + }) + + expect(count).toBe(3) + expect(result).toEqual({ healthy: false }) + }) }) diff --git a/packages/app/src/utils/server-health.ts b/packages/app/src/utils/server-health.ts index ab33460b2..929826d0d 100644 --- a/packages/app/src/utils/server-health.ts +++ b/packages/app/src/utils/server-health.ts @@ -5,10 +5,50 @@ export type ServerHealth = { healthy: boolean; version?: string } interface CheckServerHealthOptions { timeoutMs?: number signal?: AbortSignal + retryCount?: number + retryDelayMs?: number } +const defaultTimeoutMs = 3000 +const defaultRetryCount = 2 +const defaultRetryDelayMs = 100 + function timeoutSignal(timeoutMs: number) { - return (AbortSignal as unknown as { timeout?: (ms: number) => AbortSignal }).timeout?.(timeoutMs) + const timeout = (AbortSignal as unknown as { timeout?: (ms: number) => AbortSignal }).timeout + if (timeout) { + try { + return { signal: timeout.call(AbortSignal, timeoutMs), clear: undefined as (() => void) | undefined } + } catch {} + } + const controller = new AbortController() + const timer = setTimeout(() => controller.abort(), timeoutMs) + return { signal: controller.signal, clear: () => clearTimeout(timer) } +} + +function wait(ms: number, signal?: AbortSignal) { + return new Promise((resolve, reject) => { + if (signal?.aborted) { + reject(new DOMException("Aborted", "AbortError")) + return + } + const timer = setTimeout(() => { + signal?.removeEventListener("abort", onAbort) + resolve() + }, ms) + const onAbort = () => { + clearTimeout(timer) + reject(new DOMException("Aborted", "AbortError")) + } + signal?.addEventListener("abort", onAbort, { once: true }) + }) +} + +function retryable(error: unknown, signal?: AbortSignal) { + if (signal?.aborted) return false + if (!(error instanceof Error)) return false + if (error.name === "AbortError" || error.name === "TimeoutError") return false + if (error instanceof TypeError) return true + return /network|fetch|econnreset|econnrefused|enotfound|timedout/i.test(error.message) } export async function checkServerHealth( @@ -16,14 +56,24 @@ export async function checkServerHealth( fetch: typeof globalThis.fetch, opts?: CheckServerHealthOptions, ): Promise { - const signal = opts?.signal ?? timeoutSignal(opts?.timeoutMs ?? 3000) - const sdk = createOpencodeClient({ - baseUrl: url, - fetch, - signal, - }) - return sdk.global - .health() - .then((x) => ({ healthy: x.data?.healthy === true, version: x.data?.version })) - .catch(() => ({ healthy: false })) + const timeout = opts?.signal ? undefined : timeoutSignal(opts?.timeoutMs ?? defaultTimeoutMs) + const signal = opts?.signal ?? timeout?.signal + const retryCount = opts?.retryCount ?? defaultRetryCount + const retryDelayMs = opts?.retryDelayMs ?? defaultRetryDelayMs + const next = (count: number, error: unknown) => { + if (count >= retryCount || !retryable(error, signal)) return Promise.resolve({ healthy: false } as const) + return wait(retryDelayMs * (count + 1), signal) + .then(() => attempt(count + 1)) + .catch(() => ({ healthy: false })) + } + const attempt = (count: number): Promise => + createOpencodeClient({ + baseUrl: url, + fetch, + signal, + }) + .global.health() + .then((x) => (x.error ? next(count, x.error) : { healthy: x.data?.healthy === true, version: x.data?.version })) + .catch((error) => next(count, error)) + return attempt(0).finally(() => timeout?.clear?.()) } diff --git a/packages/opencode/src/file/ripgrep.ts b/packages/opencode/src/file/ripgrep.ts index c1e5113bf..58f9af7cd 100644 --- a/packages/opencode/src/file/ripgrep.ts +++ b/packages/opencode/src/file/ripgrep.ts @@ -123,9 +123,13 @@ export namespace Ripgrep { ) const state = lazy(async () => { - let filepath = Bun.which("rg") - if (filepath) return { filepath } - filepath = path.join(Global.Path.bin, "rg" + (process.platform === "win32" ? ".exe" : "")) + const system = Bun.which("rg") + if (system) { + const stat = await fs.stat(system).catch(() => undefined) + if (stat?.isFile()) return { filepath: system } + log.warn("bun.which returned invalid rg path", { filepath: system }) + } + const filepath = path.join(Global.Path.bin, "rg" + (process.platform === "win32" ? ".exe" : "")) const file = Bun.file(filepath) if (!(await file.exists())) {