From e89849d6329b572abdcaffb24758f0348aeba705 Mon Sep 17 00:00:00 2001 From: Adam <2363879+adamdotdevin@users.noreply.github.com> Date: Fri, 13 Mar 2026 09:29:20 -0500 Subject: [PATCH] Revert "fix(app): terminal cloning without retry (#17354)" This reverts commit c9e9dbeee1ceb5af3d1b1ce292317390286fe7a0. --- packages/app/e2e/actions.ts | 31 +-- .../e2e/terminal/terminal-reconnect.spec.ts | 46 ---- packages/app/e2e/tsconfig.json | 3 +- packages/app/src/components/terminal.tsx | 204 ++++++------------ packages/app/src/testing/terminal.ts | 63 ++---- 5 files changed, 89 insertions(+), 258 deletions(-) delete mode 100644 packages/app/e2e/terminal/terminal-reconnect.spec.ts diff --git a/packages/app/e2e/actions.ts b/packages/app/e2e/actions.ts index 8e21579e21..a56001248d 100644 --- a/packages/app/e2e/actions.ts +++ b/packages/app/e2e/actions.ts @@ -36,22 +36,6 @@ async function terminalID(term: Locator) { throw new Error(`Active terminal missing ${terminalAttr}`) } -export async function terminalConnects(page: Page, input?: { term?: Locator }) { - const term = input?.term ?? page.locator(terminalSelector).first() - const id = await terminalID(term) - return page.evaluate((id) => { - return (window as E2EWindow).__opencode_e2e?.terminal?.terminals?.[id]?.connects ?? 0 - }, id) -} - -export async function disconnectTerminal(page: Page, input?: { term?: Locator }) { - const term = input?.term ?? page.locator(terminalSelector).first() - const id = await terminalID(term) - await page.evaluate((id) => { - ;(window as E2EWindow).__opencode_e2e?.terminal?.controls?.[id]?.disconnect?.() - }, id) -} - async function terminalReady(page: Page, term?: Locator) { const next = term ?? page.locator(terminalSelector).first() const id = await terminalID(next) @@ -604,19 +588,12 @@ export async function seedSessionTask( .flatMap((message) => message.parts) .find((part) => { if (part.type !== "tool" || part.tool !== "task") return false - if (!("state" in part) || !part.state || typeof part.state !== "object") return false - if (!("input" in part.state) || !part.state.input || typeof part.state.input !== "object") return false - if (!("description" in part.state.input) || part.state.input.description !== input.description) return false - if (!("metadata" in part.state) || !part.state.metadata || typeof part.state.metadata !== "object") - return false - if (!("sessionId" in part.state.metadata)) return false - return typeof part.state.metadata.sessionId === "string" && part.state.metadata.sessionId.length > 0 + if (part.state.input?.description !== input.description) return false + return typeof part.state.metadata?.sessionId === "string" && part.state.metadata.sessionId.length > 0 }) - if (!part || !("state" in part) || !part.state || typeof part.state !== "object") return - if (!("metadata" in part.state) || !part.state.metadata || typeof part.state.metadata !== "object") return - if (!("sessionId" in part.state.metadata)) return - const id = part.state.metadata.sessionId + if (!part) return + const id = part.state.metadata?.sessionId if (typeof id !== "string" || !id) return const child = await sdk.session .get({ sessionID: id }) diff --git a/packages/app/e2e/terminal/terminal-reconnect.spec.ts b/packages/app/e2e/terminal/terminal-reconnect.spec.ts deleted file mode 100644 index b03ed89568..0000000000 --- a/packages/app/e2e/terminal/terminal-reconnect.spec.ts +++ /dev/null @@ -1,46 +0,0 @@ -import type { Page } from "@playwright/test" -import { disconnectTerminal, runTerminal, terminalConnects, waitTerminalReady } from "../actions" -import { test, expect } from "../fixtures" -import { terminalSelector } from "../selectors" -import { terminalToggleKey } from "../utils" - -async function open(page: Page) { - const term = page.locator(terminalSelector).first() - const visible = await term.isVisible().catch(() => false) - if (!visible) await page.keyboard.press(terminalToggleKey) - await waitTerminalReady(page, { term }) - return term -} - -test("terminal reconnects without replacing the pty", async ({ page, withProject }) => { - await withProject(async ({ gotoSession }) => { - const name = `OPENCODE_E2E_RECONNECT_${Date.now()}` - const token = `E2E_RECONNECT_${Date.now()}` - - await gotoSession() - - const term = await open(page) - const id = await term.getAttribute("data-pty-id") - if (!id) throw new Error("Active terminal missing data-pty-id") - - const prev = await terminalConnects(page, { term }) - - await runTerminal(page, { - term, - cmd: `export ${name}=${token}; echo ${token}`, - token, - }) - - await disconnectTerminal(page, { term }) - - await expect.poll(() => terminalConnects(page, { term }), { timeout: 15_000 }).toBeGreaterThan(prev) - await expect.poll(() => term.getAttribute("data-pty-id"), { timeout: 5_000 }).toBe(id) - - await runTerminal(page, { - term, - cmd: `echo $${name}`, - token, - timeout: 15_000, - }) - }) -}) diff --git a/packages/app/e2e/tsconfig.json b/packages/app/e2e/tsconfig.json index a86352b839..18e88ddc9c 100644 --- a/packages/app/e2e/tsconfig.json +++ b/packages/app/e2e/tsconfig.json @@ -2,8 +2,7 @@ "extends": "../tsconfig.json", "compilerOptions": { "noEmit": true, - "rootDir": "..", "types": ["node", "bun"] }, - "include": ["./**/*.ts", "../src/testing/terminal.ts"] + "include": ["./**/*.ts"] } diff --git a/packages/app/src/components/terminal.tsx b/packages/app/src/components/terminal.tsx index d4eccbc828..ff455ebe20 100644 --- a/packages/app/src/components/terminal.tsx +++ b/packages/app/src/components/terminal.tsx @@ -65,16 +65,6 @@ const debugTerminal = (...values: unknown[]) => { console.debug("[terminal]", ...values) } -const errorStatus = (err: unknown) => { - if (!err || typeof err !== "object") return - if (!("data" in err)) return - const data = err.data - if (!data || typeof data !== "object") return - if (!("statusCode" in data)) return - const status = data.statusCode - return typeof status === "number" ? status : undefined -} - const useTerminalUiBindings = (input: { container: HTMLDivElement term: Term @@ -199,11 +189,7 @@ export const Terminal = (props: TerminalProps) => { const start = typeof local.pty.cursor === "number" && Number.isSafeInteger(local.pty.cursor) ? local.pty.cursor : undefined let cursor = start ?? 0 - let seek = start !== undefined ? start : restore ? -1 : 0 let output: ReturnType | undefined - let drop: VoidFunction | undefined - let reconn: ReturnType | undefined - let tries = 0 const cleanup = () => { if (!cleanups.length) return @@ -467,135 +453,85 @@ export const Terminal = (props: TerminalProps) => { } const once = { value: false } - const decoder = new TextDecoder() + let closing = false - const fail = (err: unknown) => { + const url = new URL(sdk.url + `/pty/${id}/connect`) + url.searchParams.set("directory", sdk.directory) + url.searchParams.set("cursor", String(start !== undefined ? start : restore ? -1 : 0)) + url.protocol = url.protocol === "https:" ? "wss:" : "ws:" + url.username = server.current?.http.username ?? "opencode" + url.password = server.current?.http.password ?? "" + + const socket = new WebSocket(url) + socket.binaryType = "arraybuffer" + ws = socket + + const handleOpen = () => { + probe.connect() + local.onConnect?.() + scheduleSize(t.cols, t.rows) + } + socket.addEventListener("open", handleOpen) + if (socket.readyState === WebSocket.OPEN) handleOpen() + + const decoder = new TextDecoder() + const handleMessage = (event: MessageEvent) => { if (disposed) return + if (closing) return + if (event.data instanceof ArrayBuffer) { + const bytes = new Uint8Array(event.data) + if (bytes[0] !== 0) return + const json = decoder.decode(bytes.subarray(1)) + try { + const meta = JSON.parse(json) as { cursor?: unknown } + const next = meta?.cursor + if (typeof next === "number" && Number.isSafeInteger(next) && next >= 0) { + cursor = next + } + } catch (err) { + debugTerminal("invalid websocket control frame", err) + } + return + } + + const data = typeof event.data === "string" ? event.data : "" + if (!data) return + output?.push(data) + cursor += data.length + } + socket.addEventListener("message", handleMessage) + + const handleError = (error: Event) => { + if (disposed) return + if (closing) return if (once.value) return once.value = true - local.onConnectError?.(err) + console.error("WebSocket error:", error) + local.onConnectError?.(error) } + socket.addEventListener("error", handleError) - const gone = () => - sdk.client.pty - .get({ ptyID: id }) - .then(() => false) - .catch((err) => { - if (errorStatus(err) === 404) return true - debugTerminal("failed to inspect terminal session", err) - return false - }) - - const retry = (err: unknown) => { + const handleClose = (event: CloseEvent) => { if (disposed) return - if (reconn !== undefined) return - - const ms = Math.min(250 * 2 ** Math.min(tries, 4), 4_000) - reconn = setTimeout(async () => { - reconn = undefined - if (disposed) return - if (await gone()) { - if (disposed) return - fail(err) - return - } - if (disposed) return - tries += 1 - open() - }, ms) + if (closing) return + // Normal closure (code 1000) means PTY process exited - server event handles cleanup + // For other codes (network issues, server restart), trigger error handler + if (event.code !== 1000) { + if (once.value) return + once.value = true + local.onConnectError?.(new Error(language.t("terminal.connectionLost.abnormalClose", { code: event.code }))) + } } + socket.addEventListener("close", handleClose) - const open = () => { - if (disposed) return - drop?.() - - const url = new URL(sdk.url + `/pty/${id}/connect`) - url.searchParams.set("directory", sdk.directory) - url.searchParams.set("cursor", String(seek)) - url.protocol = url.protocol === "https:" ? "wss:" : "ws:" - url.username = server.current?.http.username ?? "opencode" - url.password = server.current?.http.password ?? "" - - const socket = new WebSocket(url) - socket.binaryType = "arraybuffer" - ws = socket - - const handleOpen = () => { - if (disposed) return - tries = 0 - probe.connect() - local.onConnect?.() - scheduleSize(t.cols, t.rows) - } - - const handleMessage = (event: MessageEvent) => { - if (disposed) return - if (event.data instanceof ArrayBuffer) { - const bytes = new Uint8Array(event.data) - if (bytes[0] !== 0) return - const json = decoder.decode(bytes.subarray(1)) - try { - const meta = JSON.parse(json) as { cursor?: unknown } - const next = meta?.cursor - if (typeof next === "number" && Number.isSafeInteger(next) && next >= 0) { - cursor = next - seek = next - } - } catch (err) { - debugTerminal("invalid websocket control frame", err) - } - return - } - - const data = typeof event.data === "string" ? event.data : "" - if (!data) return - output?.push(data) - cursor += data.length - seek = cursor - } - - const handleError = (error: Event) => { - if (disposed) return - debugTerminal("websocket error", error) - } - - const stop = () => { - socket.removeEventListener("open", handleOpen) - socket.removeEventListener("message", handleMessage) - socket.removeEventListener("error", handleError) - socket.removeEventListener("close", handleClose) - if (ws === socket) ws = undefined - if (drop === stop) drop = undefined - if (socket.readyState !== WebSocket.CLOSED && socket.readyState !== WebSocket.CLOSING) socket.close(1000) - } - - const handleClose = (event: CloseEvent) => { - if (ws === socket) ws = undefined - if (drop === stop) drop = undefined - socket.removeEventListener("open", handleOpen) - socket.removeEventListener("message", handleMessage) - socket.removeEventListener("error", handleError) - socket.removeEventListener("close", handleClose) - if (disposed) return - if (event.code === 1000) return - retry(new Error(language.t("terminal.connectionLost.abnormalClose", { code: event.code }))) - } - - drop = stop - socket.addEventListener("open", handleOpen) - socket.addEventListener("message", handleMessage) - socket.addEventListener("error", handleError) - socket.addEventListener("close", handleClose) - } - - probe.control({ - disconnect: () => { - if (!ws) return - ws.close(4_000, "e2e") - }, + cleanups.push(() => { + closing = true + socket.removeEventListener("open", handleOpen) + socket.removeEventListener("message", handleMessage) + socket.removeEventListener("error", handleError) + socket.removeEventListener("close", handleClose) + if (socket.readyState !== WebSocket.CLOSED && socket.readyState !== WebSocket.CLOSING) socket.close(1000) }) - - open() } void run().catch((err) => { @@ -613,8 +549,6 @@ export const Terminal = (props: TerminalProps) => { disposed = true if (fitFrame !== undefined) cancelAnimationFrame(fitFrame) if (sizeTimer !== undefined) clearTimeout(sizeTimer) - if (reconn !== undefined) clearTimeout(reconn) - drop?.() if (ws && ws.readyState !== WebSocket.CLOSED && ws.readyState !== WebSocket.CLOSING) ws.close(1000) const finalize = () => { diff --git a/packages/app/src/testing/terminal.ts b/packages/app/src/testing/terminal.ts index 4c179dee3c..aa19744045 100644 --- a/packages/app/src/testing/terminal.ts +++ b/packages/app/src/testing/terminal.ts @@ -2,28 +2,21 @@ export const terminalAttr = "data-pty-id" export type TerminalProbeState = { connected: boolean - connects: number rendered: string settled: number } -type TerminalProbeControl = { - disconnect?: VoidFunction -} - export type E2EWindow = Window & { __opencode_e2e?: { terminal?: { enabled?: boolean terminals?: Record - controls?: Record } } } const seed = (): TerminalProbeState => ({ connected: false, - connects: 0, rendered: "", settled: 0, }) @@ -32,28 +25,15 @@ const root = () => { if (typeof window === "undefined") return const state = (window as E2EWindow).__opencode_e2e?.terminal if (!state?.enabled) return - return state -} - -const terms = () => { - const state = root() - if (!state) return state.terminals ??= {} return state.terminals } -const controls = () => { - const state = root() - if (!state) return - state.controls ??= {} - return state.controls -} - export const terminalProbe = (id: string) => { const set = (next: Partial) => { - const state = terms() - if (!state) return - state[id] = { ...(state[id] ?? seed()), ...next } + const terms = root() + if (!terms) return + terms[id] = { ...(terms[id] ?? seed()), ...next } } return { @@ -61,37 +41,24 @@ export const terminalProbe = (id: string) => { set(seed()) }, connect() { - const state = terms() - if (!state) return - const prev = state[id] ?? seed() - state[id] = { - ...prev, - connected: true, - connects: prev.connects + 1, - } + set({ connected: true }) }, render(data: string) { - const state = terms() - if (!state) return - const prev = state[id] ?? seed() - state[id] = { ...prev, rendered: prev.rendered + data } + const terms = root() + if (!terms) return + const prev = terms[id] ?? seed() + terms[id] = { ...prev, rendered: prev.rendered + data } }, settle() { - const state = terms() - if (!state) return - const prev = state[id] ?? seed() - state[id] = { ...prev, settled: prev.settled + 1 } - }, - control(next: Partial) { - const state = controls() - if (!state) return - state[id] = { ...(state[id] ?? {}), ...next } + const terms = root() + if (!terms) return + const prev = terms[id] ?? seed() + terms[id] = { ...prev, settled: prev.settled + 1 } }, drop() { - const state = terms() - if (state) delete state[id] - const control = controls() - if (control) delete control[id] + const terms = root() + if (!terms) return + delete terms[id] }, } }