diff --git a/.changeset/calm-diff-scroll.md b/.changeset/calm-diff-scroll.md new file mode 100644 index 0000000000..f33b52455f --- /dev/null +++ b/.changeset/calm-diff-scroll.md @@ -0,0 +1,5 @@ +--- +"kilo-code": patch +--- + +Keep the Agent Manager inline diff position stable while scrolling upward through large reviews diff --git a/packages/kilo-ui/src/components/diff-ssr.tsx b/packages/kilo-ui/src/components/diff-ssr.tsx index a233514c87..087bf8e059 100644 --- a/packages/kilo-ui/src/components/diff-ssr.tsx +++ b/packages/kilo-ui/src/components/diff-ssr.tsx @@ -24,6 +24,7 @@ export function Diff(props: SSRDiffProps) { "selectedLines", "commentedLines", "virtualized", + "sizeKey", ]) const workerPool = useWorkerPool(props.diffStyle) diff --git a/packages/kilo-ui/src/components/diff.tsx b/packages/kilo-ui/src/components/diff.tsx index cee9fba7a4..fd503eee30 100644 --- a/packages/kilo-ui/src/components/diff.tsx +++ b/packages/kilo-ui/src/components/diff.tsx @@ -23,6 +23,23 @@ const MIN_PLACEHOLDER_HEIGHT = 160 const MAX_PLACEHOLDER_HEIGHT = 1200 type Job = { run: () => void; cancelled: boolean } +const sizes = new WeakMap>() +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() + 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(props: DiffProps) { "commentedLines", "onRendered", "virtualized", + "sizeKey", ]) const mobile = createMediaQuery("(max-width: 640px)") @@ -247,7 +265,7 @@ export function Diff(props: DiffProps) { 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(props: DiffProps) { 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(props: DiffProps) { 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(props: DiffProps) { 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(props: DiffProps) { if (!instance) return instance.setLineAnnotations(annotations ?? []) instance.rerender() + notifyRendered() }, { defer: true }, ), diff --git a/packages/kilo-ui/src/pierre/index.ts b/packages/kilo-ui/src/pierre/index.ts index 7b4b13aabe..988835e6a2 100644 --- a/packages/kilo-ui/src/pierre/index.ts +++ b/packages/kilo-ui/src/pierre/index.ts @@ -58,6 +58,9 @@ type DiffShared = FileDiffOptions & { // 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"] } diff --git a/packages/kilo-vscode/tests/diff-scroll-preservation.spec.ts b/packages/kilo-vscode/tests/diff-scroll-preservation.spec.ts index 735f6832d5..640449e0b0 100644 --- a/packages/kilo-vscode/tests/diff-scroll-preservation.spec.ts +++ b/packages/kilo-vscode/tests/diff-scroll-preservation.spec.ts @@ -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) +}) diff --git a/packages/kilo-vscode/tests/unit/agent-manager-diff-state.test.ts b/packages/kilo-vscode/tests/unit/agent-manager-diff-state.test.ts index 381ac518a3..4f836944df 100644 --- a/packages/kilo-vscode/tests/unit/agent-manager-diff-state.test.ts +++ b/packages/kilo-vscode/tests/unit/agent-manager-diff-state.test.ts @@ -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 { } } +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" })] diff --git a/packages/kilo-vscode/webview-ui/agent-manager/DiffPanel.tsx b/packages/kilo-vscode/webview-ui/agent-manager/DiffPanel.tsx index ae15fa3cf6..eff3750a77 100644 --- a/packages/kilo-vscode/webview-ui/agent-manager/DiffPanel.tsx +++ b/packages/kilo-vscode/webview-ui/agent-manager/DiffPanel.tsx @@ -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 = (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} diff --git a/packages/kilo-vscode/webview-ui/diff-viewer/FullScreenDiffView.tsx b/packages/kilo-vscode/webview-ui/diff-viewer/FullScreenDiffView.tsx index 6f708f8bd9..db2373c3e1 100644 --- a/packages/kilo-vscode/webview-ui/diff-viewer/FullScreenDiffView.tsx +++ b/packages/kilo-vscode/webview-ui/diff-viewer/FullScreenDiffView.tsx @@ -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 = (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} diff --git a/packages/kilo-vscode/webview-ui/diff-viewer/diff-state.ts b/packages/kilo-vscode/webview-ui/diff-viewer/diff-state.ts index 32ebe6db77..379266350b 100644 --- a/packages/kilo-vscode/webview-ui/diff-viewer/diff-state.ts +++ b/packages/kilo-vscode/webview-ui/diff-viewer/diff-state.ts @@ -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 remounts the row and deferred rendering swaps a // previously rendered diff above the viewport for a short placeholder. diff --git a/packages/kilo-vscode/webview-ui/src/stories/agent-manager.stories.tsx b/packages/kilo-vscode/webview-ui/src/stories/agent-manager.stories.tsx index 817310e1bb..8a44a8ca8f 100644 --- a/packages/kilo-vscode/webview-ui/src/stories/agent-manager.stories.tsx +++ b/packages/kilo-vscode/webview-ui/src/stories/agent-manager.stories.tsx @@ -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 ( + +
+ {}} + comments={[]} + onCommentsChange={() => {}} + onClose={() => {}} + /> +
+ + ) + }, +} + const buttonFixtureStyle: JSX.CSSProperties = { display: "inline-flex", "align-items": "center",