diff --git a/packages/app/e2e/browser/agent-timeline-scroll-smoothness.spec.ts b/packages/app/e2e/browser/agent-timeline-scroll-smoothness.spec.ts index ee4201ea75..feceed7869 100644 --- a/packages/app/e2e/browser/agent-timeline-scroll-smoothness.spec.ts +++ b/packages/app/e2e/browser/agent-timeline-scroll-smoothness.spec.ts @@ -2,6 +2,7 @@ import { test } from "../support/fixtures"; import { expect } from "@playwright/test"; import { findScrollJumps, + expectImageSpaceReserved, type ScrollFrame, observeTimelinePages, openOnlyTimelineTail, @@ -145,3 +146,7 @@ for (const cadence of scrollCadences) { }); }); } + +test("reserves image space before its response arrives", async ({ page }, testInfo) => { + await expectImageSpaceReserved(page, testInfo); +}); diff --git a/packages/app/e2e/support/helpers/timeline-scroll-smoothness.ts b/packages/app/e2e/support/helpers/timeline-scroll-smoothness.ts index f550b02fdc..01055ce2c1 100644 --- a/packages/app/e2e/support/helpers/timeline-scroll-smoothness.ts +++ b/packages/app/e2e/support/helpers/timeline-scroll-smoothness.ts @@ -171,9 +171,27 @@ export async function recordUpwardTraversal( let lastWheelAt = -Infinity; let inputFinishedAt: number | null = null; let imageLoads = 0; - let mounted = 0; + let mounted = scroll.querySelectorAll("[data-history-row-id]").length; let unmounted = 0; - let previousIds = new Set(); + const collectRows = (node: Node, rows: Set) => { + if (!(node instanceof Element)) return; + if (node.matches("[data-history-row-id]")) rows.add(node); + for (const row of node.querySelectorAll("[data-history-row-id]")) rows.add(row); + }; + const recordMutations = (records: MutationRecord[]) => { + // A newly inserted container and its children can both appear in one + // delivery. Count each DOM row once, including rows removed before paint. + const added = new Set(); + const removed = new Set(); + for (const record of records) { + for (const node of record.addedNodes) collectRows(node, added); + for (const node of record.removedNodes) collectRows(node, removed); + } + mounted += added.size; + unmounted += removed.size; + }; + const mutations = new MutationObserver(recordMutations); + mutations.observe(scroll, { childList: true, subtree: true }); const wheel = (event: WheelEvent) => { wheelTotal += -event.deltaY; lastWheelAt = performance.now(); @@ -205,10 +223,8 @@ export async function recordUpwardTraversal( const box = row.getBoundingClientRect(); return { id: row.dataset.historyRowId!, top: box.top - rect.top, height: box.height }; }); - const ids = new Set(rows.map((row) => row.id)); - mounted += [...ids].filter((id) => !previousIds.has(id)).length; - unmounted += [...previousIds].filter((id) => !ids.has(id)).length; - previousIds = ids; + // Include rows mounted and removed in the same commit, before sampling. + recordMutations(mutations.takeRecords()); // Preserve the row crossing the reading line. Rows below an expanding // visible image legitimately move even when the reading position is stable. const anchor = rows.find((row) => row.top < rect.height && row.top + row.height > 8); @@ -246,6 +262,7 @@ export async function recordUpwardTraversal( stop() { cancelAnimationFrame(frame); resizeObserver.disconnect(); + mutations.disconnect(); scroll.removeEventListener("wheel", wheel); scroll.removeEventListener("load", imageLoad, true); }, @@ -362,9 +379,9 @@ export async function reportScrollJumps( worstReversePx: Math.max(0, ...jumps.map((jump) => -jump.movement)), frameGapP95Ms: gaps[Math.floor(gaps.length * 0.95)], frameGapMaxMs: gaps.at(-1), - imageLoads: frames.at(-1)?.imageLoads, - mounted: frames.at(-1)?.mounted, - unmounted: frames.at(-1)?.unmounted, + imageLoads: frames[frames.length - 1].imageLoads, + mounted: frames[frames.length - 1].mounted, + unmounted: frames[frames.length - 1].unmounted, jumps, }; await writeFile(testInfo.outputPath("scroll-summary.json"), JSON.stringify(summary, null, 2)); @@ -382,7 +399,7 @@ export async function reportScrollJumps( body: JSON.stringify(react), contentType: "application/json", }); - await testInfo.attach("scroll-end", { body: await page.screenshot(), contentType: "image/png" }); + await attachTimelineScreenshot(page, testInfo, "scroll-end"); console.log( `[scroll] ${testInfo.title}: ${JSON.stringify({ ...summary, jumps: jumps.slice(0, 5) })}`, ); @@ -395,6 +412,15 @@ export async function reportScrollJumps( "must exercise virtualization", ).toBe(true); expect(summary.imageLoads, "must exercise image completion during traversal").toBeGreaterThan(0); + expect( + summary.mounted - summary.unmounted, + "row counters must balance against the final DOM", + ).toBe(frames[frames.length - 1].rows.length); + const uniqueRows = new Set(frames.flatMap((frame) => frame.rows.map((row) => row.id))).size; + expect( + summary.mounted, + "prepending must not mount a discarded range before correcting the viewport", + ).toBeLessThanOrEqual(uniqueRows * 1.25); expect( jumps, "visible text must not reverse direction while scrolling upward or move after input stops", @@ -432,3 +458,80 @@ async function traceScrollIfRequested( await testInfo.attach("scroll-chromium-trace", { path, contentType: "application/json" }); }; } + +/** Hold real image bytes until the reader has reached its placeholder. */ +export async function expectImageSpaceReserved(page: Page, testInfo: TestInfo): Promise { + let released = false; + const pending = new Set<() => void>(); + const server = createServer((_request, response) => { + const send = () => { + response.writeHead(200, { "Content-Type": "image/svg+xml", "Cache-Control": "no-store" }); + response.end( + '', + ); + }; + if (released) send(); + else pending.add(send); + }); + await new Promise((resolve) => server.listen(0, "127.0.0.1", resolve)); + try { + const address = server.address(); + if (!address || typeof address === "string") throw new Error("Image server did not listen"); + const agent = await seedMockAgentWorkspace({ + repoPrefix: "reserved-timeline-image-", + title: "Reserved image geometry", + featureValues: { + mockAssistantResponses: [ + `Before the image.\n\n![Reserved image](http://127.0.0.1:${address.port}/image.svg)\n\nText below the image.`, + ], + }, + }); + try { + await agent.client.sendAgentMessage(agent.agentId, "Show the delayed image"); + await agent.client.waitForFinish(agent.agentId, 15_000); + await openAgentTimeline(page, agent); + await expectTimelinePromptVisible(page, "Show the delayed image"); + const image = page.getByRole("img", { name: "Reserved image" }).first(); + await expect(image).toBeVisible(); + const before = await image.boundingBox(); + await attachTimelineScreenshot(page, testInfo, "image-placeholder"); + released = true; + for (const send of pending) send(); + await expect + .poll(() => + image.evaluate((element) => { + const img = + element instanceof HTMLImageElement ? element : element.querySelector("img"); + return img?.naturalWidth; + }), + ) + .toBe(240); + await expect + .poll(async () => { + const box = await image.boundingBox(); + return box ? Math.round((box.width / box.height) * 100) : 0; + }) + .toBe(150); + const after = await image.boundingBox(); + await attachTimelineScreenshot(page, testInfo, "image-loaded"); + expect(before?.height, "reserve the final image height before the HTTP response").toBe( + after?.height, + ); + } finally { + await agent.cleanup(); + } + } finally { + server.closeAllConnections(); + await new Promise((resolve) => server.close(() => resolve())); + } +} + +async function attachTimelineScreenshot( + page: Page, + testInfo: TestInfo, + name: string, +): Promise { + const path = testInfo.outputPath(`${name}.png`); + await page.getByTestId("agent-chat-scroll").screenshot({ path }); + await testInfo.attach(name, { path, contentType: "image/png" }); +} diff --git a/packages/app/src/agent-stream/reading-anchor.test.ts b/packages/app/src/agent-stream/reading-anchor.test.ts index 19b6bc9e31..74b2bd5bcd 100644 --- a/packages/app/src/agent-stream/reading-anchor.test.ts +++ b/packages/app/src/agent-stream/reading-anchor.test.ts @@ -78,6 +78,35 @@ describe("reading anchor", () => { ).toBe(2600); }); + it("projects a prepend range without consuming the correction before layout commits", () => { + const anchor = createReadingAnchor(); + anchor.reconcile(600, rows); + expect(anchor.project(440, { id: "reading", top: 2500 })).toBe(2440); + expect(anchor.project(440, { id: "reading", top: 2500 })).toBe(2440); + expect( + anchor.reconcile( + 440, + rows.map((row) => ({ ...row, top: row.top + 2000 })), + true, + ), + ).toBe(2440); + expect(anchor.getRowId()).toBe("above"); + }); + + it("repicks from user movement before applying a simultaneous layout correction", () => { + const anchor = createReadingAnchor(); + anchor.reconcile(600, rows); + anchor.scroll(440); + expect(anchor.getRowId()).toBe("above"); + expect( + anchor.reconcile( + 440, + rows.map((row) => ({ ...row, top: row.top + 2000 })), + ), + ).toBe(2440); + expect(anchor.getRowId()).toBe("above"); + }); + it("releases the old reading position for explicit navigation", () => { const anchor = createReadingAnchor(); anchor.reconcile(600, rows); diff --git a/packages/app/src/agent-stream/reading-anchor.ts b/packages/app/src/agent-stream/reading-anchor.ts index 31b965f134..4299f1d9c8 100644 --- a/packages/app/src/agent-stream/reading-anchor.ts +++ b/packages/app/src/agent-stream/reading-anchor.ts @@ -4,27 +4,40 @@ interface RowGeometry { height: number; } -// Store content coordinates, never a frozen viewport offset: user scrolling must -// survive a layout correction, including input delivered while a page is loading. +// A row must clear the reading line before the next row takes ownership. +const READING_POSITION_OFFSET_PX = 8; + +// Content coordinates survive user scrolling. Layout commits replace the geometry; +// scroll events reuse it, without another DOM measurement pass. export function createReadingAnchor() { let anchor: { id: string; top: number } | null = null; + let geometry: readonly RowGeometry[] = []; + const readingRow = (scrollTop: number) => + geometry.find((row) => row.top + row.height > scrollTop + READING_POSITION_OFFSET_PX); + const project = (scrollTop: number, row: Pick | undefined) => + scrollTop + (anchor && row?.id === anchor.id ? row.top - anchor.top : 0); return { getRowId: () => anchor?.id ?? null, + getReadingRowId: (scrollTop: number) => + readingRow(scrollTop)?.id ?? geometry.at(-1)?.id ?? null, + project, reset() { anchor = null; }, + scroll(scrollTop: number) { + const next = readingRow(scrollTop); + anchor = next ? { id: next.id, top: next.top } : null; + }, reconcile(scrollTop: number, rows: readonly RowGeometry[], userScrolled = false): number { + geometry = rows; const previous = anchor && rows.find((row) => row.id === anchor?.id); // The first virtualized commit can precede its mounted range. Keep the // pinned reader until it mounts, rather than adopting an unrelated row. if (anchor && !previous && !userScrolled) return scrollTop; - const correctedTop = scrollTop + (previous && anchor ? previous.top - anchor.top : 0); + const correctedTop = project(scrollTop, previous ?? undefined); // A prepend can expose the bottom of an estimated row above the reader. // Do not transfer ownership to it until the user moves the reading position. - const next = - previous && !userScrolled - ? previous - : rows.find((row) => row.top + row.height > correctedTop + 8); + const next = previous && !userScrolled ? previous : readingRow(correctedTop); anchor = next ? { id: next.id, top: next.top } : null; return correctedTop; }, diff --git a/packages/app/src/agent-stream/strategy-web.test.tsx b/packages/app/src/agent-stream/strategy-web.test.tsx index c0fe52e3fc..9078b0f1c3 100644 --- a/packages/app/src/agent-stream/strategy-web.test.tsx +++ b/packages/app/src/agent-stream/strategy-web.test.tsx @@ -491,10 +491,7 @@ describe("createWebStreamStrategy", () => { } Object.defineProperty(scrollContainer, "clientHeight", { configurable: true, value: 766 }); Object.defineProperty(scrollContainer, "scrollHeight", { configurable: true, value: 5725 }); - Object.defineProperty(scrollContainer, "scrollTop", { - configurable: true, - value: 4959.1708984375, - }); + scrollContainer.scrollTop = 4959.1708984375; scrollTo.mockClear(); act(() => { @@ -504,10 +501,7 @@ describe("createWebStreamStrategy", () => { expect(scrollTo).toHaveBeenCalledWith({ top: 5725, behavior: "auto" }); scrollTo.mockClear(); - Object.defineProperty(scrollContainer, "scrollTop", { - configurable: true, - value: 4960.5, - }); + scrollContainer.scrollTop = 4960.5; act(() => { viewportRef.current?.scrollToBottom("message-sent"); @@ -516,10 +510,7 @@ describe("createWebStreamStrategy", () => { expect(scrollTo).toHaveBeenCalledWith({ top: 5725, behavior: "auto" }); scrollTo.mockClear(); - Object.defineProperty(scrollContainer, "scrollTop", { - configurable: true, - value: 4967, - }); + scrollContainer.scrollTop = 4967; act(() => { viewportRef.current?.scrollToBottom("message-sent"); @@ -535,7 +526,7 @@ describe("createWebStreamStrategy", () => { y?: number, ) { const top = typeof options === "object" ? (options.top ?? 0) : (y ?? 0); - Object.defineProperty(this, "scrollTop", { configurable: true, value: top }); + this.scrollTop = top; }); const strategy = createWebStreamStrategy({ isMobileBreakpoint: true }); const viewportRef = React.createRef(); @@ -585,7 +576,7 @@ describe("createWebStreamStrategy", () => { } Object.defineProperty(scrollContainer, "clientHeight", { configurable: true, value: 400 }); Object.defineProperty(scrollContainer, "scrollHeight", { configurable: true, value: 1200 }); - Object.defineProperty(scrollContainer, "scrollTop", { configurable: true, value: 0 }); + scrollContainer.scrollTop = 0; act(() => { root?.render( @@ -621,10 +612,7 @@ describe("createWebStreamStrategy", () => { y?: number, ) { const top = typeof options === "object" ? (options.top ?? 0) : (y ?? 0); - Object.defineProperty(this, "scrollTop", { - configurable: true, - value: top, - }); + this.scrollTop = top; }); HTMLElement.prototype.scrollTo = scrollTo; @@ -682,7 +670,7 @@ describe("createWebStreamStrategy", () => { const scrollElement = scrollContainer; Object.defineProperty(scrollElement, "clientHeight", { configurable: true, value: 400 }); Object.defineProperty(scrollElement, "scrollHeight", { configurable: true, value: 400 }); - Object.defineProperty(scrollElement, "scrollTop", { configurable: true, value: 0 }); + scrollElement.scrollTop = 0; await act(async () => { await new Promise((resolve) => requestAnimationFrame(resolve)); }); @@ -724,10 +712,7 @@ describe("createWebStreamStrategy", () => { y?: number, ) { const top = typeof options === "object" ? (options.top ?? 0) : (y ?? 0); - Object.defineProperty(this, "scrollTop", { - configurable: true, - value: top, - }); + this.scrollTop = top; }); HTMLElement.prototype.scrollTo = scrollTo; @@ -786,7 +771,7 @@ describe("createWebStreamStrategy", () => { } Object.defineProperty(scrollContainer, "clientHeight", { configurable: true, value: 400 }); Object.defineProperty(scrollContainer, "scrollHeight", { configurable: true, value: 1400 }); - Object.defineProperty(scrollContainer, "scrollTop", { configurable: true, value: 1000 }); + scrollContainer.scrollTop = 1000; await act(async () => { await new Promise((resolve) => requestAnimationFrame(resolve)); }); @@ -798,7 +783,7 @@ describe("createWebStreamStrategy", () => { act(() => { scrollContainer.dispatchEvent(new WheelEvent("wheel", { deltaY: -240 })); }); - Object.defineProperty(scrollContainer, "scrollTop", { configurable: true, value: 0 }); + scrollContainer.scrollTop = 0; act(() => { scrollContainer.dispatchEvent(new Event("scroll")); }); @@ -831,10 +816,7 @@ describe("createWebStreamStrategy", () => { y?: number, ) { const top = typeof options === "object" ? (options.top ?? 0) : (y ?? 0); - Object.defineProperty(this, "scrollTop", { - configurable: true, - value: top, - }); + this.scrollTop = top; }); HTMLElement.prototype.scrollTo = scrollTo; @@ -892,7 +874,7 @@ describe("createWebStreamStrategy", () => { } Object.defineProperty(scrollContainer, "clientHeight", { configurable: true, value: 500 }); Object.defineProperty(scrollContainer, "scrollHeight", { configurable: true, value: 1491 }); - Object.defineProperty(scrollContainer, "scrollTop", { configurable: true, value: 0 }); + scrollContainer.scrollTop = 0; await act(async () => { await new Promise((resolve) => requestAnimationFrame(resolve)); }); @@ -901,17 +883,17 @@ describe("createWebStreamStrategy", () => { }); scrollTo.mockClear(); - Object.defineProperty(scrollContainer, "scrollTop", { configurable: true, value: 991 }); + scrollContainer.scrollTop = 991; act(() => { scrollContainer.dispatchEvent(new WheelEvent("wheel", { deltaY: -900 })); }); - Object.defineProperty(scrollContainer, "scrollTop", { configurable: true, value: 91 }); + scrollContainer.scrollTop = 91; act(() => { scrollContainer.dispatchEvent(new Event("scroll")); }); expect(scrollTo).not.toHaveBeenCalled(); - Object.defineProperty(scrollContainer, "scrollTop", { configurable: true, value: 0 }); + scrollContainer.scrollTop = 0; act(() => { scrollContainer.dispatchEvent(new Event("scroll")); }); @@ -937,10 +919,7 @@ describe("createWebStreamStrategy", () => { y?: number, ) { const top = typeof options === "object" ? (options.top ?? 0) : (y ?? 0); - Object.defineProperty(this, "scrollTop", { - configurable: true, - value: top, - }); + this.scrollTop = top; }); HTMLElement.prototype.scrollTo = scrollTo; @@ -992,7 +971,7 @@ describe("createWebStreamStrategy", () => { } Object.defineProperty(scrollContainer, "clientHeight", { configurable: true, value: 500 }); Object.defineProperty(scrollContainer, "scrollHeight", { configurable: true, value: 550 }); - Object.defineProperty(scrollContainer, "scrollTop", { configurable: true, value: 50 }); + scrollContainer.scrollTop = 50; await act(async () => { await new Promise((resolve) => requestAnimationFrame(resolve)); }); @@ -1004,13 +983,13 @@ describe("createWebStreamStrategy", () => { act(() => { scrollContainer.dispatchEvent(new WheelEvent("wheel", { deltaY: -30 })); }); - Object.defineProperty(scrollContainer, "scrollTop", { configurable: true, value: 20 }); + scrollContainer.scrollTop = 20; act(() => { scrollContainer.dispatchEvent(new Event("scroll")); }); expect(scrollTo).not.toHaveBeenCalled(); - Object.defineProperty(scrollContainer, "scrollTop", { configurable: true, value: 50 }); + scrollContainer.scrollTop = 50; act(() => { scrollContainer.dispatchEvent(new Event("scroll")); }); @@ -1038,7 +1017,7 @@ describe("createWebStreamStrategy", () => { it("keeps following output after geometry, nested-scroll, and zoom wheel events", () => { const scrollTo = vi.fn(function (this: HTMLElement, options?: ScrollToOptions | number) { const top = typeof options === "object" ? (options.top ?? 0) : 0; - Object.defineProperty(this, "scrollTop", { configurable: true, value: top }); + this.scrollTop = top; }); HTMLElement.prototype.scrollTo = scrollTo; const strategy = createWebStreamStrategy({ isMobileBreakpoint: true }); @@ -1084,7 +1063,7 @@ describe("createWebStreamStrategy", () => { Object.defineProperty(scrollContainer, "clientWidth", { configurable: true, value: 500 }); Object.defineProperty(scrollContainer, "offsetWidth", { configurable: true, value: 500 }); Object.defineProperty(scrollContainer, "scrollHeight", { configurable: true, value: 1500 }); - Object.defineProperty(scrollContainer, "scrollTop", { configurable: true, value: 1000 }); + scrollContainer.scrollTop = 1000; scrollContainer.getBoundingClientRect = () => ({ bottom: 500, @@ -1124,7 +1103,7 @@ describe("createWebStreamStrategy", () => { window.dispatchEvent(jitterPointerMove); }); Object.defineProperty(scrollContainer, "clientHeight", { configurable: true, value: 700 }); - Object.defineProperty(scrollContainer, "scrollTop", { configurable: true, value: 800 }); + scrollContainer.scrollTop = 800; act(() => scrollContainer.dispatchEvent(new Event("scroll"))); scrollTo.mockClear(); @@ -1132,11 +1111,11 @@ describe("createWebStreamStrategy", () => { nestedScroller.style.overflowY = "auto"; Object.defineProperty(nestedScroller, "clientHeight", { configurable: true, value: 100 }); Object.defineProperty(nestedScroller, "scrollHeight", { configurable: true, value: 300 }); - Object.defineProperty(nestedScroller, "scrollTop", { configurable: true, value: 100 }); + nestedScroller.scrollTop = 100; scrollContainer.append(nestedScroller); act(() => { nestedScroller.dispatchEvent(new WheelEvent("wheel", { bubbles: true, deltaY: -100 })); - Object.defineProperty(scrollContainer, "scrollTop", { configurable: true, value: 700 }); + scrollContainer.scrollTop = 700; scrollContainer.dispatchEvent(new Event("scroll")); scrollContainer.dispatchEvent( new WheelEvent("wheel", { bubbles: true, ctrlKey: true, deltaY: -100 }), @@ -1159,7 +1138,7 @@ describe("createWebStreamStrategy", () => { it("stops following output after scrollbar and middle-button upward scrolling", () => { const scrollTo = vi.fn(function (this: HTMLElement, options?: ScrollToOptions | number) { const top = typeof options === "object" ? (options.top ?? 0) : 0; - Object.defineProperty(this, "scrollTop", { configurable: true, value: top }); + this.scrollTop = top; }); HTMLElement.prototype.scrollTo = scrollTo; const strategy = createWebStreamStrategy({ isMobileBreakpoint: true }); @@ -1204,7 +1183,7 @@ describe("createWebStreamStrategy", () => { Object.defineProperty(scrollContainer, "clientWidth", { configurable: true, value: 500 }); Object.defineProperty(scrollContainer, "offsetWidth", { configurable: true, value: 520 }); Object.defineProperty(scrollContainer, "scrollHeight", { configurable: true, value: 1500 }); - Object.defineProperty(scrollContainer, "scrollTop", { configurable: true, value: 1000 }); + scrollContainer.scrollTop = 1000; scrollContainer.getBoundingClientRect = () => ({ bottom: 500, @@ -1232,7 +1211,7 @@ describe("createWebStreamStrategy", () => { pointerType: { value: "mouse" }, }); act(() => scrollContainer.dispatchEvent(scrollbarPointerDown)); - Object.defineProperty(scrollContainer, "scrollTop", { configurable: true, value: 700 }); + scrollContainer.scrollTop = 700; act(() => scrollContainer.dispatchEvent(new Event("scroll"))); scrollTo.mockClear(); @@ -1248,7 +1227,7 @@ describe("createWebStreamStrategy", () => { }); expect(scrollTo).not.toHaveBeenCalled(); - Object.defineProperty(scrollContainer, "scrollTop", { configurable: true, value: 1300 }); + scrollContainer.scrollTop = 1300; act(() => scrollContainer.dispatchEvent(new Event("scroll"))); Object.defineProperty(scrollContainer, "offsetWidth", { configurable: true, value: 500 }); scrollContainer.getBoundingClientRect = () => @@ -1276,7 +1255,7 @@ describe("createWebStreamStrategy", () => { pointerType: { value: "mouse" }, }); act(() => scrollContainer.dispatchEvent(overlayScrollbarPointerDown)); - Object.defineProperty(scrollContainer, "scrollTop", { configurable: true, value: 900 }); + scrollContainer.scrollTop = 900; act(() => scrollContainer.dispatchEvent(new Event("scroll"))); scrollTo.mockClear(); @@ -1295,7 +1274,7 @@ describe("createWebStreamStrategy", () => { }); expect(scrollTo).not.toHaveBeenCalled(); - Object.defineProperty(scrollContainer, "scrollTop", { configurable: true, value: 1400 }); + scrollContainer.scrollTop = 1400; act(() => scrollContainer.dispatchEvent(new Event("scroll"))); const autoscrollPointerDown = new MouseEvent("pointerdown", { bubbles: true, @@ -1329,7 +1308,7 @@ describe("createWebStreamStrategy", () => { window.dispatchEvent(autoscrollPointerUp); window.dispatchEvent(autoscrollPointerMove); }); - Object.defineProperty(scrollContainer, "scrollTop", { configurable: true, value: 1000 }); + scrollContainer.scrollTop = 1000; act(() => scrollContainer.dispatchEvent(new Event("scroll"))); scrollTo.mockClear(); @@ -1390,7 +1369,7 @@ describe("createWebStreamStrategy", () => { }; const scrollTo = vi.fn(function (this: HTMLElement, options?: ScrollToOptions | number) { const top = typeof options === "object" ? (options.top ?? 0) : 0; - Object.defineProperty(this, "scrollTop", { configurable: true, value: top }); + this.scrollTop = top; }); HTMLElement.prototype.scrollTo = scrollTo; const strategy = createWebStreamStrategy({ isMobileBreakpoint: true }); @@ -1447,12 +1426,18 @@ describe("createWebStreamStrategy", () => { throw new Error("Expected retained history rows"); } let rowsShifted = false; - scrollContainer.getBoundingClientRect = vi.fn(() => ({ top: 0 }) as DOMRect); - firstRow.getBoundingClientRect = vi.fn(() => ({ bottom: rowsShifted ? 4 : 120 }) as DOMRect); - secondRow.getBoundingClientRect = vi.fn(() => ({ bottom: rowsShifted ? 120 : 160 }) as DOMRect); + scrollContainer.getBoundingClientRect = () => ({ top: 0, height: 500, bottom: 500 }) as DOMRect; + firstRow.getBoundingClientRect = () => + ({ top: rowsShifted ? -116 : 0, height: 120, bottom: rowsShifted ? 4 : 120 }) as DOMRect; + secondRow.getBoundingClientRect = () => + ({ + top: rowsShifted ? 4 : 120, + height: rowsShifted ? 116 : 40, + bottom: rowsShifted ? 120 : 160, + }) as DOMRect; Object.defineProperty(scrollContainer, "clientHeight", { configurable: true, value: 500 }); Object.defineProperty(scrollContainer, "scrollHeight", { configurable: true, value: 1500 }); - Object.defineProperty(scrollContainer, "scrollTop", { configurable: true, value: 1000 }); + scrollContainer.scrollTop = 1000; act(() => scrollContainer.dispatchEvent(new Event("scroll"))); const viewportObservation = observed.get(scrollContainer); expect(viewportObservation).toBeDefined(); @@ -1483,7 +1468,7 @@ describe("createWebStreamStrategy", () => { act(() => root?.render(renderWithActivity(false))); scrollTo.mockClear(); Object.defineProperty(scrollContainer, "scrollHeight", { configurable: true, value: 2200 }); - Object.defineProperty(scrollContainer, "scrollTop", { configurable: true, value: 0 }); + scrollContainer.scrollTop = 0; act(() => { scrollContainer.dispatchEvent(new WheelEvent("wheel", { deltaY: -100 })); scrollContainer.dispatchEvent(new Event("scroll")); diff --git a/packages/app/src/agent-stream/strategy-web.tsx b/packages/app/src/agent-stream/strategy-web.tsx index eee4557ab7..25e7a313d2 100644 --- a/packages/app/src/agent-stream/strategy-web.tsx +++ b/packages/app/src/agent-stream/strategy-web.tsx @@ -13,6 +13,7 @@ import { measureElement as measureVirtualElement, useVirtualizer, type Range as VirtualRange, + type Virtualizer, } from "@tanstack/react-virtual"; import { flushSync } from "react-dom"; import { withUnistyles } from "react-native-unistyles"; @@ -61,9 +62,6 @@ const HISTORY_START_SLOT_HEIGHT_PX = 32; const CONTENT_PADDING_TOP_PX = 16; const UPWARD_INPUT_EVIDENCE_TIMEOUT_MS = 100; const VIRTUALIZER_SCROLL_MARGIN_PX = HISTORY_START_SLOT_HEIGHT_PX + CONTENT_PADDING_TOP_PX; -// A row has to clear this much of the viewport top before the next one takes over as the -// reading position, so a row resting exactly on the edge does not flip back and forth. -const READING_POSITION_OFFSET_PX = 8; const ThemedLoadingSpinner = withUnistyles(LoadingSpinner); const foregroundMutedColorMapping = (theme: Theme) => ({ @@ -289,6 +287,7 @@ function WebStreamViewport(props: StreamRenderInput & { isMobileBreakpoint: bool olderHistoryProgressKey, scrollEnabled, contentMaxWidth, + imageContext, isMobileBreakpoint, } = props; const historyVirtualized = useRevisedHistoryRows( @@ -373,15 +372,39 @@ function WebStreamViewport(props: StreamRenderInput & { isMobileBreakpoint: bool ); return indexes.length > 0 ? indexes : null; }, [chatFindMessageId, segments.historyVirtualized]); + const rowVirtualizerRef = useRef | null>(null); const rangeExtractor = useCallback( - (range: VirtualRange) => { - const visible = defaultRangeExtractor(range); + (range: VirtualRange): number[] => { const anchorIndex = segments.historyVirtualized.findIndex( (row) => row.id === readingAnchor.getRowId(), ); + const container = scrollContainerRef.current; + // TanStack has computed the new measurements before calling the extractor. + // Select against the pending anchor correction, before mounting any rows at + // the stale pre-prepend offset. The DOM correction still belongs to commit. + const virtualizer = rowVirtualizerRef.current!; + const measurement = virtualizer.measurementsCache[anchorIndex]; + const correctedOffset = + container && measurement + ? readingAnchor.project(container.scrollTop, { + id: String(measurement.key), + top: measurement.start, + }) + : container?.scrollTop; + let visibleRange = range; + if ( + container && + correctedOffset !== undefined && + Math.abs(correctedOffset - container.scrollTop) > 0.5 + ) { + const first = virtualizer.getVirtualItemForOffset(correctedOffset); + const last = virtualizer.getVirtualItemForOffset(correctedOffset + container.clientHeight); + if (first && last) + visibleRange = { ...range, startIndex: first.index, endIndex: last.index }; + } return [ ...new Set([ - ...visible, + ...defaultRangeExtractor(visibleRange), ...(chatFindRowIndexes ?? []), ...(anchorIndex < 0 ? [] : [anchorIndex]), ]), @@ -398,6 +421,7 @@ function WebStreamViewport(props: StreamRenderInput & { isMobileBreakpoint: bool unsubscribe?.(); }; }, []); + const contentWidth = contentRef.current?.getBoundingClientRect().width; const rowVirtualizer = useVirtualizer({ count: segments.historyVirtualized.length, enabled: shouldUseVirtualizer, @@ -406,15 +430,15 @@ function WebStreamViewport(props: StreamRenderInput & { isMobileBreakpoint: bool estimateSize: (index: number) => { const row = segments.historyVirtualized[index]; const measured = row && measuredRowHeights.current.get(row.id); - const width = contentRef.current?.getBoundingClientRect().width; - if (measured && measured.width === width) return measured.height; - return row ? estimateStreamItemHeight(row, contentMaxWidth) : 120; + if (measured && measured.width === contentWidth) return measured.height; + return row ? estimateStreamItemHeight({ item: row, contentMaxWidth, imageContext }) : 120; }, observeElementOffset: observeVirtualOffset, rangeExtractor, scrollMargin: VIRTUALIZER_SCROLL_MARGIN_PX, overscan: 8, }); + rowVirtualizerRef.current = rowVirtualizer; // Scroll correction belongs to the viewport's committed layout. The virtualizer // otherwise writes scrollTop before React applies its new row positions. rowVirtualizer.shouldAdjustScrollPositionOnItemSizeChange = () => false; @@ -646,19 +670,7 @@ function WebStreamViewport(props: StreamRenderInput & { isMobileBreakpoint: bool onReadingPositionChange(null); return; } - const readingLine = scrollContainer.getBoundingClientRect().top + READING_POSITION_OFFSET_PX; - let readingRowId: string | null = null; - for (const element of contentNode.querySelectorAll("[data-history-row-id]")) { - const rowId = element.dataset.historyRowId; - if (!rowId) { - continue; - } - readingRowId = rowId; - if (element.getBoundingClientRect().bottom > readingLine) { - break; - } - } - onReadingPositionChange(readingRowId); + onReadingPositionChange(readingAnchor.getReadingRowId(scrollContainer.scrollTop)); }); const updateScrollMetrics = useCallback(() => { @@ -684,7 +696,7 @@ function WebStreamViewport(props: StreamRenderInput & { isMobileBreakpoint: bool onNearBottomChange, }); - const reconcileReadingPosition = useStableEvent((userScrolled: boolean = false) => { + const reconcileReadingPosition = useStableEvent(() => { const container = scrollContainerRef.current; const content = contentRef.current; if (!isActiveRef.current || !container || !content) return; @@ -700,11 +712,13 @@ function WebStreamViewport(props: StreamRenderInput & { isMobileBreakpoint: bool return { id, top: bounds.top - viewportTop + scrollTop, height: bounds.height }; }, ); - if (followOutputRef.current || isJumpSettling()) { + const shouldAnchor = !followOutputRef.current && !isJumpSettling(); + if (!shouldAnchor) readingAnchor.reset(); + const correctedTop = readingAnchor.reconcile(container.scrollTop, rows); + if (!shouldAnchor) { readingAnchor.reset(); return; } - const correctedTop = readingAnchor.reconcile(container.scrollTop, rows, userScrolled); if (Math.abs(correctedTop - container.scrollTop) > 0.5) { container.scrollTop = correctedTop; lastKnownScrollTopRef.current = container.scrollTop; @@ -718,7 +732,7 @@ function WebStreamViewport(props: StreamRenderInput & { isMobileBreakpoint: bool // the ordinary-to-virtualized handoff and measurements of newly mounted rows. useLayoutEffect(() => { reconcileReadingPosition(); - }); + }, [contentWidth, isActive, segments, virtualRows, reconcileReadingPosition]); const stopFollowingOutputFromUserIntent = useStableEvent(() => { cancelPendingStickToBottom(); @@ -752,13 +766,15 @@ function WebStreamViewport(props: StreamRenderInput & { isMobileBreakpoint: bool } lastKnownScrollTopRef.current = currentScrollTop; - reconcileReadingPosition(scrolledUp || scrolledDown); + if (!followOutputRef.current && !isJumpSettling() && (scrolledUp || scrolledDown)) { + readingAnchor.scroll(currentScrollTop); + } updateScrollMetrics(); evaluateHistoryStart(); }, [ evaluateHistoryStart, isJumpSettling, - reconcileReadingPosition, + readingAnchor, stopFollowingOutputFromUserIntent, updateScrollMetrics, ]); @@ -1051,7 +1067,9 @@ function WebStreamViewport(props: StreamRenderInput & { isMobileBreakpoint: bool lastTouchClientYRef.current = null; }; - scrollContainer.addEventListener("scroll", handleDomScroll, { passive: true }); + // Read user movement before the virtualizer's bubbling listener flushes a + // layout correction. Scroll events reuse the last committed row geometry. + scrollContainer.addEventListener("scroll", handleDomScroll, { passive: true, capture: true }); scrollContainer.addEventListener("wheel", handleWheel, { passive: true }); scrollContainer.addEventListener("keydown", handleKeyDown, { passive: true }); scrollContainer.addEventListener("pointerdown", handlePointerDown, { passive: true }); @@ -1065,7 +1083,7 @@ function WebStreamViewport(props: StreamRenderInput & { isMobileBreakpoint: bool scrollContainer.addEventListener("touchcancel", handleTouchEnd, { passive: true }); return () => { - scrollContainer.removeEventListener("scroll", handleDomScroll); + scrollContainer.removeEventListener("scroll", handleDomScroll, true); scrollContainer.removeEventListener("wheel", handleWheel); scrollContainer.removeEventListener("keydown", handleKeyDown); scrollContainer.removeEventListener("pointerdown", handlePointerDown); diff --git a/packages/app/src/agent-stream/strategy.ts b/packages/app/src/agent-stream/strategy.ts index 5f43f8f6b1..3b77f6b831 100644 --- a/packages/app/src/agent-stream/strategy.ts +++ b/packages/app/src/agent-stream/strategy.ts @@ -1,3 +1,4 @@ +import type { AssistantImageContext } from "@/utils/assistant-image-metadata"; import type { ComponentType, ReactElement, ReactNode, RefObject } from "react"; import type { StyleProp, ViewStyle } from "react-native"; import type { StreamItem } from "@/types/stream"; @@ -94,6 +95,7 @@ export interface StreamRenderInput { forwardListContentContainerStyle: StyleProp; // The chat column's max width. The web viewport estimates unmeasured row heights with it. contentMaxWidth: number; + imageContext?: AssistantImageContext; } export interface ResolveStreamRenderStrategyInput { diff --git a/packages/app/src/agent-stream/view.tsx b/packages/app/src/agent-stream/view.tsx index bf13ca6330..12f628da4f 100644 --- a/packages/app/src/agent-stream/view.tsx +++ b/packages/app/src/agent-stream/view.tsx @@ -1131,6 +1131,7 @@ const AgentStreamViewComponent = forwardRef { describe("estimateStreamItemHeight", () => { it("uses compact estimates for collapsed tool sequence rows", () => { - expect(estimateStreamItemHeight(toolCall("tool", 1), DEFAULT_CONTENT_MAX_WIDTH)).toBe(40); - expect(estimateStreamItemHeight(thought("thought", 2), DEFAULT_CONTENT_MAX_WIDTH)).toBe(40); + expect( + estimateStreamItemHeight({ + item: toolCall("tool", 1), + contentMaxWidth: DEFAULT_CONTENT_MAX_WIDTH, + }), + ).toBe(40); + expect( + estimateStreamItemHeight({ + item: thought("thought", 2), + contentMaxWidth: DEFAULT_CONTENT_MAX_WIDTH, + }), + ).toBe(40); }); it("uses a larger estimate for user messages with image attachments", () => { @@ -146,7 +156,40 @@ describe("estimateStreamItemHeight", () => { ], }; - expect(estimateStreamItemHeight(item, DEFAULT_CONTENT_MAX_WIDTH)).toBe(220); + expect(estimateStreamItemHeight({ item, contentMaxWidth: DEFAULT_CONTENT_MAX_WIDTH })).toBe( + 220, + ); + }); + + it("uses local image dimensions only from the row's server and workspace", () => { + clearAssistantImageMetadataCache(); + const imageContext = { serverId: "server-one", workspaceRoot: "/workspace/one" }; + setAssistantImageMetadata( + { source: "portrait.png", ...imageContext }, + { width: 800, height: 1600 }, + ); + const item: StreamItem = { + kind: "assistant_message", + id: "local-image", + text: "![Portrait](portrait.png)", + timestamp: createTimestamp(2), + }; + + expect(estimateStreamItemHeight({ item, contentMaxWidth: 608, imageContext })).toBe(1264); + expect( + estimateStreamItemHeight({ + item, + contentMaxWidth: 608, + imageContext: { ...imageContext, workspaceRoot: "/workspace/two" }, + }), + ).toBe(464); + expect( + estimateStreamItemHeight({ + item, + contentMaxWidth: 608, + imageContext: { ...imageContext, serverId: "server-two" }, + }), + ).toBe(464); }); it("uses cached assistant image metadata when available", () => { @@ -165,7 +208,9 @@ describe("estimateStreamItemHeight", () => { timestamp: createTimestamp(2), }; - expect(estimateStreamItemHeight(item, DEFAULT_CONTENT_MAX_WIDTH)).toBeGreaterThan(220); + expect( + estimateStreamItemHeight({ item, contentMaxWidth: DEFAULT_CONTENT_MAX_WIDTH }), + ).toBeGreaterThan(220); }); }); diff --git a/packages/app/src/agent-stream/web-virtualization.ts b/packages/app/src/agent-stream/web-virtualization.ts index 5445783178..0c7f756959 100644 --- a/packages/app/src/agent-stream/web-virtualization.ts +++ b/packages/app/src/agent-stream/web-virtualization.ts @@ -1,3 +1,4 @@ +import type { AssistantImageContext } from "@/utils/assistant-image-metadata"; import type { StreamItem } from "@/types/stream"; import { estimateAssistantMessageHeightFromCache } from "@/utils/assistant-message-height-estimate"; import { @@ -43,12 +44,28 @@ export interface WebVirtualizedHistoryWindow { mountedEntries: IndexedStreamItem[]; } -export function estimateStreamItemHeight(item: StreamItem, contentMaxWidth: number): number { +interface StreamItemHeightEstimateInput { + item: StreamItem; + contentMaxWidth: number; + imageContext?: AssistantImageContext; +} + +export function estimateStreamItemHeight({ + item, + contentMaxWidth, + imageContext, +}: StreamItemHeightEstimateInput): number { switch (item.kind) { case "user_message": return item.images && item.images.length > 0 ? 220 : 96; case "assistant_message": - return estimateAssistantMessageHeightFromCache(item.text, contentMaxWidth) ?? 220; + return ( + estimateAssistantMessageHeightFromCache({ + markdown: item.text, + contentMaxWidth, + imageContext, + }) ?? 220 + ); case "tool_call": return COLLAPSED_TOOL_SEQUENCE_ROW_HEIGHT_ESTIMATE; case "thought": diff --git a/packages/app/src/assistant-image/use-assistant-image.ts b/packages/app/src/assistant-image/use-assistant-image.ts index 0c0818e8f1..3314b7a398 100644 --- a/packages/app/src/assistant-image/use-assistant-image.ts +++ b/packages/app/src/assistant-image/use-assistant-image.ts @@ -415,8 +415,8 @@ export function useAssistantImage({ const preview = dataImage ? dataImagePreview : filePreview; const previewUri = preview.status === "loaded" ? preview.uri : null; const uri = directUri ?? previewUri; - const cachedMetadata = useMemo( - () => getAssistantImageMetadata({ source, workspaceRoot, serverId }), + const cachedAspectRatio = useMemo( + () => getAssistantImageMetadata({ source, workspaceRoot, serverId })?.aspectRatio ?? null, [serverId, source, workspaceRoot], ); const [lifecycle, dispatchLifecycle] = useReducer( @@ -454,9 +454,9 @@ export function useAssistantImage({ dispatch({ type: "preview_created", uri, - aspectRatio: cachedMetadata?.aspectRatio ?? null, + aspectRatio: cachedAspectRatio, }); - }, [cachedMetadata, dispatch, uri]); + }, [cachedAspectRatio, dispatch, uri]); const handleImageError = useCallback(() => { if (uri) { @@ -484,7 +484,7 @@ export function useAssistantImage({ const metadata = dimensions ? setAssistantImageMetadata({ source, workspaceRoot, serverId }, dimensions) : null; - const aspectRatio = metadata?.aspectRatio ?? cachedMetadata?.aspectRatio ?? null; + const aspectRatio = metadata?.aspectRatio ?? cachedAspectRatio; if (!aspectRatio) { dispatch({ type: "failed", @@ -495,7 +495,7 @@ export function useAssistantImage({ } dispatch({ type: "image_loaded", uri, aspectRatio }); }, - [cachedMetadata, dispatch, serverId, source, t, uri, workspaceRoot], + [cachedAspectRatio, dispatch, serverId, source, t, uri, workspaceRoot], ); const acquisitionFailure = getAcquisitionFailure({ @@ -539,6 +539,6 @@ export function useAssistantImage({ return { status: "loading", binding, - aspectRatio: hasCurrentLifecycleUri ? lifecycle.aspectRatio : null, + aspectRatio: (hasCurrentLifecycleUri ? lifecycle.aspectRatio : null) ?? cachedAspectRatio, }; } diff --git a/packages/app/src/components/markdown/inline-image-size.ts b/packages/app/src/components/markdown/inline-image-size.ts index c4f4b67ff3..8f83f80fcc 100644 --- a/packages/app/src/components/markdown/inline-image-size.ts +++ b/packages/app/src/components/markdown/inline-image-size.ts @@ -8,7 +8,6 @@ export interface InlineImageExplicitDimensions { height?: number; } -const INLINE_IMAGE_FALLBACK_SIZE = 16; const INLINE_IMAGE_MAX_WIDTH = 240; const INLINE_IMAGE_MAX_HEIGHT = 160; @@ -59,5 +58,5 @@ function resolveInlineImageDimensions(input: { return { width: input.explicit.height, height: input.explicit.height }; } - return input.natural ?? { width: INLINE_IMAGE_FALLBACK_SIZE, height: INLINE_IMAGE_FALLBACK_SIZE }; + return input.natural ?? { width: INLINE_IMAGE_MAX_WIDTH, height: INLINE_IMAGE_MAX_HEIGHT }; } diff --git a/packages/app/src/components/markdown/renderer.test.ts b/packages/app/src/components/markdown/renderer.test.ts index 1dc279a2d5..2dcab5c327 100644 --- a/packages/app/src/components/markdown/renderer.test.ts +++ b/packages/app/src/components/markdown/renderer.test.ts @@ -63,10 +63,10 @@ describe("resolveInlineImageSize", () => { }); }); - it("uses a generic small fallback when no dimensions are known", () => { + it("reserves a capped 3:2 placeholder when no dimensions are known", () => { expect(resolveInlineImageSize({ explicit: {}, natural: null })).toEqual({ - width: 16, - height: 16, + width: 240, + height: 160, }); }); }); diff --git a/packages/app/src/components/markdown/renderer.tsx b/packages/app/src/components/markdown/renderer.tsx index fecd9026ab..83380ea919 100644 --- a/packages/app/src/components/markdown/renderer.tsx +++ b/packages/app/src/components/markdown/renderer.tsx @@ -39,6 +39,10 @@ import { type MarkdownInlineImagePart, } from "./html-ish"; import { resolveInlineImageSize, type InlineImageDimensions } from "./inline-image-size"; +import { + getAssistantImageMetadata, + setAssistantImageMetadata, +} from "@/utils/assistant-image-metadata"; import { groupMarkdownParts, type MarkdownPartGroup } from "./part-groups"; import { colorMarkdownLinkChildren } from "./link-children"; import { MarkdownLinkText } from "./link-text"; @@ -201,11 +205,16 @@ function useNaturalImageDimensions(part: MarkdownInlineImagePart): { failed: boolean; setFailed: (failed: boolean) => void; } { - const [natural, setNatural] = useState(null); + const cached = useMemo(() => getAssistantImageMetadata({ source: part.src }), [part.src]); + const [resolved, setResolved] = useState<{ + source: string; + dimensions: InlineImageDimensions; + } | null>(null); + const natural = resolved?.source === part.src ? resolved.dimensions : cached; const [failed, setFailed] = useState(false); useEffect(() => { - if (part.width && part.height) { + if ((part.width && part.height) || cached) { return; } @@ -213,8 +222,9 @@ function useNaturalImageDimensions(part: MarkdownInlineImagePart): { Image.getSize( part.src, (width, height) => { + setAssistantImageMetadata({ source: part.src }, { width, height }); if (!cancelled) { - setNatural({ width, height }); + setResolved({ source: part.src, dimensions: { width, height } }); } }, () => { @@ -227,7 +237,7 @@ function useNaturalImageDimensions(part: MarkdownInlineImagePart): { return () => { cancelled = true; }; - }, [part.height, part.src, part.width]); + }, [cached, part.height, part.src, part.width]); return { natural, failed, setFailed }; } diff --git a/packages/app/src/components/message.tsx b/packages/app/src/components/message.tsx index 4ae2ccd1b7..8d93f4588b 100644 --- a/packages/app/src/components/message.tsx +++ b/packages/app/src/components/message.tsx @@ -1,3 +1,4 @@ +import { ASSISTANT_IMAGE_DEFAULT_ASPECT_RATIO } from "@/utils/assistant-image-metadata"; import { LoadingSpinner } from "@/components/ui/loading-spinner"; import { TaskListRow } from "@/components/task-list-row"; import { @@ -814,8 +815,6 @@ export const assistantMessageStylesheet = StyleSheet.create((theme) => ({ }, })); -const ASSISTANT_IMAGE_MIN_HEIGHT = 160; - function AssistantMarkdownImage({ source, occurrenceKey, @@ -860,11 +859,9 @@ function AssistantMarkdownImage({ [containerStyle], ); const imageSizeStyle = useMemo(() => { - if (aspectRatio) { - return { aspectRatio }; - } - return { height: ASSISTANT_IMAGE_MIN_HEIGHT }; - }, [aspectRatio]); + if (image.status === "failed") return { height: 160 }; + return { aspectRatio: aspectRatio ?? ASSISTANT_IMAGE_DEFAULT_ASPECT_RATIO }; + }, [aspectRatio, image.status]); const surfaceStyle = useMemo>( () => [assistantMessageStylesheet.imageSurface, imageSizeStyle], [imageSizeStyle], @@ -882,10 +879,10 @@ function AssistantMarkdownImage({ () => [ assistantMessageStylesheet.imageFrame, containerStyle, - { height: ASSISTANT_IMAGE_MIN_HEIGHT }, + imageSizeStyle, assistantMessageStylesheet.imageState, ], - [containerStyle], + [containerStyle, imageSizeStyle], ); if (image.status === "failed") { diff --git a/packages/app/src/utils/assistant-image-metadata.test.ts b/packages/app/src/utils/assistant-image-metadata.test.ts index 8f8cca2d3f..3ebee656dd 100644 --- a/packages/app/src/utils/assistant-image-metadata.test.ts +++ b/packages/app/src/utils/assistant-image-metadata.test.ts @@ -21,25 +21,53 @@ describe("assistant image metadata", () => { ).toEqual(["/tmp/paseo.png", "https://example.com/test.png"]); }); - it("reuses cached metadata across canonical and raw source keys", () => { - setAssistantImageMetadata( - { - source: "/tmp/paseo-codex-screenshot.png", - workspaceRoot: "/workspaces/paseo", - serverId: "server-1", - }, - { width: 1200, height: 800 }, - ); + it("keeps local image metadata scoped to its server and workspace", () => { + const image = { + source: "screenshot.png", + workspaceRoot: "/workspace/one", + serverId: "server-1", + }; + setAssistantImageMetadata(image, { width: 1200, height: 800 }); - expect( - getAssistantImageMetadata({ - source: "/tmp/paseo-codex-screenshot.png", - }), - ).toEqual({ + expect(getAssistantImageMetadata({ ...image, workspaceRoot: "/workspace/two" })).toBeNull(); + expect(getAssistantImageMetadata({ ...image, serverId: "server-2" })).toBeNull(); + expect(getAssistantImageMetadata({ source: image.source })).toBeNull(); + expect(getAssistantImageMetadata(image)).toEqual({ width: 1200, height: 800, aspectRatio: 1.5, }); + + setAssistantImageMetadata( + { ...image, workspaceRoot: "/workspace/two" }, + { width: 800, height: 1200 }, + ); + expect(getAssistantImageMetadata(image)?.aspectRatio).toBe(1.5); + expect( + getAssistantImageMetadata({ ...image, workspaceRoot: "/workspace/two" })?.aspectRatio, + ).toBe(2 / 3); + }); + + it("reuses direct image metadata without workspace context", () => { + const source = "https://example.com/shared.png"; + setAssistantImageMetadata( + { source, workspaceRoot: "/workspace/one", serverId: "server-1" }, + { width: 1200, height: 800 }, + ); + expect(getAssistantImageMetadata({ source })).toEqual({ + width: 1200, + height: 800, + aspectRatio: 1.5, + }); + }); + + it("reserves the default image aspect ratio before metadata arrives", () => { + expect( + estimateAssistantMessageHeightFromCache({ + markdown: "![Pending](https://example.com/pending.png)", + contentMaxWidth: 608, + }), + ).toBe(464); }); it("estimates assistant message height from cached image metadata", () => { @@ -51,10 +79,10 @@ describe("assistant image metadata", () => { ); expect( - estimateAssistantMessageHeightFromCache( - "Here is the screenshot\n\n![Screenshot](https://example.com/landscape.png)", - DEFAULT_CONTENT_MAX_WIDTH, - ), + estimateAssistantMessageHeightFromCache({ + markdown: "Here is the screenshot\n\n![Screenshot](https://example.com/landscape.png)", + contentMaxWidth: DEFAULT_CONTENT_MAX_WIDTH, + }), ).toBeGreaterThan(220); }); @@ -62,14 +90,14 @@ describe("assistant image metadata", () => { const source = `data:image/png;base64,${"a".repeat(512)}`; setAssistantImageMetadata({ source }, { width: 1200, height: 800 }); - const imageOnlyHeight = estimateAssistantMessageHeightFromCache( - `![Screenshot](${source})`, - DEFAULT_CONTENT_MAX_WIDTH, - ); - const mixedHeight = estimateAssistantMessageHeightFromCache( - `Text\n\n![Screenshot](${source})`, - DEFAULT_CONTENT_MAX_WIDTH, - ); + const imageOnlyHeight = estimateAssistantMessageHeightFromCache({ + markdown: `![Screenshot](${source})`, + contentMaxWidth: DEFAULT_CONTENT_MAX_WIDTH, + }); + const mixedHeight = estimateAssistantMessageHeightFromCache({ + markdown: `Text\n\n![Screenshot](${source})`, + contentMaxWidth: DEFAULT_CONTENT_MAX_WIDTH, + }); expect(imageOnlyHeight).toBeGreaterThan(220); expect(mixedHeight).toBeGreaterThan(imageOnlyHeight ?? 0); diff --git a/packages/app/src/utils/assistant-image-metadata.ts b/packages/app/src/utils/assistant-image-metadata.ts index 76b8eafbb1..ac7bd217b2 100644 --- a/packages/app/src/utils/assistant-image-metadata.ts +++ b/packages/app/src/utils/assistant-image-metadata.ts @@ -1,6 +1,11 @@ import { resolveAssistantImageSource } from "@/utils/assistant-image-source"; import { createImageSourceCacheKey } from "@/attachments/utils"; +export interface AssistantImageContext { + serverId?: string; + workspaceRoot?: string; +} + export interface AssistantImageMetadata { width: number; height: number; @@ -13,6 +18,7 @@ const ASSISTANT_IMAGE_METADATA_CACHE_LIMIT = 500; const ASSISTANT_IMAGE_PARSE_CACHE_LIMIT = 500; const MARKDOWN_IMAGE_PATTERN = /!\[[^\]]*]\((<[^>]+>|[^)\n]+)\)/g; +export const ASSISTANT_IMAGE_DEFAULT_ASPECT_RATIO = 3 / 2; const ASSISTANT_IMAGE_INSET = 8; const ASSISTANT_IMAGE_MIN_HEIGHT = 160; const ASSISTANT_IMAGE_BLOCK_GAP = 24; @@ -65,15 +71,9 @@ function parseAssistantImageMarkdown(markdown: string): { }; } -function createSourceAliasKey(source: string): string { - return `source:${createImageSourceCacheKey(source)}`; -} - -function createResolutionKey(input: { - source: string; - workspaceRoot?: string; - serverId?: string; -}): string | null { +function getAssistantImageMetadataKey( + input: AssistantImageContext & { source: string }, +): string | null { const resolution = resolveAssistantImageSource({ source: input.source, workspaceRoot: input.workspaceRoot, @@ -84,53 +84,28 @@ function createResolutionKey(input: { if (resolution.kind === "direct") { return `direct:${createImageSourceCacheKey(resolution.uri)}`; } - return `file:${input.serverId ?? "unknown-server"}:${resolution.cwd}:${resolution.path}`; + // A file path identifies an image only within its daemon and workspace. + // Callers without that context must use the default aspect ratio. + if (!input.serverId) { + return null; + } + return JSON.stringify(["file", input.serverId, resolution.cwd, resolution.path]); } -function getAssistantImageMetadataKeys(input: { - source: string; - workspaceRoot?: string; - serverId?: string; -}): string[] { - const source = input.source.trim(); - if (!source) { - return []; +export function getAssistantImageMetadata( + input: AssistantImageContext & { source: string }, +): AssistantImageMetadata | null { + const key = getAssistantImageMetadataKey(input); + const metadata = key ? assistantImageMetadataCache.get(key) : undefined; + if (!key || !metadata) { + return null; } - - const keys = [createSourceAliasKey(source)]; - const resolutionKey = createResolutionKey(input); - if (resolutionKey) { - keys.unshift(resolutionKey); - } - return [...new Set(keys)]; -} - -export function getAssistantImageMetadata(input: { - source: string; - workspaceRoot?: string; - serverId?: string; -}): AssistantImageMetadata | null { - for (const key of getAssistantImageMetadataKeys(input)) { - const metadata = assistantImageMetadataCache.get(key); - if (metadata) { - touchCacheEntry( - assistantImageMetadataCache, - key, - metadata, - ASSISTANT_IMAGE_METADATA_CACHE_LIMIT, - ); - return metadata; - } - } - return null; + touchCacheEntry(assistantImageMetadataCache, key, metadata, ASSISTANT_IMAGE_METADATA_CACHE_LIMIT); + return metadata; } export function setAssistantImageMetadata( - input: { - source: string; - workspaceRoot?: string; - serverId?: string; - }, + input: AssistantImageContext & { source: string }, dimensions: { width: number; height: number }, ): AssistantImageMetadata | null { const { width, height } = dimensions; @@ -144,7 +119,8 @@ export function setAssistantImageMetadata( aspectRatio: width / height, }; - for (const key of getAssistantImageMetadataKeys(input)) { + const key = getAssistantImageMetadataKey(input); + if (key) { touchCacheEntry( assistantImageMetadataCache, key, @@ -176,28 +152,31 @@ export function extractAssistantImageSources(markdown: string): string[] { return parsed.sources; } -export function estimateAssistantMessageHeightFromCache( - markdown: string, - contentMaxWidth: number, -): number | null { +export interface AssistantMessageHeightEstimateInput { + markdown: string; + contentMaxWidth: number; + imageContext?: AssistantImageContext; +} + +export function estimateAssistantMessageHeightFromCache({ + markdown, + contentMaxWidth, + imageContext, +}: AssistantMessageHeightEstimateInput): number | null { const parsed = assistantImageParseCache.get(markdown) ?? parseAssistantImageMarkdown(markdown); if (parsed.sources.length === 0) { return null; } - const knownHeights = parsed.sources - .map((source) => getAssistantImageMetadata({ source })) - .filter((metadata): metadata is AssistantImageMetadata => metadata !== null) - .map((metadata) => - Math.max( - ASSISTANT_IMAGE_MIN_HEIGHT, - Math.round((contentMaxWidth - ASSISTANT_IMAGE_INSET) / metadata.aspectRatio), - ), + const imageHeights = parsed.sources.map((source) => { + const aspectRatio = + getAssistantImageMetadata({ source, ...imageContext })?.aspectRatio ?? + ASSISTANT_IMAGE_DEFAULT_ASPECT_RATIO; + return Math.max( + ASSISTANT_IMAGE_MIN_HEIGHT, + Math.round((contentMaxWidth - ASSISTANT_IMAGE_INSET) / aspectRatio), ); - - if (knownHeights.length === 0) { - return null; - } + }); const baseHeight = parsed.hasNonImageText ? ASSISTANT_MESSAGE_BASE_HEIGHT @@ -205,8 +184,8 @@ export function estimateAssistantMessageHeightFromCache( const estimatedHeight = baseHeight + - knownHeights.reduce((sum, height) => sum + height, 0) + - ASSISTANT_IMAGE_BLOCK_GAP * knownHeights.length; + imageHeights.reduce((sum, height) => sum + height, 0) + + ASSISTANT_IMAGE_BLOCK_GAP * imageHeights.length; return Math.max(ASSISTANT_MESSAGE_MIN_HEIGHT, estimatedHeight); } diff --git a/packages/app/src/utils/assistant-message-height-estimate.test.ts b/packages/app/src/utils/assistant-message-height-estimate.test.ts index 8df70fe588..d4e3a2ff94 100644 --- a/packages/app/src/utils/assistant-message-height-estimate.test.ts +++ b/packages/app/src/utils/assistant-message-height-estimate.test.ts @@ -29,19 +29,27 @@ describe("assistant message height estimate", () => { }); expect( - estimateAssistantMessageHeightFromCache( - "First paragraph\n\nSecond paragraph", - DEFAULT_CONTENT_MAX_WIDTH, - ), + estimateAssistantMessageHeightFromCache({ + markdown: "First paragraph\n\nSecond paragraph", + contentMaxWidth: DEFAULT_CONTENT_MAX_WIDTH, + }), ).toBe(97); }); it("reads block heights measured at the configured content width", () => { setAssistantMarkdownBlockHeight({ block: "Wide paragraph", width: 1584, height: 20 }); - expect(estimateAssistantMessageHeightFromCache("Wide paragraph", 1600)).toBe(44); expect( - estimateAssistantMessageHeightFromCache("Wide paragraph", DEFAULT_CONTENT_MAX_WIDTH), + estimateAssistantMessageHeightFromCache({ + markdown: "Wide paragraph", + contentMaxWidth: 1600, + }), + ).toBe(44); + expect( + estimateAssistantMessageHeightFromCache({ + markdown: "Wide paragraph", + contentMaxWidth: DEFAULT_CONTENT_MAX_WIDTH, + }), ).toBeNull(); }); @@ -54,10 +62,10 @@ describe("assistant message height estimate", () => { ); expect( - estimateAssistantMessageHeightFromCache( - "Here is the screenshot\n\n![Screenshot](https://example.com/landscape.png)", - DEFAULT_CONTENT_MAX_WIDTH, - ), + estimateAssistantMessageHeightFromCache({ + markdown: "Here is the screenshot\n\n![Screenshot](https://example.com/landscape.png)", + contentMaxWidth: DEFAULT_CONTENT_MAX_WIDTH, + }), ).toBeGreaterThan(220); }); }); diff --git a/packages/app/src/utils/assistant-message-height-estimate.ts b/packages/app/src/utils/assistant-message-height-estimate.ts index 638c961e01..02e153e0bb 100644 --- a/packages/app/src/utils/assistant-message-height-estimate.ts +++ b/packages/app/src/utils/assistant-message-height-estimate.ts @@ -1,4 +1,7 @@ -import { estimateAssistantMessageHeightFromCache as estimateAssistantImageMessageHeightFromCache } from "@/utils/assistant-image-metadata"; +import { + type AssistantMessageHeightEstimateInput, + estimateAssistantMessageHeightFromCache as estimateAssistantImageMessageHeightFromCache, +} from "@/utils/assistant-image-metadata"; import { splitMarkdownBlocks } from "@/utils/split-markdown-blocks"; const ASSISTANT_MARKDOWN_BLOCK_HEIGHT_CACHE_LIMIT = 1000; @@ -106,13 +109,14 @@ function estimateAssistantMarkdownBlockHeightFromCache( ); } -export function estimateAssistantMessageHeightFromCache( - markdown: string, - contentMaxWidth: number, -): number | null { +export function estimateAssistantMessageHeightFromCache({ + markdown, + contentMaxWidth, + imageContext, +}: AssistantMessageHeightEstimateInput): number | null { return ( estimateAssistantMarkdownBlockHeightFromCache(markdown, contentMaxWidth) ?? - estimateAssistantImageMessageHeightFromCache(markdown, contentMaxWidth) + estimateAssistantImageMessageHeightFromCache({ markdown, contentMaxWidth, imageContext }) ); }