Skip to content

Commit ca17311

Browse files
Apply PR #35633: fix(app): load capped review patches
2 parents f8e23ff + bd71fec commit ca17311

5 files changed

Lines changed: 199 additions & 22 deletions

File tree

‎packages/app/e2e/regression/review-terminal-stacked.spec.ts‎

Lines changed: 70 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -9,12 +9,19 @@ const title = "Review terminal stacked"
99
const branchDiffs = [
1010
fileDiff(".github/actions/setup-bun/action.yml", 7),
1111
...Array.from({ length: 2_739 }, (_, index) =>
12-
fileDiff(`src/branch/generated-${String(index).padStart(4, "0")}.ts`, 100),
12+
fileDiff(
13+
`src/branch/d${String(Math.floor(index / 100)).padStart(5, "0")}/generated-${String(index).padStart(4, "0")}.ts`,
14+
100,
15+
false,
16+
),
1317
),
1418
]
1519

1620
test("keeps the review tree and terminal sized when both panels are open", async ({ page }) => {
1721
test.setTimeout(120_000)
22+
const events: Array<{ directory: string; payload: Record<string, unknown> }> = []
23+
let detailVersion = 1
24+
let detailFailures = 1
1825
await page.setViewportSize({ width: 1400, height: 900 })
1926
await mockOpenCodeServer(page, {
2027
directory,
@@ -48,7 +55,10 @@ test("keeps the review tree and terminal sized when both panels are open", async
4855
time: { created: 1700000000000, updated: 1700000000000 },
4956
},
5057
],
58+
sessionStatus: { [sessionID]: { type: "idle" } },
5159
pageMessages: () => ({ items: [] }),
60+
events: () => events.splice(0, 1),
61+
eventRetry: 16,
5262
})
5363
await page.route(/\/vcs(?:\?.*)?$/, (route) =>
5464
route.fulfill({
@@ -57,17 +67,25 @@ test("keeps the review tree and terminal sized when both panels are open", async
5767
body: JSON.stringify({ branch: "review-pane-performance", default_branch: "dev" }),
5868
}),
5969
)
60-
await page.route("**/vcs/diff**", (route) =>
61-
route.fulfill({
70+
await page.route("**/vcs/diff**", (route) => {
71+
const url = new URL(route.request().url())
72+
const scope = url.searchParams.get("directory")?.replaceAll("\\", "/")
73+
const detail = scope?.endsWith("/src/branch/d00027")
74+
if (detail && detailFailures-- > 0) return route.fulfill({ status: 500, body: "retry detail" })
75+
return route.fulfill({
6276
status: 200,
6377
contentType: "application/json",
6478
body: JSON.stringify(
65-
new URL(route.request().url()).searchParams.get("mode") === "branch"
66-
? branchDiffs
79+
url.searchParams.get("mode") === "branch"
80+
? detail
81+
? branchDiffs
82+
.filter((diff) => diff.file.startsWith("src/branch/d00027/"))
83+
.map((diff) => fileDiff(diff.file, diff.additions, true, detailVersion))
84+
: branchDiffs
6785
: Array.from({ length: 7 }, (_, index) => fileDiff(`src/git-${index}.ts`, 1)),
6886
),
69-
}),
70-
)
87+
})
88+
})
7189
await page.route("**/pty", (route) =>
7290
route.fulfill({
7391
status: 200,
@@ -96,7 +114,7 @@ test("keeps the review tree and terminal sized when both panels are open", async
96114
await expect(page.getByRole("tab", { name: "Review 2740" })).toBeVisible()
97115
await page.keyboard.press("Control+Backquote")
98116
await expect(page.locator("#terminal-panel")).toBeVisible()
99-
await expectTree(page, 2_745, "action.yml")
117+
await expectTree(page, 2_773, "action.yml")
100118
await expectStackGeometry(page)
101119

102120
const treeViewport = page.locator('#review-panel [data-slot="session-review-v2-sidebar-tree"] .scroll-view__viewport')
@@ -113,40 +131,65 @@ test("keeps the review tree and terminal sized when both panels are open", async
113131
})
114132
expect(bottomGap).toBeGreaterThanOrEqual(0)
115133
expect(bottomGap).toBeLessThanOrEqual(16)
134+
const lazyDiff = page.waitForRequest((request) => {
135+
const url = new URL(request.url())
136+
return (
137+
url.pathname === "/vcs/diff" &&
138+
url.searchParams.get("directory")?.replaceAll("\\", "/").endsWith("/src/branch/d00027") === true
139+
)
140+
})
141+
await lastFile.click()
142+
await lazyDiff
143+
const preview = page.locator('[data-slot="session-review-v2-diff-scroll"]')
144+
await expect(preview).toContainText("after-1")
145+
detailVersion = 2
146+
events.push(statusEvent("busy"))
147+
await expect(page.getByRole("button", { name: "Stop" })).toBeVisible()
148+
const refreshedDiff = page.waitForRequest((request) => {
149+
const url = new URL(request.url())
150+
return (
151+
url.pathname === "/vcs/diff" &&
152+
url.searchParams.get("directory")?.replaceAll("\\", "/").endsWith("/src/branch/d00027") === true
153+
)
154+
})
155+
events.push(statusEvent("idle"))
156+
await refreshedDiff
157+
await expect(preview).toContainText("after-2")
116158
await selectMode(page, "Branch changes", "Git changes")
117159
await expectTree(page, 8, "git-0.ts")
160+
await page.getByRole("button", { name: "git-0.ts" }).click()
118161
await selectMode(page, "Git changes", "Branch changes")
119-
await expectTree(page, 2_745, "action.yml")
162+
await expectTree(page, 2_773, "action.yml")
120163

121164
const filter = page.getByRole("searchbox", { name: "Filter files" })
122165
await filter.fill("generated-2738")
123166
await expectTree(page, 1, "generated-2738.ts")
124167
await filter.fill("")
125-
await expectTree(page, 2_745, "action.yml")
168+
await expectTree(page, 2_773, "action.yml")
126169

127170
await page.getByRole("button", { name: "Toggle file tree" }).click()
128171
await expect(page.locator('[data-slot="session-review-v2-sidebar"]')).toHaveCount(0)
129172
await expect(page.locator('#review-panel [data-component="file-tree-v2"]')).toHaveCount(0)
130173
await page.getByRole("button", { name: "Toggle file tree" }).click()
131-
await expectTree(page, 2_745, "action.yml")
174+
await expectTree(page, 2_773, "action.yml")
132175

133176
await page.keyboard.press("Control+Backquote")
134177
await expect(page.locator("#terminal-panel")).toHaveCount(0)
135-
await expectTree(page, 2_745, "action.yml")
178+
await expectTree(page, 2_773, "action.yml")
136179
await page.keyboard.press("Control+Backquote")
137180
await expect(page.locator("#terminal-panel")).toBeVisible()
138-
await expectTree(page, 2_745, "action.yml")
181+
await expectTree(page, 2_773, "action.yml")
139182

140183
await page.getByRole("button", { name: "Toggle review" }).click()
141184
await expect(page.locator("#review-panel")).toHaveCount(0)
142185
await page.getByRole("button", { name: "Toggle review" }).click()
143-
await expectTree(page, 2_745, "action.yml")
186+
await expectTree(page, 2_773, "action.yml")
144187
await page.setViewportSize({ width: 1_000, height: 700 })
145-
await expectTree(page, 2_745, "action.yml")
188+
await expectTree(page, 2_773, "action.yml")
146189
await expectStackGeometry(page)
147190
await page.setViewportSize({ width: 1_000, height: 120 })
148191
await page.setViewportSize({ width: 1_400, height: 900 })
149-
await expectTree(page, 2_745, "action.yml")
192+
await expectTree(page, 2_773, "action.yml")
150193
await expectStackGeometry(page)
151194
})
152195

@@ -200,12 +243,21 @@ function base64Encode(value: string) {
200243
return Buffer.from(value, "utf8").toString("base64").replace(/\+/g, "-").replace(/\//g, "_").replace(/=/g, "")
201244
}
202245

203-
function fileDiff(file: string, additions: number) {
246+
function statusEvent(type: "busy" | "idle") {
247+
return {
248+
directory,
249+
payload: { type: "session.status", properties: { sessionID, status: { type } } },
250+
}
251+
}
252+
253+
function fileDiff(file: string, additions: number, loaded = true, version = 1) {
204254
return {
205255
file,
206256
additions,
207257
deletions: 0,
208258
status: "modified",
209-
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`,
259+
patch: loaded
260+
? `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-${version}'\n`
261+
: `diff --git a/${file} b/${file}\n--- a/${file}\n+++ b/${file}`,
210262
}
211263
}

‎packages/app/src/pages/session.tsx‎

Lines changed: 46 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import type { Project, UserMessage } from "@opencode-ai/sdk/v2"
1+
import type { Project, UserMessage, VcsFileDiff } from "@opencode-ai/sdk/v2"
22
import { useDialog } from "@opencode-ai/ui/context/dialog"
33
import { createQuery, skipToken, useMutation, useQueryClient } from "@tanstack/solid-query"
44
import {
@@ -81,6 +81,7 @@ import { SessionReviewEmptyChangesV2 } from "@opencode-ai/session-ui/v2/session-
8181
import { SessionReviewEmptyNoGitV2 } from "@opencode-ai/session-ui/v2/session-review-empty-no-git-v2"
8282
import { ReviewPanelV2 } from "@/pages/session/v2/review-panel-v2"
8383
import { createReviewPanelV2State } from "@/pages/session/v2/review-panel-v2-state"
84+
import { reviewDiffDirectory, reviewDiffNeedsLoad, reviewRootDirectory } from "@/pages/session/v2/review-diff-kinds"
8485
import { TerminalPanel } from "@/pages/session/terminal-panel"
8586
import { TerminalPanelV2 } from "@/pages/session/terminal-panel-v2"
8687
import { useComposerCommands } from "@/pages/session/use-composer-commands"
@@ -688,6 +689,46 @@ export default function Page() {
688689
if (reviewMode() === "git" || reviewMode() === "branch") return !vcsQuery.isPending
689690
return true
690691
}
692+
const loadReviewDiff = async (file: string, version?: number): Promise<VcsFileDiff | undefined> => {
693+
const mode = vcsMode()
694+
if (!mode) return
695+
const root = reviewRootDirectory(sync().project?.worktree ?? sdk().directory)
696+
const directory = reviewDiffDirectory(root, file)
697+
const source = reviewDiffs().find((diff) => diff.file === file)
698+
const valid = (diff: VcsFileDiff | undefined) => {
699+
if (!diff || !source) return
700+
if (diff.additions !== source.additions || diff.deletions !== source.deletions) return
701+
if (reviewDiffNeedsLoad(diff)) return
702+
return diff
703+
}
704+
const request = (scope: string, context?: number) =>
705+
queryClient
706+
.fetchQuery({
707+
queryKey: [serverSDK().scope, ...vcsKey(), mode, "directory", scope, context, version] as const,
708+
staleTime: Number.POSITIVE_INFINITY,
709+
retry: 2,
710+
queryFn: () =>
711+
sdk()
712+
.client.vcs.diff({ mode, directory: scope, context })
713+
.then((result) => result.data ?? []),
714+
})
715+
.then((diffs) => diffs.find((diff) => diff.file === file))
716+
717+
if (directory !== root) {
718+
try {
719+
const scoped = valid(await request(directory))
720+
if (scoped) return scoped
721+
} catch (error) {
722+
console.debug("[session-review] failed to load scoped vcs diff", { mode, file, directory, error })
723+
}
724+
}
725+
try {
726+
const bounded = valid(await request(root, 3))
727+
if (bounded) return bounded
728+
} catch (error) {
729+
console.debug("[session-review] failed to load bounded vcs diff", { mode, file, root, error })
730+
}
731+
}
691732

692733
const newSessionWorktree = createMemo(() => {
693734
if (store.newSessionWorktree === "create") return "create"
@@ -1219,6 +1260,10 @@ export default function Page() {
12191260
},
12201261
diffs: reviewDiffs,
12211262
diffsReady: reviewReady,
1263+
get diffVersion() {
1264+
return vcsQuery.dataUpdatedAt
1265+
},
1266+
loadDiff: loadReviewDiff,
12221267
get activeFile() {
12231268
return activeReviewFile()
12241269
},

‎packages/app/src/pages/session/v2/review-diff-kinds.test.ts‎

Lines changed: 41 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
import { describe, expect, test } from "bun:test"
2-
import { filterReviewFiles, reviewDiffKinds } from "./review-diff-kinds"
2+
import { filterReviewFiles, reviewDiffDirectory, reviewDiffKinds, reviewDiffNeedsLoad } from "./review-diff-kinds"
33

44
describe("reviewDiffKinds", () => {
55
test("maps file and directory kinds", () => {
@@ -28,3 +28,43 @@ describe("filterReviewFiles", () => {
2828
expect(filterReviewFiles(files, "")).toEqual(files)
2929
})
3030
})
31+
32+
describe("reviewDiffNeedsLoad", () => {
33+
test("loads changed files whose aggregate patch has no hunks", () => {
34+
expect(
35+
reviewDiffNeedsLoad({
36+
file: "src/a.ts",
37+
additions: 1,
38+
deletions: 0,
39+
patch: "diff --git a/src/a.ts b/src/a.ts\n--- a/src/a.ts\n+++ b/src/a.ts",
40+
}),
41+
).toBe(true)
42+
})
43+
44+
test("keeps complete patches and empty changes", () => {
45+
expect(
46+
reviewDiffNeedsLoad({
47+
file: "src/a.ts",
48+
additions: 1,
49+
deletions: 0,
50+
patch: "@@ -0,0 +1 @@\n+value",
51+
}),
52+
).toBe(false)
53+
expect(reviewDiffNeedsLoad({ file: "empty.txt", additions: 0, deletions: 0 })).toBe(false)
54+
})
55+
})
56+
57+
describe("reviewDiffDirectory", () => {
58+
test("scopes nested files to their parent directory", () => {
59+
expect(reviewDiffDirectory("/repo", "src/lib/a.ts")).toBe("/repo/src/lib")
60+
expect(reviewDiffDirectory("C:\\repo", "src/lib/a.ts")).toBe("C:\\repo\\src\\lib")
61+
})
62+
63+
test("does not rescope root files", () => {
64+
expect(reviewDiffDirectory("/repo/", "README.md")).toBe("/repo")
65+
expect(reviewDiffDirectory("/", "README.md")).toBe("/")
66+
expect(reviewDiffDirectory("C:\\", "README.md")).toBe("C:\\")
67+
expect(reviewDiffDirectory("/", "src/a.ts")).toBe("/src")
68+
expect(reviewDiffDirectory("C:\\", "src/a.ts")).toBe("C:\\src")
69+
})
70+
})

‎packages/app/src/pages/session/v2/review-diff-kinds.ts‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,24 @@ export function filterRenderableDiff(value: SnapshotFileDiff | VcsFileDiff): val
1212
return typeof value.file === "string"
1313
}
1414

15+
export function reviewDiffNeedsLoad(diff: RenderDiff) {
16+
if (diff.additions === 0 && diff.deletions === 0) return false
17+
return !diff.patch || !/^@@ /m.test(diff.patch)
18+
}
19+
20+
export function reviewRootDirectory(root: string) {
21+
return root === "/" || /^[A-Za-z]:[/\\]?$/.test(root) ? root : root.replace(/[/\\]+$/, "")
22+
}
23+
24+
export function reviewDiffDirectory(root: string, file: string) {
25+
const path = normalizePath(file)
26+
const index = path.lastIndexOf("/")
27+
const separator = root.includes("\\") ? "\\" : "/"
28+
const base = reviewRootDirectory(root)
29+
if (index < 0) return base
30+
return `${base.endsWith(separator) ? base : base + separator}${path.slice(0, index).replaceAll("/", separator)}`
31+
}
32+
1533
export function reviewDiffKinds(diffs: RenderDiff[]) {
1634
const merge = (a: Kind | undefined, b: Kind) => {
1735
if (!a) return b

‎packages/app/src/pages/session/v2/review-panel-v2.tsx‎

Lines changed: 24 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { createMemo, createSignal, Show, type JSX } from "solid-js"
1+
import { createMemo, createResource, createSignal, Show, type JSX } from "solid-js"
22
import type { SnapshotFileDiff, VcsFileDiff } from "@opencode-ai/sdk/v2"
33
import {
44
SESSION_REVIEW_V2_SIDEBAR_WIDTH_MAX,
@@ -25,6 +25,7 @@ import {
2525
filterRenderableDiff,
2626
filterReviewFiles,
2727
reviewDiffKinds,
28+
reviewDiffNeedsLoad,
2829
type RenderDiff,
2930
} from "@/pages/session/v2/review-diff-kinds"
3031
import type { ReviewPanelV2State } from "@/pages/session/v2/review-panel-v2-state"
@@ -37,6 +38,8 @@ export type ReviewPanelV2Props = {
3738
empty?: JSX.Element
3839
diffs: () => ReviewDiff[]
3940
diffsReady: () => boolean
41+
diffVersion?: number
42+
loadDiff?: (path: string, version?: number) => Promise<RenderDiff | undefined>
4043
activeFile?: string
4144
onSelectFile: (path: string) => void
4245
diffStyle: SessionReviewDiffStyle
@@ -74,7 +77,26 @@ export function ReviewPanelV2(props: ReviewPanelV2Props) {
7477
if (active && files.includes(active)) return active
7578
return files[0]
7679
})
77-
const activeItem = createMemo(() => diffs().find((diff) => diff.file === activeDiff()))
80+
const sourceActiveItem = createMemo(() => diffs().find((diff) => diff.file === activeDiff()))
81+
const detailSource = createMemo(() => {
82+
const diff = sourceActiveItem()
83+
const load = props.loadDiff
84+
if (!diff || !load || !reviewDiffNeedsLoad(diff)) return
85+
return { diff, load, version: props.diffVersion }
86+
})
87+
const [loadedDiff] = createResource(detailSource, async ({ diff, load, version }) => {
88+
const value = await load(diff.file, version)
89+
if (value?.file !== diff.file) return
90+
return { source: diff, version, value }
91+
})
92+
93+
const activeItem = createMemo(() => {
94+
const source = sourceActiveItem()
95+
if (loadedDiff.state !== "ready") return source
96+
const loaded = loadedDiff()
97+
if (loaded && loaded.source === source && loaded.version === props.diffVersion) return loaded.value
98+
return source
99+
})
78100

79101
const readFile = async (path: string) =>
80102
sdk()

0 commit comments

Comments
 (0)