fix(app): optimize large review panes (#35375)

This commit is contained in:
Luke Parker 2026-07-06 16:26:03 +10:00 committed by GitHub
commit 3a149ba71c
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
19 changed files with 1074 additions and 404 deletions

View file

@ -0,0 +1,312 @@
import type { Page } from "@playwright/test"
import { benchmark, expect } from "../benchmark"
import { setupTimelineBenchmark } from "./session-timeline-benchmark.fixture"
const changedLinesPerFile = 100
const linesPerSide = changedLinesPerFile / 2
const fileCounts = [1, 10, 100, 1_000, 10_000]
const filesPerDirectory = 100
const readyFrames = 3
const completionTimeoutMs = Number(process.env.REVIEW_PANE_COMPLETION_TIMEOUT_MS ?? 900_000)
type ReviewPaneScalingSample = {
observedAtMs: number
logicalRows: number
treeRows: number
fileRows: number
diffLines: number
header: string
ready: boolean
}
type ReviewPaneScalingProbe = {
startedAt?: number
firstTreeRowMs?: number
logicalTreeReadyMs?: number
firstDiffRenderMs?: number
stableReadyMs?: number
samples: ReviewPaneScalingSample[]
frameTimesMs: number[]
longTasks: { startTime: number; duration: number }[]
stop: () => void
}
benchmark.describe("performance: review pane scaling", () => {
for (const fileCount of fileCounts) {
const changedLines = fileCount * changedLinesPerFile
benchmark(
`${changedLines} changed lines across ${fileCount} ${fileCount === 1 ? "file" : "files"}`,
async ({ page, report }) => {
benchmark.setTimeout(1_200_000)
await page.emulateMedia({ reducedMotion: "reduce" })
const patchByteLimit = Number(process.env.REVIEW_PANE_PATCH_BYTE_LIMIT ?? Number.POSITIVE_INFINITY)
if (Number.isNaN(patchByteLimit) || patchByteLimit < 0)
throw new Error(`Invalid REVIEW_PANE_PATCH_BYTE_LIMIT: ${process.env.REVIEW_PANE_PATCH_BYTE_LIMIT}`)
const responseBody = JSON.stringify(createScalingDiffs(fileCount, patchByteLimit))
await setupTimelineBenchmark(page, {
historyTurns: 0,
eventBatch: 1,
newLayoutDesigns: true,
})
await page.route("**/vcs/diff**", (route) =>
route.fulfill({
status: 200,
contentType: "application/json",
headers: { "access-control-allow-origin": "*" },
body: responseBody,
}),
)
const expectedRows = fileCount + 2 + Math.ceil(fileCount / filesPerDirectory)
const metrics = await measureReviewPaneLoad(page, {
expectedFile: reviewFile(0),
expectedRows,
})
const search = await measureBroadReviewSearch(page, fileCount)
expect(metrics.logicalRows).toBe(expectedRows)
expect(metrics.fileRows).toBeGreaterThan(0)
expect(metrics.treeRows).toBeGreaterThan(0)
expect(metrics.diffLines).toBeGreaterThan(0)
expect(search.logicalRows).toBe(fileCount)
expect(search.renderedRows).toBeGreaterThan(0)
report(
{ ...metrics, search },
{
fileCount,
changedLinesPerFile,
changedLines,
additions: changedLines / 2,
deletions: changedLines / 2,
patchLines: changedLines,
patchByteLimit: Number.isFinite(patchByteLimit) ? patchByteLimit : null,
payloadBytes: new TextEncoder().encode(responseBody).byteLength,
expectedRows,
},
)
},
)
}
})
async function measureBroadReviewSearch(page: Page, expectedRows: number) {
const filter = page.getByRole("searchbox", { name: "Filter files" })
await filter.evaluate((element) => {
element.addEventListener(
"input",
() => {
;(window as Window & { __reviewSearchStartedAt?: number }).__reviewSearchStartedAt = performance.now()
},
{ once: true, capture: true },
)
})
await filter.fill("file-")
return page.evaluate((expectedRows) => {
const startedAt = (window as Window & { __reviewSearchStartedAt?: number }).__reviewSearchStartedAt!
return new Promise<{ stableMs: number; logicalRows: number; renderedRows: number }>((resolve) => {
let previous = -1
let streak = 0
const sample = () => {
const tree = document.querySelector<HTMLElement>('#review-panel [data-component="file-tree-v2"]')
const rows = [...document.querySelectorAll<HTMLElement>('#review-panel [data-slot="file-tree-v2-row"]')]
const logicalRows = Number(tree?.dataset.totalRows ?? rows.length)
const ready =
logicalRows === expectedRows && rows.length > 0 && rows.every((row) => row.textContent?.includes("file-"))
streak = ready && rows.length === previous ? streak + 1 : ready ? 1 : 0
previous = rows.length
if (streak >= 3) {
resolve({ stableMs: performance.now() - startedAt, logicalRows, renderedRows: rows.length })
return
}
requestAnimationFrame(sample)
}
requestAnimationFrame(sample)
})
}, expectedRows)
}
function createScalingDiffs(fileCount: number, patchByteLimit: number) {
const changes = Array.from({ length: linesPerSide }, (_, index) => {
const line = String(index).padStart(3, "0")
return `-export const value_${line} = "before"\n+export const value_${line} = "after"`
}).join("\n")
let patchBytes = 0
let capped = false
return Array.from({ length: fileCount }, (_, index) => {
const file = reviewFile(index)
const fullPatch = [
`diff --git a/${file} b/${file}`,
`--- a/${file}`,
`+++ b/${file}`,
`@@ -1,${linesPerSide} +1,${linesPerSide} @@`,
changes,
].join("\n")
if (index === 0 && fullPatch.length > patchByteLimit)
throw new Error(`REVIEW_PANE_PATCH_BYTE_LIMIT must include the active patch (${fullPatch.length} bytes)`)
const patch = !capped && patchBytes + fullPatch.length <= patchByteLimit ? fullPatch : emptyReviewPatch(file)
if (patch === fullPatch) patchBytes += fullPatch.length
else capped = true
return {
file,
patch,
additions: linesPerSide,
deletions: linesPerSide,
status: "modified" as const,
}
})
}
function emptyReviewPatch(file: string) {
return [`diff --git a/${file} b/${file}`, `--- a/${file}`, `+++ b/${file}`].join("\n")
}
function reviewFile(index: number) {
return `src/review/d${String(Math.floor(index / filesPerDirectory)).padStart(5, "0")}/file-${String(index).padStart(5, "0")}.ts`
}
async function measureReviewPaneLoad(page: Page, input: { expectedFile: string; expectedRows: number }) {
const toggle = page.getByRole("button", { name: "Toggle review" })
await expect(toggle).toBeVisible()
await toggle.evaluate((element) => element.setAttribute("data-review-pane-scaling-toggle", ""))
await installReviewPaneScalingProbe(page, input)
await toggle.click()
await page.waitForFunction(
() =>
(window as Window & { __reviewPaneScalingProbe?: ReviewPaneScalingProbe }).__reviewPaneScalingProbe
?.stableReadyMs !== undefined,
undefined,
{ timeout: completionTimeoutMs },
)
return page.evaluate(() => {
const probe = (window as Window & { __reviewPaneScalingProbe?: ReviewPaneScalingProbe }).__reviewPaneScalingProbe!
probe.stop()
const startedAt = probe.startedAt!
const final = probe.samples.at(-1)!
const resources = performance
.getEntriesByType("resource")
.filter((entry) => entry.name.includes("/vcs/diff")) as PerformanceResourceTiming[]
const resource = resources.at(-1)
const longTasks = probe.longTasks.filter(
(entry) => entry.startTime >= startedAt && entry.startTime <= startedAt + probe.stableReadyMs!,
)
const frameGaps = probe.frameTimesMs.map((time, index) => time - (probe.frameTimesMs[index - 1] ?? 0))
return {
firstTreeRowMs: probe.firstTreeRowMs ?? null,
logicalTreeReadyMs: probe.logicalTreeReadyMs ?? null,
firstDiffRenderMs: probe.firstDiffRenderMs ?? null,
stableReadyMs: probe.stableReadyMs ?? null,
responseStartMs: resource ? resource.responseStart - startedAt : null,
responseEndMs: resource ? resource.responseEnd - startedAt : null,
responseToStableMs: resource ? probe.stableReadyMs! - (resource.responseEnd - startedAt) : null,
treeRows: final.treeRows,
logicalRows: final.logicalRows,
fileRows: final.fileRows,
diffLines: final.diffLines,
samples: probe.samples.length,
maxFrameGapMs: Math.max(0, ...frameGaps),
longTaskCount: longTasks.length,
longTaskTotalMs: longTasks.reduce((sum, entry) => sum + entry.duration, 0),
maxLongTaskMs: Math.max(0, ...longTasks.map((entry) => entry.duration)),
}
})
}
async function installReviewPaneScalingProbe(page: Page, input: { expectedFile: string; expectedRows: number }) {
await page.evaluate(
({ expectedFile, expectedRows, stableFrames }) => {
let running = true
let readyStreak = 0
const basename = expectedFile.split("/").at(-1)!
const longTaskObserver = PerformanceObserver.supportedEntryTypes.includes("longtask")
? new PerformanceObserver((list) => {
probe.longTasks.push(
...list.getEntries().map((entry) => ({ startTime: entry.startTime, duration: entry.duration })),
)
})
: undefined
const probe: ReviewPaneScalingProbe = {
samples: [],
frameTimesMs: [],
longTasks: [],
stop: () => {
running = false
longTaskObserver?.disconnect()
},
}
const sample = (time: number) => {
if (!running || probe.startedAt === undefined) return
const panel = document.querySelector<HTMLElement>("#review-panel")
const tree = panel?.querySelector<HTMLElement>('[data-component="file-tree-v2"]')
const rows = panel?.querySelectorAll('[data-slot="file-tree-v2-row"]') ?? []
const fileRows = panel?.querySelectorAll('button[data-slot="file-tree-v2-row"]') ?? []
const header =
panel?.querySelector<HTMLElement>('[data-slot="session-review-v2-file-header"]')?.textContent?.trim() ?? ""
const viewers = panel
? [...panel.querySelectorAll<HTMLElement>('[data-component="file"][data-mode="diff"]')]
: []
const diffLines = viewers.reduce(
(sum, viewer) =>
sum + (viewer.querySelector("diffs-container")?.shadowRoot?.querySelectorAll("[data-line]").length ?? 0),
0,
)
const observedAtMs = time - probe.startedAt
const logicalRows = Number(tree?.dataset.totalRows ?? rows.length)
const ready =
logicalRows === expectedRows &&
fileRows.length > 0 &&
header.includes(basename) &&
viewers.length === 1 &&
diffLines > 0
const previous = probe.samples.at(-1)
const stable =
ready &&
previous?.ready === true &&
previous.logicalRows === logicalRows &&
previous.treeRows === rows.length &&
previous.fileRows === fileRows.length &&
previous.diffLines === diffLines &&
previous.header === header
probe.frameTimesMs.push(observedAtMs)
probe.samples.push({
observedAtMs,
logicalRows,
treeRows: rows.length,
fileRows: fileRows.length,
diffLines,
header,
ready,
})
if (probe.firstTreeRowMs === undefined && rows.length > 0) probe.firstTreeRowMs = observedAtMs
if (probe.logicalTreeReadyMs === undefined && logicalRows === expectedRows)
probe.logicalTreeReadyMs = observedAtMs
if (probe.firstDiffRenderMs === undefined && diffLines > 0) probe.firstDiffRenderMs = observedAtMs
readyStreak = !ready ? 0 : stable ? readyStreak + 1 : 1
if (readyStreak === stableFrames) probe.stableReadyMs = observedAtMs
if (probe.stableReadyMs === undefined) requestAnimationFrame(sample)
}
longTaskObserver?.observe({ type: "longtask", buffered: true })
document.addEventListener(
"click",
(event) => {
const toggle = event.target instanceof Element ? event.target.closest("button") : undefined
if (!toggle?.hasAttribute("data-review-pane-scaling-toggle")) return
probe.startedAt = performance.now()
performance.mark("opencode.review-pane-scaling.click")
requestAnimationFrame(sample)
},
{ capture: true, once: true },
)
;(window as Window & { __reviewPaneScalingProbe?: ReviewPaneScalingProbe }).__reviewPaneScalingProbe = probe
},
{ ...input, stableFrames: readyFrames },
)
}

View file

@ -10,6 +10,9 @@ const sessionB = "ses_review_tab_b"
const titleA = "Alpha session"
const titleB = "Beta session"
const server = `http://${process.env.PLAYWRIGHT_SERVER_HOST ?? "127.0.0.1"}:${process.env.PLAYWRIGHT_SERVER_PORT ?? "4096"}`
const diffs = Array.from({ length: 2_740 }, (_, index) =>
fileDiff(`src/generated-${String(index).padStart(4, "0")}.ts`),
)
// Marks the review pane DOM node so a remount (fresh node) is detectable.
const PROBE = "original"
@ -25,20 +28,34 @@ test("keeps the v2 review pane mounted when switching session tabs in a workspac
await expectSessionTitle(page, titleA)
await page.getByRole("button", { name: "Toggle review" }).click()
const reviewTab = page.getByRole("tab", { name: /Review/ })
const reviewTabPanel = page.getByRole("tabpanel", { name: /Review/ })
await expect(reviewTab).toHaveAttribute("aria-controls", "session-side-panel-review-tabpanel")
await expect(reviewTabPanel).toHaveAttribute("id", "session-side-panel-review-tabpanel")
const review = page.locator('#review-panel [data-component="session-review-v2"]')
await expectAppVisible(review)
await expectAppVisible(page.getByRole("button", { name: /example\.ts/ }))
await expectAppVisible(page.getByRole("button", { name: "generated-0000.ts" }))
await writeProbe(page)
await switchTab(page, titleB)
await expectSessionTitle(page, titleB)
await expectAppVisible(review)
await expectAppVisible(page.getByRole("button", { name: "generated-0000.ts" }))
expect(await readProbe(page)).toBe(PROBE)
await switchTab(page, titleA)
await expectSessionTitle(page, titleA)
await expectAppVisible(review)
await expectAppVisible(page.getByRole("button", { name: "generated-0000.ts" }))
expect(await readProbe(page)).toBe(PROBE)
const viewport = page.locator('#review-panel [data-slot="session-review-v2-sidebar-tree"] .scroll-view__viewport')
await viewport.hover()
await page.mouse.wheel(0, 100_000)
await expect
.poll(() => viewport.evaluate((element) => element.scrollHeight - element.clientHeight - element.scrollTop))
.toBeLessThanOrEqual(1)
await expect(page.getByRole("button", { name: "generated-2739.ts" })).toBeVisible()
})
type Probed = HTMLElement & { __e2eProbe?: string }
@ -80,16 +97,7 @@ async function setup(page: Page) {
default: { providerID: "opencode", modelID: "test" },
},
sessions: [session(sessionA, titleA, 1700000000000), session(sessionB, titleB, 1700000001000)],
vcsDiff: [
{
file: "src/example.ts",
additions: 1,
deletions: 1,
status: "modified",
patch:
"diff --git a/src/example.ts b/src/example.ts\n--- a/src/example.ts\n+++ b/src/example.ts\n@@ -1 +1 @@\n-export const value = 'before'\n+export const value = 'after'\n",
},
],
vcsDiff: diffs,
pageMessages: () => ({ items: [] }),
})
@ -127,3 +135,13 @@ function session(id: string, title: string, created: number) {
function sessionHref(sessionID: string) {
return `/server/${base64Encode(server)}/session/${sessionID}`
}
function fileDiff(file: string) {
return {
file,
additions: 1,
deletions: 1,
status: "modified",
patch: `diff --git a/${file} b/${file}\n--- a/${file}\n+++ b/${file}\n@@ -1 +1 @@\n-export const value = 'before'\n+export const value = 'after'\n`,
}
}

View file

@ -0,0 +1,212 @@
import { expect, test, type Page } from "@playwright/test"
import { mockOpenCodeServer } from "../utils/mock-server"
import { expectSessionTitle } from "../utils/waits"
const directory = "C:/OpenCode/ReviewTerminalStacked"
const projectID = "proj_review_terminal_stacked"
const sessionID = "ses_review_terminal_stacked"
const title = "Review terminal stacked"
const branchDiffs = [
fileDiff(".github/actions/setup-bun/action.yml", 7),
...Array.from({ length: 2_739 }, (_, index) =>
fileDiff(`src/branch/generated-${String(index).padStart(4, "0")}.ts`, 100),
),
]
test("keeps the review tree and terminal sized when both panels are open", async ({ page }) => {
test.setTimeout(120_000)
await page.setViewportSize({ width: 1400, height: 900 })
await mockOpenCodeServer(page, {
directory,
project: {
id: projectID,
worktree: directory,
vcs: "git",
name: "review-terminal-stacked",
time: { created: 1700000000000, updated: 1700000000000 },
sandboxes: [],
},
provider: {
all: [
{
id: "opencode",
name: "OpenCode",
models: { test: { id: "test", name: "Test", limit: { context: 200_000 } } },
},
],
connected: ["opencode"],
default: { providerID: "opencode", modelID: "test" },
},
sessions: [
{
id: sessionID,
slug: "review-terminal-stacked",
projectID,
directory,
title,
version: "dev",
time: { created: 1700000000000, updated: 1700000000000 },
},
],
pageMessages: () => ({ items: [] }),
})
await page.route(/\/vcs(?:\?.*)?$/, (route) =>
route.fulfill({
status: 200,
contentType: "application/json",
body: JSON.stringify({ branch: "review-pane-performance", default_branch: "dev" }),
}),
)
await page.route("**/vcs/diff**", (route) =>
route.fulfill({
status: 200,
contentType: "application/json",
body: JSON.stringify(
new URL(route.request().url()).searchParams.get("mode") === "branch"
? branchDiffs
: Array.from({ length: 7 }, (_, index) => fileDiff(`src/git-${index}.ts`, 1)),
),
}),
)
await page.route("**/pty", (route) =>
route.fulfill({
status: 200,
contentType: "application/json",
body: JSON.stringify({ id: "pty_review_terminal", title: "Terminal 1" }),
}),
)
await page.route("**/pty/pty_review_terminal", (route) =>
route.fulfill({ status: 200, contentType: "application/json", body: "{}" }),
)
await page.routeWebSocket("**/pty/pty_review_terminal/connect", () => undefined)
await page.addInitScript(() => {
localStorage.setItem("settings.v3", JSON.stringify({ general: { newLayoutDesigns: true } }))
localStorage.setItem(
"opencode.global.dat:layout",
JSON.stringify({ review: { diffStyle: "split", panelOpened: true } }),
)
})
await page.goto(`/${base64Encode(directory)}/session/${sessionID}`)
await expectSessionTitle(page, title)
await expect(page.locator("#review-panel")).toBeVisible()
await expectTree(page, 8, "git-0.ts")
await selectMode(page, "Git changes", "Branch changes")
await expect(page.getByRole("tab", { name: "Review 2740" })).toBeVisible()
await page.keyboard.press("Control+Backquote")
await expect(page.locator("#terminal-panel")).toBeVisible()
await expectTree(page, 2_745, "action.yml")
await expectStackGeometry(page)
const treeViewport = page.locator('#review-panel [data-slot="session-review-v2-sidebar-tree"] .scroll-view__viewport')
await treeViewport.hover()
await page.mouse.wheel(0, 100_000)
await expect
.poll(() => treeViewport.evaluate((element) => element.scrollHeight - element.clientHeight - element.scrollTop))
.toBeLessThanOrEqual(1)
const lastFile = page.getByRole("button", { name: "generated-2738.ts" })
await expect(lastFile).toBeVisible()
const bottomGap = await lastFile.evaluate((element) => {
const viewport = element.closest<HTMLElement>(".scroll-view__viewport")!.getBoundingClientRect()
return viewport.bottom - element.getBoundingClientRect().bottom
})
expect(bottomGap).toBeGreaterThanOrEqual(0)
expect(bottomGap).toBeLessThanOrEqual(16)
await selectMode(page, "Branch changes", "Git changes")
await expectTree(page, 8, "git-0.ts")
await selectMode(page, "Git changes", "Branch changes")
await expectTree(page, 2_745, "action.yml")
const filter = page.getByRole("searchbox", { name: "Filter files" })
await filter.fill("generated-2738")
await expectTree(page, 1, "generated-2738.ts")
await filter.fill("")
await expectTree(page, 2_745, "action.yml")
await page.getByRole("button", { name: "Toggle file tree" }).click()
await expect(page.locator('[data-slot="session-review-v2-sidebar"]')).toHaveAttribute("aria-hidden", "true")
await expect(page.locator('#review-panel [data-component="file-tree-v2"]')).toHaveCount(1)
await page.getByRole("button", { name: "Toggle file tree" }).click()
await expectTree(page, 2_745, "action.yml")
await page.keyboard.press("Control+Backquote")
await expect(page.locator("#terminal-panel")).toHaveCount(0)
await expectTree(page, 2_745, "action.yml")
await page.keyboard.press("Control+Backquote")
await expect(page.locator("#terminal-panel")).toBeVisible()
await expectTree(page, 2_745, "action.yml")
await page.getByRole("button", { name: "Toggle review" }).click()
await expect(page.locator("#review-panel")).toHaveAttribute("aria-hidden", "true")
await expect(page.locator('#review-panel [data-component="file-tree-v2"]')).toHaveCount(1)
await page.getByRole("button", { name: "Toggle review" }).click()
await expectTree(page, 2_745, "action.yml")
await page.setViewportSize({ width: 1_000, height: 700 })
await expectTree(page, 2_745, "action.yml")
await expectStackGeometry(page)
await page.setViewportSize({ width: 1_000, height: 120 })
await page.setViewportSize({ width: 1_400, height: 900 })
await expectTree(page, 2_745, "action.yml")
await expectStackGeometry(page)
})
async function selectMode(page: Page, current: string, next: string) {
await page.getByRole("button", { name: current }).click()
const option = page.getByRole("option", { name: next })
await expect(option).toBeVisible()
await option.click()
}
async function expectTree(page: Page, total: number, file: string) {
await expectMountedTree(page, total)
await expect(page.getByRole("button", { name: file })).toBeVisible()
}
async function expectMountedTree(page: Page, total: number) {
const tree = page.locator('#review-panel [data-component="file-tree-v2"]')
await expect(tree).toHaveAttribute("data-total-rows", String(total))
await expect
.poll(() => tree.evaluate((element) => element.querySelectorAll('[data-slot="file-tree-v2-row"]').length))
.toBeGreaterThan(0)
const state = await tree.evaluate((element) => ({
root: element.getBoundingClientRect().height,
viewport: element.closest<HTMLElement>(".scroll-view__viewport")!.getBoundingClientRect().height,
rows: element.querySelectorAll('[data-slot="file-tree-v2-row"]').length,
}))
expect(state.viewport).toBeGreaterThan(0)
expect(state.root).toBeGreaterThan(0)
expect(state.rows).toBeGreaterThan(0)
expect(state.rows).toBeLessThanOrEqual(60)
}
async function expectStackGeometry(page: Page) {
const geometry = await page.evaluate(() => {
const review = document.querySelector<HTMLElement>("#review-panel")!
const terminal = document.querySelector<HTMLElement>("#terminal-panel")!
const reviewParent = review.parentElement!.getBoundingClientRect()
const terminalParent = terminal.parentElement!.getBoundingClientRect()
return {
review: review.getBoundingClientRect().height,
reviewParent: reviewParent.height,
terminal: terminal.getBoundingClientRect().height,
terminalParent: terminalParent.height,
}
})
expect(Math.abs(geometry.review - geometry.reviewParent)).toBeLessThanOrEqual(1)
expect(Math.abs(geometry.terminal - geometry.terminalParent)).toBeLessThanOrEqual(1)
}
function base64Encode(value: string) {
return Buffer.from(value, "utf8").toString("base64").replace(/\+/g, "-").replace(/\//g, "_").replace(/=/g, "")
}
function fileDiff(file: string, additions: number) {
return {
file,
additions,
deletions: 0,
status: "modified",
patch: `diff --git a/${file} b/${file}\n--- a/${file}\n+++ b/${file}\n@@ -1 +1 @@\n-export const value = 'before'\n+export const value = 'after'\n`,
}
}