From c86aa40efd2d9b6f4bd33f0ced5d137646b96a4f Mon Sep 17 00:00:00 2001 From: Adam <2363879+adamdotdevin@users.noreply.github.com> Date: Wed, 11 Mar 2026 09:54:37 -0500 Subject: [PATCH 1/3] wip(app): review simplification --- packages/ui/src/components/file.tsx | 116 +----- .../components/session-review-search.test.ts | 39 -- .../src/components/session-review-search.ts | 59 --- packages/ui/src/components/session-review.tsx | 365 ------------------ packages/ui/src/pierre/file-find.ts | 180 ++------- 5 files changed, 49 insertions(+), 710 deletions(-) delete mode 100644 packages/ui/src/components/session-review-search.test.ts delete mode 100644 packages/ui/src/components/session-review-search.ts diff --git a/packages/ui/src/components/file.tsx b/packages/ui/src/components/file.tsx index f42fbb24d22..fa430e286d9 100644 --- a/packages/ui/src/components/file.tsx +++ b/packages/ui/src/components/file.tsx @@ -1,10 +1,8 @@ import { sampledChecksum } from "@opencode-ai/util/encode" import { DEFAULT_VIRTUAL_FILE_METRICS, - type ExpansionDirections, type DiffLineAnnotation, type FileContents, - type FileDiffMetadata, File as PierreFile, type FileDiffOptions, FileDiff, @@ -22,7 +20,7 @@ import { ComponentProps, createEffect, createMemo, createSignal, onCleanup, onMo import { createDefaultOptions, styleVariables } from "../pierre" import { markCommentedDiffLines, markCommentedFileLines } from "../pierre/commented-lines" import { fixDiffSelection, findDiffSide, type DiffSelectionSide } from "../pierre/diff-selection" -import { createFileFind, type FileFindReveal } from "../pierre/file-find" +import { createFileFind } from "../pierre/file-find" import { applyViewerScheme, clearReadyWatcher, @@ -65,21 +63,11 @@ type SharedProps = { search?: FileSearchControl } -export type FileSearchReveal = FileFindReveal - export type FileSearchHandle = { focus: () => void - setQuery: (value: string) => void - clear: () => void - reveal: (hit: FileSearchReveal) => boolean - expand: (hit: FileSearchReveal) => boolean - refresh: () => void } export type FileSearchControl = { - shortcuts?: "global" | "disabled" - showBar?: boolean - disableVirtualization?: boolean register: (handle: FileSearchHandle | null) => void } @@ -121,40 +109,6 @@ const sharedKeys = [ const textKeys = ["file", ...sharedKeys] as const const diffKeys = ["before", "after", ...sharedKeys] as const -function expansionForHit(diff: FileDiffMetadata, hit: FileSearchReveal) { - if (diff.isPartial || diff.hunks.length === 0) return - - const side = - hit.side === "deletions" - ? { - start: (hunk: FileDiffMetadata["hunks"][number]) => hunk.deletionStart, - count: (hunk: FileDiffMetadata["hunks"][number]) => hunk.deletionCount, - } - : { - start: (hunk: FileDiffMetadata["hunks"][number]) => hunk.additionStart, - count: (hunk: FileDiffMetadata["hunks"][number]) => hunk.additionCount, - } - - for (let i = 0; i < diff.hunks.length; i++) { - const hunk = diff.hunks[i] - const start = side.start(hunk) - if (hit.line < start) { - return { - index: i, - direction: i === 0 ? "down" : "both", - } satisfies { index: number; direction: ExpansionDirections } - } - - const end = start + Math.max(side.count(hunk) - 1, -1) - if (hit.line <= end) return - } - - return { - index: diff.hunks.length, - direction: "up", - } satisfies { index: number; direction: ExpansionDirections } -} - // --------------------------------------------------------------------------- // Shared viewer hook // --------------------------------------------------------------------------- @@ -167,7 +121,6 @@ type MouseHit = { type ViewerConfig = { enableLineSelection: () => boolean - search: () => FileSearchControl | undefined selectedLines: () => SelectedLineRange | null | undefined commentedLines: () => SelectedLineRange[] onLineSelectionEnd: (range: SelectedLineRange | null) => void @@ -207,7 +160,6 @@ function useFileViewer(config: ViewerConfig) { wrapper: () => wrapper, overlay: () => overlay, getRoot, - shortcuts: config.search()?.shortcuts, }) // -- selection scheduling -- @@ -407,14 +359,10 @@ function useFileViewer(config: ViewerConfig) { type Viewer = ReturnType -type ModeAdapter = Omit< - ViewerConfig, - "enableLineSelection" | "search" | "selectedLines" | "commentedLines" | "onLineSelectionEnd" -> +type ModeAdapter = Omit type ModeConfig = { enableLineSelection: () => boolean - search: () => FileSearchControl | undefined selectedLines: () => SelectedLineRange | null | undefined commentedLines: () => SelectedLineRange[] | undefined onLineSelectionEnd: (range: SelectedLineRange | null) => void @@ -437,7 +385,6 @@ type VirtualStrategy = { function useModeViewer(config: ModeConfig, adapter: ModeAdapter) { return useFileViewer({ enableLineSelection: config.enableLineSelection, - search: config.search, selectedLines: config.selectedLines, commentedLines: () => config.commentedLines() ?? [], onLineSelectionEnd: config.onLineSelectionEnd, @@ -448,32 +395,13 @@ function useModeViewer(config: ModeConfig, adapter: ModeAdapter) { function useSearchHandle(opts: { search: () => FileSearchControl | undefined find: ReturnType - expand?: (hit: FileSearchReveal) => boolean }) { createEffect(() => { const search = opts.search() if (!search) return const handle = { - focus: () => { - opts.find.focus() - }, - setQuery: (value: string) => { - opts.find.activate() - opts.find.setQuery(value, { scroll: false }) - }, - clear: () => { - opts.find.clear() - }, - reveal: (hit: FileSearchReveal) => { - opts.find.activate() - return opts.find.reveal(hit) - }, - expand: (hit: FileSearchReveal) => opts.expand?.(hit) ?? false, - refresh: () => { - opts.find.activate() - opts.find.refresh() - }, + focus: () => opts.find.focus(), } satisfies FileSearchHandle search.register(handle) @@ -606,7 +534,7 @@ function createLocalVirtualStrategy(host: () => HTMLDivElement | undefined, enab } } -function createSharedVirtualStrategy(host: () => HTMLDivElement | undefined, enabled: () => boolean): VirtualStrategy { +function createSharedVirtualStrategy(host: () => HTMLDivElement | undefined): VirtualStrategy { let shared: NonNullable> | undefined const release = () => { @@ -616,10 +544,6 @@ function createSharedVirtualStrategy(host: () => HTMLDivElement | undefined, ena return { get: () => { - if (!enabled()) { - release() - return - } if (shared) return shared.virtualizer const container = host() @@ -689,7 +613,6 @@ function diffSelectionSide(node: Node | null) { function ViewerShell(props: { mode: "text" | "diff" viewer: ReturnType - search: FileSearchControl | undefined class: string | undefined classList: ComponentProps<"div">["classList"] | undefined }) { @@ -708,7 +631,7 @@ function ViewerShell(props: { onPointerDown={props.viewer.find.onPointerDown} onFocus={props.viewer.find.onFocus} > - + (props: TextFileProps) { viewer = useModeViewer( { enableLineSelection: () => props.enableLineSelection === true, - search: () => local.search, selectedLines: () => local.selectedLines, commentedLines: () => local.commentedLines, onLineSelectionEnd: (range) => local.onLineSelectionEnd?.(range), @@ -941,9 +863,7 @@ function TextViewer(props: TextFileProps) { virtuals.cleanup() }) - return ( - - ) + return } // --------------------------------------------------------------------------- @@ -1029,7 +949,6 @@ function DiffViewer(props: DiffFileProps) { viewer = useModeViewer( { enableLineSelection: () => props.enableLineSelection === true, - search: () => local.search, selectedLines: () => local.selectedLines, commentedLines: () => local.commentedLines, onLineSelectionEnd: (range) => local.onLineSelectionEnd?.(range), @@ -1037,10 +956,7 @@ function DiffViewer(props: DiffFileProps) { adapter, ) - const virtuals = createSharedVirtualStrategy( - () => viewer.container, - () => local.search?.disableVirtualization !== true, - ) + const virtuals = createSharedVirtualStrategy(() => viewer.container) const large = createMemo(() => { const before = typeof local.before?.contents === "string" ? local.before.contents : "" @@ -1090,20 +1006,6 @@ function DiffViewer(props: DiffFileProps) { useSearchHandle({ search: () => local.search, find: viewer.find, - expand: (hit) => { - const active = instance as - | ((FileDiff | VirtualizedFileDiff) & { - fileDiff?: FileDiffMetadata - }) - | undefined - if (!active?.fileDiff) return false - - const next = expansionForHit(active.fileDiff, hit) - if (!next) return false - - active.expandHunk(next.index, next.direction) - return true - }, }) // -- render instance -- @@ -1158,9 +1060,7 @@ function DiffViewer(props: DiffFileProps) { dragEndSide = undefined }) - return ( - - ) + return } // --------------------------------------------------------------------------- diff --git a/packages/ui/src/components/session-review-search.test.ts b/packages/ui/src/components/session-review-search.test.ts deleted file mode 100644 index 060df64071e..00000000000 --- a/packages/ui/src/components/session-review-search.test.ts +++ /dev/null @@ -1,39 +0,0 @@ -import { describe, expect, test } from "bun:test" -import { buildSessionSearchHits, stepSessionSearchIndex } from "./session-review-search" - -describe("session review search", () => { - test("builds hits with line, col, and side", () => { - const hits = buildSessionSearchHits({ - query: "alpha", - files: [ - { - file: "a.txt", - before: "alpha\nbeta alpha", - after: "ALPHA", - }, - ], - }) - - expect(hits).toEqual([ - { file: "a.txt", side: "deletions", line: 1, col: 1, len: 5 }, - { file: "a.txt", side: "deletions", line: 2, col: 6, len: 5 }, - { file: "a.txt", side: "additions", line: 1, col: 1, len: 5 }, - ]) - }) - - test("uses non-overlapping matches", () => { - const hits = buildSessionSearchHits({ - query: "aa", - files: [{ file: "a.txt", after: "aaaa" }], - }) - - expect(hits.map((hit) => hit.col)).toEqual([1, 3]) - }) - - test("wraps next and previous navigation", () => { - expect(stepSessionSearchIndex(5, 0, -1)).toBe(4) - expect(stepSessionSearchIndex(5, 4, 1)).toBe(0) - expect(stepSessionSearchIndex(5, 2, 1)).toBe(3) - expect(stepSessionSearchIndex(0, 0, 1)).toBe(0) - }) -}) diff --git a/packages/ui/src/components/session-review-search.ts b/packages/ui/src/components/session-review-search.ts deleted file mode 100644 index 2cff0adc5a3..00000000000 --- a/packages/ui/src/components/session-review-search.ts +++ /dev/null @@ -1,59 +0,0 @@ -export type SessionSearchHit = { - file: string - side: "additions" | "deletions" - line: number - col: number - len: number -} - -type SessionSearchFile = { - file: string - before?: string - after?: string -} - -function hitsForSide(args: { file: string; side: SessionSearchHit["side"]; text: string; needle: string }) { - return args.text.split("\n").flatMap((line, i) => { - if (!line) return [] - - const hay = line.toLowerCase() - let at = hay.indexOf(args.needle) - if (at < 0) return [] - - const out: SessionSearchHit[] = [] - while (at >= 0) { - out.push({ - file: args.file, - side: args.side, - line: i + 1, - col: at + 1, - len: args.needle.length, - }) - at = hay.indexOf(args.needle, at + args.needle.length) - } - - return out - }) -} - -export function buildSessionSearchHits(args: { query: string; files: SessionSearchFile[] }) { - const value = args.query.trim().toLowerCase() - if (!value) return [] - - return args.files.flatMap((file) => { - const out: SessionSearchHit[] = [] - if (typeof file.before === "string") { - out.push(...hitsForSide({ file: file.file, side: "deletions", text: file.before, needle: value })) - } - if (typeof file.after === "string") { - out.push(...hitsForSide({ file: file.file, side: "additions", text: file.after, needle: value })) - } - return out - }) -} - -export function stepSessionSearchIndex(total: number, current: number, dir: 1 | -1) { - if (total <= 0) return 0 - if (current < 0 || current >= total) return dir > 0 ? 0 : total - 1 - return (current + dir + total) % total -} diff --git a/packages/ui/src/components/session-review.tsx b/packages/ui/src/components/session-review.tsx index 62c70e8647d..61920d9f3f5 100644 --- a/packages/ui/src/components/session-review.tsx +++ b/packages/ui/src/components/session-review.tsx @@ -9,9 +9,6 @@ import { IconButton } from "./icon-button" import { StickyAccordionHeader } from "./sticky-accordion-header" import { Tooltip } from "./tooltip" import { ScrollView } from "./scroll-view" -import { FileSearchBar } from "./file-search" -import type { FileSearchHandle } from "./file" -import { buildSessionSearchHits, stepSessionSearchIndex, type SessionSearchHit } from "./session-review-search" import { useFileComponent } from "../context/file" import { useI18n } from "../context/i18n" import { getDirectory, getFilename } from "@opencode-ai/util/path" @@ -135,15 +132,10 @@ type SessionReviewSelection = { export const SessionReview = (props: SessionReviewProps) => { let scroll: HTMLDivElement | undefined - let searchInput: HTMLInputElement | undefined let focusToken = 0 - let revealToken = 0 - let highlightedFile: string | undefined const i18n = useI18n() const fileComponent = useFileComponent() const anchors = new Map() - const searchHandles = new Map() - const readyFiles = new Set() const [store, setStore] = createStore<{ open: string[]; force: Record }>({ open: [], force: {}, @@ -152,18 +144,12 @@ export const SessionReview = (props: SessionReviewProps) => { const [selection, setSelection] = createSignal(null) const [commenting, setCommenting] = createSignal(null) const [opened, setOpened] = createSignal(null) - const [searchOpen, setSearchOpen] = createSignal(false) - const [searchQuery, setSearchQuery] = createSignal("") - const [searchActive, setSearchActive] = createSignal(0) - const [searchPos, setSearchPos] = createSignal({ top: 8, right: 8 }) const open = () => props.open ?? store.open const files = createMemo(() => props.diffs.map((d) => d.file)) const diffs = createMemo(() => new Map(props.diffs.map((d) => [d.file, d] as const))) const diffStyle = () => props.diffStyle ?? (props.split ? "split" : "unified") const hasDiffs = () => files().length > 0 - const searchValue = createMemo(() => searchQuery().trim()) - const searchExpanded = createMemo(() => searchValue().length > 0) const handleChange = (open: string[]) => { props.onOpenChange?.(open) @@ -176,266 +162,8 @@ export const SessionReview = (props: SessionReviewProps) => { handleChange(next) } - const clearViewerSearch = () => { - for (const handle of searchHandles.values()) handle.clear() - highlightedFile = undefined - } - const openFileLabel = () => i18n.t("ui.sessionReview.openFile") - const selectionLabel = (range: SelectedLineRange) => { - const start = Math.min(range.start, range.end) - const end = Math.max(range.start, range.end) - if (start === end) return i18n.t("ui.sessionReview.selection.line", { line: start }) - return i18n.t("ui.sessionReview.selection.lines", { start, end }) - } - - const focusSearch = () => { - if (!hasDiffs()) return - setSearchOpen(true) - requestAnimationFrame(() => { - searchInput?.focus() - searchInput?.select() - }) - } - - const closeSearch = () => { - revealToken++ - setSearchOpen(false) - setSearchQuery("") - setSearchActive(0) - clearViewerSearch() - } - - const positionSearchBar = () => { - if (typeof window === "undefined") return - if (!scroll) return - - const rect = scroll.getBoundingClientRect() - const title = parseFloat(getComputedStyle(scroll).getPropertyValue("--session-title-height")) - const header = Number.isNaN(title) ? 0 : title - setSearchPos({ - top: Math.round(rect.top) + header - 4, - right: Math.round(window.innerWidth - rect.right) + 8, - }) - } - - const searchHits = createMemo(() => - buildSessionSearchHits({ - query: searchQuery(), - files: props.diffs.flatMap((diff) => { - if (mediaKindFromPath(diff.file)) return [] - - return [ - { - file: diff.file, - before: typeof diff.before === "string" ? diff.before : undefined, - after: typeof diff.after === "string" ? diff.after : undefined, - }, - ] - }), - }), - ) - - const waitForViewer = (file: string, token: number) => - new Promise((resolve) => { - let attempt = 0 - - const tick = () => { - if (token !== revealToken) { - resolve(undefined) - return - } - - const handle = searchHandles.get(file) - if (handle && readyFiles.has(file)) { - resolve(handle) - return - } - - if (attempt >= 180) { - resolve(undefined) - return - } - - attempt++ - requestAnimationFrame(tick) - } - - tick() - }) - - const waitForFrames = (count: number, token: number) => - new Promise((resolve) => { - const tick = (left: number) => { - if (token !== revealToken) { - resolve(false) - return - } - - if (left <= 0) { - resolve(true) - return - } - - requestAnimationFrame(() => tick(left - 1)) - } - - tick(count) - }) - - const revealSearchHit = async (token: number, hit: SessionSearchHit, query: string) => { - const diff = diffs().get(hit.file) - if (!diff) return - - if (!open().includes(hit.file)) { - handleChange([...open(), hit.file]) - } - - if (!mediaKindFromPath(hit.file) && diff.additions + diff.deletions > MAX_DIFF_CHANGED_LINES) { - setStore("force", hit.file, true) - } - - const handle = await waitForViewer(hit.file, token) - if (!handle || token !== revealToken) return - if (searchValue() !== query) return - if (!(await waitForFrames(2, token))) return - - if (highlightedFile && highlightedFile !== hit.file) { - searchHandles.get(highlightedFile)?.clear() - highlightedFile = undefined - } - - anchors.get(hit.file)?.scrollIntoView({ block: "nearest" }) - - let done = false - for (let i = 0; i < 4; i++) { - if (token !== revealToken) return - if (searchValue() !== query) return - - handle.setQuery(query) - if (handle.reveal(hit)) { - done = true - break - } - - const expanded = handle.expand(hit) - handle.refresh() - if (!(await waitForFrames(expanded ? 2 : 1, token))) return - } - - if (!done) return - - if (!(await waitForFrames(1, token))) return - handle.reveal(hit) - - highlightedFile = hit.file - } - - const navigateSearch = (dir: 1 | -1) => { - const total = searchHits().length - if (total <= 0) return - setSearchActive((value) => stepSessionSearchIndex(total, value, dir)) - } - - const inReview = (node: unknown, path?: unknown[]) => { - if (node === searchInput) return true - if (path?.some((item) => item === scroll || item === searchInput)) return true - if (path?.some((item) => item instanceof HTMLElement && item.dataset.component === "session-review")) { - return true - } - if (!(node instanceof Node)) return false - if (searchInput?.contains(node)) return true - if (node instanceof HTMLElement && node.closest("[data-component='session-review']")) return true - if (!scroll) return false - return scroll.contains(node) - } - - createEffect(() => { - if (typeof window === "undefined") return - - const onKeyDown = (event: KeyboardEvent) => { - const mod = event.metaKey || event.ctrlKey - if (!mod) return - - const key = event.key.toLowerCase() - if (key !== "f" && key !== "g") return - - if (key === "f") { - if (!hasDiffs()) return - event.preventDefault() - event.stopPropagation() - focusSearch() - return - } - - const path = typeof event.composedPath === "function" ? event.composedPath() : undefined - if (!inReview(event.target, path) && !inReview(document.activeElement, path)) return - if (!searchOpen()) return - event.preventDefault() - event.stopPropagation() - navigateSearch(event.shiftKey ? -1 : 1) - } - - window.addEventListener("keydown", onKeyDown, { capture: true }) - onCleanup(() => window.removeEventListener("keydown", onKeyDown, { capture: true })) - }) - - createEffect(() => { - diffStyle() - searchExpanded() - readyFiles.clear() - }) - - createEffect(() => { - if (!searchOpen()) return - if (!scroll) return - - const root = scroll - - requestAnimationFrame(positionSearchBar) - window.addEventListener("resize", positionSearchBar, { passive: true }) - const observer = typeof ResizeObserver === "undefined" ? undefined : new ResizeObserver(positionSearchBar) - observer?.observe(root) - - onCleanup(() => { - window.removeEventListener("resize", positionSearchBar) - observer?.disconnect() - }) - }) - - createEffect(() => { - const total = searchHits().length - if (total === 0) { - if (searchActive() !== 0) setSearchActive(0) - return - } - - if (searchActive() >= total) setSearchActive(total - 1) - }) - - createEffect(() => { - diffStyle() - const query = searchValue() - const hits = searchHits() - const token = ++revealToken - if (!query || hits.length === 0) { - clearViewerSearch() - return - } - - const hit = hits[Math.min(searchActive(), hits.length - 1)] - if (!hit) return - void revealSearchHit(token, hit, query) - }) - - onCleanup(() => { - revealToken++ - clearViewerSearch() - readyFiles.clear() - searchHandles.clear() - }) - const selectionSide = (range: SelectedLineRange) => range.endSide ?? range.side ?? "additions" const selectionPreview = (diff: FileDiff, range: SelectedLineRange) => { @@ -499,58 +227,6 @@ export const SessionReview = (props: SessionReviewProps) => { }) }) - const handleReviewKeyDown = (event: KeyboardEvent) => { - if (event.defaultPrevented) return - - const mod = event.metaKey || event.ctrlKey - const key = event.key.toLowerCase() - const target = event.target - if (mod && key === "f") { - event.preventDefault() - event.stopPropagation() - focusSearch() - return - } - - if (mod && key === "g") { - if (!searchOpen()) return - event.preventDefault() - event.stopPropagation() - navigateSearch(event.shiftKey ? -1 : 1) - } - } - - const handleSearchInputKeyDown = (event: KeyboardEvent) => { - const mod = event.metaKey || event.ctrlKey - const key = event.key.toLowerCase() - - if (mod && key === "g") { - event.preventDefault() - event.stopPropagation() - navigateSearch(event.shiftKey ? -1 : 1) - return - } - - if (mod && key === "f") { - event.preventDefault() - event.stopPropagation() - focusSearch() - return - } - - if (event.key === "Escape") { - event.preventDefault() - event.stopPropagation() - closeSearch() - return - } - - if (event.key !== "Enter") return - event.preventDefault() - event.stopPropagation() - navigateSearch(event.shiftKey ? -1 : 1) - } - return (
@@ -594,31 +270,10 @@ export const SessionReview = (props: SessionReviewProps) => { props.scrollRef?.(el) }} onScroll={props.onScroll as any} - onKeyDown={handleReviewKeyDown} classList={{ [props.classes?.root ?? ""]: !!props.classes?.root, }} > - - (searchHits().length ? Math.min(searchActive(), searchHits().length - 1) : 0)} - count={() => searchHits().length} - setInput={(el) => { - searchInput = el - }} - onInput={(value) => { - setSearchQuery(value) - setSearchActive(0) - }} - onKeyDown={(event) => handleSearchInputKeyDown(event)} - onClose={closeSearch} - onPrev={() => navigateSearch(-1)} - onNext={() => navigateSearch(1)} - /> - -
@@ -720,9 +375,6 @@ export const SessionReview = (props: SessionReviewProps) => { onCleanup(() => { anchors.delete(file) - readyFiles.delete(file) - searchHandles.delete(file) - if (highlightedFile === file) highlightedFile = undefined }) const handleLineSelected = (range: SelectedLineRange | null) => { @@ -839,9 +491,7 @@ export const SessionReview = (props: SessionReviewProps) => { mode="diff" preloadedDiff={item().preloaded} diffStyle={diffStyle()} - expansionLineCount={searchExpanded() ? Number.MAX_SAFE_INTEGER : 20} onRendered={() => { - readyFiles.add(file) props.onDiffRendered?.() }} enableLineSelection={props.onLineComment != null} @@ -854,21 +504,6 @@ export const SessionReview = (props: SessionReviewProps) => { renderHoverUtility={props.onLineComment ? commentsUi.renderHoverUtility : undefined} selectedLines={selectedLines()} commentedLines={commentedLines()} - search={{ - shortcuts: "disabled", - showBar: false, - disableVirtualization: searchExpanded(), - register: (handle: FileSearchHandle | null) => { - if (!handle) { - searchHandles.delete(file) - readyFiles.delete(file) - if (highlightedFile === file) highlightedFile = undefined - return - } - - searchHandles.set(file, handle) - }, - }} before={{ name: file, contents: typeof item().before === "string" ? item().before : "", diff --git a/packages/ui/src/pierre/file-find.ts b/packages/ui/src/pierre/file-find.ts index 7d55cfa72b8..ee608152d5e 100644 --- a/packages/ui/src/pierre/file-find.ts +++ b/packages/ui/src/pierre/file-find.ts @@ -8,20 +8,6 @@ export type FindHost = { isOpen: () => boolean } -type FileFindSide = "additions" | "deletions" - -export type FileFindReveal = { - side: FileFindSide - line: number - col: number - len: number -} - -type FileFindHit = FileFindReveal & { - range: Range - alt?: number -} - const hosts = new Set() let target: FindHost | undefined let current: FindHost | undefined @@ -112,7 +98,6 @@ type CreateFileFindOptions = { wrapper: () => HTMLElement | undefined overlay: () => HTMLDivElement | undefined getRoot: () => ShadowRoot | undefined - shortcuts?: "global" | "disabled" } export function createFileFind(opts: CreateFileFindOptions) { @@ -120,7 +105,7 @@ export function createFileFind(opts: CreateFileFindOptions) { let overlayFrame: number | undefined let overlayScroll: HTMLElement[] = [] let mode: "highlights" | "overlay" = "overlay" - let hits: FileFindHit[] = [] + let hits: Range[] = [] const [open, setOpen] = createSignal(false) const [query, setQuery] = createSignal("") @@ -161,7 +146,7 @@ export function createFileFind(opts: CreateFileFindOptions) { const frag = document.createDocumentFragment() for (let i = 0; i < hits.length; i++) { - const range = hits[i].range + const range = hits[i] const active = i === currentIndex for (const rect of Array.from(range.getClientRects())) { if (!rect.width || !rect.height) continue @@ -237,7 +222,7 @@ export function createFileFind(opts: CreateFileFindOptions) { const scan = (root: ShadowRoot, value: string) => { const needle = value.toLowerCase() - const ranges: FileFindHit[] = [] + const ranges: Range[] = [] const cols = Array.from(root.querySelectorAll("[data-content] [data-line], [data-column-content]")).filter( (node): node is HTMLElement => node instanceof HTMLElement, ) @@ -250,28 +235,6 @@ export function createFileFind(opts: CreateFileFindOptions) { let at = hay.indexOf(needle) if (at === -1) continue - const row = col.closest("[data-line], [data-alt-line]") - if (!(row instanceof HTMLElement)) continue - - const primary = parseInt(row.dataset.line ?? "", 10) - const alt = parseInt(row.dataset.altLine ?? "", 10) - const line = (() => { - if (!Number.isNaN(primary)) return primary - if (!Number.isNaN(alt)) return alt - })() - if (line === undefined) continue - - const side = (() => { - const code = col.closest("[data-code]") - if (code instanceof HTMLElement) return code.hasAttribute("data-deletions") ? "deletions" : "additions" - - const row = col.closest("[data-line-type]") - if (!(row instanceof HTMLElement)) return "additions" - const type = row.dataset.lineType - if (type === "change-deletion") return "deletions" - return "additions" - })() as FileFindSide - const nodes: Text[] = [] const ends: number[] = [] const walker = document.createTreeWalker(col, NodeFilter.SHOW_TEXT) @@ -305,14 +268,7 @@ export function createFileFind(opts: CreateFileFindOptions) { const range = document.createRange() range.setStart(start.node, start.offset) range.setEnd(end.node, end.offset) - ranges.push({ - range, - side, - line, - alt: Number.isNaN(alt) ? undefined : alt, - col: at + 1, - len: value.length, - }) + ranges.push(range) at = hay.indexOf(needle, at + value.length) } } @@ -321,17 +277,12 @@ export function createFileFind(opts: CreateFileFindOptions) { } const scrollToRange = (range: Range) => { - const scroll = () => { - const start = range.startContainer - const el = start instanceof Element ? start : start.parentElement - el?.scrollIntoView({ block: "center", inline: "center" }) - } - - scroll() - requestAnimationFrame(scroll) + const start = range.startContainer + const el = start instanceof Element ? start : start.parentElement + el?.scrollIntoView({ block: "center", inline: "center" }) } - const setHighlights = (ranges: FileFindHit[], currentIndex: number) => { + const setHighlights = (ranges: Range[], currentIndex: number) => { const api = (globalThis as unknown as { CSS?: { highlights?: any }; Highlight?: any }).CSS?.highlights const Highlight = (globalThis as unknown as { Highlight?: any }).Highlight if (!api || typeof Highlight !== "function") return false @@ -339,37 +290,14 @@ export function createFileFind(opts: CreateFileFindOptions) { api.delete("opencode-find") api.delete("opencode-find-current") - const active = ranges[currentIndex]?.range + const active = ranges[currentIndex] if (active) api.set("opencode-find-current", new Highlight(active)) - const rest = ranges.flatMap((hit, i) => (i === currentIndex ? [] : [hit.range])) + const rest = ranges.filter((_, i) => i !== currentIndex) if (rest.length > 0) api.set("opencode-find", new Highlight(...rest)) return true } - const select = (currentIndex: number, scroll: boolean) => { - const active = hits[currentIndex]?.range - if (!active) return false - - setIndex(currentIndex) - - if (mode === "highlights") { - if (!setHighlights(hits, currentIndex)) { - mode = "overlay" - apply({ reset: true, scroll }) - return false - } - if (scroll) scrollToRange(active) - return true - } - - clearHighlightFind() - syncOverlayScroll() - if (scroll) scrollToRange(active) - scheduleOverlay() - return true - } - const apply = (args?: { reset?: boolean; scroll?: boolean }) => { if (!open()) return @@ -393,7 +321,7 @@ export function createFileFind(opts: CreateFileFindOptions) { setCount(total) setIndex(currentIndex) - const active = ranges[currentIndex]?.range + const active = ranges[currentIndex] if (mode === "highlights") { clearOverlay() clearOverlayScroll() @@ -420,23 +348,11 @@ export function createFileFind(opts: CreateFileFindOptions) { if (current === host) current = undefined } - const clear = () => { - setQuery("") - clearFind() - } - - const activate = () => { - if (opts.shortcuts !== "disabled") { - if (current && current !== host) current.close() - current = host - target = host - } - - if (!open()) setOpen(true) - } - const focus = () => { - activate() + if (current && current !== host) current.close() + current = host + target = host + if (!open()) setOpen(true) requestAnimationFrame(() => { apply({ scroll: true }) input?.focus() @@ -450,30 +366,25 @@ export function createFileFind(opts: CreateFileFindOptions) { if (total <= 0) return const currentIndex = (index() + dir + total) % total - select(currentIndex, true) - } + setIndex(currentIndex) - const reveal = (targetHit: FileFindReveal) => { - if (!open()) return false - if (hits.length === 0) return false + const active = hits[currentIndex] + if (!active) return - const exact = hits.findIndex( - (hit) => - hit.side === targetHit.side && - hit.line === targetHit.line && - hit.col === targetHit.col && - hit.len === targetHit.len, - ) - const fallback = hits.findIndex( - (hit) => - (hit.line === targetHit.line || hit.alt === targetHit.line) && - hit.col === targetHit.col && - hit.len === targetHit.len, - ) + if (mode === "highlights") { + if (!setHighlights(hits, currentIndex)) { + mode = "overlay" + apply({ reset: true, scroll: true }) + return + } + scrollToRange(active) + return + } - const nextIndex = exact >= 0 ? exact : fallback - if (nextIndex < 0) return false - return select(nextIndex, true) + clearHighlightFind() + syncOverlayScroll() + scrollToRange(active) + scheduleOverlay() } const host: FindHost = { @@ -486,21 +397,17 @@ export function createFileFind(opts: CreateFileFindOptions) { onMount(() => { mode = supportsHighlights() ? "highlights" : "overlay" - if (opts.shortcuts !== "disabled") { - installShortcuts() - hosts.add(host) - if (!target) target = host - } + installShortcuts() + hosts.add(host) + if (!target) target = host onCleanup(() => { - if (opts.shortcuts !== "disabled") { - hosts.delete(host) - if (current === host) { - current = undefined - clearHighlightFind() - } - if (target === host) target = undefined + hosts.delete(host) + if (current === host) { + current = undefined + clearHighlightFind() } + if (target === host) target = undefined }) }) @@ -541,25 +448,20 @@ export function createFileFind(opts: CreateFileFindOptions) { setInput: (el: HTMLInputElement) => { input = el }, - setQuery: (value: string, args?: { scroll?: boolean }) => { + setQuery: (value: string) => { setQuery(value) setIndex(0) - apply({ reset: true, scroll: args?.scroll ?? true }) + apply({ reset: true, scroll: true }) }, - clear, - activate, focus, close, next, - reveal, refresh: (args?: { reset?: boolean; scroll?: boolean }) => apply(args), onPointerDown: () => { - if (opts.shortcuts === "disabled") return target = host opts.wrapper()?.focus({ preventScroll: true }) }, onFocus: () => { - if (opts.shortcuts === "disabled") return target = host }, onInputKeyDown: (event: KeyboardEvent) => { From e7e17dbc904cf8404cf126cc0750ddb932da2eb3 Mon Sep 17 00:00:00 2001 From: Adam <2363879+adamdotdevin@users.noreply.github.com> Date: Wed, 11 Mar 2026 11:17:29 -0500 Subject: [PATCH 2/3] wip: scroll position --- packages/app/src/pages/session.tsx | 141 ++++++------------ packages/ui/src/components/file.tsx | 31 +++- packages/ui/src/components/session-review.tsx | 11 +- 3 files changed, 84 insertions(+), 99 deletions(-) diff --git a/packages/app/src/pages/session.tsx b/packages/app/src/pages/session.tsx index 79c8d42f558..a5c7bf90b32 100644 --- a/packages/app/src/pages/session.tsx +++ b/packages/app/src/pages/session.tsx @@ -862,6 +862,36 @@ export default function Page() {
) + const reviewEmpty = (input: { loadingClass: string; emptyClass: string }) => { + if (store.changes === "turn") return emptyTurn() + + if (hasReview() && !diffsReady()) { + return
{language.t("session.review.loadingChanges")}
+ } + + if (reviewEmptyKey() === "session.review.noVcs") { + return ( +
+
+
Create a Git repository
+
+ Track, review, and undo changes in this project +
+
+ +
+ ) + } + + return ( +
+
{language.t(reviewEmptyKey())}
+
+ ) + } + const reviewContent = (input: { diffStyle: DiffStyle onDiffStyleChange?: (style: DiffStyle) => void @@ -870,98 +900,25 @@ export default function Page() { emptyClass: string }) => ( - - - setTree("reviewScroll", el)} - focusedFile={tree.activeDiff} - onLineComment={(comment) => addCommentToContext({ ...comment, origin: "review" })} - onLineCommentUpdate={updateCommentInContext} - onLineCommentDelete={removeCommentFromContext} - lineCommentActions={reviewCommentActions()} - comments={comments.all()} - focusedComment={comments.focus()} - onFocusedCommentChange={comments.setFocus} - onViewFile={openReviewFile} - classes={input.classes} - /> - - - {language.t("session.review.loadingChanges")}
} - > - setTree("reviewScroll", el)} - focusedFile={tree.activeDiff} - onLineComment={(comment) => addCommentToContext({ ...comment, origin: "review" })} - onLineCommentUpdate={updateCommentInContext} - onLineCommentDelete={removeCommentFromContext} - lineCommentActions={reviewCommentActions()} - comments={comments.all()} - focusedComment={comments.focus()} - onFocusedCommentChange={comments.setFocus} - onViewFile={openReviewFile} - classes={input.classes} - /> - - - - -
-
Create a Git repository
-
- Track, review, and undo changes in this project -
-
- -
- ) : ( -
-
{language.t(reviewEmptyKey())}
-
- ) - } - diffs={reviewDiffs} - view={view} - diffStyle={input.diffStyle} - onDiffStyleChange={input.onDiffStyleChange} - onScrollRef={(el) => setTree("reviewScroll", el)} - focusedFile={tree.activeDiff} - onLineComment={(comment) => addCommentToContext({ ...comment, origin: "review" })} - onLineCommentUpdate={updateCommentInContext} - onLineCommentDelete={removeCommentFromContext} - lineCommentActions={reviewCommentActions()} - comments={comments.all()} - focusedComment={comments.focus()} - onFocusedCommentChange={comments.setFocus} - onViewFile={openReviewFile} - classes={input.classes} - /> - - + setTree("reviewScroll", el)} + focusedFile={tree.activeDiff} + onLineComment={(comment) => addCommentToContext({ ...comment, origin: "review" })} + onLineCommentUpdate={updateCommentInContext} + onLineCommentDelete={removeCommentFromContext} + lineCommentActions={reviewCommentActions()} + comments={comments.all()} + focusedComment={comments.focus()} + onFocusedCommentChange={comments.setFocus} + onViewFile={openReviewFile} + classes={input.classes} + /> ) diff --git a/packages/ui/src/components/file.tsx b/packages/ui/src/components/file.tsx index fa430e286d9..15915dd52d4 100644 --- a/packages/ui/src/components/file.tsx +++ b/packages/ui/src/components/file.tsx @@ -491,6 +491,29 @@ function renderViewer(opts: { opts.onReady() } +function preserve(viewer: Viewer) { + const root = scrollParent(viewer.wrapper) + if (!root) return () => {} + + const high = viewer.container.getBoundingClientRect().height + if (!high) return () => {} + + const top = viewer.wrapper.getBoundingClientRect().top - root.getBoundingClientRect().top + const prev = viewer.container.style.minHeight + viewer.container.style.minHeight = `${Math.ceil(high)}px` + + let done = false + return () => { + if (done) return + done = true + viewer.container.style.minHeight = prev + + const next = viewer.wrapper.getBoundingClientRect().top - root.getBoundingClientRect().top + const delta = next - top + if (delta) root.scrollTop += delta + } +} + function scrollParent(el: HTMLElement): HTMLElement | undefined { let parent = el.parentElement while (parent) { @@ -990,12 +1013,13 @@ function DiffViewer(props: DiffFileProps) { return { ...perf, disableLineNumbers: true } }) - const notify = () => { + const notify = (done?: VoidFunction) => { notifyRendered({ viewer, isReady: (root) => root.querySelector("[data-line]") != null, settleFrames: 1, onReady: () => { + done?.() setSelectedLines(viewer.lastSelection) viewer.find.refresh({ reset: true }) local.onRendered?.() @@ -1016,6 +1040,9 @@ function DiffViewer(props: DiffFileProps) { const virtualizer = virtuals.get() const beforeContents = typeof local.before?.contents === "string" ? local.before.contents : "" const afterContents = typeof local.after?.contents === "string" ? local.after.contents : "" + const done = preserve(viewer) + + onCleanup(done) const cacheKey = (contents: string) => { if (!large()) return sampledChecksum(contents, contents.length) @@ -1040,7 +1067,7 @@ function DiffViewer(props: DiffFileProps) { containerWrapper: viewer.container, }) }, - onReady: notify, + onReady: () => notify(done), }) }) diff --git a/packages/ui/src/components/session-review.tsx b/packages/ui/src/components/session-review.tsx index 61920d9f3f5..49c561e0bdb 100644 --- a/packages/ui/src/components/session-review.tsx +++ b/packages/ui/src/components/session-review.tsx @@ -60,6 +60,8 @@ export type SessionReviewCommentActions = { export type SessionReviewFocus = { file: string; id: string } +type ReviewDiff = FileDiff & { preloaded?: PreloadMultiFileDiffResult } + export interface SessionReviewProps { title?: JSX.Element empty?: JSX.Element @@ -83,7 +85,7 @@ export interface SessionReviewProps { classList?: Record classes?: { root?: string; header?: string; container?: string } actions?: JSX.Element - diffs: (FileDiff & { preloaded?: PreloadMultiFileDiffResult })[] + diffs: ReviewDiff[] onViewFile?: (file: string) => void readFile?: (path: string) => Promise } @@ -146,8 +148,8 @@ export const SessionReview = (props: SessionReviewProps) => { const [opened, setOpened] = createSignal(null) const open = () => props.open ?? store.open - const files = createMemo(() => props.diffs.map((d) => d.file)) - const diffs = createMemo(() => new Map(props.diffs.map((d) => [d.file, d] as const))) + const files = createMemo(() => props.diffs.map((diff) => diff.file)) + const diffs = createMemo(() => new Map(props.diffs.map((diff) => [diff.file, diff] as const))) const diffStyle = () => props.diffStyle ?? (props.split ? "split" : "unified") const hasDiffs = () => files().length > 0 @@ -282,8 +284,7 @@ export const SessionReview = (props: SessionReviewProps) => { {(file) => { let wrapper: HTMLDivElement | undefined - const diff = createMemo(() => diffs().get(file)) - const item = () => diff()! + const item = createMemo(() => diffs().get(file)!) const expanded = createMemo(() => open().includes(file)) const force = () => !!store.force[file] From 112feb620bd4127944186412f33d82e87dce0ced Mon Sep 17 00:00:00 2001 From: Adam <2363879+adamdotdevin@users.noreply.github.com> Date: Wed, 11 Mar 2026 11:42:17 -0500 Subject: [PATCH 3/3] chore: e2e test spec --- .../app/e2e/session/session-review.spec.ts | 186 ++++++++++++++++++ 1 file changed, 186 insertions(+) create mode 100644 packages/app/e2e/session/session-review.spec.ts diff --git a/packages/app/e2e/session/session-review.spec.ts b/packages/app/e2e/session/session-review.spec.ts new file mode 100644 index 00000000000..4198c733c0d --- /dev/null +++ b/packages/app/e2e/session/session-review.spec.ts @@ -0,0 +1,186 @@ +import { waitSessionIdle, withSession } from "../actions" +import { test, expect } from "../fixtures" +import { createSdk } from "../utils" + +const count = 14 + +function body(mark: string) { + return [ + `title ${mark}`, + `mark ${mark}`, + ...Array.from({ length: 32 }, (_, i) => `line ${String(i + 1).padStart(2, "0")} ${mark}`), + ] +} + +function files(tag: string) { + return Array.from({ length: count }, (_, i) => { + const id = String(i).padStart(2, "0") + return { + file: `review-scroll-${id}.txt`, + mark: `${tag}-${id}`, + } + }) +} + +function seed(list: ReturnType) { + const out = ["*** Begin Patch"] + + for (const item of list) { + out.push(`*** Add File: ${item.file}`) + for (const line of body(item.mark)) out.push(`+${line}`) + } + + out.push("*** End Patch") + return out.join("\n") +} + +function edit(file: string, prev: string, next: string) { + return ["*** Begin Patch", `*** Update File: ${file}`, "@@", `-mark ${prev}`, `+mark ${next}`, "*** End Patch"].join( + "\n", + ) +} + +async function patch(sdk: ReturnType, sessionID: string, patchText: string) { + await sdk.session.promptAsync({ + sessionID, + agent: "build", + system: [ + "You are seeding deterministic e2e UI state.", + "Your only valid response is one apply_patch tool call.", + `Use this JSON input: ${JSON.stringify({ patchText })}`, + "Do not call any other tools.", + "Do not output plain text.", + ].join("\n"), + parts: [{ type: "text", text: "Apply the provided patch exactly once." }], + }) + + await waitSessionIdle(sdk, sessionID, 120_000) +} + +async function show(page: Parameters[0]["page"]) { + const btn = page.getByRole("button", { name: "Toggle review" }).first() + await expect(btn).toBeVisible() + if ((await btn.getAttribute("aria-expanded")) !== "true") await btn.click() + await expect(btn).toHaveAttribute("aria-expanded", "true") +} + +async function expand(page: Parameters[0]["page"]) { + const close = page.getByRole("button", { name: /^Collapse all$/i }).first() + const open = await close + .isVisible() + .then((value) => value) + .catch(() => false) + + const btn = page.getByRole("button", { name: /^Expand all$/i }).first() + if (open) { + await close.click() + await expect(btn).toBeVisible() + } + + await expect(btn).toBeVisible() + await btn.click() + await expect(close).toBeVisible() +} + +async function waitMark(page: Parameters[0]["page"], file: string, mark: string) { + await page.waitForFunction( + ({ file, mark }) => { + const head = Array.from(document.querySelectorAll("h3")).find( + (node) => node instanceof HTMLElement && node.textContent?.includes(file), + ) + if (!(head instanceof HTMLElement)) return false + + return Array.from(head.parentElement?.querySelectorAll("diffs-container") ?? []).some((host) => { + if (!(host instanceof HTMLElement)) return false + const root = host.shadowRoot + return root?.textContent?.includes(`mark ${mark}`) ?? false + }) + }, + { file, mark }, + { timeout: 60_000 }, + ) +} + +test("review keeps scroll position after a live diff update", async ({ page, withProject }) => { + test.setTimeout(180_000) + + const tag = `review-${Date.now()}` + const list = files(tag) + const hit = list[list.length - 2]! + const next = `${tag}-live` + + await page.setViewportSize({ width: 1600, height: 1000 }) + + await withProject(async (project) => { + const sdk = createSdk(project.directory) + + await withSession(sdk, `e2e review ${tag}`, async (session) => { + await patch(sdk, session.id, seed(list)) + + await expect + .poll( + async () => { + const info = await sdk.session.get({ sessionID: session.id }).then((res) => res.data) + return info?.summary?.files ?? 0 + }, + { timeout: 60_000 }, + ) + .toBe(list.length) + + await expect + .poll( + async () => { + const diff = await sdk.session.diff({ sessionID: session.id }).then((res) => res.data ?? []) + return diff.length + }, + { timeout: 60_000 }, + ) + .toBe(list.length) + + await project.gotoSession(session.id) + await show(page) + + const tab = page.getByRole("tab", { name: /Review/i }).first() + await expect(tab).toBeVisible() + await tab.click() + + const view = page.locator('[data-slot="session-review-scroll"] .scroll-view__viewport').first() + await expect(view).toBeVisible() + const heads = page.getByRole("heading", { level: 3 }).filter({ hasText: /^review-scroll-/ }) + await expect(heads).toHaveCount(list.length, { + timeout: 60_000, + }) + + await expand(page) + await waitMark(page, hit.file, hit.mark) + + const row = page + .getByRole("heading", { level: 3, name: new RegExp(hit.file.replace(/[.*+?^${}()|[\]\\]/g, "\\$&")) }) + .first() + await expect(row).toBeVisible() + await row.evaluate((el) => el.scrollIntoView({ block: "center" })) + + await expect.poll(() => view.evaluate((el) => el.scrollTop)).toBeGreaterThan(200) + const prev = await view.evaluate((el) => el.scrollTop) + + await patch(sdk, session.id, edit(hit.file, hit.mark, next)) + + await expect + .poll( + async () => { + const diff = await sdk.session.diff({ sessionID: session.id }).then((res) => res.data ?? []) + const item = diff.find((item) => item.file === hit.file) + return typeof item?.after === "string" ? item.after : "" + }, + { timeout: 60_000 }, + ) + .toContain(`mark ${next}`) + + await waitMark(page, hit.file, next) + + await expect + .poll(async () => Math.abs((await view.evaluate((el) => el.scrollTop)) - prev), { timeout: 60_000 }) + .toBeLessThanOrEqual(16) + }) + }) +})