diff --git a/packages/app/e2e/performance/timeline/session-tab-switch-benchmark.spec.ts b/packages/app/e2e/performance/timeline/session-tab-switch-benchmark.spec.ts index 2e80d70381..48d4d3deb0 100644 --- a/packages/app/e2e/performance/timeline/session-tab-switch-benchmark.spec.ts +++ b/packages/app/e2e/performance/timeline/session-tab-switch-benchmark.spec.ts @@ -2,7 +2,13 @@ import type { Page } from "@playwright/test" import { expectSessionTitle } from "../../utils/waits" import { benchmark, expect, withBenchmarkPage } from "../benchmark" import { fixture } from "./session-timeline-stress.fixture" -import { installStressSessionTabs, mockStressTimeline, stressSessionHref } from "./timeline-test-helpers" +import { + createReviewDiffs, + installStressSessionTabs, + installTimelineSettings, + mockStressTimeline, + stressSessionHref, +} from "./timeline-test-helpers" import { measureSessionSwitch, waitForStableTimeline } from "./session-tab-switch-probe" type Result = Awaited> @@ -20,8 +26,41 @@ benchmark("benchmarks cold and hot session tab switching", async ({ browser, rep report({ results, summary: summarize(results) }) }) -async function trial(page: Page, mode: "cold" | "hot") { - await mockStressTimeline(page) +benchmark( + "benchmarks v2 session tab switching with and without the review pane", + async ({ browser, report }, testInfo) => { + benchmark.setTimeout(360_000) + const runs = Number(process.env.SESSION_TAB_SWITCH_RUNS ?? 5) + const results = { + closed: { cold: [] as Result[], hot: [] as Result[] }, + open: { cold: [] as Result[], hot: [] as Result[] }, + } + for (const reviewPane of ["closed", "open"] as const) { + for (const mode of ["cold", "hot"] as const) { + for (let run = 0; run < runs; run++) { + results[reviewPane][mode].push( + await withBenchmarkPage( + browser, + `session-tab-switch-v2-${reviewPane}-${mode}-${run}`, + (page) => trial(page, mode, { newLayoutDesigns: true, reviewPane }), + testInfo, + ), + ) + } + } + } + report({ results, summary: summarizeReviewPane(results) }, { runs, reviewDiffs: createReviewDiffs().length }) + }, +) + +async function trial( + page: Page, + mode: "cold" | "hot", + options?: { newLayoutDesigns?: boolean; reviewPane?: "closed" | "open" }, +) { + const reviewDiffs = options?.newLayoutDesigns ? createReviewDiffs() : undefined + await mockStressTimeline(page, { vcsDiff: reviewDiffs }) + if (options?.newLayoutDesigns) await installTimelineSettings(page) await installStressSessionTabs(page) if (mode === "hot") { await page.goto(stressSessionHref(fixture.targetID)) @@ -33,6 +72,10 @@ async function trial(page: Page, mode: "cold" | "hot") { await expectSessionTitle(page, fixture.expected.sourceTitle) } await waitForStableTimeline(page, fixture.expected.sourceMessageIDs.at(-1)!) + if (options?.reviewPane === "open") { + await openReviewPane(page) + await waitForStableTimeline(page, fixture.expected.sourceMessageIDs.at(-1)!) + } const destinationIDs = fixture.messages[fixture.targetID].map((message) => message.info.id) const sourceIDs = fixture.messages[fixture.sourceID].map((message) => message.info.id) @@ -70,6 +113,15 @@ function summarize(results: Record<"cold" | "hot", Result[]>) { ) } +function summarizeReviewPane(results: Record<"closed" | "open", Record<"cold" | "hot", Result[]>>) { + return Object.fromEntries( + Object.entries(results).map(([reviewPane, values]) => [ + reviewPane, + summarize(values as Record<"cold" | "hot", Result[]>), + ]), + ) +} + async function switchSession(page: Page, sessionID: string, title: string) { const href = stressSessionHref(sessionID) const tab = page.locator(`[data-slot="titlebar-tabs"] a[href="${href}"]`).first() @@ -77,3 +129,16 @@ async function switchSession(page: Page, sessionID: string, title: string) { await tab.click() await expectSessionTitle(page, title) } + +async function openReviewPane(page: Page) { + await page.getByRole("button", { name: "Toggle review" }).click() + const panel = page.locator("#review-panel") + await expect(panel).toBeVisible() + // Text-based readiness works across review implementations; the legacy list mounts + // diff viewers lazily while V2 mounts the active preview eagerly. + await page.waitForFunction(() => { + const panel = document.querySelector("#review-panel") + const text = panel?.textContent ?? "" + return text.includes("generated-000.ts") && text.includes("+3") + }) +} diff --git a/packages/app/e2e/performance/timeline/session-tab-switch-metrics.ts b/packages/app/e2e/performance/timeline/session-tab-switch-metrics.ts index e315c2ad43..05a0dae496 100644 --- a/packages/app/e2e/performance/timeline/session-tab-switch-metrics.ts +++ b/packages/app/e2e/performance/timeline/session-tab-switch-metrics.ts @@ -5,6 +5,12 @@ export type SessionSwitchSample = { hasVisibleRows: boolean last: boolean bottomErrorPx?: number + review?: { + fileHost: boolean + fileHostReplaced: boolean + header: string + replacedLevels: string[] + } } export function classifySessionSwitch(samples: SessionSwitchSample[]) { @@ -23,6 +29,10 @@ export function classifySessionSwitch(samples: SessionSwitchSample[]) { (sample) => sample.hasVisibleRows && sample.destination.length === 0 && sample.source.length === 0, ).length, sourceSamples: samples.filter((sample) => sample.source.length > 0).length, + reviewFileHostMissingSamples: samples.filter((sample) => sample.review && !sample.review.fileHost).length, + reviewFileHostReplacedSamples: samples.filter((sample) => sample.review?.fileHostReplaced).length, + reviewHeaders: [...new Set(samples.flatMap((sample) => (sample.review ? [sample.review.header] : [])))], + reviewReplacedLevels: [...new Set(samples.flatMap((sample) => sample.review?.replacedLevels ?? []))], } } diff --git a/packages/app/e2e/performance/timeline/session-tab-switch-probe.ts b/packages/app/e2e/performance/timeline/session-tab-switch-probe.ts index 14f9d2d003..f61160f7a7 100644 --- a/packages/app/e2e/performance/timeline/session-tab-switch-probe.ts +++ b/packages/app/e2e/performance/timeline/session-tab-switch-probe.ts @@ -16,11 +16,41 @@ async function installSessionSwitchProbe( const samples: SessionSwitchSample[] = [] let started: number | undefined let running = true + const reviewLevels: Record = { + panel: "#review-panel", + tabs: '#review-panel [data-component="tabs"]', + body: '#review-panel [data-slot="session-review-v2-body"]', + review: '#review-panel [data-component="session-review-v2"]', + preview: '#review-panel [data-slot="session-review-v2-preview"]', + scroll: '#review-panel [data-slot="session-review-v2-diff-scroll"]', + file: '#review-panel [data-component="file"][data-mode="diff"]', + } + const initialReviewNodes: Record = {} const sample = () => { if (!running || started === undefined) return setTimeout(() => { if (!running || started === undefined) return const observedAtMs = performance.now() - started + const reviewPanel = document.querySelector("#review-panel") + const reviewFile = reviewPanel?.querySelector('[data-component="file"][data-mode="diff"]') + const initialReviewFile = initialReviewNodes.file + const replacedLevels = Object.entries(reviewLevels).flatMap(([name, selector]) => { + const initial = initialReviewNodes[name] + if (!initial) return [] + const current = document.querySelector(selector) + return current && current !== initial ? [name] : [] + }) + const review = reviewPanel + ? { + fileHost: !!reviewFile, + fileHostReplaced: !!initialReviewFile && !!reviewFile && reviewFile !== initialReviewFile, + header: + reviewPanel + .querySelector('[data-slot="session-review-v2-file-header"]') + ?.textContent?.trim() ?? "", + replacedLevels, + } + : undefined const root = [...document.querySelectorAll(".scroll-view__viewport")].find((element) => element.querySelector("[data-timeline-row]"), ) @@ -44,9 +74,10 @@ async function installSessionSwitchProbe( hasVisibleRows, last: visible.includes(lastID), bottomErrorPx: spacer ? spacer.bottom - view.bottom : undefined, + review, }) } else { - samples.push({ observedAtMs, destination: [], source: [], hasVisibleRows: false, last: false }) + samples.push({ observedAtMs, destination: [], source: [], hasVisibleRows: false, last: false, review }) } requestAnimationFrame(sample) }, 0) @@ -57,6 +88,9 @@ async function installSessionSwitchProbe( const link = event.target instanceof Element ? event.target.closest("a") : undefined if (link?.getAttribute("href") !== href) return started = performance.now() + for (const [name, selector] of Object.entries(reviewLevels)) { + initialReviewNodes[name] = document.querySelector(selector) + } requestAnimationFrame(sample) }, { capture: true, once: true }, diff --git a/packages/app/e2e/performance/timeline/session-timeline-benchmark.fixture.ts b/packages/app/e2e/performance/timeline/session-timeline-benchmark.fixture.ts index 4dad1df37b..a86a55cff2 100644 --- a/packages/app/e2e/performance/timeline/session-timeline-benchmark.fixture.ts +++ b/packages/app/e2e/performance/timeline/session-timeline-benchmark.fixture.ts @@ -93,36 +93,53 @@ const assistantMessage = { parts: [editPart], } -export async function setupTimelineBenchmark(page: Page, options: { historyTurns: number; eventBatch: number }) { +export async function setupTimelineBenchmark( + page: Page, + options: { + historyTurns: number + eventBatch: number + newLayoutDesigns?: boolean + vcsDiff?: unknown[] + turnDiffs?: unknown[] + }, +) { const events: EventPayload[] = [] let eventBatch = options.eventBatch + const currentUserMessage = options.turnDiffs + ? { ...userMessage, info: { ...userMessage.info, summary: { diffs: options.turnDiffs } } } + : userMessage await mockOpenCodeServer(page, { directory, project: project(), provider: provider(), sessions: [session()], + vcsDiff: options.vcsDiff, pageMessages: () => ({ items: [ ...Array.from({ length: options.historyTurns }, (_, index) => performanceTurn(index)).flat(), - userMessage, + currentUserMessage, assistantMessage, ], }), events: () => events.splice(0, eventBatch), eventRetry: 16, }) - await page.addInitScript(() => { - localStorage.setItem( - "settings.v3", - JSON.stringify({ - general: { - editToolPartsExpanded: true, - shellToolPartsExpanded: true, - showReasoningSummaries: true, - }, - }), - ) - }) + await page.addInitScript( + (input) => { + localStorage.setItem( + "settings.v3", + JSON.stringify({ + general: { + newLayoutDesigns: input.newLayoutDesigns, + editToolPartsExpanded: true, + shellToolPartsExpanded: true, + showReasoningSummaries: true, + }, + }), + ) + }, + { newLayoutDesigns: options.newLayoutDesigns ?? false }, + ) await page.setViewportSize({ width: 1366, height: 768 }) const scroller = page.locator(".scroll-view__viewport", { has: page.locator("[data-timeline-row]") }) const text = page.locator(`[data-timeline-part-id="${textPartID}"]`).first() diff --git a/packages/app/e2e/performance/timeline/session-timeline-benchmark.spec.ts b/packages/app/e2e/performance/timeline/session-timeline-benchmark.spec.ts index 64d79283f0..1a52bc8aa3 100644 --- a/packages/app/e2e/performance/timeline/session-timeline-benchmark.spec.ts +++ b/packages/app/e2e/performance/timeline/session-timeline-benchmark.spec.ts @@ -1,3 +1,4 @@ +import type { Page } from "@playwright/test" import { benchmark, benchmarkDiagnostics, expect } from "../benchmark" import { buildInitialStreamEvent, @@ -6,80 +7,300 @@ import { textPartID, } from "./session-timeline-benchmark.fixture" import { startTimelineProfile } from "./session-timeline-profile" +import { createReviewDiffs } from "./timeline-test-helpers" import { collectTimelineStreamMetrics, installTimelineStreamProbe, startTimelineStreamProbe, } from "./session-timeline-stream-probe" +type TimelineStreamOptions = { + newLayoutDesigns?: boolean + reviewDiffs?: boolean + reviewPane?: boolean +} + +type ReviewPaneSample = { + observedAtMs: number + panelVisible: boolean + header: string + diffViewers: number + diffLines: number + codeBlocks: number + ready: boolean +} + +type ReviewPaneProbe = { + samples: ReviewPaneSample[] + start: () => void + stop: () => void +} + +const reviewReadyStreak = 3 + benchmark.describe("performance: session timeline streaming", () => { benchmark("streams assistant text without remounting or oscillating", async ({ page, report }) => { - benchmark.setTimeout(480_000) - const cpuThrottle = Number(process.env.TIMELINE_CPU_THROTTLE ?? 30) - const deltaCount = Number(process.env.TIMELINE_DELTA_COUNT ?? 160) - const historyTurns = Number(process.env.TIMELINE_HISTORY_TURNS ?? 320) - const eventBatch = Number(process.env.TIMELINE_EVENT_BATCH ?? 1) - const minimal = process.env.TIMELINE_MINIMAL === "1" - const profileCPU = process.env.TIMELINE_CPU_PROFILE === "1" - const profileVisual = !minimal && profileCPU && process.env.TIMELINE_VISUAL_PROFILE !== "0" + benchmark.setTimeout(Number(process.env.TIMELINE_COMPLETION_TIMEOUT_MS ?? 420_000) + 60_000) + const result = await runTimelineStreamBenchmark(page, {}) + report(result.metrics, result.context) + }) + + benchmark("streams assistant text in v2 with review pane closed", async ({ page, report }) => { + benchmark.setTimeout(Number(process.env.TIMELINE_COMPLETION_TIMEOUT_MS ?? 420_000) + 60_000) + const result = await runTimelineStreamBenchmark(page, { newLayoutDesigns: true }) + report(result.metrics, result.context) + }) + + benchmark("streams assistant text in v2 with review diffs and pane closed", async ({ page, report }) => { + benchmark.setTimeout(Number(process.env.TIMELINE_COMPLETION_TIMEOUT_MS ?? 420_000) + 60_000) + const result = await runTimelineStreamBenchmark(page, { newLayoutDesigns: true, reviewDiffs: true }) + report(result.metrics, result.context) + }) + + benchmark("streams assistant text in v2 with review pane open", async ({ page, report }) => { + benchmark.setTimeout(Number(process.env.TIMELINE_COMPLETION_TIMEOUT_MS ?? 420_000) + 60_000) + const result = await runTimelineStreamBenchmark(page, { newLayoutDesigns: true, reviewPane: true }) + report(result.metrics, result.context) + }) +}) + +benchmark.describe("performance: review pane", () => { + benchmark("loads v2 review diffs and switches active files", async ({ page, report }) => { + benchmark.setTimeout(240_000) + const historyTurns = Number(process.env.REVIEW_PANE_HISTORY_TURNS ?? 72) + const diffs = createReviewDiffs() const fixture = await setupTimelineBenchmark(page, { historyTurns, - eventBatch, + eventBatch: 1, + newLayoutDesigns: true, + vcsDiff: diffs, }) - fixture.transport.enqueue(buildInitialStreamEvent(deltaCount)) - const contentStart = performance.now() + fixture.transport.enqueue(buildInitialStreamEvent(1)) await expect(fixture.text).toBeVisible() await expect(fixture.text).toContainText("Implementation plan") - const initialContentObservedMs = performance.now() - contentStart await fixture.scrollToBottom() await fixture.waitForStableGeometry() - const profile = await startTimelineProfile(page, { cpuThrottle, profileCPU }) - await installTimelineStreamProbe(page, { textPartID, finalIndex: deltaCount, profileVisual, minimal }) - const deltas = buildStreamDeltaEvents(deltaCount) - await startTimelineStreamProbe(page) - fixture.transport.enqueue(deltas) - - await page.waitForFunction( - (finalIndex) => - ( - window as Window & { - __timelineStreamBenchmark?: { applied: { index: number }[] } - } - ).__timelineStreamBenchmark?.applied.some((value) => value.index === finalIndex), - deltaCount, - { timeout: 420_000 }, - ) - await expect(fixture.text).toContainText("benchmark-complete") - await expect(fixture.text).toContainText("Streaming") - await fixture.waitForStableGeometry() - const metrics = await collectTimelineStreamMetrics(page, { - textPartID, - finalIndex: deltaCount, - navigations: benchmarkDiagnostics(page).navigations, - }) - const delivered = deltas.length - fixture.transport.pendingCount() - await profile.stop() + const open = await measureReviewPaneLoad(page, diffs[0]!.file) + const switches = [] + for (const diff of diffs.slice(1, 4)) switches.push(await measureReviewNextFile(page, diff.file)) report( { - endToEndInitialContentObservedMs: initialContentObservedMs, - ...metrics, - deliveredDeltas: delivered, - pendingDeltas: fixture.transport.pendingCount(), + open, + switches, }, { - cpuThrottle, - profileCPU, - profileVisual, - minimal, - queuedDeltas: deltas.length, historyTurns, - eventBatch, + reviewDiffs: diffs.length, }, ) - - await profile.reset() }) }) + +async function runTimelineStreamBenchmark(page: Page, options: TimelineStreamOptions) { + const completionTimeoutMs = Number(process.env.TIMELINE_COMPLETION_TIMEOUT_MS ?? 420_000) + const cpuThrottle = Number(process.env.TIMELINE_CPU_THROTTLE ?? 30) + const deltaCount = Number(process.env.TIMELINE_DELTA_COUNT ?? 160) + const historyTurns = Number(process.env.TIMELINE_HISTORY_TURNS ?? 320) + const eventBatch = Number(process.env.TIMELINE_EVENT_BATCH ?? 1) + const minimal = process.env.TIMELINE_MINIMAL === "1" + const profileCPU = process.env.TIMELINE_CPU_PROFILE === "1" + const profileVisual = !minimal && profileCPU && process.env.TIMELINE_VISUAL_PROFILE !== "0" + const diffs = options.reviewDiffs || options.reviewPane ? createReviewDiffs() : undefined + const fixture = await setupTimelineBenchmark(page, { + historyTurns, + eventBatch, + newLayoutDesigns: options.newLayoutDesigns, + // Turn diffs exercise timeline data cost; the pane-open scenario serves the same + // diffs through the default git mode so it works across review implementations. + turnDiffs: options.reviewDiffs ? diffs : undefined, + vcsDiff: options.reviewPane ? diffs : undefined, + }) + + fixture.transport.enqueue(buildInitialStreamEvent(deltaCount)) + const contentStart = performance.now() + await expect(fixture.text).toBeVisible() + await expect(fixture.text).toContainText("Implementation plan") + const initialContentObservedMs = performance.now() - contentStart + await fixture.scrollToBottom() + await fixture.waitForStableGeometry() + + const reviewPane = options.reviewPane && diffs ? await measureReviewPaneLoad(page, diffs[0]!.file) : undefined + if (reviewPane) await fixture.waitForStableGeometry() + + const profile = await startTimelineProfile(page, { cpuThrottle, profileCPU }) + await installTimelineStreamProbe(page, { textPartID, finalIndex: deltaCount, profileVisual, minimal }) + const deltas = buildStreamDeltaEvents(deltaCount) + await startTimelineStreamProbe(page) + fixture.transport.enqueue(deltas) + + await page.waitForFunction( + (finalIndex) => + ( + window as Window & { + __timelineStreamBenchmark?: { applied: { index: number }[] } + } + ).__timelineStreamBenchmark?.applied.some((value) => value.index === finalIndex), + deltaCount, + { timeout: completionTimeoutMs }, + ) + await expect(fixture.text).toContainText("benchmark-complete") + await expect(fixture.text).toContainText("Streaming") + await fixture.waitForStableGeometry() + const metrics = await collectTimelineStreamMetrics(page, { + textPartID, + finalIndex: deltaCount, + navigations: benchmarkDiagnostics(page).navigations, + }) + const delivered = deltas.length - fixture.transport.pendingCount() + await profile.stop() + + const result = { + metrics: { + endToEndInitialContentObservedMs: initialContentObservedMs, + ...metrics, + deliveredDeltas: delivered, + pendingDeltas: fixture.transport.pendingCount(), + reviewPane: reviewPane ?? null, + }, + context: { + cpuThrottle, + profileCPU, + profileVisual, + minimal, + queuedDeltas: deltas.length, + historyTurns, + eventBatch, + newLayoutDesigns: options.newLayoutDesigns === true, + reviewPane: options.reviewPane === true ? "open" : "closed", + reviewDiffs: diffs?.length ?? 0, + }, + } + + await profile.reset() + return result +} + +async function measureReviewPaneLoad(page: Page, file: string) { + // Default git mode reads the mocked /vcs/diff data, so opening the pane is enough + // and the flow works across review pane implementations. + await installReviewPaneProbe(page, { file }) + await startReviewPaneProbe(page) + await page.getByRole("button", { name: "Toggle review" }).click() + await expect(page.locator("#review-panel")).toBeVisible() + return collectReviewPaneProbe(page) +} + +async function measureReviewNextFile(page: Page, file: string) { + await installReviewPaneProbe(page, { file }) + await startReviewPaneProbe(page) + await page.getByRole("button", { name: "Next file" }).click() + return collectReviewPaneProbe(page) +} + +async function installReviewPaneProbe(page: Page, input: { file: string }) { + await page.evaluate((input) => { + const samples: ReviewPaneSample[] = [] + const basename = input.file.split(/[\\/]/).at(-1) ?? input.file + let started: number | undefined + let running = true + + const paneState = () => { + const panel = document.querySelector("#review-panel") + const review = panel?.querySelector('[data-component="session-review-v2"]') + const rect = (review ?? panel)?.getBoundingClientRect() + const text = panel?.textContent ?? "" + const previewHeader = panel?.querySelector( + '[data-slot="session-review-v2-file-header"]', + )?.textContent + const header = previewHeader ?? text + const viewers = panel ? [...panel.querySelectorAll('[data-component="file"][data-mode="diff"]')] : [] + const codeBlocks = panel?.querySelectorAll("code").length ?? 0 + const diffLines = viewers.reduce( + (sum, viewer) => + sum + + (viewer.shadowRoot?.querySelectorAll("[data-line]").length ?? viewer.querySelectorAll("[data-line]").length), + 0, + ) + const panelVisible = + !!panel && panel.getAttribute("aria-hidden") !== "true" && !!rect && rect.width > 0 && rect.height > 0 + return { + panelVisible, + header: header.slice(0, 500), + diffViewers: viewers.length, + diffLines, + codeBlocks, + ready: + panelVisible && + header.includes(basename) && + (viewers.length > 0 || text.includes("+3") || diffLines > 0 || codeBlocks > 0), + } + } + + const sample = () => { + if (!running || started === undefined) return + requestAnimationFrame(() => { + setTimeout(() => { + if (!running || started === undefined) return + samples.push({ observedAtMs: performance.now() - started, ...paneState() }) + if (performance.now() - started < 10_000) sample() + }, 0) + }) + } + + ;(window as Window & { __reviewPaneProbe?: ReviewPaneProbe }).__reviewPaneProbe = { + samples, + start: () => { + started = performance.now() + performance.mark("opencode.review-pane.click") + sample() + }, + stop: () => { + running = false + }, + } + }, input) +} + +async function startReviewPaneProbe(page: Page) { + await page.evaluate(() => { + ;(window as Window & { __reviewPaneProbe?: ReviewPaneProbe }).__reviewPaneProbe!.start() + }) +} + +async function collectReviewPaneProbe(page: Page) { + await page.waitForFunction((streak) => { + const samples = (window as Window & { __reviewPaneProbe?: ReviewPaneProbe }).__reviewPaneProbe?.samples + if (!samples) return false + return samples.some((_, index) => { + const stable = samples.slice(index, index + streak) + return stable.length === streak && stable.every((sample) => sample.ready) + }) + }, reviewReadyStreak) + + const samples = await page.evaluate(() => { + const probe = (window as Window & { __reviewPaneProbe?: ReviewPaneProbe }).__reviewPaneProbe! + probe.stop() + return probe.samples + }) + return { summary: summarizeReviewPaneSamples(samples), samples } +} + +function summarizeReviewPaneSamples(samples: ReviewPaneSample[]) { + const firstReady = samples.find((sample) => sample.ready) + const stableIndex = samples.findIndex((_, index) => { + const stable = samples.slice(index, index + reviewReadyStreak) + return stable.length === reviewReadyStreak && stable.every((sample) => sample.ready) + }) + return { + samples: samples.length, + firstReadyObservedMs: firstReady?.observedAtMs ?? null, + stableReadyObservedMs: stableIndex === -1 ? null : samples[stableIndex + reviewReadyStreak - 1]!.observedAtMs, + notReadySamples: samples.filter((sample) => !sample.ready).length, + maxDiffViewers: Math.max(0, ...samples.map((sample) => sample.diffViewers)), + maxDiffLines: Math.max(0, ...samples.map((sample) => sample.diffLines)), + maxCodeBlocks: Math.max(0, ...samples.map((sample) => sample.codeBlocks)), + } +} diff --git a/packages/app/e2e/performance/timeline/timeline-test-helpers.ts b/packages/app/e2e/performance/timeline/timeline-test-helpers.ts index 6360b4a793..5028373c55 100644 --- a/packages/app/e2e/performance/timeline/timeline-test-helpers.ts +++ b/packages/app/e2e/performance/timeline/timeline-test-helpers.ts @@ -21,7 +21,10 @@ export async function installTimelineSettings(page: Page) { export function mockStressTimeline( page: Page, - input?: { onMessages?: (input: { sessionID: string; before?: string; phase: "start" | "end" }) => void }, + input?: { + onMessages?: (input: { sessionID: string; before?: string; phase: "start" | "end" }) => void + vcsDiff?: unknown[] + }, ) { return mockOpenCodeServer(page, { sessions: fixture.sessions, @@ -30,6 +33,7 @@ export function mockStressTimeline( project: fixture.project, pageMessages, onMessages: input?.onMessages, + vcsDiff: input?.vcsDiff, }) } @@ -78,3 +82,53 @@ export function stressDraftHref(draftID: string) { function stressServer() { return `http://${process.env.PLAYWRIGHT_SERVER_HOST ?? "127.0.0.1"}:${process.env.PLAYWRIGHT_SERVER_PORT ?? "4096"}` } + +export function createReviewDiffs() { + return Array.from({ length: Number(process.env.REVIEW_PANE_DIFF_COUNT ?? 72) }, (_, index) => { + const lines = index % 3 === 0 ? 300 : index % 3 === 1 ? 120 : 38 + const file = `src/review/generated-${String(index).padStart(3, "0")}.ts` + const before = reviewSource(index, lines) + const after = before + .replace(`value_${index}_4`, `updated_${index}_4`) + .replace( + `value_${index}_${Math.max(8, Math.floor(lines / 2))}`, + `updated_${index}_${Math.max(8, Math.floor(lines / 2))}`, + ) + .replace(`value_${index}_${lines - 4}`, `updated_${index}_${lines - 4}`) + return { + file, + patch: reviewPatch(file, before, after), + additions: 3, + deletions: 3, + status: "modified" as const, + } + }) +} + +function reviewSource(seed: number, lines: number) { + return Array.from( + { length: lines }, + (_, index) => `export const value_${seed}_${index} = "${reviewWords(seed + index, index % 5 === 0 ? 180 : 42)}"`, + ).join("\n") +} + +function reviewPatch(file: string, before: string, after: string) { + const beforeLines = before.split("\n") + const afterLines = after.split("\n") + return [ + `diff --git a/${file} b/${file}`, + `--- a/${file}`, + `+++ b/${file}`, + `@@ -1,${beforeLines.length} +1,${afterLines.length} @@`, + ...beforeLines.flatMap((line, index) => { + const next = afterLines[index]! + if (line === next) return [` ${line}`] + return [`-${line}`, `+${next}`] + }), + ].join("\n") +} + +function reviewWords(seed: number, length: number) { + const words = ["alpha", "bravo", "charlie", "delta", "echo", "foxtrot", "golf", "hotel", "india", "juliet"] + return Array.from({ length: Math.ceil(length / 7) }, (_, index) => words[(seed + index * 3) % words.length]).join(" ") +} diff --git a/packages/app/e2e/regression/review-image-flash.spec.ts b/packages/app/e2e/regression/review-image-flash.spec.ts new file mode 100644 index 0000000000..dd200384d4 --- /dev/null +++ b/packages/app/e2e/regression/review-image-flash.spec.ts @@ -0,0 +1,202 @@ +import { expect, test, type Page } from "@playwright/test" +import { base64Encode } from "@opencode-ai/core/util/encode" +import { mockOpenCodeServer } from "../utils/mock-server" +import { expectAppVisible, expectSessionTitle } from "../utils/waits" + +const directory = "C:/OpenCode/ReviewImageFlashRegression" +const sessionID = "ses_review_image_flash_regression" +const title = "Review image flash regression" +const imageFile = "assets/preview.png" + +test("clicking an image file in the v2 review pane does not blank the panel", async ({ page }) => { + await openReview(page) + await installReviewFlashProbe(page) + + await page.getByRole("button", { name: /preview\.png/ }).click() + await waitForReviewFlashProbe(page, 400) + const trace = await collectReviewFlashProbe(page) + const bad = trace.samples.filter((sample) => sample.blank || sample.blackCenter) + + expect(trace.samples.length).toBeGreaterThan(0) + expect( + bad, + JSON.stringify({ bad: bad.slice(0, 8), first: trace.samples.slice(0, 8), last: trace.samples.slice(-4) }, null, 2), + ).toEqual([]) +}) + +async function openReview(page: Page) { + await page.setViewportSize({ width: 960, height: 900 }) + await page.addInitScript(() => { + localStorage.setItem("settings.v3", JSON.stringify({ general: { newLayoutDesigns: true } })) + }) + await mockOpenCodeServer(page, { + directory, + project: { + id: "proj_review_image_flash_regression", + worktree: directory, + vcs: "git", + name: "review-image-flash-regression", + time: { created: 1700000000000, updated: 1700000000000 }, + sandboxes: [], + }, + provider: { all: [], connected: [], default: {} }, + sessions: [ + { + id: sessionID, + slug: "review-image-flash-regression", + projectID: "proj_review_image_flash_regression", + directory, + title, + version: "dev", + time: { created: 1700000000000, updated: 1700000000000 }, + }, + ], + 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", + }, + { + file: imageFile, + patch: "", + additions: 1, + deletions: 0, + status: "added", + }, + ], + fileContent: async (path) => { + if (path !== imageFile) return undefined + await new Promise((resolve) => setTimeout(resolve, 250)) + return { + type: "binary", + content: "iVBORw0KGgo=", + encoding: "base64", + mimeType: "image/png", + } + }, + fileList: (path) => { + if (!path) { + return [ + { name: "assets", path: "assets", absolute: `${directory}/assets`, type: "directory", ignored: false }, + { name: "src", path: "src", absolute: `${directory}/src`, type: "directory", ignored: false }, + ] + } + if (path === "assets") { + return [ + { + name: "preview.png", + path: imageFile, + absolute: `${directory}/${imageFile}`, + type: "file", + ignored: false, + }, + ] + } + if (path === "src") { + return [ + { + name: "example.ts", + path: "src/example.ts", + absolute: `${directory}/src/example.ts`, + type: "file", + ignored: false, + }, + ] + } + return [] + }, + pageMessages: () => ({ + items: [ + { + info: { + id: "msg_review_image_flash_regression", + sessionID, + role: "user", + time: { created: 1700000000000 }, + summary: { diffs: [] }, + agent: "build", + model: { providerID: "opencode", modelID: "test" }, + }, + parts: [ + { + id: "prt_review_image_flash_regression", + sessionID, + messageID: "msg_review_image_flash_regression", + type: "text", + text: "Review this change.", + }, + ], + }, + ], + }), + }) + + await page.goto(`/${base64Encode(directory)}/session/${sessionID}`) + await expectSessionTitle(page, title) + await page.getByRole("button", { name: "Toggle review" }).click() + await expectAppVisible(page.locator('#review-panel [data-component="session-review-v2"]')) + await expectAppVisible(page.getByRole("button", { name: /preview\.png/ })) +} + +async function installReviewFlashProbe(page: Page) { + await page.evaluate(() => { + const samples: Array<{ + observedAtMs: number + blank: boolean + blackCenter: boolean + text: string + background: string + }> = [] + const startedAt = performance.now() + const sample = () => { + const panel = document.querySelector('#review-panel [data-component="session-review-v2"]') + const rect = panel?.getBoundingClientRect() + const center = rect + ? document.elementFromPoint(rect.left + rect.width / 2, rect.top + rect.height / 2) + : undefined + const background = center instanceof Element ? getComputedStyle(center).backgroundColor : "" + samples.push({ + observedAtMs: performance.now() - startedAt, + blank: !panel || panel.textContent?.trim().length === 0, + blackCenter: background === "rgb(0, 0, 0)", + text: panel?.textContent?.trim().slice(0, 80) ?? "", + background, + }) + if (performance.now() - startedAt < 500) requestAnimationFrame(sample) + } + document.addEventListener( + "click", + (event) => { + const target = event.target instanceof Element ? event.target : undefined + if (!target?.closest('[data-slot="file-tree-v2-row"]')) return + requestAnimationFrame(sample) + }, + { capture: true, once: true }, + ) + ;(window as Window & { __reviewImageFlash?: { samples: typeof samples; startedAt: number } }).__reviewImageFlash = { + samples, + startedAt, + } + }) +} + +async function waitForReviewFlashProbe(page: Page, durationMs: number) { + await page.waitForFunction((durationMs) => { + const state = (window as Window & { __reviewImageFlash?: { samples: unknown[]; startedAt: number } }) + .__reviewImageFlash + return !!state && state.samples.length > 0 && performance.now() - state.startedAt >= durationMs + }, durationMs) +} + +async function collectReviewFlashProbe(page: Page) { + return page.evaluate(() => { + return (window as Window & { __reviewImageFlash?: { samples: unknown[]; startedAt: number } }).__reviewImageFlash! + }) as Promise<{ + startedAt: number + samples: Array<{ observedAtMs: number; blank: boolean; blackCenter: boolean; text: string; background: string }> + }> +} diff --git a/packages/app/e2e/utils/mock-server.ts b/packages/app/e2e/utils/mock-server.ts index 2e06df3250..dc0a8d0c41 100644 --- a/packages/app/e2e/utils/mock-server.ts +++ b/packages/app/e2e/utils/mock-server.ts @@ -17,6 +17,8 @@ export interface MockServerConfig { todos?: (sessionID: string) => unknown[] permissions?: unknown[] | (() => unknown[]) questions?: unknown[] | (() => unknown[]) + fileList?: (path: string) => unknown | Promise + fileContent?: (path: string) => unknown | Promise sessionStatus?: unknown } @@ -56,6 +58,10 @@ export async function mockOpenCodeServer(page: Page, config: MockServerConfig) { return json(route, typeof config.questions === "function" ? config.questions() : (config.questions ?? [])) if (path === "/session/status") return json(route, config.sessionStatus ?? {}) if (path === "/vcs/diff" && config.vcsDiff) return json(route, config.vcsDiff) + if (path === "/file" && config.fileList) + return json(route, await config.fileList(url.searchParams.get("path") ?? "")) + if (path === "/file/content" && config.fileContent) + return json(route, await config.fileContent(url.searchParams.get("path") ?? "")) if (emptyObject.has(path)) return json(route, {}) if (emptyList.has(path)) return json(route, []) if (path in staticRoutes) return json(route, staticRoutes[path]) diff --git a/packages/app/src/components/file-tree-v2.tsx b/packages/app/src/components/file-tree-v2.tsx new file mode 100644 index 0000000000..302d9b58a3 --- /dev/null +++ b/packages/app/src/components/file-tree-v2.tsx @@ -0,0 +1,421 @@ +import { useFile } from "@/context/file" +import { Collapsible } from "@opencode-ai/ui/collapsible" +import { FileIcon } from "@opencode-ai/ui/file-icon" +import "@opencode-ai/ui/v2/file-tree-v2.css" +import { + createEffect, + createMemo, + For, + Match, + on, + Show, + splitProps, + Switch, + untrack, + type ComponentProps, + type ParentProps, +} from "solid-js" +import { Dynamic } from "solid-js/web" +import type { FileNode } from "@opencode-ai/sdk/v2" +import { Icon } from "@opencode-ai/ui/v2/icon" +import { + dirsToExpand, + pathToFileUrl, + shouldListRoot, + visibleKind, + withFileDragImage, + type Filter, + type Kind, +} from "@/components/file-tree" + +export type { Kind } from "@/components/file-tree" + +const MAX_DEPTH = 128 + +function visibleNodesForPath(path: string, children: (dir: string) => FileNode[], current: Filter | undefined) { + const nodes = children(path) + if (!current) return nodes + + const parent = (item: string) => { + const idx = item.lastIndexOf("/") + if (idx === -1) return "" + return item.slice(0, idx) + } + + const leaf = (item: string) => { + const idx = item.lastIndexOf("/") + return idx === -1 ? item : item.slice(idx + 1) + } + + const out = nodes.filter((node) => { + if (node.type === "file") return current.files.has(node.path) + return current.dirs.has(node.path) + }) + + const seen = new Set(out.map((node) => node.path)) + + for (const dir of current.dirs) { + if (parent(dir) !== path) continue + if (seen.has(dir)) continue + out.push({ + name: leaf(dir), + path: dir, + absolute: dir, + type: "directory", + ignored: false, + }) + seen.add(dir) + } + + for (const item of current.files) { + if (parent(item) !== path) continue + if (seen.has(item)) continue + out.push({ + name: leaf(item), + path: item, + absolute: item, + type: "file", + ignored: false, + }) + seen.add(item) + } + + out.sort((a, b) => { + if (a.type !== b.type) { + return a.type === "directory" ? -1 : 1 + } + return a.name.localeCompare(b.name) + }) + + return out +} + +const INDENT_STEP = 16 + +function rowPaddingLeft(level: number, type: FileNode["type"]) { + if (type === "directory") return 8 + level * INDENT_STEP + if (level === 0) return 8 + return 8 + level * INDENT_STEP - INDENT_STEP +} + +function guideLineLeft(level: number) { + return rowPaddingLeft(level, "directory") + 8 +} + +export const kindLabel = (kind: Kind) => { + if (kind === "add") return "A" + if (kind === "del") return "D" + return "" +} + +export const kindChange = (kind: Kind) => { + if (kind === "add") return "added" + if (kind === "del") return "deleted" + return "modified" +} + +const FileTreeNodeV2 = ( + p: ParentProps & + ComponentProps<"div"> & + ComponentProps<"button"> & { + node: FileNode + level: number + active?: string + draggable: boolean + kinds?: ReadonlyMap + marks?: Set + as?: "div" | "button" + }, +) => { + const [local, rest] = splitProps(p, [ + "node", + "level", + "active", + "draggable", + "kinds", + "marks", + "as", + "children", + "class", + "classList", + ]) + const kind = () => visibleKind(local.node, local.kinds, local.marks) + + return ( + { + if (!local.draggable) return + event.dataTransfer?.setData("text/plain", `file:${local.node.path}`) + event.dataTransfer?.setData("text/uri-list", pathToFileUrl(local.node.path)) + if (event.dataTransfer) event.dataTransfer.effectAllowed = "copy" + withFileDragImage(event) + }} + {...rest} + > + {local.children} + {local.node.name} + {(() => { + const value = kind() + if (!value || local.node.type !== "file") return null + return ( + + {kindLabel(value)} + + ) + })()} + + ) +} + +// V2-styled fork of FileTree for the review sidebar. Unlike the v1 tree it never +// lists unloaded subdirectories, so callers must pass `allowed` (nodes are +// synthesized from that list) for nested content to appear. +export default function FileTreeV2(props: { + path: string + active?: string + level?: number + allowed?: readonly string[] + kinds?: ReadonlyMap + draggable?: boolean + onFileClick?: (file: FileNode) => void + + _filter?: Filter + _marks?: Set + _deeps?: Map + _kinds?: ReadonlyMap + _chain?: readonly string[] +}) { + const file = useFile() + const level = props.level ?? 0 + const draggable = () => props.draggable ?? true + + const key = (p: string) => + file + .normalize(p) + .replace(/[\\/]+$/, "") + .replaceAll("\\", "/") + const chain = props._chain ? [...props._chain, key(props.path)] : [key(props.path)] + + const filter = createMemo(() => { + if (props._filter) return props._filter + + const allowed = props.allowed + if (!allowed) return + + const files = new Set(allowed) + const dirs = new Set() + + for (const item of allowed) { + const parts = item.split("/") + const parents = parts.slice(0, -1) + for (const [idx] of parents.entries()) { + const dir = parents.slice(0, idx + 1).join("/") + if (dir) dirs.add(dir) + } + } + + return { files, dirs } + }) + + const marks = createMemo(() => { + if (props._marks) return props._marks + + const out = new Set(props.kinds?.keys() ?? []) + if (out.size === 0) return + return out + }) + + const kinds = createMemo(() => { + if (props._kinds) return props._kinds + return props.kinds + }) + + const deeps = createMemo(() => { + if (props._deeps) return props._deeps + + const out = new Map() + + const root = props.path + if (!(file.tree.state(root)?.expanded ?? false)) return out + + const seen = new Set() + const stack: { dir: string; lvl: number; i: number; kids: string[]; max: number }[] = [] + + const push = (dir: string, lvl: number) => { + const id = key(dir) + if (seen.has(id)) return + seen.add(id) + + const kids = file.tree + .children(dir) + .filter((node) => node.type === "directory" && (file.tree.state(node.path)?.expanded ?? false)) + .map((node) => node.path) + + stack.push({ dir, lvl, i: 0, kids, max: lvl }) + } + + push(root, level - 1) + + while (stack.length > 0) { + const top = stack[stack.length - 1]! + + if (top.i < top.kids.length) { + const next = top.kids[top.i]! + top.i++ + push(next, top.lvl + 1) + continue + } + + out.set(top.dir, top.max) + stack.pop() + + const parent = stack[stack.length - 1] + if (!parent) continue + parent.max = Math.max(parent.max, top.max) + } + + return out + }) + + createEffect(() => { + const current = filter() + const dirs = dirsToExpand({ + level, + filter: current, + expanded: (dir) => untrack(() => file.tree.state(dir)?.expanded) ?? false, + }) + // Nodes come from the `allowed` filter; skip listing so directories that only + // exist on the diff's base branch do not each fail with an error toast. + for (const dir of dirs) file.tree.expand(dir, { list: false }) + }) + + createEffect( + on( + () => props.path, + (path) => { + const dir = untrack(() => file.tree.state(path)) + if (!shouldListRoot({ level, dir })) return + void file.tree.list(path) + }, + { defer: false }, + ), + ) + + const nodes = createMemo(() => visibleNodesForPath(props.path, file.tree.children, filter())) + + return ( + // group/file-tree-v2 scopes the group-hover guide lines below; hosts may add + // an outer group with the same name to widen the hover area. +
+ + {(node) => { + const expanded = () => file.tree.state(node.path)?.expanded ?? false + const deep = () => deeps().get(node.path) ?? -1 + const hasChildren = () => visibleNodesForPath(node.path, file.tree.children, filter()).length > 0 + return ( + + + + open ? file.tree.expand(node.path, { list: false }) : file.tree.collapse(node.path) + } + > + + +
+ +
+
+
+ + +
+ ...
} + > + +
+ + +
+
+ + props.onFileClick?.(node)} + > + 0}> +
+ + } + > + + + + + + + + + ) + }} + +
+ ) +} diff --git a/packages/app/src/components/file-tree.tsx b/packages/app/src/components/file-tree.tsx index 211ce05ef0..93ff46d5c1 100644 --- a/packages/app/src/components/file-tree.tsx +++ b/packages/app/src/components/file-tree.tsx @@ -21,13 +21,13 @@ import type { FileNode } from "@opencode-ai/sdk/v2" const MAX_DEPTH = 128 -function pathToFileUrl(filepath: string): string { +export function pathToFileUrl(filepath: string): string { return `file://${encodeFilePath(filepath)}` } -type Kind = "add" | "del" | "mix" +export type Kind = "add" | "del" | "mix" -type Filter = { +export type Filter = { files: Set dirs: Set } @@ -78,7 +78,7 @@ const kindDotColor = (kind: Kind) => { return "background-color: var(--icon-diff-modified-base)" } -const visibleKind = (node: FileNode, kinds?: ReadonlyMap, marks?: Set) => { +export const visibleKind = (node: FileNode, kinds?: ReadonlyMap, marks?: Set) => { const kind = kinds?.get(node.path) if (!kind) return if (!marks?.has(node.path)) return @@ -99,7 +99,7 @@ const buildDragImage = (target: HTMLElement) => { return image } -const withFileDragImage = (event: DragEvent) => { +export const withFileDragImage = (event: DragEvent) => { const image = buildDragImage(event.currentTarget as HTMLElement) if (!image) return document.body.appendChild(image) diff --git a/packages/app/src/context/comments.tsx b/packages/app/src/context/comments.tsx index afc59b5956..09e890a5e5 100644 --- a/packages/app/src/context/comments.tsx +++ b/packages/app/src/context/comments.tsx @@ -85,7 +85,15 @@ function createCommentSessionState(store: Store, setStore: SetStor active: null as CommentFocus | null, }) - const all = () => aggregate(store.comments) + // Reuse the previous array when contents are unchanged so consumers keep a stable + // identity; a fresh array per call cascaded into diff annotation re-renders. + let lastAll: LineComment[] = [] + const all = () => { + const next = aggregate(store.comments) + if (next.length === lastAll.length && next.every((item, index) => item === lastAll[index])) return lastAll + lastAll = next + return next + } const setRef = ( key: "focus" | "active", diff --git a/packages/app/src/context/file/tree-store.ts b/packages/app/src/context/file/tree-store.ts index a86051d286..5769c318f0 100644 --- a/packages/app/src/context/file/tree-store.ts +++ b/packages/app/src/context/file/tree-store.ts @@ -127,10 +127,14 @@ export function createFileTreeStore(options: TreeStoreOptions) { return promise } - const expandDir = (input: string) => { + // `list: false` marks a directory expanded without fetching its children, for + // trees whose nodes are synthesized from a filter; listing directories that + // only exist on a diff's base branch fails and surfaces error toasts. + const expandDir = (input: string, behavior?: { list?: boolean }) => { const dir = options.normalizeDir(input) ensureDir(dir) setTree("dir", dir, "expanded", true) + if (behavior?.list === false) return void listDir(dir) } diff --git a/packages/app/src/pages/session.tsx b/packages/app/src/pages/session.tsx index b2b897c62b..7b175a6a30 100644 --- a/packages/app/src/pages/session.tsx +++ b/packages/app/src/pages/session.tsx @@ -24,8 +24,10 @@ import { debounce } from "@solid-primitives/scheduled" import { useLocal } from "@/context/local" import { FileProvider, selectionFromLines, useFile, type FileSelection, type SelectedLineRange } from "@/context/file" import { createStore } from "solid-js/store" +import type { SessionReviewLineComment } from "@opencode-ai/session-ui/session-review" import { ResizeHandle } from "@opencode-ai/ui/resize-handle" import { Select } from "@opencode-ai/ui/select" +import { SelectV2 } from "@opencode-ai/ui/v2/select-v2" import { isScrollKeyTarget, scrollKey, scrollKeyOwner } from "@opencode-ai/ui/scroll-view" import { Tabs } from "@opencode-ai/ui/tabs" import { ButtonV2 } from "@opencode-ai/ui/v2/button-v2" @@ -77,6 +79,10 @@ import { type DiffStyle, SessionReviewTab, type SessionReviewTabProps } from "@/ import { useSessionLayout } from "@/pages/session/session-layout" import { syncSessionModel } from "@/pages/session/session-model-helpers" import { SessionSidePanel } from "@/pages/session/session-side-panel" +import { SessionReviewEmptyChangesV2 } from "@opencode-ai/session-ui/v2/session-review-empty-changes-v2" +import { SessionReviewEmptyNoGitV2 } from "@opencode-ai/session-ui/v2/session-review-empty-no-git-v2" +import { ReviewPanelV2 } from "@/pages/session/v2/review-panel-v2" +import { createReviewPanelV2State } from "@/pages/session/v2/review-panel-v2-state" import { TerminalPanel } from "@/pages/session/terminal-panel" import { useComposerCommands } from "@/pages/session/use-composer-commands" import { useSessionCommands } from "@/pages/session/use-session-commands" @@ -1052,22 +1058,22 @@ export default function Page() { loadFile: file.load, }) + const changesLabel = (option: ChangeMode) => { + if (option === "git") return language.t("ui.sessionReview.title.git") + if (option === "branch") return language.t("ui.sessionReview.title.branch") + return language.t("ui.sessionReview.title.lastTurn") + } + const changesTitle = () => { if (!canReview()) { return null } - const label = (option: ChangeMode) => { - if (option === "git") return language.t("ui.sessionReview.title.git") - if (option === "branch") return language.t("ui.sessionReview.title.branch") - return language.t("ui.sessionReview.title.lastTurn") - } - return (
- +