From 9391ee8efc405dc8d6144be93959807ac7269053 Mon Sep 17 00:00:00 2001 From: Luke Parker <10430890+Hona@users.noreply.github.com> Date: Wed, 2 Sep 2026 18:00:12 +1000 Subject: [PATCH] fix(app): avoid redundant composer encodes (#46730) --- .../composer-write-batch-benchmark.spec.ts | 197 ++++++++++++++++++ packages/app/src/composer/editor/actions.ts | 48 +++-- packages/app/src/composer/state.ts | 29 ++- .../test-browser/composer-write-batch.test.ts | 186 +++++++++++++++++ 4 files changed, 423 insertions(+), 37 deletions(-) create mode 100644 packages/app/e2e/performance/timeline/composer-write-batch-benchmark.spec.ts create mode 100644 packages/app/test-browser/composer-write-batch.test.ts diff --git a/packages/app/e2e/performance/timeline/composer-write-batch-benchmark.spec.ts b/packages/app/e2e/performance/timeline/composer-write-batch-benchmark.spec.ts new file mode 100644 index 00000000000..82b5f85820b --- /dev/null +++ b/packages/app/e2e/performance/timeline/composer-write-batch-benchmark.spec.ts @@ -0,0 +1,197 @@ +import { base64Encode } from "@opencode-ai/util/encode" +import { benchmark, benchmarkDiagnostics, expect } from "../benchmark" +import { mockOpenCodeServer } from "../../utils/mock-server" +import { expectSessionTitle } from "../../utils/waits" +import { fixture } from "./session-timeline-stress.fixture" + +const sessionID = "ses_composer_write_batch" +const title = "Composer persistence workload" +const addition = " Keep the existing error handling and add coverage." +const text = + Array.from( + { length: 180 }, + (_, index) => + `Review requirement ${index + 1}: preserve request ordering in src/queue/worker-${index % 12}.ts. ` + + `A failed request must retain its payload, report its cause, and remain safe to retry.\n` + + `Expected: await queue.flush(); expect(await repository.read(id)).toEqual(accepted);\n`, + ).join("") + "Implementation notes:" +const items = Array.from({ length: 8 }, (_, index) => ({ + type: "file", + path: `src/queue/worker-${index}.ts`, + selection: { startLine: 10, startChar: 0, endLine: 24, endChar: 0 }, + commentID: `composer-write-batch-${index}`, + comment: `Check retry path ${index}: keep the original request identity and error cause.`, + preview: Array.from( + { length: 24 }, + (_, line) => ` const request${line} = await repository.loadPending("queue-${index}");`, + ).join("\n"), +})) +const document = { + prompt: [{ type: "text", content: text, start: 0, end: text.length }], + cursor: text.length, + mode: "normal", + context: { items }, +} +type Probe = { active: boolean; encodes: number; bytes: number; inputs: number; keyups: number } +type ProbeWindow = typeof window & { composerWriteBatch: Probe } + +benchmark.use({ + viewport: { width: 1440, height: 900 }, + video: "off", + trace: "off", + serviceWorkers: "block", + traceScope: "interaction", +}) + +for (const scenario of ["typing", "cursor-movement", "cursor-noop", "submit-cleanup"] as const) { + benchmark(`composer-write-batch: ${scenario}`, async ({ page, report }, testInfo) => { + const submitted: Record[] = [] + await mockOpenCodeServer(page, { + directory: fixture.directory, + project: fixture.project, + provider: fixture.provider, + sessions: [{ ...fixture.sessions[0], id: sessionID, title }], + pageMessages: () => ({ items: [] }), + onPrompt: (input) => submitted.push(input.body), + }) + await page.addInitScript( + ({ key, value, counts }) => { + localStorage.setItem(key, JSON.stringify(value)) + const probe: Probe = { active: false, encodes: 0, bytes: 0, inputs: 0, keyups: 0 } + ;(window as ProbeWindow).composerWriteBatch = probe + // The draft adapter parses each schema-encoded composer document once before + // its asynchronous blob walk. Count at this boundary, not at the IDB write + // (which already discards superseded writes). This fixture is ASCII only. + if (counts) { + const parse = JSON.parse + JSON.parse = (value, reviver) => { + if (probe.active && typeof value === "string" && value.startsWith('{"prompt":[')) { + probe.encodes++ + probe.bytes += value.length + } + return parse(value, reviver) + } + } + window.addEventListener("input", (event) => { + if ( + probe.active && + event.target instanceof Element && + event.target.matches('[data-component="composer-editor"]') + ) + probe.inputs++ + }) + window.addEventListener("keyup", (event) => { + if ( + probe.active && + event.target instanceof Element && + event.target.matches('[data-component="composer-editor"]') + ) + probe.keyups++ + }) + }, + { + key: `${base64Encode(fixture.directory)}/prompt/${sessionID}.v2`, + value: document, + counts: process.env.OPENCODE_PERSISTENCE_COUNTS === "1", + }, + ) + const server = `http://${process.env.PLAYWRIGHT_SERVER_HOST ?? "127.0.0.1"}:${process.env.PLAYWRIGHT_SERVER_PORT ?? "4096"}` + await page.goto(`/server/${base64Encode(server)}/session/${sessionID}`) + await expectSessionTitle(page, title) + const editor = page.getByRole("textbox", { name: "Prompt", exact: true }) + await expect(editor).toBeEditable() + await expect(editor).toHaveText(text) + await editor.focus() + await editor.press("ControlOrMeta+End") + await page.evaluate(() => window.document.fonts.ready) + const stored = async () => + page.evaluate(async (sessionID) => { + const db = await new Promise((resolve, reject) => { + const request = indexedDB.open("opencode-drafts", 1) + request.onsuccess = () => resolve(request.result) + request.onerror = () => reject(request.error) + }) + try { + const transaction = db.transaction("documents") + const keys = transaction.objectStore("documents").getAllKeys() + const values = transaction.objectStore("documents").getAll() + await new Promise((resolve, reject) => { + transaction.oncomplete = () => resolve() + transaction.onerror = () => reject(transaction.error) + }) + const index = keys.result.findIndex((key) => String(key).endsWith(`session:${sessionID}:prompt`)) + // Parse after disabling the count so the observation is not part of it. + const probe = (window as ProbeWindow).composerWriteBatch + const active = probe.active + probe.active = false + const value = index < 0 ? undefined : JSON.parse(values.result[index]) + probe.active = active + return value as { prompt: { content: string }[]; cursor: number; context: { items: unknown[] } } | undefined + } finally { + db.close() + } + }, sessionID) + await expect.poll(async () => (await stored())?.cursor).toBe(text.length) + expect((await stored())?.context.items).toHaveLength(items.length) + const cdp = await page.context().newCDPSession(page) + await cdp.send("Performance.enable") + await benchmarkDiagnostics(page).startTrace() + const before = await cdp.send("Performance.getMetrics") + await page.evaluate(() => { + ;(window as ProbeWindow).composerWriteBatch.active = true + performance.mark("composer-write-batch-start") + }) + const start = performance.now() + if (scenario === "typing") await editor.pressSequentially(addition) + if (scenario === "cursor-movement") await editor.press("ArrowLeft") + if (scenario === "cursor-noop") await editor.press("ArrowRight") + if (scenario === "submit-cleanup") await editor.press("Enter") + const expectedText = scenario === "typing" ? text + addition : scenario === "submit-cleanup" ? "" : text + const expectedCursor = + scenario === "typing" + ? text.length + addition.length + : scenario === "submit-cleanup" + ? 0 + : text.length - Number(scenario === "cursor-movement") + await expect(editor).toHaveText(expectedText) + await expect.poll(async () => (await stored())?.cursor).toBe(expectedCursor) + const elapsedMs = performance.now() - start + const after = await cdp.send("Performance.getMetrics") + const probe = await page.evaluate(() => { + performance.mark("composer-write-batch-end") + const probe = (window as ProbeWindow).composerWriteBatch + probe.active = false + return probe + }) + expect((await stored())?.prompt.map((part) => part.content).join("")).toBe(expectedText) + if (scenario === "submit-cleanup") { + await expect.poll(() => submitted.length).toBe(1) + expect(submitted[0].text).toContain(text) + expect((await stored())?.context.items).toHaveLength(0) + } + expect(probe.keyups).toBe(scenario === "typing" ? addition.length : 1) + expect(probe.inputs).toBe(scenario === "typing" ? addition.length : 0) + const metric = (name: string) => + 1000 * + ((after.metrics.find((x) => x.name === name)?.value ?? 0) - + (before.metrics.find((x) => x.name === name)?.value ?? 0)) + report( + { elapsedMs, taskMs: metric("TaskDuration"), scriptMs: metric("ScriptDuration"), ...probe }, + { + scenario, + promptBytes: Buffer.byteLength(text), + contextItems: items.length, + persistedBytes: Buffer.byteLength(JSON.stringify(document)), + typedCharacters: scenario === "typing" ? addition.length : 0, + counts: process.env.OPENCODE_PERSISTENCE_COUNTS === "1", + browser: page.context().browser()!.version(), + build: process.env.OPENCODE_PERSISTENCE_BUILD, + transport: "playwright-route", + completion: "editor text and committed IDB cursor", + }, + ) + await benchmarkDiagnostics(page).stop() + await cdp.detach() + if (testInfo.repeatEachIndex === 0) await page.screenshot({ path: testInfo.outputPath(`${scenario}.png`) }) + }) +} diff --git a/packages/app/src/composer/editor/actions.ts b/packages/app/src/composer/editor/actions.ts index c1bef657199..e595cfc53dd 100644 --- a/packages/app/src/composer/editor/actions.ts +++ b/packages/app/src/composer/editor/actions.ts @@ -1,4 +1,4 @@ -import { batch, type Accessor } from "solid-js" +import { batch, untrack, type Accessor } from "solid-js" import type { SetStoreFunction, Store } from "solid-js/store" import type { ComposerAgentPart, @@ -23,43 +23,47 @@ export function createComposerEditorActions(input: ComposerStateStoreInput) { return typeof value === "function" ? value() : value } const setStore = () => tuple()[1] - const clearRetry = () => setStore()("retry", undefined) + const clearRetry = () => { + if (untrack(() => store().retry) !== undefined) setStore()("retry", undefined) + } return { get state() { return store() }, setPrompt(prompt: ComposerPrompt, cursor?: number) { - batch(() => { - setStore()("prompt", prompt) - if (cursor !== undefined) setStore()("cursor", cursor) - clearRetry() - }) + // Persisted setters encode on every call, even inside a reactive batch. + batch(() => setStore()({ prompt, ...(cursor !== undefined ? { cursor } : {}), retry: undefined })) }, setCursor(cursor: number) { + if (untrack(() => store().cursor) === cursor) return setStore()("cursor", cursor) }, setMode(mode: "normal" | "shell") { - setStore()("mode", mode) - clearRetry() + if (untrack(() => store().mode === mode && store().retry === undefined)) return + setStore()({ mode, retry: undefined }) }, setText(content: string) { - batch(() => { - setStore()("prompt", (prompt) => [ - { type: "text", content, start: 0, end: content.length }, - ...prompt.filter((part) => part.type === "image"), - ]) - setStore()("cursor", content.length) - clearRetry() - }) + batch(() => + setStore()((state) => ({ + prompt: [ + { type: "text", content, start: 0, end: content.length }, + ...state.prompt.filter((part) => part.type === "image"), + ], + cursor: content.length, + retry: undefined, + })), + ) }, addText(content: string) { const cursor = store().cursor ?? promptLength(store().prompt) - batch(() => { - setStore()("prompt", (prompt) => insertText(prompt, cursor, content)) - setStore()("cursor", cursor + content.length) - clearRetry() - }) + batch(() => + setStore()((state) => ({ + prompt: insertText(state.prompt, cursor, content), + cursor: cursor + content.length, + retry: undefined, + })), + ) }, removeContext(key: string) { setStore()("context", "items", (items) => items.filter((item) => item.key !== key)) diff --git a/packages/app/src/composer/state.ts b/packages/app/src/composer/state.ts index 86fc97b495c..130bc201021 100644 --- a/packages/app/src/composer/state.ts +++ b/packages/app/src/composer/state.ts @@ -1,4 +1,4 @@ -import { batch, type Accessor } from "solid-js" +import { batch, untrack, type Accessor } from "solid-js" import { createStore, type SetStoreFunction } from "solid-js/store" import { Persist, persisted } from "@/runtime/persistence/storage" import { ServerScope } from "@/runtime/server/scope" @@ -43,19 +43,16 @@ export function isCommentItem(item: ContextItem | (ContextItem & { key: string } function createComposerActions(setStore: SetStoreFunction) { return { set(prompt: Prompt, cursorPosition?: number) { - const next = clonePrompt(prompt) - batch(() => { - setStore("prompt", next) - if (cursorPosition !== undefined) setStore("cursor", cursorPosition) - setStore("retry", undefined) - }) + batch(() => + setStore({ + prompt: clonePrompt(prompt), + ...(cursorPosition !== undefined ? { cursor: cursorPosition } : {}), + retry: undefined, + }), + ) }, reset() { - batch(() => { - setStore("prompt", clonePrompt(DEFAULT_PROMPT)) - setStore("cursor", 0) - setStore("retry", undefined) - }) + batch(() => setStore({ prompt: clonePrompt(DEFAULT_PROMPT), cursor: 0, retry: undefined })) }, } } @@ -86,7 +83,9 @@ function initialComposerStore(initial?: InitialPrompt): ComposerStore { function createComposerStateValue(store: ComposerStore, setStore: SetStoreFunction) { const actions = createComposerActions(setStore) - const clearRetry = () => setStore("retry", undefined) + const clearRetry = () => { + if (untrack(() => store.retry) !== undefined) setStore("retry", undefined) + } const value = { store: [() => store, setStore] as [Accessor, SetStoreFunction], current: () => store.prompt, @@ -101,8 +100,8 @@ function createComposerStateValue(store: ComposerStore, setStore: SetStoreFuncti mode: { current: () => store.mode ?? "normal", set: (mode: "normal" | "shell") => { - setStore("mode", mode) - clearRetry() + if (untrack(() => store.mode === mode && store.retry === undefined)) return + setStore({ mode, retry: undefined }) }, }, retry: { diff --git a/packages/app/test-browser/composer-write-batch.test.ts b/packages/app/test-browser/composer-write-batch.test.ts new file mode 100644 index 00000000000..a776b654d09 --- /dev/null +++ b/packages/app/test-browser/composer-write-batch.test.ts @@ -0,0 +1,186 @@ +import { expect, test } from "bun:test" +import { createComputed, createRoot } from "solid-js" +import { SessionMessage } from "@opencode-ai/schema/session-message" +import { ServerScope } from "@/runtime/server/scope" +import { createComposerState, type ComposerStore } from "@/composer/state" +import { createComposerEditorActions } from "@/composer/editor/actions" + +function setup(read: () => string | null | Promise = () => null) { + return createRoot((dispose) => { + const writes: ComposerStore[] = [] + const state = createComposerState(ServerScope.local, { draftID: "composer-write-batch-test" }, undefined, { + platform: "desktop", + os: "windows", + windowID: "composer-write-batch-test", + openExternal() {}, + restart: async () => {}, + notify: async () => {}, + storage: () => ({ + getItem: read, + setItem: (_key, value) => { + writes.push(JSON.parse(value)) + }, + removeItem() {}, + }), + }) + return { state, writes, editor: createComposerEditorActions(state.store), dispose } + }) +} + +test("composer-write-batch: typing persists prompt, cursor and retry together before returning", async () => { + const value = setup() + try { + await value.state.ready.promise + value.state.context.add({ type: "file", path: "src/queue.ts", preview: "await queue.flush()" }) + value.state.retry.set({ id: SessionMessage.ID.create(), agent: "build", providerID: "test", modelID: "test" }) + value.writes.length = 0 + value.editor.setPrompt([{ type: "text", content: "keep ordering", start: 0, end: 13 }], 13) + expect(value.writes).toHaveLength(1) + expect(value.writes[0]).toMatchObject({ + prompt: [{ type: "text", content: "keep ordering", start: 0, end: 13 }], + cursor: 13, + context: { items: [{ path: "src/queue.ts", preview: "await queue.flush()" }] }, + }) + expect(value.writes[0].retry).toBeUndefined() + value.editor.setCursor(13) + expect(value.writes).toHaveLength(1) + value.editor.setCursor(12) + expect(value.writes.map((write) => write.cursor)).toEqual([13, 12]) + value.editor.setCursor(12) + expect(value.writes).toHaveLength(2) + } finally { + value.dispose() + } +}) + +test("composer-write-batch: state replacement preserves an omitted cursor and resets in one write", async () => { + const value = setup() + try { + await value.state.ready.promise + value.state.set([{ type: "text", content: "previous", start: 0, end: 8 }], 5) + value.state.mode.set("shell") + value.state.retry.set({ id: SessionMessage.ID.create(), agent: "build", providerID: "test", modelID: "test" }) + value.writes.length = 0 + const prompt = [{ type: "text" as const, content: "next", start: 0, end: 4 }] + value.state.set(prompt) + prompt[0].content = "changed outside the store" + expect(value.state.current()[0]).toMatchObject({ content: "next" }) + expect(value.writes).toHaveLength(1) + expect(value.writes[0].cursor).toBe(5) + expect(value.writes[0].retry).toBeUndefined() + value.state.reset() + expect(value.writes).toHaveLength(2) + expect(value.writes[1]).toMatchObject({ prompt: [{ content: "" }], cursor: 0, mode: "shell" }) + value.state.mode.set("normal") + value.state.mode.set("normal") + expect(value.writes).toHaveLength(3) + expect(value.writes[2].mode).toBe("normal") + } finally { + value.dispose() + } +}) + +test("composer-write-batch: unchanged mode still clears a retry and context writes remain ordered", async () => { + const value = setup() + try { + await value.state.ready.promise + value.state.mode.set("normal") + value.state.retry.set({ id: SessionMessage.ID.create(), agent: "build", providerID: "test", modelID: "test" }) + value.writes.length = 0 + value.editor.setMode("normal") + expect(value.writes).toHaveLength(1) + expect(value.writes[0].retry).toBeUndefined() + value.state.context.add({ type: "file", path: "first.ts" }) + value.state.context.add({ type: "file", path: "second.ts" }) + value.state.context.remove(value.state.context.items()[0].key) + expect(value.writes.slice(1).map((write) => write.context.items.map((item) => item.path))).toEqual([ + ["first.ts"], + ["first.ts", "second.ts"], + ["second.ts"], + ]) + } finally { + value.dispose() + } +}) + +test("composer-write-batch: text replacement and insertion each persist once and retain attachments", async () => { + const value = setup() + try { + await value.state.ready.promise + value.state.set( + [ + { type: "text", content: "old", start: 0, end: 3 }, + { + type: "image", + id: "image", + filename: "diagram.png", + mime: "image/png", + blob: { id: "diagram", url: "blob:diagram" }, + }, + ], + 3, + ) + value.writes.length = 0 + value.editor.setText("new") + value.editor.addText(" notes") + expect(value.writes).toHaveLength(2) + expect(value.writes.map((write) => write.cursor)).toEqual([3, 9]) + expect(value.writes[1].prompt).toEqual([ + { type: "text", content: "new notes", start: 0, end: 9 }, + { + type: "image", + id: "image", + filename: "diagram.png", + mime: "image/png", + blob: { id: "diagram", url: "blob:diagram" }, + }, + ]) + } finally { + value.dispose() + } +}) + +test("composer-write-batch: an edit still wins over a pending persisted read", async () => { + const loading = Promise.withResolvers() + const value = setup(() => loading.promise) + try { + value.editor.setPrompt([{ type: "text", content: "new", start: 0, end: 3 }], 3) + expect(value.writes).toHaveLength(1) + loading.resolve( + JSON.stringify({ + prompt: [{ type: "text", content: "old", start: 0, end: 3 }], + cursor: 1, + context: { items: [] }, + }), + ) + await value.state.ready.promise + expect(value.state.current()).toEqual([{ type: "text", content: "new", start: 0, end: 3 }]) + expect(value.state.cursor()).toBe(3) + expect(value.writes).toHaveLength(1) + } finally { + value.dispose() + } +}) + +test("composer-write-batch: persistence still precedes reactive observers", async () => { + const value = setup() + try { + await value.state.ready.promise + createRoot((dispose) => { + const observed: number[] = [] + createComputed(() => { + value.state.current() + observed.push(value.writes.length) + }) + value.editor.setPrompt([{ type: "text", content: "new", start: 0, end: 3 }], 3) + value.editor.setText("replace") + value.editor.addText(" and insert") + value.state.set([{ type: "text", content: "restore", start: 0, end: 7 }], 7) + value.state.reset() + expect(observed).toEqual([0, 1, 2, 3, 4, 5]) + dispose() + }) + } finally { + value.dispose() + } +})