mirror of
https://github.com/Kilo-Org/kilocode.git
synced 2026-08-28 19:11:03 +08:00
fix(vscode): stabilize inline diff scrolling
This commit is contained in:
@@ -0,0 +1,5 @@
|
||||
---
|
||||
"kilo-code": patch
|
||||
---
|
||||
|
||||
Keep the Agent Manager inline diff position stable while scrolling upward through large reviews
|
||||
@@ -24,6 +24,7 @@ export function Diff<T>(props: SSRDiffProps<T>) {
|
||||
"selectedLines",
|
||||
"commentedLines",
|
||||
"virtualized",
|
||||
"sizeKey",
|
||||
])
|
||||
const workerPool = useWorkerPool(props.diffStyle)
|
||||
|
||||
|
||||
@@ -23,6 +23,23 @@ const MIN_PLACEHOLDER_HEIGHT = 160
|
||||
const MAX_PLACEHOLDER_HEIGHT = 1200
|
||||
type Job = { run: () => void; cancelled: boolean }
|
||||
|
||||
const sizes = new WeakMap<object, Map<number, number>>()
|
||||
const WIDTH_LIMIT = 8
|
||||
|
||||
function remember(key: object | undefined, width: number, height: number) {
|
||||
if (!key || width <= 0 || height <= 0) return
|
||||
const widths = sizes.get(key) ?? new Map<number, number>()
|
||||
widths.delete(width)
|
||||
widths.set(width, height)
|
||||
if (widths.size > WIDTH_LIMIT) widths.delete(widths.keys().next().value!)
|
||||
sizes.set(key, widths)
|
||||
}
|
||||
|
||||
function reserved(key: object | undefined, width: number) {
|
||||
if (!key || width <= 0) return
|
||||
return sizes.get(key)?.get(width)
|
||||
}
|
||||
|
||||
// A review can contain many expanded diff components. Creating one
|
||||
// IntersectionObserver per diff showed up in profiles, so all deferred diffs
|
||||
// share a single observer and only register their element + render callback.
|
||||
@@ -173,6 +190,7 @@ export function Diff<T>(props: DiffProps<T>) {
|
||||
"commentedLines",
|
||||
"onRendered",
|
||||
"virtualized",
|
||||
"sizeKey",
|
||||
])
|
||||
|
||||
const mobile = createMediaQuery("(max-width: 640px)")
|
||||
@@ -247,7 +265,7 @@ export function Diff<T>(props: DiffProps<T>) {
|
||||
|
||||
createEffect(() => {
|
||||
if (visible()) return
|
||||
container.style.minHeight = `${estimate()}px`
|
||||
container.style.minHeight = `${reserved(local.sizeKey, container.clientWidth) ?? estimate()}px`
|
||||
})
|
||||
|
||||
createEffect(() => {
|
||||
@@ -266,6 +284,17 @@ export function Diff<T>(props: DiffProps<T>) {
|
||||
return root
|
||||
}
|
||||
|
||||
createEffect(() => {
|
||||
if (typeof ResizeObserver === "undefined") return
|
||||
const resize = new ResizeObserver(() => {
|
||||
const root = getRoot()
|
||||
if (!visible() || !current() || !root?.querySelector("[data-line]")) return
|
||||
remember(local.sizeKey, container.clientWidth, container.offsetHeight)
|
||||
})
|
||||
resize.observe(container)
|
||||
onCleanup(() => resize.disconnect())
|
||||
})
|
||||
|
||||
const applyScheme = () => {
|
||||
const host = container.querySelector("diffs-container")
|
||||
if (!(host instanceof HTMLElement)) return
|
||||
@@ -370,6 +399,7 @@ export function Diff<T>(props: DiffProps<T>) {
|
||||
if (token !== renderToken) return
|
||||
// Clear the height pin now that Pierre has rendered new content.
|
||||
container.style.minHeight = ""
|
||||
remember(local.sizeKey, container.clientWidth, container.offsetHeight)
|
||||
setSelectedLines(lastSelection)
|
||||
local.onRendered?.()
|
||||
})
|
||||
@@ -411,6 +441,7 @@ export function Diff<T>(props: DiffProps<T>) {
|
||||
if (typeof MutationObserver === "undefined") {
|
||||
container.style.minHeight = ""
|
||||
if (!root || !isReady(root)) return
|
||||
remember(local.sizeKey, container.clientWidth, container.offsetHeight)
|
||||
setSelectedLines(lastSelection)
|
||||
local.onRendered?.()
|
||||
return
|
||||
@@ -777,6 +808,7 @@ export function Diff<T>(props: DiffProps<T>) {
|
||||
if (!instance) return
|
||||
instance.setLineAnnotations(annotations ?? [])
|
||||
instance.rerender()
|
||||
notifyRendered()
|
||||
},
|
||||
{ defer: true },
|
||||
),
|
||||
|
||||
@@ -58,6 +58,9 @@ type DiffShared<T> = FileDiffOptions<T> & {
|
||||
// files so eager rendering does not expand full before/after content.
|
||||
// Defaults to virtualized.
|
||||
virtualized?: boolean
|
||||
// Stable rendered-content identity used to preserve deferred height when a
|
||||
// surrounding row virtualizer unmounts and later re-creates this diff.
|
||||
sizeKey?: object
|
||||
class?: string
|
||||
classList?: ComponentProps<"div">["classList"]
|
||||
}
|
||||
|
||||
@@ -2,11 +2,16 @@ import { expect, test, type Page } from "@playwright/test"
|
||||
|
||||
const GLOBALS = "colorScheme:dark;theme:kilo-vscode;vscodeTheme:dark-modern"
|
||||
const STORY_ID = "agentmanager--full-screen-diff-agent-edit-scroll"
|
||||
const INLINE_STORY_ID = "agentmanager--diff-panel-scroll-up"
|
||||
|
||||
function storyUrl() {
|
||||
return `/iframe.html?id=${STORY_ID}&viewMode=story&globals=${GLOBALS}`
|
||||
}
|
||||
|
||||
function inlineStoryUrl() {
|
||||
return `/iframe.html?id=${INLINE_STORY_ID}&viewMode=story&globals=${GLOBALS}`
|
||||
}
|
||||
|
||||
async function disableAnimations(page: Page) {
|
||||
await page.addStyleTag({
|
||||
content: `
|
||||
@@ -159,3 +164,60 @@ test("resets virtual measurements and scroll when the review context changes", a
|
||||
await expect.poll(async () => first.evaluate((el) => el.getBoundingClientRect().height)).toBe(1_200)
|
||||
await expect.poll(async () => scroller.evaluate((el) => el.scrollTop)).toBe(0)
|
||||
})
|
||||
|
||||
test("keeps the inline diff position stable while scrolling upward", async ({ page }) => {
|
||||
await page.setViewportSize({ width: 900, height: 760 })
|
||||
await page.goto(inlineStoryUrl(), { waitUntil: "load" })
|
||||
await disableAnimations(page)
|
||||
await page.waitForSelector(".am-diff-content diffs-container", { state: "attached" })
|
||||
|
||||
const result = await page.locator(".am-diff-content").evaluate(async (el) => {
|
||||
const frame = () => new Promise((resolve) => requestAnimationFrame(resolve))
|
||||
const settle = async (count: number) => {
|
||||
for (let i = 0; i < count; i++) await frame()
|
||||
}
|
||||
const seen = new Set(
|
||||
Array.from(el.querySelectorAll("[data-file-path]"), (row) => row.getAttribute("data-file-path")),
|
||||
)
|
||||
let remounts = 0
|
||||
const observer = new MutationObserver((records) => {
|
||||
for (const record of records) {
|
||||
for (const node of record.addedNodes) {
|
||||
if (!(node instanceof HTMLElement)) continue
|
||||
const rows = node.matches("[data-file-path]") ? [node] : Array.from(node.querySelectorAll("[data-file-path]"))
|
||||
for (const row of rows) {
|
||||
const file = row.getAttribute("data-file-path")
|
||||
if (seen.has(file)) remounts++
|
||||
seen.add(file)
|
||||
}
|
||||
}
|
||||
}
|
||||
})
|
||||
observer.observe(el, { childList: true, subtree: true })
|
||||
|
||||
// Materialize every row once, then start from the settled bottom. The bug
|
||||
// appears when upward scrolling re-creates rows above the viewport.
|
||||
while (el.scrollTop < el.scrollHeight - el.clientHeight - 1) {
|
||||
el.scrollTop = Math.min(el.scrollHeight - el.clientHeight, el.scrollTop + 120)
|
||||
await frame()
|
||||
}
|
||||
await settle(30)
|
||||
|
||||
let correction = 0
|
||||
let range = 0
|
||||
while (el.scrollTop > 0) {
|
||||
const height = el.scrollHeight
|
||||
const intended = Math.max(0, el.scrollTop - 80)
|
||||
el.scrollTop = intended
|
||||
await settle(2)
|
||||
correction = Math.max(correction, Math.abs(el.scrollTop - intended))
|
||||
range = Math.max(range, Math.abs(el.scrollHeight - height))
|
||||
}
|
||||
observer.disconnect()
|
||||
return { correction, range, remounts }
|
||||
})
|
||||
|
||||
expect(result.remounts).toBeGreaterThan(0)
|
||||
expect(result.correction).toBeLessThanOrEqual(1)
|
||||
expect(result.range).toBeLessThanOrEqual(1)
|
||||
})
|
||||
|
||||
@@ -1,5 +1,5 @@
|
||||
import { describe, expect, it } from "bun:test"
|
||||
import { mergeWorktreeDiffs } from "../../webview-ui/diff-viewer/diff-state"
|
||||
import { diffSizeKey, mergeWorktreeDiffs } from "../../webview-ui/diff-viewer/diff-state"
|
||||
import {
|
||||
EXTREME_DIFF_CHANGED_LINES,
|
||||
allOpenFiles,
|
||||
@@ -28,6 +28,18 @@ function diff(overrides: Partial<WorktreeFileDiff>): WorktreeFileDiff {
|
||||
}
|
||||
}
|
||||
|
||||
describe("diffSizeKey", () => {
|
||||
it("changes with rendered content, style, and review context", () => {
|
||||
const base = diff({ summarized: false, patch: "@@ -1 +1 @@\n-old\n+new\n" })
|
||||
const key = diffSizeKey("review-a", base, "unified")
|
||||
|
||||
expect(diffSizeKey("review-a", base, "unified")).toBe(key)
|
||||
expect(diffSizeKey("review-b", base, "unified")).not.toBe(key)
|
||||
expect(diffSizeKey("review-a", base, "split")).not.toBe(key)
|
||||
expect(diffSizeKey("review-a", { ...base, patch: "@@ -1 +1 @@\n-old\n+newer\n" }, "unified")).not.toBe(key)
|
||||
})
|
||||
})
|
||||
|
||||
describe("agent manager diff state", () => {
|
||||
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" })]
|
||||
|
||||
@@ -60,7 +60,7 @@ import { VirtualDiffList } from "../diff-viewer/VirtualDiffList"
|
||||
import { treeOrder } from "../diff-viewer/file-tree-utils"
|
||||
import { isMarkdownFile, MarkdownDiffView } from "../diff-viewer/MarkdownDiffView"
|
||||
import { ImageDiffView } from "../diff-viewer/ImageDiffView"
|
||||
import { createDiffRows } from "../diff-viewer/diff-state"
|
||||
import { createDiffRows, diffSizeKey } from "../diff-viewer/diff-state"
|
||||
import { createDiffRequests } from "../diff-viewer/diff-requests"
|
||||
|
||||
// --- Data model ---
|
||||
@@ -728,6 +728,7 @@ export const DiffPanel: Component<DiffPanelProps> = (props) => {
|
||||
after={{ name: diff.file, contents: diff.after }}
|
||||
patch={diff.patch}
|
||||
diffStyle={props.diffStyle ?? "unified"}
|
||||
sizeKey={diffSizeKey(props.sessionKey, diff, props.diffStyle ?? "unified")}
|
||||
virtualized={shouldVirtualizeDiff(diff)}
|
||||
annotations={annotationsForFile(diff.file)}
|
||||
renderAnnotation={buildAnnotation}
|
||||
|
||||
@@ -61,7 +61,7 @@ import { DiffEndMarker } from "./DiffEndMarker"
|
||||
import { VirtualDiffList } from "./VirtualDiffList"
|
||||
import { isMarkdownFile, MarkdownDiffView } from "./MarkdownDiffView"
|
||||
import { ImageDiffView } from "./ImageDiffView"
|
||||
import { createDiffRows } from "./diff-state"
|
||||
import { createDiffRows, diffSizeKey } from "./diff-state"
|
||||
import { createDiffRequests } from "./diff-requests"
|
||||
|
||||
type DiffStyle = "unified" | "split"
|
||||
@@ -802,6 +802,7 @@ export const FullScreenDiffView: Component<FullScreenDiffViewProps> = (props) =>
|
||||
after={{ name: diff.file, contents: diff.after }}
|
||||
patch={diff.patch}
|
||||
diffStyle={props.diffStyle}
|
||||
sizeKey={diffSizeKey(props.sessionKey, diff, props.diffStyle)}
|
||||
virtualized={shouldVirtualizeDiff(diff)}
|
||||
annotations={annotationsForFile(diff.file)}
|
||||
renderAnnotation={buildAnnotation}
|
||||
|
||||
@@ -1,6 +1,18 @@
|
||||
import { createMemo, createSignal } from "solid-js"
|
||||
import type { WorktreeFileDiff } from "../src/types/messages"
|
||||
|
||||
const sizeKeys = new WeakMap<
|
||||
WorktreeFileDiff,
|
||||
{
|
||||
context: string | undefined
|
||||
style: string
|
||||
patch: string | undefined
|
||||
before: string
|
||||
after: string
|
||||
key: object
|
||||
}
|
||||
>()
|
||||
|
||||
export function sameDiffMeta(left: WorktreeFileDiff, right: WorktreeFileDiff) {
|
||||
return (
|
||||
left.file === right.file &&
|
||||
@@ -20,6 +32,23 @@ export function diffToken(diff: WorktreeFileDiff) {
|
||||
return diff.stamp ?? parts.join(":")
|
||||
}
|
||||
|
||||
export function diffSizeKey(context: string | undefined, diff: WorktreeFileDiff, style: string) {
|
||||
const cached = sizeKeys.get(diff)
|
||||
if (
|
||||
cached &&
|
||||
cached.context === context &&
|
||||
cached.style === style &&
|
||||
cached.patch === diff.patch &&
|
||||
cached.before === diff.before &&
|
||||
cached.after === diff.after
|
||||
)
|
||||
return cached.key
|
||||
|
||||
const key = {}
|
||||
sizeKeys.set(diff, { context, style, patch: diff.patch, before: diff.before, after: diff.after, key })
|
||||
return key
|
||||
}
|
||||
|
||||
// Keep each rendered row mounted while live detail refreshes replace its data.
|
||||
// Otherwise Solid's keyed <For> remounts the row and deferred rendering swaps a
|
||||
// previously rendered diff above the viewport for a short placeholder.
|
||||
|
||||
@@ -87,13 +87,13 @@ const foldedDiffs: WorktreeFileDiff[] = [
|
||||
]
|
||||
|
||||
const ROWS = 140
|
||||
function edited(seed: string): WorktreeFileDiff {
|
||||
function edited(seed: string, file = "src/agent-edit.ts"): WorktreeFileDiff {
|
||||
const before = Array.from({ length: ROWS }, (_, i) => `const row${i} = "${seed}-old-${i}"\n`).join("")
|
||||
const after = Array.from({ length: ROWS }, (_, i) => `const row${i} = "${seed}-new-${i}"\n`).join("")
|
||||
const patch = [
|
||||
"diff --git a/src/agent-edit.ts b/src/agent-edit.ts",
|
||||
"--- a/src/agent-edit.ts",
|
||||
"+++ b/src/agent-edit.ts",
|
||||
`diff --git a/${file} b/${file}`,
|
||||
`--- a/${file}`,
|
||||
`+++ b/${file}`,
|
||||
`@@ -1,${ROWS} +1,${ROWS} @@`,
|
||||
...before
|
||||
.trimEnd()
|
||||
@@ -107,7 +107,7 @@ function edited(seed: string): WorktreeFileDiff {
|
||||
].join("\n")
|
||||
|
||||
return {
|
||||
file: "src/agent-edit.ts",
|
||||
file,
|
||||
status: "modified",
|
||||
additions: ROWS,
|
||||
deletions: ROWS,
|
||||
@@ -322,6 +322,29 @@ export const DiffPanelWithDiffs: Story = {
|
||||
),
|
||||
}
|
||||
|
||||
export const DiffPanelScrollUp: Story = {
|
||||
name: "DiffPanel - scroll upward through large diffs",
|
||||
render: () => {
|
||||
const diffs = Array.from({ length: 5 }, (_, i) => edited(`review-${i}`, `src/review-${i}.ts`))
|
||||
return (
|
||||
<StoryProviders noPadding>
|
||||
<div style={{ height: "700px", display: "flex", "flex-direction": "column" }}>
|
||||
<DiffPanel
|
||||
diffs={diffs}
|
||||
loading={false}
|
||||
sessionKey="inline-scroll-up"
|
||||
diffStyle="unified"
|
||||
onDiffStyleChange={() => {}}
|
||||
comments={[]}
|
||||
onCommentsChange={() => {}}
|
||||
onClose={() => {}}
|
||||
/>
|
||||
</div>
|
||||
</StoryProviders>
|
||||
)
|
||||
},
|
||||
}
|
||||
|
||||
const buttonFixtureStyle: JSX.CSSProperties = {
|
||||
display: "inline-flex",
|
||||
"align-items": "center",
|
||||
|
||||
Reference in New Issue
Block a user