mirror of
https://github.com/Kilo-Org/kilocode.git
synced 2026-08-31 01:37:28 +08:00
fix(vscode): render review diffs from hunk patches
Eager review rendering must not rebuild complete before/after contents when a tiny patch occurs inside a large source file. Preserve existing unified patches through Changes sources, parse them behind the deferred visibility gate, and use eager rendering only for patch-backed detail so gray placeholders clear without regressing session switching.
This commit is contained in:
@@ -1,5 +1,6 @@
|
||||
---
|
||||
"kilo-code": patch
|
||||
"@kilocode/kilo-ui": patch
|
||||
---
|
||||
|
||||
Fix laggy scrolling and gray flashes in the diff and Changes views. Syntax highlighting now runs in a web worker instead of blocking the main thread, and normal-sized review files render their diffs up front instead of re-rendering while scrolling. Scrolling large diffs stays smooth even when scrolling fast.
|
||||
Reduce lag and gray placeholders in the diff and Changes views by enabling worker-backed highlighting and rendering patch-backed review hunks without rebuilding full source files.
|
||||
|
||||
@@ -1,4 +1,4 @@
|
||||
import { DIFFS_TAG_NAME, FileDiff, type SelectedLineRange, VirtualizedFileDiff } from "@pierre/diffs"
|
||||
import { DIFFS_TAG_NAME, FileDiff, processFile, type SelectedLineRange, VirtualizedFileDiff } from "@pierre/diffs"
|
||||
import { PreloadMultiFileDiffResult } from "@pierre/diffs/ssr"
|
||||
import { createEffect, onCleanup, onMount, Show, splitProps } from "solid-js"
|
||||
import { Dynamic, isServer } from "solid-js/web"
|
||||
@@ -16,6 +16,8 @@ export function Diff<T>(props: SSRDiffProps<T>) {
|
||||
const [local, others] = splitProps(props, [
|
||||
"before",
|
||||
"after",
|
||||
"patch",
|
||||
"fileDiff",
|
||||
"class",
|
||||
"classList",
|
||||
"annotations",
|
||||
@@ -251,9 +253,12 @@ export function Diff<T>(props: SSRDiffProps<T>) {
|
||||
)
|
||||
// @ts-expect-error - fileContainer is private but needed for SSR hydration
|
||||
fileDiffInstance.fileContainer = fileDiffRef
|
||||
const patch = "patch" in local && typeof local.patch === "string" ? local.patch : ""
|
||||
const metadata = local.fileDiff ?? (patch ? processFile(patch, { cacheKey: patch }) : undefined)
|
||||
fileDiffInstance.hydrate({
|
||||
oldFile: local.before,
|
||||
newFile: local.after,
|
||||
oldFile: metadata ? undefined : local.before,
|
||||
newFile: metadata ? undefined : local.after,
|
||||
fileDiff: metadata,
|
||||
lineAnnotations: local.annotations,
|
||||
fileContainer: fileDiffRef,
|
||||
containerWrapper: container,
|
||||
|
||||
@@ -1,5 +1,12 @@
|
||||
import { sampledChecksum } from "@opencode-ai/core/util/encode"
|
||||
import { FileDiff, type FileDiffOptions, type SelectedLineRange, VirtualizedFileDiff } from "@pierre/diffs"
|
||||
import {
|
||||
FileDiff,
|
||||
type FileDiffMetadata,
|
||||
type FileDiffOptions,
|
||||
processFile,
|
||||
type SelectedLineRange,
|
||||
VirtualizedFileDiff,
|
||||
} from "@pierre/diffs"
|
||||
import { createMediaQuery } from "@solid-primitives/media"
|
||||
import { createEffect, createMemo, createSignal, on, onCleanup, splitProps, untrack } from "solid-js"
|
||||
import { createDefaultOptions, type DiffProps, styleVariables } from "../pierre"
|
||||
@@ -142,6 +149,7 @@ export function Diff<T>(props: DiffProps<T>) {
|
||||
let container!: HTMLDivElement
|
||||
let observer: MutationObserver | undefined
|
||||
let sharedVirtualizer: NonNullable<ReturnType<typeof acquireVirtualizer>> | undefined
|
||||
let parsed: { patch: string; diff: FileDiffMetadata } | undefined
|
||||
let renderToken = 0
|
||||
let selectionFrame: number | undefined
|
||||
let dragFrame: number | undefined
|
||||
@@ -156,6 +164,7 @@ export function Diff<T>(props: DiffProps<T>) {
|
||||
const [local, others] = splitProps(props, [
|
||||
"before",
|
||||
"after",
|
||||
"patch",
|
||||
"fileDiff",
|
||||
"class",
|
||||
"classList",
|
||||
@@ -179,11 +188,24 @@ export function Diff<T>(props: DiffProps<T>) {
|
||||
})
|
||||
|
||||
const estimate = createMemo(() => {
|
||||
const value = Math.max(lines(before()), lines(after())) * ESTIMATED_LINE_HEIGHT
|
||||
// A tracked detail response already carries a hunk-bounded git patch. Base
|
||||
// placeholder height on that patch instead of the full source file so a
|
||||
// tiny change in a large file does not reserve a large gray body.
|
||||
const patch = "patch" in local && typeof local.patch === "string" ? local.patch : ""
|
||||
const value = (patch ? lines(patch) : Math.max(lines(before()), lines(after()))) * ESTIMATED_LINE_HEIGHT
|
||||
if (value === 0) return MIN_PLACEHOLDER_HEIGHT
|
||||
return Math.max(MIN_PLACEHOLDER_HEIGHT, Math.min(value, MAX_PLACEHOLDER_HEIGHT))
|
||||
})
|
||||
|
||||
const patchDiff = () => {
|
||||
if (!("patch" in local) || typeof local.patch !== "string" || local.patch.length === 0) return
|
||||
if (parsed?.patch === local.patch) return parsed.diff
|
||||
const diff = processFile(local.patch, { cacheKey: local.patch })
|
||||
if (!diff) return
|
||||
parsed = { patch: local.patch, diff }
|
||||
return diff
|
||||
}
|
||||
|
||||
const large = createMemo(() => {
|
||||
return Math.max(before().length, after().length) > 500_000
|
||||
})
|
||||
@@ -682,16 +704,19 @@ export function Diff<T>(props: DiffProps<T>) {
|
||||
|
||||
const opts = options()
|
||||
const workerPool = large() ? getWorkerPool("unified") : getWorkerPool(props.diffStyle)
|
||||
// Eager (non-virtualized) diffs render once and never re-render on scroll or
|
||||
// height changes, avoiding Pierre's re-render-all storms. Highlighting still
|
||||
// runs in the worker, so the one-time row build stays cheap. Large diffs keep
|
||||
// virtualizing to bound DOM size.
|
||||
// Eager (non-virtualized) patch-backed diffs render their visible hunks once
|
||||
// and never re-render on scroll or height changes, avoiding Pierre's
|
||||
// re-render-all storms. Full-content or oversized diffs keep virtualizing.
|
||||
const virtualizer = local.virtualized === false ? undefined : getVirtualizer()
|
||||
if (local.virtualized === false && sharedVirtualizer) {
|
||||
sharedVirtualizer.release()
|
||||
sharedVirtualizer = undefined
|
||||
}
|
||||
const annotations = untrack(() => local.annotations)
|
||||
// Parse hunk-bounded patches only after the deferred visibility gate. This
|
||||
// preserves quick session switching while avoiding a full before/after diff
|
||||
// reconstruction for tiny changes inside large source files.
|
||||
const metadata = local.fileDiff ?? patchDiff()
|
||||
|
||||
// Preserve container height during re-render to prevent scroll jumps.
|
||||
// When Pierre tears down the DOM (innerHTML = ""), the container collapses
|
||||
@@ -708,9 +733,9 @@ export function Diff<T>(props: DiffProps<T>) {
|
||||
|
||||
container.innerHTML = ""
|
||||
|
||||
if (local.fileDiff) {
|
||||
if (metadata) {
|
||||
instance.render({
|
||||
fileDiff: local.fileDiff,
|
||||
fileDiff: metadata,
|
||||
lineAnnotations: annotations,
|
||||
containerWrapper: container,
|
||||
})
|
||||
|
||||
@@ -48,9 +48,9 @@ type DiffShared<T> = FileDiffOptions<T> & {
|
||||
commentedLines?: SelectedLineRange[]
|
||||
onLineNumberSelectionEnd?: (selection: SelectedLineRange | null) => void
|
||||
onRendered?: () => void
|
||||
// When false, render the whole diff up front instead of row-virtualizing it.
|
||||
// With highlighting offloaded to a worker, eager diffs render their rows once
|
||||
// and never re-render on scroll, so fast scrolling never shows gap buffers.
|
||||
// When false, render the supplied diff once instead of row-virtualizing it.
|
||||
// Callers should supply hunk-bounded `fileDiff`/`patch` data for large source
|
||||
// files so eager rendering does not expand full before/after content.
|
||||
// Defaults to virtualized.
|
||||
virtualized?: boolean
|
||||
class?: string
|
||||
@@ -60,6 +60,8 @@ type DiffShared<T> = FileDiffOptions<T> & {
|
||||
type DiffPair<T> = DiffShared<T> & {
|
||||
before: FileContents
|
||||
after: FileContents
|
||||
/** Unified patch used to parse only rendered hunks instead of full file contents. */
|
||||
patch?: string
|
||||
fileDiff?: undefined
|
||||
}
|
||||
|
||||
@@ -67,6 +69,7 @@ type DiffPatch<T> = DiffShared<T> & {
|
||||
fileDiff: FileDiffMetadata
|
||||
before?: undefined
|
||||
after?: undefined
|
||||
patch?: undefined
|
||||
}
|
||||
|
||||
export type DiffProps<T = {}> = DiffPair<T> | DiffPatch<T>
|
||||
|
||||
@@ -71,6 +71,7 @@ export function toSessionDiffFile(raw: SnapshotFileDiff): DiffFile {
|
||||
file: raw.file,
|
||||
before: view ? text(view, "deletions") : "",
|
||||
after: view ? text(view, "additions") : "",
|
||||
patch: raw.patch,
|
||||
additions: raw.additions,
|
||||
deletions: raw.deletions,
|
||||
status: raw.status,
|
||||
|
||||
@@ -90,11 +90,17 @@ export function createStagedDiffSource(): DiffSource {
|
||||
// For added: HEAD has no blob. For deleted: index has no blob.
|
||||
const before = entry.status === "added" ? "" : await showBlob(git, dir, "HEAD", file)
|
||||
const after = entry.status === "deleted" ? "" : await showBlob(git, dir, INDEX_REF, file)
|
||||
const result = await git.execGit(
|
||||
["-c", "core.quotepath=false", "diff", "--cached", "--no-ext-diff", "--no-renames", "HEAD", "--", file],
|
||||
dir,
|
||||
)
|
||||
const patch = result.code === 0 ? result.stdout : undefined
|
||||
const summarized = before === "" && after === "" && entry.status === "modified"
|
||||
return {
|
||||
file,
|
||||
before,
|
||||
after,
|
||||
patch,
|
||||
additions: entry.additions,
|
||||
deletions: entry.deletions,
|
||||
status: entry.status,
|
||||
|
||||
@@ -130,6 +130,10 @@ export function createUnstagedDiffSource(): DiffSource {
|
||||
// after = disk content (or "" for deleted).
|
||||
const before = !entry.tracked || entry.status === "added" ? "" : await showBlob(git, dir, INDEX_REF, file)
|
||||
const after = entry.status === "deleted" ? "" : await readDisk(dir, file)
|
||||
const result = entry.tracked
|
||||
? await git.execGit(["-c", "core.quotepath=false", "diff", "--no-ext-diff", "--no-renames", "--", file], dir)
|
||||
: undefined
|
||||
const patch = result?.code === 0 ? result.stdout : undefined
|
||||
const summarized = before === "" && after === "" && entry.status === "modified"
|
||||
|
||||
// For untracked added files numstat doesn't return counts, so backfill
|
||||
@@ -139,6 +143,7 @@ export function createUnstagedDiffSource(): DiffSource {
|
||||
file,
|
||||
before,
|
||||
after,
|
||||
patch,
|
||||
additions,
|
||||
deletions: entry.deletions,
|
||||
status: entry.status,
|
||||
|
||||
@@ -139,15 +139,16 @@ async function resolveOverrideRef(
|
||||
|
||||
/**
|
||||
* Project a `WorktreeDiffEntry` from `local-diff.ts` onto the `DiffFile` shape
|
||||
* expected by the diff viewer. Drops `patch` (the webview rebuilds before/after
|
||||
* for itself) and coerces optional `before`/`after` to empty strings when the
|
||||
* entry is summarized.
|
||||
* expected by the diff viewer. Preserve its hunk-bounded `patch` so Pierre can
|
||||
* parse the git diff directly rather than recomputing a diff from full source
|
||||
* contents; summarized entries still coerce optional content to empty strings.
|
||||
*/
|
||||
function toDiffFile(entry: WorktreeDiffEntry): DiffFile {
|
||||
return {
|
||||
file: entry.file,
|
||||
before: entry.before ?? "",
|
||||
after: entry.after ?? "",
|
||||
patch: entry.patch,
|
||||
additions: entry.additions,
|
||||
deletions: entry.deletions,
|
||||
status: entry.status,
|
||||
|
||||
@@ -17,6 +17,8 @@ export interface DiffFile {
|
||||
file: string
|
||||
before: string
|
||||
after: string
|
||||
/** Hunk-bounded unified patch used by Pierre to avoid re-diffing full files. */
|
||||
patch?: string
|
||||
additions: number
|
||||
deletions: number
|
||||
status?: "added" | "deleted" | "modified"
|
||||
|
||||
@@ -28,16 +28,27 @@ function diff(overrides: Partial<WorktreeFileDiff>): WorktreeFileDiff {
|
||||
}
|
||||
|
||||
describe("agent manager diff state", () => {
|
||||
it("preserves loaded detail when summary metadata is unchanged", () => {
|
||||
const prev = [diff({ summarized: false, before: "old\n", after: "new\n" })]
|
||||
it("preserves loaded detail and patch when summary metadata is unchanged", () => {
|
||||
const prev = [diff({ summarized: false, before: "old\n", after: "new\n", patch: "@@ -1 +1 @@\n-old\n+new\n" })]
|
||||
const next = [diff({ summarized: true })]
|
||||
const result = mergeWorktreeDiffs(prev, next)
|
||||
|
||||
expect(result.diffs).toEqual([diff({ summarized: false, before: "old\n", after: "new\n" })])
|
||||
expect(result.diffs).toEqual([
|
||||
diff({ summarized: false, before: "old\n", after: "new\n", patch: "@@ -1 +1 @@\n-old\n+new\n" }),
|
||||
])
|
||||
expect(result.diffs[0]).toBe(prev[0])
|
||||
expect(result.stale.size).toBe(0)
|
||||
})
|
||||
|
||||
it("replaces detailed content when patch anchors change", () => {
|
||||
const prev = [diff({ summarized: false, before: "old\n", after: "new\n", patch: "@@ -1 +1 @@\n-old\n+new\n" })]
|
||||
const next = [diff({ summarized: false, before: "old\n", after: "new\n", patch: "@@ -100 +100 @@\n-old\n+new\n" })]
|
||||
const result = mergeWorktreeDiffs(prev, next)
|
||||
|
||||
expect(result.diffs[0]).toBe(next[0])
|
||||
expect(result.diffs[0]?.patch).toContain("@@ -100 +100 @@")
|
||||
})
|
||||
|
||||
it("preserves cached content and marks stale when summary metadata changes", () => {
|
||||
const prev = [diff({ summarized: false, before: "old\n", after: "new\n", additions: 1 })]
|
||||
const next = [diff({ summarized: true, additions: 2 })]
|
||||
@@ -101,18 +112,23 @@ describe("agent manager diff state", () => {
|
||||
})
|
||||
|
||||
describe("eager diff files", () => {
|
||||
it("renders normal files eagerly", () => {
|
||||
it("renders hunk-bounded detailed patches eagerly", () => {
|
||||
const diffs = [
|
||||
diff({ file: "src/a.ts", additions: 10, deletions: 5 }),
|
||||
diff({ file: "src/b.ts", additions: 3, deletions: 0 }),
|
||||
diff({ file: "src/a.ts", patch: "@@ -1 +1 @@\n-a\n+b\n", additions: 10, deletions: 5 }),
|
||||
diff({ file: "src/b.ts", patch: "@@ -1 +1 @@\n-a\n+b\n", additions: 3, deletions: 0 }),
|
||||
]
|
||||
expect(eagerDiffFiles(diffs)).toEqual(new Set(["src/a.ts", "src/b.ts"]))
|
||||
})
|
||||
|
||||
it("virtualizes a full-content detail without a hunk-bounded patch", () => {
|
||||
const diffs = [diff({ file: "src/large-source.ts", before: "a\n".repeat(4000), after: "b\n", additions: 1 })]
|
||||
expect(eagerDiffFiles(diffs)).toEqual(new Set())
|
||||
})
|
||||
|
||||
it("virtualizes files larger than the large-file threshold", () => {
|
||||
const diffs = [
|
||||
diff({ file: "src/big.ts", additions: EXTREME_DIFF_CHANGED_LINES + 1, deletions: 0 }),
|
||||
diff({ file: "src/small.ts", additions: 5, deletions: 0 }),
|
||||
diff({ file: "src/big.ts", patch: "large", additions: EXTREME_DIFF_CHANGED_LINES + 1, deletions: 0 }),
|
||||
diff({ file: "src/small.ts", patch: "small", additions: 5, deletions: 0 }),
|
||||
]
|
||||
expect(eagerDiffFiles(diffs)).toEqual(new Set(["src/small.ts"]))
|
||||
})
|
||||
@@ -121,12 +137,12 @@ describe("eager diff files", () => {
|
||||
// Each file is under the large-file threshold, but together they exceed the
|
||||
// aggregate budget, so the overflow falls back to virtualization.
|
||||
const diffs = [
|
||||
diff({ file: "src/a.ts", additions: 2000, deletions: 0 }),
|
||||
diff({ file: "src/b.ts", additions: 2000, deletions: 0 }),
|
||||
diff({ file: "src/c.ts", additions: 2000, deletions: 0 }),
|
||||
diff({ file: "src/d.ts", additions: EAGER_DIFF_REVIEW_LINES - 6005, deletions: 0 }),
|
||||
diff({ file: "src/e.ts", additions: 2000, deletions: 0 }),
|
||||
diff({ file: "src/f.ts", additions: 5, deletions: 0 }),
|
||||
diff({ file: "src/a.ts", patch: "a", additions: 2000, deletions: 0 }),
|
||||
diff({ file: "src/b.ts", patch: "b", additions: 2000, deletions: 0 }),
|
||||
diff({ file: "src/c.ts", patch: "c", additions: 2000, deletions: 0 }),
|
||||
diff({ file: "src/d.ts", patch: "d", additions: EAGER_DIFF_REVIEW_LINES - 6005, deletions: 0 }),
|
||||
diff({ file: "src/e.ts", patch: "e", additions: 2000, deletions: 0 }),
|
||||
diff({ file: "src/f.ts", patch: "f", additions: 5, deletions: 0 }),
|
||||
]
|
||||
const eager = eagerDiffFiles(diffs)
|
||||
expect(eager.has("src/a.ts")).toBe(true)
|
||||
|
||||
@@ -69,6 +69,7 @@ describe("createSessionDiffSource.fetch", () => {
|
||||
expect(foo.file).toBe("foo.ts")
|
||||
expect(foo.before).toBe("keep\nold\n")
|
||||
expect(foo.after).toBe("keep\nnew\n")
|
||||
expect(foo.patch).toBe(modifiedPatch)
|
||||
expect(foo.additions).toBe(1)
|
||||
expect(foo.deletions).toBe(1)
|
||||
expect(foo.status).toBe("modified")
|
||||
|
||||
@@ -613,6 +613,7 @@ export const DiffPanel: Component<DiffPanelProps> = (props) => {
|
||||
<Diff<AnnotationMeta>
|
||||
before={{ name: diff.file, contents: diff.before }}
|
||||
after={{ name: diff.file, contents: diff.after }}
|
||||
patch={diff.patch}
|
||||
diffStyle={props.diffStyle ?? "unified"}
|
||||
virtualized={!eager().has(diff.file)}
|
||||
annotations={annotationsForFile(diff.file)}
|
||||
|
||||
@@ -710,6 +710,7 @@ export const FullScreenDiffView: Component<FullScreenDiffViewProps> = (props) =>
|
||||
<Diff<AnnotationMeta>
|
||||
before={{ name: diff.file, contents: diff.before }}
|
||||
after={{ name: diff.file, contents: diff.after }}
|
||||
patch={diff.patch}
|
||||
diffStyle={props.diffStyle}
|
||||
virtualized={!eager().has(diff.file)}
|
||||
annotations={annotationsForFile(diff.file)}
|
||||
|
||||
@@ -11,14 +11,15 @@ export function isLargeDiffFile(diff: WorktreeFileDiff): boolean {
|
||||
return diff.additions + diff.deletions > EXTREME_DIFF_CHANGED_LINES
|
||||
}
|
||||
|
||||
// Files whose diffs should render eagerly (no row virtualization) so their rows
|
||||
// stay mounted and never re-render on scroll. Large files keep virtualizing, and
|
||||
// an aggregate budget keeps eager DOM bounded for very large reviews.
|
||||
// Files whose hunk-bounded patches should render eagerly (no row virtualization)
|
||||
// so their rows stay mounted and never re-render on scroll. Never eager-render a
|
||||
// detail without a patch: a tiny change in a very large source file would make
|
||||
// Pierre re-diff and render full before/after contents on the main thread.
|
||||
export function eagerDiffFiles(diffs: WorktreeFileDiff[]): Set<string> {
|
||||
const eager = new Set<string>()
|
||||
let used = 0
|
||||
for (const diff of diffs) {
|
||||
if (isLargeDiffFile(diff)) continue
|
||||
if (!diff.patch || isLargeDiffFile(diff)) continue
|
||||
const size = diff.additions + diff.deletions
|
||||
if (used + size > EAGER_DIFF_REVIEW_LINES) continue
|
||||
used += size
|
||||
|
||||
@@ -38,6 +38,7 @@ export function mergeWorktreeDiffs(prev: WorktreeFileDiff[], next: WorktreeFileD
|
||||
existing.file === diff.file &&
|
||||
existing.before === diff.before &&
|
||||
existing.after === diff.after &&
|
||||
existing.patch === diff.patch &&
|
||||
sameDiffMeta(existing, diff)
|
||||
)
|
||||
return existing
|
||||
@@ -45,8 +46,19 @@ export function mergeWorktreeDiffs(prev: WorktreeFileDiff[], next: WorktreeFileD
|
||||
if (!diff.summarized) return diff
|
||||
// Metadata matches — restore cached content as before.
|
||||
if (sameDiffMeta({ ...existing, summarized: true }, diff)) {
|
||||
const merged = { ...diff, before: existing.before, after: existing.after, summarized: false }
|
||||
if (existing.before === merged.before && existing.after === merged.after && sameDiffMeta(existing, merged))
|
||||
const merged = {
|
||||
...diff,
|
||||
before: existing.before,
|
||||
after: existing.after,
|
||||
patch: existing.patch,
|
||||
summarized: false,
|
||||
}
|
||||
if (
|
||||
existing.before === merged.before &&
|
||||
existing.after === merged.after &&
|
||||
existing.patch === merged.patch &&
|
||||
sameDiffMeta(existing, merged)
|
||||
)
|
||||
return existing
|
||||
return merged
|
||||
}
|
||||
|
||||
@@ -9,9 +9,9 @@
|
||||
//
|
||||
// Here we instead load the worker from a real dist asset (`dist/shiki-worker.js`,
|
||||
// also produced by esbuild.js) using the webview URI the extension injects into
|
||||
// the page. Highlighting then runs off the main thread and scrolling stays
|
||||
// smooth. If the URI is missing (or the worker fails to spawn) Pierre falls back
|
||||
// to its main-thread highlighter, so behaviour is never worse than before.
|
||||
// the page. Pierre can offload highlighted updates to the pool after its initial
|
||||
// plain render. The diff wrapper still needs to keep that initial render cheap,
|
||||
// which is why review surfaces pass hunk-bounded patches instead of full files.
|
||||
import { WorkerPoolManager } from "@pierre/diffs/worker"
|
||||
|
||||
export type WorkerPoolStyle = "unified" | "split"
|
||||
|
||||
@@ -123,6 +123,8 @@ export interface WorktreeFileDiff {
|
||||
file: string
|
||||
before: string
|
||||
after: string
|
||||
/** Hunk-bounded unified patch used by Pierre to avoid re-diffing full files. */
|
||||
patch?: string
|
||||
additions: number
|
||||
deletions: number
|
||||
status?: "added" | "deleted" | "modified"
|
||||
|
||||
Reference in New Issue
Block a user